Skip to content

Commit f72c16d

Browse files
committed
fix: dedupe boundary reports by cause, not by "one per request"
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.
1 parent 05991db commit f72c16d

6 files changed

Lines changed: 94 additions & 24 deletions

File tree

.agents/skills/webjs/references/routing-and-pages.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ Refusals worth knowing: `formaction=${fn}` is supported on a `<button>` anywhere
217217
- On the **500 path**, a throwing layout is handled by the next `error.{js,ts}` OUT: the walk tries each boundary in the chain, innermost first, and a layout that throws fails every attempt whose wrapped set contains it, so control ends at `global-error` (or the default 500 page) when they are exhausted.
218218
- On the **404 / 403 / 401 paths** there is NO outward walk. Each renders the one nearest boundary, so a throwing layout degrades that response to a chrome-less standalone render of the boundary with no boot script, keeping its status. A control-flow throw from a wrapped layout there (an auth-gate layout calling `redirect('/login')`) is discarded rather than honoured, deliberately: the status is already decided and the boundary page is the answer to that request.
219219

220-
A genuine layout crash is reported to `onError` (and to the dev overlay on the 404 / 403 / 401 paths, where nothing else claimed the frame) rather than being swallowed; a control-flow sentinel never is, since it is routing rather than a crash.
220+
A genuine layout crash is reported to `onError` (and to the dev overlay on the 404 / 403 / 401 paths, where nothing else claimed the frame) rather than being swallowed. Repeats of the SAME cause within one request collapse to a single report, keyed on the error's name, message and throw site, because one shared layout can fail several boundary attempts; two DIFFERENT failures are both reported, including `global-error`'s own. A control-flow sentinel never is, since it is routing rather than a crash.
221221

222222
Two further consequences: a layout that fetches runs its fetch again on a boundary response, and a `<webjs-suspense>` inside a wrapped layout shows its fallback, because a boundary response is buffered so its status is final before the first byte. A 404 for a URL that matched NO route has no chain to wrap in and stays a bare document.
223223
- Root-only (in `app/` exactly): `global-error.ts` is the app-wide catch-all after nested `error` boundaries are exhausted and renders its OWN `<!doctype><html><body>` (returned verbatim, so keep it static HTML with no components or hydration). That verbatim document is exactly why it is the one boundary left UNWRAPPED: a second shell would nest inside the root layout's, wrapping it would re-run the code that just threw, and with no boot script it could not soft-swap anyway. `global-not-found.ts` renders for an unmatched-anywhere URL when no `not-found` matches.

AGENTS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,7 @@ Default export receives `{ children, params, searchParams, url }`, must embed `c
340340

341341
### Error / loading / metadata routes
342342

343-
`error.{js,ts}` default-exports `({ error, ...ctx }) => TemplateResult` (catches sibling-page / deeper render errors, innermost wins, prod sends only `error.message`). **A boundary renders INSIDE the layouts at and above its own segment** (#1298), so it carries the keyed `wj:children` pairs and navigating into a failing page is a SOFT navigation with the site chrome and its hydrated state intact; the same holds for `not-found` / `forbidden` / `unauthorized`, which now receive a page-shaped ctx too. A layout deeper than the boundary is not rendered and is not in the boot script, which ships the boundary module instead of the page's. A boundary cannot catch its OWN segment's layout (it renders inside it, Next's `layout -> error -> page` order). On the 500 path a throwing layout falls through to the next `error` boundary OUT, and to `global-error` when the chain is exhausted; the 404 / 403 / 401 paths have no outward walk, so there a throwing layout degrades that one response to a chrome-less standalone render of the boundary (status preserved, no boot script) and a control-flow throw from a wrapped layout is discarded rather than honoured, since the status is already decided. A real layout crash is reported to `onError` instead of vanishing; a sentinel never is. `loading.{js,ts}` wraps the sibling page in `Suspense` with an immediately-flushed fallback. `forbidden.{js,ts}` / `unauthorized.{js,ts}` render the nearest 403 / 401 boundary for a thrown `forbidden()` / `unauthorized()` (#848). Two **root-only** boundaries (`app/` root exactly): `global-error.{js,ts}` is the app-wide catch-all tried after the nested `error` boundaries are exhausted, and it renders its **own** `<!doctype><html><body>` document (returned verbatim, since a root-layout failure is when it fires). It is the sole boundary that is NOT wrapped in layouts: a second shell cannot nest inside the root layout's (invariant 8), wrapping it would re-run the code that just threw, and it ships no boot script anyway. Because it is returned verbatim (no framework `<head>` splice), it ships **no importmap or boot script**, so keep it **static HTML with no components/hydration** (a last-resort page must not depend on the module system that may have just failed); under an opt-in CSP, an inline `<script>` in it must carry the nonce via `cspNonce()` (an inline `<style>` needs one only if you tighten `style-src`, since the default allows inline style outright). `global-not-found.{js,ts}` renders for an unmatched-anywhere URL when no `not-found` matches. `not-found` is nearest-wins from the throwing page's chain (#848 fixed the prior root-only behavior). Metadata routes (`sitemap`, `robots`, `manifest`, `icon`, `apple-icon`, `opengraph-image`, `twitter-image`) live at app root or static segments only and default-export a possibly-async function; `sitemap(entries)` / `sitemapIndex(sitemaps)` from `@webjsdev/server` serialize spec-valid XML. **`icon` / `apple-icon` are auto-LINKED** into the head (`<link rel="icon" href="/icon">`, `<link rel="apple-touch-icon" href="/apple-icon">`, base-path prefixed, no asserted `type` / `sizes` since the route picks its content type at request time), so writing the file is the whole wiring. A declared `metadata.icons` **suppresses** them rather than merging, matching Next's precedence for its static icon files, so an app that outgrows a placeholder route names its real icons instead of deleting it. `opengraph-image` / `twitter-image` are NOT auto-linked (a preview image is a per-page editorial choice): point `metadata.openGraph.images` / `metadata.twitter.images` at them. Declare a favicon through `metadata.icons` or a metadata route, never a hand-written `<link rel="icon">`, since only the root layout may write a shell at all (invariant 8). See `references/routing-and-pages.md`.
343+
`error.{js,ts}` default-exports `({ error, ...ctx }) => TemplateResult` (catches sibling-page / deeper render errors, innermost wins, prod sends only `error.message`). **A boundary renders INSIDE the layouts at and above its own segment** (#1298), so it carries the keyed `wj:children` pairs and navigating into a failing page is a SOFT navigation with the site chrome and its hydrated state intact; the same holds for `not-found` / `forbidden` / `unauthorized`, which now receive a page-shaped ctx too. A layout deeper than the boundary is not rendered and is not in the boot script, which ships the boundary module instead of the page's. A boundary cannot catch its OWN segment's layout (it renders inside it, Next's `layout -> error -> page` order). On the 500 path a throwing layout falls through to the next `error` boundary OUT, and to `global-error` when the chain is exhausted; the 404 / 403 / 401 paths have no outward walk, so there a throwing layout degrades that one response to a chrome-less standalone render of the boundary (status preserved, no boot script) and a control-flow throw from a wrapped layout is discarded rather than honoured, since the status is already decided. A real layout crash is reported to `onError` instead of vanishing (repeats of the SAME cause within one request collapse to one report, since a shared layout can fail several boundary attempts); a sentinel never is. `loading.{js,ts}` wraps the sibling page in `Suspense` with an immediately-flushed fallback. `forbidden.{js,ts}` / `unauthorized.{js,ts}` render the nearest 403 / 401 boundary for a thrown `forbidden()` / `unauthorized()` (#848). Two **root-only** boundaries (`app/` root exactly): `global-error.{js,ts}` is the app-wide catch-all tried after the nested `error` boundaries are exhausted, and it renders its **own** `<!doctype><html><body>` document (returned verbatim, since a root-layout failure is when it fires). It is the sole boundary that is NOT wrapped in layouts: a second shell cannot nest inside the root layout's (invariant 8), wrapping it would re-run the code that just threw, and it ships no boot script anyway. Because it is returned verbatim (no framework `<head>` splice), it ships **no importmap or boot script**, so keep it **static HTML with no components/hydration** (a last-resort page must not depend on the module system that may have just failed); under an opt-in CSP, an inline `<script>` in it must carry the nonce via `cspNonce()` (an inline `<style>` needs one only if you tighten `style-src`, since the default allows inline style outright). `global-not-found.{js,ts}` renders for an unmatched-anywhere URL when no `not-found` matches. `not-found` is nearest-wins from the throwing page's chain (#848 fixed the prior root-only behavior). Metadata routes (`sitemap`, `robots`, `manifest`, `icon`, `apple-icon`, `opengraph-image`, `twitter-image`) live at app root or static segments only and default-export a possibly-async function; `sitemap(entries)` / `sitemapIndex(sitemaps)` from `@webjsdev/server` serialize spec-valid XML. **`icon` / `apple-icon` are auto-LINKED** into the head (`<link rel="icon" href="/icon">`, `<link rel="apple-touch-icon" href="/apple-icon">`, base-path prefixed, no asserted `type` / `sizes` since the route picks its content type at request time), so writing the file is the whole wiring. A declared `metadata.icons` **suppresses** them rather than merging, matching Next's precedence for its static icon files, so an app that outgrows a placeholder route names its real icons instead of deleting it. `opengraph-image` / `twitter-image` are NOT auto-linked (a preview image is a per-page editorial choice): point `metadata.openGraph.images` / `metadata.twitter.images` at them. Declare a favicon through `metadata.icons` or a metadata route, never a hand-written `<link rel="icon">`, since only the root layout may write a shell at all (invariant 8). See `references/routing-and-pages.md`.
344344

345345
### Route handlers (`app/**/route.{js,ts}`)
346346

packages/server/src/ssr/render.js

Lines changed: 44 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -414,6 +414,23 @@ async function renderChain(route, ctx, dev, suspenseCtx, have, pageModule) {
414414
return { html: body + (await loadingTemplates(route, ctx, dev)), reduced: chain.reduced };
415415
}
416416

417+
/**
418+
* A per-request dedup key for a secondary boundary failure: the error's name,
419+
* message and throw site. Identity cannot be used, because a layout that
420+
* constructs its error yields a fresh object each time it is re-run, and the
421+
* whole point is to collapse exactly those repeats while letting a genuinely
422+
* different failure through. The first stack frame is the throw site, so two
423+
* different layouts raising the same message still read as distinct causes.
424+
*
425+
* @param {unknown} err
426+
* @returns {string}
427+
*/
428+
function boundaryErrorKey(err) {
429+
if (!(err instanceof Error)) return `raw:${String(err)}`;
430+
const site = String(err.stack || '').split('\n')[1] || '';
431+
return `${err.name}:${err.message}:${site.trim()}`;
432+
}
433+
417434
/**
418435
* Report a throw from a layout wrapped around a boundary page (#1298) to the
419436
* same sinks a page-render error reaches, then let the caller degrade to the
@@ -435,14 +452,21 @@ async function renderChain(route, ctx, dev, suspenseCtx, have, pageModule) {
435452
* standalone boundary body, exactly as it does for a real layout crash, and
436453
* the status the boundary carries is preserved.
437454
*
438-
* REPORTED AT MOST ONCE PER REQUEST, via the caller's `once` latch. The 500
439-
* path tries each boundary in the chain, and a throwing layout that is an
440-
* ancestor of several of them fails EVERY attempt: `layoutsForBoundary` selects
441-
* by segment ancestry rather than by boundary depth, so two boundaries at
442-
* different segments can resolve to the same layout set. Without the latch a
443-
* root layout that throws is reported once per boundary in the chain. Identity
444-
* deduplication would not do, because a layout that constructs its error
445-
* (`throw new Error(...)`) yields a fresh object per attempt.
455+
* Deduplicated per request by CAUSE, via the caller's `seen` set. The 500 path
456+
* tries each boundary in the chain, and a throwing layout that is an ancestor
457+
* of several of them fails EVERY attempt: `layoutsForBoundary` selects by
458+
* segment ancestry rather than by boundary depth, so two boundaries at
459+
* different segments can resolve to the same layout set. Without dedup a root
460+
* layout that throws is reported once per boundary in the chain.
461+
*
462+
* The key is the error's name, message and throw site rather than its identity,
463+
* because a layout that constructs its error (`throw new Error(...)`) yields a
464+
* fresh object per attempt. Deduplicating on "any secondary failure already
465+
* reported" instead would be wrong in the other direction: an inner boundary
466+
* throwing a TypeError would silence a LATER, unrelated root-layout crash, and
467+
* would make the `global-error` report below unreachable on every route whose
468+
* chain contains a real `error.{js,ts}`, since reaching that block at all means
469+
* an earlier attempt already threw. Distinct causes are each reported once.
446470
*
447471
* `overlay` says whether this failure may claim the DEV OVERLAY. The overlay
448472
* holds one retained frame per URL and the dev handler's slot is last-write
@@ -455,13 +479,14 @@ async function renderChain(route, ctx, dev, suspenseCtx, have, pageModule) {
455479
*
456480
* @param {unknown} err
457481
* @param {{ onError?: (e: unknown) => void, onDevError?: (e: unknown) => void }} opts
458-
* @param {{ what?: string, once?: { reported: boolean }, overlay?: boolean }} [cfg]
482+
* @param {{ what?: string, seen?: Set<string>, overlay?: boolean }} [cfg]
459483
*/
460484
function reportBoundaryLayoutError(err, opts, cfg = {}) {
461485
if (isRedirect(err) || isNotFound(err) || isForbidden(err) || isUnauthorized(err)) return;
462-
if (cfg.once) {
463-
if (cfg.once.reported) return;
464-
cfg.once.reported = true;
486+
if (cfg.seen) {
487+
const key = boundaryErrorKey(err);
488+
if (cfg.seen.has(key)) return;
489+
cfg.seen.add(key);
465490
}
466491
const what = cfg.what || 'a layout threw while wrapping a boundary page';
467492
if (typeof opts.onError === 'function') {
@@ -1070,9 +1095,11 @@ export async function ssrPage(route, params, url, opts) {
10701095
// error page's boot scripts (when moduleUrls is non-empty) and
10711096
// the meta csp-nonce tag both pass strict-CSP enforcement.
10721097
const errNonce = opts.req ? getNonce(opts.req) : undefined;
1073-
// One latch for the whole 500 path: the walk below, and the global-error
1074-
// attempt after it, report at most ONE secondary failure between them.
1075-
const secondary = { reported: false };
1098+
// One dedup set for the whole 500 path: the walk below and the
1099+
// global-error attempt after it collapse REPEATS of the same cause (a
1100+
// shared layout that fails every attempt) while still reporting a
1101+
// genuinely different failure, including global-error's own.
1102+
const secondary = new Set();
10761103
// Try nearest error.js (innermost → outermost).
10771104
for (let i = route.errors.length - 1; i >= 0; i--) {
10781105
try {
@@ -1125,7 +1152,7 @@ export async function ssrPage(route, params, url, opts) {
11251152
// a routing decision rather than a crash.
11261153
reportBoundaryLayoutError(nested, opts, {
11271154
what: 'an error boundary or its layout threw while handling a render error',
1128-
once: secondary,
1155+
seen: secondary,
11291156
overlay: false,
11301157
});
11311158
}
@@ -1156,7 +1183,7 @@ export async function ssrPage(route, params, url, opts) {
11561183
// that failed.
11571184
reportBoundaryLayoutError(nested, opts, {
11581185
what: 'global-error threw while handling a render error',
1159-
once: secondary,
1186+
seen: secondary,
11601187
overlay: false,
11611188
});
11621189
}

test/ssr/ssr.test.js

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2242,11 +2242,16 @@ test('boundary: a broken global-error is reported, not silently swallowed', asyn
22422242
const { route, appDir } = makeBoundaryApp({
22432243
files: {
22442244
'global-error.js': `export default function GE() { throw new Error('global-error-boom'); }\n`,
2245+
// A REAL error boundary in the chain, which is the shape that matters:
2246+
// control only reaches global-error because this one threw first, so a
2247+
// dedup keyed on "any secondary failure already reported" would swallow
2248+
// global-error's own crash on every route that has an error.ts at all.
2249+
'error.js': `export default function Err() { throw new Error('boundary-boom'); }\n`,
22452250
'page.js': `export default function Page() { throw new Error('page-boom'); }\n`,
22462251
},
22472252
page: 'page.js',
22482253
layouts: [],
2249-
errors: [],
2254+
errors: ['error.js'],
22502255
});
22512256
const seen = [];
22522257
const resp = await SILENT(() => ssrPage(route, {}, new URL('http://localhost/'), {
@@ -2257,9 +2262,35 @@ test('boundary: a broken global-error is reported, not silently swallowed', asyn
22572262
assert.ok(body.includes('Something went wrong'), 'it still degrades to the default 500 page');
22582263
const messages = seen.map((e) => String(e && e.message));
22592264
assert.ok(messages.includes('global-error-boom'), 'the global-error crash reached the sink');
2265+
assert.ok(messages.includes('boundary-boom'), 'and so did the boundary that failed before it');
22602266
assert.ok(messages.includes('page-boom'), 'alongside the original page error');
22612267
});
22622268

2269+
test('boundary: dedup collapses REPEATS of one cause, not distinct failures', async () => {
2270+
// The dedup exists for one layout that fails every attempt. It must not
2271+
// silence a genuinely different secondary failure: an inner boundary that
2272+
// throws its own error, then an outer attempt whose layout crashes, are two
2273+
// causes and both belong in the sink.
2274+
const { route, appDir } = makeBoundaryApp({
2275+
files: {
2276+
'layout.js': `export default function Root() { throw new Error('shared-layout-boom'); }\n`,
2277+
'error.js': HTML_IMPORT + `export default function Err() { return html\`<p>outer</p>\`; }\n`,
2278+
'docs/error.js': `export default function Err() { throw new TypeError('inner-boundary-boom'); }\n`,
2279+
'docs/page.js': `export default function Page() { throw new Error('page-boom'); }\n`,
2280+
},
2281+
page: 'docs/page.js',
2282+
layouts: ['layout.js'],
2283+
errors: ['error.js', 'docs/error.js'],
2284+
});
2285+
const seen = [];
2286+
await SILENT(() => ssrPage(route, {}, new URL('http://localhost/docs'), {
2287+
dev: false, appDir, onError: (e) => seen.push(e),
2288+
}));
2289+
const messages = seen.map((e) => String(e && e.message));
2290+
assert.ok(messages.includes('inner-boundary-boom'), 'the inner boundary crash is reported');
2291+
assert.ok(messages.includes('shared-layout-boom'), 'and the LATER, unrelated layout crash is not swallowed by it');
2292+
});
2293+
22632294
test('boundary: a boundary response is never storable and never reduced', async () => {
22642295
// Two independent guarantees. The HTML cache refuses a non-200 outright, and
22652296
// the reduced X-Webjs-Have path is structurally unreachable: the boundary

0 commit comments

Comments
 (0)