Skip to content

refactor: use local matches in adapter Match components #7052

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Sheraff wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

refactor: use local matches in adapter Match components #7052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 11 additions & 12 deletions packages/react-router/src/Match.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,15 +287,15 @@ export const MatchInner = React.memo(function MatchInnerImpl({
const out = Comp ? <Comp key={key} /> : <Outlet />

if (match._displayPending) {
throw router.getMatch(match.id)?._nonReactive.displayPendingPromise
throw match._nonReactive.displayPendingPromise
}

if (match._forcePending) {
throw router.getMatch(match.id)?._nonReactive.minPendingPromise
throw match._nonReactive.minPendingPromise
}

if (match.status === 'pending') {
throw router.getMatch(match.id)?._nonReactive.loadPromise
throw match._nonReactive.loadPromise
}

if (match.status === 'notFound') {
Expand All @@ -317,7 +317,7 @@ export const MatchInner = React.memo(function MatchInnerImpl({

invariant()
}
throw router.getMatch(match.id)?._nonReactive.loadPromise
throw match._nonReactive.loadPromise
}

if (match.status === 'error') {
Expand Down Expand Up @@ -384,11 +384,11 @@ export const MatchInner = React.memo(function MatchInnerImpl({
}, [key, route.options.component, router.options.defaultComponent])

if (match._displayPending) {
throw router.getMatch(match.id)?._nonReactive.displayPendingPromise
throw match._nonReactive.displayPendingPromise
}

if (match._forcePending) {
throw router.getMatch(match.id)?._nonReactive.minPendingPromise
throw match._nonReactive.minPendingPromise
}

// see also hydrate() in packages/router-core/src/ssr/ssr-client.ts
Expand All @@ -397,23 +397,22 @@ export const MatchInner = React.memo(function MatchInnerImpl({
const pendingMinMs =
route.options.pendingMinMs ?? router.options.defaultPendingMinMs
if (pendingMinMs) {
const routerMatch = router.getMatch(match.id)
if (routerMatch && !routerMatch._nonReactive.minPendingPromise) {
if (!match._nonReactive.minPendingPromise) {
// Create a promise that will resolve after the minPendingMs
if (!(isServer ?? router.isServer)) {
const minPendingPromise = createControlledPromise<void>()

routerMatch._nonReactive.minPendingPromise = minPendingPromise
match._nonReactive.minPendingPromise = minPendingPromise

setTimeout(() => {
minPendingPromise.resolve()
// We've handled the minPendingPromise, so we can delete it
routerMatch._nonReactive.minPendingPromise = undefined
match._nonReactive.minPendingPromise = undefined
Comment on lines +400 to +410
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Clear minPendingPromise through the store in the timeout.

At Lines 407-410, the callback mutates the closed-over match object. Match stores replace their state object on updates, so if this match is replaced before the timer fires, the live store keeps stale min-pending state and can remain forced-pending. Please clear it through matchStore.setState(...) or router.updateMatch(...) instead of mutating the captured object. The analogous SSR hydrate path already does this.

Suggested fix 
           setTimeout(() => {
             minPendingPromise.resolve()
             // We've handled the minPendingPromise, so we can delete it
-            match._nonReactive.minPendingPromise = undefined
+            matchStore.setState((prev) => {
+              if (prev._nonReactive.minPendingPromise === minPendingPromise) {
+                prev._nonReactive.minPendingPromise = undefined
+                return { ...prev }
+              }
+              return prev
+            })
           }, pendingMinMs)
🤖 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 400 - 410, The timeout
callback currently mutates the closed-over match object by setting
match._nonReactive.minPendingPromise = undefined, which can leave stale state if
the match is replaced; instead, when the timer fires use the store/router API to
clear the pending flag (e.g. call matchStore.setState(...) or
router.updateMatch(...) to set _nonReactive.minPendingPromise to undefined for
that match id) so the live store is updated atomically rather than mutating the
captured match object; locate the timeout in the block that creates
minPendingPromise and replace the direct mutation with a store/router update
using the match's identifier.

}, pendingMinMs)
}
}
}
throw router.getMatch(match.id)?._nonReactive.loadPromise
throw match._nonReactive.loadPromise
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Resolve pending suspense from current match pool

Use of match._nonReactive.loadPromise here can suspend on a stale promise when a new navigation starts before the current pending navigation finishes and both active/pending pools contain the same match.id. In that overlap, router updates and promise resolution target the pending pool first (updateMatch/getMatch behavior in router-core), so the active-store promise can be canceled or left stale while the live pending promise is elsewhere; throwing the active promise can keep the fallback stuck until a later remount instead of tracking the in-flight navigation.

Useful? React with 👍 / 👎.

}

if (match.status === 'notFound') {
Expand Down Expand Up @@ -442,7 +441,7 @@ export const MatchInner = React.memo(function MatchInnerImpl({
// false,
// 'Tried to render a redirected route match! This is a weird circumstance, please file an issue!',
// )
throw router.getMatch(match.id)?._nonReactive.loadPromise
throw match._nonReactive.loadPromise
}

if (match.status === 'error') {
Expand Down
24 changes: 9 additions & 15 deletions packages/solid-router/src/Match.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,9 +276,7 @@ export const MatchInner = (): any => {
<Solid.Match when={currentMatch()._displayPending}>
{(_) => {
const [displayPendingResult] = Solid.createResource(
() =>
router.getMatch(currentMatch().id)?._nonReactive
.displayPendingPromise,
() => match()?._nonReactive.displayPendingPromise,
)

return <>{displayPendingResult()}</>
Expand All @@ -287,9 +285,7 @@ export const MatchInner = (): any => {
<Solid.Match when={currentMatch()._forcePending}>
{(_) => {
const [minPendingResult] = Solid.createResource(
() =>
router.getMatch(currentMatch().id)?._nonReactive
.minPendingPromise,
() => match()?._nonReactive.minPendingPromise,
)

return <>{minPendingResult()}</>
Expand All @@ -302,31 +298,30 @@ export const MatchInner = (): any => {
router.options.defaultPendingMinMs

if (pendingMinMs) {
const routerMatch = router.getMatch(currentMatch().id)
const matchState = match()
if (
routerMatch &&
!routerMatch._nonReactive.minPendingPromise
matchState &&
!matchState._nonReactive.minPendingPromise
) {
// Create a promise that will resolve after the minPendingMs
if (!(isServer ?? router.isServer)) {
const minPendingPromise = createControlledPromise<void>()

routerMatch._nonReactive.minPendingPromise =
matchState._nonReactive.minPendingPromise =
minPendingPromise

setTimeout(() => {
minPendingPromise.resolve()
// We've handled the minPendingPromise, so we can delete it
routerMatch._nonReactive.minPendingPromise = undefined
matchState._nonReactive.minPendingPromise = undefined
Comment on lines +301 to +316
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Clear min-pending state against the live match, not the captured matchState.

At Lines 313-316, the timer closes over matchState and mutates _nonReactive directly. If the match store swaps to a newer state object before this fires, the cleanup hits an orphaned match while the live one keeps stale min-pending state. Please route this through router.updateMatch(...) (or the underlying store) keyed by matchState.id.

Suggested fix 
                     if (!(isServer ?? router.isServer)) {
                       const minPendingPromise = createControlledPromise<void>()
+                      const matchId = matchState.id
 
                       matchState._nonReactive.minPendingPromise =
                         minPendingPromise
 
                       setTimeout(() => {
                         minPendingPromise.resolve()
                         // We've handled the minPendingPromise, so we can delete it
-                        matchState._nonReactive.minPendingPromise = undefined
+                        router.updateMatch(matchId, (prev) => {
+                          if (
+                            prev._nonReactive.minPendingPromise ===
+                            minPendingPromise
+                          ) {
+                            prev._nonReactive.minPendingPromise = undefined
+                            return { ...prev }
+                          }
+                          return prev
+                        })
                       }, pendingMinMs)
                     }
🤖 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 301 - 316, The timeout
callback is closing over the captured matchState and clears
matchState._nonReactive.minPendingPromise directly, which can leave the live
store holding stale state if the match object was replaced; instead, when
creating the minPendingPromise for matchState (from match()), store its id and
use router.updateMatch(matchId, updater) (or the store's update method) inside
the timeout to clear the minPendingPromise on the live match object by id;
locate the block around match(), createControlledPromise(), and where
_nonReactive.minPendingPromise is assigned and replace the direct mutation in
the setTimeout with a router.updateMatch keyed by matchState.id that sets
_nonReactive.minPendingPromise = undefined.

}, pendingMinMs)
}
}
}

const [loaderResult] = Solid.createResource(async () => {
await new Promise((r) => setTimeout(r, 0))
return router.getMatch(currentMatch().id)?._nonReactive
.loadPromise
return match()?._nonReactive.loadPromise
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Read loader promise from live pool in Solid pending path

This resource now reads match()?._nonReactive.loadPromise from the locally active match object, which has the same stale-promise problem during overlapping navigations with duplicate ids across active and pending pools. Because core load updates resolve promises on the pending pool entry first, binding the Suspense resource to the active object's promise can desynchronize the UI from the real in-flight load and prolong fallback rendering until the route tree is replaced.

Useful? React with 👍 / 👎.

})

const FallbackComponent =
Expand Down Expand Up @@ -379,8 +374,7 @@ export const MatchInner = (): any => {

const [loaderResult] = Solid.createResource(async () => {
await new Promise((r) => setTimeout(r, 0))
return router.getMatch(currentMatch().id)?._nonReactive
.loadPromise
return match()?._nonReactive.loadPromise
})

return <>{loaderResult()}</>
Expand Down
13 changes: 7 additions & 6 deletions packages/vue-router/src/Match.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,14 +378,15 @@ export const MatchInner = Vue.defineComponent({
}

if (match.value.status === 'redirected') {
const currentMatch = activeMatch.value
if (!isRedirect(match.value.error)) {
if (process.env.NODE_ENV !== 'production') {
throw new Error('Invariant failed: Expected a redirect error')
}

invariant()
}
throw router.getMatch(match.value.id)?._nonReactive.loadPromise
throw currentMatch?._nonReactive.loadPromise
}

if (match.value.status === 'error') {
Expand Down Expand Up @@ -414,25 +415,25 @@ export const MatchInner = Vue.defineComponent({
}

if (match.value.status === 'pending') {
const currentMatch = activeMatch.value
const pendingMinMs =
route.value.options.pendingMinMs ?? router.options.defaultPendingMinMs

const routerMatch = router.getMatch(match.value.id)
if (
pendingMinMs &&
routerMatch &&
!routerMatch._nonReactive.minPendingPromise
currentMatch &&
!currentMatch._nonReactive.minPendingPromise
) {
// Create a promise that will resolve after the minPendingMs
if (!(isServer ?? router.isServer)) {
const minPendingPromise = createControlledPromise<void>()

routerMatch._nonReactive.minPendingPromise = minPendingPromise
currentMatch._nonReactive.minPendingPromise = minPendingPromise

setTimeout(() => {
minPendingPromise.resolve()
// We've handled the minPendingPromise, so we can delete it
routerMatch._nonReactive.minPendingPromise = undefined
currentMatch._nonReactive.minPendingPromise = undefined
}, pendingMinMs)
Comment on lines 422 to 437
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid clearing min-pending state on the captured currentMatch.

At Lines 433-436, the timeout mutates currentMatch._nonReactive directly. If the router replaces that match object before the timer fires, the live store keeps stale min-pending state. Please clear it via router.updateMatch(...) (or the match store) keyed by the current match id instead of mutating the closed-over object.

Suggested fix 
         if (
           pendingMinMs &&
           currentMatch &&
           !currentMatch._nonReactive.minPendingPromise
         ) {
+          const matchId = currentMatch.id
           // Create a promise that will resolve after the minPendingMs
           if (!(isServer ?? router.isServer)) {
             const minPendingPromise = createControlledPromise<void>()
 
             currentMatch._nonReactive.minPendingPromise = minPendingPromise
 
             setTimeout(() => {
               minPendingPromise.resolve()
               // We've handled the minPendingPromise, so we can delete it
-              currentMatch._nonReactive.minPendingPromise = undefined
+              router.updateMatch(matchId, (prev) => {
+                if (prev._nonReactive.minPendingPromise === minPendingPromise) {
+                  prev._nonReactive.minPendingPromise = undefined
+                  return { ...prev }
+                }
+                return prev
+              })
             }, pendingMinMs)
           }
         }
🤖 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 422 - 437, The timeout
currently mutates the captured currentMatch._nonReactive.minPendingPromise
directly which can leave stale state if the router replaces the match; instead,
inside the setTimeout callback resolve the controlled promise but clear the
minPendingPromise through the router's match update API (e.g., call
router.updateMatch(currentMatch.id, { _nonReactive: { minPendingPromise:
undefined } }) or use the match store update function) using the currentMatch.id
so the live store is updated atomically rather than mutating the closed-over
currentMatch object; keep creating and assigning the controlled promise to
currentMatch._nonReactive.minPendingPromise before scheduling the timeout, but
ensure the timeout uses router.updateMatch (or equivalent) to clear it.

}
}
Expand Down
4 changes: 2 additions & 2 deletions packages/vue-router/tests/store-updates-during-navigation.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ describe("Store doesn't update *too many* times during navigation", () => {
// that needs to be done during a navigation.
// Any change that increases this number should be investigated.
// Note: Vue has different update counts than React/Solid due to different reactivity
expect(updates).toBe(16)
expect(updates).toBe(14)
})

test('redirection in preload', async () => {
Expand Down Expand Up @@ -174,7 +174,7 @@ describe("Store doesn't update *too many* times during navigation", () => {
// that needs to be done during a navigation.
// Any change that increases this number should be investigated.
// Note: Vue has different update counts than React/Solid due to different reactivity
expect(updates).toBe(12)
expect(updates).toBe(10)
})

test('nothing', async () => {
Expand Down
Loading