fix(react-router): prevent stale closures in useMatchRoute with React Compiler#6561
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a routerState subscription to the useMatchRoute hook to prevent stale closures, and introduces an end-to-end test suite (React Compiler enabled) plus supporting infra to validate the fix. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/react-router/tests/Matches.test.tsx`:
- Around line 358-439: TestComponent is only mounted on indexRoute (path '/'),
but the test starts at '/home' so TestComponent is never rendered; move
TestComponent to the root layout so it remains mounted across navigations.
Update the route tree by attaching TestComponent as the root route's component
(or create a root layout route whose component is TestComponent) instead of
assigning it to indexRoute; adjust references to indexRoute/rootRoute used in
createRoute and createRouter so matches include TestComponent for routes like
'/home' and '/about'.
|
we should add a new, small e2e test project that has react-compiler enabled and that demonstrates this issue. please do this as a separate commit |
|
Hey @schiller-manuel, thanks for the suggestion, I've added a comprehensive e2e test project in a separate commit: E2E Test Project: e2e/react-router/react-compiler/
Key Discovery: The bug specifically manifests when matchRoute results are consumed in useEffect dependencies: const isHome = !!matchRoute({ to: '/home' })
useEffect(() => {
console.log('isHome', isHome)
}, [isHome]) // ← React Compiler's aggressive memoization creates stale closure hereWithout this useEffect pattern, React Compiler doesn't trigger the problematic memoization. This is why the test checks DOM state directly rather than through useEffect - but the demo app (main.tsx) includes the useEffect pattern to demonstrate the real-world scenario. The e2e tests pass with the fix, confirming the solution works correctly with React Compiler enabled. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@e2e/react-router/react-compiler/package.json`:
- Around line 13-25: The package.json uses workspace:^ for internal packages
which violates the guideline; update the dependency entries for
"@tanstack/react-router" and the devDependency "@tanstack/router-e2e-utils" to
use "workspace:*" instead of "workspace:^" so they are consistently pinned
across the workspace (modify the dependencies/devDependencies blocks in
package.json where those package names appear).
🧹 Nitpick comments (2)
e2e/react-router/react-compiler/src/main.tsx (2)
30-33: IncludeisAboutin the effect deps (or document the intentional omission).The effect reads
isAboutbut doesn’t depend on it, which can trigger exhaustive‑deps lint or lead to stale logs. If the omission is intentional for the repro, add an inline eslint disable with a brief rationale; otherwise, includeisAbout.Suggested fix
- }, [isHome]) + }, [isHome, isAbout])
127-132: Avoid non‑null assertion to keep strict type safety.
document.getElementById('app')!bypasses strict checks; a guard keeps failures explicit and preserves type safety.Suggested fix
-const rootElement = document.getElementById('app')! +const rootElement = document.getElementById('app') +if (!rootElement) { + throw new Error('Expected `#app` element to exist') +}As per coding guidelines:
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety for all code.
kamalbennani
force-pushed
the
fix/react-compiler-use-match-route-stale-closure
branch
from
addd879 to
75a12b0
Compare
|
is this a react compiler bug that we are working around here? |
|
No, this is not a React Compiler bug - this is us fixing improper hook dependency management in our code. The Problem: Our useMatchRoute hook was using useCallback with only the router reference as a dependency: return React.useCallback((opts) => {
return router.matchRoute(opts) // reads router.latestLocation, router.state
}, [router]) // ❌ router reference is stable, but its internal state is mutableThe router reference never changes, but the callback reads mutable internal state (router.latestLocation, router.state.resolvedLocation, etc.). This violates React's rules - we're depending on values that aren't in the dependency array. Why It Worked Before: When navigation occurs, useRouterState triggers a re-render of the component. If the callback is called during render, it works fine because it reads the router's current internal state: // This works - callback called during render
return <div>{matchRoute({ to: '/home' }) ? 'Active' : 'Inactive'}</div>However, if the callback result is used in a useEffect, that effect won't re-run after navigation because the callback reference hasn't changed, even though the component re-rendered: // This breaks - useEffect doesn't see the callback reference change
useEffect(() => {
const isHome = matchRoute({ to: '/home' })
if (isHome) {
doSomething()
}
}, [matchRoute]) // ❌ Effect won't re-run after navigationWhy React Compiler Exposes It: React Compiler enforces proper memoization semantics. When it sees useCallback([router]) with a stable reference, it assumes the callback never needs to update. This exposes our improper dependency management. The Fix: We now explicitly subscribe to the router state and include it in dependencies: const routerState = useRouterState({
select: (s) => [s.location.href, s.resolvedLocation?.href, s.status]
})
return React.useCallback((opts) => {
return router.matchRoute(opts)
}, [router, routerState]) // ✅ Explicit dependencies on values that actually changeThis is the correct pattern. As the React team moves toward requiring proper memoization (removing the burden from developers to manually optimize), our libraries need to follow best practices. This fix ensures our code works correctly with React Compiler and future React versions. |
|
Hey @schiller-manuel, if you have any further comments, I'd love to address them. |
kamalbennani
force-pushed
the
fix/react-compiler-use-match-route-stale-closure
branch
from
75a12b0 to
a724bfe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@e2e/react-router/react-compiler/package.json`:
- Around line 24-25: Update the dependency version for
babel-plugin-react-compiler in package.json from "^1.0.0" to the current release
(e.g., "19.1.0-rc.3"); locate the dependency entry for
"babel-plugin-react-compiler" and replace its version string so the project uses
the up-to-date package while leaving "vite" unchanged.
|
This would be great to have, it's the only place I have react compiler disabled via "use no memo" right now. |
… Compiler React Compiler's aggressive memoization can cause useMatchRoute to return stale results after navigation. The hook's useCallback only depended on the stable router reference, but the callback reads mutable router internal state (location, resolvedLocation, status). This fix: - Captures router state explicitly in a variable - Includes routerState in useCallback dependencies - Ensures the callback is recreated when navigation occurs Adds test demonstrating the issue: - Navigate from /home to /about - Call matchRoute to check current location - Verify it correctly identifies the new route Without this fix, React Compiler would memoize the callback with stale router state, causing incorrect match results. Fixes: TanStack#4499 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…er test Mount TestComponent on root route instead of index route to ensure it remains mounted during navigation. The test starts at /home but was mounting TestComponent on the index route (/), causing it to never render. Now TestComponent is properly mounted as the root component with an Outlet, so it stays mounted when navigating between /home and /about routes. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Creates a new e2e test project that demonstrates and verifies the useMatchRoute fix works correctly with React Compiler enabled. The test project: - Enables React Compiler via babel-plugin-react-compiler in vite config - Creates a minimal app using useMatchRoute to check current route - Tests navigation between /home and /about routes - Verifies matchRoute returns correct results after navigation - Checks active link highlighting based on match status Without the fix (with React Compiler enabled but without routerState in dependencies), the tests would fail because matchRoute would return stale results captured at hook initialization. With the fix, all tests pass because the callback properly updates when navigation occurs. This addresses the maintainer request for an e2e test that demonstrates the issue with React Compiler enabled. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The test is now covered by the e2e test in the react-compiler example app. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
kamalbennani
force-pushed
the
fix/react-compiler-use-match-route-stale-closure
branch
from
a724bfe to
00830c9
Compare
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/react-router/react-compiler/src/main.tsx`:
- Around line 92-112: The inline anonymous components passed as the component
prop to createRoute for homeRoute and aboutRoute should be extracted to named
React components to avoid creating new component references on every render;
define e.g. HomePage and AboutPage functions and replace the component: () =>
(...) entries with component: HomePage and component: AboutPage respectively,
keeping the same JSX content and data-testid attributes so behavior and tests
remain unchanged.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-router/react-compiler/src/main.tsx`: - Around line 92-112: The inline anonymous components passed as the component prop to createRoute for homeRoute and aboutRoute should be extracted to named React components to avoid creating new component references on every render; define e.g. HomePage and AboutPage functions and replace the component: () => (...) entries with component: HomePage and component: AboutPage respectively, keeping the same JSX content and data-testid attributes so behavior and tests remain unchanged.e2e/react-router/react-compiler/src/main.tsx (1)
92-112: Consider extracting inline route components to named functions.The anonymous arrow functions passed as
componenttocreateRoutewill create new component references on every render cycle, which can cause unnecessary unmount/remount. While this is fine for an e2e test, using named components is more idiomatic with TanStack Router and avoids potential issues if React Compiler applies additional optimizations to these components.♻️ Optional: extract to named components
+function HomeComponent() { + return ( + <div data-testid="content-home"> + <h3>Home Page</h3> + <p>Welcome to the home page!</p> + </div> + ) +} + +function AboutComponent() { + return ( + <div data-testid="content-about"> + <h3>About Page</h3> + <p>This is the about page.</p> + </div> + ) +} + const homeRoute = createRoute({ getParentRoute: () => rootRoute, path: '/home', - component: () => ( - <div data-testid="content-home"> - <h3>Home Page</h3> - <p>Welcome to the home page!</p> - </div> - ), + component: HomeComponent, }) const aboutRoute = createRoute({ getParentRoute: () => rootRoute, path: '/about', - component: () => ( - <div data-testid="content-about"> - <h3>About Page</h3> - <p>This is the about page.</p> - </div> - ), + component: AboutComponent, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-router/react-compiler/src/main.tsx` around lines 92 - 112, The inline anonymous components passed as the component prop to createRoute for homeRoute and aboutRoute should be extracted to named React components to avoid creating new component references on every render; define e.g. HomePage and AboutPage functions and replace the component: () => (...) entries with component: HomePage and component: AboutPage respectively, keeping the same JSX content and data-testid attributes so behavior and tests remain unchanged.
|
Hey @schiller-manuel @tannerlinsley, I know you guys have a lot to deal with, but I was wondering if there is anything I could do to get this PR approved? |
4 participants
Problem
React Compiler's aggressive memoization causes
useMatchRouteto return stale results after navigation (issue #4499). The hook usesuseCallbackwith only the stablerouterreference as a dependency, but the callback reads mutable internal router state (location,resolvedLocation,status).When React Compiler optimizes the code, it memoizes the callback with captured stale state, preventing it from seeing navigation changes.
The bug specifically manifests when
matchRouteresults are consumed inuseEffectdependencies:Without the useEffect pattern, React Compiler doesn't trigger the problematic memoization, which is why some tests may pass without the fix. However, consuming
matchRouteresults inuseEffectis a common real-world pattern, making this fix essential.Solution
This PR fixes the issue by:
useRouterStatein a variablerouterStatein theuseCallbackdependency arrayTest Added
Added a comprehensive e2e test project (
e2e/react-router/react-compiler/) that:babel-plugin-react-compilerThe e2e test includes console logging to capture the behavior when the bug would manifest.
Related Issue
Fixes #4499
Breaking Changes
None - This is a bug fix that maintains the same public API.
Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores