fix: don't re-execute stale loaders on unrelated location changes#6867
Merged
schiller-manuel merged 9 commits intomainfrom Mar 9, 2026
Merged
fix: don't re-execute stale loaders on unrelated location changes#6867schiller-manuel merged 9 commits intomainfrom
schiller-manuel merged 9 commits intomainfrom
Conversation
github-actions
bot
added
documentation
Contributor
|
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 (2)
📝 WalkthroughWalkthroughAdds a forceStaleReload option to the load flow to enable explicit revalidation of stale matches, switches several framework route helpers to delegate to useRouteContext, updates docs about staleTime semantics, adjusts unit/e2e test expectations, and adds tests for stale-reload triggers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Router as Router\n(+router.load)
participant LoaderEngine as loadMatches\n(+forceStaleReload)
participant Match as RouteMatch
participant LoaderFn as Loader
Client->>Router: navigation or explicit router.load()
Router->>LoaderEngine: loadMatches({ forceStaleReload? })
Note right of LoaderEngine: determine per-match\nstaleness, sync context
LoaderEngine->>Match: syncMatchContext(match)
alt match needs loader
LoaderEngine->>LoaderFn: run loader (pass previousRouteMatchId)
LoaderFn-->>LoaderEngine: loader result
LoaderEngine->>Router: update match & context
else skip loader
LoaderEngine->>Router: reuse existing match/context
end
Router-->>Client: updated state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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 |
|
View your CI Pipeline Execution ↗ for commit 79eec14
☁️ Nx Cloud last updated this comment at |
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/router-core/src/router.ts (1)
2375-2407:⚠️ Potential issue | 🟠 Major
hrefequality is too broad for forced stale reloads.
previousLocation.href === next.hrefis also true for same-URL state/history transitions, so those paths will still force-revalidate stale loaders even when the loader key is unchanged. That reintroduces the same class of unrelated-change reloads this PR is trying to remove. Please thread an explicit opt-in from the call sites that intentionally revalidate instead of deriving it from location equality here. A regression test for same-href state-only navigation would lock this down.🤖 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 2375 - 2407, The current forced stale reload decision uses previousLocation.href === next.href which incorrectly treats same-URL state/history navigations as a forced reload; change loadMatches call to rely on an explicit opt-in flag (e.g., opts?.forceStaleReload) instead of deriving it from href equality, update the loadMatches parameter name if needed to match the explicit flag (forceStaleReload) and update all call sites that intentionally want to force revalidation to pass that flag, and add a regression test for a same-href state-only navigation (no loader-key change) to assert loaders are not revalidated.packages/router-core/src/load-matches.ts (1)
4-4:⚠️ Potential issue | 🟠 MajorAvoid unconditional store writes in
syncMatchContext.
buildMatchContext()always allocates, so this helper currently emits a match update even when the merged context is unchanged. That turns every stale-skip path into extra store churn/re-renders, which is why the navigation update expectations had to move up in this PR.Suggested fix
-import { createControlledPromise, isPromise } from './utils' +import { createControlledPromise, isPromise, replaceEqualDeep } from './utils'const syncMatchContext = ( inner: InnerLoadContext, matchId: string, index: number, ): void => { const nextContext = buildMatchContext(inner, index) inner.updateMatch(matchId, (prev) => { + const context = replaceEqualDeep(prev.context, nextContext) + if (context === prev.context) { + return prev + } + return { ...prev, - context: nextContext, + context, } }) }Also applies to: 201-214
🤖 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` at line 4, syncMatchContext currently always writes the merged context created by buildMatchContext, causing unnecessary store updates; change it to first compute the merged context (call buildMatchContext as now), then compare the merged context to the existing match.context (use the same equality semantics the store expects—shallow/deep as appropriate), and only perform the store write/update when they differ; update the code paths in syncMatchContext (and the similar block at lines ~201-214) to early-return when contexts are equal to avoid unconditional store writes.
🤖 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/react-router/src/fileRoute.ts`:
- Around line 232-233: The helper LazyRoute.useRouteContext currently lets
callers override the bound route by spreading opts after from; change the call
so the route ID wins by applying opts first and then setting from to
this.options.id (i.e., ensure the object passed to useRouteContext has from:
this.options.id after spreading opts), so the bound route cannot be overridden;
update the useRouteContext invocation in LazyRoute.useRouteContext accordingly.
In `@packages/react-router/src/route.tsx`:
- Around line 117-119: The bound helper useRouteContext currently spreads opts
after specifying from which lets callers override this.id at runtime; change the
call in the useRouteContext: UseRouteContextRoute<TId> implementation so that
from: this.id is placed before ...opts is spread (i.e., ensure from is first and
opts merged after it is not allowed to override it), and make the identical
ordering change for the RouteApi and RootRoute bound variants referenced in the
comment (the other wrappers at the same pattern) so all route-bound hooks keep
from pinned to this.id.
In `@packages/solid-router/src/fileRoute.ts`:
- Around line 220-221: The helper currently spreads opts after setting from,
which allows callers to override the bound route id; change the call inside
LazyRoute.useRouteContext to ensure the route id is pinned by making from take
precedence (e.g., pass { ...(opts as any), from: this.options.id } to
useRouteContext) so useRouteContext always uses this.options.id and cannot be
overridden by opts.
In `@packages/solid-router/src/route.tsx`:
- Around line 107-109: The route-bound wrapper useRouteContext:
UseRouteContextRoute<TId> currently spreads opts after from so callers can
override the bound route id; change the call to ensure from is fixed by placing
from last (e.g. call useRouteContext({ ...(opts as any), from: this.id as any })
) or otherwise prevent opts from overwriting from; apply the same fix to the
other two route-bound wrappers that follow the same pattern so the bound route
id cannot be replaced by callers.
In `@packages/vue-router/src/fileRoute.ts`:
- Around line 220-221: The helper currently allows callers to override the bound
route because it spreads opts after setting from; change the merge so the bound
route id wins. In LazyRoute.useRouteContext (the useRouteContext property) call
useRouteContext({ ...(opts as any), from: this.options.id }) or otherwise ensure
opts cannot override from (e.g. remove opts.from before merging) so the returned
context is always pinned to this.options.id.
In `@packages/vue-router/src/route.ts`:
- Around line 108-110: The bound helper currently spreads opts after setting
from, allowing a stray opts.from to override the route id; change the object
ordering so opts are spread first and from is set last (e.g., call
useRouteContext({ ...(opts as any), from: this.id as any })), and apply the same
fix to the RouteApi and RootRoute bound variants referenced elsewhere so their
from property cannot be overridden by opts.
---
Outside diff comments:
In `@packages/router-core/src/load-matches.ts`:
- Line 4: syncMatchContext currently always writes the merged context created by
buildMatchContext, causing unnecessary store updates; change it to first compute
the merged context (call buildMatchContext as now), then compare the merged
context to the existing match.context (use the same equality semantics the store
expects—shallow/deep as appropriate), and only perform the store write/update
when they differ; update the code paths in syncMatchContext (and the similar
block at lines ~201-214) to early-return when contexts are equal to avoid
unconditional store writes.
In `@packages/router-core/src/router.ts`:
- Around line 2375-2407: The current forced stale reload decision uses
previousLocation.href === next.href which incorrectly treats same-URL
state/history navigations as a forced reload; change loadMatches call to rely on
an explicit opt-in flag (e.g., opts?.forceStaleReload) instead of deriving it
from href equality, update the loadMatches parameter name if needed to match the
explicit flag (forceStaleReload) and update all call sites that intentionally
want to force revalidation to pass that flag, and add a regression test for a
same-href state-only navigation (no loader-key change) to assert loaders are not
revalidated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1eeae827-f075-41a8-8c79-112ff22bddac
📒 Files selected for processing (15)
docs/router/api/router/RouterType.mddocs/router/guide/data-loading.mdpackages/react-router/src/fileRoute.tspackages/react-router/src/route.tsxpackages/react-router/tests/store-updates-during-navigation.test.tsxpackages/router-core/src/Matches.tspackages/router-core/src/load-matches.tspackages/router-core/src/router.tspackages/router-core/tests/load.test.tspackages/solid-router/src/fileRoute.tspackages/solid-router/src/route.tsxpackages/solid-router/tests/store-updates-during-navigation.test.tsxpackages/vue-router/src/fileRoute.tspackages/vue-router/src/route.tspackages/vue-router/tests/store-updates-during-navigation.test.tsx
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/react-router/src/fileRoute.ts (1)
232-234:⚠️ Potential issue | 🟡 MinorKeep
LazyRoute.useRouteContext()pinned to its route ID.Line 233 spreads
optsafterfrom, so callers can still override the bound route at runtime. Please flip the merge order sothis.options.idalways wins.Suggested fix
useRouteContext: UseRouteContextRoute<TRoute['id']> = (opts) => { - return useRouteContext({ from: this.options.id, ...(opts as any) }) + return useRouteContext({ ...(opts as any), from: this.options.id }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-router/src/fileRoute.ts` around lines 232 - 234, The current UseRouteContextRoute implementation spreads caller opts after setting from, so callers can override the pinned route ID; update UseRouteContextRoute (LazyRoute.useRouteContext) to spread opts first and then set from: this.options.id (e.g., call useRouteContext({ ...(opts as any), from: this.options.id })) so the route ID from this.options.id always takes precedence.packages/react-router/src/route.tsx (1)
117-119:⚠️ Potential issue | 🟡 MinorKeep the bound
useRouteContexthelpers route-scoped.Lines 118, 273, and 545 spread
optsafterfrom, soRouteApi,Route, andRootRoutecallers can override the bound ID and read another route's context. These wrappers should mirror the other bound helpers and makethis.idwin.Suggested fix
useRouteContext: UseRouteContextRoute<TId> = (opts) => { - return useRouteContext({ from: this.id as any, ...(opts as any) }) + return useRouteContext({ ...(opts as any), from: this.id as any }) } useRouteContext: UseRouteContextRoute<TId> = (opts?) => { - return useRouteContext({ from: this.id, ...(opts as any) }) + return useRouteContext({ ...(opts as any), from: this.id }) } useRouteContext: UseRouteContextRoute<RootRouteId> = (opts) => { - return useRouteContext({ from: this.id, ...(opts as any) }) + return useRouteContext({ ...(opts as any), from: this.id }) }Also applies to: 272-274, 544-546
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-router/src/route.tsx` around lines 117 - 119, The bound useRouteContext helper currently spreads opts after setting from, allowing callers to override the bound ID; update the wrappers (the UseRouteContextRoute<TId> implementations used in RouteApi, Route, and RootRoute) to make this.id win by spreading opts first and then setting from: this.id (i.e. return useRouteContext({ ...(opts as any), from: this.id as any })) so the bound route ID cannot be overridden.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/react-router/src/fileRoute.ts`:
- Around line 232-234: The current UseRouteContextRoute implementation spreads
caller opts after setting from, so callers can override the pinned route ID;
update UseRouteContextRoute (LazyRoute.useRouteContext) to spread opts first and
then set from: this.options.id (e.g., call useRouteContext({ ...(opts as any),
from: this.options.id })) so the route ID from this.options.id always takes
precedence.
In `@packages/react-router/src/route.tsx`:
- Around line 117-119: The bound useRouteContext helper currently spreads opts
after setting from, allowing callers to override the bound ID; update the
wrappers (the UseRouteContextRoute<TId> implementations used in RouteApi, Route,
and RootRoute) to make this.id win by spreading opts first and then setting
from: this.id (i.e. return useRouteContext({ ...(opts as any), from: this.id as
any })) so the bound route ID cannot be overridden.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 387f50b4-f462-4c59-91f1-d241aa656000
📒 Files selected for processing (2)
packages/react-router/src/fileRoute.tspackages/react-router/src/route.tsx
Contributor
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)
817-831:⚠️ Potential issue | 🟠 MajorSync
contextbefore returning stale UI during async revalidation.This branch starts
runLoaderin the background, butmatch.contextis left stale until that loader finishes. SincebeforeLoadhas already updated__beforeLoadContext, consumers can observe the previous route context while stale data is being rendered. CallingsyncMatchContext(inner, matchId, index)before launching the async reload keeps context aligned with the new navigation.Proposed fix
} else if (loaderShouldRunAsync && !inner.sync) { + syncMatchContext(inner, matchId, index) loaderIsRunningAsync = true ;(async () => { try { await runLoader(inner, matchPromises, matchId, index, route) const match = inner.router.getMatch(matchId)!🤖 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 817 - 831, When launching the background async loader in the branch guarded by loaderShouldRunAsync && !inner.sync, ensure the match context is synced before returning stale UI: call syncMatchContext(inner, matchId, index) right before starting the async IIFE so match.context and any __beforeLoadContext updates are applied immediately; keep the existing async runLoader(inner, matchPromises, matchId, index, route) invocation and its try/catch/redirect handling unchanged, but remove the window where match.context remains stale by invoking syncMatchContext first.
🧹 Nitpick comments (1)
e2e/react-start/spa-mode/tests/app.spec.ts (1)
44-48: Minor: Explicit type annotations are redundant but acceptable.The
{ page: Page }type annotations are technically redundant since Playwright's test fixtures already provide proper typing through@tanstack/router-e2e-utils. However, this doesn't cause any issues and can improve IDE support in some configurations.Also applies to: 64-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/spa-mode/tests/app.spec.ts` around lines 44 - 48, Remove the redundant explicit TypeScript fixture annotations in the test callbacks — replace the parameter destructuring `async ({ page }: { page: Page }) =>` with `async ({ page }) =>` for the tests named "directly visiting prerendered /posts/1" and the similar test at the second occurrence (the other test using the same `{ page: Page }` annotation); this keeps Playwright's built-in typings via `@tanstack/router-e2e-utils` while avoiding duplicate type declarations and IDE noise.
🤖 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 807-815: The fallback check comparing prevMatch.id !== match.id
never fires because both vars come from router.getMatch(matchId); change the
comparison to use the previous match id captured from the prior navigation/state
instead of the current store entry. Locate where prevMatch and match are
obtained (calls to router.getMatch(matchId)) and replace the identity comparison
with something like prevMatchId !== match.id where prevMatchId is read from the
previous router state/navigation snapshot (the value that was stored before
updating the current matches). Keep the rest of the logic
(staleMatchShouldReload, inner.forceStaleReload, match.cause,
loaderShouldRunAsync) unchanged so the reload decision correctly detects a
changed route instance even when the match entry stays mounted.
---
Outside diff comments:
In `@packages/router-core/src/load-matches.ts`:
- Around line 817-831: When launching the background async loader in the branch
guarded by loaderShouldRunAsync && !inner.sync, ensure the match context is
synced before returning stale UI: call syncMatchContext(inner, matchId, index)
right before starting the async IIFE so match.context and any
__beforeLoadContext updates are applied immediately; keep the existing async
runLoader(inner, matchPromises, matchId, index, route) invocation and its
try/catch/redirect handling unchanged, but remove the window where match.context
remains stale by invoking syncMatchContext first.
---
Nitpick comments:
In `@e2e/react-start/spa-mode/tests/app.spec.ts`:
- Around line 44-48: Remove the redundant explicit TypeScript fixture
annotations in the test callbacks — replace the parameter destructuring `async
({ page }: { page: Page }) =>` with `async ({ page }) =>` for the tests named
"directly visiting prerendered /posts/1" and the similar test at the second
occurrence (the other test using the same `{ page: Page }` annotation); this
keeps Playwright's built-in typings via `@tanstack/router-e2e-utils` while
avoiding duplicate type declarations and IDE noise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1774b631-d1d0-4ba0-a7b5-20ba9b64592a
📒 Files selected for processing (11)
e2e/react-start/spa-mode/tests/app.spec.tse2e/solid-start/spa-mode/tests/app.spec.tse2e/vue-start/spa-mode/tests/app.spec.tspackages/react-router/src/fileRoute.tspackages/react-router/src/route.tsxpackages/router-core/src/load-matches.tspackages/router-core/src/router.tspackages/solid-router/src/fileRoute.tspackages/solid-router/src/route.tsxpackages/vue-router/src/fileRoute.tspackages/vue-router/src/route.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/solid-router/src/route.tsx
- packages/router-core/src/router.ts
- packages/react-router/src/fileRoute.ts
- packages/solid-router/src/fileRoute.ts
- packages/react-router/src/route.tsx
- packages/vue-router/src/fileRoute.ts
Sheraff
reviewed
Mar 8, 2026
Sheraff
reviewed
Mar 8, 2026
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
2 participants
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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests