Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
…ersion fetch has succeeded
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughAdds runtime payload caching and hydration-aware handling for ISR-enabled routes. Introduces a server Nitro plugin that serves and caches serialized _payload.json responses, a server plugin that serializes SSR payloads onto the request event for later reuse, a client plugin to initialise payload data and trigger a refetch after suspense hydration, a new Nitro storage entry for payload-cache, changes to package page rendering and useResolvedVersion to expose status and handle SSR/SPA hydration states, adjusts ISR fallback payload shape, and adds devalue for payload serialization. Possibly related PRs
🚥 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 |
![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 (4)
app/pages/package/[[org]]/[name].vue (2)
256-272: Thedocument.querySelector('article')selector is fragile.Line 270 relies on finding the first
<article>element in the DOM. If the page layout changes (e.g., another<article>is added above the package content, or a parent component wraps content in<article>), this would capture the wrong HTML. Consider using a more specific selector, such as a data attribute or an ID.Suggested improvement
- ? (document.querySelector('article')?.innerHTML ?? null) + ? (document.getElementById('package-article')?.innerHTML ?? null)And add
id="package-article"to the<article>element on line 745:- <article v-else-if="status === 'success' && pkg" :class="$style.packagePage"> + <article v-else-if="status === 'success' && pkg" id="package-article" :class="$style.packagePage">
737-743: Rendering captured server HTML viav-html— acceptable here but worth a note.The
serverRenderedHtmloriginates from the server's own rendered DOM captured before hydration, so it's not a user-controlled XSS vector. However, a brief inline comment explaining this provenance would help future maintainers understand why raw HTML injection is safe in this specific case.server/plugins/payload-cache.ts (2)
120-130:response.body as string— unchecked type assertion.Line 124 asserts
response.bodyis astring. In Nitro's render pipeline, payload JSON responses should indeed be strings, but if a custom middleware or hook modifies the body to a different type (e.g.,Buffer,ReadableStream), this would silently store the wrong representation.A runtime guard would be more defensive:
Suggested guard
if (isPayloadRequest) { // This was a _payload.json render — cache the response body directly const routePath = getRouteFromPayloadUrl(ctx.event.path) + if (typeof response.body !== 'string') return cachePayload(ctx.event, routePath, { - body: response.body as string, + body: response.body, statusCode: response.statusCode ?? 200,
159-167: Dev log at line 171 fires before the async write completes.The
console.log('[payload-cache] CACHED: ...')on line 171 executes synchronously, before thestorage.setItempromise (insidewaitUntil) resolves. If the write fails, the log is misleading. Consider moving the success log inside the.then()or accepting the slight inaccuracy for dev-only logging.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
app/composables/npm/useResolvedVersion.tsapp/pages/package/[[org]]/[name].vueapp/plugins/fix.client.tsapp/plugins/payload-cache.server.tsmodules/cache.tsmodules/isr-fallback.tsnuxt.config.tspackage.jsonserver/plugins/payload-cache.ts
| // Don't cache if payload data is empty | ||
| const payloadData = ssrContext.payload?.data | ||
| if (!payloadData || Object.keys(payloadData).length === 0) return | ||
|
|
There was a problem hiding this comment.
Avoid caching placeholder payloads that are effectively empty.
Line 29 only checks top-level key count. Payload shapes like placeholder entries (for example, fallback-style empty records wrapped in arrays) can still pass and get cached as valid data, which risks stale empty payload reuse and hydration inconsistencies. Please harden this emptiness check before caching.
Suggested patch
// Don't cache if payload data is empty
const payloadData = ssrContext.payload?.data
- if (!payloadData || Object.keys(payloadData).length === 0) return
+ const isPlaceholderValue = (value: unknown) =>
+ Array.isArray(value)
+ && value.length === 1
+ && typeof value[0] === 'object'
+ && value[0] !== null
+ && Object.keys(value[0] as Record<string, unknown>).length === 0
+
+ const isEmptyPayload =
+ !payloadData
+ || Object.keys(payloadData).length === 0
+ || Object.values(payloadData).every((value) => value == null || isPlaceholderValue(value))
+
+ if (isEmptyPayload) return
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
app/pages/package/[[org]]/[name].vuepackage.json
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
| watch( | ||
| [resolvedStatus, resolvedVersion], | ||
| ([status, version]) => { | ||
| if ((!version && status === 'success') || status === 'error') { | ||
| showError({ | ||
| statusCode: 404, | ||
| statusMessage: $t('package.not_found'), | ||
| message: $t('package.not_found_message'), | ||
| }) | ||
| } | ||
| }, | ||
| { immediate: true }, | ||
| ) |
There was a problem hiding this comment.
watch with immediate: true may call showError during SSR.
On the server, the throw createError guard at lines 218–228 only fires when resolvedStatus is already 'success' or 'error'. If resolvedStatus is still 'pending' at that point, the guard is skipped and the watch is registered. With immediate: true, it fires synchronously during setup — on the server context as well as the client. If, by the time the immediate callback runs, resolvedStatus is already 'error' (e.g., if useResolvedVersion resolves synchronously to an error on the server), showError is called from the server context instead of throw createError, which produces a different error-response shape (soft error page vs. HTTP 404).
Consider guarding the watch so it only runs on the client:
🛡️ Proposed fix
-watch(
- [resolvedStatus, resolvedVersion],
- ([status, version]) => {
- if ((!version && status === 'success') || status === 'error') {
- showError({
- statusCode: 404,
- statusMessage: $t('package.not_found'),
- message: $t('package.not_found_message'),
- })
- }
- },
- { immediate: true },
-)
+if (import.meta.client) {
+ watch(
+ [resolvedStatus, resolvedVersion],
+ ([status, version]) => {
+ if ((!version && status === 'success') || status === 'error') {
+ showError({
+ statusCode: 404,
+ statusMessage: $t('package.not_found'),
+ message: $t('package.not_found_message'),
+ })
+ }
+ },
+ { immediate: true },
+ )
+}📝 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.
| watch( | |
| [resolvedStatus, resolvedVersion], | |
| ([status, version]) => { | |
| if ((!version && status === 'success') || status === 'error') { | |
| showError({ | |
| statusCode: 404, | |
| statusMessage: $t('package.not_found'), | |
| message: $t('package.not_found_message'), | |
| }) | |
| } | |
| }, | |
| { immediate: true }, | |
| ) | |
| if (import.meta.client) { | |
| watch( | |
| [resolvedStatus, resolvedVersion], | |
| ([status, version]) => { | |
| if ((!version && status === 'success') || status === 'error') { | |
| showError({ | |
| statusCode: 404, | |
| statusMessage: $t('package.not_found'), | |
| message: $t('package.not_found_message'), | |
| }) | |
| } | |
| }, | |
| { immediate: true }, | |
| ) | |
| } |
| <article | ||
| v-else-if="isHydratingWithServerContent && serverRenderedHtml" | ||
| :class="$style.packagePage" | ||
| v-html="serverRenderedHtml" | ||
| /> |
There was a problem hiding this comment.
v-html binding missing the eslint-disable comment used elsewhere in the file.
Line 1374 uses <!-- eslint-disable vue/no-v-html -- HTML is sanitized server-side --> before the v-html binding on the README, establishing a project convention for documenting why raw HTML is trusted. The new v-html on the hydration snapshot article at line 746 doesn't follow this pattern. Even though the source is our own server-rendered markup, the missing annotation leaves the intent undocumented and will produce a lint warning.
💅 Proposed fix
+ <!-- eslint-disable-next-line vue/no-v-html -- snapshot of server-rendered HTML, not user-supplied content -->
<article
v-else-if="isHydratingWithServerContent && serverRenderedHtml"
:class="$style.packagePage"
v-html="serverRenderedHtml"
/>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/pages/package/[[org]]/[name].vue (1)
218-227: Extract the repeated 404 payload into one helper.The not-found payload is duplicated in two places, which makes future edits easy to drift.
♻️ Suggested refactor
+function packageNotFoundError() { + return { + statusCode: 404, + statusMessage: $t('package.not_found'), + message: $t('package.not_found_message'), + } +} + if ( import.meta.server && !resolvedVersion.value && ['success', 'error'].includes(resolvedStatus.value) ) { - throw createError({ - statusCode: 404, - statusMessage: $t('package.not_found'), - message: $t('package.not_found_message'), - }) + throw createError(packageNotFoundError()) } watch( [resolvedStatus, resolvedVersion], ([status, version]) => { if ((!version && status === 'success') || status === 'error') { - showError({ - statusCode: 404, - statusMessage: $t('package.not_found'), - message: $t('package.not_found_message'), - }) + showError(packageNotFoundError()) } }, { immediate: true }, )Also applies to: 233-238
| const isSpaFallback = shallowRef(hasEmptyPayload && !nuxtApp.payload.path) | ||
| const isHydratingWithServerContent = shallowRef( | ||
| hasEmptyPayload && nuxtApp.payload.path === route.path, | ||
| ) | ||
| // When we have server-rendered content but no payload data, capture the server | ||
| // DOM before Vue's hydration replaces it. This lets us show the server-rendered | ||
| // HTML as a static snapshot while data refetches, avoiding any visual flash. | ||
| const serverRenderedHtml = shallowRef<string | null>( | ||
| isHydratingWithServerContent.value | ||
| ? (document.getElementById('package-article')?.innerHTML ?? null) | ||
| : null, | ||
| ) |
There was a problem hiding this comment.
Add a hard fallback when snapshot capture is unavailable.
Line 737 suppresses the skeleton while isHydratingWithServerContent is true, but Line 746 also requires serverRenderedHtml. If Line 274 cannot find #package-article, the page can render a blank main area until async data resolves.
💡 Suggested fix
const serverRenderedHtml = shallowRef<string | null>(
isHydratingWithServerContent.value
? (document.getElementById('package-article')?.innerHTML ?? null)
: null,
)
+
+if (isHydratingWithServerContent.value && !serverRenderedHtml.value) {
+ isHydratingWithServerContent.value = false
+ isSpaFallback.value = true
+}Also applies to: 736-750
| const age = (Date.now() - cached.cachedAt) / 1000 | ||
| if (age > PAYLOAD_CACHE_STALE_TTL) return | ||
|
|
There was a problem hiding this comment.
Evict expired payload entries to avoid unbounded cache growth.
Line 87 skips stale entries but never deletes them. Over time this can leave old keys accumulating in storage indefinitely.
🧹 Suggested fix
const age = (Date.now() - cached.cachedAt) / 1000
- if (age > PAYLOAD_CACHE_STALE_TTL) return
+ if (age > PAYLOAD_CACHE_STALE_TTL) {
+ ctx.event.waitUntil(storage.removeItem(cacheKey).catch(() => {}))
+ return
+ }Also applies to: 149-169
| if (isPayloadRequest) { | ||
| // This was a _payload.json render — cache the response body directly | ||
| if (typeof response.body !== 'string') return | ||
| const routePath = getRouteFromPayloadUrl(ctx.event.path) | ||
| cachePayload(ctx.event, routePath, { | ||
| body: response.body, | ||
| statusCode: response.statusCode ?? 200, | ||
| headers: { | ||
| 'content-type': 'application/json;charset=utf-8', | ||
| 'x-powered-by': 'Nuxt', | ||
| }, | ||
| }) | ||
| } else if (isHtmlResponse && isISRRoute(ctx.event)) { |
There was a problem hiding this comment.
Apply the same cacheability gate to payload-response writes.
The _payload.json branch caches every successful payload response, but the HTML branch only caches when isISRRoute(ctx.event) is true. This inconsistency can cache routes that were not intended to be cache-backed.
🔒 Suggested fix
if (isPayloadRequest) {
+ if (!isISRRoute(ctx.event)) return
// This was a _payload.json render — cache the response body directly
if (typeof response.body !== 'string') return
const routePath = getRouteFromPayloadUrl(ctx.event.path)
cachePayload(ctx.event, routePath, {📝 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.
| if (isPayloadRequest) { | |
| // This was a _payload.json render — cache the response body directly | |
| if (typeof response.body !== 'string') return | |
| const routePath = getRouteFromPayloadUrl(ctx.event.path) | |
| cachePayload(ctx.event, routePath, { | |
| body: response.body, | |
| statusCode: response.statusCode ?? 200, | |
| headers: { | |
| 'content-type': 'application/json;charset=utf-8', | |
| 'x-powered-by': 'Nuxt', | |
| }, | |
| }) | |
| } else if (isHtmlResponse && isISRRoute(ctx.event)) { | |
| if (isPayloadRequest) { | |
| if (!isISRRoute(ctx.event)) return | |
| // This was a _payload.json render — cache the response body directly | |
| if (typeof response.body !== 'string') return | |
| const routePath = getRouteFromPayloadUrl(ctx.event.path) | |
| cachePayload(ctx.event, routePath, { | |
| body: response.body, | |
| statusCode: response.statusCode ?? 200, | |
| headers: { | |
| 'content-type': 'application/json;charset=utf-8', | |
| 'x-powered-by': 'Nuxt', | |
| }, | |
| }) | |
| } else if (isHtmlResponse && isISRRoute(ctx.event)) { |
2 participants
🔗 Linked issue
resolves #1194
resolves #1534
resolves #1558
🧭 Context
📚 Description
this aims to implement a payload cache so we don't have to render 2x - once for HTML and once for JSON.