feat(client): post build events to the page, and guard reloads - #2425
Conversation
Three gaps against webpack-dev-server's client, found by comparing what survives if dev-server deletes its own and re-exports this one. Its client posts every build event out through `self.postMessage` "so plugins can consume it", and this one posted nothing at all — so anything listening for `webpackOk` and its siblings would have gone quiet without a word. Undocumented, which makes it easy to drop and hard to notice. All eight are posted now, under the same names and payloads, including the bare `webpackHotUpdate<hash>` string. `webpackClose` announces an outage once rather than once per retry, matching the `retries === 0` guard the other client reports it behind. A reload while the page is already navigating away reloads a page the browser is leaving, so `beforeunload` now holds reloads off — released again after a grace period, since a `beforeunload` can be cancelled, and settled by `pagehide`/`pageshow` for a page that comes back out of the back/forward cache. In an `about:blank` iframe a reload reloaded the blank document and lost the app; the nearest ancestor with a url of its own is reloaded instead, and a cross-origin one is left alone rather than thrown on. A runtime error filter could not see what a rejection carried: a plain object was wrapped in an `Error` and dropped. It is kept as `cause` now, so a filter deciding on a status code has somewhere to read it from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
🦋 Changeset detectedLatest commit: 6ee3774 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. WalkthroughThe client now posts build-status and hot-update messages to the page, and reports socket disconnects. Reload handling tracks unload events, defers reload requests during navigation, and targets the nearest ancestor with a non-about URL. Runtime error handling preserves non-Error values in Priority: ⬇️ Low Merge Risk: 🔵 Low · up to The remaining concern is a narrow test gap in cross-origin reload fallback. It does not establish a current reload failure, so the change is mergeable with that coverage follow-up. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The changes appear confined to the development client and its page. No new privileged or cross-origin access path was demonstrated, but external message consumers and some embedding behavior remain unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 9f8631f9-2bca-44b6-9003-ebe55a211101
📒 Files selected for processing (9)
.changeset/client-postmessage-and-reload-guards.md.changeset/reload-guards.md.changeset/runtime-error-cause.mdclient-src/clients/createSocket.jsclient-src/index.jsclient-src/overlay.jsclient-src/utils/reload.jsclient-src/utils/send-message.jstest/e2e/messages.test.js
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.
Two review findings.
`new Error(message, { cause })` is ES2022, and this file is compiled to an
ES5 baseline for browsers that predate it — where the option is quietly
ignored and a `runtimeErrors` filter would find nothing to read. Defined
afterwards instead, and non-enumerable, which is the descriptor the option
would have given it.
A reload asked for while the page looked like it was leaving was dropped.
A `beforeunload` can be cancelled, and then the page is staying and still
wants the update, with nothing to ask again until the next rebuild — so it
is held for the grace period and performed when the navigation turns out
not to have happened. Dropped for good on `pagehide`, where the page
really is going, and on `pageshow`, where it came back from the cache
showing its own state and a reload from before the navigation is stale.
Covered by unit tests over a driven fake page, which is what makes the
timing and the three events testable at all.
A third finding, that `onDisconnect` reports a first attempt that never
opened, is accurate but deliberate: webpack-dev-server reports that case
too, from the same reset-on-open counter, and a page loaded while the
server is down has no connection either. Its wording said "a connection
that was open", which was wrong about its own behavior; the comment and
the type now say what it does.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/client-reload.test.js (1)
162-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the cross-origin test fail at the ancestor location read.
reloadPagereadstarget.parentbefore it reads the ancestor'slocation.protocol. The current mock throws too early, so a regression that fails to catch the ancestor location error can pass.Suggested test change
- Object.defineProperty(globalThis.window, "parent", { + const parent = { location: {} }; + Object.defineProperty(parent.location, "protocol", { get() { throw new Error("cross-origin"); }, }); + globalThis.window.parent = parent;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: b1f684dc-ca2f-45f3-b716-fe20a166dca7
📒 Files selected for processing (4)
client-src/clients/createSocket.jsclient-src/overlay.jsclient-src/utils/reload.jstest/client-reload.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- client-src/overlay.js
- client-src/clients/createSocket.js
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 4 remain after this review.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2425 +/- ##
==========================================
- Coverage 96.30% 96.13% -0.17%
==========================================
Files 17 18 +1
Lines 1894 1943 +49
==========================================
+ Hits 1824 1868 +44
- Misses 70 75 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Summary
Before webpack-dev-server can delete its own client and re-export this one, everything that client does has to exist here. I compared the ~1140 lines of it that nothing had compared yet —
client-src/index.js,progress.jsandutils/— and this PR closes three of the four gaps that turned up. (The fourth,progress: "linear" | "circular", is a separate PR since it is an option change rather than a fix.)1. Nothing was posted to the page at all
webpack-dev-server's client posts every build event out through
self.postMessage— its own comment says "so plugins can consume it". This client posted nothing. Anything listening forwebpackOkand its siblings would have gone quiet the moment dev-server switched, with no error and nothing in a diff to notice, because none of it is documented.All eight go out now, under the same names and payloads:
webpackInvalid,webpackProgress,webpackOk,webpackStillOk,webpackWarnings,webpackErrors,webpackClose, and the barewebpackHotUpdate<hash>string — that last one is posted as a plain string rather than in the{ type, data }shape, which is how it has always been sent.Two mappings worth stating, since the two protocols are not identical:
built(a compilation that produced something) →webpackOk;sync(one with nothing to report) →webpackStillOk. Same distinction dev-server draws betweenokandstill-ok.webpackCloseannounces an outage, once — not once per reconnection attempt. dev-server reports it behind aretries === 0guard, and Server-Sent Events here retry for as long as the page is open, so counting attempts instead would have meant announcing a dead server every few seconds forever. Teeth-checked: dropping the guard gives threewebpackClosemessages where there should be one.2. A reload could fire at the worst two moments
While the page is already navigating away — an update landing mid-navigation reloaded a page the browser was leaving.
beforeunloadnow holds reloads off, released again after a grace period (abeforeunloadcan be cancelled, by another listener or by the user answering "stay"), withpagehide/pageshowsettling the case it gets wrong: a page restored from the back/forward cache runs the same script again, and a flag left set by the navigation away would have blocked every later update.And inside an
about:blankiframe —srcdoc, or a document written into the frame — where reloading reloaded the blank document and lost the app. The nearest ancestor that has a url of its own is reloaded instead. Unlike dev-server's version this catches the cross-origin case rather than throwing on it, and reloads this frame as the most that is permitted.3. A runtime error filter could not see what a rejection carried
Promise.reject({ status: 503 })was wrapped in anErrorand the object dropped, so aoverlay.runtimeErrorsfilter deciding on a status code had nowhere to read it from. Kept ascausenow, which is what dev-server does.What kind of change does this PR introduce?
feature (the posted messages), plus two fixes
Did you add tests for your changes?
Yes, four end-to-end cases in a new
test/e2e/messages.test.js: the order and names across a rebuild, the payloadwebpackErrorscarries,webpackClosearriving exactly once across an outage, and a filter judging a rejected plain object throughcause.Both new behaviours are teeth-checked — removing the outage guard and removing
causeeach fail their case.Verified locally: end-to-end 106/106, non-browser 6811 passed, lint and both typechecks clean.
test/logging.test.jsfails 74/74 on a cleanmaintoo, unrelated to this.One thing to be aware of, unrelated but observed:
turns an error overlay into a warning overlay on partial recoveryfailed once in a full run and passed on four subsequent runs, including two full suites, and on cleanmain. It is an existing intermittent, not something this branch introduces.Does this PR introduce a breaking change?
No. Messages that were not posted before are now posted; a reload that used to fire at a moment it should not no longer does.
If relevant, what needs to be documented once your changes are merged or what have you already documented?
The posted messages are worth documenting — they are the integration surface dev-server users rely on and neither package documents them today. Not included here.
Use of AI
AI-assisted (Claude Code). Used to audit this client against webpack-dev-server's, write the fixes and the tests, and verify them: each new behaviour was teeth-checked by reverting it and confirming the matching test fails.
🤖 Generated with Claude Code
https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
Generated by Claude Code
Summary by CodeRabbit
about:blankiframe.