test(router): add waitFor for page navigation link/optional-path-params #5741
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
Contributor
WalkthroughTests in react-router and solid-router were updated to synchronize with navigation by adding Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Code
participant DOM as Browser DOM
participant Router as Router
Test->>DOM: find link/button
Test->>DOM: click link/button
note right of DOM `#DFF7DF`: click initiates navigation
DOM->>Router: navigation request
Router->>Router: update location (async)
Router->>DOM: render updated route (async)
alt Wait for update
Test->>Test: waitFor(assertion)
Test->>DOM: query for active class / pathname
DOM-->>Test: element reflects new state
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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 f99dc18
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/nitro-v2-vite-plugin
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@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-router-ssr-query
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-static-server-functions
@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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/react-router/tests/link.test.tsx (1)
363-365: LGTM! Test synchronization correctly handles async navigation.The
waitForwrapper properly ensures the router state update completes before asserting the active class. This aligns with the PR's objective to improve test reliability for asynchronous navigation.Optional: Consider whether the subsequent assertions (lines 367-409) that check link states after navigation might benefit from being included in this
waitForblock or having their own synchronization checks. The current implementation likely works because the firstwaitForprovides sufficient delay, but explicitly grouping related assertions could make the test's intent clearer and more robust:await waitFor(() => { expect(indexFooBarLink).toHaveClass('active') + + expect(indexExactLink).toHaveClass('inactive') + expect(indexExactLink).not.toHaveClass('active') + // ... other related assertions })However, this is purely a robustness suggestion and not required if the current implementation is stable.
packages/solid-router/tests/link.test.tsx (1)
396-399: Good use of waitFor to de-flake async navigation assertion.Optional: also assert the location/search within the same waitFor, or re-query the link post‑navigation to avoid a potentially stale element reference.
packages/react-router/tests/optional-path-params.test.tsx (1)
650-665: Synchronizing via waitFor and deferring queries looks solid.Optional: prefer DOM signals over internal state for assertions, e.g., await screen.findByRole('heading', { name: 'Posts' }) and/or expect(window.location.pathname) inside waitFor, to reduce coupling to router internals.
packages/solid-router/tests/optional-path-params.test.tsx (1)
651-665: Nice stabilization with waitFor and deferred lookups.Optional: assert via UI (e.g., heading present) and/or window.location.pathname inside waitFor to avoid relying on router.state directly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/react-router/tests/link.test.tsx(1 hunks)packages/react-router/tests/optional-path-params.test.tsx(2 hunks)packages/solid-router/tests/link.test.tsx(1 hunks)packages/solid-router/tests/optional-path-params.test.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/solid-router/tests/link.test.tsxpackages/solid-router/tests/optional-path-params.test.tsxpackages/react-router/tests/optional-path-params.test.tsxpackages/react-router/tests/link.test.tsx
packages/{react-router,solid-router}/**
📄 CodeRabbit inference engine (AGENTS.md)
Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Files:
packages/solid-router/tests/link.test.tsxpackages/solid-router/tests/optional-path-params.test.tsxpackages/react-router/tests/optional-path-params.test.tsxpackages/react-router/tests/link.test.tsx
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to packages/{react-router,solid-router}/** : Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
packages/solid-router/tests/link.test.tsxpackages/solid-router/tests/optional-path-params.test.tsxpackages/react-router/tests/optional-path-params.test.tsx
📚 Learning: 2025-09-23T17:36:12.598Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to packages/{react-router,solid-router}/** : Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Applied to files:
packages/solid-router/tests/link.test.tsxpackages/solid-router/tests/optional-path-params.test.tsxpackages/react-router/tests/optional-path-params.test.tsx
📚 Learning: 2025-10-14T18:59:33.990Z
Learnt from: FatahChan
Repo: TanStack/router PR: 5475
File: e2e/react-start/basic-prerendering/src/routes/redirect/$target/via-beforeLoad.tsx:8-0
Timestamp: 2025-10-14T18:59:33.990Z
Learning: In TanStack Router e2e test files, when a route parameter is validated at the route level (e.g., using zod in validateSearch or param validation), switch statements on that parameter do not require a default case, as the validation ensures only expected values will reach the switch.
Applied to files:
packages/solid-router/tests/optional-path-params.test.tsx
📚 Learning: 2025-10-09T12:59:02.129Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/src/styles/app.css:19-21
Timestamp: 2025-10-09T12:59:02.129Z
Learning: In e2e test directories (paths containing `e2e/`), accessibility concerns like outline suppression patterns are less critical since the code is for testing purposes, not production use.
Applied to files:
packages/solid-router/tests/optional-path-params.test.tsx
⏰ 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/react-router/tests/optional-path-params.test.tsx (1)
2-9: Importing waitFor is correct for the new async assertions.packages/solid-router/tests/optional-path-params.test.tsx (1)
718-735: Good pattern: re-query element after navigation and wrap assertions in waitFor.This reduces stale node issues and flakiness. No changes needed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/react-router/tests/optional-path-params.test.tsx (1)
649-664: Good use ofwaitForfor async navigation assertions.The
waitForwrapper correctly handles asynchronous router state updates after navigation, preventing potential race conditions in the test.Optional: Remove unnecessary
awaitonfireEvent.click().The
fireEvent.click()method is synchronous and returnsvoid, so awaiting it has no effect. Consider this refactor:- await fireEvent.click(navigateAll) + fireEvent.click(navigateAll) await waitFor(() => { expect(router.state.location.pathname).toBe('/posts') }) const navigateTech = await screen.findByTestId('navigate-tech') - await fireEvent.click(navigateTech) + fireEvent.click(navigateTech) await waitFor(() => { expect(router.state.location.pathname).toBe('/posts/tech') }) const navigateSpecific = await screen.findByTestId('navigate-specific') - await fireEvent.click(navigateSpecific) + fireEvent.click(navigateSpecific) await waitFor(() => { expect(router.state.location.pathname).toBe('/posts/tech/hello-world') })Optional: Consider applying this pattern to other tests.
For consistency, other tests in this file that assert
router.state.location.pathnameafter navigation (lines 511, 533, 721, 728) could benefit from the samewaitForwrapper to ensure robustness against timing variations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-router/tests/optional-path-params.test.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/react-router/tests/optional-path-params.test.tsx
packages/{react-router,solid-router}/**
📄 CodeRabbit inference engine (AGENTS.md)
Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Files:
packages/react-router/tests/optional-path-params.test.tsx
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to packages/{react-router,solid-router}/** : Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
packages/react-router/tests/optional-path-params.test.tsx
📚 Learning: 2025-09-23T17:36:12.598Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to packages/{react-router,solid-router}/** : Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Applied to files:
packages/react-router/tests/optional-path-params.test.tsx
⏰ 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: Preview
- GitHub Check: Test
🔇 Additional comments (1)
packages/react-router/tests/optional-path-params.test.tsx (1)
2-9: LGTM! Proper import for async test assertions.The addition of
waitForto the imports is correct and necessary for handling asynchronous router navigation state updates in the test below.
This was referenced Nov 3, 2025
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.
this better handles the async navigation transitions
Summary by CodeRabbit