fix(app-router): recover SSR shell render errors via __next_error__ document#1908
fix(app-router): recover SSR shell render errors via __next_error__ document#1908NathanDrake2406 wants to merge 11 commits into
Conversation
commit: |
Runtime helper modules embedded into generated entries import vinext's own shims by package subpath (e.g. `vinext/shims/headers`), while source checkouts alias userland `next/*` imports to the local shim files. The two specifiers resolved to different module instances, so request-scoped singleton state (navigation context, headers) split between the source shim copy and the package export copy. The violated invariant is that every shim module must be a per-request singleton regardless of import specifier. Resolve `vinext/shims/*` through the same plugin path as the `next/*` aliases so both forms land on the local shim files. Exercised by the SSR shell-error recovery browser spec, whose fixture imports vinext from the source checkout and depends on shared navigation state across both import forms.
…ocument When the HTML (Fizz) render rejects during SSR, vinext re-rendered a server-side global-error page whose flight payload encodes the error tree. For an app without a custom global-error.tsx that meant the default error card with no path back to the real page: an SSR-phase-only throw (e.g. a client component using the "throw to opt out of server rendering" pattern) left the browser stuck on the card even though the client render would succeed. The violated expectation is Next.js's shell-error semantics: a failed HTML shell is served as the default `__next_error__` error document carrying the ORIGINAL flight payload and the bootstrap module, and the browser re-renders the real tree from that payload with createRoot instead of hydrating. Local error.tsx boundaries still win — they ship inside the flight payload and catch the re-thrown error client-side. handleSsr now resolves to that recovery document instead of rejecting, but only when both hold: - the error did not originate in the RSC render (no string `digest`), so flight errors, redirect()/notFound(), and server-component throws keep driving the existing rejection-based boundary machinery, and - the app has no custom global-error.tsx (the generated entry knows at build time and threads hasCustomGlobalError through dispatch/render options); apps with one keep the server-rendered boundary re-render. The browser entry switches from hydrateRoot to createRoot when the document root carries id="__next_error__", dropping the error-shell styles first. Covered by the new ssr-error-shell-recovery browser spec (recovery to real content, local error.tsx for SSR-only and unconditional client throws) and the existing tests/nextjs-compat/global-error.test.ts boundary-semantics suite.
NathanDrake2406
force-pushed
the
nathan/next-dynamic-css
branch
from
c7c5160 to
99789d7
Compare
NathanDrake2406
changed the title
- Add static prerender no-boundary recovery regression test to ssr-error-shell-recovery.browser.spec.ts - Document broad __next_error__ browser marker in app-browser-entry.ts - Extract stripJsExtension to utils/path.ts and wire into all shim resolution sites (vinext/shims/* + react-server shims) Non-functional: targeted regression coverage + code documentation + minor resolver hardening per reviewer feedback.
|
/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 SSR shell-error recovery and the vinext/shims/* resolver fix. Overall this is a clean, well-scoped change with good test coverage. The core mechanism is correct: the error-document shell reuses the same rscEmbed/createTickBufferedTransform pipeline so the flight payload + nav metadata are still embedded, and createRoot(document) is valid in React 19 (isValidContainer accepts nodeType === 9). I ran tests/entry-templates.test.ts (25 pass), tests/nextjs-compat/global-error.test.ts (24 pass), and vp check on the changed server/util files (clean).
A few observations below — none are blockers, mostly worth confirming or noting.
Design note (non-blocking): When the fallback fires, renderAppPageHtmlStreamWithRecovery now resolves successfully, so the response takes the normal options.status (200) and is eligible for ISR cache writes (boundaryOutcome: { kind: "success" } in app-page-render.ts). This is intended for the "throw to opt out of SSR" pattern and matches Next.js shell-error semantics, and the /static-recovery test exercises the cached path deliberately. But note this also broadens behavior for genuine SSR-only crashes (no digest, no custom global-error): they now silently recover to a cached 200 instead of rejecting to a 500 error boundary. That's the documented Next.js behavior, so it's likely fine — just calling it out since it's a meaningful semantic shift, not only the dynamic-CSS case.
| ) { | ||
| throw error; | ||
| } | ||
| errorMetaRenderer.capture(error); |
There was a problem hiding this comment.
errorMetaRenderer.capture(error) here is effectively a no-op for the shell-error case: the fallback only runs when the error has no string digest, and renderSsrErrorMetaTag returns "" unless getNextErrorDigest(error) yields a digest. So nothing is ever flushed into the head for these errors. Not a bug — but the call reads like it surfaces the error somewhere, when it doesn't. Consider a brief comment clarifying it's defensive/consistency-only, or dropping it.
| } | ||
|
|
||
| function markErrorShellStyle(html: string): string { | ||
| return html.replace("<style>", '<style data-vinext-error-shell-style="">'); |
There was a problem hiding this comment.
markErrorShellStyle replaces only the first <style>. That's correct today because DefaultGlobalError renders exactly one <style dangerouslySetInnerHTML>, and any inline-CSS/server-inserted styles are added later by the transform (so they correctly stay unmarked and are preserved by the browser cleanup). This is fragile against future edits to DefaultGlobalError adding a second <style> — worth a one-line comment noting the single-style assumption, since the browser-side style[data-vinext-error-shell-style] removal depends on it.
| // through this plugin so request-scoped singleton state is not split | ||
| // between source shims and the package export copy. | ||
| const vinextShimPrefix = "vinext/shims/"; | ||
| if (cleanId.startsWith(vinextShimPrefix)) { |
There was a problem hiding this comment.
This branch now intercepts every vinext/shims/* subpath before the _reactServerShims check below. That's correct for the current shim set (the react-server variants are keyed on userland next/* specifiers like next/navigation, not vinext/shims/navigation), so internal vinext/shims/navigation imports have always resolved to the plain shim. But if a future shim needs a .react-server variant when imported via its vinext/shims/* package subpath, this early return would silently shadow it. A short comment noting that this branch deliberately does not handle per-environment variants (and where to add that if needed) would prevent a subtle future regression.
|
Review posted successfully. Here's a summary of my review of PR #1908: Review SummaryThe PR carries two clean changes: SSR shell-error recovery via the Verification I did:
Findings posted (all non-blocking):
No correctness blockers found. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the SSR shell-error recovery and the vinext/shims/* resolver fix. The core mechanism is sound and I verified the important pieces:
- The error shell HTML is piped through the same
createTickBufferedTransform(rscEmbed, getInsertedHTML, ...)pipeline, so the flight payload + nav metadata + modulepreload all land in the document (the transform finds</head>inDefaultGlobalError's static markup and injects there). No double bootstrap:buildHeadInjectionHtmlonly emits<link rel="modulepreload">, andrenderToStaticMarkupdoesn't inject a Fizz bootstrap, so the manual<script id="_R_">is the only executing one. - The
vinext/shims/*resolver branch aligns source-checkout resolution with the published-package exports map (./shims/*->./dist/shims/*.js, no react-server condition)._reactServerShimsis keyed only onnext/*userland specifiers, so this doesn't shadow any current variant, and it correctly stops splitting singleton state between thenext/navigation-aliased source shim and thevinext/shims/navigationdist copy. Confirmed internal modules (app-route-handler-dispatch.ts,app-middleware.ts) importvinext/shims/navigationand rely on shared state. markErrorShellStyletargets<style>which is whatrenderToStaticMarkupemits for thedangerouslySetInnerHTMLstyle tag.
No correctness blockers. A few observations below, all non-blocking.
Dev-mode parity (worth confirming): hasCustomGlobalError threads through the generated RSC entry, which is the request handler in both dev and prod. So in dev, a digest-less SSR-only throw without a custom global-error now resolves to the recovery shell rather than surfacing the dev error overlay. The new browser __next_error__ branch in bootstrapHydration (createRoot, not reload) also runs in dev. The new spec only exercises production builds, so dev behavior for this path is untested. If the intent is to keep the dev overlay for genuine SSR-only crashes, that may be a gap.
ISR caches the recovery shell at 200 (restating the prior design note): when the fallback fires, renderAppPageHtmlStreamWithRecovery resolves with a stream and response: null, so the response keeps the normal status and is ISR-eligible. Intended for the opt-out pattern, but it also means a genuine SSR-only crash now caches a 200 error-shell instead of producing a 500. Matches Next.js shell-error semantics; flagging only because it's a meaningful surface for real bugs that manifest only during SSR.
| if (options?.waitForAllReady === true) { | ||
| await renderedHtmlStream.allReady; | ||
| } | ||
| htmlStream = renderedHtmlStream; |
There was a problem hiding this comment.
On the waitForAllReady (prerender) path, if renderedHtmlStream.allReady rejects, the already-created renderedHtmlStream is abandoned without being cancelled before we fall through to renderSsrErrorDocumentShell. Not a correctness bug, but it leaves an un-consumed/un-cancelled React stream behind. Consider void renderedHtmlStream.cancel().catch(() => {}) in the catch before building the error shell.
| const shimBase = _reactServerShims.get(id); | ||
| if (shimBase !== undefined) { | ||
| return resolveShimModulePath(shimsDir, shimBase); | ||
| return resolveShimModulePath(shimsDir, stripJsExtension(shimBase)); |
There was a problem hiding this comment.
stripJsExtension(shimBase) is a no-op here: shimBase comes from _reactServerShims.get(id), whose values are bare base names ("navigation") that never carry a .js extension. Only the userland specifier (the map key) can be extensionful. Harmless, but the wrapping reads like it's load-bearing — the id-side handling (line 2608 / 2594) is where stripJsExtension actually matters.
|
Review posted successfully. Here's a summary of my review of PR #1908: Review SummaryThe PR carries two well-scoped changes: SSR shell-error recovery via the Verification I did:
Findings posted (all non-blocking):
No correctness blockers found. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the SSR shell-error recovery and the vinext/shims/* resolver fix. The core mechanism is sound — I independently verified the recovery flow (error shell HTML piped through the same createTickBufferedTransform so rscEmbed flight chunks + getInsertedHTML nav metadata land in the static DefaultGlobalError markup; no double bootstrap since buildHeadInjectionHtml only emits a <link rel=modulepreload> and the manual <script id="_R_"> is the lone executing entry; createRoot(document) is valid in React 19). The vinext/shims/* branch correctly resolves nested subpaths (e.g. internal/app-router-context) via path.posix.join, and _shimsDir is anchored to __dirname so it works in both source and published-package layouts. The prior /bigbonk rounds already captured the design notes (200/ISR-eligible recovery, dev-overlay parity, single-<style> assumption, no-op stripJsExtension(shimBase)); I won't repeat those.
Two observations below, both non-blocking.
Background ISR revalidation does not recover (new finding). The fallback is threaded into the initial-render path via renderAppPageHtmlStream (fallbackToErrorDocumentOnShellError: options.hasCustomGlobalError === false), but the background revalidation renderFn in app-page-dispatch.ts (the revalidatedSsrEntry.handleSsr(...) call around line 653) does NOT pass it. So for a route using the throw-to-opt-out-of-SSR pattern this PR targets: the first request recovers and caches a 200 recovery shell, but every background regeneration of that same route throws and fails to refresh the entry — an internal inconsistency between first render and revalidation. The /static-recovery spec exercises the prerender-time path (which threads the flag) and the first runtime hit, not this background-regen renderFn, so the gap is untested. Consider threading hasCustomGlobalError into that handleSsr call too (the dispatch already has options.hasCustomGlobalError), or documenting that revalidation deliberately keeps reject semantics.
| style.remove(); | ||
| } | ||
| startTransition(() => { | ||
| const clientRoot = createRoot(document, hydrateRootOptions); |
There was a problem hiding this comment.
hydrateRootOptions is built by createVinextHydrateRootOptions, which always includes formState (a hydrateRoot-only option). createRoot's RootOptions does not have formState — it's silently ignored at runtime, so this isn't a bug, but it's a semantic mismatch worth noting: in the recovery path there is no server-rendered form to hydrate, so the captured formState is meaningless here. The structural cast works only because HydrateRootOptions is a superset React tolerates. A short comment (e.g. "formState is inert for createRoot; we reuse the hydrate options object only for the error handlers") would prevent a future reader from assuming form-state replay happens on the recovery path.
|
Review posted successfully. Here's a summary of my review of PR #1908: Review SummaryThe PR carries two well-scoped changes: SSR shell-error recovery via the Verification I did independently:
Findings posted (both non-blocking):
I confirmed the prior |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-reviewed PR #1908. The core mechanism is sound and I independently confirmed the key pieces:
- The error-document shell is piped through the same
createTickBufferedTransform(rscEmbed, getInsertedHTML, ...)pipeline, so the flight payload + nav metadata land inDefaultGlobalError's static<head>. No double bootstrap:buildHeadInjectionHtmlemits only a<link rel=modulepreload>(app-ssr-entry.ts:280-283), andrenderToStaticMarkupemits no Fizz bootstrap, so the manual<script id="_R_">is the sole executing entry. DefaultGlobalErrorrenders exactly one<style dangerouslySetInnerHTML>(default-global-error.tsx:177), somarkErrorShellStyle's single-<style>String.replaceis correct today._reactServerShimsis keyed on userlandnext/*specifiers with bare-name values ("navigation"), so the newvinext/shims/*branch doesn't shadow any current react-server variant and correctly stops splitting singleton state.
Two findings below. The first is a real behavioral gap for the exact use case this PR targets (worth fixing here); the second is a minor dead-code note.
Background ISR revalidation does not recover, unlike the initial render. The new flag is threaded into dispatchAppPageInner's render lifecycle (the hasCustomGlobalError: options.hasCustomGlobalError you added below at line 987, which renderAppPageHtmlStream turns into fallbackToErrorDocumentOnShellError). But the background-revalidation handleSsr call in this same file (app-page-dispatch.ts:653, the revalidatedSsrEntry.handleSsr(...) with waitForAllReady: true at line 666) does not pass it. options.hasCustomGlobalError is in scope there too.
Consequence for a route using the throw-to-opt-out-of-SSR pattern this PR targets (no custom global-error.tsx): the first request recovers and caches a 200 recovery shell, but every background regeneration of that same route rejects in handleSsr, so the renderFn rejects and the stale entry is never refreshed — first render and revalidation diverge for the same route. The new spec exercises the prerender-time path and the first runtime hit, not this background renderFn, so the gap is untested. Consider threading fallbackToErrorDocumentOnShellError: options.hasCustomGlobalError === false into that handleSsr options object (next to waitForAllReady: true), or documenting that revalidation deliberately keeps reject semantics.
| return renderPageSpecialError(options, specialError); | ||
| }, | ||
| renderToReadableStream: options.renderToReadableStream, | ||
| hasCustomGlobalError: options.hasCustomGlobalError, |
There was a problem hiding this comment.
This threads the flag into the initial render lifecycle. The background-revalidation handleSsr call in this same file (~line 653, revalidatedSsrEntry.handleSsr(...) with waitForAllReady: true at line 666) does not get the equivalent fallbackToErrorDocumentOnShellError, so revalidation of a throw-to-opt-out route permanently rejects after the first render caches a 200 recovery shell. See the review summary for details — worth threading the flag there too or documenting the divergence.
| ? `${reactServerShim}.react-server` | ||
| : reactServerShim; | ||
| return resolveShimModulePath(_shimsDir, shimName); | ||
| return resolveShimModulePath(_shimsDir, stripJsExtension(shimName)); |
There was a problem hiding this comment.
Minor: stripJsExtension(shimName) is a no-op here. _reactServerShims values are bare base names ("navigation", see index.ts:646-650) and ${reactServerShim}.react-server likewise never carries .js — only the map key (the userland specifier) can be extensionful, and that's already normalized via cleanId before the .get(cleanId) lookup. The new vinext/shims/* branch at line 2594 is where stripJsExtension actually matters (the subpath can be extensionful). Consider dropping it here so it doesn't imply the value side needs normalization:
| return resolveShimModulePath(_shimsDir, stripJsExtension(shimName)); | |
| return resolveShimModulePath(_shimsDir, shimName); |
|
Review posted successfully. Here's a summary of my review of PR #1908: Review SummaryThe PR carries two well-scoped changes: SSR shell-error recovery via the Verification I did independently:
Findings posted:
No correctness blockers found for the happy path. Note: finding (1) was already raised in the prior |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-reviewed #1908 at 2a6292a. The two changes (SSR shell-error recovery via the __next_error__ document, and the vinext/shims/* resolver fix) are correct and well-tested. I independently re-verified the core mechanism and confirmed the prior /bigbonk findings are now addressed in the follow-up commits.
Verified independently:
- Stream ordering / no double bootstrap.
renderSsrErrorDocumentShellsplices its<script id="_R_" async>before</body></html>;createTickBufferedTransformthen strips</body></html>(stripDocumentCloseSuffix), emits the flight chunks viarscEmbed, and re-appends the close last. Final order is shell -> bootstrap -> flight -></body></html>, and theasyncmodule executes after the embedded RSC payload is in the DOM.buildHeadInjectionHtmlemits only a<link rel=modulepreload>(app-ssr-entry.ts:280), and the static-markup path runs no Fizz bootstrap, so<script id="_R_">is the sole executing entry. - Head injection point exists.
DefaultGlobalErrorrenders<head>...</head>and exactly one<style>(default-global-error.tsx:177), so the transform's</head>injection andmarkErrorShellStyle's single-<style>String.replaceare both valid.errorThemeCsscontains no<style>substring, so the replace can't mis-target. vinext/shims/*resolver. The new branch (index.ts:2591) codifies the previously-implicit resolution:vinext/shims/navigationalready resolved to the plainnavigation.tsbefore this PR (it was never intercepted by the resolveId hook and fell through node resolution to the source with no react-server condition), whilenext/navigationresolved tonavigation.react-server.ts. So this is the correct fix to stop splitting singleton state, with no behavior change for the current shim set._reactServerShimsis keyed onnext/*specifiers, so no shadowing.- Prior findings resolved. Background ISR regeneration now threads
fallbackToErrorDocumentOnShellError(app-page-dispatch.ts:667, commit 2a6292a); abandoned prerender stream is cancelled (app-ssr-entry.ts:629);formState/createRoot and single-style assumptions are documented.tests/app-page-dispatch.test.ts(17) pass.
Two small residual notes below, both non-blocking. No correctness blockers.
| // `<html id="__next_error__">`, so this branch also fires for legacy server- | ||
| // rendered global-error pages. That is upstream-compatible — the browser | ||
| // should always createRoot when the document is the default error shell. | ||
| if (document.documentElement.id === "__next_error__") { |
There was a problem hiding this comment.
Behavior-change worth confirming: on main, bootstrapHydration always used hydrateRootInTransition (hydration). This branch now switches to createRoot (full client re-render, no hydration) for any document whose documentElement.id === "__next_error__", which — as your comment notes — also includes the pre-existing server-rendered default-error page (digest error, no custom global-error.tsx). That path previously hydrated the server markup; it now discards and re-renders client-side.
This is plausibly upstream-compatible (Next.js client-renders the recovery shell), and the embedded flight payload encodes the same error tree so the rendered content matches. But the new browser spec only exercises the SSR-only-throw recovery path, not the legacy server-digest-error path through this same branch. Worth a quick confirmation (or a test) that a genuine server-side digest error with no custom global-error still renders the default error card correctly via createRoot rather than relying on the now-removed hydration path.
| if (cleanId.startsWith(vinextShimPrefix)) { | ||
| return resolveShimModulePath( | ||
| _shimsDir, | ||
| stripJsExtension(cleanId.slice(vinextShimPrefix.length)), |
There was a problem hiding this comment.
Minor robustness: cleanId here is only \0-stripped, not query-stripped (unlike line 2514's derivation, which also leaves the query). stripJsExtension won't strip a trailing query, so a specifier like vinext/shims/navigation?v=123 would resolve to a wrong base name. Internal package-subpath imports never carry a query, so this isn't reachable today — but if you want it to be defensive, run the subpath through stripViteModuleQuery before stripJsExtension.
|
Review posted successfully. Here's a summary of my review of PR #1908: Review SummaryThe PR carries two well-scoped changes: SSR shell-error recovery via the Verified independently:
Findings posted (both non-blocking):
No correctness blockers found. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Independent re-review of #1908. The two changes (SSR shell-error recovery via the __next_error__ document, and the vinext/shims/* resolver fix) are correct, well-scoped, and well-tested. I verified the core mechanism from scratch rather than relying on the prior rounds, and confirmed the previously-flagged actionable items are addressed.
Verified independently:
- Recovery stream pipeline.
renderSsrErrorDocumentShellproduces<!DOCTYPE html><html id="__next_error__"><head><style>…</style></head><body>…<script id="_R_" async></script></body></html>, then pipes through the samecreateTickBufferedTransform(rscEmbed, getInsertedHTML, …).DefaultGlobalError(default-global-error.tsx:175-201) renders the required</head>(transform flight-injection point at app-ssr-stream.ts:544) and exactly one<style>(line 177), so both the</head>injection andmarkErrorShellStyle's single-<style>replace are valid.errorThemeCsscontains no<style>substring, so the replace can't mis-target. The</body></html>close is stripped by the transform and re-emitted last (app-ssr-stream.ts:610), so final order is shell → bootstrap<script>→ flight → close. error: null⇒ client-error card. The shell renders withisServerError === false(no digest footer, "Reload to try again, or go back."), matching the spec's/pageassertions.vinext/shims/*resolver. Confirmed the resolveId filter (/(?:next\/|virtual:vinext-|…)/, index.ts:2507) matchesvinext/shims/*only becausevinext/contains the substringnext/— so the new branch is actually reachable. The published-package exports map./shims/*→./dist/shims/*.jshas no react-server condition, sovinext/shims/navigationalways resolves to the plain shim in every environment. The new source-checkout branch (no environment check) matches that exactly, and_reactServerShimsis keyed only onnext/*specifiers (index.ts:646-650), so no current variant is shadowed. This correctly stops splitting singleton state betweennext/navigation→navigation.react-server.tsand thevinext/shims/navigationcopy.- Prior actionable findings resolved. Background ISR revalidation now threads
fallbackToErrorDocumentOnShellError(app-page-dispatch.ts:667); abandoned prerender stream is cancelled (app-ssr-entry.ts:629);formState/createRootand single-<style>assumptions are documented. - Tests pass locally:
tests/shims.test.ts(subpath test),tests/app-page-dispatch.test.ts(17).
No correctness blockers. Two small non-blocking notes below.
| // one gains an environment-specific variant, handle it here before | ||
| // returning rather than relying on the userland `next/*` map below. | ||
| const vinextShimPrefix = "vinext/shims/"; | ||
| if (cleanId.startsWith(vinextShimPrefix)) { |
There was a problem hiding this comment.
The new vinext/shims/ branch is reachable only because the resolveId filter regex (/(?:next\/|…)/ at index.ts:2507) happens to match vinext/shims/* — vinext/ contains the substring next/. This is load-bearing but entirely implicit: if the filter is ever tightened to anchor next/ (e.g. ^next\/ or a word boundary), this branch silently stops firing and the singleton-state split this PR fixes would quietly regress, with no test catching it (the new shims test calls the handler directly, bypassing the filter).
Consider either (a) adding an explicit vinext\/shims\/ alternative to the filter regex so the match is intentional, or (b) a one-line comment here noting the branch depends on the incidental next/-substring match in the filter above.
| getSourceRoute(sourceRouteIndex) { | ||
| return routes[sourceRouteIndex]; | ||
| }, | ||
| hasCustomGlobalError: ${globalErrorVar ? `Boolean(${globalErrorVar}?.default)` : "false"}, |
There was a problem hiding this comment.
Boolean(${globalErrorVar}?.default) correctly treats a global-error file with no default export as "no custom global error" — so an app with a malformed global-error.tsx (file present, no default) would still get the __next_error__ recovery shell rather than the server-side boundary path. That's a reasonable choice, but worth confirming it matches Next.js, which I believe validates/throws on a global-error without a default export rather than silently falling back. Non-blocking, but a candidate for a follow-up parity check.
|
Review posted successfully. Here's a summary of my independent review of PR #1908: Review SummaryThe PR carries two well-scoped changes: SSR shell-error recovery via the Verification I did from scratch:
Findings posted (both non-blocking):
No correctness blockers found. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Independent review of #1908. The two changes — SSR shell-error recovery via the __next_error__ document, and the vinext/shims/* resolver fix — are correct and well-tested. I verified the core mechanism from scratch and ran the relevant suites locally (tests/shims.test.ts subpath test, tests/app-page-dispatch.test.ts 17 pass).
Verified independently:
- Flight payload survives the rejection.
rscEmbedis built from a separate stream (sideStream, or thes2tee branch at app-ssr-entry.ts:400), independent of the SSR render'sssrStream. So whenrenderToReadableStreamrejects, the recovery shell still carries the complete flight payload. The recovery is sound. - No double bootstrap.
buildHeadInjectionHtmlemits only a<link rel=modulepreload>(viabuildModulePreloadHtml, app-ssr-entry.ts:312), and the recovery shell's<script id="_R_" async>is the sole executing entry. The static-markup path runs no Fizz bootstrap. - Single
<style>marker is valid.DefaultGlobalErrorrenders exactly one<style dangerouslySetInnerHTML>(default-global-error.tsx:177); inlinestyle={...}attributes serialize asstyle="..."(with a space) and can't match the<style>substring, anderrorThemeCsscontains no<style>literal — somarkErrorShellStylecan't mis-target. error: null⇒ client-error card (error?.digest⇒isServerError === false), matching the spec assertions.- Background ISR revalidation now recovers (app-page-dispatch.ts:667 threads the flag) — the gap from prior rounds is resolved.
stripJsExtensionis load-bearing in the new branch (the subpath can be extensionful) — not dead code.
One finding worth confirming below (the resolveId filter narrowing), plus a minor parity note already raised in prior rounds. No correctness blockers.
| // re-imports from @vitejs/plugin-rsc. | ||
| filter: { | ||
| id: /(?:next\/|virtual:vinext-|^@vercel\/og(?:\.js)?$)/, | ||
| id: /(?:^next\/|^vinext\/shims\/|virtual:vinext-|^@vercel\/og(?:\.js)?$)/, |
There was a problem hiding this comment.
This change does two things: it adds ^vinext\/shims\/ (intended), and it anchors the next/ alternative from unanchored next\/ to ^next\/ (a behavioral narrowing that comes along for the ride). The new tests/shims.test.ts case asserts unrelated-next/navigation no longer matches, so the anchoring is deliberate — but it interacts with this hook's own \0-prefix handling in a way worth confirming.
The handler strips a leading \0 (line 2515) precisely because the comment says @vitejs/plugin-rsc's generated browser entry re-imports modules with the already-resolved \0-prefixed id. With the new anchored filter, a \0-prefixed next/*, vinext/shims/*, or @vercel/og specifier no longer passes the filter, so the handler never runs for it:
\0virtual:vinext-app-browser-entry → matches (virtual:vinext- is unanchored)
\0next/navigation → NO LONGER matches
\0vinext/shims/navigation → NO LONGER matches
\0@vercel/og → NO LONGER matches
If the \0 re-import path only ever applies to virtual:vinext-* modules (which still match), this is safe and the \0-strip for the other branches is just dead defense. But if plugin-rsc can re-import a next/* or @vercel/og shim with a \0 prefix, this silently stops resolving it and would split singleton state — the exact failure mode this PR is trying to fix. Worth a quick confirmation that the \0 case is virtual-module-only, and/or anchoring with the \0 allowed (e.g. /(?:^\0?next\/|^\0?vinext\/shims\/|...)/) so the handler's \0-strip stays reachable for all branches it handles.
| getSourceRoute(sourceRouteIndex) { | ||
| return routes[sourceRouteIndex]; | ||
| }, | ||
| hasCustomGlobalError: ${globalErrorVar ? `Boolean(${globalErrorVar}?.default)` : "false"}, |
There was a problem hiding this comment.
Boolean(${globalErrorVar}?.default) treats a global-error.tsx that exists but has no default export as "no custom global error", so a malformed global-error file would silently get the __next_error__ recovery shell instead of erroring. That's a reasonable default, but Next.js validates app-router special files and throws on a missing default export rather than silently falling back. Non-blocking — worth a follow-up parity check (also flagged in a prior round).
|
Review posted successfully. Here's a summary of my independent review of PR #1908: Review SummaryThe PR carries two well-scoped changes: SSR shell-error recovery via the Verified independently:
New finding (worth confirming, non-blocking): Minor parity note: No correctness blockers found. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Independent review of #1908. The core mechanism (SSR shell-error recovery via the __next_error__ document, plus the vinext/shims/* resolver fix) is correct and well-tested. I verified the recovery stream pipeline (flight payload from a separate stream survives the rejection; buildHeadInjectionHtml emits only a <link rel=modulepreload> so there is no double bootstrap; DefaultGlobalError renders exactly one <style> and a </head>, so markErrorShellStyle and the </head> flight injection are both valid; errorThemeCss has no <style> substring), and confirmed the prior-round gaps are resolved (background ISR revalidation threads the flag at app-page-dispatch.ts:667; abandoned prerender stream is cancelled). Ran tests/app-page-dispatch.test.ts + tests/shims.test.ts (1067 pass).
One new behavioral finding worth confirming (the unconditional-throw-with-no-boundary recovery path), plus a factual correction to a recurring claim from prior /bigbonk rounds about the filter regex. Neither is a hard blocker, but the first is a genuine regression surface the new spec does not cover.
Correction to prior rounds (re: the filter regex): several prior reviews stated this PR "anchors next/ -> ^next/" and thereby narrows the filter. That is not what changed. next\/ is unanchored in both the old and new regex. The actual diff is (a) adding vinext\/shims\/ (intended), and (b) removing the ^ anchor from @vercel/og (^@vercel\/og(?:\.js)?$ -> @vercel\/og(?:\.js)?$). That broadens the filter so a \0@vercel/og re-import now passes (it did not before) and reaches the handler's isVercelOgImport branch — which the new shims test asserts (hook.filter.id.test("\0@vercel/og") is now true). So the \0-strip concern raised in the last round is the opposite of the real change: the filter got more permissive, not less. No action needed on the regex; just flagging so the record is accurate.
| // Legacy server-rendered default-error pages also use `__next_error__`, but | ||
| // keep their existing hydration path so the serialized server error digest | ||
| // remains available to DefaultGlobalError. | ||
| if (document.documentElement.id === "__next_error__" && errorShellStyles.length > 0) { |
There was a problem hiding this comment.
Behavioral regression worth confirming: unconditional client throw + no boundary + no custom global-error now leaves a blank page.
This createRoot recovery path runs on the initial bootstrap, where pendingNavigationRecoveryHref is still null (it's only set during an in-flight navigation, app-browser-entry.ts:304/573). So the prod onUncaughtError (createOnUncaughtError(() => pendingNavigationRecoveryHref), line 1593) returns early without navigating — it only console.errors (app-browser-error.ts:24-27).
Consider a client component that throws on both server and client (not the SSR-only typeof window === undefined pattern), with no local error.tsx and no custom global-error.tsx:
- On
main: the SSR shell rejects,handleSsrrejects, andrenderAppPageHtmlStreamWithRecoveryfalls torenderErrorBoundaryResponse(app-page-stream.ts:366), serving the server-rendered default error card which then hydrates and stays visible. - With this PR: the recovery flag is set (no custom global-error), so
handleSsrresolves with the error-document shell. The browser enters thiscreateRootbranch, tears down the error card, re-renders — the component throws again,onUncaughtErrorhas no recovery href, so the user is left on a blank, torn-down document instead of the error card.
The new spec only exercises the SSR-only throw (recovers) and the local-error.tsx cases (boundary catches). The unconditional-throw-no-boundary case is the one that regresses, and it's untested. Worth either (a) confirming this matches Next.js shell-error semantics for a genuinely always-failing component, or (b) adding a fixture route that throws unconditionally with no boundary and asserting the resulting UX, so the regression (if it is one) is caught.
There was a problem hiding this comment.
Checked this on the current PR head and it does not leave a blank page. The shell-recovery path already lands on the default This page couldn’t load card when there is no local boundary. I pushed a2f481b9 to lock that behavior in with a dedicated no-boundary regression case in ssr-error-shell-recovery.browser.spec.ts, then re-ran PLAYWRIGHT_PROJECT=app-router-chrome-browser-specific pnpm run test:e2e -- tests/e2e/app-router/nextjs-compat/ssr-error-shell-recovery.browser.spec.ts.
| // re-imports from @vitejs/plugin-rsc. | ||
| filter: { | ||
| id: /(?:next\/|virtual:vinext-|^@vercel\/og(?:\.js)?$)/, | ||
| id: /(?:next\/|vinext\/shims\/|virtual:vinext-|@vercel\/og(?:\.js)?$)/, |
There was a problem hiding this comment.
The meaningful change here besides adding vinext\/shims\/ is dropping the ^ anchor from the @vercel/og alternative (^@vercel\/og(?:\.js)?$ -> @vercel\/og(?:\.js)?$). That's a deliberate broadening so \0@vercel/og re-imports pass the filter (the new shims test asserts this), and it's correct — just flagging because prior review rounds described this diff as narrowing next/, which it does not. A one-line note in the filter comment that the @vercel/og alternative is intentionally unanchored (to admit \0-prefixed re-imports) would prevent someone re-adding ^ and silently breaking \0@vercel/og resolution.
|
Review posted successfully. Here's a summary of my independent review of PR #1908: Review SummaryThe two changes — SSR shell-error recovery via the Verified independently:
Findings posted (2, both non-blocking):
No correctness blockers found. |
An unconditional client throw during SSR shell recovery without a local error boundary must still land on the default global-error card. Without a regression test, a future change could tear down the recovery shell and leave a blank document.\n\nAdd a production browser case that exercises the no-boundary route and asserts the default error UI after the client re-render throws again.
2 participants
What this changes
Scoped down from the original next/dynamic CSS PR after review (see comments): this branch now carries only the SSR shell-error recovery and the shim resolver fix, as two clean commits. The dynamic-CSS ordering mechanism was removed and will return redesigned (follow-up issue linked below).
digest), and the app has no customglobal-error.tsx,handleSsrserves the default__next_error__error document carrying the original flight payload and bootstrap module. The browser detects the marker and re-renders the real tree withcreateRootinstead of hydrating — Next.js shell-error semantics. Localerror.tsxboundaries still win: they ship inside the flight payload and catch re-thrown errors client-side.vinext/shims/*resolver fix. Package-subpath shim imports resolve to the same local shim files asnext/*aliases in source checkouts, so request-scoped singleton state is not split between module copies. The recovery fixture exercises this.Why
Previously a failed SSR shell rendered a server-side global-error page whose flight payload encodes the error tree. Without a custom
global-error.tsxthat meant the default error card with no path back to the real page — an SSR-phase-only client throw (the "throw to opt out of server rendering" pattern, used by the Next.jsnext-dynamic-cssfixture) left the browser permanently on the card even though the client render would succeed.Approach
fallbackToErrorDocumentOnShellErroris threaded from the generated entry (which knowshasCustomGlobalErrorat build time) through dispatch/render options intohandleSsr. Rejection semantics are preserved whenever:digest(flight errors,redirect()/notFound(), server-component throws) — the existing rejection-driven boundary machinery inrenderAppPageHtmlStreamWithRecoveryis untouched, orglobal-error.tsx— the server-rendered boundary re-render path is kept.Non-goals: dynamic CSS ordering (removed from this PR; follow-up issue), changing custom-global-error apps' behavior in any way.
Upstream References
Validation
tests/e2e/app-router/nextjs-compat/ssr-error-shell-recovery.browser.spec.ts: recovery to real content for an SSR-only throw without a boundary; localerror.tsxsemantics for SSR-only (content renders, boundary absent) and unconditional (boundary renders) client throws; strict console-error checking apart from the intentional throws.tests/nextjs-compat/global-error.test.ts(24 tests): all local/nested/global boundary semantics unchanged.tests/entry-templates.test.ts,tests/build-optimization.test.ts,tests/deploy.test.tsgreen.Risks / follow-ups
global-error.tsxand an SSR-only client throw still get the server-rendered global-error (vinext's existing divergence from Next.js, which always serves the recovery shell). Deliberately unchanged here.