Conversation
IzumiSy
left a comment
There was a problem hiding this comment.
Review
The intent and the implementation are aligned and correct overall.
What the PR is doing
The change tightens the useMemo boundary in RouterContainer so that the router is only recreated when the route tree actually changes. To achieve that, children (the shell element) and wrapComponent (the auth guard wrapper) are moved out of the route definition path. Both values are now delivered to the root route element at render time via a local React context (RootRouteContext) instead of being baked into the RouteObject at creation time.
Why the approach is sound
RouteObject.element in React Router is resolved at render time, not at router-creation time — it is just a ReactNode. Putting render-time values (shell children, guard wrapper) into useMemo deps caused the router to be recreated on ordinary React rerenders that had nothing to do with route structure. The fix correctly recognises this boundary.
Key prerequisite — authLoader stability
The updated useMemo deps are [configurations, rootLoader, contentRoutes]. For this to fully solve the problem, rootLoader (i.e. authLoader from AuthProvider) must be referentially stable across rerenders that only change guardComponent.
I checked auth-context.tsx and confirmed this holds:
// auth-context.tsx
const authLoader = useCallback(
async (requestUrl: URL) => { ... },
[client], // stable as long as client prop doesn't change
);
const rootRouteCtxValue = useMemo(
() => ({ loader: authLoader, wrapComponent: guardComponentWrapper }),
[authLoader, guardComponentWrapper],
);authLoader depends only on client, which is expected to be module-level stable. When guardComponent changes, guardComponentWrapper changes but authLoader does not, so rootLoader stays the same reference and the router is not recreated. The regression tests verify exactly this scenario (does not recreate the router when guard wrapper changes), so the invariant is covered.
No concerns. The implementation is correct and the tests cover the intended invariants.
Summary
useMemo-based router creation approachBackground
This PR is a follow-up to the auth router stability work in #184.
That earlier change made
RouterContainerstable across auth-driven rerenders so the root loader would not re-process the same OAuth callback URL. The remaining concern was that the router was still being built from a mix of two different kinds of inputs:Those render-time values are not part of the route structure itself. If they participate in router creation, then ordinary UI rerenders can still recreate the router instance even when the route definition has not meaningfully changed.
For this codebase, I did not want to introduce an external router factory API. The goal here is to keep the current design, but tighten the boundary around what actually counts as a router-defining change.
Purpose
The purpose of this change is to preserve the current
RouterContainerAPI while making its memoization semantics more intentional.Concretely, the router should be recreated when the route tree or loader behavior changes, but not when:
To achieve that, this PR keeps the root loader in the route definition path, but moves shell rendering and wrapper application into a separate internal React context that is consumed at render time.
Result
With this change: