fix(unplugin): generate param types from override paths and stop inheritance on absolute overrides#2646
fix(unplugin): generate param types from override paths and stop inheritance on absolute overrides#2646G100my wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds parsing and merging of path params when a route provides an explicit ChangesPath override and params extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
✅ Deploy Preview for vue-router canceled.
|
![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/src/unplugin/core/treeNodeValue.ts (1)
171-178: Cache merged overrides once insidegetPathParams().
this.overridesis recomputed on each access (sort + merge). Reusing one local object avoids duplicate work and clarifies intent.Proposed refactor
getPathParams(): TreePathParam[] { - const overridePath = this.overrides.path + const overrides = this.overrides + const overridePath = overrides.path if (!overridePath) { return this.isParam() ? [...this.pathParams] : [] } - const overrideParsers = this.overrides.params?.path ?? {} + const overrideParsers = overrides.params?.path ?? {} const params: TreePathParam[] = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router/src/unplugin/core/treeNodeValue.ts` around lines 171 - 178, In getPathParams(), avoid re-evaluating the costly computed property this.overrides multiple times: cache it into a local const (e.g., const overrides = this.overrides) at the top of the method and then use overrides.path and overrides.params?.path instead of re-accessing this.overrides; keep existing logic that checks overridePath and builds params (referencing overridePath, overrideParsers, TreePathParam, and this.pathParams) but read from the cached overrides to prevent duplicate sort/merge work.
🤖 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/src/unplugin/core/treeNodeValue.ts`:
- Around line 171-178: In getPathParams(), avoid re-evaluating the costly
computed property this.overrides multiple times: cache it into a local const
(e.g., const overrides = this.overrides) at the top of the method and then use
overrides.path and overrides.params?.path instead of re-accessing
this.overrides; keep existing logic that checks overridePath and builds params
(referencing overridePath, overrideParsers, TreePathParam, and this.pathParams)
but read from the cached overrides to prevent duplicate sort/merge work.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/router/src/unplugin/core/tree.spec.tspackages/router/src/unplugin/core/tree.tspackages/router/src/unplugin/core/treeNodeValue.ts
G100my
changed the title
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2646 +/- ##
==========================================
+ Coverage 85.58% 85.62% +0.04%
==========================================
Files 86 86
Lines 9960 9989 +29
Branches 2285 2304 +19
==========================================
+ Hits 8524 8553 +29
Misses 1423 1423
Partials 13 13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } satisfies Partial<TreePathParam>) | ||
| }) | ||
|
|
||
| describe('path override and params extraction', () => { |
There was a problem hiding this comment.
I copy pasted these tests in main and they all pass. Could you provide one failing test?
There was a problem hiding this comment.
I also tried copy-pasting the tests directly onto main, but they fail on my end.
I force-pushed an updated version that splits the changes into two commits:
- the first one only adds the tests,
- and the second one contains the actual code changes.
Hopefully this makes it easier to verify without having to copy and paste the tests manually.
I also reworked the tests a bit to make them more focused.
2 participants
I discovered this problem while migrating from
vite-plugin-pages.Problem
When overriding route
pathvia<route>ordefinePage():/-prefixed) can still inherit ancestor params/query in types, producing invalid keys.Solution
path, including::id?:id+/:id*:id(.*)params.pathparser overrides to inferred params.Summary by CodeRabbit
Tests
Bug Fixes