test(react-router): add more variants to store-updates test #5019
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughAdds new router options defaultStaleTime/defaultGcTime to createRouter, updates test scaffolding/UI for navigation, introduces a back() helper, parameterizes staleTime in setup, and expands tests to cover preloading and loader behavior with revised update-count expectations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant R as Router
participant C as Cache
participant L as Loader
Note over R,C: Router created with defaultStaleTime/defaultGcTime
U->>R: Navigate to /posts (optionally preload beforehand)
alt Preloaded & fresh
R->>C: Check entry (fresh)
C-->>R: Serve cached data
R-->>U: Render with minimal updates
else Preloaded but stale
R->>C: Check entry (stale)
R->>L: Revalidate/Load
L-->>R: Data
R->>C: Update entry (respect gcTime)
R-->>U: Render with updates
else Not preloaded
R->>L: Load
L-->>R: Data
R->>C: Put entry (staleTime/gcTime)
R-->>U: Render
end
U->>R: Back
R-->>U: Render previous route (Index)
Note over C: GC respects defaultGcTime
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 (
|
|
View your CI Pipeline Execution ↗ for commit 6521321
☁️ 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 (6)
packages/react-router/tests/store-updates-during-navigation.test.tsx (6)
98-106: Prefer userEvent for clicks to better emulate user inputFireEvent works, but userEvent is closer to real interactions and can reduce edge cases with focus/blur sequencing.
Apply this minimal tweak:
- const link = await waitFor(() => screen.getByRole('link', { name: 'Back' })) - fireEvent.click(link) + const link = await waitFor(() => screen.getByRole('link', { name: 'Back' })) + await userEvent.click(link)Add the import (outside this hunk):
import userEvent from '@testing-library/user-event'
229-243: Strengthen the assertion by verifying the preload actually removed a loader callGreat coverage. To ensure preloading is truly leveraged, assert that the loader ran only once across preload + navigation.
- const params = setup({ - beforeLoad: () => Promise.resolve({ foo: 'bar' }), - loader: () => resolveAfter(100, { hello: 'world' }), + const loaderSpy = vi.fn(() => resolveAfter(100, { hello: 'world' })) + const params = setup({ + beforeLoad: () => Promise.resolve({ foo: 'bar' }), + loader: loaderSpy, staleTime: 1000, }) await params.router.preloadRoute({ to: '/posts' }) const updates = await run(params) // This number should be as small as possible to minimize the amount of work // that needs to be done during a navigation. // Any change that increases this number should be investigated. expect(updates).toBe(7) + expect(loaderSpy).toHaveBeenCalledTimes(1)
245-259: Same strengthening for sync loadersEven with sync loaders, asserting a single call validates reuse of preloaded data.
- const params = setup({ - beforeLoad: () => ({ foo: 'bar' }), - loader: () => ({ hello: 'world' }), + const loaderSpy = vi.fn(() => ({ hello: 'world' })) + const params = setup({ + beforeLoad: () => ({ foo: 'bar' }), + loader: loaderSpy, staleTime: 1000, }) await params.router.preloadRoute({ to: '/posts' }) const updates = await run(params) expect(updates).toBe(6) + expect(loaderSpy).toHaveBeenCalledTimes(1)
261-275: Leverage a spy to guarantee no second load within stale windowThe update count already indicates caching; adding a loader call assertion reduces brittleness.
- const params = setup({ - loader: () => resolveAfter(100, { hello: 'world' }), + const loaderSpy = vi.fn(() => resolveAfter(100, { hello: 'world' })) + const params = setup({ + loader: loaderSpy, staleTime: 1000, }) await run(params) await back() const updates = await run(params) expect(updates).toBe(5) + expect(loaderSpy).toHaveBeenCalledTimes(1)
277-293: Stabilize timing: replace real timers with fake timers for deterministic preload/preloadUsing real setTimeout(20) can be flaky under load. Fake timers make this race deterministic.
test('preload a preloaded route w/ async loader', async () => { - const params = setup({ - loader: () => resolveAfter(100, { hello: 'world' }), - }) + vi.useFakeTimers() + const params = setup({ + loader: () => resolveAfter(100, { hello: 'world' }), + }) await params.router.preloadRoute({ to: '/posts' }) - await new Promise((r) => setTimeout(r, 20)) + vi.advanceTimersByTime(20) const before = params.select.mock.calls.length await params.router.preloadRoute({ to: '/posts' }) const after = params.select.mock.calls.length const updates = after - before // This number should be as small as possible to minimize the amount of work // that needs to be done during a navigation. // Any change that increases this number should be investigated. expect(updates).toBe(1) + vi.useRealTimers() })
121-123: Return the resolved value from resolveAfter with correct typingCurrent signature returns
Promise<void>while resolving a value. Generics keep types accurate and self-document intent.-function resolveAfter(ms: number, value: any) { - return new Promise<void>((resolve) => setTimeout(() => resolve(value), ms)) -} +function resolveAfter<T>(ms: number, value: T): Promise<T> { + return new Promise<T>((resolve) => setTimeout(() => resolve(value), ms)) +}
📜 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/react-router/tests/store-updates-during-navigation.test.tsx(4 hunks)
🔇 Additional comments (4)
packages/react-router/tests/store-updates-during-navigation.test.tsx (4)
50-56: Root scaffolding looks good; stable anchor points for interactionsAdding explicit Back/Posts links and keeping Outlet visible makes the tests clearer and interactions deterministic.
62-62: Simplified Index component is appropriateLean markup helps reduce unrelated DOM updates during these measurements.
228-228: Non-functional whitespace changeNo action needed.
34-35: MakedefaultStaleTime/defaultGcTimeopt-in and confirm public API acceptanceUsing a conditional spread ensures you only override the defaults when a
staleTimevalue is provided, preserving the built-in defaults otherwise:- defaultStaleTime: staleTime, - defaultGcTime: staleTime, + ...(staleTime != null + ? { defaultStaleTime: staleTime, defaultGcTime: staleTime } + : {}),Verified in
packages/router-core/src/router.tsthat both
defaultStaleTime?(line 197)defaultGcTime?(line 245)are declared on the public
RouterOptionsinterface, and the router’s constructor options extend this interface, so these fields are accepted bycreateRouter. Please apply this pattern at the other occurrences (lines 43–44 and 89–91).
tannerlinsley
pushed a commit
that referenced
this pull request
Aug 30, 2025
Add a few more setup configurations to measure the number of `__store` updates: - with caching (`staleTime > 0`) - when preloading multiple times the same route (simulates hovering a list of links to the same page w/ different params) --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a few more setup configurations to measure the number of
__storeupdates:staleTime > 0)