fix(router-core): remove navigation rollback - #8092
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 afc5b7c
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview2 package(s) bumped directly, 21 bumped as dependents. 🟩 Patch bumps
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 536bbc4382
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Preserve the settlement turn in which immediately completed background | ||
| // work can publish before callers resume from `load`. | ||
| .then(), |
There was a problem hiding this comment.
When commit throws—for example, when an application onEnter, onStay, or onLeave callback throws inside commitMatches—this empty .then() propagates the rejection without finalizing the current transaction. router.load()/navigate() then rejects while the router remains pending and _commitPromise remains unresolved, even though the new matches may already have been published, leaving subsequent consumers with a permanently loading router. Preserve a rejection cleanup path that settles transaction state without necessarily rolling back the publication.
Useful? React with 👍 / 👎.
Bundle Size Benchmarks
The following scenarios have bundle-size changes compared with the baseline:
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. |
Merging this PR will degrade performance by 2.34%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem server error-paths redirect (solid) |
391.8 KB | 1,058.1 KB | -62.97% |
| ❌ | Memory | mem server peak-large-page (react) |
1.2 MB | 2.2 MB | -47.12% |
| ❌ | Memory | mem server aborted-requests (vue) |
983.8 KB | 1,148.1 KB | -14.31% |
| ❌ | Memory | mem server error-paths not-found (vue) |
485.3 KB | 551.8 KB | -12.04% |
| ❌ | Simulation | client-nested-params navigation loop (react) |
211.4 ms | 226.5 ms | -6.68% |
| ❌ | Memory | mem client interrupted-navigations (vue) |
461 KB | 480.6 KB | -4.08% |
| ❌ | Memory | mem server request-churn (solid) |
692.8 KB | 721.2 KB | -3.95% |
| ⚡ | Memory | mem server error-paths unmatched (react) |
789.7 KB | 431.3 KB | +83.12% |
| ⚡ | Memory | mem server error-paths redirect (vue) |
467.9 KB | 396.2 KB | +18.1% |
| ⚡ | Memory | mem server error-paths not-found (react) |
457.9 KB | 419.6 KB | +9.14% |
| ⚡ | Memory | mem server error-paths not-found (solid) |
623.9 KB | 576.4 KB | +8.24% |
| ⚡ | Simulation | client-route-tree-scale navigation loop (react) |
161.5 ms | 150.2 ms | +7.52% |
| ⚡ | Memory | mem server server-fn-churn (vue) |
373.4 KB | 347.3 KB | +7.51% |
| ⚡ | Simulation | ssr global-mw document (vue) |
970.4 ms | 905.3 ms | +7.19% |
| ⚡ | Memory | mem client preload-churn (solid) |
324.5 KB | 305 KB | +6.37% |
| ⚡ | Simulation | client-control-flow navigation loop (vue) |
125.2 ms | 118.9 ms | +5.3% |
| ⚡ | Simulation | client-history navigation loop (react) |
128.1 ms | 123.1 ms | +4.08% |
| ⚡ | Simulation | client-history navigation loop (solid) |
153.5 ms | 147.5 ms | +4.02% |
| ⚡ | Memory | mem server error-paths redirect (react) |
314 KB | 302.3 KB | +3.88% |
| ⚡ | Memory | mem client navigation-churn (solid) |
705.5 KB | 679.3 KB | +3.86% |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing opencode/remove-all-rollback-v5 (afc5b7c) with opencode/retained-pending-architecture-v5 (425bab3)
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud is proposing a fix for your failed CI:
We updated the HMR e2e test to align with the intentional removal of navigation rollback in router-core. The assertion expecting the crumb to revert to 'Child' after a failed onStay hook has been corrected to 'Child Failed', since one-way navigation now commits the new loader state regardless of lifecycle hook errors. This brings the test in sync with the PR's stated goal of replacing rollback checkpoints with successor-owned navigation.
Tip
✅ We verified this fix by re-running tanstack-react-start-e2e-hmr:test:e2e--rsbuild-ssr.
diff --git a/e2e/react-start/hmr/tests/app.spec.ts b/e2e/react-start/hmr/tests/app.spec.ts
index 69ce1d20..32a98ceb 100644
--- a/e2e/react-start/hmr/tests/app.spec.ts
+++ b/e2e/react-start/hmr/tests/app.spec.ts
@@ -684,7 +684,7 @@ test.describe('react-start hmr', () => {
await expect(page.getByTestId('child')).toHaveText('child')
})
- test('rolls back a failed route refresh and accepts the next HMR update', async ({
+ test('accepts the next HMR update after a failed route refresh', async ({
page,
}) => {
await page.goto('/child')
@@ -712,7 +712,7 @@ test.describe('react-start hmr', () => {
},
)
- await expect(page.getByTestId('crumb-/child')).toHaveText('Child')
+ await expect(page.getByTestId('crumb-/child')).toHaveText('Child Failed')
await expect(page.getByTestId('root-message')).toHaveValue(
'preserved through failure',
)
Or Apply changes locally with:
npx nx-cloud apply-locally W28Y-FjUD
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
1 participant
Summary
Failure Behavior Without Rollback
There is no catch-all that sends failures previously covered by rollback to the root error boundary. Failures now have distinct outcomes depending on where they occur.
Route-phase errors
Errors from route context,
beforeLoad, loaders, validation,shouldReload, and component chunk loading are normalized into route outcomes. They call the originating routeonError, mark the failing match asstatus: "error", and commit the new lane so that the route error boundary or default error component renders. The error belongs to the failing match; it is not automatically moved to the root match.Relevant paths:
packages/router-core/src/load-client.ts:313andpackages/router-core/src/load-client.ts:1166.Updater errors
router.navigate({ search: () => { throw ... } })fails whilebuildLocationis running, before history or transaction publication. The returned promise rejects and the current presentation remains unchanged.beforeLoadredirect is converted into an error on the redirect originating route. Its route error boundary renders.router.buildLocationcall throws to its caller.Relevant paths:
packages/router-core/src/router.ts:2228andpackages/router-core/src/load-client.ts:1061.Abort calls
A loader receives a loader-flight controller. If it calls
.abort()and then returns data, that data is still accepted. If it throws afterward, the throw becomes a normal route error.Route context,
beforeLoad, and planning hooks receive the transaction controller. This controller is router-owned. Router-driven aborts are paired with a successor transaction: the obsolete lane releases its resources and the successor converges without restoring an older snapshot. Calling.abort()from userland without starting a successor is not a supported cancellation mechanism; it is not converted into a route error or root boundary.Relevant path:
packages/router-core/src/load-client.ts:621.View-transition and publication errors
Errors from
document.startViewTransition, a view-transitiontypesupdater,updateCallbackDone, or the frameworkstartTransitionwrapper are not converted into route errors.runClientTransactionrejects, androuter.load()can reject. Depending on when the error occurs, the old matches may remain or the new matches may already be published. There is no rollback to the previous presentation.These failures do not currently have a general publication finalizer: router status or the navigation commit promise can remain pending until a successor navigation takes ownership.
Relevant paths:
packages/router-core/src/load-client.ts:1851andpackages/router-core/src/load-client.ts:1899.Lifecycle and event errors
route.onErrorparticipates in route-error normalization. If it throws, the replacement value is normalized as the route outcome.onEnter,onStay, andonLeaverun after the new matches have been committed. If one throws, the exception escapes publication; it is not rolled back or converted into a route boundary, and finalization can remain incomplete.onLoadandonResolvedare caught, logged, and ignored so later subscribers and navigations can continue.Relevant paths:
packages/router-core/src/load-client.ts:1650andpackages/router-core/src/router.ts:1370.In short: expected loading failures become errors on the originating route, superseded work is discarded in favor of its successor, and pre-navigation construction errors reject before publication. View-transition and post-commit route lifecycle exceptions are intentionally not translated into root-boundary errors and currently have no generic terminal recovery path.
Testing
CI=1 NX_DAEMON=false pnpm nx run @tanstack/router-core:test:unit --outputStyle=stream --skipRemoteCache(1,589 passed, 3 expected failures)CI=1 NX_DAEMON=false pnpm nx run @tanstack/react-router:test:unit --outputStyle=stream --skipRemoteCache(1,026 passed, 1 skipped)CI=1 NX_DAEMON=false pnpm nx run @tanstack/solid-router:test:unit --outputStyle=stream --skipRemoteCache(864 passed, 1 skipped across client/server)CI=1 NX_DAEMON=false pnpm nx run @tanstack/vue-router:test:unit --outputStyle=stream --skipRemoteCache(846 passed, 1 skipped)CI=1 NX_DAEMON=false pnpm nx run @tanstack/router-core:build --outputStyle=stream --skipRemoteCachepnpm formatandgit diff --checkBundle Size
Exact-base minimal gzip comparison: React +7 B, Solid +54 B, Vue +24 B.