fix(overlay): keep a runtime error a successful build cannot vouch for - #2424
Conversation
An entry that throws while it is still evaluating puts the overlay up before the transport has finished connecting. The catch-up sync arrives right behind it reporting no problems, and the reporter cleared the runtime slot on every clean build — so the overlay was dismissed milliseconds after appearing, and a page that threw on load looked fine. This is webpack-dev-server#5024, which webpack-dev-server fixes by splitting the two cases; this one had not. A build that merely succeeded says nothing about an error the page threw by itself, so it no longer clears those. A rebuild does, from `building`, because that replaces the code the error came from — matching webpack-dev-server's `BUILD_OK` and `DISMISS` split. What a rebuild looks like from the outside is unchanged. Also covers the `runtimeErrors` filter arriving through the resource query, which turned out to work already: encoded in both the layers webpack-dev-server uses, a filter round-trips intact. Writing that query by hand does not, since `encodeURIComponent` leaves `!` alone and webpack reads it as the inline-loader separator. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
🦋 Changeset detectedLatest commit: 899cd33 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. 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 (4)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. WalkthroughThe reporter now clears runtime problems separately from build problems. Successful builds clear build problems without dismissing runtime errors. A building message clears runtime problems. New browser tests cover runtime-error filtering and the persistence of an entry-evaluation error after catch-up sync. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The overlay change is ready to merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to A successful build now leaves an existing runtime error visible, as intended. A rebuild that starts but never completes can instead dismiss that error while the faulty page remains in place. No new privileged access or sensitive-data path was established. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2424 +/- ##
==========================================
- Coverage 96.19% 96.14% -0.05%
==========================================
Files 17 17
Lines 1890 1894 +4
==========================================
+ Hits 1818 1821 +3
- Misses 72 73 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Summary
An entry that throws while it is still evaluating puts the overlay up before the transport has finished connecting. The catch-up sync arrives right behind it reporting no problems, and the reporter cleared the
runtimeslot on every clean build — so the overlay was dismissed milliseconds after appearing, and a page that threw on load looked fine.This is webpack-dev-server#5024. webpack-dev-server fixes it by splitting the two cases; this package had them inverted:
DISMISS— build and runtimeBUILD_OK— build onlySo a runtime error was dropped by the one event that cannot vouch for it, and survived the one event that genuinely invalidates it.
Now: a build that merely succeeded clears only its own build problems, and a rebuild clears runtime errors from
building— that replaces the code the error came from. Same split as webpack-dev-server, reached through this package's per-source overlay slots rather than a state machine.What a rebuild looks like from the outside is unchanged, which is why the existing
resets the runtime slot on a clean buildcase still passes untouched: editing a file publishesbuildingbefore the rebuild, so the error still goes away — just one event earlier.On the
runtimeErrorsfilter through the resource queryI had flagged this as a possible second gap. It is not one: a filter round-trips intact when it is encoded in both the layers webpack-dev-server uses —
encodeURIComponenton the function, thenURLSearchParamson the parameter. My earlier attempt hand-wrote the query, andencodeURIComponentleaves!alone, which webpack reads as its inline-loader separator — so the entry request was mangled before the client ever saw it. The new test encodes both layers and covers accept and reject, so this behaviour is now pinned rather than assumed.What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Yes, two end-to-end cases:
subscribeAllhaving handled thesync(a sync that changes nothing writes no log line, so waiting on the console would not do) — teeth-checked: restoring the old clear fails it on the selector timeout;runtimeErrorsfilter through the query, asserting both the error it rejects and the one it accepts, so a filter that failed to decode cannot pass by suppressing everything.Verified locally: end-to-end 102/102, non-browser 6810 passed, lint and both typechecks clean.
test/logging.test.jsfails 74/74 on a cleanmaintoo, unrelated to this.lint:types-client— which is a separate script fromlint:types— caught a missing method on theClientReporterinterface inclient-src/globals.d.ts; added.Does this PR introduce a breaking change?
No. A runtime error now outlives a successful build instead of being dismissed by it; everything a rebuild does is unchanged.
If relevant, what needs to be documented once your changes are merged or what have you already documented?
Nothing — this restores expected behaviour rather than adding a knob.
Use of AI
AI-assisted (Claude Code). Used to locate the divergence against webpack-dev-server's overlay, write the fix and both tests, and verify it: the page was instrumented before any script ran to observe the overlay being raised and then wiped, and the fix was teeth-checked by restoring the old behaviour and confirming the new test fails.
🤖 Generated with Claude Code
https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
Summary by CodeRabbit