fix(app-router): emit streamed redirect and not-found meta tags#1261
fix(app-router): emit streamed redirect and not-found meta tags#1261NathanDrake2406 wants to merge 2 commits into
Conversation
Streaming App Router SSR currently returns a 200 once the shell commits, but redirect() or notFound() discovered inside a later Suspense boundary never emits the browser-visible meta tag that Next.js relies on. That leaves the control-flow digest only in the RSC payload until client hydration handles it. The SSR onError path now captures digest-bearing render errors and a request-local flusher injects noindex or refresh meta tags through the existing server-inserted HTML stream hook. The app page pipeline also forwards basePath into SSR so streamed redirect meta tags match early redirect responses.
commit: |
Streamed redirect meta tags checked basePath against the full redirect location string. Already-prefixed root URLs with a query or hash, such as /docs?from=checkout, failed the pathname-shaped basePath check and were prefixed a second time. Add a shared pathname-only basePath prefix helper and use it from both early redirect responses and streamed redirect metadata. The streamed path now splits query/hash before checking the pathname.
|
/bigbonk review |
There was a problem hiding this comment.
Clean, well-scoped implementation. The architecture mirrors Next.js closely — createSsrErrorMetaRenderer with incremental capture/flush maps neatly to Next.js's flushedErrorMetaTagsUntilIndex pattern in make-get-server-inserted-html.tsx. The basePath plumbing through the SSR pipeline is correct. Tests are thorough: unit tests for the helper, integration test for streaming timing, and regression coverage for query/hash basePath edge cases.
A few minor observations below, none blocking.
| if (httpError) { | ||
| let html = '<meta name="robots" content="noindex" />'; | ||
| if ((options.nodeEnv ?? process.env.NODE_ENV) === "development") { | ||
| html += '<meta name="next-error" content="not-found" />'; |
There was a problem hiding this comment.
Nit: the next-error dev meta tag is emitted for all HTTP error fallback statuses (403, 401, etc.), not just 404. The content="not-found" value is a bit misleading for a 403 forbidden error. Next.js does the same thing (isHTTPAccessFallbackError covers all of these and emits content="not-found" for all of them), so this is correct for parity — but worth noting the Next.js oddity if someone wonders about it later.
| if (!redirect) return ""; | ||
|
|
||
| const delay = redirect.status === PERMANENT_REDIRECT_STATUS ? 0 : 1; | ||
| const location = prefixRedirectLocation(redirect.url, options.basePath); |
There was a problem hiding this comment.
Minor observation: Next.js's addPathPrefix in the streaming meta tag path works on the raw URL string from getURLFromRedirectError(), which is already decoded. The prefixRedirectLocation here receives the same decoded URL from parseNextRedirectDigest. The manual pathname extraction (lines 26-36) is necessary here since new URL() would choke on relative paths. This is correct — just noting the intentional divergence from applyAppPageRedirectBasePath which can afford to use new URL(location, requestUrl) because it has a request URL for resolution.
| return value | ||
| .replace(/&/g, "&") | ||
| .replace(/"/g, """) | ||
| .replace(/</g, "<") |
There was a problem hiding this comment.
Good addition — < and > escaping makes escapeHtmlAttr properly comprehensive for any HTML attribute context. The existing callers (font URLs, nonces, module preload HREFs) won't be affected since those values don't contain angle brackets, but it's correct defense-in-depth for the new redirect URL use case.
| const insertedHTML = renderInsertedHtml(renderServerInsertedHTML()); | ||
| if (didInjectHeadHTML) return insertedHTML; | ||
| const errorMetaHTML = errorMetaRenderer.flush(); | ||
| if (didInjectHeadHTML) return insertedHTML + errorMetaHTML; |
There was a problem hiding this comment.
The ordering here — insertedHTML + errorMetaHTML — means error meta tags appear after server-inserted HTML from useServerInsertedHTML callbacks. On the first flush (head injection), this places them after the nav scripts, form state, module preloads, and font HTML. On subsequent flushes, they appear after any new server-inserted HTML. This matches Next.js where error meta tags are appended after other server-inserted elements in the JSX array. Looks correct.
| capturedRscDataRef: revalidatedCapturedRscRef, | ||
| } | ||
| : undefined, | ||
| { |
There was a problem hiding this comment.
The ISR regeneration path now always passes a basePath key in the options object (even when it's undefined). The old code passed undefined as the 4th argument when there was no sideStream. The new shape { basePath: options.basePath, ...conditionalSpread } is cleaner — it avoids the conditional undefined 4th parameter and ensures basePath flows through ISR regeneration consistently.
|
Review submitted. Here's a summary of what I found: Approved — this is a clean, well-structured PR that correctly implements streamed redirect and not-found meta tags for the App Router, matching Next.js behavior. Key observations from the review:
|
Overview
redirect()andnotFound()after the SSR shell has committed.handleSsrand flush browser-visible meta tags through the existing server-inserted HTML path.server/app-ssr-error-meta.ts,server/app-ssr-entry.ts,server/app-page-stream.ts,tests/rsc-streaming.test.tsWhy
Once the App Router HTML shell streams, a later
redirect()ornotFound()cannot turn the response into a 307 or 404. Next.js treats the streamed HTML as the browser-visible recovery boundary: it records SSR render errors and injects<meta http-equiv="refresh">or<meta name="robots" content="noindex">so browsers and crawlers react before hydration catches up.handleSsrcaptures errors from React's HTMLonErrorand flushes meta tags on each insertion pass.basePathhandling for streamed redirects should match early redirect responses.basePathinto the SSR handler and applies it to app-internal refresh URLs.<and>.What changed
redirect()after the shell has streamedpermanentRedirect()after the shell has streamed0delay for 308.notFound()/ HTTP access fallback after the shell has streamed<meta name="robots" content="noindex">.Maintainer review path
packages/vinext/src/server/app-ssr-error-meta.ts: focused helper for mapping captured digest errors to meta tags and flushing each captured error once.packages/vinext/src/server/app-ssr-entry.ts: React HTMLonErrorcapture and insertion into the existing streamed server-inserted HTML path.packages/vinext/src/server/app-page-stream.ts,app-page-render.ts,app-page-dispatch.ts:basePathplumbing into the SSR handler, including ISR regeneration.tests/app-ssr-error-meta.test.tsandtests/rsc-streaming.test.ts: direct semantics plus the post-shell streaming timing regression.Validation
vp test run tests/app-ssr-error-meta.test.ts tests/app-page-stream.test.ts tests/rsc-streaming.test.ts tests/app-page-render.test.ts tests/app-page-dispatch.test.ts tests/app-ssr-stream.test.tsvp checkknip.Risk / compatibility
basePathprefixing avoids double-prefixing already-prefixed app-internal URLs to stay consistent with vinext's existing redirect response path.Non-goals
References
makeGetServerInsertedHTMLallCapturedErrors.allCapturedErrorsand passes it to the HTML error handler.redirectdocs