fix(app-router): return HTTP 200 when notFound() is thrown from generateMetadata#1864
Conversation
…ateMetadata When generateMetadata() calls notFound(), Next.js treats the error as a suspended metadata result — the not-found UI is rendered client-side through the React NotFoundBoundary while the HTTP response stays 200. Previously, vinext was returning the boundary response with the original 404 status from renderAppPageHttpAccessFallback. The fix overrides the rendered fallback response status from 404 to 200 when specialError.fromMetadata is true for http-access-fallback errors, mirroring the existing redirect-from-metadata path which also returns 200. Fixes the failing e2e test: test/e2e/app-dir/metadata-navigation: "should support notFound in generateMetadata"
commit: |
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: notFound() from generateMetadata → HTTP 200
The fix is correct for the streaming-metadata case the e2e test exercises (next.fetch('/async/not-found') with a normal UA → 200), and the new unit tests are good. But there is one parity divergence worth addressing before merge.
Issue: 200 is forced unconditionally, ignoring serveStreamingMetadata
In buildAppPageSpecialErrorResponse, the new fromMetadata override sets status 200 regardless of serveStreamingMetadata / isRscRequest:
// packages/vinext/src/server/app-page-execution.ts:433
const responseToMerge =
options.specialError.fromMetadata === true
? new Response(fallbackResponse.body, {
headers: fallbackResponse.headers,
status: 200,
statusText: fallbackResponse.statusText,
})
: fallbackResponse;This matches Next.js only when metadata is streamed (serveStreamingMetadata: true). When metadata streaming is disabled (HTML-limited bots), Next.js does not suspend the metadata — the error blocks the render and the status is set to 404:
// .nextjs-ref/packages/next/src/server/app-render/app-render.tsx:2845
if (isHTTPAccessFallback) {
res.statusCode = getAccessFallbackHTTPStatus(err) // 404
metadata.statusCode = res.statusCode
...
}Next.js has an explicit e2e test asserting this:
// .nextjs-ref/test/e2e/app-dir/metadata-streaming/metadata-streaming.test.ts:137
it('should render blocking 404 response status when html limited bots access notFound', async () => {
const { status } = await next.fetch('/notfound', { headers: { 'user-agent': 'Twitterbot' } })
expect(status).toBe(404)
})This is the exact mirror of the redirect path, which vinext already handles correctly via the serveStreamingMetadata !== false guard (app-page-execution.ts:340-343) and which already has a ported test in this very file:
// tests/app-page-execution.test.ts:445
it("uses an HTTP redirect for metadata-originated redirects when metadata streaming is disabled", ...)
// serveStreamingMetadata: false → expect(response.status).toBe(307)So redirect-from-metadata respects serveStreamingMetadata, but the new notFound-from-metadata path does not — they should be symmetric. As written, an HTML-limited bot hitting a notFound()-from-generateMetadata route will get HTTP 200 instead of the 404 Next.js returns.
Suggested fix: gate the 200 override on streaming, e.g.:
const overrideToOk =
options.specialError.fromMetadata === true && options.serveStreamingMetadata !== false;and apply it to both the renderFallbackPage branch and the no-fallback-page branch (line ~449). serveStreamingMetadata is already plumbed into both renderPageSpecialError and renderLayoutSpecialError callers (app-page-dispatch.ts:991,1034), so the value is available here.
Please also add a unit test covering fromMetadata: true + serveStreamingMetadata: false asserting 404, mirroring the existing redirect bot test, and consider porting the Next.js "should render blocking 404 response status when html limited bots access notFound" case.
Non-blocking notes
- The
new Response(...)reconstruction correctly preservesheadersandstatusText, so cookies/boundary headers on the forced-200 response are not lost. Good. clearRequestContext()is still not called on therenderFallbackPagesuccess branch, but that is pre-existing behavior (the redirect/no-fallback branches call it), not introduced here — out of scope for this PR.
Otherwise the change is well-scoped and the comments/test coverage for the streaming case are solid.
|
Review posted to PR #1864. SummaryI reviewed PR #1864 (return HTTP 200 when Issue found: 200 forced unconditionally, ignoring
|
…gMetadata !== false Bonk finding: the 200 status override for notFound()/forbidden()/unauthorized() thrown from generateMetadata() was applied unconditionally, diverging from Next.js for html-limited bots (serveStreamingMetadata === false). In that path metadata blocks render synchronously and Next.js sets the real HTTP error status (app-render.tsx ~2845). Gate both branches of the http-access-fallback path (with fallback page and without) on `options.serveStreamingMetadata !== false`, symmetric with the existing redirect-from-metadata path which already carries the same gate. Adds unit tests: - fromMetadata:true + serveStreamingMetadata:false + fallback → 404 - fromMetadata:true + serveStreamingMetadata:false + no fallback → 404
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: notFound() from generateMetadata → HTTP 200
The second commit (6c26c80) addresses the parity divergence raised in the previous review. The 200 override is now gated on options.serveStreamingMetadata !== false in both branches:
- Fallback-page branch (
app-page-execution.ts:439-446): forced 200 only when streaming metadata is enabled, preserving the rendered boundary HTML, headers, andstatusText. - No-fallback branch (
app-page-execution.ts:459-462):responseStatusresolves to 200 only when streaming is enabled, otherwise the real error status.
This is now symmetric with the redirect-from-metadata path (app-page-execution.ts:339-343), and both callers (renderPageSpecialError, renderLayoutSpecialError) plumb serveStreamingMetadata via shouldServeStreamingMetadata(...) (app-page-dispatch.ts:991,1034).
Verified
- Tests cover the gap: four new cases assert 200 for streaming and 404 for
serveStreamingMetadata: false, in both the with-fallback and no-fallback variants (tests/app-page-execution.test.ts:601,638), plus the non-metadata 404 regression guard (:656).vp test run tests/app-page-execution.test.ts→ 36 passed. - RSC navigation: the http-access-fallback path delegates transport to
renderHttpAccessFallbackPage, which receivesisRscRequest, so the boundary renders as flight for RSC requests; keeping the 200 status matches Next.js (the not-found rides in the flight payload). Correct. - Header/cookie preservation: the
new Response(fallbackResponse.body, { headers, status: 200, statusText })reconstruction preserves headers and statusText, thenmergeAppPageSpecialErrorHeadersmerges middleware headers. No loss.
Non-blocking notes
- No-fallback 200 body text (
:464): when overriding to 200 with nonot-found.tsx, the body is still the literal"Not Found"text and theResponsestatusText defaults to"OK". This is a degenerate edge (no boundary file present) and the metadata-streaming contract surfaces the not-found in the page UI, so the body content is largely moot. Not worth blocking on, but a tiny cosmetic mismatch. clearRequestContext()is still not called on the fallback-page success branch — pre-existing, not introduced by this PR. Out of scope.
The previous review's concern is resolved. LGTM.
|
Review posted to PR #1864. SummaryThe previous Current state — the concern is resolved:
Non-blocking notes:
LGTM. |
What failed
The e2e test
app dir - metadata navigation > navigation > should support notFound in generateMetadatawas failing because vinext returned HTTP 404 whengenerateMetadata()callednotFound(), but the test assertsexpect(res.status).toBe(200).Test fixture (
test/e2e/app-dir/metadata-navigation/app/async/not-found/page.tsx):Root cause
When
notFound()is thrown fromgenerateMetadata(), it propagates out ofresolveAppPageHeadinbuildPageElements. The error is caught bybuildAppPageElement, classified as{ kind: "http-access-fallback", statusCode: 404, fromMetadata: true }, and rendered viarenderPageSpecialError→buildAppPageSpecialErrorResponse.For the
http-access-fallbackcase,buildAppPageSpecialErrorResponsecalledrenderFallbackPage(404)which rendered the local not-found boundary correctly — but with HTTP status 404 (fromrenderAppPageHttpAccessFallback). ThefromMetadata: trueflag was not consulted.Next.js treats
notFound()thrown fromgenerateMetadata()as a suspended metadata result: the not-found UI is rendered client-side through the ReactNotFoundBoundary, while the HTTP response itself stays 200. The test comment says:// metadata is suspended in SSR, it won't affect the response status.Fix
In
buildAppPageSpecialErrorResponse(packages/vinext/src/server/app-page-execution.ts), whenspecialError.fromMetadata === truefor anhttp-access-fallbackerror, override the rendered fallback response status from 404 to 200 before returning it. This mirrors the existing redirect-from-metadata path which already returns 200.The fix preserves the existing behavior for page-level
notFound()calls (nofromMetadataflag) — those still return 404.Mapping to the test
expect(res.status).toBe(200)— fixed: the HTTP response is now 200expect(flightText).toContain('Local found boundary')— already worked: the not-found boundary was already being rendered; only the status was wrongkeywords: 'parent',robots: 'noindex',description: 'Local not found description',title: 'Local not found') — already worked:renderPageSpecialErrorincludes parent layout modules in the metadata resolutionVerification
vp check: 22 pre-existing errors, 0 new errorsvp test run tests/app-page-execution.test.ts: 34 tests pass (31 existing + 3 new tests covering the fix)should support notFound in generateMetadatais expected to pass in CI