fix: refactor routing to fix errors#1575
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! |
3340156 to
2b3b8ef
Compare
11e7ec3 to
332697c
Compare
d9bc476 to
3ae3c0d
Compare
3ae3c0d to
7069d10
Compare
9475eef to
f471f07
Compare
ab57490 to
2688747
Compare
📝 WalkthroughWalkthroughIntroduces explicit trailing-slash handling site-wide: a new Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
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 |
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 (5)
app/components/Package/DeprecatedTree.vue (1)
86-93:⚠️ Potential issue | 🔴 CriticalRemove
trailing-slash="append"— feature not available in Nuxt 4.3.1The
trailingSlashper-link prop on<NuxtLink>was added in Nuxt v3.17.0 (released April 27, 2025, via PR#31820). However, it is not available in Nuxt 4.3.1. The v4.3.1 release notes do not include this feature, and trailing-slash behaviour in Nuxt 4 is controlled only via global configuration, not per-link attributes. Remove thetrailing-slash="append"attribute to prevent the prop from being silently ignored or causing unexpected behaviour.test/nuxt/components/VersionSelector.spec.ts (1)
306-326:⚠️ Potential issue | 🟠 MajorTrailing slash on file-path links breaks the code-page file viewer.
The updated expectation confirms that VersionSelector now generates a trailing slash for links whose URL pattern contains a file path (e.g.
src/index.ts/). This conflicts with thecurrentNodelogic inapp/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue, which the author explicitly documented as treating/src/index.ts/as an "incorrect file path":// - /src/index.ts/ - incorrect file path (but formally can exist as a directory)When a user selects a version from VersionSelector while viewing a file, the navigation resolves to e.g.
/package-code/pkg/v/2.0.0/src/index.ts/. IncurrentNode:
parts = ['src', 'index.ts', ''](3 segments)- At
i=1:index.tsis a file butisLast=false, so it continues;current = found.children→undefined- At
i=2:part='',isLast=true, butlastFound.type === 'file'(not'directory') → the guard fails → returnsnullResult:
isViewingFile = false, and the template falls through to show the directory listing instead of the file viewer.The simplest fix in
currentNodeis to extend the guard to also returnlastFoundwhen it is a file:🐛 Proposed fix in
currentNode-if (!part && isLast && lastFound?.type === 'directory') return lastFound +if (!part && isLast && lastFound) return lastFoundAlternatively, apply
trailing-slash="remove"(or omit the prop) on VersionSelector links that contain file-path segments.app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue (1)
315-373:⚠️ Potential issue | 🟠 MajorDirect
NuxtLinkchanges are correct — but note the cross-file regression.The three
trailing-slash="append"additions here are all on directory-type or package-page routes:
- Line 316: package route ✓
- Line 354: tree root (no file path) ✓
- Line 365: intermediate breadcrumb items (always directories, since the last crumb renders as
<span>) ✓However, the
currentNodecomputation (line 77–102) explicitly documents that a file path with a trailing slash (e.g.src/index.ts/) is treated as invalid and returnsnull. SinceversionUrlPattern(line 58–65) forwards the currently-viewed file path toVersionSelector, and VersionSelector now hastrailing-slash="append"on its version links, switching versions while on a file view will navigate to a URL thatcurrentNodecannot resolve — causing the file viewer to silently fall back to the directory listing. See the comment raised inVersionSelector.spec.tsfor the full analysis and proposed fix.app/pages/~[username]/orgs.vue (1)
84-84:⚠️ Potential issue | 🟠 MajorRemove stray unconditional
error.valueassignment.Line 84 sets
error.valueto an error string during every component setup run, beforeloadOrgs()has a chance to execute.loadOrgsonly resetserrorwhenisOwnProfileistrue; for non-connected or non-owner users it returns early without clearing this stale value. Thev-else-if="!isOwnProfile"guard in the template currently hides the error state for those users, but this is an accidental safeguard — the error should simply not be set.🐛 Proposed fix
-error.value = $t('header.orgs_dropdown.error') - // Load on mount and when connection status changes watch(isOwnProfile, loadOrgs, { immediate: true })app/middleware/trailing-slash.global.ts (1)
1-9:⚠️ Potential issue | 🟡 MinorJSDoc comment is the inverse of the actual behaviour and references removed configuration.
The comment states the middleware "removes trailing slashes" and shows examples of slash removal (
/package/vue/ → /package/vue), but the code appends them (line 23:to.path + '/'). It also claims the middleware "only runs in development" and that "Vercel handles this redirect via vercel.json", but theimport.meta.devguard was removed and the VerceltrailingSlashconfig was also removed according to the PR.Please update the comment to reflect the current behaviour.
📝 Suggested comment update
/** - * Removes trailing slashes from URLs. + * Appends trailing slashes to URLs for consistency with trailingSlash: true in nuxt.config. * - * This middleware only runs in development to maintain consistent behavior. - * In production, Vercel handles this redirect via vercel.json. + * Ensures all page routes (except /package-code/) have a trailing slash. + * Skips payload requests on the server to avoid interfering with Nuxt data fetching. * - * - /package/vue/ → /package/vue - * - /docs/getting-started/?query=value → /docs/getting-started?query=value + * - /package/vue → /package/vue/ + * - /docs/getting-started?query=value → /docs/getting-started/?query=value */
🧹 Nitpick comments (3)
nuxt.config.ts (1)
356-374:getISRConfig'sfallbackbranch is now dead code.All four package-route call sites now use
getISRConfig(60)without theoptionsargument, so thefallbackbranch (lines 360–368) is unreachable. Consider removing theISRConfigOptionsinterface and thefallbacklogic to keep the helper lean, unless there are plans to reuse it elsewhere.♻️ Proposed simplification
-interface ISRConfigOptions { - fallback?: 'html' | 'json' -} -function getISRConfig(expirationSeconds: number, options: ISRConfigOptions = {}) { - if (options.fallback) { - return { - isr: { - expiration: expirationSeconds, - fallback: - options.fallback === 'html' ? 'spa.prerender-fallback.html' : 'payload-fallback.json', - initialHeaders: options.fallback === 'json' ? { 'content-type': 'application/json' } : {}, - } as { expiration: number }, - } - } - return { - isr: { - expiration: expirationSeconds, - }, - } +function getISRConfig(expirationSeconds: number) { + return { + isr: { + expiration: expirationSeconds, + }, + } }app/components/Link/Base.vue (1)
77-79::trailing-slashbinding is redundant whenv-bind="props"is already in use.
v-bind="props"spreads the full Vue props object ontoNuxtLink, which already includes thetrailingSlashprop. The explicit:trailing-slash="trailingSlash"binding on line 79 therefore sets the same prop twice to the same value. Vue 3 silently resolves this by letting the last binding win, but in development mode this can trigger a duplicate-prop warning.If the intention is to make the binding site explicit (consistent with how
:toand:targetare also explicitly re-bound), consider adding a comment. Otherwise the extra binding can be removed.app/components/VersionSelector.vue (1)
442-445:navigateToVersionbypasses the trailing-slash normalisation added toNuxtLinkKeyboard navigation (Enter/Space in
handleListboxKeydown) callsnavigateToVersion, which invokesnavigateTo(getVersionUrl(version))without a trailing slash. The middleware will redirect to the correct URL, but this introduces a superfluous client-side redirect for every keyboard-driven version switch.Consider appending the slash directly in
navigateToVersionso it is consistent with the link-click path:♻️ Suggested fix
function navigateToVersion(version: string) { isOpen.value = false - navigateTo(getVersionUrl(version)) + navigateTo(getVersionUrl(version).replace(/\/?$/, '/')) }
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/middleware/trailing-slash.global.ts (1)
1-9:⚠️ Potential issue | 🟡 MinorStale JSDoc: the comment describes the opposite of what the code now does.
The docstring says "Removes trailing slashes" and states that Vercel handles this in production, but the middleware now appends trailing slashes and the Vercel
trailingSlashsetting has been removed. The examples are also inverted (e.g./package/vue/→/package/vueis the old behaviour).📝 Suggested docstring update
-/** - * Removes trailing slashes from URLs. - * - * This middleware only runs in development to maintain consistent behavior. - * In production, Vercel handles this redirect via vercel.json. - * - * - /package/vue/ → /package/vue - * - /docs/getting-started/?query=value → /docs/getting-started?query=value - */ +/** + * Appends trailing slashes to URLs for consistent routing. + * + * - /package/vue → /package/vue/ + * - /docs/getting-started?query=value → /docs/getting-started/?query=value + * + * Paths under /package-code/ and /api/ are excluded. + */test/e2e/docs.spec.ts (1)
82-84:⚠️ Potential issue | 🟠 MajorURL assertion will likely fail after the trailing-slash change.
Line 84 asserts
toHaveURL(/\/package\/ufo$/), which requires the URL to end withufo(no trailing slash). After clicking the package link, the trailing-slash middleware will redirect to/package/ufo/, causing this assertion to fail. The regex should account for the trailing slash.🐛 Proposed fix
- // Should navigate to package page (URL ends with /ufo) - await expect(page).toHaveURL(/\/package\/ufo$/) + // Should navigate to package page + await expect(page).toHaveURL(/\/package\/ufo\/?$/)test/e2e/create-command.spec.ts (1)
14-15:⚠️ Potential issue | 🔴 CriticalUpdate the href selectors to include trailing slashes that the router appends.
Lines 15 and 31 assert
a[href="/package/create-vite"]anda[href="/package/create-next-app"]without trailing slashes. The NuxtLink component rendering these links (inTerminal/Install.vue) usestrailing-slash="append"explicitly, which means the generated href attributes will be/package/create-vite/and/package/create-next-app/with trailing slashes. The current selectors will silently fail to match, andtoBeAttached()will not find the elements.Change the selectors to:
a[href="/package/create-vite/"]a[href="/package/create-next-app/"]
🧹 Nitpick comments (1)
test/e2e/interactions.spec.ts (1)
76-77: Inconsistent:gotoon line 77 still uses/search?q=vuewithout a trailing slash.Line 54 was updated to
/search/?q=vuebut line 77 was not. The middleware will handle the redirect transparently so this won't break, but it's inconsistent with the rest of the file.📝 Suggested fix for consistency
- await goto('/search?q=vue', { waitUntil: 'hydration' }) + await goto('/search/?q=vue', { waitUntil: 'hydration' })
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/middleware/trailing-slash.global.tstest/e2e/connector.spec.tstest/e2e/create-command.spec.tstest/e2e/docs.spec.tstest/e2e/hydration.spec.tstest/e2e/interactions.spec.tstest/e2e/package-manager-select.spec.tstest/e2e/url-compatibility.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/docs.spec.ts (1)
84-84: Optional:$anchor may be fragile if query params are ever present.
/\/package\/ufo\/$/correctly asserts a trailing slash, but$anchors to the very end of the full URL string. If Nuxt ever appends a query parameter (e.g.?tab=readme) during navigation, this assertion will produce a false failure.♻️ More resilient alternative
- await expect(page).toHaveURL(/\/package\/ufo\/$/) + await expect(page).toHaveURL(/\/package\/ufo\/($|\?)/)This still asserts a trailing slash while tolerating any subsequent query string.
11b42c5 to
575fa89
Compare
|
I was so close to the cause, but I chose the wrong conclusion. It turns out the trailing slash only worked because ISR is strict about it, so pages with slashes were essentially ignored by nitro. And as a result, I mostly solved the problems that had been added because of this The problem is rather that our condition for isr was incomplete (again due to strictness) reopened #1604 |
🔗 Linked issue
Resolves #1558 #1530 #1534 #1267 #1266 #1194
🧭 Context
From the initial research:
📚 Description
Some prefetch requests now return an 404 error - this isn't a degradation; it's just that previously, in the event of a failure, a fallback was returned with an empty object that could be cached and processed. Now the behavior is more correct.
🤞🤞🤞