refactor(router-core): rework the conditions under which a beforeLoad is executed or skipped #4993
Conversation
… is executed or skipped
WalkthroughAdds a new test suite at Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Test
participant RouterCore
participant Route.beforeLoad
Test->>RouterCore: preloadRoute("/foo") %% start preload
RouterCore->>Route.beforeLoad: invoke (preload)
alt preload resolves
Route.beforeLoad-->>RouterCore: resolve
else preload rejects (notFound/redirect/error)
Route.beforeLoad-->>RouterCore: reject
end
Test->>RouterCore: navigate("/foo") %% user navigation
alt preload in progress (pending)
RouterCore-->>Route.beforeLoad: skip
else preload resolved or no preload
RouterCore->>Route.beforeLoad: invoke (navigation)
Route.beforeLoad-->>RouterCore: resolve/reject
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ 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 (
|
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 59s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-08-18 22:05:40 UTC
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: |
![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 (3)
packages/router-core/tests/load.test.ts (3)
7-9: Avoid real 100ms timers; make the test deterministic with fake timersUsing setTimeout(100) slows the suite and can be flaky in CI. You can keep the same behavior while making timing deterministic by switching to Vitest fake timers and flushing them.
Apply this diff to control timers and avoid real sleeps:
test('current behavior', async () => { - const rootRoute = new BaseRootRoute({}) - const beforeLoad = vi.fn( - () => new Promise((resolve) => setTimeout(resolve, 100)), - ) + vi.useFakeTimers() + const rootRoute = new BaseRootRoute({}) + const beforeLoad = vi.fn( + () => new Promise((resolve) => setTimeout(resolve, 100)), + )And later in the test, flush timers and then restore real timers:
- await router.navigate({ to: '/foo' }) + const navPromise = router.navigate({ to: '/foo' }) + await vi.runAllTimersAsync() // or: await vi.advanceTimersByTimeAsync(100) + await navPromise @@ - expect(beforeLoad).toHaveBeenCalledTimes(2) + expect(beforeLoad).toHaveBeenCalledTimes(2) + vi.useRealTimers()
5-5: Rename test for clarity“current behavior” is vague. Make the assertion intent obvious in the title so failures are easier to interpret.
- test('current behavior', async () => { + test('documents current behavior: beforeLoad runs once on preload and once on navigate', async () => {
19-21: Lock in preload behavior with an explicit preload assertionAdding a quick assertion after the microtask yield makes the test’s intent clear and guards against scheduling changes in
preloadRoute. Inpackages/router-core/tests/load.test.tsaround lines 19–21:router.preloadRoute({ to: '/foo' }) await Promise.resolve() + expect(beforeLoad).toHaveBeenCalledTimes(1) // preload should have started exactly once // navigation should trigger a second load await router.navigate({ to: '/foo' })Since
preloadRouteisasync(it returns aPromise), you can optionally add a comment noting that you’re intentionally not awaiting it to test overlapping preload vs. navigation behavior.
📜 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 (1)
packages/router-core/tests/load.test.ts(1 hunks)
⏰ 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 (2)
packages/router-core/tests/load.test.ts (2)
4-24: Test structure LGTM; clearly codifies behavior under discussionNicely scoped, minimal reproduction that captures the preload vs navigate semantics of beforeLoad.
16-18: No teardown needed for RouterCore in this testRouterCore in the Node‐based load test uses an in‐memory history and does not register any DOM or background listeners, so there’s nothing to clean up between tests. You can safely omit a
.destroy()/.dispose()call here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/router-core/tests/load.test.ts (6)
52-62: Make the “preload is ongoing” test deterministic with fake timersThis test relies on a real 100ms timer to keep preload pending. Real timers can cause flakiness and slow CI. Prefer fake timers to control timing precisely and avoid waiting.
Suggested change within this test:
- test('skip if preload is ongoing', async () => { - const beforeLoad = vi.fn( + test('skip if preload is ongoing', async () => { + vi.useFakeTimers() + const beforeLoad = vi.fn( () => new Promise((resolve) => setTimeout(resolve, 100)), ) const router = setup({ beforeLoad }) router.preloadRoute({ to: '/foo' }) await Promise.resolve() await router.navigate({ to: '/foo' }) expect(beforeLoad).toHaveBeenCalledTimes(1) + vi.useRealTimers() })Please run the suite locally once with and once without the change to confirm flake reduction and parity in behavior.
64-71: Resolved preload skip case is clear and sufficientThis ensures successful preloads are reused. Minor nit: using
mockResolvedValue(undefined)is slightly more idiomatic, but not required.Apply if desired:
- const beforeLoad = vi.fn(() => Promise.resolve()) + const beforeLoad = vi.fn<BeforeLoad>().mockResolvedValue(undefined)
87-101: Deterministic timing for “pending then reject with notFound”Same timing concern as the earlier “ongoing” case. Use fake timers so the preload remains pending until you explicitly advance or end the test.
Suggested change within this test:
- test('exec if preload is pending, but will reject w/ notFound', async () => { - const beforeLoad = vi.fn<BeforeLoad>(async ({ preload }) => { + test('exec if preload is pending, but will reject w/ notFound', async () => { + vi.useFakeTimers() + const beforeLoad = vi.fn<BeforeLoad>(async ({ preload }) => { await new Promise((resolve) => setTimeout(resolve, 100)) if (preload) throw notFound() }) const router = setup({ beforeLoad, }) router.preloadRoute({ to: '/foo' }) await Promise.resolve() await router.navigate({ to: '/foo' }) - expect(beforeLoad).toHaveBeenCalledTimes(2) + expect(router.state.location.pathname).toBe('/foo') + expect(beforeLoad).toHaveBeenCalledTimes(2) + vi.useRealTimers() })
117-131: Use fake timers for the “pending then redirect” caseMirror the deterministic-timers approach here as well.
Suggested change within this test:
- test('exec if preload is pending, but will reject w/ redirect', async () => { - const beforeLoad = vi.fn<BeforeLoad>(async ({ preload }) => { + test('exec if preload is pending, but will reject w/ redirect', async () => { + vi.useFakeTimers() + const beforeLoad = vi.fn<BeforeLoad>(async ({ preload }) => { await new Promise((resolve) => setTimeout(resolve, 100)) if (preload) throw redirect({ to: '/bar' }) }) @@ - expect(router.state.location.pathname).toBe('/foo') + expect(router.state.location.pathname).toBe('/foo') expect(beforeLoad).toHaveBeenCalledTimes(2) + vi.useRealTimers() })
133-146: Consider asserting the resulting location for the “regular Error” caseFor consistency with redirect tests (and to ensure preload errors don’t pollute the subsequent navigation), you could assert
pathname === '/foo'here as well.Optional addition:
await router.navigate({ to: '/foo' }) - expect(beforeLoad).toHaveBeenCalledTimes(2) + expect(router.state.location.pathname).toBe('/foo') + expect(beforeLoad).toHaveBeenCalledTimes(2)Also, confirm that
preloadRouteresolves even whenbeforeLoadthrows a generic Error (not just notFound/redirect), since you are awaiting it here.
147-160: Make the “pending then regular Error” test deterministic with fake timersSame timer determinism recommendation applies here.
Suggested change within this test:
- test('exec if preload is pending, but will reject w/ regular Error', async () => { - const beforeLoad = vi.fn<BeforeLoad>(async ({ preload }) => { + test('exec if preload is pending, but will reject w/ regular Error', async () => { + vi.useFakeTimers() + const beforeLoad = vi.fn<BeforeLoad>(async ({ preload }) => { await new Promise((resolve) => setTimeout(resolve, 100)) if (preload) throw new Error('error') }) @@ - expect(beforeLoad).toHaveBeenCalledTimes(2) + expect(router.state.location.pathname).toBe('/foo') + expect(beforeLoad).toHaveBeenCalledTimes(2) + vi.useRealTimers() })
📜 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 (1)
packages/router-core/tests/load.test.ts(1 hunks)
🔇 Additional comments (5)
packages/router-core/tests/load.test.ts (5)
14-36: Solid, minimal test harness setupRoute tree + RouterCore construction is clear and keeps tests focused on beforeLoad behavior. The typed
BeforeLoadalias is a nice touch to keep mock signatures aligned.
38-44: Baseline “no-op” case covered wellThis verifies that merely loading the router doesn’t spuriously invoke child route beforeLoad. Good guard-rail.
45-50: Happy-path navigation coverage looks goodSingle invocation on direct navigation to /foo is the expected baseline.
102-116: Redirect-from-preload behavior validatedGreat to see the explicit assertion that a redirect thrown during preload does not hijack the subsequent navigation. This test clearly encodes the intended contract.
73-86: ConfirmpreloadRouteresolution semantics on rejection pathsI’m not seeing the
preloadRouteimplementation in the core router—could you please verify whether it’s intended to resolve (and cache) even whenbeforeLoadthrowsnotFound()? If that is the designed behavior, consider:– Documenting this contract on
RouterCore.preloadRoute.
– Strengthening the test by asserting the location remains/fooafter navigation:await router.navigate({ to: '/foo' }) expect(beforeLoad).toHaveBeenCalledTimes(2) +expect(router.state.location.pathname).toBe('/foo')
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/router-core/tests/load.test.ts (7)
11-13: Preferunknownoveranyfor tighter typings.This keeps the test type-safe while still generic.
-type AnyRouteOptions = RouteOptions<any> +type AnyRouteOptions = RouteOptions<unknown> type BeforeLoad = NonNullable<AnyRouteOptions['beforeLoad']>
68-81: Make timer-driven test deterministic and faster with fake timers.Relying on real
setTimeout(100)can lead to flakiness and slows CI. Use Vitest fake timers and flush them explicitly. Also, ensure you flush the pending timer at the end so no timers leak across tests.test('skip if preload is ongoing', async () => { const beforeLoad = vi.fn( () => new Promise((resolve) => setTimeout(resolve, 100)), ) const router = setup({ beforeLoad }) - router.preloadRoute({ to: '/foo' }) + vi.useFakeTimers() + router.preloadRoute({ to: '/foo' }) await Promise.resolve() expect(router.state.cachedMatches).toEqual( expect.arrayContaining([expect.objectContaining({ id: '/foo' })]), ) await router.navigate({ to: '/foo' }) expect(beforeLoad).toHaveBeenCalledTimes(1) + await vi.advanceTimersByTimeAsync(100) + vi.useRealTimers() })
95-107: Assert final location to ensure preload notFound doesn’t affect real navigation.This strengthens the contract you’re testing: preload failure with notFound should not prevent a subsequent successful navigation.
await router.preloadRoute({ to: '/foo' }) await router.navigate({ to: '/foo' }) + expect(router.state.location.pathname).toBe('/foo') expect(beforeLoad).toHaveBeenCalledTimes(2)
109-122: Deterministic pending-reject (notFound) test + assert final location.Use fake timers to avoid leaking a 100ms pending timer into other tests and explicitly assert that the real navigation is unaffected.
test('exec if preload is pending, but will reject w/ notFound', async () => { const beforeLoad = vi.fn<BeforeLoad>(async ({ preload }) => { await new Promise((resolve) => setTimeout(resolve, 100)) if (preload) throw notFound() }) const router = setup({ beforeLoad, }) - router.preloadRoute({ to: '/foo' }) + vi.useFakeTimers() + router.preloadRoute({ to: '/foo' }) await Promise.resolve() await router.navigate({ to: '/foo' }) + expect(router.state.location.pathname).toBe('/foo') expect(beforeLoad).toHaveBeenCalledTimes(2) + await vi.advanceTimersByTimeAsync(100) + vi.useRealTimers() })
139-153: Use fake timers for pending-reject with redirect to avoid timer leaks.Same rationale as other timer-based tests. This keeps tests fast and robust.
test('exec if preload is pending, but will reject w/ redirect', async () => { const beforeLoad = vi.fn<BeforeLoad>(async ({ preload }) => { await new Promise((resolve) => setTimeout(resolve, 100)) if (preload) throw redirect({ to: '/bar' }) }) const router = setup({ beforeLoad, }) - router.preloadRoute({ to: '/foo' }) + vi.useFakeTimers() + router.preloadRoute({ to: '/foo' }) await Promise.resolve() await router.navigate({ to: '/foo' }) expect(router.state.location.pathname).toBe('/foo') expect(beforeLoad).toHaveBeenCalledTimes(2) + await vi.advanceTimersByTimeAsync(100) + vi.useRealTimers() })
155-167: Add final location assertion to complement call-count check.Confirms that a preload error doesn’t poison subsequent normal navigation.
await router.preloadRoute({ to: '/foo' }) await router.navigate({ to: '/foo' }) + expect(router.state.location.pathname).toBe('/foo') expect(beforeLoad).toHaveBeenCalledTimes(2)
169-182: Use fake timers and assert final location for pending-reject with regular Error.Prevents timer leak and ensures correct behavior on the real navigation.
test('exec if preload is pending, but will reject w/ regular Error', async () => { const beforeLoad = vi.fn<BeforeLoad>(async ({ preload }) => { await new Promise((resolve) => setTimeout(resolve, 100)) if (preload) throw new Error('error') }) const router = setup({ beforeLoad, }) - router.preloadRoute({ to: '/foo' }) + vi.useFakeTimers() + router.preloadRoute({ to: '/foo' }) await Promise.resolve() await router.navigate({ to: '/foo' }) + expect(router.state.location.pathname).toBe('/foo') expect(beforeLoad).toHaveBeenCalledTimes(2) + await vi.advanceTimersByTimeAsync(100) + vi.useRealTimers() })
📜 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 (1)
packages/router-core/tests/load.test.ts(1 hunks)
⏰ 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 (4)
packages/router-core/tests/load.test.ts (4)
15-36: Route setup helper is clean and reusable.Good isolation per test and minimal surface area in
setup. This makes the suite easy to reason about.
45-66: Solid coverage of "regular nav" happy-path.Asserting both pending state and final match context is great; it verifies
beforeLoadis wired correctly and its return value is plumbed into the match.
83-93: LGTM: "preload resolved successfully" is covered well.The single invocation assertion after navigating is the right behavior here.
124-137: LGTM: redirect-on-preload does not side-effect navigation.Good assertion that real navigation ends at
/fooandbeforeLoadis invoked twice.
WIP
Summary by CodeRabbit