Skip to content

Commit 0af5cf8

Browse files
vivek7405vivek7405
committed
fix: report a boundary tree failure once, and wire the unrouted 404 sinks
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.
1 parent 3afb2d7 commit 0af5cf8

6 files changed

Lines changed: 114 additions & 21 deletions

File tree

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,8 @@ Refusals worth knowing: `formaction=${fn}` is supported on a `<button>` anywhere
219219

220220
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, because one shared layout can fail every boundary attempt (and, when the layout is what threw, arrives again as the error that produced the 500). The key is the STAGE plus the error's name, message and construction site: the stack below that site records how the throw was reached and differs on every re-render, so it cannot be part of the key, and the stage is what keeps `global-error`'s own crash from being swallowed by a boundary that failed through the same helper. Two DIFFERENT failures are both reported, and anything whose key cannot be derived safely (a non-Error throw) is always reported rather than risking a drop. A control-flow sentinel never is, since it is routing rather than a crash.
221221

222+
The BOUNDARY FILE's own crash (it throws, or fails to import at all) is reported the same way, and its response body follows the framework's standard rule for a thrown error: shown in dev, withheld in prod, where the page carries only its status. A thrown message is not author-controlled and may name a driver, a path or a connection string, so it does not reach the client; sanitizing the response never means losing the failure.
223+
222224
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.
223225
- 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.
224226

‎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 (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`.
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. A crash in the BOUNDARY FILE itself is reported too, and its body follows the standard thrown-error rule: shown in dev, withheld in prod (the page carries only its status). `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/dev/serve.js‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -495,7 +495,22 @@ export async function handleCore(req, ctx) {
495495
}
496496
// Unmatched anywhere: prefer the root not-found.{js,ts}, then a
497497
// global-not-found.{js,ts} (#848), else the default 404 page.
498-
return ssrNotFound(state.routeTable.notFound || state.routeTable.globalNotFound, { dev, appDir, req, url });
498+
// The APM / overlay sinks ride along here too: a root not-found that throws
499+
// or fails to load must reach them, not only the console (#1298). Built
500+
// inline rather than reused, because the ssrOpts above is scoped to the
501+
// matched-page branch, which this path did not take.
502+
//
503+
// No `route` is passed on purpose: nothing matched, so there is no layout
504+
// chain to render the boundary inside, and the response stays a bare
505+
// document with no boot script.
506+
return ssrNotFound(state.routeTable.notFound || state.routeTable.globalNotFound, {
507+
dev,
508+
appDir,
509+
req,
510+
url,
511+
onError: reportError ? (e) => reportError(e, req, 'ssr') : undefined,
512+
onDevError: dev ? (e) => reportDevError(e, { kind: 'render', url: url.pathname }) : undefined,
513+
});
499514
}
500515

501516
/** @param {Request} req @param {string} path */

‎packages/server/src/ssr/render.js‎

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -668,15 +668,25 @@ async function ssrBoundaryHtml(file, heading, opts) {
668668
body = await renderToString(tree, { ssr: true, dev: opts.dev });
669669
}
670670
} catch (layoutErr) {
671-
// A wrapped layout threw. Degrade to the standalone render this has
672-
// always produced, and to its empty boot set with it. REPORT it
673-
// first: these paths execute layout modules for the first time since
674-
// #1298, so a genuine layout crash would otherwise vanish, leaving a
675-
// developer looking at a chrome-less boundary page with nothing
676-
// saying why, and an APM sink that never heard about it.
671+
// Degrade to the standalone render this has always produced, and to
672+
// its empty boot set with it.
673+
//
674+
// The standalone attempt is also what tells us WHAT failed, so it
675+
// runs before anything is reported. If it succeeds, the fault was in
676+
// the layout chain: report it, because these paths execute layout
677+
// modules for the first time since #1298 and a genuine layout crash
678+
// would otherwise vanish, leaving a developer looking at a
679+
// chrome-less boundary page with nothing saying why. If it throws
680+
// too, the boundary's OWN tree is what is broken, so rethrow and let
681+
// the outer catch report it once, under the right label. Reporting
682+
// here first would report a tree failure twice and call it a layout.
683+
try {
684+
body = await renderToString(tree, { ssr: true, dev: opts.dev });
685+
moduleUrls = [];
686+
} catch {
687+
throw layoutErr;
688+
}
677689
reportBoundaryLayoutError(layoutErr, opts, { overlay: true });
678-
body = await renderToString(tree, { ssr: true, dev: opts.dev });
679-
moduleUrls = [];
680690
}
681691
}
682692
} catch (e) {
@@ -737,10 +747,15 @@ async function ssrNotFoundHtml(notFoundFile, opts) {
737747
body = await renderToString(tree, { ssr: true, dev: opts.dev });
738748
}
739749
} catch (layoutErr) {
740-
// Same degradation, and the same reporting, as ssrBoundaryHtml above.
750+
// Same degradation, and the same report-only-if-the-fault-was-the-
751+
// chain rule, as ssrBoundaryHtml above.
752+
try {
753+
body = await renderToString(tree, { ssr: true, dev: opts.dev });
754+
moduleUrls = [];
755+
} catch {
756+
throw layoutErr;
757+
}
741758
reportBoundaryLayoutError(layoutErr, opts, { overlay: true });
742-
body = await renderToString(tree, { ssr: true, dev: opts.dev });
743-
moduleUrls = [];
744759
}
745760
}
746761
} catch (e) {

‎packages/server/test/routing/global-boundaries.test.js‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,3 +110,31 @@ test('global-error renders its OWN full document at 500 when a page throws a rea
110110
// It rendered its own <html>, returned verbatim (not double-wrapped).
111111
assert.equal((body.match(/<html/g) || []).length, 1, 'exactly one <html> (no double wrap)');
112112
});
113+
114+
/* ------------ boundary crashes reach the APM sink (#1298) ------------ */
115+
116+
test('an unrouted 404 whose not-found THROWS reaches the onError sink', async () => {
117+
// The wiring, not the capability: this goes through the real unmatched-URL
118+
// path in dev/serve.js rather than calling ssrNotFound directly. That path
119+
// used to build its own options object without the sinks, so a root
120+
// not-found that crashed reported to the console and nothing else.
121+
const appDir = makeApp({
122+
'package.json': pkg,
123+
'app/page.js': page('export default function H() { return html`<main>home</main>`; }'),
124+
'app/not-found.js':
125+
`export default function NF() { throw new Error('ROOT_NF_BOOM'); }\n`,
126+
});
127+
const captured = [];
128+
const app = await createRequestHandler({
129+
appDir, dev: true, onError: (e) => captured.push(e),
130+
});
131+
const prev = console.error;
132+
console.error = () => {};
133+
let resp;
134+
try {
135+
resp = await app.handle(new Request('http://x/nothing-here'));
136+
} finally { console.error = prev; }
137+
assert.equal(resp.status, 404, 'it still answers 404 rather than failing');
138+
assert.equal(captured.length, 1, 'the crash reached the APM sink through the real path');
139+
assert.match(String(captured[0].message), /ROOT_NF_BOOM/);
140+
});

‎test/ssr/ssr.test.js‎

Lines changed: 41 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1538,6 +1538,13 @@ test('preloadCrossOriginAttr: adds crossorigin=anonymous for cross-origin URLs o
15381538
assert.equal(preloadCrossOriginAttr('/__webjs/vendor/dayjs@1.11.20.js'), '');
15391539
});
15401540

1541+
/** Run `fn` with console.error muted: several cases provoke a real crash. */
1542+
const SILENT = async (fn) => {
1543+
const prev = console.error;
1544+
console.error = () => {};
1545+
try { return await fn(); } finally { console.error = prev; }
1546+
};
1547+
15411548
/* ------------ ssrNotFound + not-found.js rendering ------------ */
15421549

15431550
test('ssrNotFound: no notFound file → plain 404 fallback', async () => {
@@ -1569,13 +1576,15 @@ test('ssrNotFound: not-found.js that throws falls back to an inline error body',
15691576
writeFileSync(notFoundFile,
15701577
`export default function NotFound() { throw new Error('boom'); }\n`);
15711578

1572-
const dev = await ssrNotFound(notFoundFile, { dev: true, appDir: sub });
1579+
// SILENT: the boundary crash is REPORTED to console.error as of #1298, and
1580+
// this test provokes it deliberately.
1581+
const dev = await SILENT(() => ssrNotFound(notFoundFile, { dev: true, appDir: sub }));
15731582
assert.equal(dev.status, 404);
15741583
const devBody = await dev.text();
15751584
assert.ok(devBody.includes('404: Not found'));
15761585
assert.ok(devBody.includes('boom'), 'dev still shows the failure');
15771586

1578-
const prod = await ssrNotFound(notFoundFile, { dev: false, appDir: sub });
1587+
const prod = await SILENT(() => ssrNotFound(notFoundFile, { dev: false, appDir: sub }));
15791588
assert.equal(prod.status, 404);
15801589
const prodBody = await prod.text();
15811590
assert.ok(prodBody.includes('404: Not found'), 'prod still identifies the status');
@@ -1816,12 +1825,6 @@ function assertPaired(html) {
18161825
assert.equal(stack.length, 0, 'no marker left open');
18171826
}
18181827

1819-
const SILENT = async (fn) => {
1820-
const prev = console.error;
1821-
console.error = () => {};
1822-
try { return await fn(); } finally { console.error = prev; }
1823-
};
1824-
18251828
const HTML_IMPORT = `import { html } from ${JSON.stringify(HTML_MODULE_URL)};\n`;
18261829

18271830
/** Root layout + /docs layout + a throwing /docs/crash page + /docs/error.js. */
@@ -2462,6 +2465,36 @@ test('boundary: a 404 boundary that fails to LOAD is reported too', async () =>
24622465
assert.equal(seen.length, 1, 'a boundary that cannot even be imported is not silent either');
24632466
});
24642467

2468+
test('boundary: a boundary whose own TREE fails is reported ONCE, and not as a layout', async () => {
2469+
// The standalone fallback re-renders the SAME tree, so a tree-level failure
2470+
// throws twice: once from the wrapped attempt and once from the fallback.
2471+
// Reporting before the fallback ran would report it twice and label the
2472+
// first one a layout crash, when no layout threw at all.
2473+
const { route, appDir } = makeBoundaryApp({
2474+
files: {
2475+
'layout.js': HTML_IMPORT + `export default function Root({ children }) { return html\`<div>\${children}</div>\`; }\n`,
2476+
// Fails while the tree is RENDERED, not while it is built, so the
2477+
// wrapped attempt and the standalone fallback fail identically on the
2478+
// same tree. A rejected promise in a hole is the simplest such shape:
2479+
// renderToString awaits it.
2480+
'admin/forbidden.js': HTML_IMPORT + `export default function F() {
2481+
return html\`<p>\${Promise.reject(new Error('TREE_RENDER_BOOM'))}</p>\`;
2482+
}\n`,
2483+
'admin/page.js': `import { forbidden } from ${JSON.stringify(WEBJS_MODULE_URL)};\nexport default function Page() { forbidden(); }\n`,
2484+
},
2485+
page: 'admin/page.js',
2486+
layouts: ['layout.js'],
2487+
forbiddens: ['admin/forbidden.js'],
2488+
});
2489+
const seen = [];
2490+
const resp = await SILENT(() => ssrPage(route, {}, new URL('http://localhost/admin'), {
2491+
dev: false, appDir, onError: (e) => seen.push(e),
2492+
}));
2493+
assert.equal(resp.status, 403, 'it still answers with the boundary status');
2494+
const hits = seen.filter((e) => /TREE_RENDER_BOOM/.test(String(e && e.message)));
2495+
assert.equal(hits.length, 1, 'reported once, not once per render attempt');
2496+
});
2497+
24652498
test('boundary: a boundary response is never storable and never reduced', async () => {
24662499
// Two independent guarantees. The HTML cache refuses a non-200 outright, and
24672500
// the reduced X-Webjs-Have path is structurally unreachable: the boundary

0 commit comments

Comments
 (0)