Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
180 changes: 174 additions & 6 deletions packages/vinext/src/server/app-browser-entry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -801,10 +801,84 @@ function restorePopstateScrollPosition(state: unknown): void {
});
}

async function readInitialRscStream(): Promise<ReadableStream<Uint8Array>> {
// Set on pagehide so the RSC navigation catch block can distinguish expected
// fetch aborts (triggered by the unload itself) from real errors worth logging.
let isPageUnloading = false;

const RSC_RELOAD_KEY = "__vinext_rsc_initial_reload__";

// sessionStorage can throw SecurityError in strict-mode iframes, storage-
// disabled browsers, and some Safari private-browsing configurations. Wrap
// every access so a recovery path for one error does not crash hydration.
function readReloadFlag(): string | null {
try {
return sessionStorage.getItem(RSC_RELOAD_KEY);
} catch {
return null;
}
}
function writeReloadFlag(path: string): void {
try {
sessionStorage.setItem(RSC_RELOAD_KEY, path);
} catch {}
}
function clearReloadFlag(): void {
try {
sessionStorage.removeItem(RSC_RELOAD_KEY);
} catch {}
}

// A non-ok or wrong-content-type RSC response during initial hydration means
// the server cannot deliver a valid RSC payload for this URL. Parsing the
// response as RSC causes an opaque parse failure. On the first attempt,
// reload once so the server has a chance to render the correct error page
// as HTML. On the second attempt (detected via the sessionStorage flag), the
// endpoint is persistently broken. Returns null so main() aborts the
// hydration bootstrap without registering `__VINEXT_RSC_*` globals —
// including during the brief window between reload() firing and the page
// actually unloading — so external probes never see a half-hydrated page.
function recoverFromBadInitialRscResponse(reason: string): null {
const currentPath = window.location.pathname + window.location.search;
if (readReloadFlag() === currentPath) {
clearReloadFlag();
console.error(
`[vinext] Initial RSC fetch ${reason} after reload; aborting hydration. ` +
"Server-rendered HTML remains visible; client components will not hydrate.",
);
return null;
}
writeReloadFlag(currentPath);
// Verify the write persisted. In storage-denied environments (strict-mode
// iframes, locked-down enterprise policies), every getItem returns null and
// every setItem silently no-ops, so the reload-loop guard cannot survive
// the reload — the page would loop forever. Abort instead so the user at
// least sees the server-rendered HTML.
if (readReloadFlag() !== currentPath) {
console.error(
`[vinext] Initial RSC fetch ${reason}; sessionStorage unavailable so the ` +
"reload-loop guard cannot persist — aborting hydration. " +
"Server-rendered HTML remains visible; client components will not hydrate.",
);
return null;
}
// One-shot diagnostic so a production reload is traceable. Only fires once
// per broken path thanks to the sessionStorage flag above; not noisy.
console.warn(
`[vinext] Initial RSC fetch ${reason}; reloading once to let the server render the HTML error page`,
);
window.location.reload();
return null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (not a blocker, and earlier reviews touched this): the first-attempt branch schedules window.location.reload() then returns null. The comment on the function header (lines 836-839) describes the null return as the mechanism that prevents the half-hydrated-globals invariant violation. That invariant holds only because main() happens to be synchronous after await readInitialRscStream() — there's no microtask yield between the null check at line 1025 and the global-assign lines below. If a future refactor interposes any async work (e.g. an await for React preloading) between those two points, the first-attempt branch starts leaking globals during the pre-unload window.

One way to pin the invariant at the call site is to hoist the if (rscStream === null) return; check to immediately after await readInitialRscStream() (it already is — good) and add a comment there referencing the synchronous-tail requirement. Or, more robustly, push the global-assignment into a helper that the null-branch cannot reach.

Fine as-is; flagging because the invariant is load-bearing and currently only enforced by code-placement convention.

}

async function readInitialRscStream(): Promise<ReadableStream<Uint8Array> | null> {
const vinext = getVinextBrowserGlobal();

if (vinext.__VINEXT_RSC__ || vinext.__VINEXT_RSC_CHUNKS__ || vinext.__VINEXT_RSC_DONE__) {
// Reaching the embedded-RSC branch means the server successfully rendered
// the page — any prior reload flag for this path is stale and must be
// cleared so a future failure gets its own fresh recovery attempt.
clearReloadFlag();

if (vinext.__VINEXT_RSC__) {
const embedData = vinext.__VINEXT_RSC__;
delete vinext.__VINEXT_RSC__;
Expand Down Expand Up @@ -841,6 +915,29 @@ async function readInitialRscStream(): Promise<ReadableStream<Uint8Array>> {

const rscResponse = await fetch(toRscUrl(window.location.pathname + window.location.search));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observation (non-blocking, outside the diff's scope): fetch(toRscUrl(...)) on line 916 can reject on a true network error (DNS failure, offline, connection reset before any response bytes). That rejection propagates out of readInitialRscStream, out of main, and surfaces as an unhandled promise rejection since void main() has no .catch. The non-ok path now has a clean recovery, but a rejection is still a bare red console error + window.onerror / error-reporting hooks firing.

One-line fix (consider as a follow-up, not for this PR):

void main().catch((err) => {
  console.error("[vinext] hydration aborted:", err);
});

Not in this PR's scope, but now that every documented response shape has a structured recovery, the network-error path is the last remaining case that produces the same user-visible symptom this PR was added to eliminate.


if (!rscResponse.ok) {
return recoverFromBadInitialRscResponse(`returned ${rscResponse.status}`);
}
// Guard against proxies/CDNs that return 200 with a rewritten Content-Type
// (e.g. text/html instead of text/x-component). Such responses cannot be
// parsed as RSC and would throw the same opaque parse error this fallback
// exists to prevent.
const contentType = rscResponse.headers.get("content-type") ?? "";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parity nit (non-blocking): the client-side nav path at line 1212 guards on all three conditions Next.js checks — !navResponse.ok || !isRscResponse || !navResponse.body. The initial-hydration path only checks !ok and content-type; there is no !rscResponse.body branch.

The !body case is rare in practice (a 200 with valid text/x-component content-type but a null body) but it is specifically called out in Next.js' guard and is the cheapest possible addition here for exact parity:

Suggested change
const contentType = rscResponse.headers.get("content-type") ?? "";
if (!rscResponse.ok) {
return recoverFromBadInitialRscResponse(`returned ${rscResponse.status}`);
}
// Guard against proxies/CDNs that return 200 with a rewritten Content-Type
// (e.g. text/html instead of text/x-component). Such responses cannot be
// parsed as RSC and would throw the same opaque parse error this fallback
// exists to prevent.
const contentType = rscResponse.headers.get("content-type") ?? "";
if (!contentType.startsWith("text/x-component")) {
return recoverFromBadInitialRscResponse(
`returned non-RSC content-type "${contentType || "(missing)"}"`,
);
}
// Missing body (e.g. 204 No Content, or an edge worker that returned ok
// headers without piping the stream) would fail the same way downstream.
// Matches Next.js' `!res.body` branch in fetch-server-response.ts.
if (!rscResponse.body) {
return recoverFromBadInitialRscResponse("returned empty body");
}

if (!contentType.startsWith("text/x-component")) {
return recoverFromBadInitialRscResponse(
`returned non-RSC content-type "${contentType || "(missing)"}"`,
);
}
// Missing body (e.g. 204 No Content, or an edge worker that returned ok
// headers without piping the stream) fails the same way downstream.
// Matches Next.js' `!res.body` branch in fetch-server-response.ts.
if (!rscResponse.body) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code follow-up (non-blocking): the pre-existing if (!rscResponse.body) throw new Error("[vinext] Initial RSC response had no body") that used to live a few lines below this branch was removed in this PR (the throw is gone in the diff), but worth a quick scan of the file to confirm no other stale check references the old invariant. The new guard here converts the throw into a structured recovery path, which is the right shape.

return recoverFromBadInitialRscResponse("returned empty body");
}
// Successful RSC response clears the guard so a subsequent reload of the
// same path after a transient failure still gets one recovery attempt.
clearReloadFlag();

let params: Record<string, string | string[]> = {};
const paramsHeader = rscResponse.headers.get("X-Vinext-Params");
if (paramsHeader) {
Expand All @@ -854,10 +951,6 @@ async function readInitialRscStream(): Promise<ReadableStream<Uint8Array>> {

restoreHydrationNavigationContext(window.location.pathname, window.location.search, params);

if (!rscResponse.body) {
throw new Error("[vinext] Initial RSC response had no body");
}

return rscResponse.body;
}

Expand Down Expand Up @@ -939,6 +1032,16 @@ async function main(): Promise<void> {
registerServerActionCallback();

const rscStream = await readInitialRscStream();
// null signals that readInitialRscStream aborted hydration — either because
// a reload is in flight (first-attempt recovery) or the endpoint is
// persistently broken (post-reload). Bootstrap is a separate synchronous
// helper so the null-branch structurally cannot reach any __VINEXT_RSC_*
// global assignment, even if a future refactor interposes async work here.
if (rscStream === null) return;
bootstrapHydration(rscStream);
}

function bootstrapHydration(rscStream: ReadableStream<Uint8Array>): void {
const root = normalizeAppElementsPromise(createFromReadableStream<AppWireElements>(rscStream));
const initialNavigationSnapshot = createClientNavigationRenderSnapshot(
window.location.href,
Expand Down Expand Up @@ -1100,6 +1203,55 @@ async function main(): Promise<void> {

if (navId !== activeNavigationId) return;

// Any response that isn't a valid RSC payload (non-ok status,
// missing/rewritten Content-Type, or missing body) means the server
// returned something we cannot parse — typically an HTML error page
// or a proxy-rewritten response. Parsing such a body as an RSC stream
// throws a cryptic "Connection closed" error. Match Next.js behavior
// (fetch-server-response.ts:211, `!isFlightResponse || !res.ok || !res.body`):
// hard-navigate to the response URL so the server can render the correct
// error page as HTML. The outer finally handles
// settlePendingBrowserRouterState and clearPendingPathname on this
// return path.
//
// Prefer the post-redirect response URL over `currentHref`: on a
// redirect chain like `/old` → 307 → `/new` → 500, the browser's
// fetch already followed the redirect, so `navResponse.url` is the
// failing `/new` destination. Hard-navigating there directly avoids
// bouncing off `/old` just to re-follow the same 307, which would
// flash the wrong URL in the address bar and mis-key analytics.
// Matches Next.js' `doMpaNavigation(responseUrl.toString())`. Falls
// back to `currentHref` when no response URL is available.
const navContentType = navResponse.headers.get("content-type") ?? "";
const isRscResponse = navContentType.startsWith("text/x-component");
if (!navResponse.ok || !isRscResponse || !navResponse.body) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: the !navResponse.body branch here is reachable after await fetch(...) has resolved, at which point Response.body is a ReadableStream | null but is null only for responses with no body (e.g. 204, or a HEAD request). For an actual HTTP 200/500 with an empty body, body is a non-null empty stream, not null. The practical impact is that an edge worker that returns ok headers with a 0-byte body would still be fed to createFromFetch and throw downstream — matching the behavior Next.js' !res.body check is supposed to prevent.

Not a blocker — Next.js' guard has the same theoretical gap, and the content-type check catches most of these in practice. Noting only for future reference.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor invariant observation (non-blocking): this guard fires after if (navId !== activeNavigationId) return; on line 1191, but the value of currentHref at that point reflects any server-side redirects that happened on earlier iterations of this while (true) loop (the redirect branch at line 1246-1258 reassigns it). So the fallback hardNavTarget = currentHref on line 1216 — used when responseUrl is falsy (empty string) — would point to the final destination of the redirect chain rather than the user's original clicked href. That is the right behavior for the common case of same-loop-iteration, but worth noting that currentHref is not the user's original clicked href on redirect chains: it tracks the latest history entry.

In combination with the responseUrl preference on line 1215, this is correct today. Flagging only because the invariant (currentHref kept in sync with history across redirect hops, so hard-nav on !ok is safe) is load-bearing and currently only maintained by code-placement convention in the redirect branch on line 1253. A future refactor that moves redirect handling into a helper but forgets to reassign currentHref in the caller would silently misroute users on the error path.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor invariant note (non-blocking): the !navResponse.body branch fires after await fetch(...) resolves, at which point Response.body is null only for responses explicitly without a body (e.g. 204 No Content, HEAD, or an edge worker that closed the stream before piping). A genuine HTTP 200/500 with a zero-byte body still produces a non-null empty ReadableStream, so this branch won't catch that case — it will be fed to createFromFetch and throw downstream.

Matches Next.js' same theoretical gap in fetch-server-response.ts, and the content-type check catches most of these in practice. Not worth changing; noting only for future reference if someone investigates why a 0-byte RSC response still produces an opaque parse error.

const responseUrl = navResponseUrl ?? navResponse.url;
let hardNavTarget = currentHref;
if (responseUrl) {
const parsed = new URL(responseUrl, window.location.origin);
const origUrl = new URL(currentHref, window.location.origin);
let pathname = parsed.pathname.replace(/\.rsc$/, "");
// toRscUrl strips trailing slash before appending .rsc, so the
// response URL loses it on the round-trip. Restore it when the
// original href had one so sites with trailingSlash:true don't
// incur an extra 308 to the canonical form on the error path.
if (
origUrl.pathname.length > 1 &&
origUrl.pathname.endsWith("/") &&
!pathname.endsWith("/")
) {
pathname += "/";
}
hardNavTarget = pathname + parsed.search;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hash fragment is dropped on hard-nav: hardNavTarget = pathname + parsed.search builds the target from the response URL, which — for a .rsc fetch — does not carry the browser's hash fragment. If the user clicked a <Link href="/foo#section"> that 500s, the hard-nav lands on /foo and silently loses the #section anchor.

Next.js' doMpaNavigation(responseUrl.toString()) has the same limitation (the response URL never carries a fragment), so this is parity with upstream — not a regression. Worth preserving the fragment from currentHref as a small improvement over Next.js, or at least a code comment flagging the divergence between the user's clicked href and the hard-nav target:

Suggested change
hardNavTarget = pathname + parsed.search;
hardNavTarget = pathname + parsed.search;
// Preserve the hash from the user's clicked href — the response
// URL from a .rsc fetch never carries a fragment, so dropping it
// here would silently strip `/foo#section` to `/foo`. Next.js'
// doMpaNavigation has the same limitation; this is a small
// ergonomic win over upstream.
const origHash = new URL(currentHref, window.location.origin).hash;
if (origHash) hardNavTarget += origHash;

// Preserve the hash from the user's clicked href — a .rsc response
// URL never carries a fragment, so dropping it would silently strip
// `/foo#section` down to `/foo`.
if (origUrl.hash) hardNavTarget += origUrl.hash;
}
window.location.href = hardNavTarget;
return;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parity note (non-blocking): Next.js' equivalent guard (fetch-server-response.ts in canary) is !isFlightResponse || !res.ok || !res.body. This PR only covers !res.ok.

The !res.body case is mostly theoretical, but !isFlightResponse covers a real scenario: a 200 response from a proxy/CDN that stripped or rewrote the Content-Type header (e.g. returned text/html instead of text/x-component). That would still be passed to createFromFetch and throw the exact stream-parse error this PR was added to fix.

Worth a follow-up to also check the content type before parsing:

const ct = navResponse.headers.get("content-type") ?? "";
const isFlight = ct.startsWith("text/x-component");
if (!navResponse.ok || !isFlight || !navResponse.body) {
  window.location.href = currentHref;
  return;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: currentHref is the pre-redirect URL captured at navigation start. If a server-side redirect has already updated history via replaceHistoryStateWithoutNotify earlier in this while (true) loop (the redirect branch at ~L1194-1210), the redirect loop writes history to the new path but the outer currentHref variable has also been updated on L1205, so this should be correct. Flagging it only so a future refactor that breaks the redirect-loop invariant (e.g. not updating currentHref after a redirect) doesn't silently send users to the wrong hard-nav destination. A comment pinning the invariant ("currentHref is kept in sync with history across redirect hops above, so hard-nav on !ok is safe") would help.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test coverage gap (non-blocking): the new redirect-aware hard-nav logic — preferring navResponseUrl ?? navResponse.url over currentHref so a chain like /old → 307 → /new → 500 hard-navs to /new rather than /old — has no test in rsc-fetch-errors.spec.ts. Both existing tests exercise direct 404/500 responses without redirect hops, so the responseUrl branch and the original-vs-response URL divergence it guards against are only covered by code review.

Worth adding a test with a Playwright route intercept that returns 307 Location to a second intercepted 500 endpoint, asserting the final URL is the 307 target and the address bar never flashes the source URL. Fine to defer as a follow-up; flagging because this was called out in the last review round as the scenario motivating the switch from currentHref.


const finalUrl = new URL(navResponseUrl ?? navResponse.url, window.location.origin);
const requestedUrl = new URL(rscUrl, window.location.origin);

Expand Down Expand Up @@ -1200,7 +1352,13 @@ async function main(): Promise<void> {
// Don't hard-navigate to a stale URL if this navigation was superseded by
// a newer one — the newer navigation is already in flight and would be clobbered.
if (navId !== activeNavigationId) return;
console.error("[vinext] RSC navigation error:", error);
// Suppress the diagnostic when the page is unloading: a hard-nav or anchor
// click tears down the document and aborts any in-flight RSC fetch, which
// surfaces here as an error. The page is already going away, so the log
// is just noise. Mirrors Next.js' isPageUnloading pattern.
if (!isPageUnloading) {
console.error("[vinext] RSC navigation error:", error);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking parity observation: isPageUnloading is set globally on pagehide, so this suppression applies to any error surfacing through the outer catch during the unload window — not just fetch aborts triggered by the unload itself. A racing RSC fetch that genuinely fails (server crash, stream-parse error) and happens to complete between pagehide and the document actually tearing down will now be silently swallowed. In practice the page is seconds from being gone, and Next.js has identical behavior, so this is parity — not a regression.

Worth flagging only because this slightly broadens the suppression surface vs the pre-fix behavior where every such error was logged. Noting for future debugging — if a user reports "my server threw but I see nothing in the console when I click a link", this is the code path that suppresses it.

}
window.location.href = currentHref;
} finally {
// Single settlement site: covers normal return, early returns on stale-id
Expand Down Expand Up @@ -1282,5 +1440,15 @@ async function main(): Promise<void> {
}

if (typeof document !== "undefined") {
window.addEventListener("pagehide", () => {
isPageUnloading = true;
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Substantive: isPageUnloading is set on pagehide but never reset on pageshow. When the browser restores this document from bfcache (back/forward cache), the JS execution context is resumed with isPageUnloading === true, and the outer navigation catch at line 1317 will silently drop every subsequent error — real errors, not just unload-aborts. Next.js explicitly pairs the two handlers for this exact reason (fetch-server-response.ts, canary):

window.addEventListener('pagehide', () => { isPageUnloading = true })
window.addEventListener('pageshow', () => { isPageUnloading = false })

vinext also documents the pattern on line 1316 as "Mirrors Next.js' isPageUnloading pattern", so the divergence is not intentional. Fix:

Suggested change
});
window.addEventListener("pagehide", () => {
isPageUnloading = true;
});
// Reset on pageshow so a bfcache-restored document does not resume with
// the flag stuck at true, which would silently swallow every subsequent
// RSC navigation error for the lifetime of that tab. Matches Next.js'
// fetch-server-response.ts handler pair.
window.addEventListener("pageshow", () => {
isPageUnloading = false;
});
void main();

User-visible symptom without this: after a user navigates away from a vinext app and hits Back, any genuine RSC navigation failure (stream-parse error, 500 from backend, etc.) produces a silent hard-nav with no [vinext] RSC navigation error: diagnostic in the console, making production debugging significantly harder.

// Reset on pageshow so a bfcache-restored document does not resume with
// the flag stuck at true, which would silently swallow every subsequent
// RSC navigation error for the lifetime of that tab. Matches Next.js'
// fetch-server-response.ts handler pair.
window.addEventListener("pageshow", () => {
isPageUnloading = false;
});
void main();
}
Loading
Loading