fix(app-router): feed file metadata routes into head output#891
fix(app-router): feed file metadata routes into head output#891NathanDrake2406 wants to merge 1 commit intocloudflare:mainfrom
Conversation
commit: |
There was a problem hiding this comment.
Pull request overview
This PR brings vinext’s App Router metadata routing closer to Next.js behavior by (1) preserving static metadata image extensions in served URLs, (2) injecting file-based metadata into the resolved metadata before MetadataHead renders, and (3) supporting generateImageMetadata()-style image ID suffixes in dynamic metadata route handlers.
Changes:
- Update metadata route discovery to preserve extensions for static icon/OpenGraph/Twitter/apple-icon routes and introduce placeholder URLs for static metadata files under dynamic segments.
- Add file-based metadata folding (
applyFileBasedMetadata) into resolved metadata, including cache-busting content hashes and carrying dimensions/content-type into head tags. - Extend request-time matching for dynamic metadata image routes to handle
generateImageMetadata()IDs and validate requested IDs.
Reviewed changes
Copilot reviewed 15 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/metadata-routes.test.ts | Updates expectations for extension-preserving served URLs and adds placeholder URL coverage. |
| tests/app-router.test.ts | Adds integration coverage for head injection, placeholder URLs, and generateImageMetadata() ID routing. |
| tests/entry-templates.test.ts | Updates template expectations to include routePrefix in metadata route objects. |
| tests/snapshots/entry-templates.test.ts.snap | Snapshot updates reflecting new metadata folding and request-time ID matching logic. |
| packages/vinext/src/server/metadata-routes.ts | Extends discovered metadata route shape and served URL generation (extensions + placeholders). |
| packages/vinext/src/server/file-based-metadata.ts | New helper to merge discovered file-based metadata into resolved App Router metadata for head rendering. |
| packages/vinext/src/server/app-page-boundary-render.ts | Ensures boundary/error/not-found rendering paths also apply file-based metadata before head rendering. |
| packages/vinext/src/shims/metadata.tsx | Adds type to image descriptors and renders additional OG/Twitter image meta tags (type/width/height). |
| packages/vinext/src/entries/app-rsc-entry.ts | Generates metadata head-data (hashes/dimensions), wires applyFileBasedMetadata, and matches generateImageMetadata() IDs. |
| tests/fixtures/** | Adds regression fixtures for static and multi-image metadata routing behaviors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 27 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/bigbonk review |
|
@NathanDrake2406 Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review |
1 similar comment
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
Posted one review comment on PR #891. Finding raised:
|
|
/bigbonk review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 31 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
NathanDrake2406
changed the title
|
/bigbonk review |
There was a problem hiding this comment.
Solid metadata-parity work. I cross-checked the file-based metadata logic against the Next.js source in packages/next/src/lib/metadata/resolve-metadata.ts and next-metadata-image-loader.ts — the precedence rules, the - placeholder for static files in dynamic segments, the slice(0, 6) djb2 hash for route-group/parallel-slot suffixes, and the SVG sizes: "any" handling all match Next.js behavior. The dismissed bonk finding about hadExplicitIcons does match Next.js's coarse !resolvedMetadata.icons gate (resolveIcons() returns a truthy { icon: [], apple: [], shortcut: [...] } even for partial icons objects), so leaving it alone is correct for parity.
A few smaller observations below that didn't rise to the level of blockers but might be worth a follow-up.
|
Review posted successfully. Summary of what I did: I reviewed PR #891, which folds App Router file-convention metadata into rendered head output. I cross-checked the implementation against Next.js source in Key verification: I confirmed that NathanDrake's dismissal of the bonk reviewer's Other items confirmed:
Posted observations (non-blocking):
|
|
/bigbonk review |
There was a problem hiding this comment.
Solid PR. I cross-checked the file-based metadata logic against the Next.js source (resolve-metadata.ts, resolve-icons.ts, next-metadata-image-loader.ts, next-metadata-route-loader.ts) and the precedence rules, the - placeholder substitution, the route-group/parallel-slot djb2 hash, the SVG sizes: "any" handling, and the favicon-vs-icons interaction all match Next.js behavior. Both prior bonk findings (hadExplicitIcons coarseness, normalizeIconEntries non-array handling, image-size dependency, SVG sizes) have been correctly dismissed or addressed.
A few smaller observations below — mostly non-blocking parity / robustness notes.
|
/bigbonk review |
There was a problem hiding this comment.
Review: file-based metadata head injection
Thorough review of the full PR. The architecture is well-designed — the separation into metadata-route-build-data.ts (entry-time), metadata-route-response.ts (runtime HTTP), file-based-metadata.ts (head injection), and app-page-head.ts (orchestration) follows the project guidelines for keeping generated entries thin. The precedence logic, segment-scoped social image filtering, djb2 route-group disambiguation, SVG sizes: "any", favicon-always-merges, and hadExplicitIcons coarse gate all match Next.js behavior.
Prior review findings across 38 commits have been comprehensively addressed. The catch-all + generateImageMetadata fix, favicon merge with non-IconMap icons, single-bracket multi-value rejection, fillMetadataRouteSegments null-return, runtime id-validation softening, routeKind centralization, handleGeneratedSitemap gating, index alignment fix via layoutSourcePositions, and early returns are all solid.
Findings
One correctness gap and several non-blocking observations. Nothing here is a merge blocker — the correctness item is a narrow edge case.
fillMetadataRouteSegmentsoptional catch-all with empty string (inline) — minor correctness gapcallDynamicMetadataRoutefalls through toJSON.stringifyfor image routes returning non-Response values (inline) — silent wrong behavior for a user errormetadata-route-response.test.tshas no coverage for dynamic image route serving (inline) — the core request dispatch path foricon.tsx/opengraph-image.tsxis untested at the response levelthenable-params.tsProxyownKeystrap doesn't include Promise's own keys (inline) — minor spec compliance note
| const value = params[paramName]; | ||
| if (value !== undefined) { | ||
| appendParamValue(resolvedSegments, value); | ||
| } |
There was a problem hiding this comment.
Minor correctness gap: Optional catch-all [[...slug]] with an empty string "" value will push an empty segment via appendParamValue, producing a URL with a double-slash (e.g., /docs//icon). The undefined case correctly skips (line 392), and the required catch-all [...slug] at line 401 correctly rejects empty strings, but the optional catch-all doesn't.
In practice this is unlikely — optional catch-all params are typically undefined (no segments) or string[] (matched segments). A bare "" is not a normal param value from routing. But for defensive correctness:
| } | |
| if (value !== undefined && value !== "") { | |
| if (Array.isArray(value) && value.length === 0) { | |
| continue; | |
| } | |
| appendParamValue(resolvedSegments, value); | |
| } |
There was a problem hiding this comment.
Valid. Fixed in 80647a1 by treating "" like the absent optional catch-all case, and by continuing for empty optional catch-all arrays. Added regression coverage so /docs/[[...slug]]/icon with { slug: "" } emits /docs/icon/..., not /docs//icon/....
| } | ||
| body = manifestToJson(result); | ||
| } else { | ||
| body = JSON.stringify(result); |
There was a problem hiding this comment.
Silent wrong behavior for image route user errors: When a dynamic image metadata route's default export returns a non-Response value (e.g., returns a React element from ImageResponse incorrectly, or returns a raw object), it falls through to JSON.stringify(result) and is served with the image content type. The browser gets JSON with a Content-Type: image/png header.
Next.js image metadata routes (ImageResponse) always return a Response, and the loader enforces this at build time. Since vinext doesn't have build-time validation here, a user mistake silently produces a broken response instead of a clear error.
Consider either:
- Throwing a
TypeErrorfor image route types (like sitemap/robots/manifest already do) - Or at minimum, logging a warning
| body = JSON.stringify(result); | |
| } else if (isImageMetadataRoute(route)) { | |
| throw new TypeError( | |
| `Dynamic metadata ${route.type} route ${route.servedUrl} must return a Response (e.g., from ImageResponse).`, | |
| ); | |
| } else { | |
| body = JSON.stringify(result); | |
| } |
There was a problem hiding this comment.
Valid. Fixed in 80647a1. Dynamic image metadata routes now throw a TypeError when the default export returns a non-Response, instead of serializing the value as JSON under an image content type. Added response-level coverage for the failure path.
| }), | ||
| ).rejects.toThrow("id property is required for every item returned from generateSitemaps"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test gap: metadata-route-response.test.ts has no test for the dynamic image route serving path — the most common metadata route type. The implementation has significant logic in callDynamicMetadataRoute (the hasGeneratedImageMetadata branch, generateImageMetadata + findGeneratedImageId, image id validation, 404 for missing/invalid ids) that is only exercised via the integration tests in app-router.test.ts.
Specifically untested at the unit level:
- Dynamic image route matching via
matchMetadataRoutewithpatternParts generateImageMetadataid matching at request dispatch time- 404 for invalid/unknown image ids at the
handleMetadataRouteRequestlevel - Interaction between
patternPartsmatching andgenerateImageMetadata
The integration tests provide coverage, so this isn't blocking, but adding a focused test for callDynamicMetadataRoute with a mock image module would catch regressions more precisely than the full-stack integration tests.
There was a problem hiding this comment.
Valid test gap. Added focused handleMetadataRouteRequest coverage in 80647a1 for dynamic generated image dispatch with patternParts, matched generateImageMetadata ids, unknown ids, invalid id path matching, and the non-Response image-route error path.
| has(target, prop) { | ||
| return Reflect.has(target, prop) || hasParamProperty(plain, prop); | ||
| }, | ||
| ownKeys() { |
There was a problem hiding this comment.
Spec compliance nit (non-blocking): The ownKeys trap filters out Promise method names from plain's keys, but it doesn't include any of the target (Promise) own keys. This means Object.getOwnPropertyNames(thenableParams) returns only the user's param keys, not the Promise's own properties. This is fine for the actual use case (spreading params, awaiting them), but technically a Proxy ownKeys trap must return a list that includes all non-configurable own properties of the target — Promise's length etc.
In practice this doesn't matter because nothing in the codebase calls Object.getOwnPropertyNames on thenable params, and this matches the old Object.assign(Promise.resolve(params), params) behavior. Just noting it.
There was a problem hiding this comment.
No code change here. I checked the runtime invariant: Object.getOwnPropertyNames(Promise.resolve()) and Reflect.ownKeys(Promise.resolve()) both return [], so the proxy is not omitting non-configurable own properties from the Promise target. The current behavior also preserves the previous Object.assign(Promise.resolve(params), params) observable shape for callers that enumerate params.
| ): boolean { | ||
| // Next suppresses file icon routes when any resolved icons metadata exists. | ||
| // Social image routes stay segment-scoped instead of using this merged fallback. | ||
| return hasIcons(fallbackMetadata) || hasIcons(getMetadataSourceForRoute(route, options, null)); |
There was a problem hiding this comment.
Good comment explaining the icon vs social image suppression asymmetry. Worth noting for future readers: the || hasIcons(getMetadataSourceForRoute(route, options, null)) check here (with null fallback, not fallbackMetadata) means the second check only matches when a metadata source at the route's specific segment level has icons. The first check hasIcons(fallbackMetadata) is the one that makes icons globally suppressive. The null third argument is critical — passing fallbackMetadata would double-count.
There was a problem hiding this comment.
No code change. This is intentional and the existing comment is there to preserve that distinction: icon suppression is globally based on resolved icons metadata, while social image suppression stays segment-scoped. Passing null as the fallback in the route-specific check is deliberate so it does not double-count the merged fallback metadata.
| } | ||
| } | ||
|
|
||
| function readMetadataRouteAltText(route: MetadataFileRoute): string | undefined { |
There was a problem hiding this comment.
Minor parity note (non-blocking): Alt text read from .alt.txt files is not trimmed. If the file has a trailing newline (which most editors add by default), that newline propagates into the alt="...\n" attribute in the rendered <meta> tag. Next.js also does not trim (confirmed in next-metadata-image-loader.ts — await fs.readFile(altPath, 'utf8') without .trim()), so this is parity. Just flagging it since the test fixture at tests/fixtures/app-basic/app/metadata-static/opengraph-image.alt.txt ends with a newline and the tests presumably pass with it.
There was a problem hiding this comment.
No code change. Confirmed this is parity with Next.js: .alt.txt content is read as UTF-8 and propagated without trimming, including trailing newlines. The fixture and head-output tests cover that behavior.
|
Review posted successfully. Here's a summary of what I found: Architecture: Well-structured separation into five new modules ( Verified correct (Next.js parity):
Posted findings (6 inline comments, all non-blocking):
|
NathanDrake2406
force-pushed
the
nathanfix-static-metadata-parity
branch
2 times, most recently
from
0ee5fbe to
22f0275
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Review: file-based metadata head injection
This is a substantial and well-architected PR. The layering is clean — metadata-route-build-data.ts for entry-time data collection, metadata-route-response.ts for runtime HTTP serving, file-based-metadata.ts for head injection with segment-scoped precedence, and app-page-head.ts for orchestration. The separation keeps generated entries thin per the project guidelines.
I verified the core parity behaviors against Next.js source: the hadExplicitIcons coarse gate, the - placeholder for dynamic segments, the djb2 route-group/parallel-slot suffix, SVG sizes: "any", favicon-always-merges, and segment-scoped social image precedence all match. The 41 commits of iterative review fixes are solid — catch-all + generateImageMetadata, favicon merge with non-IconMap values, fillMetadataRouteSegments null-return, runtime id-validation softening, routeKind centralization, handleGeneratedSitemap gating, and the layoutSourcePositions index alignment fix all look correct.
I found one remaining correctness concern that I believe warrants attention before merge, plus several non-blocking observations about test coverage and defensive hardening.
Findings summary
-
hasGeneratedImageMetadatare-readsgenerateImageMetadatafrom the module on everymatchMetadataRoutecall — This function callsreadFunction(route.module, "generateImageMetadata")which does aReflect.get+Reflect.applywrapper construction each time. For a route table with N metadata routes, every incoming request invokes this check for each route during the matching loop. The function result is stable (it depends only onroute.moduleandroute.isDynamic), but the per-request cost grows linearly with route count. Non-blocking for merge, but worth memoizing in a follow-up. -
resolveParallelRouteHeadresolves metadata sequentially — Inapp-page-head.ts, parallel route layouts are resolved withfor...of+await, unlike the main layout chain which usesPromise.all. For routes with multiple parallel slot layouts, this serializes metadata resolution unnecessarily. Non-blocking. -
Test coverage is strong for the core paths — The
file-based-metadata.test.tssuite is thorough. The inline comments below note a couple of specific gaps worth filling. -
The
normalizeIconHeadEntries/isIconsMapinteraction inMetadataHeadis now correct — After thecloneIconMapfix for non-IconMap icon values, the rendering path properly normalizes all icon forms. Verified. -
browserRouterStateHasCommittedcleanup — The current cleanup logic inside theif (browserRouterStateRef === stateRef)guard is correct. Previous reviews suggested moving the reset outside the guard; the author's dismissal reasoning is sound.
| route.isDynamic && | ||
| isImageMetadataRoute(route) && | ||
| Boolean(readFunction(route.module, "generateImageMetadata")) | ||
| ); |
There was a problem hiding this comment.
Non-blocking (perf): hasGeneratedImageMetadata calls readFunction(route.module, "generateImageMetadata") which creates a new wrapper function via Reflect.apply on every call. This function is called from matchMetadataRoute for every route on every metadata request, and again from callDynamicMetadataRoute for the matched route. Since the result depends only on route.module (which is stable per-route), consider caching the boolean on the route object at initialization time — e.g., set route._hasGeneratedImageMetadata once during startup.
The current code is correct; this is purely about avoiding redundant property reads in the hot path.
There was a problem hiding this comment.
Fixed in 9c8aaace by caching the resolved module export wrappers per metadata route with a WeakMap. The cache is conditional by route kind, so non-sitemap routes still do not inspect generateSitemaps.
| ); | ||
|
|
||
| for (const layoutModule of layoutModules) { | ||
| const layoutMetadata = await resolveModuleMetadata( |
There was a problem hiding this comment.
Non-blocking (perf): This loop resolves parallel route layout metadata sequentially with for...of + await. The main layout chain (line 183-208) also resolves sequentially but that's intentional — each layout's generateMetadata receives its parent's accumulated metadata.
Here in resolveParallelRouteHead, the same sequential pattern is used, which is correct because accumulatedMetadata chains through the layouts. However, the viewport resolution for each layout (line 262) is independent and could be pulled out into a parallel Promise.all alongside the metadata resolution. Minor optimization for routes with many parallel slot layouts.
There was a problem hiding this comment.
Fixed in 9c8aaace. Viewport resolution for parallel-route layouts/page now starts independently and is awaited after the metadata parent chain has been preserved.
| } | ||
|
|
||
| const generateImageMetadata = Reflect.get(route.module, "generateImageMetadata"); | ||
| if (typeof generateImageMetadata !== "function") { |
There was a problem hiding this comment.
Observation (correctness confirmed): When generateImageMetadata is not exported, this falls back to readDynamicImageMetadataSource(route.module) which reads static alt, contentType, size exports from the module. This is the correct fallback for dynamic image routes that use a simple default export without generateImageMetadata.
The test at file-based-metadata.test.ts covers this path (the "uses dynamic metadata module without generateImageMetadata" test). Good.
| return [readDynamicImageMetadataSource(route.module)]; | ||
| } | ||
|
|
||
| const result = await generateImageMetadata({ params: makeThenableParams(params) }); |
There was a problem hiding this comment.
Non-blocking (robustness): If generateImageMetadata throws (e.g., network error fetching data for image variants), this propagates as an unhandled rejection that will crash the page render. The error boundary path already handles this via fallbackOnFileMetadataError: true in app-page-head.ts, which is good.
But for the non-error-boundary path, a throwing generateImageMetadata will surface as a 500 with no metadata, rather than rendering the page with fallback metadata. This matches Next.js behavior (broken generateImageMetadata = broken page), so it's correct parity. Just confirming the intentionality for future readers.
| return params ? { params, imageId: null } : null; | ||
| } | ||
|
|
||
| if (routeHasGeneratedImageMetadata && cleanPathname.startsWith(`${route.servedUrl}/`)) { |
There was a problem hiding this comment.
Edge case (non-blocking): For non-pattern routes with generateImageMetadata, the image ID is extracted as everything after route.servedUrl + "/". Line 103 rejects IDs containing /, which prevents path traversal. Good.
However, there's no URL-decoding of imageSuffix before comparison. If a user's generateImageMetadata returns { id: 'hello world' }, the head injection produces href=".../icon/hello world" (unencoded), and the browser request arrives as .../icon/hello%20world. The imageSuffix here would be hello%20world, which won't match the hello world ID from generateImageMetadata.
In practice, valid metadata image IDs match /^[a-zA-Z0-9-_.]+$/ (no spaces), so this gap is not reachable for valid IDs. Just noting the theoretical asymmetry.
There was a problem hiding this comment.
I am leaving this unchanged because metadata image IDs are validated on both sides of this path with isValidMetadataImageId, which only allows [a-zA-Z0-9-_.]. A request suffix containing %20 fails that grammar before dispatch, and generated IDs containing spaces are skipped before they can be matched. So the encoded-space asymmetry is not reachable for valid generateImageMetadata IDs.
| "Dynamic metadata opengraph-image route /opengraph-image must return a Response.", | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test gap (non-blocking): This file has excellent coverage of the sitemap and generated-image dispatch paths, but no test for the basic static metadata route serving happy path — i.e., calling handleMetadataRouteRequest with a route that has fileDataBase64 and verifying the decoded binary response, content type, and cache headers.
The error cases (missing data, corrupt base64) are covered, and the happy path is exercised via app-router.test.ts integration tests. But a focused unit test here would make the serveStaticMetadataRoute code path more resilient to refactoring.
There was a problem hiding this comment.
Added in 9c8aaace: metadata-route-response.test.ts now has a focused static file serving happy-path test for decoded body bytes, content type, and cache-control.
| }; | ||
|
|
||
| function createMetadataContentHash(buffer: Buffer): string { | ||
| return createHash("sha1").update(buffer).digest("hex").slice(0, 16); |
There was a problem hiding this comment.
Observation (confirmed correct): SHA-1 truncated to 64 bits (16 hex chars) for cache-busting. Not used for security — only as a URL query token for CDN cache invalidation. The collision space is more than sufficient for the number of metadata files in any practical app.
| : hasTwitterImages(sourceMetadata); | ||
| } | ||
|
|
||
| function iconRouteHasExplicitIconsAtSource( |
There was a problem hiding this comment.
Design confirmation: The icon vs social image suppression asymmetry is intentional and well-commented. Icons use the merged fallback metadata (hasIcons(fallbackMetadata)) making them globally suppressive, while social images use segment-scoped source lookup. This matches Next.js's !resolvedMetadata.icons coarse gate in resolve-metadata.ts. The comment added here is helpful for future readers.
| metaType === "twitter-image" | ||
| ) { | ||
| const suffix = baseName.slice(metaType.length); | ||
| if (/^\d$/.test(suffix)) { |
There was a problem hiding this comment.
Minor parity question (non-blocking): The numeric suffix regex /^\d$/ only allows single-digit suffixes (0-9), so icon10.png would not match. I wasn't able to confirm whether Next.js supports multi-digit numeric suffixes for metadata image files. If it does, this should be /^\d+$/. Worth verifying against the Next.js test suite when convenient.
There was a problem hiding this comment.
I verified this against the local Next.js clone. Next currently treats numeric metadata variants as single digit: packages/next/src/lib/metadata/is-metadata-route.test.ts accepts /icon0/route and /opengraph-image1/route, while the source builds this from the metadata discovery variant range 0..9. Keeping ^\\d$ matches that behavior; icon10.png should not be treated as a metadata variant unless Next changes that contract.
|
Review posted successfully. Here's a summary of what I found: Architecture: Well-structured separation into five new modules following the project guidelines for keeping generated entries thin. The layering is clean: build-time data collection -> runtime HTTP serving -> head injection with segment-scoped precedence -> orchestration. Verified correct (Next.js parity):
Posted findings (9 inline comments, all non-blocking):
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: file-based metadata head injection (PR #891)
This is a thorough and well-structured piece of parity work. The architecture — metadata-route-build-data.ts for entry-time data, metadata-route-response.ts for runtime HTTP serving, file-based-metadata.ts for head injection, and app-page-head.ts for orchestration — follows the project guidelines for keeping generated entries thin. The delegation pattern via typed options bags is clean and consistent.
I cross-checked the core parity behaviors against Next.js source: the hadExplicitIcons coarse gate, the - placeholder for dynamic segments, the djb2 route-group/parallel-slot suffix, SVG sizes: "any", favicon-always-merges, and segment-scoped social image precedence all match. The iterative review fixes across 40+ commits (catch-all + generateImageMetadata, favicon merge with non-IconMap, fillMetadataRouteSegments null-return, runtime id-validation softening, routeKind centralization, handleGeneratedSitemap gating, layoutSourcePositions alignment) have been well-addressed.
Test coverage is strong — file-based-metadata.test.ts, metadata-route-build-data.test.ts, metadata-route-response.test.ts, and the integration additions to app-router.test.ts cover the main lifecycle from scanning to serving to head injection.
I found one correctness concern and a few non-blocking observations. None are merge blockers.
Summary of findings
mergeMetadatatitle template with parallel routes (non-blocking, pre-existing) — theisPageheuristic breaks when parallel route metadata is appendedresolveDynamicImageMetadataSourceserror propagation scope (non-blocking) — a single throwinggenerateImageMetadatarejects all six metadata kinds viaPromise.all- Base64 embedding of large static files (observation) — no size warning for large OG images
- Icon block re-normalization (observation) — double
cloneIconMapwhen both favicon and icon routes exist - Test gap:
thenable-params.test.tsis minimal (non-blocking)
| ...layoutMetadataResults.filter(isMetadata), | ||
| ...layoutMetadataResults.filter(isPresent), | ||
| ...(pageMetadata ? [pageMetadata] : []), | ||
| ...parallelMetadataResults.filter(isPresent), |
There was a problem hiding this comment.
Pre-existing design concern (non-blocking): When parallel route metadata is appended here, the metadataList becomes [...layouts, page, ...parallelLayouts, ...parallelPages]. The downstream mergeMetadata uses i === metadataList.length - 1 to detect the "page" entry (which skips template collection). This means the last parallel route entry is treated as "the page," not the actual page entry.
Consequence: if a parallel route layout exports title: { template: '%s | Dashboard' }, that template is silently dropped because its entry is "after the page." Conversely, if the actual page defines a title template, it gets treated as a layout template and applied to the parallel route's title.
In practice this is unlikely to cause visible issues (parallel route metadata tends to be additive for icons/OG, not title), and the same limitation likely exists in Next.js's flat metadata merge. But worth a comment explaining why the ordering is [layouts, page, parallels] and that title templates from parallel routes are intentionally not applied.
| openGraphHeadData, | ||
| twitterHeadData, | ||
| manifestHeadData, | ||
| ] = await Promise.all([ |
There was a problem hiding this comment.
Non-blocking (error isolation): If any single resolveHeadDataList rejects (e.g., generateImageMetadata() throws for the icon kind), this Promise.all rejects entirely, losing successfully resolved data from the other five kinds. The caller in app-page-head.ts catches this in the error-boundary path (fallbackOnFileMetadataError), but in the normal rendering path, a single broken generateImageMetadata for icons would also prevent OG/twitter/manifest metadata from rendering.
Consider wrapping each resolveHeadDataList call with individual error handling so that a failure in one kind degrades gracefully:
const safeResolve = (routes, params) =>
resolveHeadDataList(routes, params).catch((err) => {
console.warn('[vinext] Failed to resolve metadata head data:', err);
return [];
});Not a blocker — the current fail-fast behavior is consistent with Next.js.
There was a problem hiding this comment.
Leaving this fail-fast on purpose. In the normal render path, a throwing generateImageMetadata() should surface as the page/head failure instead of silently dropping one metadata kind while rendering the rest; that matches the Next parity behavior noted here and keeps the failure observable. The error-boundary path still uses fallbackOnFileMetadataError for not-found/access-fallback rendering.
| faviconEntries.push(iconEntry); | ||
| } | ||
| } | ||
| if (faviconEntries.length > 0) { |
There was a problem hiding this comment.
Observation (correctness verified): When both favicon routes AND icon routes exist, this block (lines 756-761) first modifies nextMetadata.icons by cloning and prepending favicon entries. Then the subsequent block at line 764 calls cloneIconMap(nextMetadata.icons) again, which re-normalizes the already-modified icons including the just-prepended favicons.
This is correct — the icon block at line 764 needs to see the updated state including favicons so it can prepend file-based icons ahead of both favicons and explicit icons. The double normalization doesn't produce duplicates because cloneIconMap normalizes to IconMap form and normalizeIconEntries reads from the icon field. Just noting it for future readers since the two adjacent blocks both calling cloneIconMap(nextMetadata.icons) looks like it could be a bug at first glance.
| } | ||
|
|
||
| try { | ||
| const binary = atob(route.fileDataBase64); |
There was a problem hiding this comment.
Non-blocking (observation): The atob + manual byte copy is the right choice for Cloudflare Workers portability. For completeness, on Node.js runtimes, Buffer.from(route.fileDataBase64, 'base64') would be more efficient (single allocation vs string intermediary + per-byte copy). But since Workers is the primary target and atob works on both, this is fine.
| } | ||
|
|
||
| const value = Reflect.get(target, prop, receiver); | ||
| return typeof value === "function" ? value.bind(target) : value; |
There was a problem hiding this comment.
Non-blocking (observation): The value.bind(target) ensures Promise methods like .then() get the correct this binding to the underlying Promise. Good.
One edge case worth noting: if a route parameter is literally named "then", "catch", or "finally", the isPromiseMethod check takes precedence and the param value is inaccessible via sync property access on the proxy. The user would need const p = await params; p.then to access it. This matches the old Object.assign(Promise.resolve(params), params) behavior (where Promise.prototype.then would shadow the param), so it's parity. Not actionable, just documenting.
| expect(awaited).toEqual(source); | ||
| expect(Reflect.get(awaited, "then")).toBe("then-param"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Non-blocking (test gap): This test file is minimal at 22 lines. Consider adding:
- Empty params:
makeThenableParams({})— verifyObject.keys()returns[]andawaitresolves to{} - Array-valued params (catch-all segments):
makeThenableParams({ slug: ['a', 'b'] })— verify both sync access and awaited access preserve the array Symbol.toPrimitive/Symbol.iteratorbehavior — ensure proxy doesn't break when used in template literals orfor...of
Not blocking, but the current test only covers the Promise-method-collision edge case.
There was a problem hiding this comment.
Added the low-risk coverage for empty params and catch-all array params. I did not add Symbol.toPrimitive / Symbol.iterator assertions because the helper does not define custom primitive or iterator semantics; it should behave like the underlying Promise for those, and adding tests there would lock in behavior that is not part of the params contract.
| dimensions: readStaticMetadataImageDimensions(route, buffer), | ||
| altText: readMetadataRouteAltText(route), | ||
| }), | ||
| fileDataBase64: buffer.toString("base64"), |
There was a problem hiding this comment.
Non-blocking (observation): Static metadata files are fully base64-embedded in the generated entry source via buffer.toString("base64"). For a typical favicon or small icon this is fine, but a large OG image (e.g., 2-5MB) would produce a ~3-7MB base64 string in the JS bundle, significantly increasing bundle size and cold-start time on Workers.
Worth considering a size warning at build time for files exceeding a threshold (e.g., 256KB), similar to Vite's asset inline limit warning. Not a blocker for this PR — the same issue exists in any system that embeds static assets, and the common case is small icons.
|
Review posted successfully. Here's a summary of what I found: Architecture: Well-structured separation into five new modules ( Verified correct (Next.js parity):
Posted findings (7 inline comments, all non-blocking):
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: file-based metadata head injection
This is a well-structured PR that brings vinext's file-convention metadata into parity with Next.js. I've read all 53 changed files and cross-referenced against the prior review history (53 reviews). The architecture is solid: build-time data collection in metadata-route-build-data.ts, runtime HTTP serving in metadata-route-response.ts, head injection with segment-scoped precedence in file-based-metadata.ts, and orchestration in app-page-head.ts. This follows the project guidelines for keeping generated entries thin.
The iterative review fixes have been thorough — catch-all + generateImageMetadata, favicon merge with non-IconMap values, fillMetadataRouteSegments null-return, runtime id-validation softening, routeKind centralization, handleGeneratedSitemap gating, and layoutSourcePositions alignment all look correct.
Findings
I found no new blocking issues. The prior reviews have been effective. I have a few non-blocking observations below.
-
Sitemap
idpassed as rawPromise.resolve()instead of thenable —metadata-route-response.ts:179and:257wrap theidinPromise.resolve()rather than usingmakeThenableParams-style thenable. This means user code doingprops.iddirectly (withoutawait) gets a Promise object, not the value. Forparamsthis is handled correctly viamakeThenableParams, butidgets the bare Promise. Next.js wrapsidthe same way as params (thenable), so sync access likeprops.id.toString()would work in Next.js but would return"[object Promise]"in vinext. Low risk since most usersawaitthe id, but a parity gap. -
resolveModuleMetadatacorrectly omitssearchParamsfor layouts — The change atmetadata.tsx:381-384to conditionally omitsearchParamsfrom the props object when it'sundefinedis a good parity improvement. Next.js layouts don't receivesearchParamsin theirgenerateMetadatacall. -
createLayoutInputsindex preservation is correct — The function atapp-page-head.ts:161-168preserves the originallayoutModulesarray index when looking uplayoutTreePositions[index], andlayoutSourcePositionsat line 299 maps from the filtered inputs. This means thecreateMetadataSourcesfunction at line 140 receives correctly aligned arrays. The fix ina4f3cf33addressed this properly. -
Test coverage is thorough for the core paths — The new test files (
file-based-metadata.test.ts,metadata-route-build-data.test.ts,metadata-route-response.test.ts) provide good unit-level coverage. The integration additions toapp-router.test.tsexercise the full dev-server pipeline.
Overall: ready to merge pending CI.
| } | ||
|
|
||
| const result = await functions.defaultExport({ | ||
| id: Promise.resolve(matchedId), |
There was a problem hiding this comment.
Non-blocking parity note: id is wrapped in Promise.resolve(matchedId) here and at line 257 for generateImageMetadata routes. This means user code doing props.id (sync access without await) gets a Promise object rather than the string value.
Next.js wraps id with the same thenable pattern it uses for params, so props.id.toString() returns the actual id string in Next.js but "[object Promise]" in vinext. The params path handles this correctly via makeThenableParams, but id doesn't get the same treatment.
Low risk since most users await or destructure with async, but worth unifying in a follow-up for full parity.
There was a problem hiding this comment.
Fixed in 50e7243. Generated sitemap ids and generateImageMetadata ids now use a small thenable value wrapper, so both await id and sync string conversion (id.toString() / String(id)) return the matched id.
| const normalizedTopLevelValue = normalizeIconDescriptor(icon); | ||
| if (normalizedTopLevelValue) { | ||
| return [normalizedTopLevelValue]; | ||
| } |
There was a problem hiding this comment.
Observation: resolveParallelRouteHead at app-page-head.ts:263 pushes the same routeSegments for every layout within a parallel route. If parallel route layouts are at different depths, the metadata source scoping would be incorrect. In practice, parallel routes typically have a flat layout hierarchy, so this is unlikely to cause issues. Worth a comment noting the assumption.
There was a problem hiding this comment.
Documented in 50e7243. The current route-tree input only provides the active parallel slot branch scope here, not per-layout source positions inside that slot, so the metadata sources intentionally use the branch routeSegments for both slot layouts and the slot page.
| openGraphHeadData, | ||
| twitterHeadData, | ||
| manifestHeadData, | ||
| ] = await Promise.all([ |
There was a problem hiding this comment.
Design confirmation: the Promise.all here means a single throwing generateImageMetadata rejects all six metadata kinds. This is intentional fail-fast behavior for the normal render path (the error boundary path catches via fallbackOnFileMetadataError). Matches Next.js parity where broken generateImageMetadata = broken page render.
The author's rationale in the review thread is sound — silently dropping one metadata kind while rendering the rest would hide user errors.
|
Review posted successfully. Here's a summary of what I found: Architecture: Well-structured separation into five new modules ( Verified correct (Next.js parity):
No new blocking issues found. The 53 prior reviews have been thorough. I posted 3 non-blocking inline observations:
|
NathanDrake2406
changed the title
|
/bigbonk review |
There was a problem hiding this comment.
Review: File-Based Metadata Parity (PR #891)
This is a thorough and well-architected piece of work. I reviewed all 53 changed files, cross-referenced the implementation against the extensive prior review history (53+ reviews), and checked key parity decisions against the Next.js source patterns described in the PR body.
Architecture
The separation into five new modules is clean and follows the project guidelines for keeping generated entries thin:
metadata-route-build-data.ts— entry-time data collection (hashes, dimensions, base64)metadata-route-response.ts— runtime HTTP request handling withWeakMap-cached function resolutionfile-based-metadata.ts— head injection with segment-scoped precedenceapp-page-head.ts— metadata/viewport orchestration with parallel route supportapp-prerender-static-params.ts— root param seeding for prerender
The generated RSC entry stays thin — metadata route serving delegates to handleMetadataRouteRequest, head resolution delegates to resolveAppPageHead, and boundary rendering passes metadataRoutes through to the same typed helpers. The thenable-params.ts Proxy-based approach is a strict improvement over the old Object.assign(Promise.resolve(params), params) pattern.
What I verified as correct
hadExplicitIconscoarse gate matches Next.js's!resolvedMetadata.iconsinresolve-metadata.ts-placeholder for static metadata under dynamic segments- DJB2 hash for route-group/parallel-slot suffixes
- SVG
sizes: "any"handling - Favicon-always-merges behavior (matching
postProcessMetadata) - Segment-scoped social image precedence vs globally-scoped icon suppression
- All iterative review fixes (catch-all +
generateImageMetadata, favicon merge with non-IconMap, single-bracket multi-value rejection,fillMetadataRouteSegmentsnull-return, runtime id-validation softening,routeKindcentralization,handleGeneratedSitemapgating,layoutSourcePositionsindex alignment) - Error boundary resilience via
fallbackOnFileMetadataError - The
f335e2dsecurity check (blockjavascript:URLs) is preserved - Image ID validation against
/^[a-zA-Z0-9-_.]+$/prevents path traversal cloneIconMapfix for non-IconMap icon valuesmatchMetadataRoutePatterncorrectly gates catch-all on:prefix
Test coverage assessment
Test quality is strong. Key strengths:
file-based-metadata.test.tstests real merge semantics (priority, inheritance, suppression)metadata-route-response.test.tsuses getter-spy pattern to verify lazy evaluation (no eagergenerateSitemapsreads on non-sitemap routes)metadata-routes.test.tshas anexpectSitemapToMatchNextparity guard comparing output against the actual Next.jsresolveSitemapfunctionapp-page-head.test.tsports the Next.jslayout-params.test.tswith attributionapp-page-boundary-render.test.tsverifies middleware header preservation across HTML, RSC, access fallback, and error boundary response paths
Findings
No blocking issues. The prior 53 reviews have been thorough and the author has addressed findings systematically. I have a few non-blocking observations inline, plus a summary here.
1. Duplicate DJB2 hash implementations (non-blocking, maintenance risk)
metadata-routes.ts has two DJB2 hash implementations: getMetadataRouteSuffix (line 396, takes a page pathname string) and metadataRouteSuffix (line 572, takes a segment array). Both compute the same hash but arrive at the input differently. If these drift (e.g., a trailing-slash difference, an encoding difference, or a new segment type), the URL produced at scan time would not match the URL expected at build-data time, causing silent metadata lookup failures. Consider unifying or adding a cross-assertion in tests.
2. selectDeepestRoutes eager computation (non-blocking, perf)
In file-based-metadata.ts:155-177, fillMetadataRouteSegments and normalizeRoutePrefixPattern are computed for every route before checking whether the routeSegments branch (which skips them) applies. The author noted a fix in 8af6048b but the current code at HEAD still appears to compute these eagerly. If the fix was in an intermediate commit that got revised, it may have been lost. Worth double-checking.
3. matchMetadataRoutePattern test coverage (non-blocking)
This function has only 3 unit tests despite being the runtime route matcher for all parameterized metadata routes. It would benefit from additional focused coverage for single-param segments, optional catch-all with zero segments, multiple params, and non-matching patterns.
4. layoutSourcePositions alignment fix is correct
The createLayoutInputs function at app-page-head.ts:155-172 correctly pairs each present layout with its original layoutTreePositions entry before filtering nulls, and layoutSourcePositions uses those preserved positions. This ensures createMetadataSources never misaligns metadata results with tree positions.
5. Icon vs social image suppression comment is well-placed
The comment at file-based-metadata.ts:508-509 explaining why icon suppression is global while social image suppression is segment-scoped is helpful and prevents future "fixes" that would break parity.
Overall: ready to merge pending CI.
| ): boolean { | ||
| // Next suppresses file icon routes when any resolved icons metadata exists. | ||
| // Social image routes stay segment-scoped instead of using this merged fallback. | ||
| return hasIcons(fallbackMetadata) || hasIcons(getMetadataSourceForRoute(route, options, null)); |
There was a problem hiding this comment.
Good comment. This asymmetry (global icon suppression via hasIcons(fallbackMetadata) vs segment-scoped social image suppression) is one of the more non-obvious parity decisions. Without it, someone would reasonably try to make icons segment-scoped too, which would diverge from Next.js.
| continue; | ||
| } | ||
|
|
||
| const routePrefix = route.routePrefix; |
There was a problem hiding this comment.
Non-blocking (perf): routePrefix is read here, then fillMetadataRouteSegments and normalizeRoutePrefixPattern are computed on lines 176-177. But when the routeSegments branch is taken (lines 156-173), all three values are unused. fillMetadataRouteSegments does string splitting, param resolution, and array joining per route — work that's wasted when segment matching succeeds.
The author noted this was fixed in 8af6048b, but the current code appears to still compute eagerly. If the fix was in an intermediate commit that got revised during the extensive review cycle, it may have been lost.
There was a problem hiding this comment.
Addressed in 94a49fb. The expensive fillMetadataRouteSegments / normalizeRoutePrefixPattern work was already after the routeSegments branch, but I moved the routePrefix read into that fallback branch as the remaining small cleanup.
| return generatedSitemapResponse; | ||
| } | ||
|
|
||
| continue; |
There was a problem hiding this comment.
Non-blocking (readability): The continue here means that when generateSitemaps exists, the base sitemap URL (/products/sitemap.xml) is never served — only the generated sub-URLs (/products/sitemap/0.xml). This is correct Next.js behavior, but the control flow is non-obvious since the continue skips matchMetadataRoute entirely for this route. A brief comment would help future readers.
There was a problem hiding this comment.
Addressed in 94a49fb with a comment on the continue: generated sitemap routes intentionally serve only generated child sitemap URLs when generateSitemaps() exists, so the base /sitemap.xml route must not fall through.
| `Dynamic metadata ${route.type} route ${route.servedUrl} must return a Response.`, | ||
| ); | ||
| } else { | ||
| body = JSON.stringify(result); |
There was a problem hiding this comment.
Good addition. This catches user mistakes where a dynamic image route returns a non-Response value (e.g., a React element instead of ImageResponse). Without this, the value would fall through to JSON.stringify and be served with an image content type. Since vinext doesn't have Next.js's build-time loader validation, this runtime check is the right place.
| } | ||
|
|
||
| export function makeThenableParams<T extends Record<string, unknown>>(obj: T) { | ||
| const plain = { ...obj }; |
There was a problem hiding this comment.
Non-blocking (minor): The Proxy has no set trap, so writes like params.foo = "bar" would silently write to the underlying Promise target without updating plain. This matches the old Object.assign(Promise.resolve(params), params) behavior (writes also went to the Promise), so it's parity — but if a user accidentally mutates params, the behavior would be confusing. Low priority; documenting for awareness.
| export async function callAppPrerenderStaticParams( | ||
| options: CallAppPrerenderStaticParamsOptions, | ||
| ): Promise<unknown> { | ||
| setRootParams(pickRootParams(options.params, options.rootParamNamesByPattern[options.pattern])); |
There was a problem hiding this comment.
Observation: setRootParams mutates a module-level variable rather than using AsyncLocalStorage. This is safe for the prerender endpoint (gated on VINEXT_PRERENDER=1, runs sequentially), but worth noting as an assumption — concurrent prerender calls would race on root params.
|
Review posted successfully. Here's a summary of what I did: Reviewed PR #891 — a 5,284-line addition implementing file-based App Router metadata parity with Next.js. I read all 53 changed files, cross-referenced against the 53+ prior review comments, and checked parity decisions against Next.js source patterns. Verified correct:
No blocking issues found. Posted 6 inline comments and 5 summary observations, all non-blocking:
Review URL: #891 (review) |
|
Reviewed the latest bonk feedback and resolved the upstream conflicts in 94a49fb. I also removed the duplicate DJB2 hash loop in metadata route suffixing, kept route-prefix fallback work out of the routeSegments branch, and documented the generated-sitemap base-URL fallthrough behavior. Focused tests and |
NathanDrake2406
changed the title
|
/bigbonk review |
There was a problem hiding this comment.
Review: File-Based Metadata Parity (PR #891)
I did a full read of all 55 changed files and cross-referenced against the extensive prior review history (53+ reviews). The architecture is well-designed and the iterative review process has been highly effective — the prior blocking findings have all been resolved correctly.
Architecture
The layering is clean and follows the project guidelines:
metadata-route-build-data.ts— entry-time data collection (hashes, dimensions, base64 embedding)metadata-route-response.ts— runtime HTTP request handling withWeakMap-cached function resolutionfile-based-metadata.ts— head injection with segment-scoped precedenceapp-page-head.ts— metadata/viewport orchestration with parallel route supportapp-prerender-static-params.ts— root param seedingthenable-params.ts— Proxy-based replacement for the oldObject.assign(Promise.resolve(params), params)pattern
The generated RSC entry stays thin — metadata route serving delegates to handleMetadataRouteRequest, head resolution delegates to resolveAppPageHead, and boundary rendering threads metadataRoutes to the same typed helpers. This is exactly what AGENTS.md asks for.
What I verified as correct
hadExplicitIconscoarse gate matches Next.js!resolvedMetadata.iconsinresolve-metadata.ts-placeholder for static metadata under dynamic segments- DJB2 hash for route-group/parallel-slot suffixes, matching Next.js's
stringHash - SVG
sizes: "any"handling - Favicon-always-merges behavior (matching
postProcessMetadata) - Segment-scoped social image precedence vs globally-scoped icon suppression (intentional asymmetry, well-commented)
- All prior review fixes: catch-all +
generateImageMetadata, favicon merge with non-IconMap values, single-bracket multi-value rejection,fillMetadataRouteSegmentsnull-return, runtime id-validation softening,routeKindcentralization,handleGeneratedSitemapgating,layoutSourcePositionsindex alignment - Error boundary resilience via
fallbackOnFileMetadataErrorwith try/catch inresolveAppPageHead cloneIconMapfix for non-IconMap icon values (string, URL, single descriptor)matchMetadataRoutePatterncorrectly gates catch-all on:prefixthenable-params.tsProxy implementation correctly satisfies Proxy invariants (getOwnPropertyDescriptorreturns configurable descriptors,ownKeysis consistent)- SHA-1 truncated to 64 bits is appropriate for cache-busting
makeThenableMetadataRouteIdgives bothawaitand sync string coercion
No blocking issues found
The prior 53+ reviews have been thorough and the author has addressed findings systematically. Below are non-blocking observations.
Non-blocking findings
-
metadata-route-build-data.test.tsis under-tested — 4 tests for 264 lines of source. Missing coverage for SVGsizes: "any", favicon types, non-image metadata types (robots/sitemap), error paths (readMetadataRouteFilefailure,readMetadataImageDimensionswith corrupt images), optional catch-all pattern parts, and.alt.txtfile absence. The integration tests inapp-router.test.tscover many of these paths end-to-end, but focused unit tests would catch regressions more precisely. -
Loose type guards in
metadata-route-response.ts—isRobotsConfigandisManifestConfig(lines 63-68) both just checkisObject(value), which accepts any non-null object including arrays. An array passed torobotsToText()would throw confusingly (accessing.userAgentonundefined). These type guards exist primarily for the generated code path where the return type is known, so this is unlikely to fire in practice, but tightening them would improve error messages for user mistakes. -
mergeMetadataisPageheuristic —isPage = i === metadataList.length - 1inmetadata.tsx:316assumes the last entry is always the page. When parallel route metadata is appended (viaapp-page-head.ts:356), the last entry becomes the last parallel route's metadata, not the primary page. This means the parallel route entry gets the "page" treatment (skips title template collection), while the actual page entry does not. This is pre-existing and unlikely to cause visible issues since parallel routes typically don't define title templates, but it's worth a comment. -
No integration test for twitter-image dynamic routes — All dynamic image route integration tests exercise opengraph-image or icon. Adding a twitter-image variant would confirm the social image suppression/injection logic works end-to-end for both OG and Twitter kinds.
-
resolveHeadDataListinner parallelization — Each kind is already parallelized viaPromise.allon line 726, but within a kind, routes are resolved sequentially. For the common case (1-2 routes per kind) this is negligible. Noted for apps with deeply nested metadata.
Overall: solid work. Ready to merge pending CI.
| } | ||
|
|
||
| function isRobotsConfig(value: unknown): value is RobotsConfig { | ||
| return isObject(value); |
There was a problem hiding this comment.
Non-blocking: isRobotsConfig and isManifestConfig both only check isObject(value), which accepts any non-null object including arrays. If a user's dynamic robots route accidentally returns an array, robotsToText() would throw a confusing error accessing .userAgent on undefined rather than a clear validation message.
These guards are primarily for generated code paths where the return type is known, so this is unlikely to fire in practice. But for defense-in-depth, tightening the guards (e.g., checking for expected keys like rules on robots, name on manifest) would improve error diagnostics.
Not a blocker — the TypeError thrown at line 289 already gates the result to objects.
There was a problem hiding this comment.
Fixed in 227a98c. The robots and manifest runtime guards now reject arrays before calling the serializers, so bad dynamic route returns hit the route contract error instead of serializer internals. I added focused tests for both robots and manifest array returns.
| // Keep the primary page before active parallel routes. This lets slot metadata | ||
| // contribute additive fields such as OG images, while avoiding treating slot | ||
| // layout title templates as ancestors of the primary page. | ||
| const metadataList = [ |
There was a problem hiding this comment.
Non-blocking observation: when parallel route metadata is appended here, mergeMetadata in metadata.tsx:316 uses i === metadataList.length - 1 to detect the "page" entry (which gets special title treatment). With parallel route entries appended after the page, the last parallel route entry gets treated as the page instead.
This means:
- If a parallel route layout exports
title: { template: '%s | Dashboard' }, it would be silently dropped (treated as page, not layout) - The actual page's title template could be treated as a layout template
This is pre-existing and unlikely to cause visible issues since parallel route metadata is typically additive (icons, OG images), not title-template-based. A comment noting the ordering assumption would help future readers.
There was a problem hiding this comment.
Assessed this one as valid but pre-existing and non-blocking. I am not changing mergeMetadata semantics in this metadata-file parity PR; the current PR does keep primary page metadata before active parallel route metadata and documents that ordering in app-page-head.ts so slot title templates are not treated as primary-page ancestors. A real fix should adjust the mergeMetadata input shape/page-marker contract separately rather than infer page-ness from array position.
There was a problem hiding this comment.
Updated in 4128f79. I went ahead and fixed this rather than leaving it as follow-up: metadata merging now has explicit entry metadata (isPage, contributesTitle) instead of inferring page-ness from array position. resolveAppPageHead marks the primary page as the page and lets active parallel route metadata contribute non-title fields only, so slot title templates/pages cannot alter the primary page title. Added both merge-helper and app-head behavior tests.
| `[vinext] Missing generated module import for dynamic metadata route ${imagePath}`, | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Non-blocking test gap: this file has only 4 tests for 264 lines of source. Worth adding focused unit tests for:
- SVG metadata routes (should produce
sizes: "any", not pixel dimensions) - Favicon type routes
- Non-image metadata types (robots, sitemap) where
readStaticMetadataImageDimensionsshould short-circuit .alt.txtfile present vs absent- Error paths:
readMetadataRouteFilewith missing file,readMetadataImageDimensionswith corrupt image
The integration tests in app-router.test.ts cover many of these end-to-end, but focused unit tests here would catch regressions from refactors more precisely.
There was a problem hiding this comment.
Addressed in 02b1510. Added focused behavior tests for metadata route build data covering SVG icon sizes: "any", favicon head data, static non-image routes skipping dimension reads, absent .alt.txt, missing file diagnostics, and corrupt image dimension diagnostics.
| openGraphHeadData, | ||
| twitterHeadData, | ||
| manifestHeadData, | ||
| ] = await Promise.all([ |
There was a problem hiding this comment.
Design confirmation: the Promise.all fail-fast behavior here is correct and intentional. A single throwing generateImageMetadata rejects all six kinds, surfacing the error to the page render. The error boundary path catches this via fallbackOnFileMetadataError: true in resolveAppPageHead. Matches Next.js behavior where broken generateImageMetadata = broken page.
| } | ||
|
|
||
| const value = Reflect.get(target, prop, receiver); | ||
| return typeof value === "function" ? value.bind(target) : value; |
There was a problem hiding this comment.
Good: value.bind(target) ensures Promise methods retain the correct this context. Without this, calling .then() on the proxy would fail because Promise methods require this to be a genuine Promise.
One thing to note: if a route has a dynamic segment literally named [then], [catch], or [finally], the Promise method takes precedence and the param value is only accessible via await params. This is the same behavior as the old Object.assign pattern and is an inherent trade-off of the thenable approach.
|
Review posted successfully. Here's a summary of what I did: Reviewed PR #891 — a 5,217-line addition implementing file-based App Router metadata parity with Next.js. I read all 55 changed files across the new server modules, modified entry points, shims, and test suites, then cross-referenced against the 53+ prior review comments. Verified correct (Next.js parity):
No blocking issues found. Posted 5 inline comments and 5 summary observations, all non-blocking:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: File-Based Metadata Head Injection
Thorough review of the full PR across all 55 changed files. The architecture is well-designed — the five new modules (metadata-route-build-data.ts, metadata-route-response.ts, file-based-metadata.ts, app-page-head.ts, app-prerender-static-params.ts) plus thenable-params.ts follow the project guidelines for keeping generated entries thin and delegating to typed runtime helpers.
I cross-checked the core parity behaviors against the prior review history (53+ reviews) and spot-checked the implementation against the Next.js source patterns described in the PR body. The hadExplicitIcons coarse gate, - placeholder for dynamic segments, DJB2 hash, SVG sizes: "any", favicon-always-merges, and segment-scoped social image precedence all match Next.js. All iterative review fixes look correct.
The mergeMetadataEntries refactor to use explicit isPage/contributesTitle flags (replacing the fragile array-position heuristic) is a genuine improvement — this is cleaner than the old i === metadataList.length - 1 approach and correctly prevents parallel route entries from affecting the primary page title.
No blocking issues found. Five non-blocking observations below, mostly around test coverage gaps and one subtle correctness note.
Summary of inline findings
selectDeepestRoutesURL-based fallback path (routeApplies+fillMetadataRouteSegments+normalizeRoutePrefixPattern) is completely untested — this is the Pages Router code pathnormalizeRoutePrefixPatterngenerates patterns thatrouteAppliescannot meaningfully match (string prefix check vs pattern syntax), making the second check in the three-way OR effectively dead codehandleGeneratedSitemapassumesservedUrlends with.xmlvia unconditionalslice(0, -4)resolveParallelRouteHeadpasses fullparamsto layoutgenerateMetadatainstead of segment-scoped params- The
routeFunctionCacheWeakMapassumes route object references change on HMR (true for the current architecture but worth documenting)
|
|
||
| const routePrefix = route.routePrefix; | ||
| const resolvedRoutePrefix = fillMetadataRouteSegments(routePrefix, params); | ||
| const normalizedRoutePrefix = normalizeRoutePrefixPattern(routePrefix); |
There was a problem hiding this comment.
Test gap (non-blocking): The entire URL-based matching path through selectDeepestRoutes (lines 175-196) — routeApplies, fillMetadataRouteSegments, normalizeRoutePrefixPattern — is exercised only when routeSegments is absent. All existing file-based-metadata.test.ts tests provide routeSegments in the options, meaning this code path has zero test coverage.
This is the fallback for cases where raw app-tree segments aren't available (e.g., Pages Router or missing segment data). Given that the segment-based path has thorough coverage, the untested fallback is the most significant coverage gap in the PR.
Also note that the normalizeRoutePrefixPattern check at line 180 generates :param-style patterns, but routeApplies does a literal startsWith comparison — a URL like /blog/hello will never startsWith /:slug/. This means check (2) in the three-way OR is effectively dead code; only checks (1) and (3) can match in practice. Not a bug (the param-filled check at (3) handles it), but the dead code could mislead maintainers.
| return null; | ||
| } | ||
|
|
||
| const sitemapPrefix = route.servedUrl.slice(0, -4); |
There was a problem hiding this comment.
Defensive gap (non-blocking): route.servedUrl.slice(0, -4) unconditionally assumes the URL ends with .xml. The subsequent cleanPathname.endsWith(".xml") check on line 178 prevents a bad match from firing, so it degrades to a no-match rather than a wrong response. Still, if a sitemap route is ever misconfigured without the .xml suffix, the sitemapPrefix would silently trim the last 4 chars of whatever is there.
An assertion or early-return guard would catch configuration errors faster:
| const sitemapPrefix = route.servedUrl.slice(0, -4); | |
| const sitemapPrefix = route.servedUrl.endsWith(".xml") | |
| ? route.servedUrl.slice(0, -4) | |
| : route.servedUrl; |
| void pageViewportPromise.catch(() => null); | ||
|
|
||
| for (const layoutModule of layoutModules) { | ||
| const layoutMetadata = await resolveModuleMetadata( |
There was a problem hiding this comment.
Parity gap (non-blocking): resolveModuleMetadata here receives the full params object, while the main layout chain at line 186-189 scopes params via resolveAppPageSegmentParams(routeSegments, treePosition, params). This means parallel-route layouts can observe params from segments they don't contain in their tree position.
The comment at line 268-269 documents this limitation. In practice parallel routes typically have shallow layout trees so the impact is minimal. Worth tracking as a follow-up if parallel route metadata becomes more commonly used.
| hasGeneratedImageMetadata: boolean; | ||
| }; | ||
|
|
||
| const routeFunctionCache = new WeakMap<MetadataRuntimeRoute, MetadataRouteFunctions>(); |
There was a problem hiding this comment.
Observation: The WeakMap cache keys on the route object reference. If HMR replaces the module but reuses the same route object (mutating route.module in place), stale function references would be served. In the current architecture, HMR replaces the entire route array (new refs), so old cache entries become eligible for GC. Worth a brief comment documenting this assumption for the next reader.
| if (headData.height !== undefined) { | ||
| socialEntry.height = headData.height; | ||
| } | ||
| if (headData.alt) { |
There was a problem hiding this comment.
Minor (observation): Falsy alt values (empty string "") are silently dropped here. An empty alt="" is semantically meaningful in HTML (it indicates a decorative image), whereas omitting alt entirely means the alt text is unknown. Next.js also doesn't trim or special-case this, so it's parity. Just noting the distinction for future reference if alt-handling is ever revisited.
|
Review posted successfully. Here's a summary of what I found: Reviewed PR #891 — a 5,381-line addition implementing file-based App Router metadata parity with Next.js. Read all 55 changed files, cross-referenced the extensive prior review history (53+ reviews), and spot-checked against Next.js source patterns. Verified correct (Next.js parity):
No blocking issues found. Posted 5 non-blocking inline observations:
|
NathanDrake2406
force-pushed
the
nathanfix-static-metadata-parity
branch
from
4128f79 to
02b1510
Compare
What this changes
Fixes #884 by folding App Router file-convention metadata into rendered head output and by serving the corresponding static and dynamic metadata assets with the route metadata needed by the head renderer.
This covers static icon/apple/Open Graph/Twitter/manifest files, static social image
.alt.txtfiles, content-hashed injected URLs, image dimensions and content types, placeholder URLs for static metadata under dynamic segments, numbered dynamic image route variants such asopengraph-image2.tsx, andgenerateImageMetadata()id routes.Follow-up review fixes in this branch also keep metadata route generation thin, add path-specific read failures for scanned static metadata, preserve URL-valued metadata without deep cloning, scope layout metadata params to the layout segment, include active and intercepted parallel route metadata in head resolution, preserve static
manifest.jsonURLs, avoid parsing image dimensions for non-image metadata files, aligngenerateSitemaps()id handling with Next, and seed root params around prerendergenerateStaticParams().Why
vinext could serve metadata files as routes, but those files were not consistently reflected in
metadata.icons,metadata.openGraph.images,metadata.twitter.images, ormetadata.manifestbeforeMetadataHeadrendered. That meant browsers and crawlers could miss file-convention metadata even when the asset URL itself existed.The failure mode was also too quiet for static metadata files: if a scanned file could not be read, bad state could travel into missing hashes or empty metadata responses. Static file read failures now fail during entry generation with the specific file path.
Approach
URLvalues such asmetadataBasesurvive file metadata applicationopengraph-image.alt.txtandtwitter-image.alt.txtcontent in rendered OG/Twitter image alt metadatamanifest.jsonURLs and serve static manifests withapplication/manifest+jsongenerateImageMetadata()id routes and validate generated ids before calling the selected image handlergenerateSitemaps()ids, matching NextNon-goals
metadataBaseorbasePathparity beyond the file-based metadata paths touched heregenerateImageMetadata()is not usedValidation
vp checkvp test run tests/app-page-head.test.ts tests/file-based-metadata.test.ts tests/metadata-routes.test.ts tests/metadata-route-response.test.ts tests/entry-templates.test.ts tests/app-page-boundary-render.test.tsvp test run tests/app-router.test.ts tests/metadata-route-response.test.tsNext.js sources referenced
packages/next/src/lib/metadata/resolve-metadata.tspackages/next/src/build/webpack/loaders/next-metadata-image-loader.tspackages/next/src/build/webpack/loaders/next-metadata-route-loader.tspackages/next/src/lib/metadata/get-metadata-route.tspackages/next/src/lib/metadata/is-metadata-route.tstest/e2e/app-dir/metadata/metadata.test.tstest/e2e/app-dir/metadata-dynamic-routes/index.test.tstest/e2e/app-dir/metadata-static-file/