refactor: accept versioned pkg route builder in useCommandPaletteVersionCommands#2635
Open
refactor: accept versioned pkg route builder in useCommandPaletteVersionCommands#2635
useCommandPaletteVersionCommands#2635Conversation
The placeholder shape made callers that already had named-route helpers resolve routes to strings and then patch encoded placeholders back into place. That was brittle, especially for routes where Vue Router encodes params differently from the URL shape users see. We have a documented guideline that internal navigation should use route object syntax with named routes instead of strings. This commit allows callers to respect that guideline.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Contributor
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaces URL-pattern string routing for command-palette version navigation with typed route-builder callbacks across the app. Introduces a timeline command and centralized Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/pages/package/[[org]]/[name].vue (1)
490-492: Optional: redundant with the composable's default fallback.The composable already defaults to
packageRoute(context.packageName, version)when norouteForVersionis supplied, so this callback duplicates that behaviour. You could omit the second argument entirely on this page and rely on the default, which would also let you drop the inline arrow.Proposed simplification
-useCommandPaletteVersionCommands(commandPalettePackageContext, version => - packageRoute(packageName.value, version), -) +useCommandPaletteVersionCommands(commandPalettePackageContext)That said, keeping it explicit is fine if you prefer the call sites to be uniform across pages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/package/`[[org]]/[name].vue around lines 490 - 492, The call to useCommandPaletteVersionCommands currently passes an explicit callback duplicating the composable's default; remove the second argument so the call becomes useCommandPaletteVersionCommands(commandPalettePackageContext) and let the composable use its built-in fallback (which constructs packageRoute(context.packageName, version)), or if you want to keep it explicit for consistency leave as-is; locate the call to useCommandPaletteVersionCommands in this file and remove the inline arrow that returns packageRoute(packageName.value, version) to simplify.app/pages/package-docs/[...path].vue (1)
127-137: Nit: the destructuring is reconstructing the array it just split.
[firstSegment = name, ...remainingSegments] = name.split('/')followed by[firstSegment, ...remainingSegments, 'v', version]is equivalent to[...name.split('/'), 'v', version]. The destructuring appears to exist solely to satisfy the[string, ...string[]]non-empty-tuple shape without a cast. Consider either spreading directly with a cast (matching the existing pattern at lines 52 and 66 in the same file), or extracting a small helper to share the conversion. Functionally fine as-is.Possible simplification
function docsVersionRoute(version: string): RouteLocationRaw { const name = pkg.value?.name || packageName.value - const [firstSegment = name, ...remainingSegments] = name.split('/') - return { name: 'docs', params: { - path: [firstSegment, ...remainingSegments, 'v', version], + path: [...name.split('/'), 'v', version] as [string, ...string[]], }, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/package-docs/`[...path].vue around lines 127 - 137, The destructuring in docsVersionRoute (using [firstSegment = name, ...remainingSegments] = name.split('/')) reconstructs the same array and is unnecessary; replace it by building params.path directly from the split result using the same non-empty-tuple cast/pattern used elsewhere (see the file's existing pattern around lines 52/66) or factor a small helper to convert string.split('/') into a [string, ...string[]] tuple, then set params.path to [...thatTuple, 'v', version]; update references to pkg and packageName as needed to compute the base name before the split.app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue (1)
90-99: Consider returning a named-route object for consistency with the PR objective.The PR description notes that the motivation for accepting a route builder is to "use route object syntax with named routes". Here,
codeVersionRoutestill returns a path string viagetCodeUrl, while sibling pages (docsVersionRoute,fromVersionRoute/diffRoute, and the package page) all return named-route objects. Aligning this caller would make the intent uniform and avoid relying on Vue Router's path parsing for params that may need encoding (e.g., scoped names, file paths with special characters).Proposed refactor
function codeVersionRoute(nextVersion: string): RouteLocationRaw { - return getCodeUrl({ - org: route.params.org, - packageName: route.params.packageName, - version: nextVersion, - filePath: filePath.value, - }) + return { + name: 'code', + params: { + org: route.params.org, + packageName: route.params.packageName, + version: nextVersion, + filePath: filePath.value ?? '', + }, + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/package-code/`[[org]]/[packageName]/v/[version]/[...filePath].vue around lines 90 - 99, codeVersionRoute currently returns a path string from getCodeUrl but should return a named-route object like the other builders (docsVersionRoute, fromVersionRoute/diffRoute) to preserve params and encoding; change codeVersionRoute to return an object of the form { name: '<code-route-name>', params: { org: route.params.org, packageName: route.params.packageName, version: nextVersion, filePath: filePath.value } } (or use the same route name constant used elsewhere) and pass that into useCommandPaletteVersionCommands so params are preserved and routed via Vue Router's named-route syntax instead of a path string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/pages/package-code/`[[org]]/[packageName]/v/[version]/[...filePath].vue:
- Around line 90-99: codeVersionRoute currently returns a path string from
getCodeUrl but should return a named-route object like the other builders
(docsVersionRoute, fromVersionRoute/diffRoute) to preserve params and encoding;
change codeVersionRoute to return an object of the form { name:
'<code-route-name>', params: { org: route.params.org, packageName:
route.params.packageName, version: nextVersion, filePath: filePath.value } } (or
use the same route name constant used elsewhere) and pass that into
useCommandPaletteVersionCommands so params are preserved and routed via Vue
Router's named-route syntax instead of a path string.
In `@app/pages/package-docs/`[...path].vue:
- Around line 127-137: The destructuring in docsVersionRoute (using
[firstSegment = name, ...remainingSegments] = name.split('/')) reconstructs the
same array and is unnecessary; replace it by building params.path directly from
the split result using the same non-empty-tuple cast/pattern used elsewhere (see
the file's existing pattern around lines 52/66) or factor a small helper to
convert string.split('/') into a [string, ...string[]] tuple, then set
params.path to [...thatTuple, 'v', version]; update references to pkg and
packageName as needed to compute the base name before the split.
In `@app/pages/package/`[[org]]/[name].vue:
- Around line 490-492: The call to useCommandPaletteVersionCommands currently
passes an explicit callback duplicating the composable's default; remove the
second argument so the call becomes
useCommandPaletteVersionCommands(commandPalettePackageContext) and let the
composable use its built-in fallback (which constructs
packageRoute(context.packageName, version)), or if you want to keep it explicit
for consistency leave as-is; locate the call to useCommandPaletteVersionCommands
in this file and remove the inline arrow that returns
packageRoute(packageName.value, version) to simplify.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 169b561f-bd1c-4767-a514-3083879efdd3
📒 Files selected for processing (6)
app/composables/useCommandPaletteVersionCommands.tsapp/pages/diff/[[org]]/[packageName]/v/[versionRange].vueapp/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vueapp/pages/package-docs/[...path].vueapp/pages/package/[[org]]/[name].vuetest/nuxt/composables/use-command-palette-commands.spec.ts
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
1 participant
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.
🔗 Linked issue
N/A
🧭 Context
The string placeholder shape used by
useCommandPaletteVersionCommandsmade callers that already had named route helpers resolve routes to strings and then patch encoded placeholders back into place. This is brittle, especially for routes where vue-router encodes params differently from the URL shape users see.We have a documented guideline that internal navigation should use route object syntax with named routes instead of strings. This shape was preventing us from respecting our own guideline.
📚 Description
Pass a versioned package route builder to
useCommandPaletteVersionCommandsinstead of a string with a version placeholder.