fix(router-core): avoid preload cleanup crashes after invalidation 2#7006
fix(router-core): avoid preload cleanup crashes after invalidation 2#7006
Conversation
📝 WalkthroughWalkthroughThis PR enhances route preload robustness by preventing cache eviction errors. Changes include adding match availability guards, improving promise handling centralization, updating cancellation semantics to resolve pending promises, validating preload results correspond to router state, and verifying cache lifecycle operations don't break in-flight preloads. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 14m 51s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 48s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-03-21 17:43:42 UTC
🚀 Changeset Version Preview1 package(s) bumped directly, 21 bumped as dependents. 🟩 Patch bumps
|
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
![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: 4bf77c51a6
ℹ️ 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".
| const match = inner.router.getMatch(matchId) | ||
| if (!match) return |
There was a problem hiding this comment.
beforeLoad when the match was already canceled
cancelMatch() now resolves _nonReactive.beforeLoadPromise/loaderPromise before aborting the match (packages/router-core/src/router.ts:1790-1794). That wakes any concurrent load that was blocked in preBeforeLoadSetup(), but this entry point only checks for a missing match, not an aborted one. If the canceled match is still present in the active/pending pool (for example during an invalidate() or same-URL reload while an async beforeLoad is in flight), the stale load continues into route.options.beforeLoad and can still redirect or mutate __beforeLoadContext after the navigation was canceled.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/router.ts (1)
1541-1577:⚠️ Potential issue | 🟠 MajorPreserve the previous
loadPromisewhen reviving an aborted match.
load-matches.tschains the next suspense promise onto the currentloadPromiseso older waiters are released when the retry completes. This reset branch always replacesloadPromise, which severs that chain and can leave an already-suspended match waiting on an orphaned promise after an invalidate/cancel+retry cycle.💡 Suggested fix
+ const preservedLoadPromise = + existingMatch._nonReactive.loadPromise ?? + createControlledPromise<void>() + match = { ...existingMatch, cause, @@ _nonReactive: { ...existingMatch._nonReactive, - loadPromise: - existingMatch._nonReactive.loadPromise ?? - createControlledPromise(), + loadPromise: preservedLoadPromise, }, ...(shouldResetExistingMatch ? { @@ _nonReactive: { ...existingMatch._nonReactive, beforeLoadPromise: undefined, loaderPromise: undefined, pendingTimeout: undefined, - loadPromise: createControlledPromise(), + loadPromise: preservedLoadPromise, displayPendingPromise: undefined, minPendingPromise: undefined, dehydrated: undefined, error: undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/router.ts` around lines 1541 - 1577, When reviving an aborted match we currently always replace existingMatch._nonReactive.loadPromise with createControlledPromise(), which breaks the chaining used by load-matches.ts and can orphan older waiters; update the reset branch that runs when shouldResetExistingMatch is true to preserve existingMatch._nonReactive.loadPromise if it exists (i.e., leave loadPromise as existingMatch._nonReactive.loadPromise rather than unconditionally creating a new one), while still resetting other nonReactive fields like beforeLoadPromise, loaderPromise, pendingTimeout, displayPendingPromise, minPendingPromise, dehydrated, and error; reference the match construction that spreads existingMatch and the nested _nonReactive block to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/router-core/src/router.ts`:
- Around line 2798-2820: The code re-filters cachedMatches after calling
cancelMatch, but cancelMatch mutates match objects so the second filter can
fail; instead, collect the ids of matchesToClear (e.g., const idsToClear = new
Set(matchesToClear.map(m => m.id))), call this.cancelMatch for each match, and
then call this.stores.setCachedMatches(cachedMatches.filter(m =>
!idsToClear.has(m.id))); update references to matchesToClear, cachedMatches,
cancelMatch, and this.stores.setCachedMatches accordingly so removal is done by
id rather than re-running the predicate that may observe mutated _nonReactive
fields.
---
Outside diff comments:
In `@packages/router-core/src/router.ts`:
- Around line 1541-1577: When reviving an aborted match we currently always
replace existingMatch._nonReactive.loadPromise with createControlledPromise(),
which breaks the chaining used by load-matches.ts and can orphan older waiters;
update the reset branch that runs when shouldResetExistingMatch is true to
preserve existingMatch._nonReactive.loadPromise if it exists (i.e., leave
loadPromise as existingMatch._nonReactive.loadPromise rather than
unconditionally creating a new one), while still resetting other nonReactive
fields like beforeLoadPromise, loaderPromise, pendingTimeout,
displayPendingPromise, minPendingPromise, dehydrated, and error; reference the
match construction that spreads existingMatch and the nested _nonReactive block
to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93789982-c782-477e-8e9b-f8ea9eb6d6c9
📒 Files selected for processing (5)
.changeset/moody-kings-travel.mdpackages/react-router/src/Match.tsxpackages/router-core/src/load-matches.tspackages/router-core/src/router.tspackages/router-core/tests/load.test.ts
| const matchesToClear = | ||
| filter !== undefined | ||
| ? cachedMatches.filter((match) => | ||
| filter(match as MakeRouteMatchUnion<this>), | ||
| ) | ||
| : cachedMatches | ||
|
|
||
| matchesToClear.forEach((match) => { | ||
| if ( | ||
| match.status === 'pending' || | ||
| match.isFetching || | ||
| match._nonReactive.beforeLoadPromise || | ||
| match._nonReactive.loaderPromise || | ||
| match._nonReactive.loadPromise | ||
| ) { | ||
| this.cancelMatch(match.id) | ||
| } | ||
| }) | ||
|
|
||
| if (filter !== undefined) { | ||
| this.stores.setCachedMatches( | ||
| this.stores.cachedMatchesSnapshot.state.filter( | ||
| (m) => !filter(m as MakeRouteMatchUnion<this>), | ||
| ), | ||
| cachedMatches.filter((m) => !filter(m as MakeRouteMatchUnion<this>)), | ||
| ) |
There was a problem hiding this comment.
Remove cleared matches by id instead of re-running the filter.
cancelMatch() mutates the same match objects before the second filter(...) runs. If a caller's filter depends on _nonReactive promise fields or abort state, the entry can stop matching and remain in cachedMatches even though it was selected for clearing.
💡 Suggested fix
const matchesToClear =
filter !== undefined
? cachedMatches.filter((match) =>
filter(match as MakeRouteMatchUnion<this>),
)
: cachedMatches
+ const matchIdsToClear = new Set(matchesToClear.map((match) => match.id))
@@
if (filter !== undefined) {
this.stores.setCachedMatches(
- cachedMatches.filter((m) => !filter(m as MakeRouteMatchUnion<this>)),
+ cachedMatches.filter((m) => !matchIdsToClear.has(m.id)),
)
} else {📝 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.
| const matchesToClear = | |
| filter !== undefined | |
| ? cachedMatches.filter((match) => | |
| filter(match as MakeRouteMatchUnion<this>), | |
| ) | |
| : cachedMatches | |
| matchesToClear.forEach((match) => { | |
| if ( | |
| match.status === 'pending' || | |
| match.isFetching || | |
| match._nonReactive.beforeLoadPromise || | |
| match._nonReactive.loaderPromise || | |
| match._nonReactive.loadPromise | |
| ) { | |
| this.cancelMatch(match.id) | |
| } | |
| }) | |
| if (filter !== undefined) { | |
| this.stores.setCachedMatches( | |
| this.stores.cachedMatchesSnapshot.state.filter( | |
| (m) => !filter(m as MakeRouteMatchUnion<this>), | |
| ), | |
| cachedMatches.filter((m) => !filter(m as MakeRouteMatchUnion<this>)), | |
| ) | |
| const matchesToClear = | |
| filter !== undefined | |
| ? cachedMatches.filter((match) => | |
| filter(match as MakeRouteMatchUnion<this>), | |
| ) | |
| : cachedMatches | |
| const matchIdsToClear = new Set(matchesToClear.map((match) => match.id)) | |
| matchesToClear.forEach((match) => { | |
| if ( | |
| match.status === 'pending' || | |
| match.isFetching || | |
| match._nonReactive.beforeLoadPromise || | |
| match._nonReactive.loaderPromise || | |
| match._nonReactive.loadPromise | |
| ) { | |
| this.cancelMatch(match.id) | |
| } | |
| }) | |
| if (filter !== undefined) { | |
| this.stores.setCachedMatches( | |
| cachedMatches.filter((m) => !matchIdsToClear.has(m.id)), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/router-core/src/router.ts` around lines 2798 - 2820, The code
re-filters cachedMatches after calling cancelMatch, but cancelMatch mutates
match objects so the second filter can fail; instead, collect the ids of
matchesToClear (e.g., const idsToClear = new Set(matchesToClear.map(m =>
m.id))), call this.cancelMatch for each match, and then call
this.stores.setCachedMatches(cachedMatches.filter(m => !idsToClear.has(m.id)));
update references to matchesToClear, cachedMatches, cancelMatch, and
this.stores.setCachedMatches accordingly so removal is done by id rather than
re-running the predicate that may observe mutated _nonReactive fields.
1 participant
Alternative solution to #7003
Summary by CodeRabbit
Bug Fixes
Tests