refactor(router-core): Verify changes are necessary before updating store #4964
+39
−31
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
WalkthroughRefactors router load lifecycle to avoid writing undefined loaderData, compute and conditionally set preload, initialize per-match loaderPromise, and limit cleanup updates. Tests add a resolveAfter helper and lower expected store-update counts to match the new async behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Router
participant Match
App->>Router: navigate()
Router->>Match: set fresh loaderPromise (for preload/load)
Router->>Match: run beforeLoad / loader (async or sync)
alt loaderData !== undefined
Router->>Match: write loaderData
end
Router->>Router: compute nextPreload
alt nextPreload != match.preload
Router->>Match: update preload
end
Router->>Router: clear pending timeout
alt load was async
Router->>Match: keep loaderPromise
else
Router->>Match: reset loaderPromise
end
Router->>Match: reset dehydrated
Router->>Match: update isFetching/invalid only if changed
Router-->>App: emit minimal state updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 (
|
|
View your CI Pipeline Execution ↗ for commit 4a45c06
☁️ 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/src/router.ts (1)
2842-2854: Preload computation + conditional update looks good; minor nit on early loaderPromise creationComputing
nextPreloadand only updating when it differs reduces store churn. One nit:match._nonReactive.loaderPromiseis created unconditionally even when no loader will run and we’ll resolve it immediately later. Consider lazily creating it only in the branches that actually run a loader to trim unnecessary promise churn. Not a blocker.packages/react-router/tests/store-updates-during-navigation.test.tsx (2)
107-110: Type the helper to return the awaited value
resolveAftercurrently returnsPromise<void>but resolves withvalue. This compiles due to loose typing but is misleading and weakens type-safety.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)) +}
170-172: Deflake the “nothing” test to avoid timing varianceThe >=>= range and “flaky” note suggest timer/tick race. Consider using fake timers to control
pendingMs/pendingMinMstiming and eliminate nondeterminism:
vi.useFakeTimers()before the test- Advance deterministically:
vi.advanceTimersByTime(...)await Promise.resolve()between advances if needed to flush microtasksNot required for this PR, but it will make this assertion stable.
📜 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 (2)
packages/react-router/tests/store-updates-during-navigation.test.tsx(6 hunks)packages/router-core/src/router.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/router-core/src/router.ts (4)
packages/router-core/src/utils.ts (1)
createControlledPromise(398-422)packages/router-core/src/index.ts (1)
createControlledPromise(278-278)packages/react-router/src/index.tsx (1)
createControlledPromise(35-35)packages/solid-router/src/index.tsx (1)
createControlledPromise(35-35)
⏰ 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 (3)
packages/router-core/src/router.ts (2)
2901-2914: LGTM: precise end-of-load cleanup reduces unnecessary store writes
- Clearing
pendingTimeoutand resetting its field unconditionally- Only resetting
loaderPromisewhen not running async- Resetting
dehydrated- Change-aware
isFetching/invalidupdateAll of this helps minimize redundant updates and avoids flicker.
2690-2706: Preserve the existing undefined‐guard to avoid unintended data clearsThe current
if (loaderData !== undefined)check is intentional: many loaders (especially those with a curly‐block body and no explicitreturn) implicitly returnundefined, and we rely on that to prevent child or auxiliary loaders from wiping out previously fetched data. Applying the suggested change would clear validloaderDatawhenever a loader doesn’t explicitly return a value—breaking existing tests and expected behavior.→ Retain the
loaderData !== undefinedguard as is.Likely an incorrect or invalid review comment.
packages/react-router/tests/store-updates-during-navigation.test.tsx (1)
125-125: Updated expectations align with fewer store updatesThe reduced update counts match the router-core’s conditional store writes and async handling tweaks. Looks good.
Also applies to: 143-144, 159-159, 170-172, 211-211
schiller-manuel
approved these changes
Aug 16, 2025
Sheraff
deleted the
refactor-router-core-check-store-changed-before-updating
branch
This was referenced Aug 16, 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.
Following #4916, #4925, #4926, and #4928, this PR keeps reducing the number of store updates (
__store.setStatethroughupdateMatch).We check the values we want to set in the store are actually different from what is already in the store before calling
updateMatchIn the
store-updates-during-navigationtest tracking the number of executions of auseRouterState > selectmethod during a navigation, our main test case goes from 14 calls without this PR, to 10 calls with this PR. Most test cases show significant improvements as well.Should be a partial improvement of #4359
Summary by CodeRabbit
Bug Fixes
Performance
Tests