fix(react-router): prevent stale closures in useMatchRoute with React Compiler#6561
Conversation
… 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>
📝 WalkthroughWalkthroughSubscribes useMatchRoute to router state by adding a useRouterState selector and including routerState in useCallback deps; removes an existing unit test file and adds a React-Compiler e2e project with Playwright tests and supporting config/files to validate the behavior across navigations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
…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>
|
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.
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
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. |
3 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
Tests
Chores