fix: keep reloading after a cancelled "Leave site?" dialog - #5744
Conversation
`beforeunload` set `isUnloading` and nothing ever cleared it. The event only means the page *may* be leaving, though: any listener can cancel it, and when the user then clicks "Cancel" the page stays with the flag set, so `reloadApp` returns early for the rest of its life and every hot update and live reload is silently dropped until a manual refresh. Form guards and editor-integration tools set such a listener routinely. There is no event that announces a cancelled unload. Measured in Chrome, one fires `beforeunload`, `blur` and `focus` — and a confirmed unload fires those three too, then `pagehide`. So `pagehide` is what separates them, but it only arrives once the next document has loaded: with the response delayed 1s and 4s, `beforeunload`-to-`pagehide` measured 1023ms and 4024ms. Its absence therefore cannot be told from a slow navigation at any fixed moment. The suppression is now a grace period instead of permanent, with `pagehide` making it permanent once the page really is going. The length trades the two failures against each other — too short and a slow navigation can still be interrupted by a reload, which is what #544 added this for; too long and the cancelled dialog keeps dropping updates. `pageshow` clears it as well, for the same bug reached the other way: navigate away, come back, and the page restored from the back/forward cache is the same script with the flag left set. Verified end to end in Chrome against the real server, cancelling the dialog and then editing a file: on `main` the client logs "App updated. Recompiling..." and stops there; with this change it goes on to "App updated. Reloading..." and the page reloads. Fixes #5571 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
🦋 Changeset detectedLatest commit: 4990db8 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. WalkthroughThe client now clears its unload grace timer and resets Priority: ➖ Normal 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Out of Scope Changes checkExplanation The changes to
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
test/e2e/overlay.test.jsESLint failed to execute (timeout). 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 |
`overlay.test.js:447` timed out for its full two minutes on macOS/Node 26, waiting for `#webpack-dev-server-client-overlay` to become hidden after a second, different build error is written. That hidden state is not one the test can rely on seeing. The client dismisses the overlay when the `invalid` message announces the rebuild and shows it again from the errors that rebuild produces, so it exists only for the length of the build in between — and only if `invalid` arrives separately at all. Waiting for it is a race, and losing it costs the whole timeout rather than reporting anything useful. It is also not what the test is about. Its name says "then show other error", so what matters is the second error reaching the overlay, whether or not the overlay flickered getting there. It now waits for the overlay's content to change instead, which holds however the client gets there. The snapshots are untouched, so the same page and overlay HTML is still asserted. Point the second write at the same content as the first, so no new error is produced, and the test fails — it is the wait that changed, not what is being checked. The file's six other `hidden: true` waits are left alone: those follow an Escape keypress or a write of the good fixture, where hidden is where the overlay stays. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
macOS/Node 24 failed with "Execution context was destroyed, most likely because of a navigation" at `overlay.test.js:486`, 15s in rather than on a timeout. Restoring the good fixture produces a build that succeeds, so the client does two things in sequence: it dismisses the overlay when `invalid` announces the rebuild, then live reloads the page when the build lands. The test read the page on the first of those — `waitForSelector(hidden)` resolves on the dismiss — which leaves the read racing the reload that follows, and losing it destroys the execution context underneath `page.evaluate`. Both tests that restore the fixture now read the page until it settles, with the same retry covering the reload, instead of reading once the moment the overlay goes. Same shape as the previous commit: the wait was keyed on a transient step rather than the state being asserted. Snapshots are untouched again, so the same page HTML is asserted, and the overlay suite passes three runs in a row. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 9f2537b7-87be-4e75-9412-bf3b5da55d3f
📒 Files selected for processing (1)
test/e2e/overlay.test.js
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.
`CodeRabbit` pointed out that the previous commit's wait can settle before the live reload, so the page it snapshots is the document from before the fix. Measured, and it is right: marking the document and reading it the moment the overlay goes always caught the pre-reload one. The HTML happens to be identical either way here, so nothing was flaky or wrong, but the test claims to check the page after the fix and was checking the page before it — and reading on the dismiss is also what left the evaluate racing the reload in the first place. Both tests now mark the document before restoring the fixture and wait for one that no longer carries the mark, so the capture is of the reloaded page. Point that wait at a value no document will have and both fail, where before the same edit left them passing. Snapshots are untouched, and the suite passes three runs in a row. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5744 +/- ##
==========================================
+ Coverage 90.29% 90.65% +0.36%
==========================================
Files 13 13
Lines 6241 6271 +30
==========================================
+ Hits 5635 5685 +50
+ Misses 606 586 -20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Summary
Fixes #5571.
beforeunloadsetsisUnloadingand nothing ever clears it, but that event only means the page may be leaving — any listener can cancel it. When the user clicks "Cancel" on the dialog, the page stays with the flag set, soreloadAppreturns early for the rest of its life and every hot update and live reload is silently dropped until a manual refresh. Form guards and editor-integration tools install such a listener routinely.No event announces a cancelled unload. Measured in Chrome:
beforeunload,blur,focus— then nothing, page alivebeforeunload,blur,focus,pagehide,visibilitychange:hidden,unloadSo
pagehideis what separates them, but it only arrives once the next document has loaded — with the response delayed 1s and 4s, thebeforeunload→pagehidegap measured 1023ms and 4024ms. Its absence therefore can't be told from a slow navigation at any fixed moment, which is the wall #5574 hit.The suppression is now a grace period rather than permanent, with
pagehidemaking it permanent once the page really is going. The length is an explicit trade, documented at the constant: too short and a slow navigation can still be interrupted by a reload, which is what #841 added this for; too long and the cancelled dialog keeps dropping updates.pageshowclears it too, for the same bug reached the other way — navigate away, hit Back, and the page restored from the back/forward cache is the same script with the flag left set.What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Yes — five cases under
test/client/index.test.js, covering a reload with no unload in progress, suppression while the page may be leaving, the cancelled unload lapsing,pagehidekeeping it suppressed, and the back/forward-cache restore. The two that assert the fix fail onmainand the two asserting preserved behaviour still pass. Also verified end to end in Chrome against a real server, cancelling the dialog and then editing a file: onmainthe client logsApp updated. Recompiling...and stops there; with this change it goes on toApp updated. Reloading...and the page reloads.Does this PR introduce a breaking change?
No.
If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a
Use of AI
Claude Code investigated the report, measured the event sequences and timings in Chrome that the fix turns on, wrote the change and the tests, and ran the before/after verification above.
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Tests