test(router-core): fix flakiness of store-updates 'preload then navigate' test #4995
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
WalkthroughThe test for hover-preload in packages/react-router/tests/store-updates-during-navigation.test.tsx adds a 100ms delayed loader, inserts a 50ms wait after focusing a link before clicking, and updates the expected store update count from 15 to 14. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
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. ✨ 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 fb9fc25
☁️ 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 (4)
packages/react-router/tests/store-updates-during-navigation.test.tsx (4)
199-201: Replace wall-time sleep with a state-based wait to reduce flakinessSleeping for 50ms is still sensitive to CI load. Wait for the first store update triggered by focus instead, which is what we actually need before clicking.
Apply this diff:
fireEvent.focus(link) - await new Promise((resolve) => setTimeout(resolve, 50)) + await waitFor(() => { + expect(select.mock.calls.length).toBeGreaterThan(before) + })
212-212: Make the assertion non-brittle: assert an upper boundThe surrounding comment says “should be as small as possible.” Asserting an exact count invites future flakiness due to timing nuance. Use an upper bound aligned with your target.
Apply this diff:
- expect(updates).toBe(14) + expect(updates).toBeLessThanOrEqual(14)
107-109: Type correctness: resolveAfter resolves a value but returns PromiseThis currently resolves a value while declaring Promise. Tighten types to prevent accidental misuse and improve editor help.
Apply this diff:
-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)) +}
199-199: Test name vs. stimulus mismatch (hover vs. focus)The test title mentions “hover”, but it uses focus(). Either update the name or simulate hover to better reflect the scenario.
If you want to align with the name using minimal changes:
- fireEvent.focus(link) + fireEvent.mouseEnter(link)Note: switching to hover may change the update count and require re-tuning the expectation.
📜 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(2 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 (1)
packages/react-router/tests/store-updates-during-navigation.test.tsx (1)
192-192: Delay in loader looks good to stabilize preload overlapSwitching the loader to a 100ms delay makes the preload overlap with the navigation consistently. Good call.
tannerlinsley
pushed a commit
that referenced
this pull request
Aug 26, 2025
…ate' test (#4995) It turns out the 'hover preload, then navigate, w/ async loaders' test in `packages/react-router/tests/store-updates-during-navigation.test.tsx` was sometimes (but very rarely) yielding 16 updates instead of the expected 15. This PR makes minimal changes to this test so that it's now reliably 14. I'm not exactly sure where we lost that 1 update, so we might not be measuring exactly the same thing. But it's still worth it to have a robust test suite. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved reliability of hover preloading and navigation behavior tests by introducing realistic loader delays and aligning interaction timing, reducing flakiness. * Updated assertions for store updates during navigation to reflect observed behavior, ensuring more stable and predictable test outcomes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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.
It turns out the 'hover preload, then navigate, w/ async loaders' test in
packages/react-router/tests/store-updates-during-navigation.test.tsxwas sometimes (but very rarely) yielding 16 updates instead of the expected 15. This PR makes minimal changes to this test so that it's now reliably 14.I'm not exactly sure where we lost that 1 update, so we might not be measuring exactly the same thing. But it's still worth it to have a robust test suite.
Summary by CodeRabbit