fix(router-core): on navigate (with preload), route component isn't rendered with undefined context if beforeLoad is pending #5002
Conversation
…endered with undefined context if beforeLoad is pending
WalkthroughAdds a reproducer test for preload/pending route context behavior and updates router-core to include a route's prior __beforeLoadContext when constructing context passed to beforeLoad. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Router
participant Matcher as loadMatches/executeBeforeLoad
participant Route as Route.beforeLoad
participant View as Route Component
participant Sel as useRouteContext(selector)
App->>Router: intent preload to '/foo'
Router->>Matcher: preload('/foo')
Note over Matcher: Build context = __beforeLoadContext + parentMatchContext + routeContext
Matcher->>Route: beforeLoad(cause: 'preload', preload: true)
Route-->>Matcher: (delayed) resolves context -> store __beforeLoadContext
Router->>View: (pending) show pendingComponent — route component not rendered with empty context
App->>Router: navigate/enter '/foo'
Router->>Matcher: enter('/foo')
Matcher->>Route: beforeLoad(cause: 'enter', preload: false)
Route-->>Matcher: resolves context (updated)
Matcher-->>Router: finalize match context
Router->>View: render Route Component
View->>Sel: useRouteContext(selector)
Sel-->>View: selector receives final context (e.g., foo: 2)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/router-core/src/load-matches.ts (1)
372-372: Precedence and safety nits: confirm desired override order and guard against undefined spreadsTwo minor points to consider:
- Precedence: With
{ ...__beforeLoadContext, ...parent, ...__routeContext }, the route’s static__routeContextoverrides any overlapping keys from the previously resolved__beforeLoadContext. During an enter whenbeforeLoadis pending, this could temporarily “hide” values derived during preload if keys overlap. If the intent is for previously resolvedbeforeLoadvalues to take precedence while a newbeforeLoadis pending, consider placing__beforeLoadContextlast:
{ ...parentMatchContext, ...match.__routeContext, ...match.__beforeLoadContext }
This is not a blocker, but worth verifying with an explicit test for overlapping keys.- Safety: While these are usually objects in practice, using nullish fallbacks makes the spread robust and explicit.
Suggested change for both clarity and robustness:
- const context = { ...match.__beforeLoadContext, ...parentMatchContext, ...match.__routeContext } + // Prefer explicit fallbacks to avoid spreading undefined/null. + // If you want previously computed beforeLoad values to win during pending, + // flip the order and spread __beforeLoadContext last instead. + const context = { + ...(match.__beforeLoadContext ?? {}), + ...(parentMatchContext ?? {}), + ...(match.__routeContext ?? {}), + }If you do want
__beforeLoadContextto win while pending, apply this alternative ordering:- const context = { - ...(match.__beforeLoadContext ?? {}), - ...(parentMatchContext ?? {}), - ...(match.__routeContext ?? {}), - } + const context = { + ...(parentMatchContext ?? {}), + ...(match.__routeContext ?? {}), + ...(match.__beforeLoadContext ?? {}), + }Would you like me to add a focused test that asserts precedence when
context()andbeforeLoad()both define the same key (e.g., “title”) across preload→enter?packages/react-router/tests/routeContext.test.tsx (1)
3129-3211: Solid repro for #4998; asserts no empty-context renders and correct preload→enter flowThis test effectively locks in the intended behavior:
- preload
beforeLoadruns and resolves first- navigation triggers
beforeLoadagain withcause: 'enter'- the route component never renders with
{}; it renders first with the preloaded context and then updates.Two small stability nits (optional):
- Preload on focus can be async; consider
waitForfor the initial “calledTimes(1)” assertion to reduce timing flakiness.- You can further harden the “no empty-context render” guarantee by asserting the component isn’t present before click.
Example refinements:
@@ - fireEvent.focus(linkToFoo) - expect(beforeLoad).toHaveBeenCalledTimes(1) + fireEvent.focus(linkToFoo) + await waitFor(() => expect(beforeLoad).toHaveBeenCalledTimes(1)) @@ - expect(select).not.toHaveBeenCalled() + expect(select).not.toHaveBeenCalled() + // Ensure the route component hasn't rendered yet during preload + expect(screen.queryByText('Foo index page')).toBeNull()And add the missing import at the top of the file (outside this hunk):
import { waitFor } from '@testing-library/react'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/react-router/tests/routeContext.test.tsx(1 hunks)packages/router-core/src/load-matches.ts(1 hunks)
🔇 Additional comments (1)
packages/router-core/src/load-matches.ts (1)
372-372: Good call: seedcontextwith prior__beforeLoadContextto avoid empty context during preload→enterThis directly addresses #4998 by ensuring the route gets a non-empty context (from the preload pass) while the enter pass
beforeLoadis pending.
|
View your CI Pipeline Execution ↗ for commit 02fb203
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-plugin
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-plugin
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-server-functions-client
@tanstack/start-server-functions-fetcher
@tanstack/start-server-functions-server
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/react-router/tests/routeContext.test.tsx (5)
3129-3153: Rename spy to avoid ambiguity with the route’s beforeLoad propUsing the identifier “beforeLoad” for the spy easily reads like recursion. Rename to a distinct name for clarity.
- test("reproducer #4998 - on navigate (with preload), route component isn't rendered with undefined context if beforeLoad is pending", async () => { - const beforeLoad = vi.fn() + test("reproducer #4998 - on navigate (with preload), route component isn't rendered with undefined context if beforeLoad is pending", async () => { + const beforeLoadSpy = vi.fn() @@ - const fooRoute = createRoute({ + const fooRoute = createRoute({ @@ - beforeLoad: async (...args) => { - beforeLoad(...args) + beforeLoad: async (...args) => { + beforeLoadSpy(...args)
3165-3179: Stabilize timing: avoid relying on fixed sleep for preload completionRelying on exact WAIT_TIME can be flaky under CI load. Wait for the expected condition instead.
- fireEvent.focus(linkToFoo) - expect(beforeLoad).toHaveBeenCalledTimes(1) - expect(resolved).toBe(0) - - await sleep(WAIT_TIME) - - expect(beforeLoad).toHaveBeenCalledTimes(1) + fireEvent.focus(linkToFoo) + expect(beforeLoadSpy).toHaveBeenCalledTimes(1) + expect(resolved).toBe(0) + + // Wait until the preload beforeLoad resolves + await waitFor(() => expect(resolved).toBe(1)) + + expect(beforeLoadSpy).toHaveBeenCalledTimes(1) expect(resolved).toBe(1) - expect(beforeLoad).toHaveBeenNthCalledWith( + expect(beforeLoadSpy).toHaveBeenNthCalledWith( 1, expect.objectContaining({ cause: 'preload', preload: true, }), )Also add waitFor to the RTL import:
// at the top import import { act, cleanup, configure, fireEvent, render, screen, waitFor } from '@testing-library/react'
3183-3196: Wait for the second beforeLoad call instead of asserting synchronouslyThe second invocation may not be observable immediately after click. Use waitFor to avoid race conditions.
- fireEvent.click(linkToFoo) - expect(beforeLoad).toHaveBeenCalledTimes(2) + fireEvent.click(linkToFoo) + await waitFor(() => expect(beforeLoadSpy).toHaveBeenCalledTimes(2)) expect(resolved).toBe(1) @@ - expect(beforeLoad).toHaveBeenCalledTimes(2) - expect(beforeLoad).toHaveBeenNthCalledWith( + expect(beforeLoadSpy).toHaveBeenCalledTimes(2) + expect(beforeLoadSpy).toHaveBeenNthCalledWith( 2, expect.objectContaining({ cause: 'enter', preload: false, }), )
3197-3204: Strengthen the “no empty context” assertionThis ensures every invocation received a non-empty context with “foo” defined, not just that one call matched.
- expect(select).toHaveBeenNthCalledWith( + expect(select).toHaveBeenNthCalledWith( 1, expect.objectContaining({ foo: 1, }), ) - expect(select).not.toHaveBeenCalledWith({}) + // Every call so far should include foo + select.mock.calls.forEach(([ctx]) => { + expect(ctx).toEqual(expect.objectContaining({ foo: expect.any(Number) })) + })
3205-3218: Deflake: wait for the “enter” beforeLoad resolution by condition, not timeMirror the preload-side change to keep the test robust under varying runtime speeds.
- await sleep(WAIT_TIME) - expect(beforeLoad).toHaveBeenCalledTimes(2) + await waitFor(() => expect(resolved).toBe(2)) + expect(beforeLoadSpy).toHaveBeenCalledTimes(2) expect(resolved).toBe(2) @@ - // the route component will be rendered multiple times, ensure it always has the context - expect(select).not.toHaveBeenCalledWith({}) + // The route component renders multiple times; all should have non-empty context + select.mock.calls.forEach(([ctx]) => { + expect(ctx).toEqual(expect.objectContaining({ foo: expect.any(Number) })) + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/react-router/tests/routeContext.test.tsx(1 hunks)packages/router-core/src/load-matches.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/router-core/src/load-matches.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/react-router/tests/routeContext.test.tsx (1)
packages/react-router/src/router.ts (1)
createRouter(80-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (1)
packages/react-router/tests/routeContext.test.tsx (1)
3129-3218: Overall: great, targeted reproducer that asserts the correct context flowThe test captures the preload → enter interplay well and guards against the regression described in #4998. With the minor deflakes and naming tweak above, this should be rock-solid in CI.
|
|
||
| const context = { ...parentMatchContext, ...match.__routeContext } | ||
| const context = { | ||
| ...match.__beforeLoadContext, |
There was a problem hiding this comment.
this is not correct IMO as this will feed the previous beforeLoad context into beforeLoad again.
There was a problem hiding this comment.
@schiller-manuel yes that is the intention, as otherwise the route component will be rendered while the beforeLoad is resolving without the beforeLoad context, which leads to the runtime undefined issues in #4998 .
Or are you suggesting that the route component shouldn't be rendered at all while the beforeLoad is resolving? If so I couldn't pinpoint when that behaviour was changed since I went back to over a year ago and it seems to have been always the case.
There was a problem hiding this comment.
@schiller-manuel this also makes sense to me, FWIW. I'm not sure what the behaviour was before, but from a type perspective it definitely makes more sense to include the context generated by the route preload. Perhaps there's another approach that would fix this. Perhaps preload needs to load data into different context so it's unique compared to __beforeLoadContext? Feels a little clunky but this is blocking our usage of preload so i'll take any ideas. Alternatively, what if we have a flag to indicate that __beforeLoadContext was loaded by preload and only merge it then, would that satisfy?
There was a problem hiding this comment.
I've created #5701 that I think solves the core issue by making sure we only merge when updating context for pending state after a preload. Ideally we could somehow not render component during pending but that's not how suspense works so at least this way we maintain the runtime typesafety of context.
3 participants
Fixes #4998
Summary by CodeRabbit
Bug Fixes
Tests