perf(router-core): cache current-location lightweight matches#7011
perf(router-core): cache current-location lightweight matches#7011
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces an internal lightweight-match cache keyed to location object identity, refactors Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 12m 20s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 42s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-03-22 18:37:21 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: b745f7d39f
ℹ️ 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".
packages/router-core/src/router.ts
Outdated
| // From search should always use the current location | ||
| const fromSearch = lightweightResult.search | ||
| const fromSearch = currentLocation.search |
There was a problem hiding this comment.
_fromLocation
When buildLocation is driven from an _fromLocation (render-time <Link>s do this via packages/react-router/src/link.tsx:407, and preloadRoute retries redirects with _fromLocation: next in router.ts:2863-2866), currentLocation.search is only the raw parsed query string, not the route-validated search state. Before this change matchRoutesLightweight re-ran validateSearch, so defaults like validateSearch: () => ({ postPage: 0 }) were still visible to search updaters even when the URL omitted ?postPage=. Now those updaters receive {} instead, which breaks relative links and redirect/preload flows that rely on inherited defaults (for example the redirect-preload case covered in packages/react-router/tests/link.test.tsx:3899-4020).
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/router.ts (1)
1844-1849:⚠️ Potential issue | 🟠 MajorPreserve a validated search snapshot here.
Line 1845 now feeds the live
currentLocation.searchinto the middleware/updater chain. Because thevalidateSearchmiddlewares only run on unwind,search.middlewaresanddest.searchcallbacks now observe raw parsed search instead of the validated/defaulted snapshot they used to receive. Any in-place mutation can also leak back into router state because this object is no longer copied likefromParamsis. Please keep using a validated snapshot here (cached if you want the perf win) instead of the live location search.🤖 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 1844 - 1849, The code sets fromSearch = currentLocation.search which feeds the live mutable search into middleware/updater chain; instead capture and use the validated/defaulted search snapshot used by validateSearch (not the live object) — e.g., build fromSearch the same way fromParams is created (a shallow copy of the validated search snapshot from lightweightResult or the cached validatedSearch), and ensure search.middlewares and dest.search receive that copied snapshot so in-place mutations cannot leak back into router state.
🧹 Nitpick comments (1)
packages/router-core/src/router.ts (1)
1690-1693: Refresh the lightweight-matching docstrings.These comments still mention lightweight
searchaccumulation, but the result now only carriesmatchedRoutes,fullPath, andparams.Also applies to: 1808-1810
🤖 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 1690 - 1693, Update the docstrings that describe the lightweight route matching used by buildLocation: remove references to accumulating `search` and the older list of skipped items (AbortController, ControlledPromise, loaderDeps, full match objects), and instead state precisely that this lightweight match only returns `matchedRoutes`, `fullPath`, and `params`. Locate and edit the comment blocks around the `buildLocation`/lightweight matching implementation (the docblocks near the current lines ~1690 and ~1808) to reflect the new return shape and behavior.
🤖 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/router.ts`:
- Around line 1844-1849: The code sets fromSearch = currentLocation.search which
feeds the live mutable search into middleware/updater chain; instead capture and
use the validated/defaulted search snapshot used by validateSearch (not the live
object) — e.g., build fromSearch the same way fromParams is created (a shallow
copy of the validated search snapshot from lightweightResult or the cached
validatedSearch), and ensure search.middlewares and dest.search receive that
copied snapshot so in-place mutations cannot leak back into router state.
---
Nitpick comments:
In `@packages/router-core/src/router.ts`:
- Around line 1690-1693: Update the docstrings that describe the lightweight
route matching used by buildLocation: remove references to accumulating `search`
and the older list of skipped items (AbortController, ControlledPromise,
loaderDeps, full match objects), and instead state precisely that this
lightweight match only returns `matchedRoutes`, `fullPath`, and `params`. Locate
and edit the comment blocks around the `buildLocation`/lightweight matching
implementation (the docblocks near the current lines ~1690 and ~1808) to reflect
the new return shape and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 230c6370-10f4-4301-9458-594d504e2f0f
📒 Files selected for processing (1)
packages/router-core/src/router.ts
Merging this PR will not alter performance
Comparing 
|
1 participant
Summary
matchRoutesLightweightwhenbuildLocationis called withrouter.stores.location.statefromSearchfromcurrentLocationdirectly and drop the unused lightweight search accumulationTesting
CI=1 NX_DAEMON=false pnpm nx run @tanstack/router-core:test:unit --outputStyle=stream --skipRemoteCache -- tests/build-location.test.tsCI=1 NX_DAEMON=false pnpm nx run @tanstack/router-core:test:types --outputStyle=stream --skipRemoteCacheCI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf --outputStyle=stream --skipRemoteCacheSummary by CodeRabbit