Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(flags): prevent kea logic loop and clean filters on tab switch #26258

Merged
merged 3 commits into from
Nov 19, 2024

Conversation

havenbarnes
Copy link
Contributor

@havenbarnes havenbarnes commented Nov 18, 2024

Problem

Issue resolved by #26257 re-occurred, after investigating I found that a fresh navigation to the FF UI still exhibits the Maximum call stack size exceeded error here:
https://posthog.sentry.io/issues/6063840317/?notification_uuid=65ceb7bd-8e81-49e2-bb7b-956fddd59861&project=1899813&referrer=regression_activity-email

Changes

  • Fixed issue where a fresh navigation to Feature flags can still have the maximum depth exceeded error
  • Cleaned up page search param being carried over between tab switches

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

https://www.loom.com/share/b5e4c76aba3e4d63b0218e96d51da944

if (page !== undefined) {
pageFiltersFromUrl.page = parseInt(page)
}
pageFiltersFromUrl.active = active ? String(active) : undefined
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the primary bug fix, it keeps the comparison with DEFAULT_FILTERS a few lines down stable regardless of present search params

Copy link
Contributor

github-actions bot commented Nov 18, 2024

Size Change: 0 B

Total Size: 1.16 MB

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

compressed-size-action

* so let's update filter state if it changes
*/
const isChangingPage = page !== undefined && page !== values.filters.page
if (isInitializingFilters || isChangingPage) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These checks were previously preventing a loop, but aren't needed; the new diff

setActiveTab: () => {
            // Don't carry over pagination from previous tab
            actions.setFeatureFlagsFilters({ page: 1 }, true)
        },

prevents this loop by resetting the filters properly. Not having this check ensures we (properly) reload the table whenever we navigate back to the features list

@havenbarnes havenbarnes enabled auto-merge (squash) November 19, 2024 01:08
@havenbarnes havenbarnes merged commit a41bd2d into master Nov 19, 2024
96 checks passed
@havenbarnes havenbarnes deleted the fix/ff-toggle-tabs-loop-2 branch November 19, 2024 01:17
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.

2 participants