Repository navigation
fix(app-router): classify static metadata after rendering - #2330
james-elicx wants to merge 12 commits into
Conversation
|
/bigbonk review for issues |
commit: |
|
Performance benchmarksCompared 0 improved · 3 regressed · 3 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: classify static metadata after rendering
Reviewed the metadata placement deferral, the new trackStreamCompletion/lifecycle plumbing, the head-if-static wire path, and the SSR/browser placement. The core approach is sound and well-tested (ISR head, body-dynamic, parallel/default slots, navigation). A few concerns, one of which I think is a real regression.
1. Request context is cleared too early on the late special-error path (likely regression)
packages/vinext/src/server/app-page-render.ts ~L1020:
if (specialError) {
void htmlStream.cancel().catch(() => {});
finalizeRenderLifecycle(specialError); // <- calls options.clearRequestContext()
return options.renderPageSpecialError(specialError);
}finalizeRenderLifecycle() calls options.clearRequestContext() (which nulls the headers + navigation contexts in app-request-context.ts). It now runs before renderPageSpecialError(specialError).
But renderPageSpecialError → buildAppPageSpecialErrorResponse (app-page-execution.ts) assumes the request context is still live: it calls getAndClearPendingCookies() and renderFallbackPage() → renderHttpAccessFallbackPage(), which performs a full RSC+SSR re-render of the not-found/forbidden/unauthorized boundary (with layouts) that can call headers()/cookies(). It then clears the context itself at the end (L333/L459).
On main this path returned options.renderPageSpecialError(specialError) without clearing context first. This PR adds the early clear, so a notFound()/forbidden() thrown after the shell (the hasLoadingBoundary || !probePageBeforeRender branch) will render its boundary with a null request context. This is exactly the class of bug that issue #660 warns about.
The same early clear happens at the htmlRender.response alternate-response path (~L983); that one is lower-risk since the shell-recovery response is already materialized, but it's worth confirming it doesn't read context lazily.
Suggested fix: don't clear the request context on paths that delegate to renderPageSpecialError (let that helper own the clear, as it does today). Only abortRscRender is needed there, not clearRequestContext. Consider splitting finalizeRenderLifecycle into "abort the RSC render + detach listeners" vs "clear request context" so the special-error path can abort without prematurely tearing down context.
Please add a test that throws notFound() (or forbidden()) late — after the shell, with a loading boundary — from a route whose boundary reads cookies()/headers(), and assert the boundary still sees the context. The existing suite passes but I don't see coverage for context-reading inside a late special-error boundary.
2. trackStreamCompletion never resolves on cancel
packages/vinext/src/server/defer-until-stream-consumed.ts:
cancel(reason) {
return reader.cancel(reason); // complete() is never called
},If the wrapped flight stream is cancelled before being fully read (client disconnect, or both tee branches cancelled), onComplete (resolveFlightCompletion) never fires, so pendingMetadataPlacement stays unresolved forever. In practice the only awaiter is PendingAppMetadata inside an SSR render that is itself being torn down, so this is a dangling-promise rather than a hard hang today — but it's asymmetric with the sibling deferUntilStreamConsumed, which you deliberately taught to fire its onAbort on cancel/error in this same PR. Recommend calling complete() (or resolving placement to a safe default) in cancel for consistency and to avoid a latent hang if a future caller awaits this promise outside the SSR scope.
3. Double metadata render across SSR and BrowserRoot (please confirm hydration parity)
On initial load, metadata is rendered by PendingAppMetadata (SSR, app-ssr-entry.ts) and also by BrowserRoot (app-browser-entry.ts L1050) when pendingMetadata is present in the initial tree state. These are two different component trees emitting the same hoistable <title>/<meta> tags, relying on React 19 metadata hoisting to dedupe. This generally works, but the markup must match exactly (same pathname/trailingSlash, same tag order) to avoid a hydration mismatch. The browser spec should be covering this; please confirm there are no console hydration warnings on the static/ISR routes (not just navigation). If BrowserRoot only needs to render this on client-side navigation, gating it on a non-initial render would remove the duplication risk entirely.
Minor
app-page-dispatch.ts~L896: the nested ternary formetadataPlacement(VINEXT_PRERENDER === "1" || shouldRenderSharedArtifact ? (VINEXT_PRERENDER === "1" ? "head" : "head-if-static") : undefined) is correct but hard to read and evaluatesprocess.env.VINEXT_PRERENDERtwice. A smallif/elseor a helper would be clearer.app-page-route-wiring.tsxcreateAppPageRouteBodyMetadata: the refactor toconst metadataHtml = (...); return metadataHtml;is a no-op vs returning the JSX directly. Fine, just noise in the diff.
What looks good
- The
head-if-staticwire path (app-elements-wire.ts) withreadPendingMetadatavalidation, and theslot.tsxguard so transported pending-metadata isn't rendered as a slot element — both well done and tested. - Backpressure test (
does not drain the Flight stream ahead of a slow HTML consumer) and the abort-on-cancel / abort-on-request-abort tests are exactly the right coverage for the new lifecycle. - ISR MISS/HIT/STALE/regen head-metadata test and the parallel/default-slot dynamic body cases match the Next.js v16.2.6 behavior and are linked to the upstream tests. Good parity discipline.
Overall: addressing #1 (early context clear on the special-error path) is the blocker; #2 and #3 are correctness-hardening I'd like confirmed before merge.
|
Review posted to PR #2330. Summary of findings: Review summaryI reviewed the metadata-placement deferral, the new stream/lifecycle plumbing, and the 1. Request context cleared too early on the late special-error path (blocker) — 2. 3. Double metadata render (SSR Plus two minor nits (nested ternary readability in dispatch; a no-op refactor in I verified the targeted unit/integration tests pass ( |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: classify static metadata after rendering (re-review @ f23179e)
I re-reviewed the full diff against main and traced the new metadata-deferral + lifecycle plumbing end to end. The approach is sound and the test coverage (ISR head, body-dynamic, parallel/default slots, backpressure, abort paths, Next.js v16.2.6 parity) is strong. The latest commit (f23179e, after the previous review) only reverts PageInvoker to sync and makes APP_PENDING_METADATA_KEY module-private — it does not address the three concerns from the prior review, all of which still stand on the current HEAD.
1. Request context cleared too early on the late special-error path (blocker — still present)
app-page-render.ts:1014-1024:
if (options.hasLoadingBoundary || !probePageBeforeRender) {
const captured = rscErrorTracker.getCapturedSpecialError();
if (captured) {
const specialError = resolveAppPageSpecialError(captured);
if (specialError) {
void htmlStream.cancel().catch(() => {});
finalizeRenderLifecycle(specialError); // <- clears request context
return options.renderPageSpecialError(specialError);
}
}
}finalizeRenderLifecycle() (L711-720) calls options.clearRequestContext(), which is clearAppRequestContext() → setHeadersContext(null) + setAppNavigationContext(null) (app-request-context.ts:27-30). This PR adds that clear before renderPageSpecialError runs; on main the path returned renderPageSpecialError(specialError) with the context still live.
renderPageSpecialError → buildAppPageSpecialErrorResponse (app-page-execution.ts:329) reads pending cookies (getAndClearPendingCookies(), L312/396/419) and, for notFound()/forbidden()/unauthorized(), calls renderFallbackPage() → renderHttpAccessFallback (app-fallback-renderer.ts:201), which does a full RSC+SSR re-render of the boundary. That re-render path (app-page-boundary-render.ts:296-320) reconstructs a navigation-context fallback but does not re-set the headers context — so a not-found.tsx/forbidden.tsx boundary that calls cookies()/headers() will see a null headers context. This is the issue #660 class of bug.
This path is reachable in production: it fires for routes with a loading.tsx (hasLoadingBoundary) or page-probe-skipped routes that throw notFound()/forbidden() late, after the shell.
I confirmed the regression with a focused test (ordering of clearRequestContext vs renderPageSpecialError):
- Expected ["renderPageSpecialError", "clearRequestContext"]
+ Received ["clearRequestContext", "renderPageSpecialError"]
The existing renderPageSpecialError test mock returns a static Response without reading context, so the current suite cannot catch this.
Suggested fix: don't clearRequestContext on paths that delegate to renderPageSpecialError — that helper already owns the clear (app-page-execution.ts:333/459, and after the boundary render). Split finalizeRenderLifecycle into "abort RSC render + detach listeners" vs "clear request context" so the special-error path can abort without tearing down context. Please also add a test that throws notFound()/forbidden() late from a route whose boundary reads cookies()/headers() and asserts the boundary still sees the context.
The htmlRender.response alternate-response path (L982-985) adds the same early clear; lower risk since that response is already materialized, but worth confirming it doesn't read context lazily.
2. trackStreamCompletion never resolves on cancel (still present)
defer-until-stream-consumed.ts:76-78:
cancel(reason) {
return reader.cancel(reason); // complete() is never called
},If the wrapped flight stream is cancelled before being fully read (client disconnect, both tee branches cancelled), onComplete never fires, so pendingMetadataPlacement (app-page-render.ts:755) stays unresolved forever. Today the only awaiter is PendingAppMetadata inside an SSR render that is itself being torn down, so it's a dangling promise rather than a hard hang — but it's asymmetric with the sibling deferUntilStreamConsumed, which this same PR deliberately taught to fire on cancel/error (L41-45). Recommend calling complete() in cancel for consistency and to avoid a latent hang if a future caller awaits this promise outside the SSR scope.
3. Double metadata render across SSR and BrowserRoot (looks covered, one gap)
On initial load, pendingMetadata is rendered by PendingAppMetadata (SSR, app-ssr-entry.ts:424-429) and by BrowserRoot (app-browser-entry.ts:1047-1054) — two trees emitting the same hoistable <title>/<meta>, relying on React 19 hoisting dedup.
The new browser spec is good evidence this works: it asserts head > title count is exactly 1 on initial load and after navigation (metadata-static-navigation.browser.spec.ts:144,156,169). The remaining gap is that no test asserts the absence of console hydration warnings on the static/ISR initial render. Please confirm there are none (or assert it in the spec). If BrowserRoot only needs MetadataHead for client-side navigation, gating it to non-initial renders would remove the duplication risk entirely.
Minor
app-page-dispatch.ts:896-901: the nested ternary formetadataPlacement(VINEXT_PRERENDER === "1" || shouldRenderSharedArtifact ? (VINEXT_PRERENDER === "1" ? "head" : "head-if-static") : undefined) is correct but hard to read and evaluatesprocess.env.VINEXT_PRERENDERtwice. A smallif/elseor helper would be clearer.app-page-route-wiring.tsx:528-536createAppPageRouteBodyMetadata: theconst metadataHtml = (...); return metadataHtml;refactor is a no-op vs returning the JSX directly — just diff noise.
What looks good
head-if-staticwire path (app-elements-wire.tsreadPendingMetadatavalidation) + theslot.tsxisPendingMetadataValueguard so transported pending-metadata isn't rendered as a slot element — both well done and tested.- Backpressure test and the abort-on-cancel / abort-on-request-abort coverage are exactly the right tests for the new lifecycle.
- ISR MISS/HIT/STALE/regen head-metadata test and the parallel/default-slot dynamic body cases match Next.js v16.2.6 and link to the upstream tests. Good parity discipline.
Verification: ran tests/app-page-render.ts, tests/slot.test.ts, tests/app-elements.test.ts, tests/app-page-element-builder.test.ts, tests/app-router-production-server.test.ts — all green (261 tests). The blocker (#1) is not caught by the existing suite for the reason noted above.
Overall: #1 is the blocker before merge; #2 is a small consistency fix; #3 just needs the hydration-warning confirmation.
|
Review posted to PR #2330. SummaryI re-reviewed the full diff against 1. Request context cleared too early on the late special-error path (blocker). 2. 3. Double metadata render (SSR + BrowserRoot). Relies on React 19 hoisting dedup. The browser spec asserts Plus two minor nits (nested ternary readability in dispatch; a no-op refactor in |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
Review: classify static metadata after rendering (re-review @
|
|
Posted the review to PR #2330. SummaryI re-reviewed the full diff at the current HEAD ( 1. Late special-error context clear (was the blocker) — FIXED. 2. 3. Double metadata render (SSR + BrowserRoot) — still open. SSR's Plus two unchanged minor nits (nested ternary in dispatch, no-op refactor). Verified targeted suites pass and |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: classify static metadata after rendering (re-review @ b3e041a)
Re-reviewed the full diff against main and traced the metadata-deferral + lifecycle plumbing end to end. The latest commit (b3e041a, "align metadata hydration placement") resolves the last open concern (#3). All three items from the prior reviews are now addressed. Verified the fixes in code/tests and that the changed files pass vp check.
1. Late special-error context clear (was the blocker) — RESOLVED ✅
23b6db3 split the lifecycle into finalizeRenderLifecycle (abort RSC + detach listeners) vs clearRequestContext vs completeRenderLifecycle. The late special-error path (app-page-render.ts:1028-1035) now calls finalizeRenderLifecycle(specialError) (abort-only) before renderPageSpecialError, and only clears context on the error path. The regression test (app-page-render.test.ts, "keeps request headers and cookies available while rendering a late special error") confirms headers()/cookies() stay live and clearRequestContext hasn't run while the boundary streams. Issue-#660 class closed.
2. trackStreamCompletion never resolves on cancel — RESOLVED ✅
defer-until-stream-consumed.ts:76-79 cancel() now calls complete(), so pendingMetadataPlacement no longer dangles — symmetric with the sibling deferUntilStreamConsumed's onAbort-on-cancel.
3. Double metadata render across SSR and BrowserRoot — RESOLVED ✅
b3e041a removes the SSR↔client divergence two ways:
BrowserRootno longer rendersMetadataHeadon initial hydration — it is now gated to client navigation only (app-browser-entry.ts:1053:treeState.renderId !== 0 && treeMetadata.pendingMetadata). So on the initial render the client tree produces no head metadata, leaving the SSR-emitted placement (head tags for static, hidden body<div>for dynamic) untouched. This eliminates the body-placement case where SSR put the title in the body while the hydrating client put it in the head.- SSR marks the body div and
BrowserRootcleans it up on navigation —PendingAppMetadatanow tags its hidden div withdata-vinext-pending-metadata="body"(app-pending-metadata.tsx:28), and the navigation layout effect removes it whenrenderId !== 0(app-browser-entry.ts:1033-1035). On navigation the new metadata hoists into<head>and the stale body div is dropped.
I confirmed the eager body-metadata source (createAppPageRouteBodyMetadata, wiring) and the deferred one (PendingAppMetadata, head-if-static) are mutually exclusive per metadataPlacement, so there's never a double body div, and only the deferred (marked) div needs the explicit querySelector().remove() — the eager one rides in the route element subtree and is reconciled by React on navigation.
Most importantly, the new browser test (metadata-static-navigation.browser.spec.ts:156-201, ported from Next.js v16.2.6 ppr-metadata-streaming) is exactly the body-placement hydration-warning coverage the prior reviews asked for:
- asserts SSR HTML puts dynamic metadata in the body, not head;
- after hydration asserts
head > title== 0,body title== 1, andconsoleErrorsis empty (hydration mismatches surface asconsole.error, and theconsoleErrorsfixture does not filter them — verified intests/e2e/fixtures.ts:14-27); - after navigation asserts metadata moves to
<head>(count 1), the body div is gone, and again no console errors.
This matches Next.js behavior (dynamic metadata streams into the body and only relocates to head on client navigation) and is linked to the upstream test. Good parity discipline.
Minor (unchanged, optional)
app-page-dispatch.ts:896-901: the nested ternary formetadataPlacementis correct but hard to read and evaluatesprocess.env.VINEXT_PRERENDERtwice — a smallif/elseor helper would read better.app-page-route-wiring.tsx:528-536createAppPageRouteBodyMetadata:const metadataHtml = (...); return metadataHtml;is a no-op vs returning the JSX directly — diff noise.
What looks good
- Lifecycle split (
finalizeRenderLifecycle/clearRequestContext/completeRenderLifecycle) is clean, and the late-special-error regression test is precisely the right coverage. head-if-staticwire path (app-elements-wire.tsreadPendingMetadatavalidation) + theslot.tsxisPendingMetadataValueguard so transported pending-metadata isn't rendered as a slot element — both well done and tested.- Hydration-placement gating in
BrowserRoot+ the marked body div cleanup is a clean way to remove the SSR/client divergence without losing the "move to head on navigation" behavior. - Backpressure test and abort-on-cancel / abort-on-request-abort coverage are exactly right for the new lifecycle.
- ISR MISS/HIT/STALE/regen head-metadata test and the parallel/default-slot dynamic body cases match Next.js v16.2.6 and link to the upstream tests.
Verification: ran tests/app-page-render.test.ts (56), tests/slot.test.ts (25), tests/app-elements.test.ts (78), tests/app-page-element-builder.test.ts (39), tests/app-router-production-server.test.ts + tests/app-page-stream.test.ts (89) — all green; vp check clean on the changed server files.
Overall: all three concerns are now resolved. Only the two optional readability nits remain. LGTM.
|
Review posted to PR #2330. SummaryI re-reviewed the full diff at the current HEAD ( 1. Late special-error context clear (original blocker) — RESOLVED. 2. 3. Double metadata render (SSR + BrowserRoot) — RESOLVED in
Critically, the new browser test (ported from Next.js v16.2.6 I verified the eager vs deferred body-metadata sources are mutually exclusive (no double body div), ran the targeted unit/integration suites (287 tests, all green), and confirmed |
|
Worker B note: I checked this against GitHub Actions run 28478866791 / job 84413308650. This PR appears to cover the metadata-static-generation and metadata-streaming-static-generation failures in that run: the touched runtime path is the async/static metadata placement/classification path, and the PR validation already calls out the exact static-generation suite. I am treating those suites as covered here and will skip duplicating this fix. |
|
Deploy-suite run 28478866791 / job 84413308650 triage: I think this PR already covers the static metadata title failures from |
|
This appears to cover run 28478866791 / job 84413308650 for the App Router |
|
This looks like it likely covers the non-cacheComponents metadata lane from run 28478866791 / job 84413308650: |
|
Targeted follow-up for run 28478866791 / report job 84413308650: I checked the exact Command used from a clean proof worktree: PATH="/Users/jamesanderson/.vite-plus/js_runtime/node/24.15.0/bin:/Users/jamesanderson/.vite-plus/bin:$PATH" \
REPO="$(pwd)" \
NEXTJS_DIR="/Users/jamesanderson/Developer/vinext/.nextjs-ref" \
NEXTJS_PREPARE=0 \
VINEXT_BUILD=0 \
NEXT_TEST_CONCURRENCY=1 \
./scripts/run-nextjs-deploy-suite.sh "/Users/jamesanderson/Developer/vinext/.nextjs-ref" \
--retries 0 \
-c 1 \
test/e2e/app-dir/metadata-streaming-static-generation/metadata-streaming-static-generation.test.tsResult: failed, 5 passed / 2 failed. The three original static metadata rows from run 28478866791 now pass on this branch, but the full targeted file still fails:
So I am still treating this PR as the owner for the original static rows and skipping duplicate implementation work, but I would not count the target as exact-proofed until this file passes end to end. |
Summary
Validation
Excludes cacheComponents/PPR/resume work.