Keep background Browser WebViews automatable#6027
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR implements automation command leases for offscreen WebView hosting during browser automation. A new ChangesAutomation Command Lease for Offscreen Hosting
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (19 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/BrowserScreenshotSnapshotter.swift`:
- Around line 63-184: Move the OffscreenRenderHostLease class (and the
BrowserScreenshotOffscreenRenderPanel type if it’s declared in the same block)
out of BrowserScreenshotSnapshotter.swift into a new file under Sources/Panels
(e.g., OffscreenRenderHostLease.swift); keep the `@MainActor` attribute, any
imports, and the same access level, and ensure the new file declares the same
types (OffscreenRenderHostLease and BrowserScreenshotOffscreenRenderPanel) so
existing callers (e.g., BrowserScreenshotWebViewSnapshotter.restoreWebView and
any usages in BrowserScreenshotSnapshotter) still compile; after moving, remove
the nested/inline definition from BrowserScreenshotSnapshotter.swift and run a
build to fix any missing import/access issues or to adjust visibility if
necessary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1efa4673-b211-4e5d-9981-2bd079346d37
📒 Files selected for processing (4)
Sources/Panels/BrowserPanel.swiftSources/Panels/BrowserScreenshotSnapshotter.swiftSources/TerminalController.swiftcmuxTests/BrowserPanelTests.swift
👮 Files not reviewed due to content moderation or server errors (1)
- Sources/TerminalController.swift
| @MainActor | ||
| final class OffscreenRenderHostLease { | ||
| private weak var webView: WKWebView? | ||
| private let previousSuperview: NSView? | ||
| private let previousFrame: NSRect | ||
| private let previousBounds: NSRect | ||
| private let previousAutoresizingMask: NSView.AutoresizingMask | ||
| private let previousTranslatesAutoresizingMaskIntoConstraints: Bool | ||
| private let restoreAnchor: NSView? | ||
| private let restorePosition: NSWindow.OrderingMode | ||
| private let window: BrowserScreenshotOffscreenRenderPanel | ||
| private var isActive = true | ||
|
|
||
| init(webView: WKWebView, viewportSize: NSSize) { | ||
| self.webView = webView | ||
| previousSuperview = webView.superview | ||
| let previousSubviews = previousSuperview?.subviews ?? [] | ||
| let previousIndex = previousSubviews.firstIndex(of: webView) | ||
| previousFrame = webView.frame | ||
| previousBounds = webView.bounds | ||
| previousAutoresizingMask = webView.autoresizingMask | ||
| previousTranslatesAutoresizingMaskIntoConstraints = webView.translatesAutoresizingMaskIntoConstraints | ||
|
|
||
| if let previousIndex, previousIndex > 0 { | ||
| restoreAnchor = previousSubviews[previousIndex - 1] | ||
| restorePosition = .above | ||
| } else if let previousIndex, previousIndex == 0, previousSubviews.count > 1 { | ||
| restoreAnchor = previousSubviews[1] | ||
| restorePosition = .below | ||
| } else { | ||
| restoreAnchor = nil | ||
| restorePosition = .above | ||
| } | ||
|
|
||
| let normalizedSize = Self.normalizedViewportSize(viewportSize) | ||
| let frame = NSRect( | ||
| x: -100_000 - normalizedSize.width, | ||
| y: -100_000 - normalizedSize.height, | ||
| width: normalizedSize.width, | ||
| height: normalizedSize.height | ||
| ) | ||
| let window = BrowserScreenshotOffscreenRenderPanel( | ||
| contentRect: frame, | ||
| styleMask: [.borderless, .nonactivatingPanel], | ||
| backing: .buffered, | ||
| defer: false | ||
| ) | ||
| window.isReleasedWhenClosed = false | ||
| window.identifier = NSUserInterfaceItemIdentifier("cmux.browserVisualAutomationRender") | ||
| window.hasShadow = false | ||
| window.isOpaque = false | ||
| window.backgroundColor = .clear | ||
| window.alphaValue = 0.01 | ||
| window.ignoresMouseEvents = true | ||
| window.hidesOnDeactivate = false | ||
| window.collectionBehavior = [.transient, .ignoresCycle, .stationary, .canJoinAllSpaces] | ||
| window.isExcludedFromWindowsMenu = true | ||
| self.window = window | ||
|
|
||
| let contentView = NSView(frame: NSRect(origin: .zero, size: normalizedSize)) | ||
| contentView.wantsLayer = true | ||
| webView.removeFromSuperview() | ||
| webView.frame = contentView.bounds | ||
| webView.autoresizingMask = [.width, .height] | ||
| contentView.addSubview(webView) | ||
| window.contentView = contentView | ||
| window.orderFrontRegardless() | ||
| } | ||
|
|
||
| func end() { | ||
| guard isActive else { return } | ||
| isActive = false | ||
| if let webView { | ||
| Self.restoreWebView( | ||
| webView, | ||
| to: previousSuperview, | ||
| frame: previousFrame, | ||
| bounds: previousBounds, | ||
| autoresizingMask: previousAutoresizingMask, | ||
| translatesAutoresizingMaskIntoConstraints: previousTranslatesAutoresizingMaskIntoConstraints, | ||
| anchor: restoreAnchor, | ||
| position: restorePosition | ||
| ) | ||
| } | ||
| window.orderOut(nil) | ||
| window.contentView = nil | ||
| window.close() | ||
| } | ||
|
|
||
| deinit { | ||
| guard isActive else { return } | ||
| MainActor.assumeIsolated { | ||
| end() | ||
| } | ||
| } | ||
|
|
||
| private static func normalizedViewportSize(_ viewportSize: NSSize) -> NSSize { | ||
| BrowserScreenshotWebViewSnapshotter.normalizedViewportSize(viewportSize) | ||
| } | ||
|
|
||
| private static func restoreWebView( | ||
| _ webView: WKWebView, | ||
| to superview: NSView?, | ||
| frame: NSRect, | ||
| bounds: NSRect, | ||
| autoresizingMask: NSView.AutoresizingMask, | ||
| translatesAutoresizingMaskIntoConstraints: Bool, | ||
| anchor: NSView?, | ||
| position: NSWindow.OrderingMode | ||
| ) { | ||
| BrowserScreenshotWebViewSnapshotter.restoreWebView( | ||
| webView, | ||
| to: superview, | ||
| frame: frame, | ||
| bounds: bounds, | ||
| autoresizingMask: autoresizingMask, | ||
| translatesAutoresizingMaskIntoConstraints: translatesAutoresizingMaskIntoConstraints, | ||
| anchor: anchor, | ||
| position: position | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Extract the lease/panel helpers to a dedicated file to stay within the Swift file-size budget.
This new lease abstraction is coherent, but adding it here pushes Sources/Panels/BrowserScreenshotSnapshotter.swift past the 800-line budget. Please move OffscreenRenderHostLease (and, if appropriate, BrowserScreenshotOffscreenRenderPanel) into a focused companion file under Sources/Panels/.
As per coding guidelines, {Sources,CLI,Packages,cmuxTests,cmuxUITests}/**/*.swift should be flagged when a production Swift file exceeds 800 lines, even with coherent responsibility.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/Panels/BrowserScreenshotSnapshotter.swift` around lines 63 - 184,
Move the OffscreenRenderHostLease class (and the
BrowserScreenshotOffscreenRenderPanel type if it’s declared in the same block)
out of BrowserScreenshotSnapshotter.swift into a new file under Sources/Panels
(e.g., OffscreenRenderHostLease.swift); keep the `@MainActor` attribute, any
imports, and the same access level, and ensure the new file declares the same
types (OffscreenRenderHostLease and BrowserScreenshotOffscreenRenderPanel) so
existing callers (e.g., BrowserScreenshotWebViewSnapshotter.restoreWebView and
any usages in BrowserScreenshotSnapshotter) still compile; after moving, remove
the nested/inline definition from BrowserScreenshotSnapshotter.swift and run a
build to fix any missing import/access issues or to adjust visibility if
necessary.
Source: Coding guidelines
Greptile SummaryRoutes all socket JS commands through a new
Confidence Score: 3/5Functional for single-concurrent-command usage, but two simultaneous socket commands on the same hidden panel can leave the webview parented to a detached view; the deinit safety net in OffscreenRenderHostLease also uses an unchecked main-thread assertion that crashes if ever triggered off-thread. The happy path (one socket command at a time, hidden panel) is correct and well-tested. The two issues — non-LIFO lease teardown when commands overlap, and the Sources/Panels/BrowserScreenshotSnapshotter.swift (deinit safety net) and Sources/TerminalController.swift (concurrent lease ordering). Important Files Changed
Sequence DiagramsequenceDiagram
participant WT as Worker Thread
participant Main as Main Actor
participant BP as BrowserPanel
participant Lease as OffscreenRenderHostLease
WT->>Main: "v2MainSync { beginAutomationCommandLease }"
Main->>BP: "activeVisualAutomationCaptureCount += 1"
BP->>BP: cancelHiddenWebViewDiscard()
BP->>BP: restoreDiscardedWebViewIfNeeded()
BP->>Lease: init(webView, viewportSize)
Lease->>Lease: capture previousSuperview
Lease->>Lease: move webView to offscreen window
Main-->>WT: return lease
WT->>WT: body(ctx) — JS eval / snapshot
WT->>Main: "defer: v2MainSync { endAutomationCommandLease(lease) }"
Main->>Lease: end()
Lease->>Lease: restore webView to previousSuperview
Lease->>Lease: close offscreen window
Main->>BP: "activeVisualAutomationCaptureCount -= 1"
BP->>BP: scheduleHiddenWebViewDiscardIfNeeded (if still hidden)
Reviews (1): Last reviewed commit: "Keep background browser surfaces automat..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| deinit { | ||
| guard isActive else { return } | ||
| MainActor.assumeIsolated { | ||
| end() | ||
| } | ||
| } |
There was a problem hiding this comment.
assumeIsolated in deinit is an unchecked crash
@MainActor final class does not guarantee that deinit runs on the main actor in current Swift (SE-0371 "Isolated synchronous deinit" was returned for revision and is not in the language). If the last strong reference to an OffscreenRenderHostLease is released off the main thread when isActive is still true, MainActor.assumeIsolated will preconditionFailure-crash the app.
In the current flow the guard short-circuits in every reachable path (endAutomationCommandLease sets isActive = false before dropping the lease), but the safety net is meant to fire for unexpected drops — precisely the case where thread provenance is unknown. The correct defensive pattern is DispatchQueue.main.async { self.end() } (accepting that cleanup is deferred by one run-loop turn) rather than an unchecked assertion about the current thread.
| @@ -5325,6 +5326,7 @@ class TerminalController { | |||
| failure = .err(code: "invalid_params", message: "Surface is not a browser", data: ["surface_id": surfaceId.uuidString]) | |||
| return | |||
| } | |||
| automationLease = browserPanel.beginAutomationCommandLease(reason: "browser.socketCommand") | |||
| resolved = V2BrowserPanelContext( | |||
| workspaceId: ws.id, | |||
| surfaceId: surfaceId, | |||
| @@ -5333,6 +5335,11 @@ class TerminalController { | |||
| ) | |||
| } | |||
| guard let resolved else { return failure } | |||
| defer { | |||
| v2MainSync { | |||
| resolved.browserPanel.endAutomationCommandLease(automationLease, reason: "browser.socketCommand") | |||
| } | |||
| } | |||
| return body(resolved) | |||
There was a problem hiding this comment.
v2BrowserWithPanelContext is nonisolated and its body runs on a worker thread. When two socket commands arrive simultaneously for the same hidden panel, both enter v2MainSync and create OffscreenRenderHostLeases sequentially on the main thread — lease A moves the webview to window_A, then lease B moves it to window_B (capturing window_A.contentView as previousSuperview). The two defers then race to post back to the main queue via DispatchQueue.main.sync.
If lease A's endAutomationCommandLease is processed first (FIFO, wrong order): restoreWebView removes the webview from window_B and adds it to originalSuperview, then window_A.contentView = nil is called — making lease B's strong previousSuperview reference point to a detached view. Lease B then removes the webview from originalSuperview and adds it to that detached view. The webview ends up parented to a view that belongs to no window, and scheduleHiddenWebViewDiscardIfNeeded is called with the panel in an inconsistent state.
The fix is to record which lease is outermost and only allow restoration when the counter returns to its pre-lease value, or to adopt a stack/push-pop model where each lease validates that it still owns the current position before restoring.
1 participant
Summary
Testing
xcodebuild test -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-browser-bg-test -only-testing:cmuxTests/BrowserPanelVisualAutomationRestoreHostTests\n-./scripts/lint-pbxproj-test-wiring.sh\n-./scripts/reload-cloud.sh --tag bgbrws\n- Preflighted tagged appbgbrws: created Browser surface, backgrounded it, ranbrowser --surface surface:2 eval 'document.body.dataset.ready'andbrowser --surface surface:2 snapshot --compact, captured/tmp/cmux-bgbrws-browser.png.\nNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes WKWebView superview/window lifecycle and memory-discard gating during automation; mistakes could cause UI glitches or premature discard, but scope is isolated to hidden/background browser paths and reuses existing screenshot hosting.
Overview
Browser socket commands (
eval,snapshot, etc.) now acquire an automation command lease for the whole RPC so hidden or background surfaces stay usable without theWKWebViewbeing discarded mid-command.BrowserPanelgainsbeginAutomationCommandLease/endAutomationCommandLease, which bump the existing visual-automation capture counter, cancel pending hidden-webview discard, restore memory-discarded webviews when needed, and—when the view isn’t visibly on-screen—attach it via a reusableOffscreenRenderHostLease(same offscreen panel path as screenshots). When the lease ends and the surface is still hidden, discard scheduling resumes.Offscreen hosting is refactored into
OffscreenRenderHostLeaseso screenshot capture and panel automation share one implementation;v2BrowserWithPanelContexttakes the lease on the main actor before running the worker-thread body and releases it in adefer.Tests cover portal-hidden hosting, discard blocking during automation, and restore-before-host for discarded webviews.
Reviewed by Cursor Bugbot for commit b960b34. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Background browser surfaces remain automatable by temporarily hosting hidden or discarded WebViews in an offscreen render window during socket commands, then restoring normal lifecycle after the command. This blocks memory discard while automation runs and restores discarded WebViews before capture.
New Features
BrowserPanel, used byTerminalControllerfor all socket JS commands.Refactors
OffscreenRenderHostLeaseto reuse the screenshot offscreen render host for automation and snapshots.BrowserScreenshotWebViewSnapshotterto use the lease, reducing duplicated attach/detach logic.Written for commit b960b34. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Tests