feat(dev): React-based runtime error overlay by james-elicx · Pull Request #1029 · cloudflare/vinext

james-elicx · 2026-05-02T22:34:02Z

Summary

Adds a React-based dev-only runtime error overlay that surfaces three error sources currently only visible in the devtools console: React render errors caught by an error.tsx boundary (onCaughtError), render errors with no boundary above them (onUncaughtError), and plain script errors / unhandled promise rejections via window listeners.
The overlay renders into a separate createRoot mounted on a detached <div> appended to body, so it survives an unmount of the main hydrateRoot(document, …) tree — necessary because most errors we want to surface take that tree down.
Includes a /dev-overlay-test fixture page and a Playwright spec covering each error source plus dismiss and prev/next pagination across multiple errors.

Test plan

PLAYWRIGHT_PROJECT=app-router npx playwright test tests/e2e/app-router/dev-error-overlay.spec.ts — 7 passing
PLAYWRIGHT_PROJECT=app-router npx playwright test tests/e2e/app-router/error-handling.spec.ts tests/e2e/app-router/error-interactive.spec.ts — 13 passing (no regression in existing error tests)
pnpm exec vp check --fix clean

🤖 Generated with Claude Code

pkg-pr-new · 2026-05-02T22:40:13Z

npm i https://pkg.pr.new/vinext@1029

commit: 52dcf5c

github-actions · 2026-05-02T22:40:29Z

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original

james-elicx · 2026-05-03T00:15:00Z

/bigbonk review

ask-bonk

Good design overall — separate React root so the overlay survives the main tree tearing down, useSyncExternalStore-driven store, DevRecoveryBoundary to keep BrowserRoot alive for HMR. The architecture is sound.

However, CI is red: 4 existing E2E tests fail because the vinext dev overlay's backdrop intercepts pointer events on top of the error boundary UI those tests are trying to click. The tests in error-interactive.spec.ts were updated with the #__vinext_dev_error_overlay_root CSS hiding workaround, but the same treatment wasn't applied to error-nav.spec.ts, layout-persistence.spec.ts, or navigation-flows.spec.ts. These are the exact 4 tests failing in CI. This must be fixed before merge.

See inline comments for all issues found.

ask-bonk · 2026-05-03T00:17:59Z

@@ -1,5 +1,8 @@
+import { reportDevError } from "./dev-error-overlay.js";


Prod bundle concern: unconditional import of dev-only module

app-browser-error.ts is imported in both dev and prod (the prod createOnUncaughtError path is used unconditionally). This top-level import { reportDevError } from "./dev-error-overlay.js" pulls the entire overlay module (React component tree, 622 lines of JSX, inline CSS, createRoot import, etc.) into the production client bundle.

reportDevError is only called from devOnCaughtError and devOnUncaughtError, which are only used behind import.meta.env.DEV checks in app-browser-entry.ts. Vite/Rolldown will tree-shake the call sites away in prod, but whether it successfully tree-shakes the imported module itself depends on side-effect analysis. Since dev-error-overlay.tsx has module-level let reactRoot, let installed, and const style objects, the bundler may conservatively keep the module.

Suggestion: gate the import so it's structurally unreachable in prod:

// At the call sites in devOnCaughtError / devOnUncaughtError: const { reportDevError } = await import("./dev-error-overlay.js");

Or move devOnCaughtError and devOnUncaughtError into dev-error-overlay.tsx itself so the entire file is only imported behind import.meta.env.DEV guards in app-browser-entry.ts.

ask-bonk · 2026-05-03T00:17:59Z

+
+const FONT_STACK =
+  "ui-sans-serif, system-ui, -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, sans-serif";
+const FONT_STACK_LITERAL = FONT_STACK;


FONT_STACK_LITERAL is just assigned FONT_STACK and used in exactly one place (the template literal on line 429). This alias serves no purpose — just use FONT_STACK directly. If the intent was to avoid interpolating into the CSS string, it doesn't achieve that since it's still interpolated via ${FONT_STACK_LITERAL}.

ask-bonk · 2026-05-03T00:17:59Z

+
+  return (
+    <div style={backdropStyle} onClick={onMinimize} data-testid="vinext-dev-error-backdrop">
+      <style>{overlayStylesheet}</style>


The <style> tag containing overlayStylesheet is rendered twice — once here in DevErrorOverlay and once in DevErrorIndicator (line 154). Since the overlay app only renders one or the other at a time, this isn't a correctness issue, but:

If both were somehow mounted (e.g., a future change), you'd have duplicate @keyframes and class rules.

The stylesheet is ~60 lines of CSS that gets injected/removed on every state transition between minimized and expanded.

Consider rendering the <style> tag once at the DevErrorOverlayApp level, outside the conditional branch, so it's always present when any overlay UI is mounted and doesn't re-inject on minimize/expand toggling.

ask-bonk · 2026-05-03T00:17:59Z

+  }
+
+  componentDidCatch(error: unknown): void {
+    this.props.onCatch?.(this.props.resetKey, error);


onCatch second argument silently dropped: componentDidCatch calls this.props.onCatch?.(this.props.resetKey, error), passing two args. But the actual callback in app-browser-entry.ts is:

function handleDevRecoveryBoundaryCatch(resetKey: number): void { drainPrePaintEffects(resetKey); }

It ignores the second error argument. This works at runtime (extra args are silently ignored in JS), and the types are compatible. But it means the error is silently dropped — if someone later wants to log or report it from this callback, they'd need to remember to add the parameter. Consider accepting and discarding it explicitly: (resetKey: number, _error: unknown).

ask-bonk · 2026-05-03T00:18:00Z

@@ -0,0 +1,229 @@
+import { test, expect, type Page } from "@playwright/test";


CI is red — the overlay breaks 4 existing E2E tests that were not updated.

The vinext-dev-error-backdrop intercepts pointer events on top of the error boundary UI that these tests need to click:

error-nav.spec.ts:12 — "can navigate away from error page" (#link-to-result blocked)

error-nav.spec.ts:54 — "navigation to error page then back works" (#link-back-home blocked)

layout-persistence.spec.ts:184 — "navigating away from error and back clears the error" ([data-testid="error-go-home"] blocked)

navigation-flows.spec.ts:192 — "error → reset → navigate flow" (#error-boundary button blocked)

The fix applied in error-interactive.spec.ts (adding the CSS rule to hide #__vinext_dev_error_overlay_root) needs to be applied to these test files too. Every test that intentionally triggers an error and then tries to interact with the page underneath the overlay needs this workaround.

ask-bonk · 2026-05-03T00:18:00Z

+  if (installed || typeof window === "undefined") return;
+  installed = true;
+
+  window.addEventListener("error", (event: ErrorEvent) => {


Missing deduplication with React error callbacks: When a React render error fires, it goes through devOnCaughtError or devOnUncaughtError (which call reportDevError), AND if the error propagates to window.onerror/unhandledrejection, these listeners will also call reportDevError for the same error.

For devOnUncaughtError errors in particular: React fires the callback, then the error continues propagating up to the window error handler. The user will see the same error twice in the overlay — once as "Unhandled Runtime Error" and once as "Unhandled Script Error".

Consider adding dedup logic, e.g. a WeakSet<Error> that tracks errors already reported through React callbacks, and skip them in the window listeners.

ask-bonk · 2026-05-03T00:18:00Z

+  return snapshot;
+}
+
+export function reportToOverlay(error: ReportedError): void {


Unbounded error accumulation: reportToOverlay appends to the errors array without any limit. In a dev scenario with a hot-reloading loop that repeatedly fails (or a useEffect that throws on every render), this could accumulate hundreds or thousands of error objects in memory, each with a full stack trace string.

Consider capping the array at a reasonable limit (e.g., 50 or 100) with a FIFO eviction policy, or at least deduplicating consecutive identical errors (same message + stack).

ask-bonk · 2026-05-03T00:18:10Z

Review posted successfully. Here's a summary of what I flagged:

Blocking (CI is red):

4 existing E2E tests broken — error-nav.spec.ts (2 tests), layout-persistence.spec.ts (1 test), and navigation-flows.spec.ts (1 test) all timeout because the overlay backdrop intercepts pointer events. The error-interactive.spec.ts fix (CSS hiding #__vinext_dev_error_overlay_root) needs to be applied to these files too.

Should fix:

Prod bundle concern — app-browser-error.ts unconditionally imports reportDevError from the 622-line overlay module. This could leak dev-only code into production if the bundler's side-effect analysis isn't perfect. The fix is to either use dynamic import or move the dev-only functions into the overlay file itself.
Duplicate error reporting — No deduplication between React error callbacks and window.onerror/unhandledrejection listeners, so the same error can appear twice in the overlay.
Unbounded error accumulation — The store grows without limit, which could be a problem during a hot-reload loop that repeatedly fails.

Nits:

FONT_STACK_LITERAL is a pointless alias for FONT_STACK
<style> tag rendered in both DevErrorOverlay and DevErrorIndicator instead of once at the parent level
handleDevRecoveryBoundaryCatch silently drops the error argument

github run

james-elicx · 2026-05-03T00:33:29Z

/bigbonk review

ask-bonk

All 7 issues from the first review are resolved. CI is green. The code is clean and ready to merge.

What was fixed since last review:

CI green — all 4 broken E2E tests (error-nav.spec.ts, layout-persistence.spec.ts, navigation-flows.spec.ts, error-interactive.spec.ts) now use the disableDevErrorOverlay helper. All 30 CI checks pass.
Prod bundle isolation — app-browser-error.ts no longer imports the overlay module. The dev-only imports (devOnCaughtError, devOnUncaughtError, dismissOverlay, installDevErrorOverlay) live in app-browser-entry.ts and every call site is behind import.meta.env.DEV, so the bundler can statically eliminate them in prod.
FONT_STACK_LITERAL removed — FONT_STACK is used directly in the CSS template literal.
Single <style> tag — the stylesheet is now rendered once at the DevErrorOverlayApp level, outside the minimized/expanded conditional.
_error explicitly accepted — handleDevRecoveryBoundaryCatch signature is now (resetKey: number, _error: unknown) with an explanatory comment.
Deduplication — a WeakSet<object> (reportedErrors) tracks errors already routed through devOnCaughtError/devOnUncaughtError, and the window.onerror/unhandledrejection listeners skip them via alreadyReported().
Bounded accumulation — MAX_ERRORS = 50 with FIFO eviction in reportToOverlay.

Architecture is sound: separate React root on a detached div, useSyncExternalStore-driven store, DevRecoveryBoundary keeping BrowserRoot alive for HMR, HMR handler clearing stale errors via dismissOverlay(). No remaining concerns.

ask-bonk · 2026-05-03T00:44:12Z

Review posted. All 7 issues from the first review have been addressed:

CI is green -- the 4 broken E2E tests now use disableDevErrorOverlay
Prod bundle isolation -- dev-only imports moved behind import.meta.env.DEV guards
FONT_STACK_LITERAL -- removed, using FONT_STACK directly
Single <style> tag -- rendered once at DevErrorOverlayApp level
_error parameter -- explicitly accepted with underscore prefix
Error deduplication -- WeakSet tracks React-reported errors, skipped in window listeners
Bounded accumulation -- MAX_ERRORS = 50 with FIFO eviction

Approved the PR -- the architecture is solid and the code is clean.

github run

Surfaces three error sources that previously only reached the console: React render errors caught by an error.tsx boundary (onCaughtError), render errors with no boundary above them (onUncaughtError), and plain script errors / unhandled promise rejections via window listeners. The overlay renders into a separate createRoot mounted on a detached div appended to body, so it survives an unmount of the main hydrateRoot tree — necessary because most errors we want to surface take that tree down. Includes a /dev-overlay-test fixture and Playwright spec covering all three error sources, dismiss, and prev/next pagination across multiple errors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

james-elicx force-pushed the claude/mystifying-goldberg-20355c branch 2 times, most recently from 2894719 to 69db0ab Compare May 2, 2026 22:39

james-elicx force-pushed the claude/mystifying-goldberg-20355c branch 4 times, most recently from 15edc47 to be3922f Compare May 3, 2026 00:10

ask-bonk Bot requested changes May 3, 2026

View reviewed changes

james-elicx force-pushed the claude/mystifying-goldberg-20355c branch 2 times, most recently from 998e946 to 33626ee Compare May 3, 2026 00:32

james-elicx force-pushed the claude/mystifying-goldberg-20355c branch 2 times, most recently from 1eed865 to 92dc3df Compare May 3, 2026 00:36

ask-bonk Bot approved these changes May 3, 2026

View reviewed changes

james-elicx force-pushed the claude/mystifying-goldberg-20355c branch from 92dc3df to a44dca3 Compare May 3, 2026 16:49

james-elicx marked this pull request as ready for review May 3, 2026 17:04

james-elicx force-pushed the claude/mystifying-goldberg-20355c branch from a44dca3 to 52dcf5c Compare May 3, 2026 17:07

james-elicx merged commit fffda16 into main May 3, 2026
33 checks passed

james-elicx deleted the claude/mystifying-goldberg-20355c branch May 3, 2026 17:12

		@@ -1,5 +1,8 @@
		import { reportDevError } from "./dev-error-overlay.js";

		@@ -0,0 +1,229 @@
		import { test, expect, type Page } from "@playwright/test";

Conversation

james-elicx commented May 2, 2026

Summary

Test plan

Uh oh!

pkg-pr-new Bot commented May 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 2, 2026

Uh oh!

james-elicx commented May 3, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot May 3, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot May 3, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot May 3, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot May 3, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot May 3, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot May 3, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot May 3, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented May 3, 2026

Uh oh!

james-elicx commented May 3, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented May 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

pkg-pr-new Bot commented May 2, 2026 •

edited

Loading