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 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Test
participant RouterCore
participant Route.beforeLoad
participant Route.loader
Note over Test,RouterCore: Preload phase (preloadRoute)
Test->>RouterCore: preloadRoute("/foo")
alt beforeLoad present
RouterCore->>Route.beforeLoad: invoke (preload)
else no beforeLoad
RouterCore-->>RouterCore: skip beforeLoad
end
alt loader present
RouterCore->>Route.loader: invoke (preload)
else no loader
RouterCore-->>RouterCore: skip loader
end
alt preload resolves
Route.beforeLoad-->>RouterCore: resolve
Route.loader-->>RouterCore: resolve(data)
else preload pending
Route.beforeLoad-->>RouterCore: (pending)
Route.loader-->>RouterCore: (pending)
else preload rejects (notFound/redirect/error)
Route.beforeLoad-->>RouterCore: reject(reason)
Route.loader-->>RouterCore: reject(reason)
end
Note over Test,RouterCore: Navigation phase (navigate)
Test->>RouterCore: navigate("/foo")
alt preload pending
RouterCore-->>RouterCore: reuse pending/cached, avoid duplicate invocation
else preload resolved or none
RouterCore->>Route.beforeLoad: invoke (navigation) / or reuse resolved
RouterCore->>Route.loader: invoke (navigation) / or reuse resolved
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 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). (1)
✨ 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 (
|
|
View your CI Pipeline Execution ↗ for commit 360177f
☁️ 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: |
![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.
|
Would it make sense to also include tests for the following?
Reason I'm asking is that I've filed #4572, where it seems like |
Definitely, I think tests inside |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/router-core/tests/load.test.ts (7)
17-40: Reduce duplication across setup functionsYou have two almost-identical setup() helpers (one per describe). Consider a single factory that accepts optional beforeLoad and/or loader and returns a router. This will DRY the file and make it easier to add parent/root variants for the extra tests reviewers requested.
Example direction (not exhaustive):
+const makeRouter = ({ + beforeLoad, + loader, + extendFoo, +}: { beforeLoad?: BeforeLoad; loader?: Loader; extendFoo?: (foo: BaseRoute<any>) => void } = {}) => { + const rootRoute = new BaseRootRoute({}) + const fooRoute = new BaseRoute({ getParentRoute: () => rootRoute, path: '/foo', beforeLoad, loader }) + extendFoo?.(fooRoute) + const barRoute = new BaseRoute({ getParentRoute: () => rootRoute, path: '/bar' }) + const routeTree = rootRoute.addChildren([fooRoute, barRoute]) + return new RouterCore({ routeTree, history: createMemoryHistory() }) +}
41-46: Baseline check is fine; optionally assert initial locationYou can also assert the initial pathname to guard against future changes in default history initialization.
await router.load() expect(beforeLoad).toHaveBeenCalledTimes(0) +expect(router.state.location.pathname).toBe('/')
48-69: Potential race on immediate call-count assertion
navigatemay schedule work synchronously today, but assertingtoHaveBeenCalledTimes(1)immediately after calling it can become flaky if the internals change. A tiny microtask yield before asserting makes this resilient without changing intent.const navigation = router.navigate({ to: '/foo' }) -expect(beforeLoad).toHaveBeenCalledTimes(1) +await Promise.resolve() +expect(beforeLoad).toHaveBeenCalledTimes(1)If you want to go further, also assert the call arg reflects a non-preload invocation (e.g.,
preloadis falsy).
71-187: Add explicit location/state assertions after navigate for consistencySeveral tests assert only the call count after
await router.navigate(...). Adding an explicit location check reduces the chance of false positives if routing side-effects change in the future. Examples:@@ await router.navigate({ to: '/foo' }) expect(beforeLoad).toHaveBeenCalledTimes(2) +expect(router.state.location.pathname).toBe('/foo')Consider adding the same assert to:
- exec if resolved preload (success)
- exec if pending preload (success)
- exec if rejected preload (notFound)
- exec if pending preload (notFound)
- exec if rejected preload (error)
- exec if pending preload (error)
190-214: Mirror the setup refactor here tooSame duplication as the beforeLoad block. Using a single
makeRouter(see earlier comment) will remove drift between suites.
245-361: Strengthen assertions in loader tests and guard against timing flake
- Where you
await router.navigate(...), consider asserting finalpathname(missing in several cases).- In “skip if pending preload (success|notFound|redirect|error)” add a short microtask yield before the first call-count assertion, mirroring the earlier navigate race comment.
Example snippets:
@@ await router.navigate({ to: '/foo' }) -expect(loader).toHaveBeenCalledTimes(2) +expect(loader).toHaveBeenCalledTimes(2) +expect(router.state.location.pathname).toBe('/foo')@@ router.preloadRoute({ to: '/foo' }) -await Promise.resolve() +await Promise.resolve() expect(router.state.cachedMatches).toEqual( expect.arrayContaining([expect.objectContaining({ id: '/foo' })]), )
364-366: Consider fake timers for deterministic sleepsReal timers (10/100ms) can slow the suite and be flaky under load. Vitest fake timers plus
vi.runOnlyPendingTimersAsync()make these tests deterministic.Example direction:
-import { describe, expect, test, vi } from 'vitest' +import { describe, expect, test, vi, beforeEach, afterEach } from 'vitest' +beforeEach(() => vi.useFakeTimers()) +afterEach(() => vi.useRealTimers()) @@ - await sleep(10) + await vi.runOnlyPendingTimersAsync()
📜 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). (1)
- GitHub Check: Test
🔇 Additional comments (2)
packages/router-core/tests/load.test.ts (2)
1-15: Types and imports look goodSolid use of RouteOptions to derive BeforeLoad/Loader types; using NonNullable ensures the mocks match the real signatures.
127-157: Double‑check intent: redirect semantics differ for beforeLoad vs loaderIn the pending preload + redirect case, beforeLoad keeps the final pathname at
/foo, whereas loader redirects to/bar. If intentional, please codify that in docs/spec to avoid future “fixes” that align them. If not intentional, we should harmonize.Do you want me to open a follow-up test that clearly documents this divergence and references the agreed behavior?
Is this what you had in mind? 1bfb9f4 |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/router-core/tests/load.test.ts (1)
16-188: Add dedicated tests requested in review: parent and root beforeLoad propagationPer larrifax’s feedback, let’s add a minimal suite explicitly asserting that:
- a parent’s beforeLoad runs when navigating to a nested child,
- the root’s beforeLoad runs when navigating to a top-level path.
Note: The “exec on stay” test partially covers this, but explicit tests will prevent regressions and are easier to discover.
@@ describe('beforeLoad skip or exec', () => { // ...existing tests... }) +describe('beforeLoad parent/root propagation', () => { + test('parent beforeLoad is called when navigating to a nested child', async () => { + const root = new BaseRootRoute({}) + const parentBeforeLoad = vi.fn<BeforeLoad>(() => Promise.resolve({ parent: true })) + const childBeforeLoad = vi.fn<BeforeLoad>(() => Promise.resolve({ child: true })) + const parent = new BaseRoute({ + getParentRoute: () => root, + path: '/foo', + beforeLoad: parentBeforeLoad, + }) + const child = new BaseRoute({ + getParentRoute: () => parent, + path: 'child', + beforeLoad: childBeforeLoad, + }) + parent.addChildren([child]) + const tree = root.addChildren([parent]) + const router = new RouterCore({ routeTree: tree, history: createMemoryHistory() }) + + await router.navigate({ to: '/foo/child' }) + + expect(parentBeforeLoad).toHaveBeenCalledTimes(1) + expect(childBeforeLoad).toHaveBeenCalledTimes(1) + // Parent should run before child + expect(parentBeforeLoad.mock.invocationCallOrder[0]).toBeLessThan( + childBeforeLoad.mock.invocationCallOrder[0], + ) + expect(router.state.location.pathname).toBe('/foo/child') + }) + + test('root beforeLoad is called when navigating to a top-level route', async () => { + const rootBeforeLoad = vi.fn<BeforeLoad>(() => Promise.resolve({ fromRoot: true })) + const root = new BaseRootRoute({ beforeLoad: rootBeforeLoad as any }) + const foo = new BaseRoute({ getParentRoute: () => root, path: '/foo' }) + const tree = root.addChildren([foo]) + const router = new RouterCore({ routeTree: tree, history: createMemoryHistory() }) + + await router.navigate({ to: '/foo' }) + + expect(rootBeforeLoad).toHaveBeenCalledTimes(1) + expect(router.state.location.pathname).toBe('/foo') + }) +})
🧹 Nitpick comments (8)
packages/router-core/tests/load.test.ts (8)
48-69: Assert the preload flag for clarity of new semanticsSince this PR is about when beforeLoad executes vs. preloads, add an explicit assertion that the regular navigation call sees preload === false. This will document the contract and prevent regressions.
Apply this diff within the test after awaiting navigation:
const navigation = router.navigate({ to: '/foo' }) expect(beforeLoad).toHaveBeenCalledTimes(1) expect(router.state.pendingMatches).toEqual( expect.arrayContaining([expect.objectContaining({ id: '/foo' })]), ) await navigation + // Regular navigation should call beforeLoad with preload === false + const firstArgs = beforeLoad.mock.calls[0]?.[0] + expect(firstArgs?.preload).toBe(false) expect(router.state.location.pathname).toBe('/foo')
71-83: Make the preload-vs-nav dichotomy explicitThis test encodes the new rule “resolved preloads do not suppress beforeLoad on navigation.” Strengthen it by asserting the two invocations correspond to preload then non-preload.
await router.preloadRoute({ to: '/foo' }) expect(router.state.cachedMatches).toEqual( expect.arrayContaining([expect.objectContaining({ id: '/foo' })]), ) await sleep(10) await router.navigate({ to: '/foo' }) - expect(beforeLoad).toHaveBeenCalledTimes(2) + expect(beforeLoad).toHaveBeenCalledTimes(2) + const flags = beforeLoad.mock.calls.map((c) => c?.[0]?.preload) + expect(flags).toEqual([true, false])
84-96: Pending preload (success): assert call flags to avoid ambiguityGreat case. Recommend asserting the [true, false] preload flags, same as the resolved case.
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(2) + expect(beforeLoad).toHaveBeenCalledTimes(2) + const flags = beforeLoad.mock.calls.map((c) => c?.[0]?.preload) + expect(flags).toEqual([true, false])
127-157: Add inline commentary to document intentional beforeLoad vs. loader divergence on redirectThese two tests assert that:
- beforeLoad redirect during pending preload is ignored for the actual navigation (location stays /foo),
- but loader redirect during pending preload is honored (navigation ends at /bar).
A short comment near the expectations will help future maintainers.
await router.navigate({ to: '/foo' }) - expect(router.state.location.pathname).toBe('/foo') + // Intentionally: redirect thrown during beforeLoad preload must NOT affect actual navigation + expect(router.state.location.pathname).toBe('/foo') expect(beforeLoad).toHaveBeenCalledTimes(2)await router.navigate({ to: '/foo' }) - expect(router.state.location.pathname).toBe('/bar') + // Intentionally: redirect thrown during loader preload MUST affect navigation when still pending + expect(router.state.location.pathname).toBe('/bar') expect(loader).toHaveBeenCalledTimes(1)
266-278: Add a focused assertion that cached loaderData is reused within staleTimeThis test proves skipping, but not that data is reused. Add a companion test returning a sentinel so we assert that the loaderData came from the preload cache.
@@ test('skip if resolved preload (success) within staleTime duration', async () => { const loader = vi.fn() const router = setup({ loader, staleTime: 1000 }) @@ expect(loader).toHaveBeenCalledTimes(1) }) + + test('reuse cached loaderData within staleTime window', async () => { + const loader = vi.fn(() => Promise.resolve({ from: 'preload' })) + const router = setup({ loader, staleTime: 1000 }) + await router.preloadRoute({ to: '/foo' }) + await sleep(10) + await router.navigate({ to: '/foo' }) + // Skipped on nav; only preload invocation + expect(loader).toHaveBeenCalledTimes(1) + // Data should be available from cache + expect(router.state.matches).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + id: '/foo', + loaderData: expect.objectContaining({ from: 'preload' }), + }), + ]), + ) + })
385-455: Assert parent and root execution on initial nested navigation; then verify re-exec on sibling “stay”This test already proves re-exec on sibling navigation. Add explicit assertions immediately after the first navigate to document that:
- root and layout beforeLoad both ran and were awaited,
- their loaders ran once too.
This also partially addresses the reviewer request about parent/root beforeLoad invocation.
await router.navigate({ to: '/foo' }) expect(router.state.location.pathname).toBe('/foo') + // Initial nested nav should execute parents' beforeLoad and loader + expect(rootBeforeLoad).toHaveBeenCalledTimes(1) + expect(layoutBeforeLoad).toHaveBeenCalledTimes(1) + expect(rootLoader).toHaveBeenCalledTimes(1) + expect(layoutLoader).toHaveBeenCalledTimes(1) + // And they should have been awaited + expect(rootBeforeLoadResolved).toBe(true) + expect(layoutBeforeLoadResolved).toBe(true) rootBeforeLoadResolved = false layoutBeforeLoadResolved = false vi.clearAllMocks()
71-187: Reduce flakiness: prefer deterministic timers over arbitrary sleepsMultiple tests rely on sleep(10) / sleep(100) to allow preloads to resolve. On slower CI, these can be flaky. Consider using vitest fake timers and advancing time deterministically, or at least centralizing a slightly larger constant (e.g., 25–50ms) with comments.
You can prototype with fake timers in one describe to ensure RouterCore’s timers cooperate:
// Example pattern describe('with fake timers', () => { beforeEach(() => vi.useFakeTimers()) afterEach(() => vi.useRealTimers()) test('pending preload (success) with timers', async () => { const beforeLoad = vi.fn<BeforeLoad>(async () => { await sleep(100) }) const router = setup({ beforeLoad }) router.preloadRoute({ to: '/foo' }) await Promise.resolve() // Drive pending preload to completion deterministically await vi.advanceTimersByTimeAsync(100) await router.navigate({ to: '/foo' }) expect(beforeLoad).toHaveBeenCalledTimes(2) }) })If fake timers interfere with internals, consider bumping sleep(10) to sleep(25) and add a comment explaining why.
Also applies to: 253-265, 266-278, 279-291, 292-305, 307-320, 322-336, 338-352, 354-367, 369-382, 385-455
457-459: Utility helper is fineTiny helper is adequate. If you adopt fake timers, keep this but ensure tests advance timers instead of sleeping where possible.
📜 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 (3)
packages/router-core/tests/load.test.ts (3)
1-15: Imports, type aliases, and test primitives look solidGood use of RouteOptions-derived utility types to keep mocks typed. No issues here.
16-40: Setup scaffold is clear and minimalThe route tree builder for the beforeLoad suite is straightforward and mirrors real usage. No changes needed.
190-222: Loader suite setup is good; gcTime mirrors staleTimeMirroring gcTime to staleTime avoids cache GC between preload and nav. Looks correct.
WIP
Summary by CodeRabbit