refactor: use object-syntax/named routes#1041
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis PR consistently replaces string-path navigations with named-route objects and introduces a centralized packageRoute helper used across the app. It adds a basePath: string[] prop to code-browser components (FileTree, DirectoryListing, MobileTreeDrawer) and updates tests to supply this prop. usePackageRoute now derives packageName, requestedVersion and orgName from typed route params. Client-side canonical-redirects middleware was removed and equivalent server-side middleware added. nuxt.config routeRules were made more granular and several minimal stub package pages were added to support routing. Possibly related PRs
🚥 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: 3
🧹 Nitpick comments (1)
app/pages/package-code/[...path].vue (1)
201-205: Consider explicit handling for null version.The
version.value ?? ''fallback works in practice because the Code components are only rendered whenfileTreeis loaded (which requires a valid version). However, for type clarity and defensive coding, consider making the nullability explicit or adding an early return.The current implementation is safe due to the template guards, but this could be made clearer:
🔧 Optional: More explicit null handling
// Base path segments for route objects (e.g., ['nuxt', 'v', '4.2.0'] or ['@nuxt', 'kit', 'v', '1.0.0']) const basePath = computed(() => { + if (!version.value) return [] const segments = packageName.value.split('/') - return [...segments, 'v', version.value ?? ''] + return [...segments, 'v', version.value] })
| :to="{ | ||
| name: '~username', | ||
| params: { username: maintainer.username || maintainer.name || '' }, | ||
| }" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find and examine the TableRow.vue file to see the full context
fd -t f "TableRow.vue"Repository: npmx-dev/npmx.dev
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Search for maintainer type definitions in the codebase
rg -t ts -t tsx "type.*[Mm]aintainer|interface.*[Mm]aintainer" -A 5Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Look for npm-registry related type definitions that might define Maintainer
fd -t f -e ts -e tsx | xargs grep -l "maintainer\|Maintainer" | head -20Repository: npmx-dev/npmx.dev
Length of output: 585
🏁 Script executed:
#!/bin/bash
# Read the npm-registry types file
cat shared/types/npm-registry.tsRepository: npmx-dev/npmx.dev
Length of output: 9637
🏁 Script executed:
#!/bin/bash
# Check the size and then read TableRow.vue
wc -l app/components/Package/TableRow.vueRepository: npmx-dev/npmx.dev
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Read TableRow.vue to see the full context around lines 109-112
cat -n app/components/Package/TableRow.vueRepository: npmx-dev/npmx.dev
Length of output: 7783
Guard against empty maintainer route parameters.
Lines 109–112 can pass params: { username: '' } to the route when both username and name are undefined, resulting in an invalid profile link. Render a plain span displaying the email address when no username or name is available.
🔧 Proposed fix
<NuxtLink
+ v-if="maintainer.username || maintainer.name"
:to="{
name: '~username',
- params: { username: maintainer.username || maintainer.name || '' },
+ params: { username: maintainer.username || maintainer.name },
}"
class="hover:text-accent-fallback transition-colors duration-200"
`@click.stop`
- >{{ maintainer.username || maintainer.name || maintainer.email }}</NuxtLink
+ >{{ maintainer.username || maintainer.name }}</NuxtLink
><span v-if="idx < Math.min(pkg.maintainers.length, 3) - 1">, </span>
+ <span v-else class="text-fg-subtle">{{ maintainer.email }}</span>| const docsLink = computed(() => { | ||
| if (!resolvedVersion.value) return null | ||
|
|
||
| return `/package-docs/${pkg.value!.name}/v/${resolvedVersion.value}` | ||
| return { | ||
| name: 'docs' as const, | ||
| params: { | ||
| path: [pkg.value!.name, 'v', resolvedVersion.value] satisfies [string, string, string], | ||
| }, | ||
| } | ||
| }) |
There was a problem hiding this comment.
Add null check for pkg.value to ensure type safety.
The computed property checks resolvedVersion.value but uses a non-null assertion on pkg.value!.name without verifying pkg.value exists. During the loading transition, resolvedVersion may be populated before pkg loads, which could cause a runtime error.
🛡️ Proposed fix to add defensive null check
const docsLink = computed(() => {
- if (!resolvedVersion.value) return null
+ if (!resolvedVersion.value || !pkg.value) return null
return {
name: 'docs' as const,
params: {
- path: [pkg.value!.name, 'v', resolvedVersion.value] satisfies [string, string, string],
+ path: [pkg.value.name, 'v', resolvedVersion.value] satisfies [string, string, string],
},
}
})As per coding guidelines: "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing".
📝 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.
| const docsLink = computed(() => { | |
| if (!resolvedVersion.value) return null | |
| return `/package-docs/${pkg.value!.name}/v/${resolvedVersion.value}` | |
| return { | |
| name: 'docs' as const, | |
| params: { | |
| path: [pkg.value!.name, 'v', resolvedVersion.value] satisfies [string, string, string], | |
| }, | |
| } | |
| }) | |
| const docsLink = computed(() => { | |
| if (!resolvedVersion.value || !pkg.value) return null | |
| return { | |
| name: 'docs' as const, | |
| params: { | |
| path: [pkg.value.name, 'v', resolvedVersion.value] satisfies [string, string, string], | |
| }, | |
| } | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
nuxt.config.ts (1)
293-299: Type assertion masks the actual return shape.The
as { expiration: number }assertion on line 298 hides thefallbackproperty from TypeScript, which could lead to confusion when consuming this configuration. Consider using a proper union type or Nuxt's ISR types instead.💡 Suggested improvement
-function getISRConfig(expirationSeconds: number, fallback = false) { +function getISRConfig(expirationSeconds: number, fallback = false): { expiration: number; fallback?: string } { if (fallback) { return { expiration: expirationSeconds, fallback: 'spa.prerender-fallback.html', - } as { expiration: number } + } } return { expiration: expirationSeconds, } }
| /** | ||
| * Redirect legacy URLs to canonical paths (client-side only) | ||
| * | ||
| * - /package/code/* → /package-code/* | ||
| * - /code/* → /package-code/* | ||
| * - /package/docs/* → /package-docs/* | ||
| * - /docs/* → /package-docs/* | ||
| * - /org/* → /@* | ||
| * - /* → /package/* (Unless it's an existing page) | ||
| */ |
There was a problem hiding this comment.
Documentation does not match implementation.
The comment block describes several redirects that are not implemented:
/package/code/*→/package-code/*— not implemented/code/*→/package-code/*— not implemented/package/docs/*→/package-docs/*— not implemented/docs/*→/package-docs/*— not implemented
Additionally, the comment says /org/* → /@* but the code on lines 33-35 does the opposite: /@org → /org/org.
Please update the documentation to accurately reflect the implemented redirects, or implement the missing rules.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nuxt.config.ts (1)
293-303:⚠️ Potential issue | 🟡 MinorType assertion misrepresents the actual return type.
The
as { expiration: number }assertion on line 298 hides thefallbackproperty from TypeScript, creating a mismatch between the runtime value and the declared type. This violates strict type safety.If this is a workaround for Nuxt's incomplete ISR types, consider defining an accurate type or using a union return type.
🛡️ Suggested fix
+interface ISRConfig { + expiration: number + fallback?: string +} + -function getISRConfig(expirationSeconds: number, fallback = false) { +function getISRConfig(expirationSeconds: number, fallback = false): ISRConfig { if (fallback) { return { expiration: expirationSeconds, fallback: 'spa.prerender-fallback.html', - } as { expiration: number } + } } return { expiration: expirationSeconds, } }If Nuxt's
RouteRuleConfigtype doesn't accept thefallbackproperty and you need to bypass type checking, at minimum document why the assertion is needed:// Type assertion needed: Nuxt's ISR types don't include 'fallback' yet } as { expiration: number }As per coding guidelines: "Ensure you write strictly type-safe code".
🧹 Nitpick comments (1)
nuxt.config.ts (1)
90-93: Consider extracting the cache duration to a named constant.The value
365 * 24 * 60 * 60(one year in seconds) is repeated across multiple route rules. Extracting it to a constant would improve readability and make future adjustments easier.♻️ Suggested refactor
+const ONE_YEAR_IN_SECONDS = 365 * 24 * 60 * 60 + export default defineNuxtConfig({ // ... routeRules: { // API routes '/api/**': { isr: 60 }, - '/api/registry/docs/**': { isr: true, cache: { maxAge: 365 * 24 * 60 * 60 } }, - '/api/registry/file/**': { isr: true, cache: { maxAge: 365 * 24 * 60 * 60 } }, - '/api/registry/provenance/**': { isr: true, cache: { maxAge: 365 * 24 * 60 * 60 } }, - '/api/registry/files/**': { isr: true, cache: { maxAge: 365 * 24 * 60 * 60 } }, + '/api/registry/docs/**': { isr: true, cache: { maxAge: ONE_YEAR_IN_SECONDS } }, + '/api/registry/file/**': { isr: true, cache: { maxAge: ONE_YEAR_IN_SECONDS } }, + '/api/registry/provenance/**': { isr: true, cache: { maxAge: ONE_YEAR_IN_SECONDS } }, + '/api/registry/files/**': { isr: true, cache: { maxAge: ONE_YEAR_IN_SECONDS } },Apply the same pattern to the
/package-code/**and/package-docs/**rules on lines 114-116.
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)
nuxt.config.ts (1)
295-305:⚠️ Potential issue | 🔴 CriticalRemove unsupported
fallbackproperty fromgetISRConfig—it is not valid in Nitro's ISR configuration.The
fallbackproperty at line 299 is not supported by Nitro's ISR config schema. Whilst Nitro does supportisr: { expiration, bypassToken, ... }and other serializable fields (includingpassQueryas seen at line 106), afallbackproperty is not among them. The type assertion at line 300 masks this mistake by casting to{ expiration: number }, hiding the fact that thefallbackproperty will be silently ignored at runtime and the intended fallback behaviour will not work.If you need fallback rendering on Vercel, that is handled separately through Vercel's prerender fallback configuration, not through Nitro route rules.
Recommendation: Remove the
fallbackparameter and the conditional logic ingetISRConfig, or refactor if you have a separate mechanism for setting up fallback behaviour:function getISRConfig(expirationSeconds: number, fallback = false) { - if (fallback) { - return { - expiration: expirationSeconds, - fallback: 'spa.prerender-fallback.html', - } as { expiration: number } - } return { expiration: expirationSeconds, } }Then remove the
fallbackargument from calls at lines 111–114.
🧹 Nitpick comments (7)
app/composables/usePackageRoute.ts (1)
11-11: Consider broadening the route type constraint to match documented usage.The composable documents support for both non-versioned and versioned package URLs (lines 4–8), and includes a guard for
'version' in route.params(line 18). However,useRoute<'package'>('package')on line 11 constrains the route to only the 'package' name. Nuxt's file-based routing generates separate route names ('package' for/package/[name]and 'package-version' for/package/[name]/v/[version]), so the type does not align with the stated design intent.Currently the composable is only called from the base package page, so this is not a runtime issue. If this composable is intended to support versioned routes in future, or if the version handling on line 18 is meant to cover broader usage, consider either:
- Updating the route constraint to a union of both route names, or
- Documenting that this composable is for non-versioned routes only and removing the version handling
server/middleware/canonical-redirects.global.ts (4)
27-30: The> 1threshold on route rules is a magic number — please document the rationale.
Object.keys(routeRules).length > 1implies there is always at least one default key in the returned object. If that assumption changes in a future Nitro/Nuxt version, this guard silently breaks and the middleware either skips everything or nothing.Add a short comment explaining why the threshold is
1(e.g. which default key is always present), or consider a more explicit check such as inspecting specific rule properties.Nuxt getRouteRules default return value
47-47: Consider using 301 (permanent) instead of the default 302 for canonical redirects.These are legacy/shorthand URL redirects to stable canonical paths. A 302 (temporary) means every request re-hits the server and search engines won't transfer link equity. Since the canonical targets are not expected to change, 301 is the standard choice here.
- return sendRedirect(event, `/package/${args}`) + return sendRedirect(event, `/package/${args}`, 301)(Same for lines 58 and 64.)
Also applies to: 58-58, 64-64
32-32: Prefer a safe fallback over the non-null assertion.
split('?')[0]is always defined in practice, but the coding guidelines ask for safe access when indexing into arrays. A nullish coalescing fallback avoids the!assertion with no cost.- const path = event.path.split('?')[0]! + const path = event.path.split('?')[0] ?? event.pathAs per coding guidelines, "ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".
26-26: Unnecessaryasync— the handler contains noawaitexpressions.Removing
asyncavoids wrapping every return value in an extra resolved promise. Minor nit, but it also signals to readers that the handler is synchronous.-export default defineEventHandler(async event => { +export default defineEventHandler(event => {modules/isr-fallback.ts (1)
29-36: Optional: extract the duplicated directory path resolution.The directory path
resolve(nitro.options.output.serverDir, '..', path)is computed twice — once as part ofoutputPath(lines 29–32) and again formkdirSync(line 35). Extracting it into a local variable would reduce duplication and make the intent clearer.♻️ Suggested refactor
- const outputPath = resolve( - nitro.options.output.serverDir, - '..', - path, - 'spa.prerender-fallback.html', - ) - mkdirSync(resolve(nitro.options.output.serverDir, '..', path), { recursive: true }) - writeFileSync(outputPath, spaTemplate) + const dir = resolve(nitro.options.output.serverDir, '..', path) + const outputPath = resolve(dir, 'spa.prerender-fallback.html') + mkdirSync(dir, { recursive: true }) + writeFileSync(outputPath, spaTemplate)nuxt.config.ts (1)
90-93: Consider extracting the repeated magic number365 * 24 * 60 * 60.The expression
365 * 24 * 60 * 60(one year in seconds) appears seven times across the route rules. A named constant would improve readability and reduce the risk of inconsistent edits.♻️ Suggested refactor
Add above
defineNuxtConfig:const ONE_YEAR_IN_SECONDS = 365 * 24 * 60 * 60Then replace all occurrences, e.g.:
- '/api/registry/docs/**': { isr: true, cache: { maxAge: 365 * 24 * 60 * 60 } }, + '/api/registry/docs/**': { isr: true, cache: { maxAge: ONE_YEAR_IN_SECONDS } },Also applies to: 116-118
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
server/middleware/canonical-redirects.global.ts (2)
1-14: Documentation largely improved, but line 7 is still ambiguous.
/@org → /org/orgreads as though both segments are the literal string "org". Consider using a placeholder syntax like/@{org} → /org/{org}to distinguish the captured group from the literal path prefix.
29-33: Clarify thegetRouteRuleslength check.
Object.keys(routeRules).length > 1assumes that every route has exactly one default rule key, so "more than 1" means explicit custom rules exist. This is an implicit contract with Nitro's internals. A brief inline comment explaining the rationale (e.g.// routes with explicit routeRules are handled by Nitro) would help future maintainers.
| > | ||
| <NuxtLink | ||
| :to="`/package/${typesPackageName}`" | ||
| :to="packageRoute(typesPackageName!)" |
There was a problem hiding this comment.
Looks risky. I think we should either specify that the type is required in the props, or make it optional inside the method and if (!name) return undefined.
| return | ||
| } | ||
|
|
||
| const [path = '/', query] = event.path.split('?') |
There was a problem hiding this comment.
Could be sth like https://npmx.dev/nuxt#user-content-getting-started
It seems to be working now, apparently the path doesn't include query and hash and perhaps this logic is unnecessary
There was a problem hiding this comment.
hash is typically not sent to the server at all
| path.match(/^\/(?:(?<org>@[^/]+)\/)?(?<name>[^/@]+)\/v\/(?<version>[^/]+)$/) || | ||
| path.match(/^\/(?:(?<org>@[^/]+)\/)?(?<name>[^/@]+)@(?<version>[^/]+)$/) |
There was a problem hiding this comment.
same as
path.match(/^\/(?:(?<org>@[^/]+)\/)?(?<name>[^/@]+)(?:\/v\/|@)(?<version>[^/]+)$/)
(not-captured group with \/v\/ or @)
|
@danielroe I was just checking how it will work now and caught these little moments. They're all minor, so it just to not to miss them 🙃 |
2 participants
closes #158