fix: stop rapid dev edits leaving the page unstyled - #1400
Conversation
|
Design rationale: why the debounce is trailing-only, browser-side, and capped Three choices here are easy to get wrong in the other direction, so writing down why each went the way it did. Trailing, never leading. A leading-edge debounce (fire immediately, suppress the rest) is the usual reflex and it is exactly wrong here. The first signal of a burst is emitted by a process Browser-side, not server-side. Under Capped, not a pure quiet window. A quiet window alone freezes the page on stale content for the whole burst, and an agent burst can run for a minute. The cap bounds that. It is armed once per batch and never re-armed, which is the subtle half: re-arming it on each signal would let it slide out with the burst forever, so it would never fire and the cap would not exist. There is a test aimed squarely at that, and re-arming makes three cases go red. On the numbers: 2000ms is above the measured 1071ms inter-save gap so a realistic burst collapses, and deliberately not near it, since a window close to that gap fires just as the next restart begins, which is the worst possible phase. It is also under the 1900ms analysis warm measured on the website app, so on a real app the wait overlaps work the reload request would have blocked on anyway. Turbo and Vite both debounce a reload signal this way (Turbo at 150ms, Vite at 20ms), but neither carries a cap, because in both cases the coalesced signal is cheap and the burst is short. Here the signal costs a full page reload gated on a process restart, so the cap is the addition rather than something to copy. |
|
Correction found while verifying: the traversal test was observing nothing Worth recording, because the guard is a security check and the test that covers it was tautological before this PR touched it. The moved-in comment claimed The WHATWG URL parser decodes The vector that does reach the guard encodes the SLASH: The guard itself was always correct, so there is no live vulnerability here and nothing to disclose; what was wrong was the comment describing it and the assertion covering it. Both are fixed in |
vivek7405
left a comment
There was a problem hiding this comment.
Read the whole diff fresh. The debounce logic itself holds up: I traced the two-timer emitter against all five new coalescing cases and the cap-not-re-armed counterfactual, and the tryServePublicAsset extraction preserves prod behaviour exactly, return null fall-through and containment short-circuit included. The early serve sits inside produce() after the base-path ingress strip, so security headers, conditional GET and the access log still apply. The ..%2F correction to the traversal vector is a genuine improvement on what the plan specified.
Five things to fix, two of which matter more than they look.
The shim port is the real one. Routing the per-tab fallback through the shared relay is the right call, but fanout deletes a port when postMessage throws, and that heuristic was written for a real MessagePort where a throw means the tab is gone. On a shim whose postMessage synchronously runs the overlay renderer, an application throw now permanently unsubscribes the tab. The old fallback attached to the EventSource directly, where a handler throw detached nothing, so this is a behaviour regression the port-sharing bought us.
The ?v= test is the other. It says "on the early path" and builds the handler with dev: false, and the early call is inside if (dev), so it never touches the path it names. It cannot be written the other way either, since fileResponse hard-codes no-cache whenever dev is true. The coverage is real, the claim about what it covers is not.
The rest is doc drift the PR opened and then half-closed. Correcting the middleware prose while leaving two code-block comments two lines below saying the opposite is worse than not having touched it, and the same claim is still sitting on the agent-facing surface the scaffold ships.
Two findings sit outside the diff so I could not anchor them inline:
.agents/skills/webjs/references/muscle-memory-gotchas.md:205says a rootmiddleware.ts"runs on every request", the identical claim this PR judged inaccurate enough to correct on the docs site. That file ships to every scaffolded app, so it is the surface where the stale version does the most damage.- Root
AGENTS.md:188has the same wording in the app-layout table. Terser and lower stakes, but it is the same fact.
|
Round 1 findings outside the diff, both fixed Two of the five findings sat on files this PR had not touched, so they could not be anchored inline. Recording the resolution here so the trail is complete.
Grepping for the claim turned up a third instance the review did not name, in Fixed in |
vivek7405
left a comment
There was a problem hiding this comment.
Delta pass over the previous round's fix commit, tracing what it touched across the rest of the PR. Four things, and two of them are the fix commit making a claim that is not true.
The empty catch is the substantive one. Guarding the shim port against detachment was right, but discarding the error is not the same as what the old fallback did: a throw out of an EventSource listener reaches the console, and so does one out of the SharedWorker path's onmessage ten lines below. So that catch quietly made the fallback the one path where a dev-overlay bug leaves no trace, which is a new behaviour dressed up as a restored one.
The slice is the embarrassing one. It was added specifically to stop the guard assertion matching the bootstrap's try/catch, and it ends at indexOf('if (typeof SharedWorker'), which still trails a closing brace and try {. So the bootstrap's try is inside the slice and the first assertion still passes with the guard removed. The comment above it claims a counterfactual was run and held. The other two assertions do discriminate, so the test as a whole was not vacuous, but the assertion the comment is about was.
Then doc drift the fix commit created rather than closed: it corrected the middleware claim on four surfaces and left the architecture page's Request Lifecycle saying root middleware runs first with internal endpoints and static files after it, so two pages on this site now contradict each other on exactly the fact the PR exists to correct.
|
Delta round: two findings outside the fix commit's own hunks
Also fixed the brand casing in Fixed in |
vivek7405
left a comment
There was a problem hiding this comment.
Delta pass over 82e1fd28. Three findings, and two of them are that commit breaking something it had just fixed.
The indexOf comparison is the one worth internalising. indexOf returns -1 when the needle is missing, and -1 is less than any real index, so indexOf('try {') < indexOf('__webjsReloadWhenReady()') PASSES when the guard is gone entirely, which is the only case the assertion exists for. That is the third vacuous assertion on this PR and the second one in this same test, added by the commit whose stated purpose was removing vacuous assertions from that slice. The other assertions there do catch the removal, so the file still goes red, but this one contributes nothing.
The comment contradiction is the same shape. The catch was changed to report, and the comment eight lines above it still ended with "swallowing here restores that behaviour rather than adding a new one", which is now false on both halves. Two adjacent comments about the same line, disagreeing, and a reader hits the wrong one first.
The architecture-page finding is the one with real consequences. Hoisting a claim about /__webjs/* into step 2 swept in the server-action RPC endpoint, which is dispatched inside handleCore and therefore runs AFTER root middleware. So the page told anyone gating actions with auth or rate-limit middleware that their middleware does not run for actions, which is both wrong and exactly backwards from the direction you would want to be wrong in. The endpoint had also dropped out of the lifecycle list entirely.
|
Delta 2: the architecture-page step 2 was wrong in the dangerous direction This one could not be anchored inline (the file is outside the PR diff), and it is the finding from this round with real consequences, so recording it in full. Correcting the Request Lifecycle in the previous commit, I hoisted a claim about Step 2 now names the set it actually covers rather than a wildcard: the health, readiness and version probes, the core runtime, the dev reload client and its SharedWorker, and downloaded vendor bundles, all verified against Fixed in |
vivek7405
left a comment
There was a problem hiding this comment.
Delta pass over 12b34db4. The code and test hunks in that commit hold up: the shim comment now matches what the emitted client does, the slice really does stop at the fallback function's brace, and the -1 fix is right. All three findings are in the third part of it, the architecture page.
The pattern is the finding, more than any single item. Three rounds running, I have written a CLOSED enumeration of what bypasses root middleware, and three rounds running the next reviewer has found another member. This round it is two: a WebSocket upgrade bound for a route.ts exporting WS, and the dev SSE stream at /__webjs/events. Both are intercepted by the listener shell before the app handler is called, on the node and Bun shells alike. The WS one is the sharper miss, because route.ts is unambiguously an app route and "every APP request" was the exact wording chosen to draw that line.
So the fix is not a third member added to the list. The docs now state the RULE, that anything the listener shell or the pre-analysis stage answers bypasses root middleware and everything routed with the app reaches it, and demote the enumeration to examples. That is the only version of this that cannot be wrong again next round.
The step-reference break is the same brittleness in a different form: inserting one <li> shifted the list under a "Step 6 above" reference elsewhere on the page, which then pointed at segment middleware instead of the SSR pipeline. Now referenced by name, so a future insert cannot break it.
|
Delta 3: why the middleware exception list became a rule All three findings this round were on Two of them were additional members of the bypass list: a WebSocket upgrade bound for a I have now written this enumeration three times and had a reviewer find a missing member three times. That is enough evidence that the enumeration is the wrong shape, not that I keep picking the wrong members. All three surfaces now state the rule, anything the listener shell or the pre-analysis stage answers bypasses root middleware and everything routed with the app reaches it, with the specific paths kept as illustrations rather than as a closed set. A reader who internalises the rule gets the right answer for a case none of us listed. The third finding is the same brittleness in miniature: inserting one step into the lifecycle shifted a "Step 6 above" reference further down the page onto the wrong item. It refers to the SSR pipeline by name now. Fixed in |
vivek7405
left a comment
There was a problem hiding this comment.
Delta pass over 5d1b8fdb. The rule-over-list rewrite holds up, and the WS, SSE, framework-static, dev and prod /public/* and server-action claims all check out against both listener shells. Two more ordering facts were wrong.
webjs.redirects and webjs.trailingSlash are resolved ahead of the probes and well ahead of root middleware, so a 308 from a redirect rule is answered with the app's own middleware never running. The rule I wrote last round does technically cover them, and that is not good enough: it only works if a reader classifies a config key they wrote themselves as a "framework pre-analysis stage", which is precisely the guess that goes the wrong way. Stating a rule does not excuse leaving the one case the rule is counter-intuitive for unnamed. Called out by name on all three surfaces now, with the practical consequence (a logging or auth middleware does not see those requests) and what to do instead.
103 Early Hints were listed after root middleware and are emitted before it, from inside the node listener shell ahead of the app handler. That is the same listener-shell step I restructured the list around last round, so it should have moved with the WS step and did not. Also node-only rather than prod-only, since Bun.serve has no informational-response API, which now matters because the neighbouring steps name both shells explicitly.
Both findings are on the architecture page, outside the diff, so no inline anchors.
vivek7405
left a comment
There was a problem hiding this comment.
Delta pass over cc3e695d. Both ordering claims in it check out against the runtime: applyRedirects and applyTrailingSlash do run in produce() ahead of ensureReady() and root middleware, and 103 Early Hints really are emitted in the node listener shell before app.handle(), so moving that bullet up was a genuine fix.
One problem. The step I added was headed "Declarative rewrites from your webjs config", and neither key rewrites anything: webjs.redirects replies with a configured redirect status and a Location header, and webjs.trailingSlash 308-redirects the non-canonical form to the canonical one. A rewrite is a distinct concept the audience for this page brings with them from Next, where it proxies internally with no client-visible redirect, and there is no webjs.rewrites, so the heading invented a feature. A bold step name is the most-scanned token in an ordered list, and every other surface in this PR already says redirect, so it was the odd one out as well as the wrong one. Fixed in 29305bc1.
This is the fifth delta round, so the chain stops here rather than buying a sixth. That fix is on the branch unreviewed, and the PR stays a draft until it has been read.
|
Cycle stopped at the five-delta cap, with one unreviewed fix on the branch Recording where the review cycle ended and why, so a cold reader does not have to infer it from the round trail. Six reviews ran: one over the whole diff, then five delta rounds each scoped to the previous round's fix commit. Every round found something, which is what kept the chain going, and the fifth is the cap. So The shape of the chain matters more than its length. The framework change, the reload debounce and the dev So the honest read is that the cap fired on a docs surface that was already wrong in several places, not on an unstable code change. The lifecycle list is worth its own pass, since the parts this PR did not need to touch have not been audited against the runtime, but that is a separate piece of work and not one I am filing off my own review. Nothing was rejected and nothing was deferred across the six rounds. Every finding was fixed. |
Editing several files a few seconds apart in dev left the browser showing unstyled HTML until a manual refresh. Two independent causes, both fixed here. Each save produces TWO reload signals, not one: the in-process rebuild frame, then a changed boot id when the browser reconnects to the process node --watch restarted (measured 429ms apart, with 1071ms between saves). Acting on every one reloads into a server that is about to be killed again. Both signals now route through one debounced emitter in the reload relay, which fires once the signals stop for 2 seconds or at the latest 5 seconds into a sustained burst, so the page never freezes on stale content either. The cap timer is armed once per batch and never re-armed, so it measures from the first signal instead of sliding with the burst. Error frames are not debounced, since an overlay has to appear at once. The per-tab fallback now runs the same relay over a shim port rather than a second copy of the boot-id rule. The stylesheet is the visible casualty because a failed stylesheet request is non-fatal to the browser and /public/tailwind.css is the slowest request on the page: it waited on ensureReady(), measured at 1907ms cold on the website app. In dev, /public/* is now served ahead of the analysis, since a static asset needs neither the module graph nor the vendor importmap. Dev only, because in prod /__webjs/ready already holds traffic off a cold instance and hoisting there would un-gate a file an app middleware protects. Both call sites share one function, so the traversal guard exists in exactly one place. Closes #1397
The counterfactual did not discriminate: removing the containment check
left the test green. `/public/%2E%2E/secret.txt` never enters the public
branch at all, because the WHATWG URL parser decodes `%2E%2E` and
normalises the dot segment away, so the request arrives as plain
`/secret.txt`. The test was observing nothing.
An encoded slash survives parsing intact, so `/public/..%2Fsecret.txt`
enters the branch with a path `join` then resolves outside appDir/public/,
which is what the guard is for. With that vector the test goes red when
the guard is removed.
The comment this was derived from asserted the opposite ("after URL
parsing, which doesn't touch %2E"), so correct it where it now lives
rather than carry an inaccurate claim into the extracted function.
The base-path guard matched a literal `new EventSource("<url>")` in the
served client. The per-tab fallback now hands that URL to the shared relay
instead of constructing the EventSource itself, so both cases went red on a
shape change with the invariant intact.
They assert the same thing at the call that carries the URL, and the
negative widened from "no bare new EventSource(...)" to "no bare
/__webjs/events string anywhere in the script", which is the stronger form
of what it was guarding.
… drift Review found five things. The shim port the per-tab fallback hands the relay runs application code synchronously in its postMessage, and the relay deletes a port whose postMessage throws. That heuristic reads a throw as "the tab is gone", which is right for a real MessagePort and wrong here: an overlay render that threw would permanently unsubscribe the tab and silently kill its live reload. The pre-#1397 fallback attached to the EventSource directly, where a handler throw detached nothing, so the guard restores the old behaviour rather than adding a new one. The ?v= test claimed to cover the early path while building the handler with dev: false, so it exercised the prod path instead. It cannot be written the other way, since fileResponse hard-codes no-cache whenever dev is true, so immutable is unobservable there. Retitled to what it actually pins, plus a dev-mode assertion that a fingerprinted asset still serves on the early path. Correcting the middleware prose left two code-block comments two lines below still saying "every request", and the same claim was still on the agent-facing skill reference the scaffold ships, in the root AGENTS.md layout table, and in the blog example's own middleware. Inserting the two constants between the module header and the function orphaned its @PARAM tags onto a constant, and the header no longer mentioned that reloadClientJs inlines this source too.
Delta review of the previous commit found four things.
The shim port's catch was empty, which hid dev-overlay bugs on that one
path. A throw out of an EventSource listener (the old shape) and out of the
SharedWorker path's onmessage ten lines below both reach the console, so
discarding it here was a new behaviour rather than the restored one the
comment claimed. It is reported now.
The slice meant to anchor that guard's assertion to the fallback function
ended at the bootstrap that follows it, so it still trailed a closing brace
plus `try {`, leaving the bootstrap's try inside the slice. The first
assertion therefore matched with the guard removed, and the comment
asserted a counterfactual that did not hold. The slice now ends at the
function's own closing brace, with an assertion that the bootstrap is
outside it, and each assertion was re-checked to fail with the guard gone.
The architecture page's Request Lifecycle said root middleware runs first
and put internal endpoints and static files after it. That was already
wrong for /__webjs/* before this PR, and the dev /public/* hoist widens it,
so two pages on the same site contradicted each other on the fact this PR
exists to correct.
The blog example's middleware comment wrote the brand as a lowercase code
token in prose, which invariant 11 does not allow.
…tion step Second delta review found three things, two of them mine from the round before. The block comment above the shim guard still ended with "swallowing here restores that behaviour", which the previous commit made false on both halves: the code no longer swallows, and swallowing was never the restored behaviour. Two adjacent comments asserted opposite things about the same line, and the stale one came first. Merged into one statement of what is actually being prevented, which is detachment and not reporting. The ordering assertion compared indexOf results directly, and indexOf returns -1 for a missing needle, which is less than any real index. So with the guard removed entirely, the one case the assertion exists for, it passed. Both indices are asserted present first now. The comment above also claimed three assertions where five follow it, so it named the wrong set; it now says how each was checked instead of counting them. The architecture page's new step 2 said everything under /__webjs/* is answered before root middleware. The server-action RPC endpoint is dispatched in handleCore, which is reached through next() after middleware runs, so that was wrong in the direction that matters: it would tell someone gating actions with auth or rate-limit middleware that their middleware does not run. Step 2 now names the set it actually covers, and the action endpoint is back in the routing step with a note that middleware does run for it.
Third delta review found three more members of an enumeration I kept asserting was complete. Every round has found another one, so this stops listing and states the rule: anything the listener shell or the pre-analysis stage answers bypasses root middleware, and everything routed with the app reaches it. The examples stay, but as examples. The two genuinely missing members are real bypasses. A WebSocket upgrade bound for a route.ts exporting WS is intercepted at the server level on both listener shells before the app handler is called, and it is unambiguously an app route, which is the line the reworded "every app request" was drawn to make. The dev SSE stream at /__webjs/events is intercepted the same way, so the previous wording, which said the named set was everything under /__webjs/* except the action endpoint, was wrong in the direction where believing it costs you. Inserting a step into the architecture lifecycle also shifted the list under a "Step 6 above" reference further down the page, which now pointed at segment middleware instead of the SSR pipeline. That reference is by name now, so inserting a step cannot break it again. The lifecycle also listed WebSocket upgrades as a late step, after middleware and route matching, which the same interception contradicts.
Fourth delta review found two more ordering facts wrong on the lifecycle. webjs.redirects and webjs.trailingSlash are resolved ahead of the probes and well ahead of root middleware, so a 308 from a redirect rule is answered with the app's own middleware never running. The rule stated last commit does cover them, but only if a reader classifies a config key they wrote themselves as a framework pre-analysis stage, which is exactly the guess that goes the wrong way. So they are called out by name on all three surfaces rather than left to the rule. 103 Early Hints were listed after root middleware and are emitted before it, from inside the node listener shell ahead of the app handler. That is the same listener-shell step the last commit restructured the list around, so it should have moved then. It is also node-only rather than merely prod-only, since Bun.serve has no informational-response API, which now matters because the surrounding steps name both shells.
The lifecycle step added last commit was headed "Declarative rewrites", and neither key rewrites anything: webjs.redirects replies with a configured redirect status and a Location header, and webjs.trailingSlash 308-redirects the non-canonical form. A rewrite is a distinct concept the audience for this page brings from Next, where it proxies internally with no client-visible redirect, and WebJs ships no such key, so the heading invented a feature. Every other surface this PR touches already says redirect.
29305bc to
c82f2b7
Compare
|
Rebased onto main after #1401 landed
Worth recording how it surfaced, because it looked alarming and was not. My end-of-cycle check greps the branch diff for One real conflict, in the Post-rebase: 270 tests green across the dev, base-path and docs suites, and the new Bun script still passes on node. The full matrix has not been re-run, because the review cycle stopped at its round cap and those suites gate a flip to ready for review that is not happening yet. |
Final whole-diff review found three things. The rationale for RELOAD_QUIET_MS said 2000ms is "under the 1900ms analysis warm", which is backwards: 2000 is above 1900. The number carries the argument for picking this value, so an inverted relation is not a stray adjective. The behaviour it describes is real, the debounce lands just past the point where the restarted process has finished warming, so it now says that. Corrected in the module and in the server AGENTS.md row that repeats it. The new bun script took 9500 + pid % 240, byte-identical to dev-reload-retry. Every other dev-server bun script takes a disjoint base on purpose, so that two FILES can never contend whatever their pids, leaving the modulus to separate concurrent runs of one file. Both ship a test wrapper the node runner can schedule concurrently, so the only thing preventing a bind collision was the two pids not being congruent mod 240. The every-request sweep missed the middleware sample on the backend-only docs page, which was the last surviving instance of the claim.
vivek7405
left a comment
There was a problem hiding this comment.
Final pass over the whole diff, on the rebased head. The debounce, the /public/* hoist, the extraction and the test counterfactuals all hold up. Three things wrong, one of which is arithmetic.
The rationale for RELOAD_QUIET_MS says 2000ms is "under the 1900ms analysis warm". 2000 is above 1900. That is not a stray adjective: this number is the load-bearing half of the argument for picking 2000 over anything else, and the claim is repeated verbatim in the server AGENTS.md row and in the PR body, so the same inverted relation shipped to three surfaces. The behaviour being described is real and worth stating, the debounce lands just past the point where the restarted process has finished warming, which is the useful place for it, so it now says that instead.
The new bun script took 9500 + (process.pid % 240), byte-identical to dev-reload-retry.mjs. Every other dev-server bun script deliberately takes a disjoint base (9700, 9750, 9800) so that two FILES can never contend whatever their pids, leaving the modulus to separate concurrent runs of a single file. Both files ship a *.test.mjs wrapper the node runner can schedule concurrently in separate child processes, so the only thing standing between them was the two pids not being congruent mod 240. Moved to its own base.
And the every-request sweep missed the middleware sample on the backend-only docs page, which was the last surviving instance of the exact claim this PR set out to correct.
All three fixed in 21dfc1d7.
The previous commit moved this script to base 9850 and called the bases disjoint. They are not. A base only separates two files if the earlier one's modulus window stops before the later one's base, and these windows run well past: reload-retry reaches 9739, hot-reload 9949, extra-watch 9989, overlay-scope 9979. So 9850-9989 landed inside three of them, where the old 9500-9739 had overlapped two. The change made the collision it claimed to fix more likely, and narrowing the modulus to 140 also raised the same-file rate the comment credited it with lowering. 9989 is the highest port any existing script reaches, so this takes 10000-10255, which cannot collide with any of the four for any pair of pids, and restores a modulus at least as wide as the original for the same-file case. The overlapping ranges among the other four are pre-existing and left alone.
vivek7405
left a comment
There was a problem hiding this comment.
Check of 21dfc1d7, the fix commit from the final review. Its two doc corrections are fine. Its one functional edit, the port move, was wrong in the direction that matters: it made the collision more likely, not less.
I had reasoned that a distinct base separates two files. It does not, unless the earlier file's modulus window stops before the later file's base, and here none of them do. The reachable RANGES are dev-reload-retry 9500-9739, dev-hot-reload 9700-9949, dev-extra-watch 9750-9989 and dev-overlay-scope 9800-9979: they already overlap each other freely, so "each script has its own base" was never the invariant I took it for. Moving to 9850-9989 put this file inside three of those ranges, where the original 9500-9739 had sat inside two. Narrowing the modulus from 240 to 140 also doubled the same-file collision rate that the comment credited it with reducing. Both halves of the rationale I wrote were contradicted by the change they annotated.
9989 is the highest port any of the four can reach, so this file now takes 10000-10255, which cannot collide with any of them for any pair of pids, with the modulus back to 256 so the same-file case is no worse than where it started. The overlaps among the other four are pre-existing and deliberately left alone.
Verified the ranges arithmetically rather than by inspection this time, since eyeballing bases is exactly what produced the bad fix. Fixed in 6696c33f.
The comment credited the modulus with separating concurrent runs of this file. No runner here produces those: each runner runs a given file once, and the node and bun runs are sequential CI steps. The base is what separates this file from its siblings; the offset is only defensive against a leftover socket lingering in TIME_WAIT, which is what dev-hot-reload.mjs already says about the identical construct. Two sibling files gave incompatible accounts of the same mechanism and this one was wrong.
vivek7405
left a comment
There was a problem hiding this comment.
Check of 6696c33f. The port move itself is right: I re-derived the four sibling ranges, confirmed 9989 is the ceiling, that nothing else in the repo binds 10000-10255, and that the range sits below the OS ephemeral floor on Linux and macOS.
One thing left, and it is the comment rather than the code. I credited the per-pid modulus with separating "concurrent RUNS of this file", and no runner here produces those: each runner runs a given file once, and the node and bun runs are sequential CI steps. The base is doing all the separating. dev-hot-reload.mjs already documents the identical base + pid % n construct correctly, as defensive against a leftover socket lingering in TIME_WAIT, so I had two sibling files giving incompatible accounts of the same mechanism with mine being the wrong one. Corrected to match in 7532ac02.
That is a comment-accuracy fix with no behaviour attached, so it ends the cycle rather than buying another round.
The script raced its two fetches, and /__webjs/ready answers immediately (it reports unready without blocking on the analysis), so the pair passed whether or not the hoist exists: the ready fetched at t=0 was 503 either way. Proven vacuous by running it against a server without the hoist, where it printed OK. Sequenced, the timeline discriminates. With the hoist the css returns in milliseconds, long before the fixture middleware's sleep releases the analysis, so the ready fetched after it is still 503. Without the hoist the css itself blocks on ensureReady, so by the time it returns the analysis is done and the same fetch reads 200. Verified in both directions: OK against this branch's server on node and bun, and the assertion fails against main's server. The middleware sleep widened from 1500ms to 3000ms so the discrimination has margin over css fetch latency.
|
Live verification: the bug reproduced on main, the fix confirmed end to end, and one vacuous test caught by it The review cycle verified this PR against a model of the problem (the issue's measured signal cadence), never against the symptom itself, so I ran the real thing: On main, the reported bug reproduces exactly. A single edit fired the reload ~800ms after the signal, straight into the window where On this branch, the same drive behaves. The single edit reloads once, 2.6s after the signal (the 2s quiet window plus the readiness probe), and lands on a live server with The live run also caught a vacuous test that survived the whole review cycle. Method note for whoever dogfoods this next: in a linked worktree the CLI resolves |
Four problems from the delta round, three of them in the two fixes it was scoped to. The stylesheet refresh dropped the old link on error as well as on load, so a re-request that 404s (the server mid-restart, a renamed file) left the page with no stylesheet at all, which is the failure #936 and #1400 exist to prevent. On error it now drops the replacement and keeps the sheet that still works. Its de-dupe keyed on the path alone, so an author's second legitimately distinct link to the same file (a media=print sheet) was deleted on the first refresh and never came back. The key is now every attribute except the href query, which still collapses the bare/busted pair the head merge re-appends. refreshPage read , which is the wrong question. An HTML body of any status is swapped in place, so a page rendered through notFound(), forbidden(), or an error boundary applied fine and was reported as a failure, making the dev client reload on top of a swap that already happened. fetchAndApply now reports separately. The e2e comments also named the wrong mechanism: the fixture link is hoisted into head by the SSR, and the page tier merges through addNewHeadElements rather than mergeHead.
Four problems from the delta round, three of them in the two fixes it was scoped to. The stylesheet refresh dropped the old link on error as well as on load, so a re-request that 404s (the server mid-restart, a renamed file) left the page with no stylesheet at all, which is the failure #936 and #1400 exist to prevent. On error it now drops the replacement and keeps the sheet that still works. Its de-dupe keyed on the path alone, so an author's second legitimately distinct link to the same file (a media=print sheet) was deleted on the first refresh and never came back. The key is now every attribute except the href query, which still collapses the bare/busted pair the head merge re-appends. refreshPage read the response's ok flag, which is the wrong question. An HTML body of any status is swapped in place, so a page rendered through notFound(), forbidden(), or an error boundary applied fine and was reported as a failure, making the dev client reload on top of a swap that already happened. fetchAndApply now reports applied-ness separately. The e2e comments also named the wrong mechanism: the fixture link is hoisted into head by the SSR, and the page tier merges through addNewHeadElements rather than mergeHead.
* feat: refresh page and layout edits in place in dev Every dev edit produced a full page reload: the page blinked, hydrated component state reset, scroll position was lost. Most dev edits do not need that. Pages and layouts never hydrate, so a freshly rendered page is the complete truth for them and nothing in the browser can be stale after a re-render. The dev watcher now classifies the changed file against the module graph and puts a ternary verdict on the SSE reload frame, and the browser picks the lightest correct response: morph the deepest shared boundary for a page edit (scroll and the hydrated state of components outside it survive), replace the whole body for a layout edit (its own markup sits outside every children range), and reload for anything else. The classification walks the graph rather than the path shape, because a page that imports a client-effecting util ships whole and a util under lib/utils reachable from a component is a component edit whose path says nothing. Everything unclassifiable reloads: a wrong morph is a broken page, a wrong reload is a flash. Component edits stay a full reload by design. customElements.define is once-per-tag, so a morph would apply new markup wired to the old class. This hot-swaps no module and does not reopen the deferred HMR question. * test: browser coverage for the reload verdict relay and refreshPage The relay's strongest-verdict rule is order-independent, so the burst test gets a counterfactual that a last-write-wins implementation fails. The refreshPage suite asserts what survives a swap against real DOM identity, a real custom-element upgrade, and real scroll, which is why it is a browser test rather than a string assertion. * test: e2e proof that a dev page or layout edit never reloads The headline criteria are browser facts: whether the document reloaded is invisible in the DOM, so the fixture layout stamps a per-document token that survives a script re-run and can only change when the global scope is replaced. The component edit is the counterfactual in the direction that matters, since a morph there would wire fresh markup to the old class with no recovery short of a reload. * feat(gallery): demo refreshPage beside navigate and revalidate A new @webjsdev/core export has to be demoed or consciously exempted in the scaffold teaching-coverage manifest, and refreshPage earns a demo: re-rendering the page you are already on is a real capability an app can use after a mutation elsewhere, not just the seam the dev reload client calls. The page gains a server-rendered timestamp so the swap is visible, since the page function runs only on the server and restamps on every render. * fix: read the refresh outcome, and re-request stylesheets after a swap Two defects in the same fallback path. refreshPage resolved true for anything that did not throw, and fetchAndApply throws for none of its real failures: a rejected fetch, a non-HTML body, an unparseable one, a discarded swap all return ok:false. So the dev client's full-reload fallback could never fire for the cases it exists to cover, and a tab would silently sit on stale content. It now reads the outcome. An aborted navigation is not a failure, since a newer navigation owns the page and reloading would yank the reader out of it. A swap also never re-requested the page's stylesheets: mergeHead preserves them unconditionally and the dev href carries no content hash, so the link node is kept by identity. A full reload used to do that asking, which is how webjs.dev.regenerate ever ran, since it rebuilds a stale output on request. Without it an edit that adds a utility class renders with no backing rule until a manual reload, which every in-repo app is configured for. * fix: keep the working stylesheet on a failed re-request, report applied Four problems from the delta round, three of them in the two fixes it was scoped to. The stylesheet refresh dropped the old link on error as well as on load, so a re-request that 404s (the server mid-restart, a renamed file) left the page with no stylesheet at all, which is the failure #936 and #1400 exist to prevent. On error it now drops the replacement and keeps the sheet that still works. Its de-dupe keyed on the path alone, so an author's second legitimately distinct link to the same file (a media=print sheet) was deleted on the first refresh and never came back. The key is now every attribute except the href query, which still collapses the bare/busted pair the head merge re-appends. refreshPage read the response's ok flag, which is the wrong question. An HTML body of any status is swapped in place, so a page rendered through notFound(), forbidden(), or an error boundary applied fine and was reported as a failure, making the dev client reload on top of a swap that already happened. fetchAndApply now reports applied-ness separately. The e2e comments also named the wrong mechanism: the fixture link is hoisted into head by the SSR, and the page tier merges through addNewHeadElements rather than mergeHead. * fix: extract the stylesheet refresh so a browser test can drive it The load-versus-error rule was proven only by two regexes matched against the served client's source, which cannot tell a correct handler from one that removes the wrong node in a differently-formatted body. That is the half that leaves the page permanently unstyled, so it needs a real test. dev-styles.js follows the dev-overlay.js and dev-reload-worker.js pattern: a browser-safe module inlined verbatim into the served client after an export strip, so the browser test drives the exact shipping code. The new test asserts against real link elements and real load and error events, including the counterfactual that a 404 replacement removes itself and leaves the working sheet on the page. Three smaller things from the same round. performNavigation's returns tag did not declare the applied field its one consumer reads. The stream-action branch hardcoded applied true even when a newer navigation had superseded it, contradicting the contract written three lines above. And the public/ rung in the classifier still justified itself with "mergeHead preserves stylesheets so a swap would do nothing", which this PR made false; the rung stands, but the honest reason is that public/ is outside the module graph so the server cannot tell what the file is. * fix: force ok false on a superseded stream response The stream branch was the only return in fetchAndApply that could produce ok true alongside aborted true, so a caller reading ok as "this response was not superseded" would have been wrong there and nowhere else. Every sibling abort return forces ok false, and the function's own contract says so. The HTTP status is still on the status field for anyone who wants it. * fix: complete the applied contract on loadFrame, and two doc slips loadFrame passes the fetchAndApply outcome straight through, so it gained applied on its success path while its three guard returns and both its JSDoc and published .d.ts still described the three-field shape. That is the same contract hole the flag exists to close, left on the exported wrapper of the contract. The gallery teaching comment had the refreshPage paragraph spliced into the middle of a sentence, leaving a dangling clause and the same token twice. That comment ships as reference material in every scaffolded app. The runtime docs page told users to run the CLI directly while every other run instruction on it is an npm script. * fix: keep classifying after a rebuild, and stop claiming an uncommitted swap The feature turned itself off for every edit after the first in a burst. doRebuild invalidates the lazy analysis, nothing re-warms it until an HTTP request arrives, and the relay defers that request by its 2000ms quiet window while the measured inter-save gap is about a second. So the second save classified analysis-cold and the strongest-verdict rule collapsed the whole batch to a full reload. The gate now reads whether the derived sets are POPULATED rather than whether they are current, which is what the rest of the code already assumed: classifying against the previous build's graph is intended, and it is conservative in the right direction, since a file that graph has never seen falls through to a reload. applySwap returns without committing on four paths (a missing frame, and three degradations to a hard navigation), and all four still reported applied true, which is the hole the flag exists to close. They return a sentinel now and fetchAndApply maps it. Three doc corrections. The gallery copy described a counter that is not on that page. The skill reference kept the direct CLI invocation the website copy had already dropped. And both runtime surfaces listed five watched directories while the supervisor also watches root middleware, so a middleware edit is a full reload rather than the in-place refresh they implied. * fix: report the uncommitted swap without changing the pipeline The previous commit returned early on the four non-committing applySwap paths, which also skipped the history push, the scroll block, and the streaming tail those paths used to fall through to. A click-driven frame nav records history, so a frame-missing response would have stopped advancing the URL. Nothing covers that, so the absence of a red test was not evidence. It is a flag now, so only the reported value changes. * test: cover the applied flag on the paths that commit nothing Both the sentinel and the flag that reads it shipped with no assertion: the only applied:false cases already returned false before the change, so they passed identically with it reverted. Three cases close that. A frame-missing loadFrame reports applied:false while a matching one reports true, a refresh that degrades to a hard navigation reports it did not apply, and a click-driven frame nav still advances the URL, which is the fall-through the flag exists to preserve and which nothing else in the suite would notice losing. * test: make the URL-advance case observe its own click It asserted the URL reached the link's target, but an earlier case in the same suite clicks the same link and leaves it there, and web-test-runner isolates per file rather than per test. So the assertion was satisfied by that case's history push and would have passed with this one's removed. It parks the URL somewhere the click cannot reach first. Proven by deleting the earlier case and re-running: the assertion still fails under the early-return shape and passes under the fix. * test: move the URL parking inside the case's try block The parking replaceState and its assertion sat between setup() and the try, the one point in the file where a throw skips the finally. A failed park, or WebKit rate-limiting history mutations, would have leaked the parked URL, the patched console, the nav guard and the container into the three later cases, which would then fail against the wrong container for reasons naming nothing about the cause. That is the cross-case state leak this case exists to close. Proven by injecting a throw at the park step: exactly one case fails, the one that threw, with no cascade. * test: assert the history call instead of dancing the URL around Reading `location` to prove a history push needed two mutations of its own, a park before and a restore after. Both were hazards: this file's only cross-case leaks came from exactly those two lines, and WebKit rate -limits history mutations so either can throw and strand shared page state on the cases below. The last fix moved the park inside the try and left the restore, in the same finally, doing the same thing. Spying on history.pushState needs neither mutation, depends on no sibling case, and asserts the call directly rather than inferring it from a global the whole file shares. Proven by deleting every other case in the file: the spy case alone still passes under the fix and fails under the early-return shape. Also drops a no-op in refresh-page.test.js whose comment claimed to restore router state for "the rest of the file", in the last test of the suite, after the finally had already left it disabled. * fix: dismiss a live error overlay before an in-place refresh The overlay's teardown is keyed on the URL CHANGING, and a same-url refresh never changes it, so a refresh has no exit for a live overlay the way a full reload did by replacing the document. Shipping this as defensive, because the scenario that looks like it needs it does not hold, and that is worth recording so the next reader does not re-derive it. A page-render error cannot strand an overlay: the dev 500 page carries no children boundaries and no layout, so applying it shares no boundary with the live page and degrades to a hard navigation in BOTH directions, and the reload clears the overlay. Measured, not assumed. What remains is an UNSCOPED frame (a rebuild or ts-strip failure), which the nav sync deliberately never clears and which can coexist with a page that still renders and therefore still refreshes. Dismissing before rather than after is self-correcting: a render that fails again pushes a fresh frame during it, so the overlay returns describing the current error. Afterwards would race that push. No e2e for it. I wrote one, found it passed because the break had already hard-navigated rather than because of the dismiss, and removed it rather than ship a test that observes the wrong thing.
Closes #1397
Summary
Editing several files a few seconds apart in dev left the browser showing unstyled HTML until a manual refresh. Two independent causes, fixed together because either one alone still reproduces the symptom.
Each save produces two reload signals, not one. The in-process rebuild frame fires first, then a changed boot id arrives when the browser reconnects to the process
node --watchrestarted (measured 429ms apart, with 1071ms between saves). Acting on every one reloads into a server that is about to be killed again. Both signals now route through one debounced emitter indev-reload-worker.js, which fires once the signals stop forRELOAD_QUIET_MS(2000ms) or at the latestRELOAD_MAX_HOLD_MS(5000ms) after the first signal of the batch, so a sustained burst repaints instead of freezing on stale content. The cap timer is armed once per batch and never re-armed, which is what makes it measure from the first signal rather than sliding with the burst.webjs-erroris not debounced, since an overlay has to appear at once, and the cached error is cleared on the signal rather than on the debounced emit so a tab connecting mid-window is not replayed an overlay for an error the rebuild already fixed.The stylesheet is the visible casualty because a failed stylesheet request is non-fatal to the browser (it paints unstyled rather than erroring) and
/public/tailwind.cssis the slowest request on the page: it waited onensureReady(), measured at 1907ms cold on the website app of which 1900ms was the analysis. In dev,/public/*is now served ahead of the analysis, since a static asset needs neither the module graph nor the vendor importmap.Decisions worth knowing
The debounce lives browser-side because the server process dies on every edit, so no server-side timer can span a burst. The SharedWorker survives, being keyed by script URL.
The per-tab fallback (
__webjsDirectEvents, live whereverSharedWorkeris missing or its construction throws under a strict dev CSP) now runs the SAME relay over a shim port instead of re-implementing the boot-id rule. Leaving it alone would mean the debounce simply does not exist on those browsers, and a second copy would be a second thing to drift.The
/public/*hoist is dev-only. In prod,/__webjs/readyalready holds traffic off a cold instance until the analysis and the first vendor attempt complete, and hoisting there would silently un-gate a/public/*file an app middleware protects. The cost is a narrow dev/prod divergence (root middleware does not run for those paths in dev), which is the same trade the framework statics already make in both modes, and it is documented rather than left implicit. ThehandleCorebranch is not dead: it is the live path in prod and the fallback in dev, and both call sites share onetryServePublicAssetso the traversal guard exists in exactly one place.Test plan
Every layer run at the rebased head. Six node/Bun failures are pre-existing linked-worktree artifacts, not regressions: each is proven by running the same file in the primary checkout, where it passes.
4319 pass, 5 failof 4325. The five are the documented linked-worktree set (test/bun/listener.test.mjs,listener-overhead, and three assertions inpackages/server/test/elision/differential-elision.test.js). Proof: that elision file runs9 pass, 0 failin the primary checkout, which carries none of these changes.857 passed, 0 failedin Chromium, Firefox and WebKit.reload-worker.test.jsgrew from 7 cases to 14.94 pass, 0 fail(WEBJS_E2E=1), against acore/distrebuilt in this worktree so the bundle matches the branch.317 pass, 1 genuine fail, that one beingtest/bun/listener.test.mjs. Same proof:bun test/bun/listener.mjspasses in the primary and fails here. The newtest/bun/dev-public-before-warm.mjspasses onnode 26.7.0andbun 1.3.14, as do the three dev scripts it now sits beside./,/docs/middleware,/docs/architecture,/uiand/ui/button, with no broken modulepreload hints (50 preloads checked on/ui/button). The blog is covered by the e2e run.webjs checkpasses on website,examples/blogandgallery.node --watchrestart cycle, and building one to observe a 2 to 5 second debounce would add a multi-second real-time wait for behaviour the browser layer already asserts deterministically against the shipped module.Counterfactuals
Each new assertion was proven to fail with its change reverted, checked individually rather than per-file (checking per-file is what let two vacuous assertions through mid-review: the run went red on a later assertion and the earlier one was credited without being looked at):
tryServePublicAssetcall ahead ofensureReady(): the ordering test flips to['warm', 'public'], and the dev/prod middleware-gating test also reds...%2F). The encoded-dots vector the plan specified does not work, because the URL parser normalises it away before it reaches the branch.try-present and ordering assertions red. Remove only itsconsole.error: the reporting assertion reds alone.Review
Seven reviews: one whole-diff, five delta rounds each scoped to the prior round's fix commit, and a final whole-diff pass. Nothing was rejected and nothing deferred; every finding was fixed. The substantive code finding was the shim port, where routing the per-tab fallback through the shared relay let an application throw permanently unsubscribe the tab, since the relay deletes a port whose
postMessagethrows. The rest were test assertions that did not observe what they claimed and doc surfaces that contradicted each other on what bypasses root middleware.Docs
packages/server/AGENTS.md, thedev.jsmodule-map row: the coalescing, both constants and what each is for, and the fallback now running the same relay.packages/server/AGENTS.md, package invariant 3:/public/*served beforeensureReadyin dev, why it is dev-only, and the root-middleware consequence.website/app/docs/middleware/page.ts: the page claimed root middleware "runs on every request", which was already inaccurate for/__webjs/*and this widens it. It now names both exceptions.framework-dev.md, the dev-loop section: a reload is coalesced, so a reload that looks missing right after a save is the first thing to check.AGENTS.md: N/A, no public API, nowebjs.*config key, no CLI flag, no invariant moves.