Fix the html preview crash and the blast radius that turned it into a blank window - #44
Merged
Merged
Conversation
… webview
Opening an html preview and then splitting or closing the pane blanked the
whole window. The preview was an Electron <webview>, whose getWebContentsId()
throws until the guest attaches and fires dom-ready:
if(!e||!e.guestInstanceId) throw new Error("The WebView must be attached to
the DOM and the dom-ready event emitted before this method can be called.")
Cmd+D and Cmd+W both unmount it, so unmounting before that landed threw inside
an effect cleanup. The tab's ErrorBoundary is unmounting in the same commit, so
nothing caught it and React tore down the entire root.
Previews also hung on "loading" forever, because the attach handler hard-set
webPreferences.javascript = false. Any page that draws itself with a script
showed its own pre-render placeholder and never advanced.
Both are gone because the webview is gone. Each preview now loads in a plain
iframe on its own dorabot-preview:// origin, served by a protocol handler that
refuses any path outside the folder the file lives in. There is no attach
lifecycle left to lose a race against, so the crash is not guarded, it is
unreachable.
The origin is doing real work. Measured with scripts enabled on both schemes:
file:// fetch sibling OK | /etc/passwd 200, 9344 bytes
dorabot-preview:// fetch sibling OK | /etc/passwd 404
A scripted file:// preview can read the whole disk, so simply enabling
javascript on the old path would have handed agent-written html every file the
user can read.
Remote loading is now one CSP header set by the same handler rather than a
webRequest filter, a partition, an allow-list of webContents ids and an ipc
event stream, all of which are deleted.
Two protections that lived on the webview are re-implemented, because dropping
them silently would have been a regression. A hostile preview confirmed it:
before reachedNetwork ["exfil-selfnav"] frame left on https://example.com
after blocked, frame stayed on dorabot-preview://
CSP governs what a frame may load, not where it may go, so location.href still
escaped. will-frame-navigate now blocks a preview frame navigating off its own
origin, and permission requests from preview origins are denied. fetch, image
beacons, window.open and path escapes were already blocked.
Net effect is 53 fewer lines of production code and one fewer moving part.
main.tsx rendered <App/> bare. The only ErrorBoundary in the renderer sat inside EditorGroupPanel, which is itself part of the subtree that unmounts when a tab closes, so it could not catch anything thrown on the way out. That is why a single unguarded call in one viewer blanked the entire app rather than one pane. Wrap the root, and reload rather than re-render on reset, because a root-level failure usually recurs on retry. Then remove the calls that were most likely to reach it: FileExplorer called el.scrollIntoView() unguarded. A purely cosmetic scroll should never be able to take the render tree down. This also removes five uncaught exceptions from the test run, which had been passing 62 tests while throwing on the side. Four localStorage.setItem calls were unguarded while two others, one of them three lines away in the same hook, already carried "quota exceeded, ignore". On a full profile the guarded write swallows and the next one unwinds the tree. The useEditorPrefs case is the worst shape of the four: the write sits inside a setState updater, so it runs during render. Quota is unlikely here, since only ids, paths and labels are persisted and diff tabs strip their contents first. The inconsistency is the defect.
Nothing in CI executed the test suite or a typecheck. security.yml runs npm audit, claude-review.yml posts a review, build.yml is workflow_dispatch only, and auto-release.yml cuts a release on any push to main that touches package.json. So the path from merge to a published DMG had no automated verification at all. That is how a preview feature reached v0.2.98 with a bug that blanks the app on a common key combination, while 65 passing tests sat in the repo unrun. Also fails on vitest's Errors line, not just on failed tests. Unhandled errors outside a test do not affect the exit code, which is exactly how five real uncaught exceptions went unnoticed while every test reported green. Pinned to node 22 to match build.yml, and confirmed the suite passes there. Next step, deliberately not taken here: make auto-release.yml depend on this job, so a red build cannot ship.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Opening an html preview and then splitting or closing the pane blanked the entire app. Two separate defects had to line up for that, and both shipped in v0.2.98.
The crash
The preview was an Electron
<webview>. ItsgetWebContentsId()throws until the guest attaches and firesdom-ready, verbatim from the Electron 40.8.5 binary:Cmd+D(splitHorizontal) andCmd+W(closeTab) both unmount it. Unmount before that lands and the effect cleanup throws. The tab'sErrorBoundaryis unmounting in the same commit, so nothing catches it and React tears down the root.Reproduced against the shipped code: 3 of 4 new tests fail with that exact error escaping
unmount(). All 4 pass after.The hang
will-attach-webviewhard-setwebPreferences.javascript = false, so any page that draws itself with a script sat on its own placeholder forever.The fix
The webview is gone. Each preview loads in a plain iframe on its own
dorabot-preview://origin, served by a handler that refuses any path outside the previewed file's folder. No attach lifecycle means the crash is unreachable rather than guarded.The origin is load-bearing. Measured with scripts on, both schemes:
/etc/passwdfile://dorabot-preview://So just flipping
javascripton the old path would have handed agent-written html every file the user can read.Remote loading is now one CSP header instead of a webRequest filter, a partition, an id allowlist and an ipc event stream, all deleted.
Two webview protections are re-implemented rather than dropped. A hostile preview proved it mattered:
CSP governs what a frame may load, not where it may go.
will-frame-navigatenow blocks a preview leaving its origin, and permission requests from preview origins are denied.Blast radius
main.tsxrendered<App/>bare. The onlyErrorBoundaryin the renderer lived insideEditorGroupPanel, i.e. inside the subtree that unmounts when a tab closes, so it could never catch anything on the way out. That is why one unguarded call blanked the whole app instead of one pane. Now wrapped at the root, reloading rather than re-rendering on reset.Then the calls most likely to reach it: an unguarded
el.scrollIntoView()(which also removes five uncaught exceptions the suite had been throwing while reporting green), and four unguardedlocalStorage.setItemcalls sitting alongside two that already carriedcatch { /* quota exceeded, ignore */ }- one of them three lines away in the same hook.CI
Nothing in CI ran the test suite or a typecheck.
security.ymlis npm audit,build.ymlis manual, andauto-release.ymlcuts a release on any push to main touchingpackage.json. The path from merge to published DMG had no verification, which is how this reached a release while 65 passing tests sat unrun.Also fails on vitest's
Errorsline, since unhandled errors outside a test do not affect the exit code.Deliberately not done here: making
auto-release.ymldepend on this job, so a red build cannot ship.Verification
65 tests, desktop and backend typecheck,
electron-vite buildall green. Net 53 fewer lines of production code.Not verified: behaviour in a packaged build, and whether the iframe shares the renderer process the way
<webview>did not. Origin isolation holds regardless.🤖 Generated with Claude Code