Skip to content

refactor(router-core): shouldExecuteBeforeLoad is always true #4992

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

Merged
merged 2 commits into from
Aug 19, 2025
Merged

refactor(router-core): shouldExecuteBeforeLoad is always true #4992

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
66772ea
refactor(router-core): shouldExecuteBeforeLoad is always true
Sheraff Aug 18, 2025
0dcac36
ci: apply automated fixes
autofix-ci[bot] Aug 18, 2025
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
41 changes: 12 additions & 29 deletions packages/router-core/src/load-matches.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,11 +147,10 @@ const shouldSkipLoader = (
return true
}

if (inner.router.isServer) {
if (match.ssr === false) {
return true
}
if (inner.router.isServer && match.ssr === false) {
return true
}

return false
}

Expand Down Expand Up @@ -302,11 +301,11 @@ const setupPendingTimeout = (
}
}

const shouldExecuteBeforeLoad = (
const preBeforeLoadSetup = (
inner: InnerLoadContext,
matchId: string,
route: AnyRoute,
): boolean | Promise<boolean> => {
): void | Promise<void> => {
const existingMatch = inner.router.getMatch(matchId)!

// If we are in the middle of a load, either of these will be present
Expand All @@ -315,25 +314,21 @@ const shouldExecuteBeforeLoad = (
!existingMatch._nonReactive.beforeLoadPromise &&
!existingMatch._nonReactive.loaderPromise
)
return true
return

setupPendingTimeout(inner, matchId, route, existingMatch)

const then = () => {
let result = true
const match = inner.router.getMatch(matchId)!
if (match.status === 'error') {
result = true
} else if (
if (
match.preload &&
(match.status === 'redirected' || match.status === 'notFound')
) {
handleRedirectAndNotFound(inner, match, match.error)
}
return result
}
Comment on lines 322 to 329
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Guard against missing match after preload redirect to avoid runtime crash

In preload flows, a prior beforeLoad/loader can redirect and the match may no longer exist in router.state. Accessing match.preload/status via a non-null assertion can crash at runtime.

Apply a defensive check before dereferencing:

-    const match = inner.router.getMatch(matchId)!
-    if (
+    const match = inner.router.getMatch(matchId)
+    if (!match) return
+    if (
       match.preload &&
       (match.status === 'redirected' || match.status === 'notFound')
     ) {
       handleRedirectAndNotFound(inner, match, match.error)
     }

I can add a regression test that preloads a route whose beforeLoad redirects, ensuring no crash here. Want me to open a follow-up?

📝 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.

Suggested change
const match = inner.router.getMatch(matchId)!
if (match.status === 'error') {
result = true
} else if (
if (
match.preload &&
(match.status === 'redirected' || match.status === 'notFound')
) {
handleRedirectAndNotFound(inner, match, match.error)
}
return result
}
const match = inner.router.getMatch(matchId)
if (!match) return
if (
match.preload &&
(match.status === 'redirected' || match.status === 'notFound')
) {
handleRedirectAndNotFound(inner, match, match.error)
}
🤖 Prompt for AI Agents 
In packages/router-core/src/load-matches.ts around lines 322 to 329, the code
dereferences match with a non-null assertion after getting it from
inner.router.getMatch(matchId), which can crash if a prior beforeLoad/loader
redirected and removed the match; add a defensive check for match existence and
only proceed to check match.preload and match.status if match is defined (e.g.,
if (!match) continue/return), otherwise skip calling handleRedirectAndNotFound;
update control flow so handleRedirectAndNotFound is only invoked when match !==
undefined.


// Wait for the beforeLoad to resolve before we continue
// Wait for the previous beforeLoad to resolve before we continue
return existingMatch._nonReactive.beforeLoadPromise
? existingMatch._nonReactive.beforeLoadPromise.then(then)
: then()
Expand Down Expand Up @@ -494,24 +489,12 @@ const handleBeforeLoad = (

const queueExecution = () => {
if (shouldSkipLoader(inner, matchId)) return
const shouldExecuteBeforeLoadResult = shouldExecuteBeforeLoad(
inner,
matchId,
route,
)
return isPromise(shouldExecuteBeforeLoadResult)
? shouldExecuteBeforeLoadResult.then(execute)
: execute(shouldExecuteBeforeLoadResult)
}

const execute = (shouldExec: boolean) => {
if (shouldExec) {
// If we are not in the middle of a load OR the previous load failed, start it
return executeBeforeLoad(inner, matchId, index, route)
}
return
const result = preBeforeLoadSetup(inner, matchId, route)
return isPromise(result) ? result.then(execute) : execute()
}

const execute = () => executeBeforeLoad(inner, matchId, index, route)

return serverSsr()
}

Expand Down