Fix auth router initialization stability#184
Conversation
commit: |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This reverts commit 4bd795a.
… up EnhancedAuthClient interface - useEnsureAuthInitialized now accepts callbackStatus as a parameter and only skips initialization while the callback is 'pending'; a 'rejected' status falls through so checkAuthStatus can still resolve the session - getCallbackStatusSnapshot and subscribeCallbackStatus made optional on EnhancedAuthClient so consumers who mock or implement the interface do not need to provide internal callback-tracking methods - useCallbackStatus uses optional chaining with 'idle' fallback for clients that do not supply the optional methods - update createAuthClient JSDoc to document the OAuth callback side effect - update guardComponent JSDoc to reflect that AuthProvider renders the guard directly (no router dependency) - add tests: rejected callback triggers checkAuthStatus, createAuthClient calls handleCallback on OAuth URL and skips it on plain URLs
|
/review |
|
No API changes, so API design review emits noop. Good to go. |
interacsean
left a comment
interacsean
left a comment
There was a problem hiding this comment.
I think good @IzumiSy - a couple of AI review notes, but nothing a blocker
| @@ -81,13 +151,41 @@ export interface EnhancedAuthClient extends AuthClient { | |||
| export function createAuthClient(config: AuthClientConfig): EnhancedAuthClient { | |||
There was a problem hiding this comment.
Feedback from Claude:
Side effect inside createAuthClient (auth-context.tsx:151-170). Constructing a client now fires a network request if the URL has ?code/?error. The docstring warns about it, but this makes the factory non-idempotent — two calls during HMR, StrictMode double-invoke, or a misconfigured test setup would each attempt a single-use code exchange. Consider either (a) gating with a module-level "already started" flag keyed on the URL, or (b) moving the auto-start into a dedicated startCallbackIfPending() method the consumer calls explicitly. At minimum, a StrictMode test proving it's safe would be reassuring.
I think it's probably ok to let the caller manage only calling this function once.
However, it's probably fair and valid to try to be protective of misuse, given this is a library and we want to minimize errors on the consumer-side, even if they are from incorrect usage. So thought I will mention it anyway
There was a problem hiding this comment.
The StrictMode concern doesn't apply here — createAuthClient is designed to be called at module scope, outside the React lifecycle entirely. StrictMode's double-invocation only affects components, hooks, and effects, not module-level code. For HMR, a developer actively editing source files while sitting on an OAuth callback URL (?code=...) isn't a realistic scenario in practice. The JSDoc already documents this as an intentional side effect that runs before any React render. No change planned for this.
| }), | ||
| [configurations.modules, configurations.settingsResources, rootComponent, props.rootGuards], | ||
| ); | ||
| const routes = useMemo( |
There was a problem hiding this comment.
Claude review:
routes memo depends on children (router.tsx:87-99). The useMemo for routes (and transitively router) includes children in its deps. If callers pass a fresh JSX tree each render (common — e.g. ), the memo never hits, defeating the stated stability goal. The stability assertion in router.test.tsx uses
Homeinline in renderWithConfig, which is a stable reference within a single render but not across auth-driven reparenting. Worth verifying in an integration-level test that simulates an auth state change while children is a fresh fragment.
I'm unsure if this is valid based on usage
There was a problem hiding this comment.
This is a valid concern. In practice, children passed to RouterContainer from AppShell includes <BuiltInCommandPalette /> which is a fresh JSX reference every render, so the routes memo never hits and the router ends up being recreated more than intended. This is being addressed in #185, which moves shell rendering and the auth guard wrapper out of the route definition path into a separate internal React context consumed at render time. That PR is a direct follow-up to this one.
| children, | ||
| }), | ||
| ] satisfies Array<RouteObject>, | ||
| [configurations, contentRoutes, children], |
There was a problem hiding this comment.
configurations could also easily not be memoized and therefore bypass the impact of useMemo.
Context values are only stable if the provider memoizes them. useContext returns whatever was last passed to
I don't think there's anything efficient we can do about it if the consumer is passing in a fresh object every time
There was a problem hiding this comment.
In the current architecture, configurations is stable in practice because AppShell already memoizes the context value via useMemo before passing it to AppShellConfigContext.Provider. Any consumer going through the intended API receives a stable reference. Agreed there's no efficient fix if someone constructs a fresh object and passes it directly to the Provider, but that's outside the supported usage pattern.
2 participants
Summary
AuthProvider@tailor-platform/app-shellBackground
This branch fixes an auth flow bug around the OAuth redirect path.
Two problems were overlapping:
The fix moves auth ownership into
AuthProviderand makes callback progress explicit.AuthProvidernow owns the normal mount-time auth check, skips that path while the current URL is still an OAuth callback, and reads a dedicated callback-status store to decide when unguarded children should render. Guarded trees continue to rely on auth state, but callback handling is coordinated by the same provider-level flow.On the router side, the router instance is memoized so auth state changes do not recreate it for the same route configuration. The old root-route auth context is removed, leaving the router responsible only for navigation loading, route creation, and rendering.
Together, these changes make the post-login path deterministic and remove the callback/initialization races that were leaving the app in an inconsistent state.