fix: prevent crash in buildLocation when params stringification fails (#6310) #6349
+5
−1
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
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 Fix all issues with AI agents
In @packages/router-core/src/router.ts:
- Around line 1704-1708: The empty catch swallowing errors in the try around
Object.assign(nextParams, fn(nextParams)) hides stringify failures; update the
catch to log the error (including the error object and relevant context such as
nextParams and fn.name or a stringified fn) so developers can debug, using the
router's existing logger if available (e.g., this.logger or a module logger) or
fallback to console.error, and guard the log behind a development check (e.g.,
process.env.NODE_ENV !== 'production' or an existing debug flag) so production
noise is avoided.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/router.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/router-core/src/router.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/src/router.ts
🧠 Learnings (5)
📓 Common learnings
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.
📚 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
📚 Learning: 2025-12-17T02:17:55.086Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:55.086Z
Learning: In `packages/router-generator/src/generator.ts`, pathless_layout routes must receive a `path` property when they have a `cleanedPath`, even though they are non-path routes. This is necessary because child routes inherit the path from their parent, and without this property, child routes would not have the correct full path at runtime.
Applied to files:
packages/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/src/router.ts
📚 Learning: 2025-12-21T12:52:35.231Z
Learnt from: Sheraff
Repo: TanStack/router PR: 6171
File: packages/router-core/src/new-process-route-tree.ts:898-898
Timestamp: 2025-12-21T12:52:35.231Z
Learning: In `packages/router-core/src/new-process-route-tree.ts`, the matching logic intentionally allows paths without trailing slashes to match index routes with trailing slashes (e.g., `/a` can match `/a/` route), but not vice-versa (e.g., `/a/` cannot match `/a` layout route). This is implemented via the condition `!pathIsIndex || node.kind === SEGMENT_TYPE_INDEX` and is a deliberate design decision to provide better UX by being permissive with missing trailing slashes.
Applied to files:
packages/router-core/src/router.ts
Comment on lines
+1704
to
+1708
| try { | ||
| Object.assign(nextParams, fn(nextParams)) | ||
| } catch (err) { | ||
| // | ||
| } |
Contributor
There was a problem hiding this comment.
Add logging for stringify failures to aid debugging.
While preventing crashes is the right approach for resilience, the empty catch block makes it difficult to diagnose when and why parameter stringification fails. This could hide bugs or configuration issues that developers need to be aware of.
Consider logging the error at least in development mode:
🔍 Proposed enhancement to add development logging
try {
Object.assign(nextParams, fn(nextParams))
} catch (err) {
- //
+ if (process.env.NODE_ENV !== 'production') {
+ console.warn(
+ `Failed to stringify params for route ${route.id}:`,
+ err,
+ )
+ }
}This maintains the crash prevention while providing visibility into stringify failures during development.
📝 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
| try { | |
| Object.assign(nextParams, fn(nextParams)) | |
| } catch (err) { | |
| // | |
| } | |
| try { | |
| Object.assign(nextParams, fn(nextParams)) | |
| } catch (err) { | |
| if (process.env.NODE_ENV !== 'production') { | |
| console.warn( | |
| `Failed to stringify params for route ${route.id}:`, | |
| err, | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In @packages/router-core/src/router.ts around lines 1704 - 1708, The empty catch
swallowing errors in the try around Object.assign(nextParams, fn(nextParams))
hides stringify failures; update the catch to log the error (including the error
object and relevant context such as nextParams and fn.name or a stringified fn)
so developers can debug, using the router's existing logger if available (e.g.,
this.logger or a module logger) or fallback to console.error, and guard the log
behind a development check (e.g., process.env.NODE_ENV !== 'production' or an
existing debug flag) so production noise is avoided.
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.
Description
Fixes #6310
I investigated the issue using the reproduction repository provided in the issue.
Currently,
buildLocationattempts to executestringifyeven whenparams.parsefails or throws an error. In SSR environments, this causes the server to throw a 500 error (crash) instead of rendering theNotFoundboundary.Proposed Solution
I wrapped the
stringifyexecution in atry-catchblock withinbuildLocation.If stringification fails, it now silently ignores the error. This prevents the app from crashing and allows the router to render the
NotFoundcomponent correctly.Before:
After:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.