fix(router-core): propagate beforeLoad context to sub-route while its loader reloads in the background#7603
Conversation
… loader reloads in the background
📝 WalkthroughWalkthroughThis PR fixes a bug where context values set by a parent route's ChangesContext Propagation in Background Reloads
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
View your CI Pipeline Execution ↗ for commit 92da7bf
☁️ Nx Cloud last updated this comment at |
Merging this PR will degrade performance by 3.79%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | client-side navigation loop (react) |
55.2 ms | 58.1 ms | -4.99% |
| ❌ | Simulation | ssr control-flow unmatched 404 (react) |
39.9 ms | 41.2 ms | -3.31% |
| ❌ | Simulation | ssr control-flow route headers (react) |
25.2 ms | 26 ms | -3.07% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ulrichstark:fix(router-core)--propagate-beforeLoad-context-to-sub-route-while-its-loader-reloads-in-the-background (92da7bf) with main (a2b9d51)
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-router/tests/routeContext.test.tsx`:
- Around line 2799-2859: The test currently only asserts the final rendered text
after loader completion, so it doesn't prove context propagation during an
in-flight background reload; change the test (the one named "context value from
beforeLoad..." using contextPropagationRoute, contextPropagationIndexRoute,
beforeLoad, loader and useRouteContext) to assert the route context value while
the second loader run is still pending: make the loader delay (already uses
sleep), start the navigation that triggers the background reload, then assert
immediately (without awaiting loader completion or findByText that waits) that
the component still reads number === 42 (e.g., use a non-awaiting query/get or
waitFor with a short timeout to capture the interim render) before letting the
loader finish so the test verifies context propagation during the in-flight
reload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76bc0d76-5649-4338-99ba-29a16650b706
📒 Files selected for processing (1)
packages/react-router/tests/routeContext.test.tsx
| test('context value from beforeLoad is propagated to a sub-route while its loader reloads in the background', async () => { | ||
| let sawUndefinedContext = false | ||
|
|
||
| const rootRoute = createRootRoute({ | ||
| component: () => <Outlet />, | ||
| }) | ||
| const homeRoute = createRoute({ | ||
| getParentRoute: () => rootRoute, | ||
| path: '/', | ||
| component: () => <div>Home page</div>, | ||
| }) | ||
| const contextPropagationRoute = createRoute({ | ||
| getParentRoute: () => rootRoute, | ||
| path: '/context-propagation', | ||
| beforeLoad: () => ({ number: 42 }), | ||
| component: () => <Outlet />, | ||
| }) | ||
| const contextPropagationIndexRoute = createRoute({ | ||
| getParentRoute: () => contextPropagationRoute, | ||
| path: '/', | ||
| staleTime: 0, | ||
| loader: async () => { | ||
| await sleep(WAIT_TIME) | ||
| }, | ||
| component: () => { | ||
| const { number } = contextPropagationIndexRoute.useRouteContext() | ||
| sawUndefinedContext ||= number === undefined | ||
|
|
||
| return ( | ||
| <div> | ||
| number = {String(number)}, saw undefined ={' '} | ||
| {String(sawUndefinedContext)} | ||
| </div> | ||
| ) | ||
| }, | ||
| }) | ||
|
|
||
| const routeTree = rootRoute.addChildren([ | ||
| homeRoute, | ||
| contextPropagationRoute.addChildren([contextPropagationIndexRoute]), | ||
| ]) | ||
| const router = createRouter({ routeTree, history }) | ||
|
|
||
| render(<RouterProvider router={router} />) | ||
|
|
||
| await act(() => router.navigate({ to: '/context-propagation' })) | ||
|
|
||
| expect( | ||
| await screen.findByText('number = 42, saw undefined = false'), | ||
| ).toBeInTheDocument() | ||
|
|
||
| await act(() => router.navigate({ to: '/' })) | ||
|
|
||
| expect(await screen.findByText('Home page')).toBeInTheDocument() | ||
|
|
||
| act(() => router.history.back()) | ||
|
|
||
| expect( | ||
| await screen.findByText('number = 42, saw undefined = false'), | ||
| ).toBeInTheDocument() | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The regression test does not yet prove the in-flight background reload scenario.
Current checks validate the final state after findByText(...), so this can pass even if render waits for loader completion (or if background reload behavior regresses). The test should assert context while the second loader run is actively pending.
Suggested tightening
import {
act,
cleanup,
configure,
fireEvent,
render,
screen,
+ waitFor,
} from '`@testing-library/react`'
@@
test('context value from beforeLoad is propagated to a sub-route while its loader reloads in the background', async () => {
let sawUndefinedContext = false
+ let loaderCalls = 0
+ let releaseSecondLoad!: () => void
+ const secondLoadStarted = vi.fn()
+ const secondLoadGate = new Promise<void>((resolve) => {
+ releaseSecondLoad = resolve
+ })
@@
staleTime: 0,
loader: async () => {
- await sleep(WAIT_TIME)
+ loaderCalls += 1
+ if (loaderCalls === 2) {
+ secondLoadStarted()
+ await secondLoadGate
+ }
},
@@
act(() => router.history.back())
+ await waitFor(() => expect(secondLoadStarted).toHaveBeenCalledOnce())
expect(
- await screen.findByText('number = 42, saw undefined = false'),
+ screen.getByText('number = 42, saw undefined = false'),
).toBeInTheDocument()
+ releaseSecondLoad()
})Based on learnings from the PR objectives and review stack context, this test should explicitly validate context propagation during the background reload window.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/react-router/tests/routeContext.test.tsx` around lines 2799 - 2859,
The test currently only asserts the final rendered text after loader completion,
so it doesn't prove context propagation during an in-flight background reload;
change the test (the one named "context value from beforeLoad..." using
contextPropagationRoute, contextPropagationIndexRoute, beforeLoad, loader and
useRouteContext) to assert the route context value while the second loader run
is still pending: make the loader delay (already uses sleep), start the
navigation that triggers the background reload, then assert immediately (without
awaiting loader completion or findByText that waits) that the component still
reads number === 42 (e.g., use a non-awaiting query/get or waitFor with a short
timeout to capture the interim render) before letting the loader finish so the
test verifies context propagation during the in-flight reload.
1 participant
Fixes #7602
Summary by CodeRabbit
Bug Fixes
Tests