Skip to content

Commit 641dca1

Browse files
committed
fix: diagnose a boundary failure by the fallback, not the marker alone
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.
1 parent c0c978c commit 641dca1

4 files changed

Lines changed: 119 additions & 24 deletions

File tree

packages/server/src/dev/serve.js

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -503,7 +503,15 @@ export async function handleCore(req, ctx) {
503503
// No `route` is passed on purpose: nothing matched, so there is no layout
504504
// chain to render the boundary inside, and the response stays a bare
505505
// document with no boot script.
506-
return ssrNotFound(state.routeTable.notFound || state.routeTable.globalNotFound, {
506+
// Same supersede rule the matched-page branch follows (#1047 / #893): a
507+
// LATER successful render of this same url clears the frame it retained, so
508+
// an intermittently-failing not-found (a query behind it, not a source edit,
509+
// which the rebuild already clears) cannot leave a frame that paints over a
510+
// page that has since recovered. Scoped to this url, so a good render here
511+
// never erases an error still current for a page the user is looking at.
512+
const devErrorUrl = withBasePath(url.pathname, basePath()) + url.search;
513+
const before = dev ? state.lastDevError : null;
514+
const resp = await ssrNotFound(state.routeTable.notFound || state.routeTable.globalNotFound, {
507515
dev,
508516
appDir,
509517
req,
@@ -524,12 +532,14 @@ export async function handleCore(req, ctx) {
524532
// `state.lastDevError` for the SSE to replay into a fresh tab. Prefetch is
525533
// on by default and fetches unrouted hrefs too, so this is reachable.
526534
onDevError: dev && req.headers.get('x-webjs-prefetch') !== '1'
527-
? (e) => reportDevError(e, {
528-
kind: 'render',
529-
url: withBasePath(url.pathname, basePath()) + url.search,
530-
})
535+
? (e) => reportDevError(e, { kind: 'render', url: devErrorUrl })
531536
: undefined,
532537
});
538+
if (
539+
before && state.lastDevError === before
540+
&& before.kind === 'render' && before.url === devErrorUrl
541+
) state.lastDevError = null;
542+
return resp;
533543
}
534544

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

packages/server/src/ssr/render.js

Lines changed: 38 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -701,24 +701,36 @@ async function ssrBoundaryHtml(file, heading, opts) {
701701
body = await renderToString(tree, { ssr: true, dev: opts.dev });
702702
}
703703
} catch (layoutErr) {
704-
// A TREE failure is not this catch's business: the standalone
705-
// fallback would re-render the same tree and fail the same way, so
706-
// rethrow and let the outer catch report it ONCE under the boundary
707-
// label. Reporting here as well would double-report it and call it a
708-
// layout crash.
709-
if (!isLayoutPhase(layoutErr)) throw layoutErr;
710-
// A wrapped layout threw. Degrade to the standalone render this has
711-
// always produced, and to its empty boot set with it. Report it
712-
// either way: these paths execute layout modules for the first time
713-
// since #1298, so a genuine layout crash would otherwise vanish.
714-
reportBoundaryLayoutError(layoutErr, opts, { overlay: true });
704+
// Always attempt the standalone render, which is the degradation
705+
// this path has always produced. Its OUTCOME is half the diagnosis
706+
// and the phase marker is the other half; neither alone is enough.
707+
//
708+
// The marker records a layout that threw when it was CALLED. It
709+
// cannot record a layout that fails while being SERIALIZED (the
710+
// idiomatic `html`<nav>${getNav()}</nav>${children}`` with a
711+
// rejecting hole), because by then its output is fused into one tree
712+
// with the boundary's and `renderToString` cannot say which half
713+
// threw. That is what the fallback answers: if the boundary renders
714+
// alone, the fault was layout-side whether or not it was marked.
715+
let treeErr = null;
715716
try {
716717
body = await renderToString(tree, { ssr: true, dev: opts.dev });
717718
moduleUrls = [];
718-
} catch (treeErr) {
719-
// BOTH are broken. The layout crash is already reported above, so
720-
// hand the outer catch the TREE error: it is a second, distinct
721-
// failure, and it is the one whose text the dev body should show.
719+
} catch (e) {
720+
treeErr = e;
721+
}
722+
if (treeErr === null) {
723+
// The boundary is fine on its own, so the chain is what failed.
724+
reportBoundaryLayoutError(layoutErr, opts, { overlay: true });
725+
} else {
726+
// The boundary's own tree is broken too. Report a MARKED layout
727+
// failure as the separate thing it is, then hand the outer catch
728+
// the tree error: it is what defeated the fallback and what the
729+
// dev body should show. An UNMARKED one is not reported here,
730+
// because it may be this same tree error arriving twice.
731+
if (isLayoutPhase(layoutErr)) {
732+
reportBoundaryLayoutError(layoutErr, opts, { overlay: false });
733+
}
722734
throw treeErr;
723735
}
724736
}
@@ -781,13 +793,20 @@ async function ssrNotFoundHtml(notFoundFile, opts) {
781793
body = await renderToString(tree, { ssr: true, dev: opts.dev });
782794
}
783795
} catch (layoutErr) {
784-
// Same phase rule as ssrBoundaryHtml above.
785-
if (!isLayoutPhase(layoutErr)) throw layoutErr;
786-
reportBoundaryLayoutError(layoutErr, opts, { overlay: true });
796+
// Same diagnosis rule as ssrBoundaryHtml above.
797+
let treeErr = null;
787798
try {
788799
body = await renderToString(tree, { ssr: true, dev: opts.dev });
789800
moduleUrls = [];
790-
} catch (treeErr) {
801+
} catch (e) {
802+
treeErr = e;
803+
}
804+
if (treeErr === null) {
805+
reportBoundaryLayoutError(layoutErr, opts, { overlay: true });
806+
} else {
807+
if (isLayoutPhase(layoutErr)) {
808+
reportBoundaryLayoutError(layoutErr, opts, { overlay: false });
809+
}
791810
throw treeErr;
792811
}
793812
}

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,3 +187,38 @@ test('a PREFETCH of an unrouted url raises no dev overlay frame (#1047)', async
187187
assert.equal(frames.length, 0, 'no overlay frame from a speculative fetch');
188188
assert.equal(errors.length, 1, 'but the APM sink still hears about it, since the render really threw');
189189
});
190+
191+
test('a recovered unrouted 404 clears the frame it retained (#1047 supersede)', async () => {
192+
// An intermittently-failing not-found must not leave a frame that paints
193+
// over the same url once it renders again. A source edit is cleared by the
194+
// rebuild; this is the other route in, and it only became reachable once the
195+
// frame stamp was correct enough for the overlay to accept it.
196+
const appDir = makeApp({
197+
'package.json': pkg,
198+
'app/page.js': page('export default function H() { return html`<main>home</main>`; }'),
199+
// Fails on the first render of a url, succeeds afterwards.
200+
// globalThis, not module scope: dev re-imports the module per request with
201+
// a cache-bust query, so module-level state resets every time.
202+
'app/not-found.js': `import { html } from ${JSON.stringify(CORE)};
203+
export default function NF() {
204+
if (!globalThis.__flakyNfSeen) { globalThis.__flakyNfSeen = true; throw new Error('FLAKY_NF_BOOM'); }
205+
return html\`<main>missing</main>\`;
206+
}
207+
`,
208+
});
209+
const app = await createRequestHandler({ appDir, dev: true });
210+
const prev = console.error;
211+
console.error = () => {};
212+
try {
213+
const first = await app.handle(new Request('http://x/gone?q=1'));
214+
assert.equal(first.status, 404);
215+
const second = await app.handle(new Request('http://x/gone?q=1'));
216+
assert.equal(second.status, 404);
217+
assert.match(await second.text(), /missing/, 'the second render succeeded');
218+
} finally { console.error = prev; }
219+
// Observed directly, not through a probe endpoint that might not exist: a
220+
// conditional assertion here would pass whether or not the fix works.
221+
assert.equal(typeof app.getLastDevError, 'function', 'the handler exposes the retained frame');
222+
const held = app.getLastDevError();
223+
assert.equal(held, null, 'no stale frame retained for a url that recovered');
224+
});

test/ssr/ssr.test.js

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2525,6 +2525,37 @@ test('boundary: when the layout AND the tree are broken, BOTH are reported', asy
25252525
assert.match(await resp.text(), /TREE_HALF_BOOM/);
25262526
});
25272527

2528+
test('boundary: a layout failing in a TEMPLATE HOLE still degrades to the boundary', async () => {
2529+
// The idiomatic async layout: it returns fine and fails while being
2530+
// serialized. The phase marker cannot see this, because by then the layout's
2531+
// output is fused into one tree with the boundary's, so the FALLBACK is what
2532+
// diagnoses it: the boundary renders alone, therefore the chain was at fault.
2533+
// Getting this wrong serves a bare heading instead of the boundary page.
2534+
const { route, appDir } = makeBoundaryApp({
2535+
files: {
2536+
'layout.js': HTML_IMPORT + `export default function Root({ children }) {
2537+
return html\`<nav>\${Promise.reject(new Error('LAYOUT_HOLE_BOOM'))}</nav>\${children}\`;
2538+
}\n`,
2539+
'admin/forbidden.js': HTML_IMPORT + `export default function F() { return html\`<p id="fb">no access</p>\`; }\n`,
2540+
'admin/page.js': `import { forbidden } from ${JSON.stringify(WEBJS_MODULE_URL)};\nexport default function Page() { forbidden(); }\n`,
2541+
},
2542+
page: 'admin/page.js',
2543+
layouts: ['layout.js'],
2544+
forbiddens: ['admin/forbidden.js'],
2545+
});
2546+
const seen = [];
2547+
const resp = await SILENT(() => ssrPage(route, {}, new URL('http://localhost/admin'), {
2548+
dev: false, appDir, onError: (e) => seen.push(e),
2549+
}));
2550+
assert.equal(resp.status, 403);
2551+
const body = await resp.text();
2552+
assert.ok(body.includes('id="fb"'), 'the boundary page is served, not a bare heading');
2553+
assert.equal(markersOf(body).length, 0, 'chrome-less, since the chain could not render');
2554+
assert.ok(!body.includes('<script type="module">'), 'and no boot set for a chain that did not render');
2555+
const hits = seen.filter((e) => /LAYOUT_HOLE_BOOM/.test(String(e && e.message)));
2556+
assert.equal(hits.length, 1, 'the layout failure is reported exactly once');
2557+
});
2558+
25282559
test('boundary: a boundary response is never storable and never reduced', async () => {
25292560
// Two independent guarantees. The HTML cache refuses a non-200 outright, and
25302561
// the reduced X-Webjs-Have path is structurally unreachable: the boundary

0 commit comments

Comments
 (0)