refactor: add proper route segments for package code #1301
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR replaces the array-based Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/pages/package/[[org]]/[name].vue (1)
519-533: Avoid non‑null assertions when splitting package names.The split array indices are asserted with
!. Prefer explicit guards to keep index access type‑safe and avoid unexpected runtime issues when the name is malformed.
As per coding guidelines, "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".♻️ Suggested refactor
-const linkCode = computed((): Parameters<typeof navigateTo>[0] | null => { - if (pkg.value == null || resolvedVersion.value == null) { - return null - } - const split = pkg.value.name.split('/') - return { - name: 'code', - params: { - org: split.length === 2 ? split[0]?.replace(/^@/, '') : undefined, - packageName: split.length === 2 ? split[1]! : split[0]!, - version: resolvedVersion.value, - filePath: '', - }, - } -}) +const linkCode = computed((): Parameters<typeof navigateTo>[0] | null => { + if (pkg.value == null || resolvedVersion.value == null) return null + + const [first, second] = pkg.value.name.split('/') + const isScoped = pkg.value.name.startsWith('@') && !!second + const packageName = isScoped ? second : first + if (!packageName) return null + + return { + name: 'code', + params: { + org: isScoped ? first.replace(/^@/, '') : undefined, + packageName, + version: resolvedVersion.value, + filePath: '', + }, + } +})
app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue
Show resolved
Hide resolved
essenmitsosse
force-pushed
the
refactor-change-routing
branch
from
c7a2dab to
b08b19d
Compare
|
this is absolutely something we should be doing - would you check the failing CI? |
essenmitsosse
force-pushed
the
refactor-change-routing
branch
from
28eda99 to
7648ef9
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Ok, so unfortunately there is no elegant way to enforce the @danielroe should be fine now. |
| function getCodeLink(filePath: string): RouteLocationRaw { | ||
| const split = props.packageName.split('/') | ||
|
|
||
| console.log({ split }) | ||
| return { | ||
| name: 'code', | ||
| params: { | ||
| org: split.length === 2 ? split[0] : null, | ||
| packageName: split.length === 2 ? split[1]! : split[0]!, | ||
| version: props.version, | ||
| filePath: '', | ||
| }, | ||
| } |
There was a problem hiding this comment.
Use filePath and avoid null for org when building code links.
getCodeLink currently ignores the filePath argument (so links always go to the root) and sets org to null, which can produce /package-code/null/... for unscoped packages. Also, the debug console.log should be removed.
💡 Suggested fix
function getCodeLink(filePath: string): RouteLocationRaw {
- const split = props.packageName.split('/')
-
- console.log({ split })
+ const scopedName = props.packageName.startsWith('@')
+ const [org, name] = scopedName
+ ? props.packageName.slice(1).split('/')
+ : [undefined, props.packageName]
return {
name: 'code',
params: {
- org: split.length === 2 ? split[0] : null,
- packageName: split.length === 2 ? split[1]! : split[0]!,
+ org,
+ packageName: name,
version: props.version,
- filePath: '',
+ filePath,
},
}
}📝 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.
| function getCodeLink(filePath: string): RouteLocationRaw { | |
| const split = props.packageName.split('/') | |
| console.log({ split }) | |
| return { | |
| name: 'code', | |
| params: { | |
| org: split.length === 2 ? split[0] : null, | |
| packageName: split.length === 2 ? split[1]! : split[0]!, | |
| version: props.version, | |
| filePath: '', | |
| }, | |
| } | |
| function getCodeLink(filePath: string): RouteLocationRaw { | |
| const scopedName = props.packageName.startsWith('@') | |
| const [org, name] = scopedName | |
| ? props.packageName.slice(1).split('/') | |
| : [undefined, props.packageName] | |
| return { | |
| name: 'code', | |
| params: { | |
| org, | |
| packageName: name, | |
| version: props.version, | |
| filePath, | |
| }, | |
| } | |
| } |
app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue
Show resolved
Hide resolved
| const codeLink = computed((): RouteLocationRaw | null => { | ||
| if (pkg.value == null || resolvedVersion.value == null) { | ||
| return null | ||
| } | ||
| const split = pkg.value.name.split('/') | ||
| return { | ||
| name: 'code', | ||
| params: { | ||
| org: split.length === 2 ? split[0] : undefined, | ||
| packageName: split.length === 2 ? split[1]! : split[0]!, | ||
| version: resolvedVersion.value, | ||
| filePath: '', | ||
| }, | ||
| } |
There was a problem hiding this comment.
Strip the leading “@” when building org for code routes.
For scoped packages, pkg.value.name is @scope/pkg, so split[0] includes @. The code page reconstructs as @${org}/${packageName}, which becomes @@scope/pkg and breaks code navigation for scoped packages. Use orgName or strip the prefix before setting params.
💡 Suggested fix
- const split = pkg.value.name.split('/')
- return {
- name: 'code',
- params: {
- org: split.length === 2 ? split[0] : undefined,
- packageName: split.length === 2 ? split[1]! : split[0]!,
- version: resolvedVersion.value,
- filePath: '',
- },
- }
+ const scopedName = pkg.value.name.startsWith('@')
+ const [org, name] = scopedName
+ ? pkg.value.name.slice(1).split('/')
+ : [undefined, pkg.value.name]
+ return {
+ name: 'code',
+ params: {
+ org,
+ packageName: name,
+ version: resolvedVersion.value,
+ filePath: undefined,
+ },
+ }
|
seems like a couple of good comments from @coderabbitai 🙏 |
|
Tip For best results, initiate chat on the files or code changes.
|
|
Tip For best results, initiate chat on the files or code changes.
(ﾉ◕ヮ◕)ﾉ*:･ﾟ✧ 🐰 |
| function getCurrentCodeUrl(path?: string): string { | ||
| return getCodeUrl({ | ||
| ...route.params, | ||
| filePath: path, | ||
| }) |
There was a problem hiding this comment.
Preserve the current file path when building URLs.
getCurrentCodeUrl() overwrites filePath with undefined when called without a path, so Line 251’s canonical URL (and any no-arg use) points to the package root even when a file is open. Only override filePath when a value is provided.
💡 Suggested fix
function getCurrentCodeUrl(path?: string): string {
return getCodeUrl({
...route.params,
- filePath: path,
+ ...(path !== undefined ? { filePath: path } : {}),
})
}Also applies to: 251-251
|
there may be some patterns here: #158 (comment) |
2 participants
This is the first step to get rid of the catch all segments in routing.
If this looks good and we want to continue like this, I would like to change all routes to be properly described. Once they are all converted, a lot of the manual path splitting and build code can disappear and we can just pass around the route objects which should remove a lot of boiler plate code. It also will improve typing.
This also "fixes" the escaping of the slash in package names with an org.