fix(browser): stop webview NSView leak on close + tighten toolbar#75
Merged
Merged
Conversation
…lbar Two independent fixes for the embedded browser pane. 1) Webview leaks on pane close `BrowserView::close` only persisted state and emitted PaneEvent::Close. With the default `UndoClosedPanes` feature flag, pane_group keeps the `BrowserView` alive in a shadow state instead of dropping it, so the `Vec<Rc<RefCell<NativeBrowserWebView>>>` survives, the wry::WebView survives, and its WKWebView NSView stays attached to the parent NSView. The result is a visible artifact of the browser content painting over the workspace after the pane is gone. Add `NativeBrowserWebView::detach_native()` which drops the wry::WebView (triggering wry's Drop that removes the NSView) without touching `desired_visible`. Call it for every webview from `BrowserView::close`. If the pane is restored via Cmd+Shift+T, `set_bounds`/`attach_if_needed` will rebuild the webview from `pending_url` on the next paint; page state since closure is lost but the URL is preserved. 2) Toolbar too tall `TOOLBAR_HEIGHT` was 48pt around a 32pt URL bar, leaving ~16pt of dead space and making the browser chrome look bulky next to neighboring panes. Tightened to 36pt (URL bar + 4pt total padding, 2pt each side). Verified: - cargo check -p warp-app --bin cast-codes --features gui -> clean, 8 pre-existing warnings. - Manual relaunch pending; visual confirmation required for the toolbar tightening and for the close-artifact fix. Out of scope: a third reported symptom -- HTML in-page clicks dead inside the webview -- has a deeper root cause in GPUI NSView hit-test routing (the parent canvas's `hitTest:withEvent:` likely returns self before subviews are tested). That needs framework-level work in the warpui crate and will be addressed separately. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses two UI issues in the embedded browser pane: it ensures native WKWebView views are detached on pane close to prevent macOS NSView artifacts when UndoClosedPanes keeps the pane alive, and it tightens the toolbar height to reduce excess vertical space.
Changes:
- Add
NativeBrowserWebView::detach_native()to drop the underlyingwry::WebView(detaching the NSView) while preserving intended visibility state. - Invoke
detach_native()for all tab webviews inBrowserView::close()before persisting state and emitting the close event. - Reduce
TOOLBAR_HEIGHTfrom 48pt to 36pt and update the surrounding sizing rationale comment.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| app/src/browser/webview_host.rs | Adds a detach_native() helper to explicitly drop the native webview so its NSView is removed immediately. |
| app/src/browser/browser_view.rs | Calls detach_native() on close to prevent lingering native views, and tightens toolbar height constant/commentary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+55
to
+56
| // Toolbar = URL bar height + 4pt total vertical padding (2pt each side). The | ||
| // previous 48pt left ~16pt of dead space around a 32pt input and made the |
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
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.
Description
Two small fixes for the embedded browser pane.
Webview leak on pane close.
BrowserView::closeonly persisted state and emittedPaneEvent::Close. With the defaultUndoClosedPanesfeature flag the pane group keepsBrowserViewalive in a shadow state instead of dropping it, so theVec<Rc<RefCell<NativeBrowserWebView>>>survives, thewry::WebViewsurvives, and its WKWebView NSView stays attached to the parent NSView — painting as a visible artifact of the browser content over the workspace after the pane is gone.Fix: add
NativeBrowserWebView::detach_native()which drops thewry::WebView(triggering wry'sDrop, which callsremoveFromSuperview) without touchingdesired_visible.BrowserView::closecalls it for every webview. If the pane is restored via Cmd+Shift+T,set_bounds→attach_if_neededrebuilds the webview frompending_urlon the next paint. Page state since closure is lost; URL is preserved.Toolbar too tall.
TOOLBAR_HEIGHTwas 48pt around a 32pt URL bar — ~16pt of dead space that made the chrome look bulky next to neighboring panes. Tightened to 36pt (URL bar + 4pt total padding, 2pt each side).Linked Issue
No linked issue — reported directly during manual testing of #74.
ready-to-specorready-to-implement.Testing
cargo check -p warp-app --bin cast-codes --features gui→ clean, 8 pre-existing warnings.Manually built and ran via
./script/run; reporter confirmed "browser closes clean" — no artifact remains after closing a browser pane.I have manually tested my changes locally with
./script/runScreenshots / Videos
To add — before/after of toolbar height; close-pane recording showing no leftover content. The artifact bug is fully reproducible on
main(open browser pane → close → see lingering webview content over workspace) and clean on this branch.Agent Mode
Out of scope
A third symptom reported alongside these — HTML in-page clicks dead inside the embedded webview (
<select>dropdowns and in-page buttons all unresponsive) — is not fixed here. Root cause appears to be inwarpui(GPUI fork) NSView hit-test routing: the parent canvas'shitTest:withEvent:likely returnsselfbefore subviews are tested, so AppKit never delivers events to the WKWebView. That needs framework-level work and deserves its own investigation/PR.Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com