Skip to content

Conversation

daibhin
Copy link
Contributor

@daibhin daibhin commented Oct 17, 2025

Problem

Addresses all the feedback in https://posthog.slack.com/archives/C07AA937K9A/p1759420827044679

Changes

  • Do not re-run query if all events are removed
  • All persistent state to url

Copy link
Contributor

Size Change: 0 B

Total Size: 3.3 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 3.3 MB

compressed-size-action

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Comment on lines +128 to +130
if (params.events && !equal(params.events, values.events)) {
actions.setEvents(params.events)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: potential desync if URL parameter is manually removed

the condition only checks if params.events exists, so if a user has events selected and then manually removes the events param from the URL, the state won't reset to null. consider handling the undefined case:

Suggested change
if (params.events && !equal(params.events, values.events)) {
actions.setEvents(params.events)
}
if (!equal(params.events ?? null, values.events)) {
actions.setEvents(params.events ?? null)
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/error_tracking/frontend/scenes/ErrorTrackingScene/tabs/impact/errorTrackingImpactListLogic.tsx
Line: 128:130

Comment:
**style:** potential desync if URL parameter is manually removed

the condition only checks if `params.events` exists, so if a user has events selected and then manually removes the `events` param from the URL, the state won't reset to `null`. consider handling the undefined case:

```suggestion
            if (!equal(params.events ?? null, values.events)) {
                actions.setEvents(params.events ?? null)
            }
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +137 to +139
if (params.activeTab && !equal(params.activeTab, values.activeTab)) {
actions.setActiveTab(params.activeTab)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: potential desync if URL parameter is manually removed

the condition only checks if params.activeTab exists, so if someone manually removes the activeTab param from the URL, the state won't reset to the default. consider handling the undefined case:

Suggested change
if (params.activeTab && !equal(params.activeTab, values.activeTab)) {
actions.setActiveTab(params.activeTab)
}
if (!equal(params.activeTab ?? DEFAULT_ACTIVE_TAB, values.activeTab)) {
actions.setActiveTab(params.activeTab ?? DEFAULT_ACTIVE_TAB)
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/error_tracking/frontend/scenes/ErrorTrackingScene/errorTrackingSceneLogic.ts
Line: 137:139

Comment:
**style:** potential desync if URL parameter is manually removed

the condition only checks if `params.activeTab` exists, so if someone manually removes the `activeTab` param from the URL, the state won't reset to the default. consider handling the undefined case:

```suggestion
            if (!equal(params.activeTab ?? DEFAULT_ACTIVE_TAB, values.activeTab)) {
                actions.setActiveTab(params.activeTab ?? DEFAULT_ACTIVE_TAB)
            }
```

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant