fix: client HMR dev overlay recovery - #1758
Conversation
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: client HMR dev overlay recovery
Nice, focused follow-up to #1746. #1746 covered server/RSC HMR recovery; this extends recovery to client component Fast Refresh, which takes a different code path. I verified the design and behavior:
__registerBeforePerformReactRefreshis a real hook exposed by@vitejs/plugin-react's refresh runtime (node_modules/.../refresh-runtime.js:599). The registered callbacks areawaited beforeperformReactRefresh()(line 602-605), so dispatching the recovery event there correctly resets error boundaries before the refreshed modules re-render. Good choice of seam.- Recovery coverage is complete: generic client render errors are only caught by
ErrorBoundaryInner(whenerror.tsxexists) or the catch-allDevRecoveryBoundary. Both got the listener. The other boundaries (RedirectErrorBoundary,NotFoundBoundary,Forbidden,Unauthorized) only catch navigation-signal errors, so they correctly don't need it. - The defensive retry scheduling (
queueMicrotask+setTimeoutfallback) is reasonable given the runtime preamble normally loads before app code, so the first synchronoustryInstallwill almost always succeed. - The
dismissOverlay()added toreportViteHmrError(and in the refresh callback) correctly prevents stale runtime errors from lingering in overlay pagination when a build error or new runtime error supersedes them. The three new e2e tests cover toggle, runtime-replace, and build-error-replace cases.
Verification:
vp checkon all 4 changed source/test files: pass (format, lint, type)vp test run tests/app-browser-entry.test.ts -t "React Refresh recovery": 2 passed
Only minor, non-blocking observations below. LGTM.
| if (typeof register !== "function") return false; | ||
|
|
||
| refreshWindow.__vinextReactRefreshErrorRecoveryInstalled = true; | ||
| register(() => { |
There was a problem hiding this comment.
Minor (non-blocking): this callback runs dismissOverlay() on every Fast Refresh, including refreshes with no active error. dismissOverlay() is not idempotent — it always reassigns the snapshot and calls emit() (dev-error-overlay-store.ts:102-105), notifying subscribers even when the overlay is already empty. In practice each refresh is debounced at 16ms so the extra emit is negligible, but you could guard it with a snapshot check (e.g. only dismiss when there are errors) if you want to avoid the redundant store notification. Not required.
| void Promise.resolve().then(retry); | ||
| } | ||
| window.setTimeout(retry, 0); | ||
| } |
There was a problem hiding this comment.
Minor (non-blocking): if both the microtask and the setTimeout(0) retry fire before the refresh runtime has set __registerBeforePerformReactRefresh, retry() resets __vinextReactRefreshErrorRecoveryInstallScheduled = false and no further attempt is scheduled. Since installReactRefreshErrorRecovery() is only called once via installDevErrorOverlay() at startup, recovery would silently never install in that (unlikely) race. The refresh preamble is normally injected before app code so the first synchronous tryInstall succeeds, making this purely theoretical — just noting it in case the install site ever changes.
|
Review posted successfully. Summary of my findings: Review complete — PR #1758 LGTMThis is a clean, focused follow-up to #1746 that extends dev overlay recovery from server/RSC HMR to client component Fast Refresh. What I verified:
Two minor, non-blocking notes posted inline:
Neither is a blocker. |
|
/bigbonk review |
There was a problem hiding this comment.
Re-review: client HMR dev overlay recovery
Verified independently — this is a clean, well-tested follow-up to #1746 that extends overlay recovery to client-component Fast Refresh. Agree with the prior review: LGTM.
What I confirmed:
__registerBeforePerformReactRefreshis the right seam — its callbacks are awaited beforeperformReactRefresh(), so resetting the error boundaries before refreshed modules re-render is correct.- Recovery listeners land on the two boundaries that catch generic client render errors:
ErrorBoundaryInner(productionerror.tsxwrapper) and the dev-onlyDevRecoveryBoundary. Navigation-signal-only boundaries correctly don't get it. - The new unit tests pass locally (
vp test run tests/app-browser-entry.test.ts -t "React Refresh recovery"→ 2 passed). The 3 e2e scenarios (toggle, runtime-replace, build-error-replace) cover the meaningful paths. - The dev recovery listener in
ErrorBoundaryInneris unconditional (noimport.meta.env.DEVguard), but it's harmless in production since the event is never dispatched outside the dev overlay. Negligible per-boundaryaddEventListenercost.
Three minor, non-blocking notes inline. None block merge.
| // Vite build errors describe the current HMR update. Replace any previous | ||
| // runtime/HMR failure so stale errors from earlier edits do not remain in the | ||
| // overlay pagination. | ||
| dismissOverlay(); |
There was a problem hiding this comment.
Non-blocking: this unconditional dismissOverlay() changes existing Vite-HMR behavior — every vite:error now clears the entire overlay before reporting the new build error, not just superseding runtime errors. In practice Vite emits one error per failed update so this matches the intended "build error replaces stale runtime error" behavior (and the new e2e test covers it). Just flagging that any previously-stacked entries (including other Vite errors) are dropped too, which is the desired outcome here but worth a one-line note in the comment if you want to be explicit.
| } else { | ||
| void Promise.resolve().then(retry); | ||
| } | ||
| window.setTimeout(retry, 0); |
There was a problem hiding this comment.
Non-blocking (same as noted previously): if both the microtask and setTimeout(0) retries fire before the refresh runtime sets __registerBeforePerformReactRefresh, retry() clears __vinextReactRefreshErrorRecoveryInstallScheduled and nothing reschedules — recovery would silently never install. The preamble normally loads first so the synchronous tryInstall succeeds, making this purely theoretical. A small hardening would be to keep retrying (e.g. re-arm via another setTimeout) instead of giving up after the second attempt.
| refreshWindow.__vinextReactRefreshErrorRecoveryInstalled = true; | ||
| register(() => { | ||
| window.dispatchEvent(new Event(VINEXT_DEV_ERROR_RECOVERY_EVENT)); | ||
| dismissOverlay(); |
There was a problem hiding this comment.
Non-blocking: this callback runs dismissOverlay() on every Fast Refresh, even when no error is active. dismissOverlay() always reassigns the snapshot and calls emit() (dev-error-overlay-store.ts:102-105), so subscribers get a redundant notification when the overlay is already empty. Negligible given the 16ms debounce, but you could guard with a snapshot check if you want to avoid the no-op emit.
| callbacks.push(cb); | ||
| }, | ||
| dispatchEvent, | ||
| setTimeout: vi.fn(), |
There was a problem hiding this comment.
Minor: stubbing setTimeout: vi.fn() plus providing __registerBeforePerformReactRefresh up front means the synchronous tryInstall always succeeds, so the deferred retry path (microtask / setTimeout fallback) is never exercised by these tests. Consider adding a case where __registerBeforePerformReactRefresh is initially absent then set, invoking the captured retry, to cover the scheduling branch.
|
Review posted successfully. Review complete — PR #1758 LGTMI independently verified this follow-up to #1746, which extends dev-overlay recovery from server/RSC HMR to client-component Fast Refresh. Verified:
4 minor, non-blocking notes posted inline:
None of these block merge. |
|
/bigbonk review |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk re-review for issues |
There was a problem hiding this comment.
Re-review: client HMR dev overlay recovery — LGTM
Independently verified this follow-up to #1746. It extends dev-overlay recovery from server/RSC HMR to client-component Fast Refresh, and it has addressed the minor points raised in the earlier /bigbonk reviews.
What I confirmed:
- Correct seam.
__registerBeforePerformReactRefreshis a real hook from@vitejs/plugin-react's refresh runtime, and its callbacks are awaited beforeperformReactRefresh(). Dispatchingvinext:dev-error-recoverythere resets the error boundaries before refreshed modules re-render. Right choice. - Boundary coverage is complete. The listener lands on the two boundaries that catch generic client render errors —
ErrorBoundaryInner(error-boundary.tsx:220) and the dev-onlyDevRecoveryBoundary(error-boundary.tsx:550). Navigation-signal-only boundaries (NotFound/Redirect/Forbidden/Unauthorized) correctly don't get it. Both handlers no-op whenstate.erroris null, and both clean up incomponentWillUnmount. - Idempotency note from prior reviews is now fixed.
dismissOverlay()got an early-return guard (dev-error-overlay-store.ts:103), so the redundant store emit on no-op refreshes is gone, with a dedicated unit test ("does not notify subscribers when an empty overlay is dismissed"). - Retry-install "race" from prior reviews does not actually exist. I traced
installReactRefreshErrorRecovery(dev-error-overlay.tsx:98-130): on failure,retry()always re-armsscheduleTimeoutRetry(16)and only clears__vinextReactRefreshErrorRecoveryInstallScheduledon success — so it keeps retrying indefinitely rather than giving up after the second attempt. The new "retries until the React Refresh runtime hook is available" test exercises this branch directly. - Prod safety. The unconditional
addEventListenerinErrorBoundaryInneris a harmless no-op in production since the event is never dispatched outside the dev overlay. - Build-error replacement. The new
dismissOverlay()inreportViteHmrError(dev-error-overlay.tsx:219) is documented with a comment and covered by the build-error e2e test.
Verification run locally:
vp checkon all 4 changed source files → pass (format, lint, type)vp test run tests/app-browser-entry.test.ts -t "React Refresh recovery"→ 3 passedvp test run tests/app-browser-entry.test.ts -t "dev overlay store"→ 1 passed
No blocking issues. One trivial optional nit inline.
| scheduleTimeoutRetry(REACT_REFRESH_RECOVERY_RETRY_DELAY_MS); | ||
| } | ||
|
|
||
| if (typeof queueMicrotask === "function") { |
There was a problem hiding this comment.
Optional / trivial: scheduleTimeoutRetry(0) already schedules a retry on the next macrotask, so the queueMicrotask path is a small head-start optimization rather than load-bearing. The fallback void Promise.resolve().then(retry) for environments without queueMicrotask is dead in practice (any runtime with window.setTimeout has queueMicrotask), but it's harmless. No change needed.
|
Review posted successfully. Re-review complete — PR #1758 LGTMI independently re-reviewed this follow-up to #1746, which extends dev-overlay recovery from server/RSC HMR to client-component Fast Refresh. Verified:
Local checks: One trivial optional inline nit (dead |
|
fyi @hyoban, the new overlay tests are appearing to be rather flaky - https://github.com/cloudflare/vinext/actions/runs/27077153897/job/79916376566 |
2 participants
Follow #1746