Skip to content

ref(settings): Replace billing navigation config with a react-hook#115808

Merged
evanpurkhiser merged 1 commit into
masterfrom
evanpurkhiser/ref-settings-replace-billing-navigation-config-with-a-react-hook
May 19, 2026
Merged

ref(settings): Replace billing navigation config with a react-hook#115808
evanpurkhiser merged 1 commit into
masterfrom
evanpurkhiser/ref-settings-replace-billing-navigation-config-with-a-react-hook

Conversation

@evanpurkhiser
Copy link
Copy Markdown
Member

GSBillingNavigationConfig was a null-rendering class component whose only job was to register settings:organization-navigation-config on HookStore in componentDidMount, after subscription data became available via withSubscription. This meant OrganizationSettingsNavigation had to listen to HookStore for late registrations — an awkward side channel.

This PR replaces the whole mechanism with a react-hook:use-billing-navigation-config hook registered statically at app startup. useBillingNavigationConfig reads from SubscriptionStore via useLegacyStore, so reactivity comes from React state rather than HookStore. OrganizationSettingsNavigation becomes a simple functional component with no store subscriptions.

The settings:organization-navigation and settings:organization-navigation-config hook names are removed entirely.

@evanpurkhiser evanpurkhiser requested review from a team as code owners May 19, 2026 15:50
@evanpurkhiser evanpurkhiser requested review from billyvg, joshuarli and markstory and removed request for a team May 19, 2026 15:50
@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label May 19, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 19, 2026

📊 Type Coverage Diff

✅ No new type safety issues introduced. Coverage: 93.56%

Comment on lines 89 to 95
});

setFuzzy(search);
}, [organization, project, searchOptions]);
}, [billingNavConfig, organization, project, searchOptions]);

useEffect(() => void createSearch(), [createSearch]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: The createSearch callback depends on billingNavConfig, which is a new object on every render, causing an infinite render loop when billing data is present.
Severity: HIGH

Suggested Fix

Memoize the result of buildBillingNavigationConfig within the useBillingNavigationConfig hook, likely using useMemo. This will provide a stable object reference for billingNavConfig across re-renders, breaking the infinite loop.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: static/app/components/search/sources/routeSource.tsx#L89-L95

Potential issue: An infinite render loop occurs when subscription data is loaded. The
`useBillingNavigationConfig` hook returns a `billingNavConfig` object that is newly
created on every render. This unstable object reference is used as a dependency in a
`useCallback` hook for the `createSearch` function. Consequently, `createSearch` is
recreated on every render, triggering a `useEffect` that updates component state with a
new `Fuse` instance. This state update causes a re-render, completing the infinite loop
and freezing the component.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This does sort of sound like a problem lol

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's not actually infinite rendering in practice, but let me wrap in a use memo

@evanpurkhiser evanpurkhiser requested a review from a team May 19, 2026 15:54
Comment thread static/gsApp/hooks/useBillingNavigationConfig.tsx
GSBillingNavigationConfig was a null-rendering component whose only
purpose was to register 'settings:organization-navigation-config' on
HookStore after subscription data became available via componentDidMount.
This required OrganizationSettingsNavigation to listen to HookStore for
late registrations — an awkward workaround.

Replace the whole mechanism with a proper react-hook registered
statically at app startup. useBillingNavigationConfig reads from
SubscriptionStore via useLegacyStore, so reactivity comes from React
state rather than HookStore. OrganizationSettingsNavigation becomes a
simple functional component.

Removes the 'settings:organization-navigation' and
'settings:organization-navigation-config' hook names entirely.
@evanpurkhiser evanpurkhiser force-pushed the evanpurkhiser/ref-settings-replace-billing-navigation-config-with-a-react-hook branch from 893d1ca to 8bcb9a5 Compare May 19, 2026 15:57
@evanpurkhiser evanpurkhiser enabled auto-merge (squash) May 19, 2026 16:02
@evanpurkhiser evanpurkhiser merged commit b4f07e8 into master May 19, 2026
71 checks passed
@evanpurkhiser evanpurkhiser deleted the evanpurkhiser/ref-settings-replace-billing-navigation-config-with-a-react-hook branch May 19, 2026 16:04
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 8bcb9a5. Configure here.

const project = useProjectFromSlug({organization, projectSlug: params.projectId});
const useBillingNavConfig =
HookStore.get('react-hook:use-billing-navigation-config') ?? (() => null);
const billingNavConfig = useBillingNavConfig();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unstable hook return causes infinite render loop

Medium Severity

The useBillingNavConfig call on every render returns a new object when the registered hook is a plain function (as in the test mock at line 38 of the spec). Since billingNavConfig is included in the useCallback dependency array for createSearch, a new reference each render recreates the callback, re-triggers the useEffect, which calls setFuzzy with a new Fuse instance, causing another render — producing an infinite async render loop. The production hook (useBillingNavigationConfig) mitigates this via useMemo, but the useMemo deps include organization and subscription, which could also become referentially unstable. The PR discussion acknowledges this ("let me wrap in a use memo") but the fix isn't present in this diff.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 8bcb9a5. Configure here.

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

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants