fix(app-router): preserve streamed metadata placement parity#2572
fix(app-router): preserve streamed metadata placement parity#2572LubomirGeorgiev wants to merge 7 commits into
Conversation
Async generateMetadata was serialized via dangerouslySetInnerHTML into a hidden <div> in the body, which React cannot hoist. For JS-capable clients (browsers, Googlebot) the tags stayed in <body>, where Google ignores rel=canonical, hreflang and robots. Render the resolved metadata as real <title>/<meta>/<link> elements through MetadataHead so React 19 hoists them into <head>, matching Next.js. Remove the now-unused renderMetadataToHtml string serializer.
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: metadata hoisting fix
Solid, well-scoped change. I verified the core mechanism and it holds up:
Correctness — confirmed good
MetadataHeadonly ever emits<title>,<meta>, and non-stylesheet<link>elements. All three are hoisted by React 19 from anywhere in the tree, so replacing the opaquedangerouslySetInnerHTMLstring with real elements does move them into<head>. I read the full component (packages/vinext/src/shims/metadata.tsx:877-1389) and confirmed there are no<script>(e.g. JSON-LD) or<style>tags that React would not hoist and would leave stuck in the body.resolvedMetadatais already the merged root+page metadata, so there's exactly one<title>— no risk of React's title dedup conflicting with the head path (which only emitscharSet).- Server renders
<div hidden></div>(metadata hoisted out) and the client re-renders<div hidden><MetadataHead/></div>which also hoists — no hydration mismatch. - No dev/prod parity gap:
renderMetadataToHtmlnow has a single (deprecated) consumer and the body-metadata path is App-Router-only, shared between dev and prod via the RSC entry. Pages Router has no equivalent path.
Tests — ran tests/app-page-route-wiring.test.ts (44 pass) and the MetadataHead rendering block of tests/features.test.ts (39 pass). vp check clean on the changed source files. The switch to renderRouteDocument rendering a full <html>/<head>/<body> document is a genuine improvement — it actually exercises hoisting rather than asserting on the raw outlet string.
Two minor, non-blocking notes below.
| expect(html).toContain('rel="alternate"'); | ||
| expect(html).toContain('hreflang="en-US"'); | ||
| expect(html).toContain('href="https://example.com/products/en"'); | ||
| expect(html).toContain('href="https://example.com/products/en" hreflang="en-US"'); |
There was a problem hiding this comment.
This assertion change isn't required by this PR. renderMetadataToHtml's serializer is unchanged (only a @deprecated doc comment was added), and the old split assertions (toContain('hreflang="en-US"') + toContain('href="https://example.com/products/en"')) still pass. Folding them into a single adjacency check couples the test to the exact attribute order emitted by renderMetadataAttributes (rel, href, hrefLang, media, type, sizes), making it brittle to a harmless serializer reorder. It does match the existing pattern at line 3234, so this is a style nit rather than a defect — feel free to keep it, but it's unrelated churn for a metadata-hoisting PR.
| __html: renderMetadataToHtml(metadata, pathname, { trailingSlash }), | ||
| }} | ||
| /> | ||
| <div hidden> |
There was a problem hiding this comment.
The <div hidden> wrapper is now purely vestigial: its metadata children are all hoisted to <head>, leaving an empty <div hidden></div> in the body (as your own test at tests/app-page-route-wiring.test.ts asserts). That's harmless and keeps a stable outlet, so no change needed — but worth a one-line comment noting the wrapper is intentionally retained as the streaming outlet placeholder, since a future reader may be tempted to drop it. Related: as the PR body notes, once the metadataPlacement UA gate is removed this body branch collapses into the head path entirely.
|
Review posted successfully to PR #2572. SummaryI reviewed PR #2572 (metadata hoisting fix) thoroughly and posted a batch review with a COMMENT verdict (no blocking issues). Verdict: LGTM — well-scoped, correct change. Correctness verification I performed:
Two minor, non-blocking notes posted inline:
|
|
There's a regression in the streamed metadata nextjs e2e. Will look at fixing. |
james-elicx
changed the title
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: streamed metadata placement parity
I traced the full placement mechanism end-to-end and the change is sound and well-scoped. Notes below, plus one open item.
Mechanism is correct
The two placement modes are asymmetric by design, and the code gets both right:
metadataPlacement === "body"serializes metadata viarenderMetadataToHtmlinto a<div hidden dangerouslySetInnerHTML>(app-page-route-wiring.tsx:557-565). Because that markup is opaque innerHTML, React 19 does not hoist it — it stays in<body>. That is exactly what restores Next.js request-time streaming parity.metadataPlacement === "head"renders real<MetadataHead>elements (app-page-route-wiring.tsx:543-545), which React 19 hoists into<head>.
(For the record: the earlier bot review described this inverted — it claimed the fix replaces dangerouslySetInnerHTML with real elements to move metadata to head. It's the opposite: the body path uses the serializer to keep metadata in body. The conclusion still holds, but the mental model in that review was backwards.)
Threading is consistent and backward-compatible
placeGeneratedMetadataInBody = (!isPrerender || pprFallbackShell !== undefined) && serveStreamingMetadata(app-page-dispatch.ts:577-578) correctly forces head placement for full prerenders while preserving request-time placement for PPR fallback shells.- ISR/cache regeneration hardcodes
serveStreamingMetadata: false(app-page-dispatch.ts:734) — correct, since the stored artifact must have metadata resolved into<head>. - The
serveStreamingMetadata ?? shouldServeStreamingMetadata(...)fallback in the builder (app-page-element-builder.ts:366) is backward-compatible: dispatch always passes a boolean, other callers passundefinedand keep the old UA-based behavior. - Special-error paths correctly keep the raw UA-based
serveStreamingMetadata(notplaceGeneratedMetadataInBody), because there it drives HTTP semantics (redirect refresh-tag vs 307, notFound 200 vs error status inapp-page-execution.ts:333/453), not element placement. Good separation.
Verification
tests/app-page-route-wiring.test.ts(46) +tests/app-page-dispatch.test.ts(63) pass locally.tests/nextjs-compat/metadata.test.ts+metadata-suspense.test.ts(48) pass locally.
Open item (already flagged by @james-elicx)
The noted regression in the Next.js streamed-metadata e2e is the thing to confirm green in CI before merge — I can't reproduce the full Playwright/Next.js e2e suite quickly here, and it's outside the vitest coverage that passes. That's the gating check.
Two minor non-blocking notes inline.
| const start = html.indexOf(`<${tagName}>`); | ||
| const end = html.indexOf(`</${tagName}>`); | ||
| if (start === -1 || end === -1) { | ||
| throw new Error(`Rendered document is missing <${tagName}>`); |
There was a problem hiding this comment.
readDocumentSection matches bare <head>/<body> via indexOf("<head>"). That works because renderRouteDocument emits attribute-less tags, but it will silently break if the document ever renders <body class=...> or similar (the indexOf returns -1 and throws "missing "). Fine for this test as written — just noting the coupling in case the document wrapper grows attributes later.
| await waitForAppRouterHydration(page); | ||
|
|
||
| await expect(page).toHaveTitle("Dynamic Metadata Page"); | ||
| await expect(page.locator("head title")).toHaveCount(0); |
There was a problem hiding this comment.
Nice — asserting head title count is 0 and body title count is 1 after hydration is the right guard: it proves the client re-render of the opaque <div hidden dangerouslySetInnerHTML> doesn't leak metadata into <head>. This is the case the unit tests can't cover (they only render the server tree). Good addition.
|
Review posted successfully to PR #2572. SummaryI traced the streamed-metadata placement mechanism end-to-end and posted a COMMENT review (no blocking code issues from me). Verdict: correct and well-scoped. The change is sound. What I verified:
Gating open item: I explicitly called out the maintainer's own note about the Next.js streamed-metadata e2e regression as the check to confirm green in CI before merge — that's outside the vitest coverage I could run locally. Two minor inline notes: |
2 participants
Summary
Restore Next.js-compatible metadata placement across request-time streaming and static artifact rendering.
generateMetadataoutput in<body>.<head>.<head>.Refs #2007.
Root cause
The original change rendered streamed metadata as real React elements so React 19 hoisted it into
<head>on every request. That diverged from Next.js: for normal request-time streaming, delayed metadata intentionally remains in<body>. Reverting that change restored the primary metadata-streaming suite, but exposed a second distinction: complete static artifacts must place generated metadata in<head>.Vinext uses a manually serialized metadata outlet, so it cannot infer this distinction from React flush timing. This change threads an explicit placement decision from the App Router dispatcher into element construction while keeping UA-driven bot and metadata-error semantics separate.
Verification
metadata-streaming.test.ts: 19/19 passedvp checkpasses on all changed filesThe upstream PPR metadata-streaming suite is currently
describe.skip; this change preserves existing fallback-shell placement rather than guessing at unsupported metadata staticness classification.