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 18 commits intocloudflare:mainfrom
Conversation
File-based metadata routes were only served, not merged into page metadata, so Firefox and Safari never saw rel=icon, apple-touch-icon, open graph, twitter, or manifest tags for app metadata files. Static files under dynamic segments also kept raw [param] paths instead of Next's - placeholder, and generated image modules did not contribute sizes, type, or alt metadata.\n\nThe metadata scanner now preserves logical route prefixes, normalizes static dynamic-segment URLs to placeholder paths, and records per-route content hashes. The App Router entry and boundary renderer now resolve file-based metadata into MetadataHead for static files, dynamic image modules, manifest routes, and not-found fallbacks while preserving user-authored metadata precedence.\n\nUpdated targeted metadata route and App Router tests cover static files, dynamic image modules, dynamic-segment placeholders, and fallback rendering.
File based metadata could inject multiple icon URLs from generateImageMetadata, but the app entry only matched the base metadata route path. Requests for the generated /icon/<id> URLs fell through, so head output and the route handler disagreed once multi-image metadata was present. The route matcher now recognizes metadata image id suffixes, validates returned ids, and passes the selected id into the handler. The app-router regression tests cover head injection, successful image fetches, missing-id 404s, and the generated entry snapshot.
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.
| try { | ||
| const dimensions = imageSize(buffer); | ||
| if (route.type === "favicon" || route.type === "icon" || route.type === "apple-icon") { | ||
| if (dimensions.width && dimensions.height) { |
There was a problem hiding this comment.
For SVG metadata icons, image-size may return concrete dimensions (e.g. from viewBox/width/height), but Next.js expects sizes="any" for vector icons. Consider forcing sizes: "any" when route.contentType is image/svg+xml (or when the served URL ends with .svg) instead of using ${dimensions.width}x${dimensions.height}.
| try { | |
| const dimensions = imageSize(buffer); | |
| if (route.type === "favicon" || route.type === "icon" || route.type === "apple-icon") { | |
| if (dimensions.width && dimensions.height) { | |
| const isSvgRoute = | |
| route.contentType === "image/svg+xml" || route.servedUrl.toLowerCase().endsWith(".svg"); | |
| try { | |
| const dimensions = imageSize(buffer); | |
| if (route.type === "favicon" || route.type === "icon" || route.type === "apple-icon") { | |
| if (isSvgRoute) { | |
| properties.push(`sizes: ${JSON.stringify("any")}`); | |
| } else if (dimensions.width && dimensions.height) { |
There was a problem hiding this comment.
Already addressed in 26171159. app-rsc-entry.ts:103-104 computes isSvgRoute from route.contentType === "image/svg+xml" or .svg extension, then :112-113 forces sizes: "any" for SVG icons before falling back to imageSize dimensions.
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.
| if (typeof iconValue === "string" || iconValue instanceof URL) { | ||
| return [{ url: iconValue }]; | ||
| } | ||
| return [...iconValue]; |
There was a problem hiding this comment.
normalizeIconEntries() assumes icons.icon is iterable when it isn't a string/URL. At runtime, a single descriptor object (common in Next.js: { url, sizes, type }) would hit return [...iconValue] and throw TypeError: iconValue is not iterable, breaking head injection. Consider handling the non-array object case (wrap in an array when it has a url property) and/or guarding with Array.isArray() before spreading.
| return [...iconValue]; | |
| if (Array.isArray(iconValue)) { | |
| return [...iconValue]; | |
| } | |
| if ( | |
| typeof iconValue === "object" && | |
| "url" in iconValue && | |
| (typeof iconValue.url === "string" || iconValue.url instanceof URL) | |
| ) { | |
| return [iconValue]; | |
| } | |
| return []; |
There was a problem hiding this comment.
Dismissing — the non-array case is already handled. normalizeIconEntries() at file-based-metadata.ts:177-200 checks Array.isArray(iconValue) and only spreads in that branch; otherwise it falls through to normalizeIconValue(iconValue), which handles both string | URL and the single descriptor object via normalizeIconDescriptor. No TypeError.
|
/bigbonk review |
|
@james-elicx Bonk workflow was cancelled. 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.
Unreadable scanned metadata files could be carried into generated App Router entries. Static files became empty runtime responses, and dynamic files lost their content hash diagnostic path. The entry generator now reads metadata files through one helper that throws a path-specific vinext error before route objects are emitted. Entry-template tests cover both static metadata asset reads and dynamic metadata hash reads.
File metadata application now uses raw route segments and per-segment metadata sources, so inherited parent social images do not block leaf file images while same-segment explicit images still win. Static manifest files now override config manifests, static social image .alt.txt files are surfaced in rendered metadata, and URL metadata values are preserved by avoiding deep cloning. Focused tests cover route-group selection, manifest override, URL preservation, segment-scoped social image precedence, and rendered static alt tags.
NathanDrake2406
changed the title
The CI failure exposed a hydration race where __VINEXT_RSC_NAVIGATE__ could be called after hydrateRoot published the global function but before BrowserRoot's layout effect published browserRouterStateRef. In that window, client navigation fell into the generic error fallback and hard-navigated to the original redirect source instead of following the RSC response URL. RSC navigation now waits for the first committed browser router state before reading it. This keeps early external navigation calls deterministic without changing the later unmounted-root error behavior.
Programmatic router pushes need to enter the pending browser-router state synchronously once router state has committed. The previous CI fix waited before beginning that pending state, which let useTransition briefly report idle and broke the router-push pending E2E checks. Keep the initial router-state readiness wait for early non-programmatic callers, but start the pending transition immediately for committed programmatic navigations.
|
/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):
|
Simplify the static dynamic-segment placeholder check, document why raw app-tree route segments are authoritative for metadata selection, and extend the file-based Open Graph test to assert inherited fields survive file-image replacement.
|
/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.
|
Review posted successfully. SummaryI reviewed PR #891 (file-based App Router metadata parity work). I cross-checked the implementation against Next.js source ( Verified correct (parity with Next.js):
Posted observations (non-blocking):
Review URL: #891 (review) |
Share metadata route kind mapping across metadata head generation and file metadata application. Dynamic metadata route matches without a default export now warn and return 404 instead of falling through. Also document that servedUrl is query-free before content hashes are appended and simplify the file-icon assignment guard.
|
/bigbonk review |
There was a problem hiding this comment.
Cross-checked the file-based metadata logic against Next.js source (resolve-metadata.ts, next-metadata-image-loader.ts, next-metadata-route-loader.ts, resolve-icons.ts). The precedence rules, the - placeholder for static metadata in dynamic segments, the djb2 route-group/parallel-slot suffix, the favicon/explicit-icons interaction, and the SVG sizes: "any" handling all match Next.js. The dismissals on the prior bonk findings (hadExplicitIcons coarseness, normalizeIconEntries non-array fallthrough, image-size dependency) are correct.
Three small parity / robustness notes below, plus one observation worth flagging now: this PR is currently behind main, which has since landed f335e2d (block javascript: URLs in router push/replace/prefetch). On rebase, the action-redirect security check that was added to app-browser-entry.ts:registerServerActionCallback will need to come back — the diff currently shows that block being removed because it never existed at this PR's merge base. Worth a sanity check after rebase that x-action-redirect continues to gate dangerous schemes.
|
Review posted successfully. Summary: I reviewed PR #891 (file-based App Router metadata parity work) against Next.js source ( Posted findings (non-blocking):
Review URL: #891 (review) |
Merge upstream/main into the metadata parity branch, preserving the file-based metadata additions while taking main's app boundary middleware-header changes. Also validate generateImageMetadata ids, drop unfilled dynamic metadata URLs with a diagnostic, reuse getMetadataRouteKind in dynamic head-data creation, and document dynamic metadata content hash semantics.
|
/bigbonk review |
There was a problem hiding this comment.
Reviewed against Next.js source (get-metadata-route.ts, resolve-metadata.ts, next-metadata-image-loader.ts). The core parity work — precedence rules, the - placeholder for static metadata under dynamic segments, the djb2 route-group/parallel-slot suffix, favicon/icon merge interaction, the same-segment vs leaf-segment image precedence, SVG sizes: "any", and the metadataBase URL preservation via shallow clone — all check out. Prior bonk dismissals (hadExplicitIcons coarse gate, normalizeIconEntries non-array fallthrough, image-size dependency move) are correct.
A few remaining observations, ordered by risk. None look like merge blockers, but #1 is worth thinking about before this lands.
|
Review posted successfully. Summary: I cross-checked the file-based metadata work in PR #891 against Next.js source ( Posted findings (non-blocking):
Review URL: #891 (review) |
NathanDrake2406
changed the title
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 now 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, andgenerateImageMetadata()id routes.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.Approach
URLvalues such asmetadataBasesurvive file metadata applicationopengraph-image.alt.txtandtwitter-image.alt.txtcontent in rendered OG/Twitter image alt metadatagenerateImageMetadata()id routes and validate generated ids before calling the selected image handlerNon-goals
metadataBaseorbasePathparity beyond the file-based metadata paths touched heregenerateImageMetadata()is not usedValidation
vp test run tests/file-based-metadata.test.tsvp test run tests/metadata-routes.test.tsvp test run tests/entry-templates.test.tsvp test run tests/app-router.test.ts -t "metadata routes integration"vp check packages/vinext/src/entries/app-rsc-entry.ts packages/vinext/src/server/app-page-boundary-render.ts packages/vinext/src/server/file-based-metadata.ts packages/vinext/src/server/metadata-routes.ts tests/file-based-metadata.test.ts tests/app-router.test.ts tests/metadata-routes.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.ts