fix: handle notFound from params.parse correctly #5864
+68
−4
Conversation
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
Contributor
WalkthroughModifies path-parameter error handling in RouterCore to propagate NotFound and Redirect errors directly from params.parse, while wrapping other errors in PathParamError. Updates paramsError type from Changes
Sequence DiagramsequenceDiagram
participant Router
participant ParamParser as Param Parser
participant ErrorHandler as Error Handler
rect rgb(200, 220, 255)
Note over Router,ErrorHandler: Old Behavior (All errors wrapped)
Router->>ParamParser: parse(params)
ParamParser-->>Router: Error (any type)
Router->>ErrorHandler: Wrap in PathParamError
ErrorHandler-->>Router: PathParamError
end
rect rgb(220, 255, 220)
Note over Router,ErrorHandler: New Behavior (Type-aware propagation)
Router->>ParamParser: parse(params)
alt NotFound or Redirect
ParamParser-->>Router: NotFound/Redirect
Router->>ErrorHandler: Propagate as-is
else Other Error
ParamParser-->>Router: Other Error
Router->>ErrorHandler: Wrap in PathParamError
end
ErrorHandler-->>Router: NotFound/Redirect or PathParamError
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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: 1
🧹 Nitpick comments (2)
packages/router-core/tests/load.test.ts (2)
512-570: Consider extracting shared test setup to reduce duplication.Both tests have nearly identical setup code (route creation, router instantiation). This could be refactored into a shared helper function or
beforeEachblock to improve maintainability.Example refactor:
describe('params.parse notFound', () => { const setupRouter = (initialPath: string) => { const rootRoute = new BaseRootRoute({}) const testRoute = new BaseRoute({ getParentRoute: () => rootRoute, path: '/test/$id', params: { parse: ({ id }: { id: string }) => { const parsed = parseInt(id, 10) if (Number.isNaN(parsed)) { throw notFound() } return { id: parsed } }, }, }) const routeTree = rootRoute.addChildren([testRoute]) const router = new RouterCore({ routeTree, history: createMemoryHistory({ initialEntries: [initialPath] }), }) return { router, testRoute } } test('throws notFound on invalid params', async () => { const { router, testRoute } = setupRouter('/test/invalid') await router.load() const match = router.state.pendingMatches?.find( (m) => m.routeId === testRoute.id, ) expect(match?.status).toBe('notFound') }) test('succeeds on valid params', async () => { const { router, testRoute } = setupRouter('/test/123') await router.load() const match = router.state.matches.find((m) => m.routeId === testRoute.id) expect(match?.status).toBe('success') expect(router.state.statusCode).toBe(200) }) })
512-570: Consider adding test coverage for redirect() in params.parse.The PR changes handle
redirect()thrown fromparams.parseas a special case (similar tonotFound()), but there's no test coverage for this scenario. Adding a test would ensure redirect behavior works correctly.Example test:
test('throws redirect on conditional params', async () => { const rootRoute = new BaseRootRoute({}) const testRoute = new BaseRoute({ getParentRoute: () => rootRoute, path: '/test/$id', params: { parse: ({ id }: { id: string }) => { if (id === 'redirect-me') { throw redirect({ to: '/other' }) } return { id } }, }, }) const routeTree = rootRoute.addChildren([testRoute]) const router = new RouterCore({ routeTree, history: createMemoryHistory({ initialEntries: ['/test/redirect-me'] }), }) await router.load() expect(router.state.redirect).toBeDefined() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-core/src/router.ts(2 hunks)packages/router-core/tests/load.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: FatahChan
Repo: TanStack/router PR: 5475
File: e2e/react-start/basic-prerendering/src/routes/redirect/$target/via-beforeLoad.tsx:8-0
Timestamp: 2025-10-14T18:59:33.990Z
Learning: In TanStack Router e2e test files, when a route parameter is validated at the route level (e.g., using zod in validateSearch or param validation), switch statements on that parameter do not require a default case, as the validation ensures only expected values will reach the switch.
📚 Learning: 2025-10-14T18:59:33.990Z
Learnt from: FatahChan
Repo: TanStack/router PR: 5475
File: e2e/react-start/basic-prerendering/src/routes/redirect/$target/via-beforeLoad.tsx:8-0
Timestamp: 2025-10-14T18:59:33.990Z
Learning: In TanStack Router e2e test files, when a route parameter is validated at the route level (e.g., using zod in validateSearch or param validation), switch statements on that parameter do not require a default case, as the validation ensures only expected values will reach the switch.
Applied to files:
packages/router-core/tests/load.test.tspackages/router-core/src/router.ts
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
packages/router-core/tests/load.test.ts
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
packages/router-core/src/router.ts
🧬 Code graph analysis (1)
packages/router-core/src/router.ts (2)
packages/router-core/src/index.ts (3)
isNotFound(380-380)isRedirect(374-374)PathParamError(204-204)packages/router-core/src/redirect.ts (1)
isRedirect(118-120)
🔇 Additional comments (2)
packages/router-core/src/router.ts (2)
1382-1382: Type change enables direct propagation of NotFound and Redirect errors.The type change from
PathParamError | undefinedtounknownis correct. This allowsparamsErrorto holdNotFoundError,Redirect, orPathParamErrorobjects, enabling the router to distinguish between navigation control flow errors (notFound/redirect) and actual parameter validation errors.
1395-1401: Error handling correctly differentiates NotFound/Redirect from validation errors.The updated logic properly handles the three error cases:
NotFoundandRedirecterrors are assigned directly toparamsError, allowing the router to recognize them as navigation control flow- Other errors are wrapped in
PathParamErrorto indicate parameter validation failuresThis resolves issue #5733 where
notFound()thrown fromparams.parsewas incorrectly wrapped inPathParamError, resulting in a 500 response instead of 404.
Comment on lines
+513
to
+541
| test('throws notFound on invalid params', async () => { | ||
| const rootRoute = new BaseRootRoute({}) | ||
| const testRoute = new BaseRoute({ | ||
| getParentRoute: () => rootRoute, | ||
| path: '/test/$id', | ||
| params: { | ||
| parse: ({ id }: { id: string }) => { | ||
| const parsed = parseInt(id, 10) | ||
| if (Number.isNaN(parsed)) { | ||
| throw notFound() | ||
| } | ||
| return { id: parsed } | ||
| }, | ||
| }, | ||
| }) | ||
| const routeTree = rootRoute.addChildren([testRoute]) | ||
| const router = new RouterCore({ | ||
| routeTree, | ||
| history: createMemoryHistory({ initialEntries: ['/test/invalid'] }), | ||
| }) | ||
|
|
||
| await router.load() | ||
|
|
||
| const match = router.state.pendingMatches?.find( | ||
| (m) => m.routeId === testRoute.id, | ||
| ) | ||
|
|
||
| expect(match?.status).toBe('notFound') | ||
| }) |
Contributor
There was a problem hiding this comment.
Add statusCode assertion to verify 404 response.
The test verifies the match status is 'notFound' but doesn't check that the router's statusCode is set to 404. According to the PR objectives, this test should verify both the match status and the statusCode.
Add this assertion after line 540:
expect(match?.status).toBe('notFound')
+expect(router.state.statusCode).toBe(404)📝 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
| test('throws notFound on invalid params', async () => { | |
| const rootRoute = new BaseRootRoute({}) | |
| const testRoute = new BaseRoute({ | |
| getParentRoute: () => rootRoute, | |
| path: '/test/$id', | |
| params: { | |
| parse: ({ id }: { id: string }) => { | |
| const parsed = parseInt(id, 10) | |
| if (Number.isNaN(parsed)) { | |
| throw notFound() | |
| } | |
| return { id: parsed } | |
| }, | |
| }, | |
| }) | |
| const routeTree = rootRoute.addChildren([testRoute]) | |
| const router = new RouterCore({ | |
| routeTree, | |
| history: createMemoryHistory({ initialEntries: ['/test/invalid'] }), | |
| }) | |
| await router.load() | |
| const match = router.state.pendingMatches?.find( | |
| (m) => m.routeId === testRoute.id, | |
| ) | |
| expect(match?.status).toBe('notFound') | |
| }) | |
| test('throws notFound on invalid params', async () => { | |
| const rootRoute = new BaseRootRoute({}) | |
| const testRoute = new BaseRoute({ | |
| getParentRoute: () => rootRoute, | |
| path: '/test/$id', | |
| params: { | |
| parse: ({ id }: { id: string }) => { | |
| const parsed = parseInt(id, 10) | |
| if (Number.isNaN(parsed)) { | |
| throw notFound() | |
| } | |
| return { id: parsed } | |
| }, | |
| }, | |
| }) | |
| const routeTree = rootRoute.addChildren([testRoute]) | |
| const router = new RouterCore({ | |
| routeTree, | |
| history: createMemoryHistory({ initialEntries: ['/test/invalid'] }), | |
| }) | |
| await router.load() | |
| const match = router.state.pendingMatches?.find( | |
| (m) => m.routeId === testRoute.id, | |
| ) | |
| expect(match?.status).toBe('notFound') | |
| expect(router.state.statusCode).toBe(404) | |
| }) |
🤖 Prompt for AI Agents
In packages/router-core/tests/load.test.ts around lines 513 to 541, the test
checks that the match status is 'notFound' but doesn't assert the router's
overall statusCode; add an assertion after the existing
expect(match?.status).toBe('notFound') to also expect router.statusCode to equal
404 so the test verifies both the match status and the router's 404 statusCode.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Fixes #5733
Previously, these errors were wrapped in PathParamError, causing a 500 error instead of 404.
When notFound() is thrown from params.parse, the router now correctly Sets match status to notFound
Changes
load.test.tsfor params.parse throwing notFound()Test Plan
Summary by CodeRabbit
Bug Fixes
Tests