fix(router-core): preserve replacement promises on loader abort#7814
fix(router-core): preserve replacement promises on loader abort#7814LadyBluenotes wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughLoader matching now tracks controlled promises per route match, prevents stale aborted loads from clearing replacement promises, and coordinates loader finalization. A regression test covers rapid search updates with reused parent matches. ChangesLoader promise lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 ebca5c7
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview1 package(s) bumped directly, 22 bumped as dependents. 🟩 Patch bumps
|
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. |
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 has identified a possible root cause for your failed CI:
We determined this failure is unrelated to the PR's changes. The tanstack-solid-start-e2e-basic e2e test failed because port 44231 was already in use in the CI environment (EADDRINUSE), preventing the test server from starting. Our analysis confirms the PR only touched @tanstack/react-router and @tanstack/router-core, so this is an environment-level issue that requires a re-run.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router-core/src/load-matches.ts (1)
777-784: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the
loaderPromisereset in the outer catchpackages/router-core/src/load-matches.ts:777-784still clearsmatch._nonReactive.loaderPromiseunconditionally; that can wipe out a newer in-flight load’s promise.🤖 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/load-matches.ts` around lines 777 - 784, Update the outer catch around handleRedirectAndNotFound to reset match._nonReactive.loaderPromise only when it still refers to the failed load’s promise, preserving any newer in-flight loader promise. Use the existing loader promise captured by the surrounding load flow and retain the missing-match guard.
🤖 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/react-router/tests/loaders.test.tsx`:
- Around line 1013-1023: Update the test around the filter-change loop to verify
that the root loader actually reruns for the reused parent match, rather than
only asserting its in-flight request was aborted. Assert the root loader mock’s
call count or configure the root route to depend on the navigation, while
preserving the existing abort and error-component assertions.
---
Outside diff comments:
In `@packages/router-core/src/load-matches.ts`:
- Around line 777-784: Update the outer catch around handleRedirectAndNotFound
to reset match._nonReactive.loaderPromise only when it still refers to the
failed load’s promise, preserving any newer in-flight loader promise. Use the
existing loader promise captured by the surrounding load flow and retain the
missing-match guard.
🪄 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: 163b889e-c1dd-4e3c-8e15-7c2e75806f38
📒 Files selected for processing (2)
packages/react-router/tests/loaders.test.tsxpackages/router-core/src/load-matches.ts
Merging this PR will degrade performance by 24.12%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem server server-fn-churn (vue) |
274.8 KB | 4,248.2 KB | -93.53% |
| ❌ | Memory | mem server error-paths redirect (vue) |
304.3 KB | 778.3 KB | -60.91% |
| ❌ | Memory | mem server error-paths redirect (solid) |
441 KB | 832.7 KB | -47.03% |
| ❌ | Memory | mem server error-paths unmatched (react) |
641.2 KB | 770.4 KB | -16.77% |
| ❌ | Simulation | client-preload interaction loop (react) |
57.3 ms | 60.6 ms | -5.41% |
| ❌ | Simulation | client-route-tree-scale navigation loop (react) |
74.2 ms | 77.2 ms | -3.9% |
| ❌ | Memory | mem server error-paths not-found (react) |
266.8 KB | 276.3 KB | -3.44% |
| ⚡ | Memory | mem server peak-large-page (react) |
2,080.9 KB | 953.8 KB | ×2.2 |
| ⚡ | Memory | mem server serialization-payload (react) |
5.3 MB | 3.4 MB | +53.32% |
| ⚡ | Simulation | ssr selective (react) |
64.3 ms | 61.9 ms | +3.78% |
| ⚡ | Simulation | ssr selective (solid) |
74.7 ms | 72 ms | +3.7% |
| ⚡ | Simulation | ssr streaming deferred (react) |
73.1 ms | 70.7 ms | +3.42% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/abort-error-loader-race (ebca5c7) with main (11a6a0d)
Summary by CodeRabbit