fix(router-core): avoid preload cleanup crashes after invalidation#7003
fix(router-core): avoid preload cleanup crashes after invalidation#7003
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughCapture the match used for stale-while-revalidate preloads, handle cases where the match is removed during in-flight preload by cleaning up loader promises and related state, and add tests and a changeset ensuring cache eviction during preload does not error. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 unit tests (beta)
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 10m 42s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 48s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-03-21 17:36:55 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/router-core/tests/load.test.ts (1)
442-502: Good regression test for invalidate during in-flight preload.The test properly exercises the second scenario from the PR objectives, verifying that
router.invalidate()doesn't cause errors when a preload is still in-flight.Minor suggestion: Consider wrapping the spy cleanup in a
try/finallyor usingafterEachto ensuremockRestore()runs even if assertions fail. This prevents spy leakage affecting subsequent tests.+ test('does not error when invalidate clears an in-flight preload', async () => { + const consoleErrorSpy = vi + .spyOn(console, 'error') + .mockImplementation(() => {}) + + try { // ... test body ... + + expect(consoleErrorSpy).not.toHaveBeenCalled() + // ... remaining assertions ... + } finally { + consoleErrorSpy.mockRestore() + } + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/tests/load.test.ts` around lines 442 - 502, Wrap the console.error spy setup/cleanup in a safe teardown to guarantee restore even on test failures: move the vi.spyOn(...).mockImplementation to the start of the test and ensure mockRestore() is executed in a finally block (or register the spy with an afterEach cleanup) so consoleErrorSpy.mockRestore() always runs; reference the existing consoleErrorSpy variable and its mockRestore call in the "does not error when invalidate clears an in-flight preload" test in packages/router-core/tests/load.test.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/router-core/tests/load.test.ts`:
- Around line 442-502: Wrap the console.error spy setup/cleanup in a safe
teardown to guarantee restore even on test failures: move the
vi.spyOn(...).mockImplementation to the start of the test and ensure
mockRestore() is executed in a finally block (or register the spy with an
afterEach cleanup) so consoleErrorSpy.mockRestore() always runs; reference the
existing consoleErrorSpy variable and its mockRestore call in the "does not
error when invalidate clears an in-flight preload" test in
packages/router-core/tests/load.test.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2b1be56-69c4-4523-a55d-6a979300cb85
📒 Files selected for processing (2)
packages/router-core/src/load-matches.tspackages/router-core/tests/load.test.ts
![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: ce450adb9a
ℹ️ 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".
| if (!match) { | ||
| return inner.matches[index]! |
There was a problem hiding this comment.
If a loader ignores abortController and the user navigates away before it settles, beforeLoad() for the newer navigation replaces the old pending match (router.ts:2362-2369). Hitting this fallback turns the canceled loadRouteMatch() into a success, so loadMatches() still reaches triggerOnReady(), and the onReady callback in router.ts:2409-2490 commits whatever newer pendingMatchesSnapshot exists at that moment. That can mount the second navigation before its own loaders have finished.
Useful? React with 👍 / 👎.
| if (!match) { | ||
| return inner.matches[index]! |
There was a problem hiding this comment.
For preloads, preloadRoute()'s custom updateMatch writes loader results into the router store rather than mutating inner.matches (router.ts:2843-2849). If cache GC or invalidate() evicts the match before the loader finishes, this fallback returns the original inner.matches[index]!, whose status and loaderData are still stale (pending with no data). Any caller that awaits router.preloadRoute() and consumes the returned matches will get an incorrect result instead of a missing/failed preload.
Useful? React with 👍 / 👎.
Sheraff
force-pushed
the
opencode/shiny-orchid
branch
from
3e988de to
911a46e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/load-matches.ts`:
- Around line 946-961: The eviction cleanup must run even when loadRouteMatch
takes the loaderShouldRunAsync/loaderIsRunningAsync early-return path; attach a
background cleanup that executes after the detached loader finishes and checks
inner.router.getMatch(matchId) and then performs the same steps currently inside
the if-block (resolve preloadCleanupMatch._nonReactive.loaderPromise and
.loadPromise, clear pendingTimeout, unset loaderPromise/loadPromise/dehydrated).
Concretely, in the loaderShouldRunAsync branch of loadRouteMatch, before
returning, register a finally/then handler on
prevMatch._nonReactive.loaderPromise (or create a small async detached task)
that re-checks inner.router.getMatch(matchId) and, if missing, runs the existing
cleanup logic for preloadCleanupMatch (resolving promises and clearing
nonReactive fields) so a subsequent preload won't hang waiting on an orphaned
loaderPromise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2242935-144c-4829-bd58-15bb69b780c0
📒 Files selected for processing (1)
packages/router-core/src/load-matches.ts
| const currentMatch = inner.router.getMatch(matchId) | ||
| if (!currentMatch && inner.preload && preloadCleanupMatch) { | ||
| if (!loaderIsRunningAsync) { | ||
| preloadCleanupMatch._nonReactive.loaderPromise?.resolve() | ||
| preloadCleanupMatch._nonReactive.loadPromise?.resolve() | ||
| preloadCleanupMatch._nonReactive.loadPromise = undefined | ||
| } | ||
|
|
||
| clearTimeout(preloadCleanupMatch._nonReactive.pendingTimeout) | ||
| preloadCleanupMatch._nonReactive.pendingTimeout = undefined | ||
| if (!loaderIsRunningAsync) { | ||
| preloadCleanupMatch._nonReactive.loaderPromise = undefined | ||
| } | ||
| preloadCleanupMatch._nonReactive.dehydrated = undefined | ||
|
|
||
| return preloadCleanupMatch |
There was a problem hiding this comment.
Cleanup still misses evictions after the background reload has already detached.
This branch only runs while the current loadRouteMatch() call is still executing. In the loaderShouldRunAsync path, the function returns first, so an eviction that happens afterwards never reaches this cleanup. If another preload starts in that window, it will block on the existing prevMatch._nonReactive.loaderPromise, and the detached task still resolves that promise only via inner.router.getMatch(matchId). Once the store entry is gone, that promise is orphaned and the second preload can hang indefinitely.
🔧 Suggested direction
} else if (
loaderShouldRunAsync &&
!inner.sync &&
shouldReloadInBackground
) {
loaderIsRunningAsync = true
+ const matchForCleanup = prevMatch
;(async () => {
try {
await runLoader(inner, matchPromises, matchId, index, route)
- const match = inner.router.getMatch(matchId)!
- match._nonReactive.loaderPromise?.resolve()
- match._nonReactive.loadPromise?.resolve()
- match._nonReactive.loaderPromise = undefined
- match._nonReactive.loadPromise = undefined
} catch (err) {
if (isRedirect(err)) {
await inner.router.navigate(err.options)
}
+ } finally {
+ const match = inner.router.getMatch(matchId) ?? matchForCleanup
+ match._nonReactive.loaderPromise?.resolve()
+ match._nonReactive.loadPromise?.resolve()
+ match._nonReactive.loaderPromise = undefined
+ match._nonReactive.loadPromise = undefined
}
})()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/router-core/src/load-matches.ts` around lines 946 - 961, The
eviction cleanup must run even when loadRouteMatch takes the
loaderShouldRunAsync/loaderIsRunningAsync early-return path; attach a background
cleanup that executes after the detached loader finishes and checks
inner.router.getMatch(matchId) and then performs the same steps currently inside
the if-block (resolve preloadCleanupMatch._nonReactive.loaderPromise and
.loadPromise, clear pendingTimeout, unset loaderPromise/loadPromise/dehydrated).
Concretely, in the loaderShouldRunAsync branch of loadRouteMatch, before
returning, register a finally/then handler on
prevMatch._nonReactive.loaderPromise (or create a small async detached task)
that re-checks inner.router.getMatch(matchId) and, if missing, runs the existing
cleanup logic for preloadCleanupMatch (resolving promises and clearing
nonReactive fields) so a subsequent preload won't hang waiting on an orphaned
loaderPromise.
1 participant
Summary
loadRouteMatchso preload cleanup does not crash when an in-flight match has already been evicted from cacherouter.invalidate()clearing an in-flight preload_nonReactiveconsole error reported after the recent preload changesTesting
CI=1 NX_DAEMON=false pnpm nx run @tanstack/router-core:test:unit --outputStyle=stream --skipRemoteCache -- tests/load.test.tsCI=1 NX_DAEMON=false pnpm nx run @tanstack/router-core:test:types --outputStyle=stream --skipRemoteCacheSummary by CodeRabbit
Bug Fixes
Tests
Chores