fix(app-router): match streaming metadata error responses#1794
Conversation
Vinext currently serializes metadata-originated redirects into a Flight payload for document requests. That response is not an HTML page, so direct browser visits cannot follow the streamed redirect and html-limited bots receive 200 instead of the blocking redirect. The dispatcher also renders HTTP access fallback head metadata from layouts only, so not-found boundary metadata is omitted when generateMetadata() throws notFound(). Use Next.js's split response model: RSC requests keep the Flight redirect digest, streaming-capable document requests receive the refresh meta tag HTML, and html-limited bots fall through to the HTTP redirect. Thread the selected HTTP access boundary module into fallback rendering so boundary metadata is resolved exactly once.
commit: |
The unit CI failure came from a strict dispatch test receiving optional fallback fields with undefined or null values. Those fields are only meaningful when a boundary module is actually selected. Build fallback option objects without absent boundary fields, preserving the existing call shape for ordinary page-level HTTP access fallbacks. Add focused coverage for fromMetadata redirects on RSC requests so metadata document refresh handling cannot accidentally consume the client navigation Flight path.
|
/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.
Reviewed the metadata streaming error split. The transport split (RSC → Flight digest, streaming document → HTML refresh meta tag, html-limited bot → blocking 307) is correctly threaded through dispatch and the boundary-module metadata resolution looks right (verified resolveAppPageHead now receives pageModule: boundaryModule, and the new renderAppPageHttpAccessFallback self-resolves the boundary module for the non-layout-aligned not-found.tsx case). Cookie/header handling on the new HTML response is consistent with the existing Flight and 307 paths.
One correctness-adjacent issue: a pre-existing docblock just above this hunk is now factually wrong after this change (it still claims metadata redirects never become a 307 and always ride in the flight payload). Since this PR is the thing that invalidates it, it should be updated here to avoid misleading future readers. Details inline. Two minor notes as well.
| // 2. `generateMetadata()` redirects — metadata is suspended in Next.js, | ||
| // so the redirect rides inside the streamed flight payload even for | ||
| // full document SSR. The status line stays 200. | ||
| // Document requests that redirect from `generateMetadata()` are HTML |
There was a problem hiding this comment.
The docblock for APP_PAGE_METADATA_ERROR_MARKER (lines 39-50, just above this hunk, not shown in the diff) is now stale and contradicts this PR:
Metadata resolution is suspended/streamed in Next.js, so a redirect from metadata never becomes an HTTP-level 307 — it rides inside the flight payload with a 200 status, regardless of whether the request is RSC or a full document SSR.
After this change that is no longer true: document requests get an HTML refresh meta tag (not a flight payload), and html-limited bots get a blocking 307. Since this PR is what invalidates the comment, please update it (and the metadata-navigation test reference) in the same PR so the marker's documentation matches the new transport split.
| const boundaryComponent = | ||
| options.boundaryComponent ?? | ||
| getDefaultExport(boundaryModule) ?? | ||
| resolveAppPageHttpAccessBoundaryComponent({ |
There was a problem hiding this comment.
Minor: when options.boundaryComponent and options.boundaryModule are both unset, getDefaultExport(boundaryModule) (line 310) and resolveAppPageHttpAccessBoundaryComponent(...) (line 311) compute the same component — boundaryModule is resolvedBoundaryModule, which resolveAppPageHttpAccessBoundaryComponent derives via the same resolveAppPageHttpAccessBoundaryModule call. The fallback to resolveAppPageHttpAccessBoundaryComponent is now effectively dead for the resolved-module path. Not a bug, but you could drop line 311's branch (or keep it only for the explicit-boundaryComponent case) to avoid the duplicate resolution.
| } | ||
|
|
||
| const errorMetaTags = renderSsrErrorMetaTags([{ digest: options.digest }]); | ||
| return new Response(`<!DOCTYPE html><html><head>${errorMetaTags}</head><body></body></html>`, { |
There was a problem hiding this comment.
The streaming-redirect document is a minimal stub (<head> with only the refresh meta tag, empty <body>), whereas Next.js inserts the __next-page-redirect tag into the otherwise-rendered streamed document. For a redirect this is functionally fine (the browser follows the refresh regardless of body), but worth a one-line note here that this intentionally diverges from Next.js's full-document insertion, so a future reader doesn't assume page content was dropped by mistake.
|
Review submitted successfully. Here's a summary of my review of PR #1794: Review SummaryThe PR correctly implements the metadata streaming error transport split. I verified the core mechanics: Correct:
Findings posted (1 main + 2 minor):
No blocking correctness bugs found. |
…ad boundary fallback Address review feedback on the metadata streaming error split: - Update the stale APP_PAGE_METADATA_ERROR_MARKER docblock to describe the new three-way transport split (flight digest / HTML refresh meta tag / 307). - Note the intentional minimal-stub HTML divergence from Next.js's full-document insertion in buildMetadataRedirectHtmlResponse. - Drop the redundant resolveAppPageHttpAccessBoundaryComponent fallback in renderAppPageHttpAccessFallback (boundaryModule already covers both paths).
|
/bigbonk re-review for issues |
There was a problem hiding this comment.
Re-review after f41090c. The two prior findings (the stale app-page-execution.ts docblock and the redundant boundary-component resolution) are resolved, and the divergence comment for the stub HTML redirect is now in place.
Verified correct:
- Transport split (RSC → Flight digest, streaming document → HTML refresh meta tag, html-limited bot → blocking 307) is correctly threaded through dispatch via
serveStreamingMetadata/shouldServeStreamingMetadata. - Build-time
__htmlLimitedBotsinjection covers prod, not just dev. - basePath is applied exactly once: the digest URL is pre-prefixed via
sameOriginPathOrAbsolute(prefixedLocation, …)andrenderSsrErrorMetaTagsis called without abasePathoption, so no double-prefix. escapeHtmlAttrguards the refresh URL in the meta tag.serveStreamingMetadata !== falsedefaults to HTML refresh when unset, and both prod callers (renderLayoutSpecialError,renderPageSpecialError) pass the computed flag — consistent withapp-page-element-builder.ts:206-213placing metadata inhead(blocking) for html-limited bots, which is what surfaces the redirect synchronously.- Not-found boundary metadata fix (
pageModule: boundaryModuleintoresolveAppPageHead) renders the title exactly once; verified by the new test and thatpageModuleis a distinct metadata source fromlayoutModules. resolveAppPageHttpAccessBoundaryComponentstill exported/tested; refactor is functionally equivalent.- Cookie/middleware-header handling on the new HTML response matches the Flight and 307 paths.
Targeted tests pass locally (tests/app-page-execution.test.ts, tests/app-page-boundary-render.test.ts, tests/app-page-boundary.test.ts).
Two remaining doc nits (no inline anchor — the lines fall outside the diff hunks):
- (net-new — the prior review missed this file)
packages/vinext/src/server/app-page-head.ts:22-26— theresolveModuleMetadatadocblock still asserts metadata redirects "ride inside the flight payload with a 200 status code even for document SSR." This PR invalidates that exact statement: document SSR now returns an HTML refresh meta tag, and html-limited bots return a blocking 307. This is the same factual error the prior review flagged inapp-page-execution.ts, but in a file that review didn't examine. Suggested rewrite:
* suspended/streamed in Next.js. Its redirects no longer become a plain
* HTTP 307: RSC navigation rides inside the flight payload (200), streaming
* document SSR gets an HTML refresh meta tag (200), and html-limited bots
* get a blocking 307 — whereas page redirects still emit a 307 for SSR.
* See https://github.com/cloudflare/vinext/issues/1347
* and Next.js test/e2e/app-dir/metadata-streaming.
packages/vinext/src/server/app-page-execution.ts:54-56— the docblock body was correctly rewritten to describe the three transports, but theSee Next.js testreference still points atmetadata-navigation. Since the behavioural contracts here are ported frommetadata-streaming, the reference should follow:
* See Next.js test:
* test/e2e/app-dir/metadata-streaming/metadata-streaming.test.ts
Neither is a correctness bug — both are stale comments that will mislead future readers about the transport behavior this PR establishes. No blocking issues found.
|
Review submitted successfully. SummaryI re-reviewed PR #1794 after the latest commit ( Resolved from prior review:
Verified correct (no blocking bugs):
New findings (2 doc nits, neither blocking):
Both are stale comments that would mislead future readers. I posted suggested rewrites as a top-level review (inline anchoring failed with 422 because both docblocks fall outside the diff hunks). |
|
/bigbonk Addressed all three review findings in f41090c:
Please re-review. |
There was a problem hiding this comment.
Re-reviewed f41090c. The three prior findings from the original review are correctly resolved:
APP_PAGE_METADATA_ERROR_MARKERdocblock (app-page-execution.ts:39-52) — rewritten to describe the three-way transport split accurately.buildMetadataRedirectHtmlResponsedivergence note (app-page-execution.ts:320-324) — clear comment added explaining the intentional minimal-stub vs. Next.js full-document insertion.- Dead
resolveAppPageHttpAccessBoundaryComponentfallback (app-page-boundary-render.ts:306-312) — removed;boundaryComponentnow derives fromgetDefaultExport(boundaryModule)as the single source of truth, and the unused import is gone.
Verified correct (no blocking bugs)
- The transport split in
buildAppPageSpecialErrorResponseis sound:fromMetadata && !isRscRequest && serveStreamingMetadata !== false-> HTML refresh meta tag;isRscRequest-> Flight digest; everything else (includingserveStreamingMetadata === false) -> blocking 307. The early-return ordering means html-limited bots fall through to the 307 path correctly. __htmlLimitedBotsis build-time injected viaJSON.stringifyinapp-rsc-entry.ts:511, so prod is covered, not just dev. It's threaded into bothrenderLayoutSpecialErrorandrenderPageSpecialError.shouldServeStreamingMetadata(used in dispatch) is consistent withapp-page-element-builder.ts:206-213, which places metadata inhead(blocking) for html-limited bots — that's what surfaces the redirect synchronously.boundaryComponent/boundaryModuleare always paired at every call site (both dispatch closures and bothapp-fallback-renderer.tspaths), so there's no risk of a mismatched component/module feeding the wrong metadata intoresolveAppPageHead.{ digest }is consumed correctly byrenderSsrErrorMetaTags->getNextErrorDigest, andescapeHtmlAttrguards the refresh URL.- Cookie/middleware-header handling on the new HTML response matches the Flight and 307 paths.
Two remaining doc nits (non-blocking)
Both were flagged in the prior re-review and are still present in f41090c. Both fall outside the diff hunks, so I could not anchor them inline (the inline attempt returned 422). Neither blocks merge, but they are stale comments that will mislead future readers about the transport behavior this PR establishes.
1. packages/vinext/src/server/app-page-head.ts:23-26 — the resolveModuleMetadata docblock still asserts metadata redirects "ride inside the flight payload with a 200 status code even for document SSR." This PR invalidates that exact statement (document SSR now returns an HTML refresh meta tag; html-limited bots return a blocking 307). Same factual error that was fixed in app-page-execution.ts, just in a sibling file. Suggested rewrite:
* suspended/streamed in Next.js. Its redirects no longer become a plain
* HTTP 307: RSC navigation rides inside the flight payload (200), streaming
* document SSR gets an HTML refresh meta tag (200), and html-limited bots
* get a blocking 307 — whereas page redirects still emit a 307 for SSR.
* See https://github.com/cloudflare/vinext/issues/1347
* and Next.js test/e2e/app-dir/metadata-streaming.
2. packages/vinext/src/server/app-page-execution.ts:54-56 — the docblock body was correctly rewritten to describe all three transports, but the See Next.js test reference still points at metadata-navigation. The behavioural contracts here are ported from metadata-streaming (per the PR description and the new tests), so the reference should follow:
* See Next.js test:
* test/e2e/app-dir/metadata-streaming/metadata-streaming.test.ts
No blocking issues found.
|
Review submitted successfully. SummaryI re-reviewed PR #1794 after commit Resolved from prior review (verified):
Verified correct (no blocking bugs):
Two remaining non-blocking doc nits (both flagged in the prior re-review, both still present; they fall outside the diff hunks so inline anchoring returned 422):
No blocking issues found. |
…nd test reference Address remaining bonk re-review nits: - Update resolveModuleMetadata docblock in app-page-head.ts to describe the three-way transport split (it still claimed redirects ride in the flight payload even for document SSR). - Point the APP_PAGE_METADATA_ERROR_MARKER Next.js test reference at metadata-streaming, which is where the behavioural contracts are ported from.
|
/bigbonk Addressed the two remaining doc nits in a1e749c:
Please re-review. |
There was a problem hiding this comment.
Re-reviewed a1e749cd. Both remaining doc nits from the prior re-review are now resolved.
Resolved (verified):
packages/vinext/src/server/app-page-head.ts:23-28— theresolveModuleMetadatadocblock no longer claims metadata redirects "ride inside the flight payload with a 200 status code even for document SSR." It now describes the three-way transport split accurately (RSC flight digest 200 / streaming document HTML refresh meta tag 200 / html-limited bot blocking 307) and theSeereference points atmetadata-streaming.packages/vinext/src/server/app-page-execution.ts:54-55— theSee Next.js testreference now points attest/e2e/app-dir/metadata-streaming/metadata-streaming.test.ts, matching the behavioural contracts this PR ports.
No blocking issues. The docblocks are now consistent with the transport split this PR establishes. LGTM.
|
Re-review submitted. Both remaining doc nits from the prior review are resolved in
No blocking issues. The docblocks are now consistent with the transport split the PR establishes. LGTM. |
2 participants
Overview
app-page-execution.ts,app-page-dispatch.ts,app-page-boundary-render.tsmetadata-streamingdeploy-suite file now passes against vinext.Why
Metadata errors are not a single response shape in Next.js. A
redirect()thrown fromgenerateMetadata()stays encoded as a Flight digest for RSC navigation, becomes an HTML refresh meta tag for streaming-capable document requests, and becomes an HTTP redirect when metadata is blocking for html-limited bots.notFound()from metadata also needs the selectednot-found.tsxboundary module to participate in metadata resolution exactly once.__next-page-redirectrefresh meta tag for streaming document redirects.htmlLimitedBotsinto dispatch and falls through to HTTP 307/308 when streaming metadata is disabled.What changed
generateMetadata()redirect, RSC requestgenerateMetadata()redirect, streaming document requesttext/x-componentFlight payloadgenerateMetadata()redirect, html-limited botLocationgenerateMetadata()not-found into a route boundaryMaintainer review path
packages/vinext/src/server/app-page-execution.tsfor the transport split in metadata redirect responses.packages/vinext/src/server/app-page-dispatch.tsandpackages/vinext/src/entries/app-rsc-entry.tsforhtmlLimitedBotspropagation into the runtime decision.packages/vinext/src/server/app-page-boundary.tsandapp-page-boundary-render.tsfor resolving the selected HTTP access boundary module separately from its default component.tests/app-page-execution.test.ts,tests/app-page-boundary-render.test.ts, andtests/app-router-dev-server.test.tsfor the ported behavioural contracts.Validation
vp checkvp test run tests/app-page-execution.test.ts tests/app-page-boundary-render.test.tsvp test run tests/app-router-dev-server.test.ts -t "redirect\\(\\) from generateMetadata"knip.vp env exec --node 24 ./scripts/run-nextjs-deploy-suite.sh /Users/nathan/Projects/vinext/.refs/nextjs-v16.2.6 --retries 0 -c 1 --debug test/e2e/app-dir/metadata-streaming/metadata-streaming.test.tsRisk / compatibility
Non-goals
redirect()ornotFound()semantics outside metadata.References
__next-page-redirectrefresh meta tag emitted for streamed redirect errors.