fix(router-core): keep useLocation on the rendered route's location - #8038
Open
ulrichstark wants to merge 1 commit into
Open
fix(router-core): keep useLocation on the rendered route's location#8038ulrichstark wants to merge 1 commit into
ulrichstark wants to merge 1 commit into
Conversation
Contributor
|
View your CI Pipeline Execution ↗ for commit 962cf68
☁️ Nx Cloud last updated this comment at |
Contributor
📝 WalkthroughWalkthroughChangesPresented location synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
ulrichstark
changed the title
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/router-core/src/router.ts`:
- Around line 1086-1087: Update the _committedLocation property to use
ParsedLocation<FullSearchSchema<TRouteTree>> instead of the default
ParsedLocation type, matching latestLocation so route-specific search keys and
value types are preserved.
In `@packages/solid-router/src/useLocation.tsx`:
- Around line 30-41: Add braces to both unbraced if statements in the
useLocation flow: the opts?.select guard and the prev === undefined check inside
Solid.createMemo. Preserve their existing return behavior and do not change
surrounding selection logic.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f128de46-8c05-4de1-8d4d-310bd07653f8
📒 Files selected for processing (10)
.changeset/use-location-presented-lane.mddocs/router/api/router/useLocationHook.mdpackages/react-router/src/useLocation.tsxpackages/react-router/tests/issue-8037-use-location-stale-pathname.test.tsxpackages/router-core/src/load-client.tspackages/router-core/src/load-server.tspackages/router-core/src/router.tspackages/router-core/src/stores.tspackages/solid-router/src/useLocation.tsxpackages/vue-router/src/useLocation.tsx
Comment on lines
+1086
to
+1087
| /** The location that produced `_committed`. */ | ||
| _committedLocation?: ParsedLocation |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file outline ---'
ast-grep outline packages/router-core/src/router.ts | head -120
printf '%s\n' '--- relevant declarations and usages ---'
rg -n -C 4 'ParsedLocation|FullSearchSchema|_committedLocation|latestLocation' packages/router-core/src/router.ts packages/router-core/srcRepository: TanStack/router
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- location and schema types ---'
cat -n packages/router-core/src/location.ts | sed -n '1,70p'
rg -n -C 3 'export type AnySchema|type AnySchema' packages/router-core/src
printf '%s\n' '--- RouterCore declaration and fields ---'
cat -n packages/router-core/src/router.ts | sed -n '1000,1125p'
printf '%s\n' '--- committed-location signatures and calls ---'
rg -n -C 8 '_committedLocation|function publishMatches|publishMatches\(' packages/router-core/src/load-client.ts packages/router-core/src/router.tsRepository: TanStack/router
Length of output: 18632
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- load-client type aliases and location-bearing types ---'
rg -n -C 10 'type CoordinatorRouter|interface CoordinatorRouter|type LoadTransaction|type ProjectedLane|type PublicationCheckpoint|previousPresentedLocation|function publishMatches' packages/router-core/src/load-client.ts
printf '%s\n' '--- location-producing declarations passed to publication ---'
sed -n '145,245p' packages/router-core/src/load-client.ts
sed -n '1565,1630p' packages/router-core/src/load-client.ts
sed -n '2620,2660p' packages/router-core/src/load-client.ts
printf '%s\n' '--- compiler availability ---'
if command -v tsc >/dev/null 2>&1; then
tsc --version
else
printf '%s\n' 'tsc unavailable'
fiRepository: TanStack/router
Length of output: 12429
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- AnyRouter and route-info aliases ---'
rg -n -C 8 'export type AnyRouter|type AnyRouter|export type FullSearchSchema|type FullSearchSchema|type ParseRoute|PartialMergeAll' packages/router-core/src/router.ts packages/router-core/src/routeInfo.ts packages/router-core/src
printf '%s\n' '--- strict assignability probe ---'
cat > /tmp/router-location-type-probe.ts <<'TS'
type AnySchema = {}
interface ParsedLocation<TSearchObj extends AnySchema = {}> {
search: TSearchObj
maskedLocation?: ParsedLocation<TSearchObj>
}
type RequiredSearch = { required: string }
declare const bare: ParsedLocation
declare const typed: ParsedLocation<RequiredSearch>
const typedFromBare: ParsedLocation<RequiredSearch> = bare
const bareFromTyped: ParsedLocation = typed
interface Router<TRouteTree> {
committed?: ParsedLocation<TRouteTree>
}
declare const router: Router<RequiredSearch>
router.committed = bare
TS
tsc --strict --noEmit --skipLibCheck /tmp/router-location-type-probe.ts 2>&1 || trueRepository: TanStack/router
Length of output: 17099
🏁 Script executed:
#!/bin/bash
set -eu
cat > /tmp/router-location-type-probe.ts <<'TS'
type AnySchema = {}
interface ParsedLocation<TSearchObj extends AnySchema = {}> {
search: TSearchObj
maskedLocation?: ParsedLocation<TSearchObj>
}
type RequiredSearch = { required: string }
type OptionalSearch = { optional?: string }
declare const bare: ParsedLocation
declare const requiredTyped: ParsedLocation<RequiredSearch>
declare const optionalTyped: ParsedLocation<OptionalSearch>
const requiredFromBare: ParsedLocation<RequiredSearch> = bare
const optionalFromBare: ParsedLocation<OptionalSearch> = bare
const bareFromRequired: ParsedLocation = requiredTyped
const bareFromOptional: ParsedLocation = optionalTyped
interface Router<TRouteTree> {
committed?: ParsedLocation<TRouteTree>
}
declare const requiredRouter: Router<RequiredSearch>
declare const optionalRouter: Router<OptionalSearch>
requiredRouter.committed = bare
optionalRouter.committed = bare
TS
tsc --ignoreConfig --strict --noEmit --skipLibCheck /tmp/router-location-type-probe.ts 2>&1 || trueRepository: TanStack/router
Length of output: 751
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- search-schema merge helper ---'
sed -n '95,132p' packages/router-core/src/utils.ts
printf '%s\n' '--- optional route-schema probe ---'
cat > /tmp/router-location-optional-schema-probe.ts <<'TS'
type AnySchema = {}
interface ParsedLocation<TSearchObj extends AnySchema = {}> {
search: TSearchObj
}
type FullSearchSchema = { page?: number; filter?: string }
declare const bare: ParsedLocation
declare const typed: ParsedLocation<FullSearchSchema>
const page: number | undefined = typed.search.page
const filter: string | undefined = typed.search.filter
const bareFromTyped: ParsedLocation = typed
// These must fail: the bare default loses the route-specific keys.
const missingPage: number | undefined = bare.search.page
const missingFilter: string | undefined = bare.search.filter
TS
tsc --ignoreConfig --strict --noEmit --skipLibCheck /tmp/router-location-optional-schema-probe.ts 2>&1 || trueRepository: TanStack/router
Length of output: 1346
Preserve the route search schema in _committedLocation.
ParsedLocation defaults its search type to {}, which removes route-specific search keys and value types. Use ParsedLocation<FullSearchSchema<TRouteTree>>, consistent with latestLocation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/router-core/src/router.ts` around lines 1086 - 1087, Update the
_committedLocation property to use ParsedLocation<FullSearchSchema<TRouteTree>>
instead of the default ParsedLocation type, matching latestLocation so
route-specific search keys and value types are preserved.
Source: Coding guidelines
Comment on lines
30
to
41
| if (!opts?.select) { | ||
| return (() => router.stores.location.get()) as Accessor< | ||
| return (() => router.stores.presentedLocation.get()) as Accessor< | ||
| UseLocationResult<TRouter, TSelected> | ||
| > | ||
| } | ||
|
|
||
| const select = opts.select | ||
|
|
||
| return Solid.createMemo((prev: TSelected | undefined) => { | ||
| const res = select(router.stores.location.get()) | ||
| const res = select(router.stores.presentedLocation.get()) | ||
| if (prev === undefined) return res | ||
| return replaceEqualDeep(prev, res) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add braces to both if statements.
Line 30 and line 40 use unbraced control bodies. Add braces while changing this function.
Proposed fix
- if (!opts?.select) {
+ if (!opts?.select) {
return (() => router.stores.presentedLocation.get()) as Accessor<
UseLocationResult<TRouter, TSelected>
>
}
@@
- if (prev === undefined) return res
+ if (prev === undefined) {
+ return res
+ }As per coding guidelines, **/*.{ts,tsx} requires braces for every control statement. Based on learnings, changed Solid adapter code must fix directly related unbraced branches.
📝 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
| if (!opts?.select) { | |
| return (() => router.stores.location.get()) as Accessor< | |
| return (() => router.stores.presentedLocation.get()) as Accessor< | |
| UseLocationResult<TRouter, TSelected> | |
| > | |
| } | |
| const select = opts.select | |
| return Solid.createMemo((prev: TSelected | undefined) => { | |
| const res = select(router.stores.location.get()) | |
| const res = select(router.stores.presentedLocation.get()) | |
| if (prev === undefined) return res | |
| return replaceEqualDeep(prev, res) | |
| if (!opts?.select) { | |
| return (() => router.stores.presentedLocation.get()) as Accessor< | |
| UseLocationResult<TRouter, TSelected> | |
| > | |
| } | |
| const select = opts.select | |
| return Solid.createMemo((prev: TSelected | undefined) => { | |
| const res = select(router.stores.presentedLocation.get()) | |
| if (prev === undefined) { | |
| return res | |
| } | |
| return replaceEqualDeep(prev, res) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/solid-router/src/useLocation.tsx` around lines 30 - 41, Add braces
to both unbraced if statements in the useLocation flow: the opts?.select guard
and the prev === undefined check inside Solid.createMemo. Preserve their
existing return behavior and do not change surrounding selection logic.
Sources: Coding guidelines, Learnings
Merging this PR will regress 11 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem server error-paths unmatched (react) |
434.1 KB | 548.7 KB | -20.89% |
| ❌ | Memory | mem client unique-location-churn (solid) |
384.9 KB | 449.4 KB | -14.35% |
| ❌ | Memory | mem server server-fn-churn (vue) |
327.8 KB | 369.9 KB | -11.38% |
| ❌ | Memory | mem server error-paths redirect (vue) |
439.7 KB | 493.9 KB | -10.97% |
| ❌ | Memory | mem client interrupted-navigations (vue) |
512.8 KB | 541.6 KB | -5.32% |
| ❌ | Memory | mem server error-paths redirect (solid) |
366.3 KB | 386.8 KB | -5.28% |
| ❌ | Simulation | client-route-tree-scale navigation loop (vue) |
253.8 ms | 267.2 ms | -5.03% |
| ❌ | Memory | mem server request-churn (react) |
649.8 KB | 678.6 KB | -4.25% |
| ❌ | Simulation | client-history navigation loop (vue) |
154.9 ms | 160.8 ms | -3.67% |
| ❌ | Simulation | client-history navigation loop (react) |
133.6 ms | 138.4 ms | -3.47% |
| ❌ | Simulation | client-mount loop (react) |
129.8 ms | 134 ms | -3.12% |
| ⚡ | Memory | mem server request-churn (vue) |
2,905.2 KB | 862.6 KB | ×3.4 |
| ⚡ | Memory | mem server serialization-payload (solid) |
5.9 MB | 4.6 MB | +28% |
| ⚡ | Memory | mem client unique-location-churn (vue) |
1.3 MB | 1.1 MB | +22.12% |
| ⚡ | Memory | mem server error-paths not-found (vue) |
573.3 KB | 471.8 KB | +21.51% |
| ⚡ | Memory | mem server aborted-requests (vue) |
1,103 KB | 1,007.1 KB | +9.51% |
| ⚡ | Memory | mem client navigation-churn (solid) |
714.4 KB | 658.2 KB | +8.55% |
| ⚡ | Memory | mem server peak-large-page (solid) |
1.2 MB | 1.1 MB | +5.08% |
| ⚡ | Memory | mem server error-paths not-found (react) |
417.3 KB | 399.4 KB | +4.48% |
| ⚡ | Memory | mem server error-paths unmatched (vue) |
608.4 KB | 586.4 KB | +3.75% |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ulrichstark:fix-issue-8037 (962cf68) with main (783f883)
Collaborator
|
i actually kinda like this proposal. But this would be a breaking change, some people actually rely on |
Contributor
Author
|
Since the original issue is fixed for me, we can do anything you like with this PR. Do you want me to close it or turn it into a draft PR? |
Collaborator
I'll keep it on the side for v2
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This change is AI generated.
useLocation read router.stores.location, which is written when a navigation starts rather than when its matches are published, so a component on the route being left re-rendered once with the destination's pathname before unmounting.
Add stores.presentedLocation, published atomically with the match lane inside setMatches, and read it from useLocation in react, solid, and vue. router.state.location is unchanged and still reports the requested location while it loads.
Closes #8037
Summary by CodeRabbit
Bug Fixes
useLocationbehavior across React, Solid, and Vue routers so it reflects the location associated with currently rendered routes.router.state.locationduring loading.Documentation
useLocationupdates and how to observe requested locations during navigation.Tests