Add configurable browser engine selection - #8163
Closed
austinywang wants to merge 42 commits into
Closed
austinywang wants to merge 42 commits into
austinywang wants to merge 42 commits into
Conversation
|
To use Codex here, create a Codex account and connect to github. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds selectable WebKit and Chromium browser engines, shared browser-session contracts, Chromium CDP and viewport integration, engine-aware panels and automation, persisted engine selection, settings UI, tests, localization, and configuration schema updates. ChangesBrowser engine contracts and selection
Chromium transport and integration
Persistence and automation
Settings and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 errors, 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Greptile SummaryThis PR adds a configurable browser engine setting (Auto/WebKit/Chromium) for new browser panes, backed by a full CDP-over-WebSocket Chromium integration. It introduces an engine-neutral
Confidence Score: 4/5Safe to merge with one open item: CDPConnection introduces an inline ContinuousClock().sleep timeout that the repository policy requires to be a shared, testable abstraction. The core engine-neutral abstraction, CDP integration, session persistence, and localization are all well-executed. CDPConnection.swift introduces the same inline Task.sleep timeout pattern that ChromiumProcessController already has open — the cmux policy asks for a shared, test-covered timeout utility. That is the only structural concern remaining. CDPConnection.swift (timing timeout pattern) and TerminalController.swift (dead helper methods). Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant TC as TerminalController (socket worker)
participant BP as BrowserPanel (MainActor)
participant WK as WebKitBrowserEngineSession
participant CH as ChromiumBrowserEngineSession
participant CDP as CDPConnection (actor)
participant CR as Chromium process
TC->>BP: v2RunEngineJavaScript(browserPanel, script, world)
BP->>BP: "Task @MainActor evaluateJavaScript"
alt WebKit engine
BP->>WK: evaluateJavaScript
WK->>WK: callAsyncJavaScript awaits Promise
WK-->>BP: BrowserJavaScriptValue
else Chromium engine
BP->>CH: evaluateJavaScript
CH->>CDP: send Runtime.evaluate awaitPromise true
CDP->>CR: WebSocket CDP message
CR-->>CDP: CDP response
CDP-->>CH: CDPJSONValue result
CH-->>BP: BrowserJavaScriptValue
end
BP-->>TC: finish callback
TC-->>TC: v2AwaitCallback resolves
%%{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 TC as TerminalController (socket worker)
participant BP as BrowserPanel (MainActor)
participant WK as WebKitBrowserEngineSession
participant CH as ChromiumBrowserEngineSession
participant CDP as CDPConnection (actor)
participant CR as Chromium process
TC->>BP: v2RunEngineJavaScript(browserPanel, script, world)
BP->>BP: "Task @MainActor evaluateJavaScript"
alt WebKit engine
BP->>WK: evaluateJavaScript
WK->>WK: callAsyncJavaScript awaits Promise
WK-->>BP: BrowserJavaScriptValue
else Chromium engine
BP->>CH: evaluateJavaScript
CH->>CDP: send Runtime.evaluate awaitPromise true
CDP->>CR: WebSocket CDP message
CR-->>CDP: CDP response
CDP-->>CH: CDPJSONValue result
CH-->>BP: BrowserJavaScriptValue
end
BP-->>TC: finish callback
TC-->>TC: v2AwaitCallback resolves
Reviews (11): Last reviewed commit: "Preserve Chromium JavaScript exception t..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
6195
to
+6201
| surfaceId: UUID, | ||
| timeout: TimeInterval = 3.0 | ||
| ) -> Bool { | ||
| let engineKind = v2MainSync { browserPanel.engineKind } | ||
| if engineKind == .chromium { | ||
| return v2MainSync { ObjectIdentifier(browserPanel.webView) == ObjectIdentifier(webView) } | ||
| } |
Contributor
There was a problem hiding this comment.
For Chromium panes v2EnsureBrowserDocumentLoaded returns as soon as the viewport WKWebView object identity matches — it never probes the actual Chromium page's document.readyState. The WebKit path uses this gate to prevent script execution against an uninteractive page (blank/provisional navigation), but Chromium gets no equivalent guard. Automation commands like browser.eval or browser.get.text that arrive immediately after a browser.navigate will succeed (because CDP Runtime.evaluate works in any load state) but may observe partially-constructed DOM, missing scripts, or stale content — a correctness difference a user cannot distinguish from a timing error. Consider awaiting Page.loadEventFired or Page.frameStoppedLoading in the Chromium path, or at least checking document.readyState via a CDP Runtime.evaluate before returning true.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Sources/TerminalController.swift (1)
3129-3149: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDo not report the viewport transport PID as Chromium’s content PID.
For Chromium,
browserPanel.webViewis only the canvas transport, sobrowser_web_content_pididentifies the wrong process. Return the actual Chromium content PID, ornulluntil one is available.🤖 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 3129 - 3149, Update the browser metadata construction around webContentPID and browser_web_content_pid so Chromium does not use CmuxWebContentProcessIdentifier.pid(for: browserPanel.webView), which is only the viewport transport process. Resolve and report the actual Chromium content PID, returning null when unavailable, while preserving the existing PID behavior for non-Chromium engines and keeping the nested webviews metadata consistent.Sources/Workspace.swift (1)
7889-7938: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
browser.enginewhen no engine was explicitly requested.Falling back to the selected sibling’s engine means changing the setting has no effect when creating a new tab or split beside an existing browser. Only restoration, duplication, or link-opening callers should pass an explicit engine.
Proposed fix
- let inheritedEngineKind = browserEngineKind ?? (panels[panelId] as? BrowserPanel)?.engineKind + let inheritedEngineKind = browserEngineKind- let inheritedEngineKind = browserEngineKind ?? sourcePanelId.flatMap { - (panels[$0] as? BrowserPanel)?.engineKind - } + let inheritedEngineKind = browserEngineKindAs per path instructions, engine choice and routing must come from a single authoritative source.
Also applies to: 8007-8052
🤖 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/Workspace.swift` around lines 7889 - 7938, Update the browser panel creation flow around inheritedEngineKind and the BrowserPanel initializer to use the current browser.engine setting when no browserEngineKind is explicitly supplied, rather than deriving the engine from the source panel. Preserve explicit engine values for restoration, duplication, and link-opening callers, and apply the same change to the corresponding creation path near the other referenced call site.Source: Path instructions
Sources/Panels/BrowserPanel+AutomationRecovery.swift (1)
42-100: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftGive these callback bridges a cancellation handle.
Task {@mainactor... }is still fire-and-forget here, so a timeout or teardown can leave JS, screenshot, or init-script work running after the socket reply has already failed. Store the task and cancel it whensocketAwaitCallbackreturnsnil, or make the async operation itself own the lifecycle:
Sources/Panels/BrowserPanel+AutomationRecovery.swift#L42-L100Sources/TerminalController.swift#L5957-L5977Sources/TerminalController.swift#L6013-L6034Sources/Panels/BrowserPanel.swift#L7252-L7265Sources/Panels/BrowserPanel.swift#L11590-L11594🤖 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/BrowserPanel`+AutomationRecovery.swift around lines 42 - 100, Give every fire-and-forget callback bridge a cancellation handle so timed-out or failed socket replies stop in-flight async work. Update the probe Tasks in Sources/Panels/BrowserPanel+AutomationRecovery.swift:42-100 and the corresponding callback bridges in Sources/TerminalController.swift:5952-5976, Sources/TerminalController.swift:6012-6034, Sources/Panels/BrowserPanel.swift:7252-7265, and Sources/Panels/BrowserPanel.swift:11590-11594 to retain and cancel their Task when socketAwaitCallback returns nil, or move lifecycle ownership into the async operation; preserve the existing JavaScript, screenshot, and init-script behavior otherwise.Source: Coding guidelines
🤖 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
`@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserApplicationProviding.swift`:
- Around line 2-9: Make the LaunchServices methods defaultBrowserApplications()
and installedChromiumApplications() asynchronous, and update
BrowserApplicationProviding isolation/conformance as needed so implementations
can execute queries off the main actor. Propagate async changes through all
conforming implementations and callers, including browser-pane creation, while
preserving the existing returned application results.
In
`@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserJavaScriptValue.swift`:
- Around line 53-54: Remove the unnecessary `@MainActor` isolation from the
foundationValue computed property in BrowserJavaScriptValue, leaving it
nonisolated because it only performs value conversion and does not access
main-actor state.
In
`@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/CDPConnection.swift`:
- Around line 78-121: Update receiveMessages and handleMessage so malformed
message decoding is caught and logged per message, then skipped while the
receive loop continues; reserve the existing connection teardown path for
webSocketTask.receive failures. Also reuse shared JSONDecoder and JSONEncoder
instances across send and handleMessage instead of allocating them for each
message.
- Around line 34-59: Add a bounded timeout to the CDPConnection.send method so
every request fails when Chromium does not respond, rather than leaving its
continuation and pendingRequests entry indefinitely. Race the existing
webSocketTask.send/request continuation against a timeout, remove the request
from pendingRequests, and resume it with an appropriate Chromium protocol error;
preserve normal replies and send failures.
In
`@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumBrowserEngineSession.swift`:
- Around line 336-337: Update the frame delivery code around
ChromiumViewportDocumentJSONLiteral().encode and window.cmuxChromiumFrame so it
passes the base64 data as a bridged JavaScript argument instead of interpolating
it into a newly built script string. Keep the viewport script fixed and prepend
the data-URI prefix inside that script, avoiding full-frame escaping and
intermediate string construction on the MainActor.
- Around line 257-269: Update the startupTask catch path around connect(to:) to
invoke the existing teardown flow before presentLaunchFailure(). Ensure the
teardown cancels eventTask, clears connection and cdpSessionID, closes the CDP
connection, and stops processController, covering failures after Chromium
launches or partial session state is assigned.
- Line 194: Remove the "grantUniveralAccess": .bool(true) entry from the
parameters used by Page.createIsolatedWorld in ChromiumBrowserEngineSession,
leaving the option unset so the API default of false applies.
In
`@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumProcessController.swift`:
- Around line 45-58: Update the continuation-based launch flow around endpoint
resolution to use withTaskCancellationHandler, ensuring cancellation immediately
terminates the Chromium process and resumes or fails the pending continuation.
Coordinate this with endpointContinuation, timeoutTask, and stderrTask so
cancellation cannot leave the caller waiting or background tasks/processes
running.
In
`@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumViewportDocument.swift`:
- Around line 80-92: Update the viewport event handlers in
ChromiumViewportDocument so mousemove derives the reported button from the
event.buttons bitmask, keydown treats a single Unicode grapheme such as an emoji
as text while excluding named keys, and the canvas suppresses the native
contextmenu event while preserving proxying to Chromium.
In
`@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/WebKitBrowserEngineSession.swift`:
- Around line 106-112: Update the screenshot method in
WebKitBrowserEngineSession to extract the underlying CGImage, then perform
NSBitmapImageRep PNG encoding inside a Task.detached block so the `@MainActor`
remains responsive. Preserve the existing emptyScreenshot error behavior when
image extraction or encoding fails, and return the detached task’s encoded PNG
result.
In `@Resources/Localizable.xcstrings`:
- Around line 238050-238052: Update the affected localized string values in
Resources/Localizable.xcstrings for the es, fr, pl, th, and tr locales to
preserve “Chromium” as the product name, replacing translations that render it
as “Chrome” or the chemical element while leaving unrelated localization text
unchanged.
In `@Sources/KeyboardShortcutSettingsFileStore.swift`:
- Around line 903-909: Update the browser-section parsing flow around
BrowserEnginePreference and logInvalid so an invalid browser.engine value is
logged without returning from the enclosing parser. Skip only the invalid engine
assignment, then continue processing subsequent browser settings such as search
engine and theme.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 4705-4718: Update the Chromium replacement flows around
replaceWebViewPreservingState, including the paths at the referenced
workspace/profile transitions, to capture engineSession.state.url ?? currentURL
before closing the old session and pass that URL explicitly into restoration.
Ensure restoreURL no longer relies on oldWebView.url for Chromium transport
views, while preserving existing restoration behavior for other engines.
In `@Sources/TerminalController.swift`:
- Around line 5979-5999: Remove the Chromium rejection paths in the
browser.cookies.* and browser.state.* handlers that rely on v2WebKitOnlyError.
Route Chromium operations through the existing CDP cookie and storage APIs,
while retaining WebKit implementations, so both engines expose equivalent
cookie/state automation behavior.
---
Outside diff comments:
In `@Sources/Panels/BrowserPanel`+AutomationRecovery.swift:
- Around line 42-100: Give every fire-and-forget callback bridge a cancellation
handle so timed-out or failed socket replies stop in-flight async work. Update
the probe Tasks in Sources/Panels/BrowserPanel+AutomationRecovery.swift:42-100
and the corresponding callback bridges in
Sources/TerminalController.swift:5952-5976,
Sources/TerminalController.swift:6012-6034,
Sources/Panels/BrowserPanel.swift:7252-7265, and
Sources/Panels/BrowserPanel.swift:11590-11594 to retain and cancel their Task
when socketAwaitCallback returns nil, or move lifecycle ownership into the async
operation; preserve the existing JavaScript, screenshot, and init-script
behavior otherwise.
In `@Sources/TerminalController.swift`:
- Around line 3129-3149: Update the browser metadata construction around
webContentPID and browser_web_content_pid so Chromium does not use
CmuxWebContentProcessIdentifier.pid(for: browserPanel.webView), which is only
the viewport transport process. Resolve and report the actual Chromium content
PID, returning null when unavailable, while preserving the existing PID behavior
for non-Chromium engines and keeping the nested webviews metadata consistent.
In `@Sources/Workspace.swift`:
- Around line 7889-7938: Update the browser panel creation flow around
inheritedEngineKind and the BrowserPanel initializer to use the current
browser.engine setting when no browserEngineKind is explicitly supplied, rather
than deriving the engine from the source panel. Preserve explicit engine values
for restoration, duplication, and link-opening callers, and apply the same
change to the corresponding creation path near the other referenced call site.
🪄 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: d9d64e56-9f48-4cd9-8d7d-012a21b2eb1a
📒 Files selected for processing (76)
Packages/macOS/CmuxBrowser/Package.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserApplication.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserApplicationProviding.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserEngineResolver.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserEngineSelection.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserEngineSelectionService.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserEngineSession.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserEngineSessionError.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserEngineState.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserJavaScriptValue.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserJavaScriptWorld.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/BrowserChromiumProfileDirectory.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/CDPConnection.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/CDPEvent.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/CDPJSONValue.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumBrowserEngineSession+Viewport.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumBrowserEngineSession.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumProcessController.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumViewportDocument.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumViewportDocumentJSONLiteral.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumViewportMessageHandler.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/LaunchServicesBrowserApplicationProvider.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/WebKitBrowserEngineSession.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/RecentlyClosed/ClosedBrowserPanelRestoreSnapshot.swiftPackages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/BrowserApplicationProviderFake.swiftPackages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/BrowserEngineResolverTests.swiftPackages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/BrowserEngineSelectionServiceTests.swiftPackages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/BrowserJavaScriptValueTests.swiftPackages/macOS/CmuxCore/Sources/CmuxCore/Browser/BrowserEngineKind.swiftPackages/macOS/CmuxCore/Sources/CmuxCore/Browser/BrowserEnginePreference.swiftPackages/macOS/CmuxSettings/Package.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Codable/BrowserEnginePreference+SettingCodable.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Keys/BrowserCatalogSection.swiftPackages/macOS/CmuxSettingsUI/Package.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/BrowserEngineSettingRow.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/BrowserSection.swiftResources/Localizable.xcstringsSources/CmuxSettingsJSONPathSupport.swiftSources/DockSplitStore.swiftSources/Find/BrowserFindWebViewEvaluator.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/Panels/BrowserEngineSelection+Current.swiftSources/Panels/BrowserPanel+AutomationRecovery.swiftSources/Panels/BrowserPanel.swiftSources/SessionPersistence.swiftSources/TabManager.swiftSources/TerminalController+BrowserAutomationRecovery.swiftSources/TerminalController+WindowDockBrowserRouting.swiftSources/TerminalController.swiftSources/Workspace+DockBrowserLookup.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxprojskills/cmux-settings/SKILL.mdskills/cmux-settings/references/all-keys.mdweb/data/cmux.schema.jsonweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/en.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/ja.jsonweb/messages/km.jsonweb/messages/ko.jsonweb/messages/no.jsonweb/messages/pl.jsonweb/messages/pt-BR.jsonweb/messages/ru.jsonweb/messages/th.jsonweb/messages/tr.jsonweb/messages/uk.jsonweb/messages/zh-CN.jsonweb/messages/zh-TW.json
Comment on lines
+2
to
+9
| @MainActor | ||
| public protocol BrowserApplicationProviding: AnyObject { | ||
| /// Returns the applications LaunchServices selects for representative HTTPS and HTTP URLs. | ||
| func defaultBrowserApplications() -> [BrowserApplication] | ||
|
|
||
| /// Returns installed Chromium-family applications that cmux knows how to launch. | ||
| func installedChromiumApplications() -> [BrowserApplication] | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Make LaunchServices queries asynchronous to avoid blocking the main thread.
LaunchServices queries (such as resolving default browsers or installed applications) involve IPC and can occasionally hang or block. As per coding guidelines, do not add expensive synchronous syscalls to the main actor or latency-sensitive interactive paths.
Consider making these requirements async (and the protocol Sendable/nonisolated) so that implementations can perform the queries on a background actor, preventing potential UI freezes when creating a new browser pane.
♻️ Proposed refactor
-@MainActor
-public protocol BrowserApplicationProviding: AnyObject {
+public protocol BrowserApplicationProviding: AnyObject, Sendable {
/// Returns the applications LaunchServices selects for representative HTTPS and HTTP URLs.
- func defaultBrowserApplications() -> [BrowserApplication]
+ func defaultBrowserApplications() async -> [BrowserApplication]
/// Returns installed Chromium-family applications that cmux knows how to launch.
- func installedChromiumApplications() -> [BrowserApplication]
+ func installedChromiumApplications() async -> [BrowserApplication]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @MainActor | |
| public protocol BrowserApplicationProviding: AnyObject { | |
| /// Returns the applications LaunchServices selects for representative HTTPS and HTTP URLs. | |
| func defaultBrowserApplications() -> [BrowserApplication] | |
| /// Returns installed Chromium-family applications that cmux knows how to launch. | |
| func installedChromiumApplications() -> [BrowserApplication] | |
| } | |
| public protocol BrowserApplicationProviding: AnyObject, Sendable { | |
| /// Returns the applications LaunchServices selects for representative HTTPS and HTTP URLs. | |
| func defaultBrowserApplications() async -> [BrowserApplication] | |
| /// Returns installed Chromium-family applications that cmux knows how to launch. | |
| func installedChromiumApplications() async -> [BrowserApplication] | |
| } |
🤖 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
`@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserApplicationProviding.swift`
around lines 2 - 9, Make the LaunchServices methods defaultBrowserApplications()
and installedChromiumApplications() asynchronous, and update
BrowserApplicationProviding isolation/conformance as needed so implementations
can execute queries off the main actor. Propagate async changes through all
conforming implementations and callers, including browser-pane creation, while
preserving the existing returned application results.
Source: Coding guidelines
Comment on lines
+106
to
+112
| guard let tiff = image.tiffRepresentation, | ||
| let bitmap = NSBitmapImageRep(data: tiff), | ||
| let png = bitmap.representation(using: NSBitmapImageRep.FileType.png, properties: [:]) else { | ||
| throw BrowserEngineSessionError.emptyScreenshot | ||
| } | ||
| return png | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Hop off the main actor for CPU-heavy image encoding.
Generating the PNG representation via NSBitmapImageRep is a CPU-heavy operation. Because WebKitBrowserEngineSession is @MainActor-isolated, performing this synchronously will block the main thread and cause UI stuttering, especially for large viewports.
As per coding guidelines, expensive parsing and CPU-heavy loads should be explicitly hopped off UI isolation. Consider moving the encoding work to a Task.detached block by extracting the underlying CGImage.
🛠 Proposed fix
- guard let tiff = image.tiffRepresentation,
- let bitmap = NSBitmapImageRep(data: tiff),
- let png = bitmap.representation(using: NSBitmapImageRep.FileType.png, properties: [:]) else {
- throw BrowserEngineSessionError.emptyScreenshot
- }
- return png
+ guard let cgImage = image.cgImage(forProposedRect: nil, context: nil, hints: nil) else {
+ throw BrowserEngineSessionError.emptyScreenshot
+ }
+
+ return try await Task.detached {
+ let bitmap = NSBitmapImageRep(cgImage: cgImage)
+ guard let png = bitmap.representation(using: .png, properties: [:]) else {
+ throw BrowserEngineSessionError.emptyScreenshot
+ }
+ return png
+ }.value📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| guard let tiff = image.tiffRepresentation, | |
| let bitmap = NSBitmapImageRep(data: tiff), | |
| let png = bitmap.representation(using: NSBitmapImageRep.FileType.png, properties: [:]) else { | |
| throw BrowserEngineSessionError.emptyScreenshot | |
| } | |
| return png | |
| } | |
| guard let cgImage = image.cgImage(forProposedRect: nil, context: nil, hints: nil) else { | |
| throw BrowserEngineSessionError.emptyScreenshot | |
| } | |
| return try await Task.detached { | |
| let bitmap = NSBitmapImageRep(cgImage: cgImage) | |
| guard let png = bitmap.representation(using: .png, properties: [:]) else { | |
| throw BrowserEngineSessionError.emptyScreenshot | |
| } | |
| return png | |
| }.value |
🤖 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
`@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/WebKitBrowserEngineSession.swift`
around lines 106 - 112, Update the screenshot method in
WebKitBrowserEngineSession to extract the underlying CGImage, then perform
NSBitmapImageRep PNG encoding inside a Task.detached block so the `@MainActor`
remains responsive. Preserve the existing emptyScreenshot error behavior when
image extraction or encoding fails, and return the detached task’s encoded PNG
result.
Source: Coding guidelines
Comment on lines
+903
to
+909
| if let raw = jsonString(section["engine"]) { | ||
| guard let preference = BrowserEnginePreference(rawValue: raw) else { | ||
| logInvalid("browser.engine", sourcePath: sourcePath) | ||
| return | ||
| } | ||
| snapshot.managedUserDefaults[SettingCatalog().browser.engine.userDefaultsKey] = .string(preference.rawValue) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not abort parsing the entire browser section for an invalid engine.
The return skips valid settings that follow browser.engine—including search engine, theme, and other browser options. Log the invalid engine and continue parsing the remaining fields.
Proposed fix
if let raw = jsonString(section["engine"]) {
- guard let preference = BrowserEnginePreference(rawValue: raw) else {
+ if let preference = BrowserEnginePreference(rawValue: raw) {
+ snapshot.managedUserDefaults[SettingCatalog().browser.engine.userDefaultsKey] = .string(preference.rawValue)
+ } else {
logInvalid("browser.engine", sourcePath: sourcePath)
- return
}
- snapshot.managedUserDefaults[SettingCatalog().browser.engine.userDefaultsKey] = .string(preference.rawValue)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let raw = jsonString(section["engine"]) { | |
| guard let preference = BrowserEnginePreference(rawValue: raw) else { | |
| logInvalid("browser.engine", sourcePath: sourcePath) | |
| return | |
| } | |
| snapshot.managedUserDefaults[SettingCatalog().browser.engine.userDefaultsKey] = .string(preference.rawValue) | |
| } | |
| if let raw = jsonString(section["engine"]) { | |
| if let preference = BrowserEnginePreference(rawValue: raw) { | |
| snapshot.managedUserDefaults[SettingCatalog().browser.engine.userDefaultsKey] = .string(preference.rawValue) | |
| } else { | |
| logInvalid("browser.engine", sourcePath: sourcePath) | |
| } | |
| } |
🤖 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/KeyboardShortcutSettingsFileStore.swift` around lines 903 - 909,
Update the browser-section parsing flow around BrowserEnginePreference and
logInvalid so an invalid browser.engine value is logged without returning from
the enclosing parser. Skip only the invalid engine assignment, then continue
processing subsequent browser settings such as search engine and theme.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
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
`@Packages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/ChromiumViewportInputQueueTests.swift`:
- Around line 44-67: Update ChromiumViewportInputQueue.enqueue(_:) or the
saturatedOrderedInputKeepsTheNewestCompleteKeyGesture test so they share the
same contract: a full queue containing only non-coalescible key commands must
either evict the oldest ordered command and retain the complete NewestKey
gesture, or the test must assert that the new gesture is dropped. Preserve the
existing maximum queue count and complete key-pair expectations.
🪄 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: 373cb3b7-41f0-46cd-a3ad-8ce5952addcd
📒 Files selected for processing (1)
Packages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/ChromiumViewportInputQueueTests.swift
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumBrowserEngineSession.swift`:
- Around line 532-546: Update failViewportInput() and its callers in the
viewport input flow so it is used only for genuine I/O failures, not transient
local backpressure. Preserve the session connection and recovery path for local
queue saturation, while retaining the existing shutdown behavior for actual
connection failures. Ensure viewportInputFailed is not permanently set for
recoverable conditions.
In
`@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumBrowserEngineSession`+Viewport.swift:
- Around line 236-242: Update enqueueViewportInput and the related
viewport-input draining call site so queue backpressure is handled without
invoking the session-ending failViewportInput(). Reserve failViewportInput() for
actual I/O failures, while preserving the existing enqueue and draining behavior
for successfully accepted commands.
🪄 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: de75b4b9-096a-4657-8349-302240ba0c30
📒 Files selected for processing (13)
Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserEngineCookie.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserEngineSession.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/BrowserChromiumProfileDirectory.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/CDPConnection.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumBrowserCookieCodec.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumBrowserEngineSession+Viewport.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumBrowserEngineSession.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/WebKitBrowserEngineSession.swiftPackages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/ChromiumBrowserCookieCodecTests.swiftPackages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/ChromiumViewportInputQueueTests.swiftResources/Localizable.xcstringsSources/Panels/BrowserPanel.swiftSources/TerminalController.swift
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/Panels/BrowserPanel.swift (1)
428-439: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winStale Chromium runtime stays cached after clearing profile data.
clearProfileDatareadschromiumRuntimes[id]but never removes it from the cache before closing the runtime and deleting its directory. Every laterchromiumRuntime(for: id)lookup (new panel init,switchToProfile,reloadChromium) returns the same now-closed instance instead of a fresh one, breaking Chromium browsing for that profile until app restart.🐛 Proposed fix
func clearProfileData(id: UUID) async -> BrowserProfileClearOutcome? { guard repository.profileDefinition(id: id) != nil else { return nil } - let runtime = chromiumRuntimes[id] + let runtime = chromiumRuntimes.removeValue(forKey: id) if let runtime { await runtime.close() } let directory = chromiumProfileDirectory.url(profileID: id) await fileRemover.removeItemIfExists(at: directory) let result = await repository.clearProfileData(id: id) mirrorPublishedState() return result }🤖 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/BrowserPanel.swift` around lines 428 - 439, Update clearProfileData(id:) to remove the profile’s entry from chromiumRuntimes before closing the runtime and deleting its directory. Ensure subsequent chromiumRuntime(for:), switchToProfile, and reloadChromium lookups create a fresh runtime while preserving the existing cleanup and repository-clearing flow.Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumBrowserEngineSession.swift (1)
149-161: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftOwn and serialize browser operations instead of spawning fire-and-forget tasks.
These tasks survive
close()and can execute after newer actions. For example, a delayedPage.stopLoadingcan stop a subsequent navigation, while an older failure task can overwrite newer viewport state.Route navigation/control operations through stored, cancellable tasks or one serialized action owner, and cancel them during teardown or replacement.
As per coding guidelines, “Do not create fire-and-forget
Task { ... }work with meaningful lifecycle unless it is stored, cancellable, or tied to a caller-owned operation.”Also applies to: 177-180, 685-719, 807-815
🤖 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 `@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumBrowserEngineSession.swift` around lines 149 - 161, Replace the unowned fire-and-forget Task operations in the navigation and related control paths, including Page.navigate, the operations around lines 177-180, and the flows in the viewport/state methods around 685-719 and 807-815, with stored cancellable tasks or a single serialized action owner. Ensure each task is cancelled when replaced and during session teardown in close(), preventing stale operations and failures from affecting newer browser actions.Source: Coding guidelines
♻️ Duplicate comments (3)
Sources/KeyboardShortcutSettingsFileStore.swift (1)
909-915: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not abort parsing the entire browser section for an invalid engine.
The
returnstatement skips valid settings that followbrowser.engine—including search engine, theme, and other browser options. Log the invalid engine and continue parsing the remaining fields.🐛 Proposed fix
if let raw = jsonString(section["engine"]) { - guard let preference = BrowserEnginePreference(rawValue: raw) else { + if let preference = BrowserEnginePreference(rawValue: raw) { + snapshot.managedUserDefaults[SettingCatalog().browser.engine.userDefaultsKey] = .string(preference.rawValue) + } else { logInvalid("browser.engine", sourcePath: sourcePath) - return } - snapshot.managedUserDefaults[SettingCatalog().browser.engine.userDefaultsKey] = .string(preference.rawValue) }🤖 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/KeyboardShortcutSettingsFileStore.swift` around lines 909 - 915, In the browser engine parsing guard block, remove the return statement that follows the logInvalid call for an invalid BrowserEnginePreference. Keep the logInvalid logging in place to record the invalid engine value, but allow execution to continue past this block so that subsequent browser settings (search engine, theme, and other options) are still parsed from the section.Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/WebKitBrowserEngineSession.swift (1)
171-177:⚠️ Potential issue | 🟠 MajorHop off the main actor for CPU-heavy image encoding.
Generating the PNG representation via
NSBitmapImageRepis a CPU-heavy operation. BecauseWebKitBrowserEngineSessionis@MainActor-isolated, performing this synchronously will block the main thread and cause UI stuttering, especially for large viewports.As per coding guidelines, explicitly hop CPU-heavy, file-I/O-heavy, parsing-heavy, or network-heavy async helpers called from UI isolation away from the main actor. Consider moving the encoding work to a
Task.detachedblock by extracting the underlyingCGImage.🛠 Proposed fix
- guard let tiff = image.tiffRepresentation, - let bitmap = NSBitmapImageRep(data: tiff), - let png = bitmap.representation(using: NSBitmapImageRep.FileType.png, properties: [:]) else { - throw BrowserEngineSessionError.emptyScreenshot - } - return png + guard let cgImage = image.cgImage(forProposedRect: nil, context: nil, hints: nil) else { + throw BrowserEngineSessionError.emptyScreenshot + } + + return try await Task.detached { + let bitmap = NSBitmapImageRep(cgImage: cgImage) + guard let png = bitmap.representation(using: .png, properties: [:]) else { + throw BrowserEngineSessionError.emptyScreenshot + } + return png + }.value🤖 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 `@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/WebKitBrowserEngineSession.swift` around lines 171 - 177, Move the PNG encoding work out of the main actor in the screenshot method by extracting the underlying CGImage and performing NSBitmapImageRep creation and PNG representation inside a Task.detached operation. Preserve the existing BrowserEngineSessionError.emptyScreenshot failure behavior and return the encoded PNG data after awaiting the detached task.Source: Coding guidelines
Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumProcessController.swift (1)
63-76: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle task cancellation to prevent hanging and leaking the process.
If the calling task is cancelled,
withCheckedThrowingContinuationdoes not automatically resume or propagate cancellation to unstructured tasks. The caller will hang until the 10-secondtimeoutTaskcompletes, and the Chromium process will remain running during that time.Wrap the continuation in
withTaskCancellationHandlerto immediately terminate the process and resume the continuation upon cancellation.🛠 Proposed fix
- return try await withCheckedThrowingContinuation { continuation in - endpointContinuation = continuation - stderrTask = Task { [weak self] in - await self?.consumeStandardError(stderrPipe.fileHandleForReading) - } - timeoutTask = Task { [weak self, launchTimeout] in - // A bounded launch deadline is intentional behavior; never leave a pane waiting forever for DevTools. - try? await ContinuousClock().sleep(for: launchTimeout) - guard !Task.isCancelled else { return } - await self?.failUnresolvedEndpoint( - BrowserEngineSessionError.chromiumLaunch("Chromium timed out before opening its DevTools endpoint.") - ) - } + return try await withTaskCancellationHandler { + try await withCheckedThrowingContinuation { continuation in + endpointContinuation = continuation + stderrTask = Task { [weak self] in + await self?.consumeStandardError(stderrPipe.fileHandleForReading) + } + timeoutTask = Task { [weak self, launchTimeout] in + // A bounded launch deadline is intentional behavior; never leave a pane waiting forever for DevTools. + try? await ContinuousClock().sleep(for: launchTimeout) + guard !Task.isCancelled else { return } + await self?.failUnresolvedEndpoint( + BrowserEngineSessionError.chromiumLaunch("Chromium timed out before opening its DevTools endpoint.") + ) + } + } + } onCancel: { + Task { await self.close() } }🤖 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 `@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumProcessController.swift` around lines 63 - 76, Update the continuation flow around the Chromium endpoint wait to use withTaskCancellationHandler, so cancellation immediately terminates the Chromium process, cancels the stderr and timeout tasks, and resumes the pending continuation with cancellation rather than waiting for launchTimeout. Keep the existing endpoint success and timeout behavior unchanged for non-cancelled launches, using the surrounding process-controller cleanup symbols such as failUnresolvedEndpoint and the stored continuation.
🤖 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
`@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/CDPConnection.swift`:
- Around line 90-95: Update CDPConnection.events so Page.screencastFrame traffic
cannot evict control events such as Fetch.requestPaused or
Target.attachedToTarget. Replace the indiscriminate bufferingNewest policy with
separate delivery paths or equivalent classification that preserves control
events losslessly while coalescing only replaceable screencast frames.
In
`@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumProfileRuntime.swift`:
- Around line 29-88: Update acquireTarget so ownership of the
Target.createTarget result transfers to the runtime immediately after creation
and remains tracked until the target is successfully inserted into
activeTargetIDs. Ensure every subsequent failure, including attach errors and
cancellation after creation, closes the created target before propagating the
error; only relinquish cleanup responsibility once the lease is committed
successfully.
In
`@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/WebKitBrowserEngineSession.swift`:
- Around line 90-97: Update the JavaScript wrapper in the async evaluation
method around functionBody and callAsyncJavaScript: pass the original script as
an argument and execute it through eval() within the async function, then await
its result. Remove the parenthesized interpolation so scripts containing
trailing semicolons or multiple statements remain valid while preserving
implicit return behavior and promise awaiting.
In
`@Packages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/CDPConnectionTests.swift`:
- Around line 103-114: Update deliverAndWaitUntilConsumed to distinguish
immediate delivery from buffered delivery: when receiveContinuation is absent
and data is appended to bufferedData, wait for the subsequent receive/process
completion signal rather than receiveCount + 1, ensuring handleMessage has
broadcast the payload before the test continues. Preserve the existing
continuation-resume path for an actively waiting receiver.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 460-480: The cleanup started by removeChromiumRuntimeAndData must
no longer run in an untracked fire-and-forget Task. Make this operation
asynchronous and await runtime.close() and fileRemover.removeItemIfExists(at:)
from the caller, updating deleteProfile and any other callers to propagate
completion and errors through the existing lifecycle flow.
---
Outside diff comments:
In
`@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumBrowserEngineSession.swift`:
- Around line 149-161: Replace the unowned fire-and-forget Task operations in
the navigation and related control paths, including Page.navigate, the
operations around lines 177-180, and the flows in the viewport/state methods
around 685-719 and 807-815, with stored cancellable tasks or a single serialized
action owner. Ensure each task is cancelled when replaced and during session
teardown in close(), preventing stale operations and failures from affecting
newer browser actions.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 428-439: Update clearProfileData(id:) to remove the profile’s
entry from chromiumRuntimes before closing the runtime and deleting its
directory. Ensure subsequent chromiumRuntime(for:), switchToProfile, and
reloadChromium lookups create a fresh runtime while preserving the existing
cleanup and repository-clearing flow.
---
Duplicate comments:
In
`@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumProcessController.swift`:
- Around line 63-76: Update the continuation flow around the Chromium endpoint
wait to use withTaskCancellationHandler, so cancellation immediately terminates
the Chromium process, cancels the stderr and timeout tasks, and resumes the
pending continuation with cancellation rather than waiting for launchTimeout.
Keep the existing endpoint success and timeout behavior unchanged for
non-cancelled launches, using the surrounding process-controller cleanup symbols
such as failUnresolvedEndpoint and the stored continuation.
In
`@Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/WebKitBrowserEngineSession.swift`:
- Around line 171-177: Move the PNG encoding work out of the main actor in the
screenshot method by extracting the underlying CGImage and performing
NSBitmapImageRep creation and PNG representation inside a Task.detached
operation. Preserve the existing BrowserEngineSessionError.emptyScreenshot
failure behavior and return the encoded PNG data after awaiting the detached
task.
In `@Sources/KeyboardShortcutSettingsFileStore.swift`:
- Around line 909-915: In the browser engine parsing guard block, remove the
return statement that follows the logInvalid call for an invalid
BrowserEnginePreference. Keep the logInvalid logging in place to record the
invalid engine value, but allow execution to continue past this block so that
subsequent browser settings (search engine, theme, and other options) are still
parsed from the section.
🪄 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: 739347d6-a641-48ce-a969-2dbcdc193335
📒 Files selected for processing (58)
Packages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserEngineNavigationDecision.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserEngineNavigationDisposition.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserEngineNavigationPolicyHandler.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserEngineNavigationRequest.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserEngineResolver.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserEngineSession.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/BrowserEngineSessionError.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/BrowserChromiumProfileDirectory.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/CDPConnection.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumBrowserEngineSession+Viewport.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumBrowserEngineSession.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumDocumentTitleObservation.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumNavigationInterceptor.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumProcessController.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumProfileRuntime.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumProfileRuntimeConnection.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumProfileRuntimeLease.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/Chromium/ChromiumViewportDocument.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Engine/WebKitBrowserEngineSession.swiftPackages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/BrowserChromiumProfileDirectoryTests.swiftPackages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/BrowserEngineResolverTests.swiftPackages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/CDPConnectionTests.swiftPackages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/ChromiumBrowserEngineSessionTests.swiftPackages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/ChromiumNavigationInterceptorTests.swiftPackages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/ChromiumViewportDocumentLoadDelegate.swiftPackages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/ChromiumViewportDocumentTests.swiftPackages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/ChromiumViewportInputQueueTests.swiftPackages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/ChromiumViewportNoOpMessageHandler.swiftPackages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/NavigationTestCDPTransport.swiftPackages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Engine/WebKitBrowserEngineSessionTests.swiftResources/Localizable.xcstringsSources/KeyboardShortcutSettingsFileStore.swiftSources/Panels/BrowserPanel.swiftSources/TerminalController.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserWebContentProcessTests.swiftcmuxTests/WorkspaceUnitTests.swiftweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/en.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/ja.jsonweb/messages/km.jsonweb/messages/ko.jsonweb/messages/no.jsonweb/messages/pl.jsonweb/messages/pt-BR.jsonweb/messages/ru.jsonweb/messages/th.jsonweb/messages/tr.jsonweb/messages/uk.jsonweb/messages/zh-CN.jsonweb/messages/zh-TW.json
Contributor
|
Too many files changed for review. ( Bypass the limit by tagging |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
lawrencecchen
added
the
stale-revisit
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
3 participants
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
Testing
xcodebuild -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination "platform=macOS" -derivedDataPath /tmp/cmux-feat-browser-engine-choice -jobs 4 buildswift testinPackages/macOS/CmuxBrowser— 171 tests passed.swift test --skip-buildinPackages/macOS/CmuxSettings— 252 tests passed.swift test --skip-buildinPackages/macOS/CmuxSettingsUI— 112 tests passed../scripts/reload.sh --tag feat-browser-engine-choice%@placeholder checks passed.Demo Video
Review Trigger (Copy/Paste as PR comment)
Checklist
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds a configurable browser engine for new panes (Auto, WebKit, Chromium) with a unified, engine‑agnostic session layer. Engine choice persists across restores; topology reports the engine and PID; and Chromium’s viewport/input, control‑event buffering, process lifecycles, and main‑frame loading state are complete and bounded.
New Features
browser.engine(auto | webkit | chromium) with a Settings picker; Auto resolves via LaunchServices and selects Chromium only for known Chromium‑family apps; explicit picks bypass system defaults.browser_engineand resolve the correct content‑process PID per engine. Settings schema/docs/localizations updated.Bug Fixes
Written for commit 5bd81fd. Summary will update on new commits.
Summary by CodeRabbit