fix(router-core): keep latest load alive through match commit#7424
Open
schiller-manuel wants to merge 1 commit into
Open
fix(router-core): keep latest load alive through match commit#7424schiller-manuel wants to merge 1 commit into
schiller-manuel wants to merge 1 commit into
Conversation
A router load now records its controlled load promise as latest before the transition body starts. This gives the load a stable identity for the entire navigation, including the synchronous part of startTransition. Pass that identity into loadMatches so stale loads cannot call onReady and promote their pending matches after a newer load has started. Also guard the onReady commit itself before scheduling the view-transition update and again inside the update callback, so a load that becomes stale while waiting for the view transition cannot mutate the active match stores. Add a local commitPromise around the pending-to-active match commit. The router's startViewTransition wrapper is fire-and-forget, so without this latch onReady could resolve, loadMatches could finish, and load() could clear latestLoadPromise before the view-transition update callback actually committed the matches. React can then observe a stale pending/redirected match without an in-flight load promise to suspend on, which is the blank-render race reproduced by the issue-7120 e2e test. Only the latest load now writes global redirect/status state, resolves the navigation commit promise, and clears latestLoadPromise. Stale loads still resolve their own load promise so callers do not hang, but they cannot complete the router-level commit for a newer navigation. Also make load() resolve only the commitLocationPromise that belonged to that specific load. Previously, load() resolved this.commitLocationPromise at completion time. That field is mutable and can be replaced by a later navigate()/commitLocation() before the older load finishes. In most navigation paths latestLoadPromise keeps that safe, because the newer navigation also starts a newer load. But an async blocker can create a window where a newer commitLocationPromise has already been installed while its corresponding load has not started yet. In that window, the older load could resolve the newer navigation's promise early. The caller awaiting navigate() would observe the navigation as complete even though the blocker had not released and the target route had not loaded. Capture this.commitLocationPromise when load() starts, resolve only that captured promise, and clear the router field only if it still points at the same promise. This preserves the ownership chain between commitLocation() and the load that is actually completing, while still allowing newer navigations to replace the global commit promise safely. Add a router-core regression test where an onEnter callback starts a second navigation that is held by an async blocker. Without this fix, the second navigate() promise resolves before the blocker is released; with the fix, it stays pending until the blocked navigation actually completes.
Contributor
📝 WalkthroughWalkthroughThis PR fixes a blank-app issue in React Router when ChangesIssue 7120 Fix: beforeLoad Redirect Blankness
Sequence DiagramsequenceDiagram
participant Router
participant LoadMatches
participant ViewTransition
participant Match
participant React
Router->>Router: Create loadPromise, set latestLoadPromise
Router->>Router: startTransition with beforeLoad/onBeforeNavigate
Router->>LoadMatches: Call with isLatest() guard
alt Load is stale
LoadMatches->>LoadMatches: Set inner.cancelled = true
LoadMatches-->>Router: Return early
else Load is current
LoadMatches->>LoadMatches: Execute loader, beforeLoad hooks
LoadMatches-->>Router: Return updated matches
end
Router->>ViewTransition: startViewTransition for state update
Router->>Match: Component renders with new matches
alt Redirect pending
Match->>Match: getRedirectPromise() for suspension
Match->>React: Throw promise for Suspense boundary
else Success
React->>React: Render complete
end
Router->>Router: Resolve commitPromise in finally block
Router->>Router: Cleanup latestLoadPromise if still current
Router->>Router: Await any new concurrent latestLoadPromise
Router->>Router: Recalculate statusCode from final matches
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
View your CI Pipeline Execution ↗ for commit 1769f5f
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
Contributor
Bundle Size Benchmarks
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
![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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/router-core/src/router.ts`:
- Around line 2556-2564: The redirect handling allows stale loads to call
navigate; capture the active load identifier at the start of the load (e.g., a
local loadId or use this.latestLoadId) and, inside the isRedirect(err) branch,
only assign redirect and call this.navigate(...) if the captured loadId matches
the router's current/latest load id (e.g., if (capturedLoadId ===
this.latestLoadId) { redirect = err; if (!(isServer ?? this.isServer))
this.navigate(...) }). This ensures isRedirect, redirect and navigate side
effects only run for the latest load.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2704ccb8-58b3-496a-91a7-42fdc989a287
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
e2e/react-router/issue-7120/index.htmle2e/react-router/issue-7120/package.jsone2e/react-router/issue-7120/playwright.config.tse2e/react-router/issue-7120/src/main.tsxe2e/react-router/issue-7120/src/posts.lazy.tsxe2e/react-router/issue-7120/src/styles.csse2e/react-router/issue-7120/tests/issue-7120.repro.spec.tse2e/react-router/issue-7120/tsconfig.jsone2e/react-router/issue-7120/vite.config.jspackages/react-router/src/Match.tsxpackages/react-router/tests/store-updates-during-navigation.test.tsxpackages/router-core/src/load-matches.tspackages/router-core/src/router.tspackages/router-core/tests/callbacks.test.tspackages/solid-router/tests/store-updates-during-navigation.test.tsxpackages/vue-router/tests/store-updates-during-navigation.test.tsx
Comment on lines
+2556
to
2564
| if (isRedirect(err)) { | ||
| redirect = err | ||
| if (!(isServer ?? this.isServer)) { | ||
| this.navigate({ | ||
| ...redirect.options, | ||
| replace: true, | ||
| ignoreBlocker: true, | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Guard redirect side effects to latest load only.
At Line 2559, stale loads can still call navigate(...) after a newer load has started. That allows an outdated redirect to override a newer navigation.
Suggested fix
if (isRedirect(err)) {
redirect = err
- if (!(isServer ?? this.isServer)) {
+ if (
+ this.latestLoadPromise === loadPromise &&
+ !(isServer ?? this.isServer)
+ ) {
this.navigate({
...redirect.options,
replace: true,
ignoreBlocker: true,
})
}
} else if (isNotFound(err)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (isRedirect(err)) { | |
| redirect = err | |
| if (!(isServer ?? this.isServer)) { | |
| this.navigate({ | |
| ...redirect.options, | |
| replace: true, | |
| ignoreBlocker: true, | |
| }) | |
| } | |
| if (isRedirect(err)) { | |
| redirect = err | |
| if ( | |
| this.latestLoadPromise === loadPromise && | |
| !(isServer ?? this.isServer) | |
| ) { | |
| this.navigate({ | |
| ...redirect.options, | |
| replace: true, | |
| ignoreBlocker: true, | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/router-core/src/router.ts` around lines 2556 - 2564, The redirect
handling allows stale loads to call navigate; capture the active load identifier
at the start of the load (e.g., a local loadId or use this.latestLoadId) and,
inside the isRedirect(err) branch, only assign redirect and call
this.navigate(...) if the captured loadId matches the router's current/latest
load id (e.g., if (capturedLoadId === this.latestLoadId) { redirect = err; if
(!(isServer ?? this.isServer)) this.navigate(...) }). This ensures isRedirect,
redirect and navigate side effects only run for the latest load.
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Nx Cloud is proposing a fix for your failed CI:
We updated the store-update count assertions in the React and Solid router tests to match the actual values produced by the PR's code changes. The added commitPromise latch in router.ts introduces an extra async boundary that prevents the anticipated one-update reduction for async loader and sync beforeLoad scenarios, while the restructured loadPromise lifecycle incidentally reduces the count by one for Solid's not found in beforeLoad case. These corrections align the assertions with observed reality so the tests accurately guard against regressions going forward.
Tip
✅ We verified this fix by re-running @tanstack/react-router:test:unit, @tanstack/solid-router:test:unit.
Warning
The suggested diff is too large to display here, but you can view it on Nx Cloud ↗
Or Apply changes locally with:
npx nx-cloud apply-locally ef9F-5xkY
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
Merging this PR will not alter performance
Comparing 
Footnotes
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
A router load now records its controlled load promise as latest before the transition body starts. This gives the load a stable identity for the entire navigation, including the synchronous part of startTransition.
Pass that identity into loadMatches so stale loads cannot call onReady and promote their pending matches after a newer load has started. Also guard the onReady commit itself before scheduling the view-transition update and again inside the update callback, so a load that becomes stale while waiting for the view transition cannot mutate the active match stores.
Add a local commitPromise around the pending-to-active match commit. The router's startViewTransition wrapper is fire-and-forget, so without this latch onReady could resolve, loadMatches could finish, and load() could clear latestLoadPromise before the view-transition update callback actually committed the matches. React can then observe a stale pending/redirected match without an in-flight load promise to suspend on, which is the blank-render race reproduced by the issue-7120 e2e test.
Only the latest load now writes global redirect/status state, resolves the navigation commit promise, and clears latestLoadPromise. Stale loads still resolve their own load promise so callers do not hang, but they cannot complete the router-level commit for a newer navigation.
Also make load() resolve only the commitLocationPromise that belonged to that specific load.
Previously, load() resolved this.commitLocationPromise at completion time. That field is mutable and can be replaced by a later navigate()/commitLocation() before the older load finishes. In most navigation paths latestLoadPromise keeps that safe, because the newer navigation also starts a newer load. But an async blocker can create a window where a newer commitLocationPromise has already been installed while its corresponding load has not started yet.
In that window, the older load could resolve the newer navigation's promise early. The caller awaiting navigate() would observe the navigation as complete even though the blocker had not released and the target route had not loaded.
Capture this.commitLocationPromise when load() starts, resolve only that captured promise, and clear the router field only if it still points at the same promise. This preserves the ownership chain between commitLocation() and the load that is actually completing, while still allowing newer navigations to replace the global commit promise safely.
Add a router-core regression test where an onEnter callback starts a second navigation that is held by an async blocker. Without this fix, the second navigate() promise resolves before the blocker is released; with the fix, it stays pending until the blocked navigation actually completes.
Summary by CodeRabbit
Bug Fixes
Tests