fix: handle beforeLoad throwing notFound correctly#6811
fix: handle beforeLoad throwing notFound correctly#6811schiller-manuel merged 7 commits intomainfrom
Conversation
|
View your CI Pipeline Execution ↗ for commit 5617b69
☁️ Nx Cloud last updated this comment at |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors loadMatches to deterministically resolve notFound thrown in beforeLoad (including routeId-targeting and boundary selection), adds not-found test routes and tests across frameworks, updates docs and generated route trees, and adjusts SSR hydration/dehydration and prerender filters. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant RouterCore as RouterCore (loadMatches)
participant BeforeLoad as Route.beforeLoad
participant Loader as Route.loader
participant Boundary as NotFoundBoundary
Browser->>RouterCore: navigate to /not-found/...
RouterCore->>BeforeLoad: run beforeLoad hooks (ordered)
alt beforeLoad throws notFound
BeforeLoad-->>RouterCore: throw notFound({ routeId?, data? })
RouterCore->>RouterCore: getNotFoundBoundaryIndex(err) → select boundary index
RouterCore->>Loader: optionally run loaders up to/for boundary
RouterCore->>Boundary: ensure boundary has notFoundComponent (use default if needed)
Boundary-->>RouterCore: notFoundComponent (with loader data) ready
RouterCore->>Browser: finalize navigation → render selected notFoundComponent
else no notFound
RouterCore->>Loader: run loaders
Loader-->>RouterCore: loader data
RouterCore->>Browser: render route component
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
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: 6
🧹 Nitpick comments (2)
packages/router-core/src/load-matches.ts (1)
593-593: Newas anycasts in the core loading path reduce type safety.Line 593 and Line 1001 introduce broad casts that can hide interface drift in loader and router option contracts.
As per coding guidelines,
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.Also applies to: 1001-1002
🤖 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 593, The new broad casts using "as any" on parentMatchPromise (from matchPromises) remove type safety; replace them by tightening the matchPromises and parentMatchPromise types instead of casting: update the declaration/type of matchPromises (and related variables used around loadMatches / loadRouteModule / loadMatches functions) to the correct generic Promise<RouteMatch<T> | undefined>[] (or the specific RouteMatch<T> union your loader returns), propagate those generics through loadMatches/loadRouteModule signatures, and where necessary use unknown + safe type guards or Promise.resolve<T>() to narrow types rather than "as any"; specifically remove the "as any" on parentMatchPromise and the similar cast later and ensure callers supply the appropriate generic types so the compiler enforces the loader/router option interfaces.packages/router-core/tests/load.test.ts (1)
893-895: Tighten helper signatures to avoidanyin new tests.The
beforeLoadNotFoundFactorytuple andrunLoadMatchesAndCapture(router: any)lose useful compile-time safety in a critical behavior test suite.As per coding guidelines,
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.Also applies to: 994-1006
🤖 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 893 - 895, The helper signatures use unsafe any types: replace the tuple element types and the router parameter with concrete router types to restore compile-time safety. Change routes: readonly [any, any, any, any] to a strongly typed tuple (e.g., readonly [RouteRecordRaw, RouteRecordRaw, RouteRecordRaw, RouteRecordRaw] or the project's equivalent route descriptor type) and update the return type still using ReturnType<typeof notFound>; also change runLoadMatchesAndCapture(router: any) to accept the proper Router/RouterInstance type used in the codebase (e.g., Router or RouterHistory/RouterInstance type) so tests and helpers (beforeLoadNotFoundFactory and runLoadMatchesAndCapture) use explicit types; apply the same replacements for the other occurrences around lines 994-1006.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/react-start/basic/src/routes/not-found/deep/b/c/d.tsx`:
- Around line 21-24: Replace the untyped props in the notFoundComponent with the
exported NotFoundRouteProps type: import NotFoundRouteProps from the router
types (or the module that exports it) and change the component signature from
(props: any) => ... to (props: NotFoundRouteProps) => ..., ensuring you update
the import and any references to props.data, props.isNotFound, or props.routeId
accordingly so the component uses the correct typed properties.
In `@e2e/react-start/basic/src/routes/not-found/deep/b/c/route.tsx`:
- Around line 12-15: Replace the loose any in the notFoundComponent declaration
with the proper NotFoundRouteProps type from `@tanstack/router-core`: add an
import for NotFoundRouteProps and change the component signature from (props:
any) => ... to (props: NotFoundRouteProps) => ... so the props.data,
props.isNotFound and props.routeId are correctly typed for the
notFoundComponent.
In `@e2e/react-start/basic/src/routes/not-found/deep/b/route.tsx`:
- Around line 12-15: Replace the loose any type on the notFoundComponent handler
with the proper NotFoundRouteProps type: import NotFoundRouteProps from
'@tanstack/react-router' (or add it to existing imports) and change the handler
signature in notFoundComponent from (props: any) => ... to (props:
NotFoundRouteProps) => ... so the component props are strictly typed; ensure the
import name matches the used symbol NotFoundRouteProps and update any usages of
props accordingly.
In `@e2e/react-start/basic/src/routes/not-found/parent-boundary/route.tsx`:
- Around line 19-27: Replace the unsafe any prop on
ParentBoundaryNotFoundComponent with a narrow typed shape: declare a
NotFoundProps (or NotFoundPayload) interface like { data?: { source?: string } }
and change the component signature from function
ParentBoundaryNotFoundComponent(props: any) to
ParentBoundaryNotFoundComponent(props: NotFoundProps); keep the existing
accessors (props?.data?.source) and Route.useLoaderData() unchanged so runtime
behavior is identical while restoring TypeScript strict safety.
In `@packages/react-router/tests/not-found.test.tsx`:
- Around line 315-317: Replace the hardcoded and cast route id in the beforeLoad
handler with the in-scope parentRoute.id: in the beforeLoad function where it
currently throws notFound({ routeId: '/parent' as never }), change the payload
to use parentRoute.id (i.e., throw notFound({ routeId: parentRoute.id })) so the
notFound call uses the typed route identifier instead of an unsafe string cast.
In `@packages/router-core/src/load-matches.ts`:
- Around line 951-971: The catch block currently awaits
Promise.allSettled(matchPromises) which can hang if any matcher promise never
settles; instead, on error inspect matchPromises immediately by attaching
short-lived rejection handlers to each promise to capture redirect/not-found
reasons without waiting for full settlement: iterate over matchPromises and for
each do p.catch(err => { if (!firstRedirect && isRedirect(err)) firstRedirect =
err; if (!firstNotFound && isNotFound(err)) firstNotFound = err; /* swallow to
avoid unhandledRejection */ return undefined }); then skip awaiting allSettled
and proceed to throw firstRedirect or firstNotFound as before (references:
matchPromises, firstRedirect, firstNotFound, isRedirect, isNotFound).
---
Nitpick comments:
In `@packages/router-core/src/load-matches.ts`:
- Line 593: The new broad casts using "as any" on parentMatchPromise (from
matchPromises) remove type safety; replace them by tightening the matchPromises
and parentMatchPromise types instead of casting: update the declaration/type of
matchPromises (and related variables used around loadMatches / loadRouteModule /
loadMatches functions) to the correct generic Promise<RouteMatch<T> |
undefined>[] (or the specific RouteMatch<T> union your loader returns),
propagate those generics through loadMatches/loadRouteModule signatures, and
where necessary use unknown + safe type guards or Promise.resolve<T>() to narrow
types rather than "as any"; specifically remove the "as any" on
parentMatchPromise and the similar cast later and ensure callers supply the
appropriate generic types so the compiler enforces the loader/router option
interfaces.
In `@packages/router-core/tests/load.test.ts`:
- Around line 893-895: The helper signatures use unsafe any types: replace the
tuple element types and the router parameter with concrete router types to
restore compile-time safety. Change routes: readonly [any, any, any, any] to a
strongly typed tuple (e.g., readonly [RouteRecordRaw, RouteRecordRaw,
RouteRecordRaw, RouteRecordRaw] or the project's equivalent route descriptor
type) and update the return type still using ReturnType<typeof notFound>; also
change runLoadMatchesAndCapture(router: any) to accept the proper
Router/RouterInstance type used in the codebase (e.g., Router or
RouterHistory/RouterInstance type) so tests and helpers
(beforeLoadNotFoundFactory and runLoadMatchesAndCapture) use explicit types;
apply the same replacements for the other occurrences around lines 994-1006.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
docs/router/guide/not-found-errors.mde2e/react-start/basic/src/routeTree.gen.tse2e/react-start/basic/src/routes/__root.tsxe2e/react-start/basic/src/routes/not-found/deep/b/c/d.tsxe2e/react-start/basic/src/routes/not-found/deep/b/c/route.tsxe2e/react-start/basic/src/routes/not-found/deep/b/route.tsxe2e/react-start/basic/src/routes/not-found/deep/index.tsxe2e/react-start/basic/src/routes/not-found/deep/route.tsxe2e/react-start/basic/src/routes/not-found/index.tsxe2e/react-start/basic/src/routes/not-found/parent-boundary/index.tsxe2e/react-start/basic/src/routes/not-found/parent-boundary/route.tsxe2e/react-start/basic/src/routes/not-found/parent-boundary/via-beforeLoad.tsxe2e/react-start/basic/src/routes/not-found/via-beforeLoad-target-root.tsxe2e/react-start/basic/tests/not-found.spec.tse2e/react-start/basic/vite.config.tse2e/solid-start/basic/src/routeTree.gen.tse2e/solid-start/basic/src/routes/not-found/parent-boundary/route.tsxe2e/solid-start/basic/src/routes/not-found/parent-boundary/via-beforeLoad.tsxe2e/solid-start/basic/src/routes/not-found/via-beforeLoad-target-root.tsxe2e/solid-start/basic/tests/not-found.spec.tse2e/vue-start/basic/src/routeTree.gen.tse2e/vue-start/basic/src/routes/not-found/parent-boundary/route.tsxe2e/vue-start/basic/src/routes/not-found/parent-boundary/via-beforeLoad.tsxe2e/vue-start/basic/src/routes/not-found/via-beforeLoad-target-root.tsxe2e/vue-start/basic/tests/not-found.spec.tspackages/react-router/tests/not-found.test.tsxpackages/react-router/tests/store-updates-during-navigation.test.tsxpackages/router-core/src/load-matches.tspackages/router-core/tests/load.test.tspackages/solid-router/tests/not-found.test.tsxpackages/solid-router/tests/store-updates-during-navigation.test.tsxpackages/vue-router/tests/not-found.test.tsxpackages/vue-router/tests/store-updates-during-navigation.test.tsx
e2e/react-start/basic/src/routes/not-found/parent-boundary/route.tsx
Outdated
Show resolved
Hide resolved
| if (firstRedirect) { | ||
| throw firstRedirect |
There was a problem hiding this comment.
Should the redirection be honored if it was found "under" a notFound? I feel like whichever first error we see (notFound or redirect) is the one that should be used
There was a problem hiding this comment.
I think first one in wins, especially here.
There was a problem hiding this comment.
this is the scenario where a loader throws a redirect after a child beforeLoad threw a notFound
i think we cannot honor the notFound since there is no loader data for this route and so we cannot render its route component (the types would not make the loader data optional if you throw a redirect)
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/router-core/src/load-matches.ts (1)
951-975:⚠️ Potential issue | 🟠 MajorAvoid
allSettledblocking and enforce first terminal error precedence.At Line 954, waiting on
Promise.allSettled(matchPromises)can stall forever if any loader hangs, even after a redirect/notFound is already known. Also, Line 969 currently makes redirect always win over notFound once both exist; this can invert “first terminal error” behavior.💡 Proposed fix
- try { - await Promise.all(matchPromises) - } catch { - const settled = await Promise.allSettled(matchPromises) - - for (const result of settled) { - if (result.status !== 'rejected') continue - - const reason = result.reason - if (!firstRedirect && isRedirect(reason)) { - firstRedirect = reason - } - if (!firstNotFound && isNotFound(reason)) { - firstNotFound = reason - } - } - } + const trackedMatchPromises = matchPromises.map((p) => + p.catch((reason) => { + // First terminal error wins + if (!firstRedirect && !firstNotFound) { + if (isRedirect(reason)) firstRedirect = reason + else if (isNotFound(reason)) firstNotFound = reason + } + throw reason + }), + ) + + try { + await Promise.all(trackedMatchPromises) + } catch { + // firstRedirect / firstNotFound already captured by the wrappers above + }🤖 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 951 - 975, The current catch block blocks on Promise.allSettled(matchPromises) and always prefers redirect over notFound; instead, attach per-promise rejection handlers before awaiting Promise.all so rejections are recorded immediately: for each promise in matchPromises call .catch(reason => { if (!firstRedirect && isRedirect(reason)) firstRedirect = reason; if (!firstNotFound && isNotFound(reason)) firstNotFound = reason; throw reason }) so the earliest terminal errors are captured in chronological order without waiting for allSettled; after awaiting Promise.all(matchPromises) keep the existing throw-firstRedirect logic but compute notFoundToThrow using firstNotFound (which now reflects the earliest notFound) and preserve the beforeLoadNotFound fallback only when inner.preload is false.
🤖 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/router-core/src/load-matches.ts`:
- Around line 951-975: The current catch block blocks on
Promise.allSettled(matchPromises) and always prefers redirect over notFound;
instead, attach per-promise rejection handlers before awaiting Promise.all so
rejections are recorded immediately: for each promise in matchPromises call
.catch(reason => { if (!firstRedirect && isRedirect(reason)) firstRedirect =
reason; if (!firstNotFound && isNotFound(reason)) firstNotFound = reason; throw
reason }) so the earliest terminal errors are captured in chronological order
without waiting for allSettled; after awaiting Promise.all(matchPromises) keep
the existing throw-firstRedirect logic but compute notFoundToThrow using
firstNotFound (which now reflects the earliest notFound) and preserve the
beforeLoadNotFound fallback only when inner.preload is false.
63465c5 to
ca1f3bd
Compare
There was a problem hiding this comment.
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/ssr/ssr-server.ts (1)
45-56:⚠️ Potential issue | 🟠 MajorSerialize
gonly whenglobalNotFound === true.Line 54 currently serializes
gfor any non-undefinedglobalNotFoundvalue. IfglobalNotFoundisfalse, hydration can still treatgas present and incorrectly promote it to global not-found on the client.Suggested fix
const properties = [ ['__beforeLoadContext', 'b'], ['loaderData', 'l'], ['error', 'e'], ['ssr', 'ssr'], - ['globalNotFound', 'g'], ] as const for (const [key, shorthand] of properties) { if (match[key] !== undefined) { dehydratedMatch[shorthand] = match[key] } } + if (match.globalNotFound === true) { + dehydratedMatch.g = true + }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 `@packages/router-core/src/ssr/ssr-server.ts` around lines 45 - 56, The loop that fills dehydratedMatch incorrectly serializes the 'g' shorthand for any non-undefined match.globalNotFound; update the condition so that when key === 'globalNotFound' you only assign dehydratedMatch['g'] if match.globalNotFound === true, otherwise keep the existing undefined check for other keys (i.e., for keys !== 'globalNotFound' continue to use match[key] !== undefined); reference the properties array, the loop for (const [key, shorthand] of properties), match, and dehydratedMatch when making this change.
♻️ Duplicate comments (1)
packages/router-core/src/load-matches.ts (1)
950-954:⚠️ Potential issue | 🟠 Major
Promise.allSettledin the failure path can still deadlock redirect/notFound finalization.At Line 953, waiting for all promises to settle can block forever if any sibling loader hangs, even when another loader already rejected with a redirect/notFound.
💡 Minimal fast-path improvement
- try { - await Promise.all(matchPromises) - } catch { - const settled = await Promise.allSettled(matchPromises) + try { + await Promise.all(matchPromises) + } catch (err) { + if (isRedirect(err)) { + throw err + } + const settled = await Promise.allSettled(matchPromises)Also applies to: 959-964
🤖 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 950 - 954, The current catch block awaits Promise.allSettled(matchPromises) which can hang if any sibling loader never settles; change the logic so you short-circuit once a decisive rejection (redirect/notFound) is observed instead of blindly waiting for all promises. Concretely, wrap each entry in matchPromises with a small helper that races the original promise against a "settled marker" (or a timeout) so you can use Promise.race to detect the first rejection and then only await Promise.allSettled for the already-wrapped promises (or immediately proceed) rather than blocking indefinitely; update the catch block around await Promise.all(matchPromises) / const settled = await Promise.allSettled(matchPromises) to use this race/short-circuit approach with the same matchPromises identifier so redirect/notFound finalization is not deadlocked.
🧹 Nitpick comments (1)
packages/router-core/src/ssr/ssr-client.ts (1)
31-33: NormalizeglobalNotFounddeterministically during hydration.Line 31 only sets
globalNotFoundwhengis present, but does not clear it when omitted. Prefer assigning from the sentinel explicitly to avoid stale state carryover if match objects are reused.Suggested change
- if (deyhydratedMatch.g !== undefined) { - match.globalNotFound = true - } + match.globalNotFound = deyhydratedMatch.g === true ? true : undefinedAs 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 `@packages/router-core/src/ssr/ssr-client.ts` around lines 31 - 33, The hydration currently only sets match.globalNotFound when deyhydratedMatch.g exists, which can leave stale true values on reused match objects; change the logic in the hydration code that handles deyhydratedMatch and match (the block referencing deyhydratedMatch.g and match.globalNotFound) to deterministically assign a boolean instead of conditionally setting it — e.g., set match.globalNotFound to the result of the sentinel check (deyhydratedMatch.g !== undefined) so the flag is explicitly true or false during hydration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/router-core/src/ssr/ssr-server.ts`:
- Around line 45-56: The loop that fills dehydratedMatch incorrectly serializes
the 'g' shorthand for any non-undefined match.globalNotFound; update the
condition so that when key === 'globalNotFound' you only assign
dehydratedMatch['g'] if match.globalNotFound === true, otherwise keep the
existing undefined check for other keys (i.e., for keys !== 'globalNotFound'
continue to use match[key] !== undefined); reference the properties array, the
loop for (const [key, shorthand] of properties), match, and dehydratedMatch when
making this change.
---
Duplicate comments:
In `@packages/router-core/src/load-matches.ts`:
- Around line 950-954: The current catch block awaits
Promise.allSettled(matchPromises) which can hang if any sibling loader never
settles; change the logic so you short-circuit once a decisive rejection
(redirect/notFound) is observed instead of blindly waiting for all promises.
Concretely, wrap each entry in matchPromises with a small helper that races the
original promise against a "settled marker" (or a timeout) so you can use
Promise.race to detect the first rejection and then only await
Promise.allSettled for the already-wrapped promises (or immediately proceed)
rather than blocking indefinitely; update the catch block around await
Promise.all(matchPromises) / const settled = await
Promise.allSettled(matchPromises) to use this race/short-circuit approach with
the same matchPromises identifier so redirect/notFound finalization is not
deadlocked.
---
Nitpick comments:
In `@packages/router-core/src/ssr/ssr-client.ts`:
- Around line 31-33: The hydration currently only sets match.globalNotFound when
deyhydratedMatch.g exists, which can leave stale true values on reused match
objects; change the logic in the hydration code that handles deyhydratedMatch
and match (the block referencing deyhydratedMatch.g and match.globalNotFound) to
deterministically assign a boolean instead of conditionally setting it — e.g.,
set match.globalNotFound to the result of the sentinel check (deyhydratedMatch.g
!== undefined) so the flag is explicitly true or false during hydration.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
e2e/react-start/basic/src/routes/not-found/deep/b/c/d.tsxe2e/react-start/basic/src/routes/not-found/deep/b/c/route.tsxe2e/react-start/basic/src/routes/not-found/deep/b/route.tsxe2e/react-start/basic/src/routes/not-found/parent-boundary/route.tsxpackages/react-router/tests/not-found.test.tsxpackages/router-core/src/load-matches.tspackages/router-core/src/ssr/ssr-client.tspackages/router-core/src/ssr/ssr-server.tspackages/router-core/src/ssr/types.tspackages/router-core/tests/hydrate.test.tspackages/router-core/tests/load.test.tspackages/solid-router/tests/not-found.test.tsxpackages/vue-router/tests/not-found.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/react-start/basic/src/routes/not-found/parent-boundary/route.tsx
- packages/react-router/tests/not-found.test.tsx
- packages/vue-router/tests/not-found.test.tsx
fixes #6779
Summary by CodeRabbit
New Features
Documentation
Tests