refactor(router-core): restore 'url' field on ParsedLocation#6939
refactor(router-core): restore 'url' field on ParsedLocation#6939
Conversation
🦋 Changeset detectedLatest commit: 7b3861b The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
documentation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a memoized, non-enumerable Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 unit tests (beta)
📝 Coding Plan
Comment |
|
View your CI Pipeline Execution ↗ for commit 447c9f8
☁️ Nx Cloud last updated this comment at |
Changeset Version Preview1 package(s) bumped directly, 21 bumped as dependents. Direct bumps
Dependency bumps (21)
|
![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: 26f6655750
ℹ️ 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".
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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)
1999-2042:⚠️ Potential issue | 🟠 MajorMemoize the internal URL, not the output-rewritten one.
Line 2006 stores
rewrittenUrlinmemoUrl, while Lines 2031-2037 still populatehrefandpathnamefrom the router-internal location. When an output rewrite changes the URL,buildLocation()ends up returning aParsedLocationwhoseurlis public whilehref/pathnameare internal, sobuildLocation()andparseLocation()no longer agree about whaturlrepresents.🛠️ Suggested fix
- const url = new URL(fullPath, this.origin) - const rewrittenUrl = executeRewriteOutput(this.rewrite, url) - memoUrl = rewrittenUrl - href = url.href.replace(url.origin, '') - origin = rewrittenUrl.origin + const internalUrl = new URL(fullPath, this.origin) + const rewrittenUrl = executeRewriteOutput( + this.rewrite, + new URL(internalUrl.href), + ) + memoUrl = internalUrl + href = internalUrl.href.replace(internalUrl.origin, '') + origin = internalUrl.origin🤖 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 1999 - 2042, The bug is that memoUrl is set to the output-rewritten URL (rewrittenUrl) causing mismatch between the internal href/pathname and the exported url; change memoization to store the internal URL used by the router so buildLocation() and parseLocation() agree: in the rewrite branch, set memoUrl = url (the URL constructed from fullPath and this.origin) instead of memoUrl = rewrittenUrl, keep origin = rewrittenUrl.origin and the existing publicHref/ href/ pathname logic, and ensure augmentLocationWithUrl is still called with that internal memoUrl so the ParsedLocation.url reflects the router-internal location while publicHref reflects the rewritten output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/router-core/src/router.ts`:
- Around line 1305-1320: The temporary parsed location used for masked
navigations (`__tempLocation` / `parsedTempLocation`) loses the non-enumerable
`url` when it is recreated via object spread (e.g., `{ ...parsedTempLocation,
maskedLocation: location }`), so ensure you restore the `url` there too: instead
of relying on the spread, call augmentLocationWithUrl(parsedTempLocation,
this.origin!) (or after the spread call Object.defineProperty to re-add the
non-enumerable `url`) so masked navigations produce a ParsedLocation with `url`
set; update the same pattern around the other similar return paths (the block
around lines 1337–1353) where `parsedTempLocation` is reconstructed.
---
Outside diff comments:
In `@packages/router-core/src/router.ts`:
- Around line 1999-2042: The bug is that memoUrl is set to the output-rewritten
URL (rewrittenUrl) causing mismatch between the internal href/pathname and the
exported url; change memoization to store the internal URL used by the router so
buildLocation() and parseLocation() agree: in the rewrite branch, set memoUrl =
url (the URL constructed from fullPath and this.origin) instead of memoUrl =
rewrittenUrl, keep origin = rewrittenUrl.origin and the existing publicHref/
href/ pathname logic, and ensure augmentLocationWithUrl is still called with
that internal memoUrl so the ParsedLocation.url reflects the router-internal
location while publicHref reflects the rewritten output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3bcd65ef-d8b0-4b06-a3e7-3ddd5b9a9c1a
📒 Files selected for processing (4)
.changeset/five-otters-feel.mddocs/router/api/router/ParsedLocationType.mdpackages/router-core/src/location.tspackages/router-core/src/router.ts
1 participant
back by popular demand
Summary by CodeRabbit
New Features
urlproperty (exposed as a computed, memoized getter).Documentation
urlis computed on demand; repeatedly accessing it in tight/hot loops may impact performance.