-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: Do not quote numerical search params #6000
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.
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/router-core/src/searchParams.ts (1)
74-83: Behavior change looks correct; consider tightening parser typing in this branchUsing
const parsed = parser(val); return stringify(parsed)is the right fix: it makesstringifySearchWithactually round‑trip parseable strings (e.g."123","true","null") according to the providedparser/stringifypair, and avoids adding extra quotes for numeric/boolean‑like search params. The silent error handling and overall control flow remain consistent withparseSearchWith, so this should be a safe and targeted behavior change.One minor follow‑up: since
parseris typed as optional, calling it here can trip strict TypeScript checks (Object is possibly 'undefined'). You already guard it viahasParser, but TS doesn’t narrow from that. If you want to keep strict mode fully happy, consider narrowing directly onparserin the condition, e.g.:} else if (parser && typeof val === 'string') { try { const parsed = parser(val) return stringify(parsed) } catch (_err) { // silent } }This preserves runtime behavior while aligning with strict type expectations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/searchParams.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety throughout the codebase
Files:
packages/router-core/src/searchParams.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: nlynzaad
Repo: TanStack/router PR: 5182
File: e2e/react-router/basic-file-based/src/routes/non-nested/named/$baz_.bar.tsx:3-5
Timestamp: 2025-09-22T00:56:49.237Z
Learning: In TanStack Router, underscores are intentionally stripped from route segments (e.g., `$baz_` becomes `baz` in generated types) but should be preserved in base path segments. This is the correct behavior as of the fix in PR #5182.
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 11m 9s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 20s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-12-01 19:37:23 UTC

The router adds quotes around numerical strings in search params, for example:
http://my-app.com?p=123changes tohttp://my-app.com?p="123"Maybe this is intentional, but seems a bit wonky to me, so here is a fix.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.