fix: render error boundaries inside their layout chain - #1413
Conversation
A boundary render was handed straight to wrapInDocument with no layouts, so the response carried none of the keyed wj:children comments the layout wrapping emits. The client router scanned both DOMs, found no shared boundary, and hard-navigated. Every navigation into a throwing page threw away the SPA's scroll, its hydrated state, and the site chrome the user needed to navigate back out with, and the boundary page itself arrived with no nav on it at all. Boundaries now render in the page's position inside the layouts at and above their own segment, through the same emitter the happy path uses, so the two cannot drift apart. Sharing the loop is the point: a mismatched pair degrades the swap just as a missing one does, which would look fixed in a diff and still hard-load. A boundary sits inside its own segment's layout and so cannot catch it. A throwing layout falls through to the next boundary out, which the existing bounded loop already does, and each step outward wraps a strictly smaller set, so it converges. global-error stays unwrapped: it owns its document, wrapping it would re-run what just threw, and it ships no boot script. The boot set is now what actually rendered. It used to carry the page module and every layout, including ones deeper than the boundary, and omit the boundary itself, so a component the boundary rendered never upgraded.
The dev-overlay suite never clicked through to /crash: it reached the page with a plain goto, or only prefetched it. That gap was silent, and it was silent because the navigation could not work, so there was nothing to assert. Its fixture gains the error.ts it never had, nested one segment down so there is a layout above the boundary that has to survive.
The #963 case asserted the import-only page's frontier component was emitted on the error path. That followed from the old boot set, which carried the page module; the page threw, so the element that component upgrades is not in the boundary's DOM and registering it bought nothing. The property #963 exists for is unchanged and now stronger: the page's module cannot reach the error boot by any route.
|
Decision: the boundary boot set is what rendered, and one #963 assertion had to move with it The boot set on a boundary response used to be That collided with one existing assertion in I took the new behaviour as correct rather than restoring the old set. The page threw, so its So I retargeted the case rather than deleting it. It now locks the tighter contract, with the reasoning in the test. One thing I want on the record because it cost real time and will cost it again. The first run of the new e2e failed, and it was not a bug: in a linked worktree every bare |
Three defects in the boundary wrapping, all found in review. The page region was emitted whenever its segment differed from the innermost WRAPPED layout, so a boundary sitting above the route's innermost layout emitted that layout's own id around content the layout was not in. The client then advertised the id in X-Webjs-Have, the next navigation into that subtree short-circuited on it, and the server returned a fragment assuming chrome the page never had. The user landed on the next page with the layout missing. The region is now skipped whenever its segment belongs to ANY layout of the route, so the emitted ids are a subset of the happy path's rather than a different set. The 403 / 401 / 404 responses rendered the layout chain but still shipped an empty boot set, so the chrome painted and never hydrated: a theme toggle that does nothing, and every link out a full page load. Chrome that looks live and is not is worse than the no-chrome page this replaced, so those paths now build the same boot set the 500 path does. Every boundary kind is already a browserEntryFiles entry, so the urls are servable. boundarySegmentPath matched `not-found` INSIDE `global-not-found`, deriving the segment `/global-`. Benign only by accident, since the root layout is the sole ancestor of that too. The match is anchored to a separator now and the global forms are listed first.
vivek7405
left a comment
There was a problem hiding this comment.
I went over the boundary wrapping, the boot set, the X-Webjs-Have contract on both sides, and the doc surfaces.
The core idea holds up: one emitter shared with the happy path is the right call, and the throwing-layout fallthrough falling out of the existing bounded loop rather than needing failure-point tracking is the part I'd have most expected to get wrong.
Where it goes wrong is the page region. Emitting it under an id that belongs to a layout the boundary deliberately did NOT render is a lie the client believes and then acts on two navigations later, which is the worst shape a bug like this can take: the boundary response itself looks correct, and the damage shows up on the page after it. The fix is to skip the region whenever its segment belongs to any layout of the route, so the emitted set is a subset of the happy path's rather than a different set.
The other one I'd flag as a design consequence worth watching: rendering chrome on the 403/401/404 responses without also booting it produced pages that look interactive and are not, which is a worse failure than the chrome-less page it replaced. Worth remembering that adding markup to a response is also a commitment to hydrate it.
Two throwaway probe apps landed in 170310a. They sit outside every workspaces glob, so nothing referenced them and nothing failed, which is how they would have reached main unnoticed. The fixture they duplicate is built properly by makeBoundaryApp in test/ssr/ssr.test.js.
vivek7405
left a comment
There was a problem hiding this comment.
Second read, scoped to the fix commit and its blast radius.
The two logic changes hold. I checked the emitted-id set against the client swap plan and the have-header build for the excluded-layout, deeper-layout, dynamic-param and route-group shapes, and confirmed every boundary kind is already a browserEntryFiles entry, so the urls the 403 / 401 / 404 paths newly boot are servable through the auth gate.
Two problems, neither in the runtime change. Both are path-level rather than inline, since one is about files that no longer exist at this head and the other is about the PR body.
-
.vfy-1VDqpT/and.vfy-4S3NJQ/: two verification scratch apps were committed by the fix commit. Nothing generated, referenced or ignored them, and they sat outside everyworkspacesglob, so nothing failed and they would have reachedmainunnoticed. They duplicated a fixturemakeBoundaryAppalready builds properly. -
The PR body described the page-region skip rule as reading against the innermost WRAPPED layout and keeping the emitted set identical to the happy path. That is the rule the fix commit deleted. The shipped rule tests membership in the full
route.layoutsset and makes the set a strict subset. A squash merge takes the body as the commit body, so the retracted rule would have landed in git history as the explanation for the change.
|
Both fixed. The scratch apps are removed in |
Making the 403 / 401 / 404 boundaries render their layout chain also made their modules ship, and three things that were harmless while they never shipped are not harmless now. webjs check's always-ship set listed errors, loadings and the not-found pages but not forbidden, unauthorized or global-not-found. Those reach the browser now, so a forbidden.ts importing a server-only module is the exact throw-at-load crash the rule exists to catch, and it passed a green check. instrumentation-client was missing from every boundary boot set, so the error and 403 pages were the only pages running app modules without the app's client error reporting installed, which is where it matters most. A layout throwing while wrapping a 403 / 401 / 404 was swallowed by a bare catch. Those paths execute layout modules for the first time, so a real crash degraded to a chrome-less page with nothing in dev saying why and nothing reaching the APM sink. It reports through the same sinks a page render error does, then degrades as before.
vivek7405
left a comment
There was a problem hiding this comment.
Final read over the whole diff at its finished state.
The wrapping itself, the emitted-id rule and the boundary walk all hold up on this pass. What it turned up instead is one theme, and it is the interesting part of this change: making the 403 / 401 / 404 boundaries render their layout chain also made their modules SHIP, and three things that were harmless while they never shipped stopped being harmless the moment they did. None of the three is in the wrapping logic, which is why the earlier rounds missed them.
-
packages/server/src/check/runner-support.js: the always-ship set listed errors, loadings and the not-found pages, but not forbidden, unauthorized or global-not-found. Those reach the browser now, so aforbidden.tsdoingimport { auth } from '#lib/auth.server.ts'is the exact throw-at-load crashno-server-import-in-browser-moduleexists to catch, and it passed a green check. A 403 that greets the signed-in user by name is the natural shape of that file, so it is not a hypothetical import. -
packages/server/src/ssr/render.js,boundaryModuleUrls: instrumentation-client was missing from every boundary boot set, so the error and 403 pages became the only pages that run app modules with the app's client error reporting absent. On the pages where it matters most. -
packages/server/src/ssr/render.js, the 403 / 401 / 404 catch: a layout throwing while wrapping was swallowed by a bare catch with no report at all. Those paths execute layout modules for the first time here, so a genuine crash degraded to a chrome-less page with nothing in dev saying why, and nothing reaching the APM sink.
The lesson worth keeping: adding markup to a response is a commitment to boot it, to lint it, and to report what breaks while producing it.
|
All three fixed in The check rule now carries forbidden, unauthorized and global-not-found (global-ERROR stays out on purpose: it returns its own document with no boot script, so nothing of it reaches the browser). Three cases added, and I toggled the fix back off to confirm all three go red without it rather than passing for some other reason. instrumentation-client is prepended to the boundary boot set under the same first-import contract the happy path uses, but only when the set is non-empty: an empty set means no boot script at all, and instrumentation alone is not a reason to start emitting one. A layout throw on those paths now reports through onError and onDevError before degrading, with a test asserting the sink actually receives it and that the response still degrades to the standalone 403 rather than failing. |
…he 500 one The new boundary-layout reporter treated every throw as an error, so a layout throwing redirect() while wrapping a 403 reached the app's APM sink and painted a false dev error overlay over the page the user was looking at. That is the classification the page path already makes before it reports, and it matters more here because of what the overlay lands on top of. Sentinels are filtered out now. They are still not HONOURED, which is deliberate and written down: the status is decided by the time a boundary renders and the boundary page is the answer to the request, so a layout redirect cannot replace a 403 the app asked for with a bounce it did not. The 500 path kept swallowing the same throw the commit before this one exists to stop losing. Its fallthrough to the next boundary out is right, but the sinks only ever heard the original page error, so a boundary or layout that crashed while handling it vanished. It reports on the way past now. The non-empty guard on the instrumentation prepend was unreachable and its comment described a state the function cannot be in: the boundary file is always pushed, since only pages and layouts are fed to the elision analysis.
vivek7405
left a comment
There was a problem hiding this comment.
Check of the previous round's fixes.
Three problems, all in the reporting I added rather than in the wrapping.
-
packages/server/src/ssr/render.js,reportBoundaryLayoutError: it treated every throw as an error, so a layout throwingredirect()(an auth bounce, a supported shape) while wrapping a 403 reached the app's APM sink AND painted a dev error overlay over the 403 the user is looking at. This file already makes exactly that classification on the page path, peeling the sentinels off before it callsonErrorwith the comment that they are control flow, not errors. The new code did not. -
packages/server/src/ssr/render.js, the 500 path'scatch (nested): still swallows a wrapped-layout throw with no report, which is the precise vanishing the previous commit exists to prevent, left in place on the one path that had the problem first. When the walk exhausts every boundary because each candidate's wrap set contains the throwing layout, the sinks hear only the ORIGINAL page error, so the layout crash is never named anywhere. -
packages/server/src/ssr/render.js, the instrumentation prepend: the&& urls.lengthguard is unreachable and its comment describes a state the function cannot be in. The boundary file is always pushed, because only pages and layouts are fed to the elision analysis, which is what the docblock a few lines up already says. Dead defensive code carrying a false rationale is worse than neither.
|
All three fixed in The reporter filters control-flow sentinels now, with a test asserting a layout The 500 path reports on the way past its fallthrough, with a test that a layout crash and the original page error both reach the sink. The dead guard and its comment are gone. |
vivek7405
left a comment
There was a problem hiding this comment.
Check of the previous round's fixes. Four findings, all OPEN. I am stopping the review cycle here rather than fixing them, because this is the second fix-check in a row to surface must-fix work and continuing would be chasing.
-
packages/server/src/ssr/render.js, the report call inside the error-boundary loop: it reports the same layout error once per boundary in the chain. My stated convergence argument, that each step outward wraps a strictly smaller set, is not whatlayoutsForBoundaryguarantees. It filters by segment ancestry, not by boundary depth, so two boundaries at different segments can resolve to an IDENTICAL layout set. With layouts['app/layout.ts']and errors['app/error.ts', 'app/docs/error.ts'], a throwing root layout is delivered twice toonError, twice toonDevError, and printed twice. Three nested boundaries, three times. The loop still terminates, because the bounded descending index is what guarantees that, but the reason written next to it is wrong. My own test cannot catch this: its throwing layout is excluded from the second attempt's set, and the assertion isincludesrather than a count. -
packages/server/src/ssr/render.js, theglobal-errorattempt: it still swallows exactly the class of error this reporting exists to stop losing.catch (nested)is bound and unused. A brokenglobal-error.ts, the app's last-resort boundary, reaches no sink, no overlay and no console line, so the developer sees the generic default 500 and the original error with nothing saying the global boundary is what failed. -
packages/server/src/ssr/render.js: the new report makes the dev overlay show the SECONDARY failure instead of the root cause. The original page error goes toonDevErrorfirst, then this pushesnestedthrough the same sink, and the dev handler's frame slot is last-write-wins. So the frame the developer looks at is the boundary crash and the page error that actually produced the 500 is gone. Finding 1 compounds it. That slot is per URL and retained, so the loss survives a reconnect. -
.agents/skills/webjs/references/routing-and-pages.mdandAGENTS.mddocument a fallthrough that exists only on the 500 path. Both say a throwing layout is handled by the next boundary out, and the skill says it explicitly ofnot-found/forbidden/unauthorized. Those three have no outward walk: onenearest(...)file, one attempt, and a layout throw degrades to a chrome-less standalone render. Neither surface mentions that degradation, nor that a control-flow sentinel from a wrapped layout is discarded there, so an auth-gate layout callingredirect('/login')is a silent no-op on those responses.
The pattern worth naming: the wrapping half of this change has been clean since the first delta round, and every finding in the last three rounds has been in the REPORTING I added around it. I kept treating "report it" as a one-line addition when it is a feature with its own design (deduplication, sink precedence, which frame wins the overlay slot, control-flow classification). That is the part that is not finished.
Three defects in the reporting, plus the docs that described it wrongly. The report sat inside the error-boundary walk, so one throwing layout was reported once per boundary in the chain. The convergence argument written beside it was wrong: layoutsForBoundary selects by segment ancestry, not by boundary depth, so two boundaries can resolve to the same layout set and a root layout that throws fails every attempt. The loop still terminates, on its bounded index rather than on a shrinking set. Deduplicating by identity would not work either, since a layout that constructs its error yields a fresh object per attempt, so the 500 path carries one latch across the walk and the global-error attempt. The dev overlay keeps ONE retained frame per url and its slot is last-write wins, so pushing a secondary failure after the page error replaced the root cause with a symptom, and the replacement survived a reconnect. The 500 path no longer sends secondary failures to the overlay; the page error owns that frame. The 404 / 403 / 401 paths still do, because nothing else claims it there: their trigger is a sentinel, which is never reported. A broken global-error, the app's last-resort boundary, still reached no sink at all. It reports through the same latch now. The docs said a throwing layout falls through to the next boundary out, which is true only on the 500 path. The other three render one nearest boundary and degrade to a chrome-less standalone render, and discard a control-flow throw from a wrapped layout rather than honouring it. Both surfaces say so now.
|
All four fixed in The 500 path carries one latch across the walk AND the global-error attempt, so a root layout that throws is reported once for the request rather than once per boundary. Identity dedup would not have worked, since a layout that constructs its error yields a fresh object per attempt. I also corrected the convergence claim written beside the loop: it terminates on its bounded index, not on a shrinking wrapped set, because Secondary failures no longer reach the dev overlay on the 500 path, so the page error keeps the frame. The 404 / 403 / 401 paths still send theirs, because nothing else claims the slot there (their trigger is a sentinel, which is never reported). A broken Both doc surfaces now say the outward fallthrough is the 500 path only, that the other three degrade to a chrome-less standalone render with the status preserved, and that a control-flow throw from a wrapped layout is discarded there rather than honoured. I toggled the latch and the overlay policy back off to confirm the two new assertions go red without them. |
The latch was wrong in both directions. Control only reaches the global-error attempt because an earlier boundary threw, so on any route with a real error.ts the latch was already spent and global-error's own crash was reported nowhere, which is exactly what the previous commit added it to prevent. My test missed it by using an empty errors chain, so it passed without ever exercising the path that matters. It uses a real boundary now. In the other direction the latch silenced genuinely distinct failures: an inner boundary throwing a TypeError consumed it, so a later, unrelated layout crash vanished. That made the sentence the same commit added to AGENTS.md and the skill, that a real layout crash is reported rather than swallowed, false as written. Dedup is keyed on the error's name, message and throw site instead. That collapses the case it exists for, one shared layout failing several boundary attempts, while two different causes are both reported. Identity cannot be the key, since a layout that constructs its error yields a fresh object per attempt. The docs site was also left behind: both website pages stated only the outward-walk rule while AGENTS.md and the skill had gained the 500 versus 404/403/401 divergence. All four surfaces agree now, including the dedup.
|
All three fixed in Dedup is keyed on the error name, message and throw site now, so it collapses the case it exists for (one shared layout failing several boundary attempts) while two different causes are both reported, The global-error test now uses a real Both website pages carry the divergence now, so all four doc surfaces agree, including the dedup. |
vivek7405
left a comment
There was a problem hiding this comment.
Check of the previous round's fixes. Three findings, all in the deduplication I added.
-
The shared latch made the new
global-errorreport unreachable in the one case it was written for. Control only reaches that block because an earlier boundary attempt threw, so on any route whose chain contains a realerror.{js,ts}the latch is already spent and a brokenglobal-errorreaches no sink at all, which is the outcome the comment beside it claims to prevent. The test added with it used an emptyerrorschain, so it passed without exercising the path that matters. -
In the other direction the latch drops genuinely distinct failures. It keys on "any secondary failure already reported", not on "this same crash", so an inner boundary throwing a
TypeErrorsilences a later, unrelated root-layout crash. That makes the sentence the same commit added toAGENTS.mdand the skill, that a real layout crash is reported rather than swallowed, false as written. -
The docs site was not brought along.
AGENTS.mdand the skill gained the 500 versus 404 / 403 / 401 divergence, including the user-visible part (a control-flow throw from a wrapped layout is discarded there and the response degrades chrome-less). Both website pages still stated only the outward-walk rule, and the PR body lists them as docs for this change.
The fingerprint was wrong on three counts and unsafe on a fourth. It never saw the original error, so the commonest layout crash of all was reported twice: when a layout is what threw, it is both the error that produced the 500 and the thing re-run around every boundary it wraps. The set is seeded with it now. Keying on the whole stack cannot work, and neither can keying on the first frame alone. The frames below the construction site record how the throw was reached, so the same layout re-rendered around a different boundary never matches itself; the construction site alone, meanwhile, is shared by two genuinely different failures that go through one helper. Those two shapes are indistinguishable by stack, so the key carries the STAGE instead. One layout failing repeatedly inside the walk collapses to one report, while a shared helper failing in both the walk and global-error reports twice. The key could also throw. String(Object.create(null)) throws, and this runs inside the catch that keeps the response alive, ahead of the guarded sinks, so a null-prototype throw escaped ssrPage and took the 500 page with it. It is total now and returns null when no safe key exists, which means report rather than risk dropping. That also covers a non-Error throw, which used to collapse every plain object to one key and silently drop the second.
vivek7405
left a comment
There was a problem hiding this comment.
Check of the previous round's fixes. Five findings, all in the deduplication.
-
The set was never seeded with the original error, so the commonest layout crash of all was still reported twice. When a LAYOUT is what throws, it is both the error that produced the 500 and the thing re-run around every boundary it wraps, and the root layout wraps every boundary. No test covered that shape: the nearest one pairs the throwing layout with a page that also throws, so the layout crash is never the original error there.
-
The key computation could throw, ahead of the guarded sinks, inside the catch that is supposed to keep the response alive.
String(Object.create(null))throws on both runtimes, so a null-prototype throw escapedssrPageentirely and took the default 500 page with it. The latch it replaced could not throw. -
Every non-Error throw collapsed to one key, so two unrelated plain-object failures shared
raw:[object Object]and the second was dropped. -
Keying on the first stack frame collapses two different boundaries that fail through one shared helper, so
global-error's own crash is swallowed by an earlier boundary's. The test meant to guard that passes only because its fixtures throw distinct messages from distinct files. -
A stale comment left by the rename, still describing the latch that no longer exists.
|
All five fixed in Findings 1 and 4 turned out to be the same problem from opposite ends, and they are not both solvable by fingerprinting the error. The frames BELOW the construction site record how the throw was reached, so the same layout re-rendered around a different boundary never matches itself on a full-stack key; the construction site ALONE, meanwhile, is shared by two genuinely different failures that go through one helper. Those two shapes are indistinguishable by stack, which is why I had one or the other broken on every attempt. So the key carries the STAGE (the boundary walk versus the global-error attempt) alongside the error's name, message and construction site. One layout failing repeatedly inside the walk collapses to a single report; a shared helper failing in both the walk and global-error reports twice. The set is seeded with the original error under the walk's stage, which closes finding 1. The key is total now and returns null when none can be derived safely, meaning report rather than risk dropping. That covers finding 2 (it ran ahead of the guarded sinks inside the catch that keeps the response alive, so it could take the 500 page down) and finding 3, since a non-Error no longer collapses to a shared key. Reverting the seed, the stage, and the totality independently reds the three new assertions, so they observe the defects rather than passing incidentally. |
…om throwing Two hazards and four comments that described a mechanism the code does not have. The 403 / 401 / 404 catches rendered the thrown value into the response body in PRODUCTION, so a boundary that crashed leaked its error message to the client. The 500 path has always drawn that line, because a thrown message is not author-controlled. Those pages now show the detail in dev and the heading alone in prod. Both of those catches, and the dev branch of the default 500 page, also stringified the thrown value unguarded. String(Object.create(null)) throws on both runtimes, and all three run inside a catch whose job is to keep the response alive, so formatting the failure could escape the handler and turn a handled 500 into an unhandled one. One total formatter covers all three. The comments claimed the dedup key is the full stack. It is the stage plus the name, message and construction site, and the same block argues against a full-stack key three lines later. Two test comments said the same thing, one of them on the test that would fail under the mechanism it described.
vivek7405
left a comment
There was a problem hiding this comment.
Check of the previous round's fixes. Six findings: two real hazards, four comments describing a mechanism the code does not have.
-
The dedup docblock opens by calling the key "the error's name, message and FULL stack". The code keys on the single construction frame, and the same block argues against a full-stack key three lines later. The opening sentence is the contract a reader takes.
-
boundaryErrorKeygained astageparameter and no@paramfor it, in a package whose only typing mechanism is JSDoc.
3, 4. Two test comments repeat the full-stack claim, one of them on the very test that would fail under a stack-based key. They pass because of the stage component, which neither mentions.
-
The dev branch of the default 500 page stringifies the thrown value unguarded.
String(Object.create(null))throws, and this runs inside the catch that keeps the response alive, so the guarantee the previous commit added holds only in prod. The test pinneddev: false, so nothing covered it. -
The 403 / 401 and 404 catches have the same hazard, and a second problem beside it: they render
String(e)into the response body in PRODUCTION, so a boundary that crashes leaks its error message to the client. The 500 path deliberately does not.
|
All six fixed in Finding 6 is the one that matters: a crashing 403 / 401 / 404 boundary was rendering the thrown message into the response body in production. Those pages show the detail in dev and the heading alone in prod now, which is the line the 500 path already drew. One total formatter covers all three unguarded stringifications, so formatting a failure can no longer escape the catch that exists to keep the response alive. The four comments now say what ships: the key is the stage plus the name, message and construction site. Reverting the guard and the prod gate independently reds the three new assertions. |
This case asserted the thrown message appears in the body with dev:false, which is the leak the previous commit removed. It now pins both halves: dev still shows the failure, prod shows the status alone.
Sanitizing the 403 / 401 / 404 body removed the only production-visible trace of that failure and put nothing in its place, so a boundary whose module throws or cannot be imported produced a bare heading and no server signal at all, on a request that already returned a 4xx to a real user. Sanitizing a response should move a failure out of the RESPONSE, not out of sight. It reports through the same path the adjacent layout crash does. The docs site gains the rule this commit and the last one established for the boundary's own crash: shown in dev, withheld in prod, reported either way.
vivek7405
left a comment
There was a problem hiding this comment.
Check of the previous round's fixes. Two findings, both consequences of the sanitization.
-
A crashing boundary MODULE is now completely silent in production. Sanitizing the body removed the only production-visible trace of that failure and put nothing in its place: that catch calls no
console.error, noonError, noonDevError, andloadModulelogs nothing either. So aforbidden.ts/unauthorized.ts/not-found.tsthat throws or fails to import produces a bare heading and zero server signal, on a request that already returned a 4xx to a real user. It contradicts the sibling catch three lines above, which reports precisely so a crash does not vanish, and every other sanitizing path in the codebase, each of which pairs sanitization with a server-side log. -
That commit shipped a user-visible production behaviour change touching only source and tests. What a production 404 / 403 / 401 body contains when the boundary crashes changed, and no doc surface said so. The docs-with-src gate exists for exactly that shape, and its bypass covers only an internal change with no behaviour change.
|
Both fixed in Finding 1 is the sharper one and I had it backwards: sanitizing a response is supposed to move a failure out of the RESPONSE, not out of sight. Both boundary catches report through the same path the adjacent layout crash uses, covering the module that throws and the one that cannot be imported at all. Reverting either call reds its assertion. The docs site now carries the rule both commits established for the boundary's own crash: shown in dev, withheld in prod, reported either way. |
…inks The inner catch reported before the standalone fallback ran, so a failure in the boundary's OWN tree was reported twice (the fallback re-renders the same tree and throws the same way) and the first report called it a layout crash when no layout threw. The fallback now runs first and decides: if it succeeds the fault was the layout chain and that is what gets reported; if it throws too the tree is broken, so the error propagates and the outer catch reports it once under the right label. The unmatched-url 404 in dev/serve.js built its own options object without onError or onDevError, so a root not-found that threw or failed to load reached only the console, which is the exact path the previous commit claimed to cover. The sinks are built inline there (the matched-page ssrOpts is scoped to a branch this path does not take) and a test drives the real path rather than passing the hook in directly. Two older cases printed real stack traces into the suite output now that the crash they provoke is reported; both are muted. The skill and AGENTS.md gained the boundary-crash rule the website page already had.
vivek7405
left a comment
There was a problem hiding this comment.
Check of the previous round's fixes. Four findings.
-
One failure reported twice, and the first one mislabelled. The standalone fallback re-renders the SAME tree, so when the fault is in the boundary's own tree rather than in a wrapped layout, the inner catch reports it, the fallback throws identically, and the outer catch reports it again.
onError,onDevErrorand the console each fire twice for one crash, and the first is labelled "a layout threw" when no layout threw at all. -
The unmatched-url 404 in
dev/serve.jsbuilds its own options object with noonError/onDevError, so a rootnot-foundthat throws or fails to load still reaches only the console. That is the most common shape of the very thing the previous commit set out to fix, and its test passes the hook straight tossrNotFound, proving the capability without exercising the production wiring. -
Two older cases print real stack traces into the suite output now that the crash they provoke is reported. Every other case in that file that provokes a boundary throw is muted.
-
The behaviour change is documented on the website page only. The skill reference and
AGENTS.mdstill scope the reporting guarantee to a layout crash and say nothing about the boundary's own throw being sanitized in prod, so an agent reading the skill gets the pre-commit picture.
|
All four fixed in The ordering is the fix for finding 1: the fallback runs FIRST and is what tells us what failed. If it succeeds the fault was the layout chain, so that is what gets reported; if it throws too the tree itself is broken, so the error propagates and the outer catch reports it once under the right label. Finding 2 was the sharper miss, since it is the common shape of what the previous commit claimed to cover. The sinks are built inline at that call site, because the matched-page ssrOpts is scoped to a branch the unmatched path never takes, and the new test drives the real unmatched-url path instead of passing the hook in directly. Reverting either fix reds its own assertion. The two noisy cases are muted, and the skill and AGENTS.md now carry the boundary-crash rule the website page already had. |
Inferring the phase by re-rendering is unsound when BOTH a wrapped layout and the boundary tree are broken: the standalone attempt throws too, so the tree error was discarded and the LAYOUT error was reported under the boundary label. Two failures, one report, wrong name, in the code whose whole purpose is that a boundary crash is never lost. The phase is known exactly where it happens, so renderBoundaryInChain marks it and the catch reads it. Both errors are reported now, each under its own label, and the dev body shows the tree error, which is the one that defeated the fallback. The unrouted 404's dev frame also stamped a bare pathname. The overlay gate compares against location.pathname + location.search, so the frame was refused (and then dropped) for any url carrying a query, and for every url on a base-path deploy, where the ingress strip removed the prefix from url while the browser still has it. It stamps what the matched-page branch stamps. That hook is also dropped for a speculative prefetch now, per the same #1047 rule: hovering a link to a broken url must not raise an overlay on the page the user is actually on.
|
All three fixed in Finding 1 is the one worth naming: I was INFERRING the failure phase by re-rendering, and the phase is known exactly where it happens. The frame stamp now matches the matched-page branch exactly, and the hook is dropped for a speculative prefetch. Reverting each fix independently reds its own assertion, including the two new ones that assert the stamp and the prefetch gate, which a render-level test cannot see. |
vivek7405
left a comment
There was a problem hiding this comment.
Check of the previous round's fixes. Three findings.
-
The "run the standalone render first, and if it throws too the boundary's own tree is what is broken" inference is unsound when BOTH a wrapped layout and the boundary tree are broken. The layout functions run before the tree renders, so the wrapped attempt yields the layout error, the standalone attempt then throws the TREE's error, the bare catch discards it, and the layout error is handed to the outer catch, which reports it under the boundary label. Two failures, one report, wrong name, in the code whose stated purpose is that a boundary crash is never lost.
-
The unrouted 404's dev frame stamps a bare
url.pathname, while the overlay's scope gate compares againstlocation.pathname + location.search. So the frame is refused for any unrouted url carrying a query, and for EVERY url on a base-path deploy, since the ingress strip already removed the prefix fromurlwhile the browser's location still has it. A refused frame goes to the pending slot and is then dropped, so the overlay this commit set out to wire up never paints. The matched-page branch builds the correct stamp 100 lines above, and both helpers are already imported. -
That same hook has no prefetch gate. The matched-page branch drops it deliberately (#1047: hovering a link to a broken page must not raise an overlay on the page you are on, nor become the frame the SSE replays into a fresh tab). Prefetch is on by default and fetches unrouted hrefs too.
The marker records a layout that throws when CALLED. It cannot record one that fails while being SERIALIZED, the idiomatic async form with a rejecting template hole, because by then the layout's output is fused into one tree with the boundary's and renderToString cannot say which half threw. Treating unmarked as tree-broken therefore skipped the standalone fallback for that whole class, so a 403 whose layout had a rejecting hole served a bare heading where it used to serve the boundary page, reported under the wrong label. The fallback's OUTCOME is the other half of the diagnosis: if the boundary renders alone the fault was layout-side, marked or not. The unrouted 404 also gains the supersede rule its matched-page sibling has: a later successful render of the same url clears the frame it retained, so an intermittently-failing not-found cannot leave one that paints over a page that has since recovered. Only reachable once the stamp was correct enough for the overlay to accept the frame at all, which the previous commit did.
vivek7405
left a comment
There was a problem hiding this comment.
Check of the previous round's fixes. Three findings, the first two one defect.
1 and 2. The layout-phase marker covers layout INVOCATION only, not layout RENDERING. A layout that returns fine and fails while being serialized, the idiomatic async form with a rejecting template hole, throws from renderToString(chain.tree), which sits outside the marked try. It arrives unmarked, so the new rule treats it as a broken boundary tree, skips the standalone fallback and serves a bare heading where the boundary page used to be served, reported under the wrong label. Every test of a throwing layout in the suite uses a synchronously-throwing one, which is why nothing caught it.
- The unrouted 404 now stamps a frame the overlay gate accepts, but has none of the matched-page branch's supersede logic, so an intermittently-failing not-found leaves a retained frame that paints over the same url once it renders again. Before the stamp was fixed the frame was refused and then dropped, so this was unreachable; making the stamp correct is what exposed it.
|
All three fixed in The marker was never going to be sufficient on its own, and that is the useful correction: once a layout has returned, its output is fused into one tree with the boundary's, so The unrouted 404 gains the supersede rule its matched-page sibling has. Its test observes Both fixes reverted independently red their own assertions. |
…inless boundary The retained-frame clear infers that a render succeeded from the frame it captured still being current, and that inference holds only when a sink was installed to write a new one. A prefetch installs none, so a prefetch of a url whose not-found throws AGAIN looked exactly like a recovery and wiped a frame that was still current, which is the gap the retention exists to close. The matched-page branch gates its whole supersede closure on !isPrefetch for this reason; this one was gated only on dev. The standalone fallback also ran when no route was supplied, where there is no layout chain and the try body already ran that exact renderToString. It re-executed the boundary tree to learn nothing, and a tree that is not idempotent under re-render could succeed on the retry and be reported as a layout crash that never happened.
vivek7405
left a comment
There was a problem hiding this comment.
Check of the previous round's fixes. Two findings.
-
The retained-frame clear fires on a PREFETCH whose render also failed, wiping a still-current frame. The clear infers "this render succeeded" from "the frame I captured is still current", and that inference holds only when a sink was installed to write a new one. A prefetch installs none, so a prefetch of a url that throws AGAIN writes nothing and looks exactly like a recovery. The url is still broken and the SSE replay into a fresh tab now has nothing to hand it, which is the gap the retention exists to close. The matched-page branch this copies gates its whole supersede closure on
!isPrefetch; the new one is gated only ondev. Reachable with the default router: navigate to the broken url, navigate away, hover its link. -
The standalone fallback is no longer guarded by
opts.route, so on the unrouted path it re-runs the identical call that just threw. No layout was ever loaded there, so the retry either throws the same way, having executed the boundary tree a second time for nothing, or, for a tree that is not idempotent under re-render, succeeds and is reported as a layout crash that never happened.
|
Both fixed in The supersede is gated on The prefetch case has a test now: navigate to the broken url, then hover its link, and assert the frame survives. Reverting the gate reds it. |
…tless re-render The Bun job was red on this branch and I had been reporting it green: I last ran the matrix before adding these fixtures and kept quoting that result. A Promise.reject built at template-construction time is racy for unhandled-rejection detection, and Bun failed on it where Node did not. A hole whose toString throws fails at the same point in the render, on both runtimes, with no floating promise. Verified under bun and node directly, not just through the matrix. The no-layout short-circuit also only covered a missing route. A route whose wrap set is empty (a legal app with no root layout) reduces to the same renderToString the fallback would repeat, so it hit the case the guard was written to prevent. Both go through one wrapLayoutsFor helper now, which also removes the second derivation the boot set was doing.
… path Wrapping only happened when a boundary FILE existed, so a matched route with no forbidden.ts / not-found.ts of its own still returned a bare document with no markers and hard-loaded. That is the common case, since most apps ship no forbidden.ts, and it meant the headline fix did not reach them at all. The framework's own default body now renders inside the route's layout chain and boots the layouts that rendered. A route with no layouts keeps the bare default, since there is nothing to wrap in and no marker should pretend otherwise. The form path also resolved notFound() against the ROOT not-found file only, so the same url produced two different 404s depending on whether the throw came from the page render or from a form action on it. It walks the matched route's chain now, which is what #848 made the page path do.
Closes #1298
Summary
A boundary render (
error,not-found,forbidden,unauthorized) was served without its layout chain: the catch rendered the boundary standalone and handed it towrapInDocument. Since the keyed<!--wj:children:...-->comments come from the layout wrapping, the response carried none of them, the client router's scan found no shared boundary, and every navigation into a failing page degraded to a full document load. The user lost their scroll, their hydrated state, and the site chrome, and landed on a bare page with no nav to escape with.Boundaries now render in the page's position inside the layouts at and above their own segment, through the same emitter the happy path uses.
What changed
wrapLayoutChainis lifted out ofrenderChain, unchanged, so the happy path and every boundary path share ONE emitter. A mismatched pair degrades the swap exactly as a missing one does, so a second copy of the loop would look fixed in a diff and still hard-load.X-Webjs-Have, and the NEXT navigation into that subtree short-circuits on it and returns a fragment assuming chrome the page never had.forbidden.ts/not-found.tsof the app's own) renders in the chain too. That is the common case, and leaving it bare meant the fix did not reach most apps. A route with no layouts keeps the bare default, since there is nothing to wrap in.instrumentation-clientfirst as on the happy path. Booting matters more now than it did: a response that paints layout chrome and never hydrates it is worse than one that paints none.{}. The form path resolvesnotFound()nearest-wins, matching the page path (Close Next.js 16 file-routing parity gaps (async params, forbidden/unauthorized, global-error/not-found, instrumentation) #848).Error reporting
Making boundaries render layouts also made those paths execute layout modules, and ship them, for the first time. That surfaced a cluster of observability gaps, all closed here:
global-erroris reported toonErrorinstead of vanishing. Repeats of one cause collapse to a single report, keyed on the stage plus the error's name, message and construction site; distinct causes are each reported. A control-flow sentinel is never reported, and never honoured on a boundary path, since the status is already decided.webjs check's always-ship set gainedforbidden/unauthorized/global-not-found, which now reach the browser, so a.server.tsimport in one is caught rather than passing a green check and crashing at load.Decisions
A boundary is wrapped by its OWN segment's layout, not only the ones above it. That is Next's hierarchy (
layoutthenerrorthenpage), and it is why a boundary cannot catch its own segment's layout: a throwing layout falls through to the next boundary out on the 500 path, and degrades to a chrome-less standalone render on the 404 / 403 / 401 paths, which have no outward walk.Diagnosing WHICH part failed needs two signals, not one. The marker records a layout that throws when CALLED; it cannot see one that fails while being SERIALIZED, because by then its output is fused into one tree with the boundary's. The fallback's outcome answers that half: if the boundary renders alone, the fault was layout-side.
global-errorstays unwrapped. It writes its own document (invariant 8), wrapping it would re-run the code that just threw, and it ships no boot script, so it could not soft-swap anyway.The layout chain runs a second time on a boundary response. Measured at about 0.21ms for a two-layout chain, on a response that is already a failure. Reusing the happy path's output would mean rendering each layout against a placeholder and re-joining strings, which moves the happy path's bytes and every ETag with them. The consequence (a fetching layout fetches twice) is documented instead.
Test plan
test/ssr/ssr.test.jsand 8 pure-derivation cases inpackages/server/test/ssr/region-route-key.test.js, plus new cases inpackages/server/test/routing/global-boundaries.test.js(the unrouted-404 sinks, the frame stamp, the prefetch gate, the supersede) andpackages/server/test/routing/form-dispatch.test.js(nearest-wins), and three inpackages/server/test/check/no-server-import-in-browser-module.test.js.test/bun/ssr-boundary-chain.mjsproves a 500 and a 403 carry matched markers on both runtimes.test/bun/routing-boundaries.mjsgains the layout derivation and, for the first time, a.test.mjswrapper, so it runs on Node as well as Bun.webjs checkandwebjs doctorclean ingallery,examples/blogandwebsite.gallery/features/boundaries/{gated,private,crash}serve their layout chain plus a boot script where they previously served neither.Docs
website/app/docs/error-handling/page.ts,website/app/docs/routing/page.ts,.agents/skills/webjs/references/routing-and-pages.md,AGENTS.md, and the gallery boundaries demo. The scaffold copy of the skill syncs from the repo-root.agents/at prepack, so no separate template edit is needed.