fix: handle stale redirected matches during view transitions#7158
fix: handle stale redirected matches during view transitions#7158schiller-manuel wants to merge 2 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughThis PR prevents a client-side crash when a root Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant FrameworkMatch as Match Component
participant RouterCore
participant LazyRoute as Lazy Loader
Browser->>FrameworkMatch: Navigate (root)
FrameworkMatch->>RouterCore: beforeLoad runs -> redirect to /posts
Browser->>FrameworkMatch: Render pendingComponent (view transition starts)
FrameworkMatch->>RouterCore: markMatchPendingVisible(matchId)
RouterCore->>LazyRoute: start loading /posts (async)
alt Lazy resolves before redirect finalizes
LazyRoute-->>FrameworkMatch: component ready
FrameworkMatch->>RouterCore: consume load result -> finish navigation
else Redirect/transition races and match becomes stale
RouterCore-->>FrameworkMatch: provide retainedPendingPromise fallback
FrameworkMatch->>Browser: keep/clear pending UI safely (no crash)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 docstrings
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit afbe97a
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview1 package(s) bumped directly, 4 bumped as dependents. 🟩 Patch bumps
|
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
e2e/react-router/issue-7120/src/posts.ts (1)
9-19: Prefer an explicit return type forfetchPostsfor stronger type contracts.This helps keep call-site expectations stable if response typing changes later.
♻️ Suggested update
export const fetchPosts = async () => { +export const fetchPosts = async (): Promise<Array<PostType>> => { return axios .get<Array<PostType>>(`${queryURL}/posts`) .then((r) => r.data.slice(0, 10)) }As per coding guidelines,
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-router/issue-7120/src/posts.ts` around lines 9 - 19, The function fetchPosts lacks an explicit return type; update its signature to include a strong TypeScript return type (e.g. export const fetchPosts = async (): Promise<PostType[]> => { ... }) so callers know they receive an array of PostType (the function still returns the first 10 items from queryURL via axios.get<Array<PostType>>). Keep the implementation unchanged except for the annotated return type (or use Promise<Array<PostType>> if you prefer that notation).
🤖 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/solid-router/src/Match.tsx`:
- Around line 336-343: The helper getRetainedPendingPromise loses access to a
stale match's internal _nonReactive because rawMatchState().match returns a
stripped match; restore or preserve the original match object so its
_nonReactive is available as a fallback. Concretely, update rawMatchState (or
the code that produces fallbackMatch) to return the full match object including
_nonReactive (instead of stripping it), then change getRetainedPendingPromise to
prefer router.getMatch(matchId)?._nonReactive.retainedPendingPromise and fall
back to fallbackMatch?._nonReactive.retainedPendingPromise; apply the same
change to the second occurrence (the block around lines 455-462) so both places
can suspend on the retainedPendingPromise of a stale match.
- Around line 32-37: The current Solid.onMount call only runs once so when
PendingRouteMatch remains mounted and props.matchId changes the new match isn't
marked pending-visible; replace or supplement the onMount with a reactive
watcher (e.g., Solid.createEffect) that depends on props.matchId, call
router.getMatch(props.matchId) inside it and invoke
markMatchPendingVisible(match) whenever a non-null match is found; keep or
remove the original onMount as needed to preserve initial behavior, and
reference the existing functions Solid.onMount / Solid.createEffect,
router.getMatch, markMatchPendingVisible and the PendingRouteMatch component to
locate where to add the reactive effect.
In `@packages/vue-router/src/Match.tsx`:
- Around line 304-309: The component currently only calls
markMatchPendingVisible in Vue.onMounted, so when props.matchId changes in a
reused instance new matches aren't marked; add a watcher using Vue.watch(() =>
props.matchId, ...) (in addition to the existing Vue.onMounted call) that
retrieves the new match via router.getMatch(props.matchId) and invokes
markMatchPendingVisible(match) when a match exists, ensuring pending visibility
is re-applied on matchId updates.
---
Nitpick comments:
In `@e2e/react-router/issue-7120/src/posts.ts`:
- Around line 9-19: The function fetchPosts lacks an explicit return type;
update its signature to include a strong TypeScript return type (e.g. export
const fetchPosts = async (): Promise<PostType[]> => { ... }) so callers know
they receive an array of PostType (the function still returns the first 10 items
from queryURL via axios.get<Array<PostType>>). Keep the implementation unchanged
except for the annotated return type (or use Promise<Array<PostType>> if you
prefer that notation).
🪄 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: 58c421fc-673d-4c3f-88da-a66531f614e0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
.changeset/bright-cars-lie.mde2e/react-router/issue-7120/index.htmle2e/react-router/issue-7120/package.jsone2e/react-router/issue-7120/playwright.config.tse2e/react-router/issue-7120/src/main.tsxe2e/react-router/issue-7120/src/posts.lazy.tsxe2e/react-router/issue-7120/src/posts.tse2e/react-router/issue-7120/src/styles.csse2e/react-router/issue-7120/tests/issue-7120.repro.spec.tse2e/react-router/issue-7120/tests/setup/global.setup.tse2e/react-router/issue-7120/tests/setup/global.teardown.tse2e/react-router/issue-7120/tsconfig.jsone2e/react-router/issue-7120/vite.config.jspackages/react-router/src/Match.tsxpackages/react-router/tests/redirect.test.tsxpackages/router-core/src/Matches.tspackages/router-core/src/index.tspackages/router-core/src/load-matches.tspackages/solid-router/src/Match.tsxpackages/solid-router/tests/redirect.test.tsxpackages/vue-router/src/Match.tsxpackages/vue-router/tests/redirect.test.tsx
| Solid.onMount(() => { | ||
| const match = router.getMatch(props.matchId) | ||
| if (match) { | ||
| markMatchPendingVisible(match) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Re-run markMatchPendingVisible when Solid reuses the pending component.
onMount only fires once. If PendingRouteMatch stays mounted and props.matchId changes, the new match is never marked pending-visible, so later redirect/pending transitions won't retain the right promise.
Suggested fix
- Solid.onMount(() => {
+ Solid.createEffect(() => {
const match = router.getMatch(props.matchId)
if (match) {
markMatchPendingVisible(match)
}
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/solid-router/src/Match.tsx` around lines 32 - 37, The current
Solid.onMount call only runs once so when PendingRouteMatch remains mounted and
props.matchId changes the new match isn't marked pending-visible; replace or
supplement the onMount with a reactive watcher (e.g., Solid.createEffect) that
depends on props.matchId, call router.getMatch(props.matchId) inside it and
invoke markMatchPendingVisible(match) whenever a non-null match is found; keep
or remove the original onMount as needed to preserve initial behavior, and
reference the existing functions Solid.onMount / Solid.createEffect,
router.getMatch, markMatchPendingVisible and the PendingRouteMatch component to
locate where to add the reactive effect.
| const getRetainedPendingPromise = ( | ||
| matchId: string, | ||
| fallbackMatch: AnyRouteMatch | undefined, | ||
| ) => { | ||
| return ( | ||
| router.getMatch(matchId)?._nonReactive.retainedPendingPromise ?? | ||
| fallbackMatch?._nonReactive.retainedPendingPromise | ||
| ) |
There was a problem hiding this comment.
Keep the stale Solid match's _nonReactive state available.
These helpers only work while router.getMatch(matchId) still resolves. rawMatchState().match currently strips _nonReactive, so once the redirected match drops out of the router pool there is no retained promise left to suspend on and this branch falls through instead of preserving the pending UI.
Suggested fix
return {
key,
routeId,
- match: {
- id: currentMatch.id,
- status: currentMatch.status,
- error: currentMatch.error,
- _forcePending: currentMatch._forcePending ?? false,
- _displayPending: currentMatch._displayPending ?? false,
- },
+ match: currentMatch,
}- const routerMatch = router.getMatch(matchId)
+ const fallbackMatch = currentMatch()
if (!isRedirect(currentMatch().error)) {
if (process.env.NODE_ENV !== 'production') {
throw new Error(
'Invariant failed: Expected a redirect error',
@@
const [loaderResult] = Solid.createResource(async () => {
await new Promise((r) => setTimeout(r, 0))
- return getLoadPromise(matchId, routerMatch)
+ return getLoadPromise(matchId, fallbackMatch)
})
const [retainedPendingResult] = Solid.createResource(
async () => {
await new Promise((r) => setTimeout(r, 0))
- return getRetainedPendingPromise(matchId, routerMatch)
+ return getRetainedPendingPromise(matchId, fallbackMatch)
},
)Also applies to: 455-462
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/solid-router/src/Match.tsx` around lines 336 - 343, The helper
getRetainedPendingPromise loses access to a stale match's internal _nonReactive
because rawMatchState().match returns a stripped match; restore or preserve the
original match object so its _nonReactive is available as a fallback.
Concretely, update rawMatchState (or the code that produces fallbackMatch) to
return the full match object including _nonReactive (instead of stripping it),
then change getRetainedPendingPromise to prefer
router.getMatch(matchId)?._nonReactive.retainedPendingPromise and fall back to
fallbackMatch?._nonReactive.retainedPendingPromise; apply the same change to the
second occurrence (the block around lines 455-462) so both places can suspend on
the retainedPendingPromise of a stale match.
| Vue.onMounted(() => { | ||
| const match = router.getMatch(props.matchId) | ||
| if (match) { | ||
| markMatchPendingVisible(match) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Re-run pending visibility marking when matchId changes.
PendingRouteMatch only calls markMatchPendingVisible on mount. Vue can reuse this component instance while patching a new matchId, so later pending/redirect transitions never mark the new match as visible and won't retain its pending promise.
Suggested fix
setup(props) {
const router = useRouter()
- Vue.onMounted(() => {
- const match = router.getMatch(props.matchId)
+ const markPendingVisible = (matchId: string) => {
+ const match = router.getMatch(matchId)
if (match) {
markMatchPendingVisible(match)
}
- })
+ }
+
+ Vue.onMounted(() => {
+ markPendingVisible(props.matchId)
+ })
+
+ Vue.watch(
+ () => props.matchId,
+ (matchId) => {
+ markPendingVisible(matchId)
+ },
+ { flush: 'post' },
+ )
return () => (props.pendingComponent ? Vue.h(props.pendingComponent) : null)
},📝 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.
| Vue.onMounted(() => { | |
| const match = router.getMatch(props.matchId) | |
| if (match) { | |
| markMatchPendingVisible(match) | |
| } | |
| }) | |
| const markPendingVisible = (matchId: string) => { | |
| const match = router.getMatch(matchId) | |
| if (match) { | |
| markMatchPendingVisible(match) | |
| } | |
| } | |
| Vue.onMounted(() => { | |
| markPendingVisible(props.matchId) | |
| }) | |
| Vue.watch( | |
| () => props.matchId, | |
| (matchId) => { | |
| markPendingVisible(matchId) | |
| }, | |
| { flush: 'post' }, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vue-router/src/Match.tsx` around lines 304 - 309, The component
currently only calls markMatchPendingVisible in Vue.onMounted, so when
props.matchId changes in a reused instance new matches aren't marked; add a
watcher using Vue.watch(() => props.matchId, ...) (in addition to the existing
Vue.onMounted call) that retrieves the new match via
router.getMatch(props.matchId) and invokes markMatchPendingVisible(match) when a
match exists, ensuring pending visibility is re-applied on matchId updates.
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/react-router/src/Match.tsx`:
- Around line 444-467: Add a concise clarifying comment inside the
suspendOrKeepPending function (near the final "return null") explaining that
returning null is intentional: it covers the race where a resolved promise
cleared before retainedPendingPromise was set (i.e., there is simply no active
promise to throw/suspend on), so the caller should continue rendering normally;
reference the function name suspendOrKeepPending and variables promise and
retainedPendingPromise to indicate the exact fallback path being documented.
🪄 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: 95471f9b-c5d8-4377-b41c-233743a4f0ae
📒 Files selected for processing (1)
packages/react-router/src/Match.tsx
| const suspendOrKeepPending = ( | ||
| key: 'displayPendingPromise' | 'minPendingPromise' | 'loadPromise', | ||
| ) => { | ||
| const routerMatch = router.getMatch(match.id) | ||
|
|
||
| const promise = | ||
| routerMatch?._nonReactive[key] ?? | ||
| match._nonReactive[key] ?? | ||
| router.latestLoadPromise | ||
|
|
||
| if (promise) { | ||
| throw promise | ||
| } | ||
|
|
||
| const retainedPendingPromise = | ||
| routerMatch?._nonReactive.retainedPendingPromise ?? | ||
| match._nonReactive.retainedPendingPromise | ||
|
|
||
| if (retainedPendingPromise) { | ||
| throw retainedPendingPromise | ||
| } | ||
|
|
||
| return null | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any test cases or comments explaining when suspendOrKeepPending might return null
# Search for test coverage or comments about this behavior
rg -n -C3 "suspendOrKeepPending|return null" packages/react-router/src/
# Check if there's e2e coverage for this specific edge case
rg -n "retainedPendingPromise" e2e/Repository: TanStack/router
Length of output: 6869
🏁 Script executed:
# Read the Match.tsx file around the suspendOrKeepPending function to understand the full context
sed -n '430,530p' packages/react-router/src/Match.tsxRepository: TanStack/router
Length of output: 3287
🏁 Script executed:
# Search for where retainedPendingPromise is set and used
rg -n "retainedPendingPromise" packages/react-router/src/ packages/router-core/src/ --max-count=50
# Also check if there are comments explaining when suspendOrKeepPending might return null
rg -n -B5 "return null" packages/react-router/src/Match.tsx | head -100Repository: TanStack/router
Length of output: 1488
🏁 Script executed:
# See the context around where retainedPendingPromise is set and cleared
sed -n '100,120p' packages/router-core/src/Matches.ts
# See the context where it's cleared
sed -n '40,55p' packages/router-core/src/load-matches.ts
# Search for tests related to this edge case
rg -n "suspendOrKeepPending|retainedPendingPromise" e2e/ --type tsx --type ts | head -30Repository: TanStack/router
Length of output: 1049
🏁 Script executed:
# Search for e2e tests that cover pending/suspense behavior
find e2e -name "*.tsx" -o -name "*.ts" | head -20
# Look for tests related to pending or suspense
rg -n "pending|suspense" e2e/ --max-count=30 | grep -i "test\|it\|describe"
# Check the Matches.ts file for more context on how retainedPendingPromise is used
sed -n '100,180p' packages/router-core/src/Matches.tsRepository: TanStack/router
Length of output: 21100
🏁 Script executed:
# Look at the full context of clearRetainedPending and when it's called
rg -n "clearRetainedPending" packages/router-core/src/ -A 5 -B 5
# Check the load-matches.ts to understand when promises are cleared
sed -n '30,60p' packages/router-core/src/load-matches.tsRepository: TanStack/router
Length of output: 5198
🏁 Script executed:
# Look at the updateMatch callback and how match status changes
sed -n '220,280p' packages/router-core/src/load-matches.tsRepository: TanStack/router
Length of output: 1589
🏁 Script executed:
# Search for where _displayPending and _forcePending are set
rg -n "_displayPending|_forcePending" packages/router-core/src/ --max-count=50Repository: TanStack/router
Length of output: 637
🏁 Script executed:
# Look at the context where _displayPending and _forcePending are set and cleared
sed -n '110,130p' packages/router-core/src/ssr/ssr-client.ts
sed -n '280,310p' packages/router-core/src/ssr/ssr-client.tsRepository: TanStack/router
Length of output: 1719
🏁 Script executed:
# Look for where markMatchPendingVisible is called to understand when retainedPendingPromise is set
rg -n "markMatchPendingVisible" packages/ --max-count=20Repository: TanStack/router
Length of output: 687
🏁 Script executed:
# Look at the context around line 253 where markMatchPendingVisible is called
sed -n '240,280p' packages/react-router/src/Match.tsxRepository: TanStack/router
Length of output: 1171
🏁 Script executed:
# Look at what renders PendingRouteMatch and when pendingElement is provided
sed -n '300,400p' packages/react-router/src/Match.tsxRepository: TanStack/router
Length of output: 2895
🏁 Script executed:
# Continue reading the client-side rendering logic to see when PendingRouteMatch is rendered
sed -n '400,480p' packages/react-router/src/Match.tsxRepository: TanStack/router
Length of output: 2442
🏁 Script executed:
# Look for any tests or code that might exercise the return null path
rg -n "return null" packages/react-router/src/Match.tsx -B 10 | head -40
# Check if there's any documentation about when return null is expected
rg -n "pending\|redirected" packages/react-router/src/Match.tsx | grep -i "comment\|todo\|fixme\|note"Repository: TanStack/router
Length of output: 1269
🏁 Script executed:
# Search for any comments explaining the suspendOrKeepPending logic
sed -n '430,480p' packages/react-router/src/Match.tsx | cat -nRepository: TanStack/router
Length of output: 1890
Consider adding a clarifying comment explaining the return null fallback path.
The return null at line 466 can be reached when a promise resolves before retainedPendingPromise is set (a race condition window that occurs when pending UI hasn't rendered yet). This is expected behavior, but adding a comment explaining that returning null is appropriate when there's no promise to suspend on would improve code clarity and prevent future maintainers from questioning this logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-router/src/Match.tsx` around lines 444 - 467, Add a concise
clarifying comment inside the suspendOrKeepPending function (near the final
"return null") explaining that returning null is intentional: it covers the race
where a resolved promise cleared before retainedPendingPromise was set (i.e.,
there is simply no active promise to throw/suspend on), so the caller should
continue rendering normally; reference the function name suspendOrKeepPending
and variables promise and retainedPendingPromise to indicate the exact fallback
path being documented.
There was a problem hiding this comment.
Nx Cloud is proposing a fix for your failed CI:
We updated four Solid.createResource factory functions in packages/solid-router/src/Match.tsx to return Promise<null> instead of Promise<void>, fixing TypeScript 5.5 error TS1345 ("An expression of type 'void' cannot be tested for truthiness"). The factories now chain .then(() => null) on the underlying promises so their resolved type is null | undefined rather than void | undefined, making the ?? renderPending() expressions valid while preserving identical runtime suspension behavior.
Tip
✅ We verified this fix by re-running @tanstack/solid-router:test:types.
Suggested Fix changes
diff --git a/packages/solid-router/src/Match.tsx b/packages/solid-router/src/Match.tsx
index 3f07c8889b..e43df877d3 100644
--- a/packages/solid-router/src/Match.tsx
+++ b/packages/solid-router/src/Match.tsx
@@ -354,9 +354,10 @@ export const MatchInner = (): any => {
<Solid.Match when={currentMatch()._displayPending}>
{(_) => {
const [displayPendingResult] = Solid.createResource(
- () =>
- router.getMatch(currentMatch().id)?._nonReactive
- .displayPendingPromise,
+ (): Promise<null> | undefined =>
+ router
+ .getMatch(currentMatch().id)
+ ?._nonReactive.displayPendingPromise?.then(() => null),
)
return <>{displayPendingResult() ?? renderPending()}</>
@@ -365,9 +366,10 @@ export const MatchInner = (): any => {
<Solid.Match when={currentMatch()._forcePending}>
{(_) => {
const [minPendingResult] = Solid.createResource(
- () =>
- router.getMatch(currentMatch().id)?._nonReactive
- .minPendingPromise,
+ (): Promise<null> | undefined =>
+ router
+ .getMatch(currentMatch().id)
+ ?._nonReactive.minPendingPromise?.then(() => null),
)
return <>{minPendingResult() ?? renderPending()}</>
@@ -401,11 +403,16 @@ export const MatchInner = (): any => {
}
}
- const [loaderResult] = Solid.createResource(async () => {
- await new Promise((r) => setTimeout(r, 0))
- return router.getMatch(currentMatch().id)?._nonReactive
- .loadPromise
- })
+ const [loaderResult] = Solid.createResource(
+ (): Promise<null> =>
+ new Promise<void>((r) => setTimeout(r, 0))
+ .then(
+ () =>
+ router.getMatch(currentMatch().id)?._nonReactive
+ .loadPromise,
+ )
+ .then(() => null),
+ )
return <>{loaderResult() ?? renderPending()}</>
}}
@@ -447,16 +454,20 @@ export const MatchInner = (): any => {
invariant()
}
- const [loaderResult] = Solid.createResource(async () => {
- await new Promise((r) => setTimeout(r, 0))
- return getLoadPromise(matchId, routerMatch)
- })
+ const [loaderResult] = Solid.createResource(
+ (): Promise<null> =>
+ new Promise<void>((r) => setTimeout(r, 0))
+ .then(() => getLoadPromise(matchId, routerMatch))
+ .then(() => null),
+ )
const [retainedPendingResult] = Solid.createResource(
- async () => {
- await new Promise((r) => setTimeout(r, 0))
- return getRetainedPendingPromise(matchId, routerMatch)
- },
+ (): Promise<null> =>
+ new Promise<void>((r) => setTimeout(r, 0))
+ .then(() =>
+ getRetainedPendingPromise(matchId, routerMatch),
+ )
+ .then(() => null),
)
return <>{loaderResult() ?? retainedPendingResult()}</>
Or Apply changes locally with:
npx nx-cloud apply-locally jPV0-cpOm
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
Summary by CodeRabbit
Bug Fixes
Tests