fix: scope the dev error overlay to the URL that produced it - #1294
Conversation
In dev, the Server render error overlay leaked onto pages that render fine. Two independent causes. The overlay is appended to document.body while the client router swaps strictly inside the keyed boundary comment ranges, so it outlived every soft navigation. And link prefetch, on by default, fires a real GET of a throwing page on hover, which reports a frame to every open tab, so the overlay appeared without any navigation at all. A render frame now carries the URL that produced it, and the browser overlay is the single gate deciding whether a frame belongs on the page being viewed. A prefetch render reports nothing, and a good render of a URL supersedes a retained error for that same URL so the SSE replay cannot resurrect it in a new tab. The gate cannot simply drop a foreign frame: the SSE frame is pushed during the render, before the navigation response is sent, so it lands while location is still the old page. A refused frame is held and re-evaluated once the URL advances, and is consumed by the first navigation after it arrives.
Both halves of #1047 are only observable in a browser. Hovering a link to a throwing page fires a real prefetch GET with no navigation at all, which no server-side test can reproduce, and whether the overlay survives a client-router swap is a DOM fact. The fixture reaches a live overlay honestly, by breaking a page the browser is already sitting on and re-rendering it out of band, the way another tab or a background revalidation would. A page whose render throws is served without the layout chain, so navigating INTO one always degrades to a document load and could never exercise the soft-nav path.
Each listener shell replays the retained dev error frame with its own code (node:http writes into a res, Bun.serve enqueues into a ReadableStream controller), and the change edits the SSR dispatch path, so the frame's new url field and the prefetch exemption have to hold on both runtimes rather than on the one the suite happens to run under.
framework-dev.md and the docs site both said only that the overlay dismisses on the next successful rebuild, which is now wrong for a render frame. The server AGENTS.md module map also had no dev-overlay.js row at all, so the browser half of the overlay was undocumented.
The counterfactual run showed the case still passes with the webjs:before-cache handler removed, so its comment was claiming coverage it does not have. The handler is proven in the browser test; the e2e asserts the outcome.
|
Design rationale: why the gate holds a refused frame instead of dropping it The obvious shape for this is "refuse a render frame whose url is not the current url, dismiss anything stale". That shape is wrong, and it took a run through the ordering to see why. The SSE frame is written inside the SSR catch ( So there are two slots: the frame currently rendered, and the most recent one the gate refused. A refused frame is HELD, and the nav sync re-evaluates it once the URL advances. It is consumed by the first navigation after it arrives, matching (so it renders) or not (so it is dropped). That bounds retention without a timer, and it is honest about the timing: a pending frame and the navigation it belongs to are milliseconds apart, so "the next navigation decides" is not a heuristic, it is the actual relationship. Two smaller things the same reasoning turned up:
Precedent for the general approach: Next keeps overlay state in a reducer with an explicit supersede action, and needed the |
|
Finding along the way: an error render is served without its layout chain, so navigating into one always full-loads While building the e2e I assumed I could soft-navigate INTO the throwing page and assert the overlay appears there. It never worked, and the reason is not in this diff.
So today every navigation to a page whose render throws is a full document load, boundary or not. Nested error boundaries not rendering inside their layouts is a real difference from Next, and it also means the "frame arrives before the swap" ordering is not reachable by navigating into a broken page. It IS reachable the other way, when a page you are already on starts failing (another tab, a background revalidation), which is what the e2e fixture drives with its Nothing here changes that behaviour. Flagging it because it is load-bearing for how the e2e is written, and because it is a much bigger call than this fix. |
The interesting assertion is a negative one (no error frame was replayed), and in that case the stream goes quiet after the hello frame, so a bare reader.read() blocked until the next keepalive. That tripped bun test's per-test timeout and turned a passing assertion into a hang in the Bun matrix. Each read now races a deadline; the script runs in under two seconds on both runtimes.
Two defects in the scope gate, both from treating webjs:before-cache as a back/forward signal. It is not. The router dispatches it from snapshotCurrent, which runs at the top of EVERY navigation and form submission. So stripping the overlay there, which the event's own contract invites, tore a rebuild or ts-strip overlay off the page the instant you clicked any link, while the build was still broken and nothing would put it back. That is the opposite of the invariant this change documents, and the browser test missed it by calling the sync directly rather than going through the event. Being the nav-START signal is what before-cache is actually good for, and it closes the second defect. A held frame had no way to be superseded, so one that arrived while the tab sat idle (a link prefetch, another tab's render) would paint on a later visit to that url even if the page rendered perfectly by then, which is the very symptom the gate exists to stop. A held frame now records which navigation was in flight when it landed and renders only if that is still the one finishing.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Went looking for whatever the URL gate gets wrong, and both things I found are in the same place: I treated webjs:before-cache as a back/forward signal when it is nothing of the sort.
The server half reads well. Stamping the frame through withBasePath with the raw pathname is right, dropping the hook on a prefetch is the correct place to cut it, and keying the supersede clear on frame identity AND url is exactly the care that rule needs. The browser half is where the thinking was sloppy, in a way the tests did not catch because one of them exercises a path the real router never takes.
Comments inline.
The router's tier-4 popstate restore replaceChildren's the body straight from its snapshot, and that snapshot is outerHTML taken while an overlay may have been on screen, so it reinserts a parsed COPY. The copy is not the node this module holds, so nothing could ever remove it and its Dismiss button carries no listener: an undismissable card. The sync now sweeps any overlay element it does not own, notices when its own node was replaced out from under it, and re-renders a real one when the frame is still current. Also drives the idle-held-frame test through the installed listeners rather than calling the nav-start marker by hand, since before-cache is the only thing that bumps the seq in the shipping client and the test would otherwise pass with that wiring deleted. And drops the link prefetch as the example of an idle-time frame everywhere it appeared: a prefetch reports no frame at all, so it cannot produce one, and citing it made the two halves of the fix look mutually redundant.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Second read, scoped to the fix commit. It holds up, but it left the same hole it was fixing: the new nav-start marker is the thing the whole seq rule rests on, and every test called it by hand, so the shipping before-cache wiring could be deleted with the suite green. That is exactly what I criticised the previous code for.
Two more. Taking the DOM strip out of before-cache reopened the snapshot leak for real, and it is reachable through the tier-4 popstate restore, which replaceChildrens the body from the cached HTML. And I kept citing a link prefetch as the canonical idle-time held frame in four places, which this same PR makes impossible, since a prefetch reports no frame at all.
Comments inline.
The sweep alone cannot cover every ordering. Non-VT the router's tier-4 body swap runs synchronously inside its popstate handler, before the reload client's listener, so the sync sees the reinserted copy. With an opt-in view transition the swap is deferred past the sync, and the revalidation that follows a snapshot restore discards without dispatching webjs:navigate, so the copy sat there undismissable until the next manual navigation. Fixed at the source instead: before-cache detaches the overlay across the router's synchronous outerHTML read and re-attaches the same node a microtask later, so the cached HTML never carries a copy and the overlay on screen never flickers. Distinct from the strip this replaced, which dropped the frame for good and tore down rebuild overlays. The sweep stays as the backstop for a copy from anywhere else. Also stops crediting the URL gate with fixing the prefetch case in the last three places that still did. The server-side guard in dev.js is what cuts that; the gate covers any render of a page this tab is not looking at.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Third read, scoped to the sweep commit. Two problems, and one of them means the sweep does not actually fix the case it was added for.
The sweep is correctly placed for the ordinary path: the router's tier-4 body swap runs synchronously inside its own popstate handler, and the router's listener is registered before the reload client's, so by the time the sync runs the copy is already in the DOM. Under an opt-in view transition that stops being true, and there is no second sync to catch it.
The other one is the same stale prefetch attribution I thought I had cleaned up, still sitting in three places including the docstring of the file that commit edited.
Comment and doc text only, no behaviour change. Three stale rationales the previous commits left behind. The server AGENTS.md row had the two mechanisms inverted, and the example it gave proves the inversion: under a view transition the router defers the body swap PAST the sync, so the sweep cannot cover that ordering at all. The detach is what covers it. The sweep's own comment still named the router's snapshot as its reason, which the detach now prevents, so it reads as a backstop for everything else instead. And the dev-error.js JSDoc still listed a link prefetch as something the frame url lets the overlay refuse; no frame is ever built for a prefetch, so there is nothing to refuse.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Fourth read, scoped to the detach commit. The mechanism itself holds: I traced both listener orders for popstate, forward nav, form submit, and frame nav, and the __wjOverlay === el guard correctly suppresses a stale re-attach when the sync re-rendered in the same task. queueMicrotask provably runs after snapshotCurrent's synchronous outerHTML read.
What is wrong is documentation the commit itself rewrote, in three places. Nothing executable.
The prefetch e2e awaited a bare promise with no deadline, and node --test has no default per-test timeout, so a miss would have stalled CI until its own ceiling rather than failing. It is reachable: the prefetch strategy is device-adaptive, and on the viewport branch the already visible link can be prefetched during load, before a listener attached afterwards exists. The listener now goes on before the navigation and the wait fails on a deadline, in five seconds rather than never. The Bun script's comment claimed a same-url supersede assertion it never makes; that half needs a page that recovers, which the fixture's unconditional throw cannot do, and it is covered on the Node path. And the fixture layout typed children as unknown, which is the exact shape AGENTS.md's derive-the-type rule names.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Final pass over the whole diff, on the head the fix rounds produced.
The mechanism is sound. I checked the boot-vs-reload script order in ssr.js to confirm the router's popstate listener really is registered before the dev client's, so the nav-seq bump precedes the sync on a back/forward; the detach and re-attach ordering, the pending slot's consume-once, and the supersede clear's identity-plus-url keying all hold.
Nothing wrong in the shipping code. Three things in the tests and one fixture.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Checked the last round's fixes on their own. The bounded wait is genuinely bounded and nothing else in the PR carries an unbounded await; the reachability I claimed for the viewport-branch prefetch holds against the router's dwell timings; the same-url supersede really is covered on the Node path and the clear lives in shared code rather than either listener shell, so the Bun script's corrected comment is accurate; and LayoutProps strips cleanly in the fixture.
Nothing left open.
|
Follow-up: the error-boundary layout gap is now tracked Filed as #1298. Nothing in this PR changes it; the overlay fix works either way. It is here because the e2e fixture and its comments are written around the limitation, and they will need correcting when that lands. |
1 participant
Closes #1047
Summary
In dev, the "Server render error" overlay leaked onto pages that render fine, reliably around the scaffold's
boundariesgallery pages. Two independent causes, and both had to go or the symptom stays.renderDevOverlayappends todocument.bodywhile the client router's two-tier swap operates strictly inside the keyed boundary comment ranges, so the overlay outlived every soft navigation. And link prefetch, on by default, fires a real GET of a throwing page on hover, which server-renders it, reachesreportDevError, and fans the frame out to every open tab over the shared SSE channel, so the overlay appeared with no navigation at all.A
renderframe now carries the URL that produced it, and the browser-side overlay is the single gate deciding whether a frame belongs on the page currently being viewed. One rule covers the navigation half, the prefetch half, and the multi-tab case. Two supporting server changes ride along: a speculative prefetch render reports nothing, and a successful render of a URL supersedes a retained error for that same URL so the SSE replay cannot resurrect it in a new tab.The gate is not a plain refuse-and-drop, and the reason is in the ordering: the SSE frame is pushed during the render, before the navigation response is even sent, so it reaches the browser while
locationis still the old page. Dropping it would lose the overlay on the very page that threw. A refused frame is held pending and re-evaluated once the URL advances.A held frame renders only for the navigation it actually belongs to. One that arrives while a navigation is in flight is that navigation's; one that arrived while the tab sat idle (a link prefetch, another tab's render) is not, and must never paint on a later visit to that url, because by then the page may well render fine and an overlay over it would be this bug all over again.
webjs:before-cacheis the nav-START marker that separates the two, since the router snapshots the page it is leaving before it fetches.Two smaller consequences: the gate runs BEFORE any removal, so a refused frame cannot wipe a live
rebuild/ts-stripoverlay; and the path comparison is encoding-tolerant, because a mismatch fails closed and would otherwise hide a genuine error on a percent-encoded dynamic segment.packages/coreis untouched. The sync rides the existingwebjs:navigate,popstate, andwebjs:before-cacheevents, so core stays free of dev-overlay knowledge and the whole fix lives in@webjsdev/server.ts-stripandrebuildframes carry no url and are deliberately never scoped: they describe a still-broken build rather than one page, so navigation leaves them alone and only the next successful rebuild clears them. That constrains whatbefore-cachemay do, since it fires on EVERY navigation: stripping the overlay there, which the event's own contract invites, would tear a rebuild overlay off the page on any link click while the build was still broken. So it detaches the node across the router's synchronousouterHTMLread and re-attaches the SAME node a microtask later, which keeps the overlay out of the back/forward snapshot without ever dropping it. Without that, a restore reinserts a parsed copy this module does not own, whose Dismiss button has no listener; the sync also sweeps any such element as a backstop.Test plan
packages/server/test/dev/dev-error.test.js: the frame carries an expliciturl,nullwithout one.packages/server/test/dev/dev-error-overlay.test.js: the stamped url covers path + query; anx-webjs-prefetch: 1render reports nothing and leavesgetLastDevError()untouched; a good render of the same url supersedes the retained frame while an unrelated one leaves it standing; the url carrieswebjs.basePath.packages/server/test/dev/reload-shared-connection.test.js: the emitted reload client inlines the nav sync, installs it, survives theexportstrip, and parses.packages/server/test/dev/browser/dev-overlay.test.js, 19 tests on chromium, firefox, and webkit, driving the shipping module: the scope gate, the held-frame ordering, an idle-time frame not painting on a later visit, the rebuild-overlay carve-out (through the real event order, so thebefore-cacheregression is caught), the overlay being out of the DOM at the exact point the router readsouterHTML, a body-replacement copy being swept, manual dismiss not resurrecting, encoding tolerance, and all three wired events.test/e2e/dev-overlay-nav.test.mjs, 5/5: the page that threw still shows the overlay, a soft nav away takes a live one with it, back/forward does not resurrect it, one tab's render error stays out of another, and hovering a link to a throwing page raises nothing.test/bun/dev-overlay-scope.{mjs,test.mjs}: green on node 26.1.0 and bun 1.3.14, and in the Bun matrix (284 pass, 1 genuine fail which is the pre-existinglistener.test.mjs)./,/docs/error-handling,/ui, and/ui/buttonwith no broken modulepreload hints; the redirect hosts pass their mapping tests.Every remaining node/Bun failure was verified pre-existing by swapping in
origin/main'sdev.jsand getting the identical result: the threedifferential-elisioncases, andtest/bun/listener.mjs(clientIp; got "_anon_"). The blog smoke failures were a worktree with no migratedexamples/blogdatabase and pass once it is seeded.Final numbers on the head being merged: node 3889 pass / 5 fail (that same pre-existing set), browser 752 + 742 + 752 pass with 0 fail, e2e 5/5, the Bun matrix 284 pass with 1 genuine fail (
listener.test.mjs, pre-existing), the Bun parity script green on node 26.1.0 and bun 1.3.14, and the website booting 200 on/,/docs/error-handling,/ui, and/ui/buttonwith no broken modulepreload hints.Counterfactuals
Each piece was proven load-bearing by neutering it, re-running, and restoring:
renderDevOverlayinstallDevOverlayNavSync()callwebjs:before-cachewiringbefore-cachedetachbefore-cache(the reviewed defect, restored)That last run showed the back/forward e2e was not discriminating the
before-cachehandler, so its comment was corrected rather than left claiming coverage it does not have.Doc surfaces
framework-dev.md, the "Dev error overlay" section, which said only that a successful rebuild clears it.website/app/docs/error-handling/page.ts, the same correction in user-facing copy.packages/server/AGENTS.md: thedev-error.jsanddev.jsrows, plus a newdev-overlay.jsrow (the module map had none, so the browser half of the overlay was undocumented).boundaries/crashdemo is deliberately unchanged, since the overlay showing on that page is correct.