Repository navigation
Keep blocking browser automation off main - #6696
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughBrowser automation commands that wait on WebKit/page callbacks (screenshot, frame select, dialog respond, cookies, storage, console, errors, state save/load, script/style injection) are moved to the socket-worker dispatch lane via ChangesBrowser Automation Off-Main Routing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 1 warning, 1 inconclusive)
✅ 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7ea1eae. Configure here.
Contributor
Greptile SummaryThis PR moves 15+ blocking browser automation commands (screenshot, frame select, dialogs, cookies, storage, console/errors, state save/load, script/style injection) off the main actor and onto the socket-worker lane, matching the pattern already used for
Confidence Score: 5/5All WebKit waits now run off the main actor; WebKit/AppKit access and mutable-state updates stay inside explicit v2MainSync hops. The refactor is consistent across all 15 moved commands and matches the pre-existing browser.eval/navigate pattern exactly. Every moved handler is nonisolated, uses v2BrowserWithPanelContext to resolve panels on main, and wraps WKHTTPCookieStore / screenshot / WKUserContentController calls inside v2MainSync before the blocking v2AwaitCallback wait. Policy tests assert worker routing for all moved commands and correctly verify that browser.get.title and browser.frame.main remain on main. No commands were missed, no WebKit mutations happen off-main without a hop, and the renamed v2BrowserAutomationCommandOnSocketWorker router is exhaustive. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client as Socket Client
participant Worker as Socket Worker Lane
participant Main as Main Actor (v2MainSync)
participant WK as WebKit / WKHTTPCookieStore
Client->>Worker: browser.screenshot / browser.cookies.get / browser.frame.select / etc.
Note over Worker: v2BrowserAutomationCommandOnSocketWorker()
Worker->>Main: v2BrowserWithPanelContext resolve panel + webView
Main-->>Worker: V2BrowserPanelContext (workspaceId, surfaceId, browserPanel, webView)
Worker->>Main: "v2MainSync { store = webView.configuration...httpCookieStore }"
Main-->>Worker: WKHTTPCookieStore reference
Worker->>Main: "v2AwaitCallback { v2MainSync { store.getAllCookies { finish(items) } } }"
Note over Worker: blocking wait off main
Main->>WK: store.getAllCookies(completionHandler:)
WK-->>Main: [HTTPCookie] callback
Main-->>Worker: finish([HTTPCookie])
Worker-->>Client: .ok(v2BrowserPanelFields(ctx, adding: [...]))
Note over Client,WK: browser.get.title / browser.frame.main stay on Main Actor (no blocking wait)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Client as Socket Client
participant Worker as Socket Worker Lane
participant Main as Main Actor (v2MainSync)
participant WK as WebKit / WKHTTPCookieStore
Client->>Worker: browser.screenshot / browser.cookies.get / browser.frame.select / etc.
Note over Worker: v2BrowserAutomationCommandOnSocketWorker()
Worker->>Main: v2BrowserWithPanelContext resolve panel + webView
Main-->>Worker: V2BrowserPanelContext (workspaceId, surfaceId, browserPanel, webView)
Worker->>Main: "v2MainSync { store = webView.configuration...httpCookieStore }"
Main-->>Worker: WKHTTPCookieStore reference
Worker->>Main: "v2AwaitCallback { v2MainSync { store.getAllCookies { finish(items) } } }"
Note over Worker: blocking wait off main
Main->>WK: store.getAllCookies(completionHandler:)
WK-->>Main: [HTTPCookie] callback
Main-->>Worker: finish([HTTPCookie])
Worker-->>Client: .ok(v2BrowserPanelFields(ctx, adding: [...]))
Note over Client,WK: browser.get.title / browser.frame.main stay on Main Actor (no blocking wait)
Reviews (6): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/TerminalController.swift (1)
9468-9494: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not return
loaded: trueafter ignored restore failures.Cookie conversion/set failures and storage JS failures are currently ignored, so callers can get success after a partial state restore. Return an error before the final OK response.
Propagate cookie and storage restore failures
if let cookieRows = raw["cookies"] as? [[String: Any]] { for row in cookieRows { - if let cookie = v2BrowserCookieFromObject(row, fallbackURL: cookieContext.fallbackURL) { - _ = v2BrowserCookieStoreSet(cookieContext.store, cookie: cookie) + guard let cookie = v2BrowserCookieFromObject(row, fallbackURL: cookieContext.fallbackURL) else { + return .err( + code: "invalid_params", + message: "Invalid cookie in state file", + data: ["name": row["name"] as? String ?? ""] + ) + } + guard v2BrowserCookieStoreSet(cookieContext.store, cookie: cookie) else { + return .err(code: "timeout", message: "Timed out setting cookie", data: ["name": cookie.name]) } } } @@ - _ = v2RunBrowserJavaScript(ctx.webView, surfaceId: ctx.surfaceId, script: script, timeout: 10.0) + switch v2RunBrowserJavaScript(ctx.webView, surfaceId: ctx.surfaceId, script: script, timeout: 10.0) { + case .failure(let message): + return .err(code: "js_error", message: message, data: nil) + case .success: + break + } }🤖 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/TerminalController.swift` around lines 9468 - 9494, The code is currently ignoring failures when setting browser cookies and executing the storage JavaScript, which causes the function to return success even after partial state restore failures. Instead of assigning the results of v2BrowserCookieStoreSet and v2RunBrowserJavaScript calls to underscore, capture their return values and check for error conditions. Additionally, track when v2BrowserCookieFromObject returns nil for a cookie row. If any of these operations fail, propagate an appropriate error to the caller rather than allowing the function to return a success response (loaded: true).
🤖 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/TerminalController.swift`:
- Around line 8943-8944: The error response in the guard statement for
v2BrowserCookieFromObject is exposing the raw cookie payload in the data
dictionary, which may contain sensitive session material and credential
information. Remove the raw cookie from the error data field or replace it with
only safe, non-sensitive fields such as cookie name or domain. Do not return the
unredacted cookie payload in the API error response.
- Around line 9396-9400: The code currently treats a timeout from
v2BrowserCookieStoreAll(store) as an empty cookie list by using the
nil-coalescing operator with an empty array, which silently succeeds with no
cookies when a timeout occurs. Instead, modify the cookie snapshot logic to
explicitly check if v2BrowserCookieStoreAll(store) returns nil and propagate
that failure (timeout) rather than defaulting to an empty array, ensuring the
state save operation fails when cookie retrieval times out rather than
succeeding with incomplete data.
- Around line 7378-7386: The PNG encoding operation via v2PNGData(from: image)
is currently executing inside the WebKit callback, which blocks the callback
thread. Move the PNG conversion outside of the v2AwaitCallback block to happen
after it returns. In the .success case inside
captureAutomationVisibleViewportSnapshot callback, pass the image directly to
finish() instead of converting it to PNG data. After v2AwaitCallback completes
and returns the image, perform the PNG encoding on the socket worker thread
where the result is processed.
---
Outside diff comments:
In `@Sources/TerminalController.swift`:
- Around line 9468-9494: The code is currently ignoring failures when setting
browser cookies and executing the storage JavaScript, which causes the function
to return success even after partial state restore failures. Instead of
assigning the results of v2BrowserCookieStoreSet and v2RunBrowserJavaScript
calls to underscore, capture their return values and check for error conditions.
Additionally, track when v2BrowserCookieFromObject returns nil for a cookie row.
If any of these operations fail, propagate an appropriate error to the caller
rather than allowing the function to return a success response (loaded: true).
🪄 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: 925df2ba-07a6-4a7d-b74a-96358830ed05
📒 Files selected for processing (9)
.coderabbit.yaml.github/review-bot-rules/README.md.github/review-bot-rules/browser-automation-webkit-waits-off-main.md.greptile/config.json.greptile/files.json.greptile/rules.mdPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Wire/ControlCommandExecutionPolicy.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandExecutionPolicyTests.swiftSources/TerminalController.swift
Comment on lines
7378
to
+7386
| let snapshotResult: Data?? = v2AwaitCallback(timeout: 15.0) { finish in | ||
| browserPanel.captureAutomationVisibleViewportSnapshot { result in | ||
| switch result { | ||
| case .success(let image): | ||
| finish(self.v2PNGData(from: image)) | ||
| case .failure: | ||
| finish(nil) | ||
| v2MainSync { | ||
| browserPanel.captureAutomationVisibleViewportSnapshot { result in | ||
| switch result { | ||
| case .success(let image): | ||
| finish(self.v2PNGData(from: image)) | ||
| case .failure: | ||
| finish(nil) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Keep PNG encoding off the WebKit capture callback.
Line 7383 converts the snapshot to TIFF/PNG before signaling the worker wait. For large screenshots, that still keeps CPU-heavy encoding on the WebKit/AppKit callback path. Return the captured image first, then encode after v2AwaitCallback resumes on the socket worker.
Move PNG conversion after the main/WebKit callback returns
- let snapshotResult: Data?? = v2AwaitCallback(timeout: 15.0) { finish in
+ let snapshotResult: NSImage?? = v2AwaitCallback(timeout: 15.0) { finish in
v2MainSync {
browserPanel.captureAutomationVisibleViewportSnapshot { result in
switch result {
case .success(let image):
- finish(self.v2PNGData(from: image))
+ finish(image)
case .failure:
finish(nil)
}
}
}
@@
- guard let imageData = snapshotResult else {
+ guard let image = snapshotResult else {
return .err(code: "internal_error", message: "Failed to capture snapshot", data: nil)
}
+ guard let imageData = v2PNGData(from: image) else {
+ return .err(code: "internal_error", message: "Failed to encode snapshot", data: nil)
+ }As per path instructions, WebKit-waiting browser automation should do the minimum UI hop and keep waits/work on the socket-worker lane.
🤖 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/TerminalController.swift` around lines 7378 - 7386, The PNG encoding
operation via v2PNGData(from: image) is currently executing inside the WebKit
callback, which blocks the callback thread. Move the PNG conversion outside of
the v2AwaitCallback block to happen after it returns. In the .success case
inside captureAutomationVisibleViewportSnapshot callback, pass the image
directly to finish() instead of converting it to PNG data. After v2AwaitCallback
completes and returns the image, perform the PNG encoding on the socket worker
thread where the result is processed.
Source: Path instructions
Comment on lines
+9396
to
+9400
| let store = v2MainSync { | ||
| ctx.webView.configuration.websiteDataStore.httpCookieStore | ||
| } | ||
| let cookies = (v2BrowserCookieStoreAll(store) ?? []).map(v2BrowserCookieDict) | ||
| let stateSnapshot = v2MainSync { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail state save when cookie snapshot times out.
Line 9399 treats a cookie-store timeout as an empty cookie list, then writes a successful state file that silently drops cookies. Match browser.cookies.get and return a timeout instead.
Preserve cookie snapshot failures
let store = v2MainSync {
ctx.webView.configuration.websiteDataStore.httpCookieStore
}
- let cookies = (v2BrowserCookieStoreAll(store) ?? []).map(v2BrowserCookieDict)
+ guard let cookieRows = v2BrowserCookieStoreAll(store) else {
+ return .err(code: "timeout", message: "Timed out reading cookies", data: nil)
+ }
+ let cookies = cookieRows.map(v2BrowserCookieDict)
let stateSnapshot = v2MainSync {🤖 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/TerminalController.swift` around lines 9396 - 9400, The code
currently treats a timeout from v2BrowserCookieStoreAll(store) as an empty
cookie list by using the nil-coalescing operator with an empty array, which
silently succeeds with no cookies when a timeout occurs. Instead, modify the
cookie snapshot logic to explicitly check if v2BrowserCookieStoreAll(store)
returns nil and propagate that failure (timeout) rather than defaulting to an
empty array, ensuring the state save operation fails when cookie retrieval times
out rather than succeeding with incomplete data.
5 tasks done
This branch was successfully deployed
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.
Summary
Verification
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches many browser control-socket code paths and main-thread synchronization; incorrect routing could hang the UI or regress automation, though policy tests and the established worker-lane pattern reduce that risk.
Overview
Moves blocking browser socket automation off the main actor so WebKit/JavaScript waits no longer freeze SwiftUI.
Previously, commands such as screenshot, frame select, dialogs, cookies, storage, console/errors, state save/load, and script/style injection ran on the main
processV2Commandpath. They are now listed inControlCommandExecutionPolicy.socketWorkerMethods, dispatched from the socket worker viav2BrowserAutomationCommandOnSocketWorker(renamed from the JS-only router), and removed from the main-actor browser switch.Handlers are refactored to
nonisolatedwithv2BrowserWithPanelContext: blocking work stays on the worker thread whilev2MainSynccovers WebKit/AppKit access, cookie-store callbacks, screenshot capture, and browser state dictionary updates. Response assembly is centralized withv2BrowserPanelFields.Adds the
browser-automation-webkit-waits-off-mainreview rule and a CodeRabbit/Greptile pre-merge check, plusControlCommandExecutionPolicyTestscoverage so the moved commands stay worker-routed (lightweight commands likebrowser.get.titleandbrowser.frame.mainremain on main).Reviewed by Cursor Bugbot for commit a4209bc. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Moves blocking browser automation off the main actor so the UI doesn’t hang. Routes all page JS, WebKit cookie, hook, and screenshot waits through the socket worker, and trims response payload boilerplate.
Refactors
v2BrowserAutomationCommandOnSocketWorker.v2MainSync; runWKHTTPCookieStoreget/set/delete, screenshot capture callbacks, and telemetry/dialog hook source fetches on main. Mark blocking helpersnonisolatedand usev2BrowserWithPanelContextto resolve panel/webView safely.v2BrowserPanelFieldsto unify workspace/surface fields.browser-automation-webkit-waits-off-mainreview rule and enforce the “cmux browser automation off-main” check in CodeRabbit/Greptile.Tests
ControlCommandExecutionPolicyTeststo assert worker routing for the moved commands; keepbrowser.get.titleandbrowser.frame.mainon main.Written for commit a4209bc. Summary will update on new commits.
Summary by CodeRabbit
Improvements
Chores