refactor(router-core): no validateSearch in buildLocation#6979
refactor(router-core): no validateSearch in buildLocation#6979
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 15m 26s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 52s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-03-20 11:04:20 UTC
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0af5ff06a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| matchedRoutes, | ||
| fullPath: lastRoute.fullPath, | ||
| search: accumulatedSearch, | ||
| search: location.search, |
There was a problem hiding this comment.
This drops the route-validated/defaulted search state whenever a caller preserves or updates the current search (search: true or search: prev => ...). buildLocation callers like the framework Link components (packages/react-router/src/link.tsx:418-419) and redirects (packages/router-core/src/router.ts:2711-2714) do not pass _includeValidateSearch, so they now see the raw parsed query instead of the validated one. In the redirect scenario covered by packages/react-router/tests/link.test.tsx:3948-4019, the callback should receive { postPage: 0 } from the current route's validateSearch, but after this change it receives {}, so links/redirects to routes that do not reintroduce the same validator lose existing defaulted params and generate the wrong URL/search state.
Useful? React with 👍 / 👎.
Merging this PR will improve performance by 3.56%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | client-side navigation loop (react) |
44.8 ms | 43.2 ms | +3.56% |
Comparing refactor-router-core-no-search-validation-in-build-location (9c41ff7) with main (91cc628)1
Footnotes
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/router-core/src/router.ts (1)
1693-1739: The change correctly removes search validation from lightweight matching, but the uncertain comment should be resolved.The refactor to return
location.searchdirectly instead of validated search is consistent with the PR objective. This is a reasonable optimization since:
- The source location's search was already validated when that route was originally matched
- Destination route validation still occurs later in
buildLocationwhen_includeValidateSearchis set (lines 1903-1921)However, the comment on lines 1693-1694 expressing uncertainty ("I don't know if we should run the full search middleware chain, or just validateSearch") reads like an unresolved TODO rather than a design rationale. Consider either:
- Rephrasing to document the deliberate decision (e.g., "We intentionally skip validation here because...")
- Removing the commented-out code if this approach is confirmed
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/router.ts` around lines 1693 - 1739, Remove or replace the uncertain TODO comment and commented-out code around the lightweight match return to reflect the deliberate design: either delete the commented-out applySearchMiddleware block and the "I don't know..." line, or replace it with a concise explanatory comment such as "Intentionally skip full search middleware here; source search was validated on original match and destination validation runs in buildLocation via _includeValidateSearch." Reference the surrounding symbols matchedRoutes, location.search, buildLocation, and _includeValidateSearch to ensure the explanatory comment makes clear why location.search is returned directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/router-core/src/router.ts`:
- Around line 1693-1739: Remove or replace the uncertain TODO comment and
commented-out code around the lightweight match return to reflect the deliberate
design: either delete the commented-out applySearchMiddleware block and the "I
don't know..." line, or replace it with a concise explanatory comment such as
"Intentionally skip full search middleware here; source search was validated on
original match and destination validation runs in buildLocation via
_includeValidateSearch." Reference the surrounding symbols matchedRoutes,
location.search, buildLocation, and _includeValidateSearch to ensure the
explanatory comment makes clear why location.search is returned directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89a7fa96-5f4d-4784-92ef-0f043c553fdb
📒 Files selected for processing (1)
packages/router-core/src/router.ts
1 participant
Summary by CodeRabbit