Repository navigation
Add browser profile targeting to CLI pane creation - #8874
Conversation
|
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:
📝 WalkthroughWalkthroughChangesThe PR adds browser profile selector resolution, CLI and socket plumbing, profile-ID propagation into browser creation, localized errors/help, and Swift/Python integration coverage for valid, missing, and ambiguous selectors. Browser profile selection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant TerminalController
participant BrowserProfileStore
participant Workspace
CLI->>TerminalController: send profile selector
TerminalController->>BrowserProfileStore: resolveProfileSelection(selector)
BrowserProfileStore-->>TerminalController: matched ID or invalid result
TerminalController->>Workspace: create browser surface or split with preferredProfileID
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (22 passed)
✨ Finishing Touches🧪 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 |
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. |
![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 `@cmuxTests/BrowserProfileSocketTests.swift`:
- Around line 12-33: Register cleanup before the throwing profile-creation calls
in browserCreationCommandsHonorExplicitProfilesAndRejectInvalidSelectors, using
a mutable collection of created profile IDs that cleanup can update as each
creation succeeds. Ensure the defer restores BrowserAvailabilitySettings and
effectiveLastUsedProfileID and deletes every partially or fully created profile,
including when any try `#require` call fails.
🪄 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 Plus
Run ID: 0f447d20-32da-4f4a-9ae2-b5a65b941c81
📒 Files selected for processing (4)
Packages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Profiles/BrowserProfileRepositoryTests.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserProfileSocketTests.swifttests/test_browser_profile_cli.py
Greptile SummaryThis PR adds
Confidence Score: 5/5Safe to merge; the profile-selection and socket-plumbing changes are well-scoped and fully tested. The implementation covers all acceptance criteria with matching unit and integration tests. Profile resolution is correctly isolated to the MainActor through BrowserProfileRepository, actor isolation on omitsRemoteMirrorOnlyWindow is properly fixed, and all new user-facing strings have English and Japanese translations. The one observable behavioural asymmetry — profile-not-found fires before browser-disabled when the browser is disabled — is intentional and pinned by the BrowserProfileSocketTests assertions. No correctness bugs, blocking primitives, or test seams in production source were found. Files Needing Attention: Sources/TerminalController+ControlPaneContext.swift and Sources/TerminalController.swift share the same profile-resolution-before-availability ordering; worth revisiting if the UX of disabled-browser + unresolved selector comes up in user feedback. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["CLI --profile selector"] --> B["parseBrowserProfileOption"]
B --> C{pane type?}
C -->|not browser| D["CLIError: profileRequiresBrowserPane"]
C -->|browser| E["Send profile= to socket"]
E --> F{Multiple profile keys?}
F -->|yes| G["invalid_params: multipleProfileSelectors"]
F -->|no| H{Value is non-string?}
H -->|yes| I["invalid_params: invalidProfileSelector"]
H -->|no| J{selector present?}
J -->|no| K["preferredProfileID = nil (existing fallback)"]
J -->|yes| L["BrowserProfileStore.resolveProfileSelection"]
L -->|matched| M["preferredProfileID = profile.id"]
L -->|notFound| N["invalid_params: profileNotFound"]
L -->|ambiguous| O["invalid_params: ambiguousProfile + candidate UUIDs"]
M --> P{Browser disabled?}
K --> P
P -->|yes + profile given| Q["invalid_params: browserDisabled"]
P -->|yes, no profile| R["legacy disabled flow"]
P -->|no| S{Remote workspace?}
S -->|yes + profile given| T["invalid_params: profileUnavailableInRemoteWorkspace"]
S -->|no| U["newBrowserSplit / DockSplitStore with preferredProfileID"]
Reviews (7): Last reviewed commit: "test: import remote workspace configurat..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let first = repo.createProfile(named: "Shared")! | ||
| let second = repo.createProfile(named: "shared")! | ||
|
|
||
| #expect(repo.resolveProfileSelection("SHARED") == .ambiguous([first, second])) |
There was a problem hiding this comment.
The assertion == .ambiguous([first, second]) depends on the implementation returning candidates in insertion/creation order. If resolveProfileSelection builds its candidate list from a Set, Dictionary, or any other unordered structure, this test will fail spuriously even when the result is semantically correct. The downstream socket test (BrowserProfileSocketTests) correctly avoids this by using Set-based comparison. Consider extracting IDs and comparing as a Set here too, or pinning the order expectation to whatever the production implementation guarantees (e.g. by name).
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in 242422d: the test now compares the ambiguous candidate UUIDs as a Set, so it asserts membership without inventing an ordering contract.
— Claude Code
| @Test func browserCreationCommandsHonorExplicitProfilesAndRejectInvalidSelectors() throws { | ||
| let defaults = UserDefaults.standard | ||
| let wasBrowserDisabled = BrowserAvailabilitySettings.isDisabled(defaults: defaults) | ||
| let store = BrowserProfileStore.shared | ||
| let previousLastUsedProfileID = store.effectiveLastUsedProfileID | ||
| let suffix = UUID().uuidString | ||
| let target = try #require(store.createProfile(named: "Issue 2720 Target \(suffix)")) | ||
| let fallback = try #require(store.createProfile(named: "Issue 2720 Fallback \(suffix)")) | ||
| let ambiguousName = "Issue 2720 Shared \(suffix)" | ||
| let ambiguousFirst = try #require(store.createProfile(named: ambiguousName)) | ||
| let ambiguousSecond = try #require(store.createProfile(named: ambiguousName.lowercased())) | ||
| let createdProfileIDs = [target.id, fallback.id, ambiguousFirst.id, ambiguousSecond.id] | ||
|
|
||
| BrowserAvailabilitySettings.setDisabled(false, defaults: defaults) | ||
| defer { | ||
| TerminalController.shared.setActiveTabManager(nil) | ||
| for profileID in createdProfileIDs { | ||
| _ = store.deleteProfile(id: profileID) | ||
| } | ||
| store.noteUsed(previousLastUsedProfileID) | ||
| BrowserAvailabilitySettings.setDisabled(wasBrowserDisabled, defaults: defaults) | ||
| } | ||
|
|
||
| store.noteUsed(fallback.id) | ||
| let openManager = TabManager() | ||
| defer { openManager.tabs.forEach { $0.teardownAllPanels() } } | ||
| let openWorkspace = try #require(openManager.selectedWorkspace) | ||
| let openSourceID = try #require(openWorkspace.focusedPanelId) | ||
| TerminalController.shared.setActiveTabManager(openManager) | ||
|
|
||
| let openResponse = try call( | ||
| method: "browser.open_split", | ||
| params: [ | ||
| "workspace_id": openWorkspace.id.uuidString, | ||
| "surface_id": openSourceID.uuidString, | ||
| "url": "about:blank", | ||
| "profile": target.displayName.lowercased(), | ||
| "focus": false, | ||
| ] | ||
| ) | ||
| let openResult = try successfulResult(openResponse) | ||
| let openedSurfaceID = try #require( | ||
| (openResult["surface_id"] as? String).flatMap(UUID.init(uuidString:)) | ||
| ) | ||
| let openedPanel = try #require(openWorkspace.panels[openedSurfaceID] as? BrowserPanel) | ||
| #expect(openedPanel.profileID == target.id) | ||
|
|
||
| store.noteUsed(fallback.id) | ||
| let paneManager = TabManager() | ||
| defer { paneManager.tabs.forEach { $0.teardownAllPanels() } } | ||
| let paneWorkspace = try #require(paneManager.selectedWorkspace) | ||
| let paneSourceID = try #require(paneWorkspace.focusedPanelId) | ||
| TerminalController.shared.setActiveTabManager(paneManager) | ||
|
|
||
| let paneResponse = try call( | ||
| method: "pane.create", | ||
| params: [ | ||
| "workspace_id": paneWorkspace.id.uuidString, | ||
| "surface_id": paneSourceID.uuidString, | ||
| "direction": "right", | ||
| "type": "browser", | ||
| "url": "about:blank", | ||
| "profile": target.id.uuidString, | ||
| "focus": false, | ||
| ] | ||
| ) | ||
| let paneResult = try successfulResult(paneResponse) | ||
| let paneSurfaceID = try #require( | ||
| (paneResult["surface_id"] as? String).flatMap(UUID.init(uuidString:)) | ||
| ) | ||
| let panePanel = try #require(paneWorkspace.panels[paneSurfaceID] as? BrowserPanel) | ||
| #expect(panePanel.profileID == target.id) | ||
|
|
||
| store.noteUsed(fallback.id) | ||
| let fallbackManager = TabManager() | ||
| defer { fallbackManager.tabs.forEach { $0.teardownAllPanels() } } | ||
| let fallbackWorkspace = try #require(fallbackManager.selectedWorkspace) | ||
| let fallbackSourceID = try #require(fallbackWorkspace.focusedPanelId) | ||
| TerminalController.shared.setActiveTabManager(fallbackManager) | ||
|
|
||
| let fallbackResponse = try call( | ||
| method: "browser.open_split", | ||
| params: [ | ||
| "workspace_id": fallbackWorkspace.id.uuidString, | ||
| "surface_id": fallbackSourceID.uuidString, | ||
| "url": "about:blank", | ||
| "focus": false, | ||
| ] | ||
| ) | ||
| let fallbackResult = try successfulResult(fallbackResponse) | ||
| let fallbackSurfaceID = try #require( | ||
| (fallbackResult["surface_id"] as? String).flatMap(UUID.init(uuidString:)) | ||
| ) | ||
| let fallbackPanel = try #require(fallbackWorkspace.panels[fallbackSurfaceID] as? BrowserPanel) | ||
| #expect(fallbackPanel.profileID == fallback.id) | ||
|
|
||
| let unknownSelector = "Issue 2720 Missing \(suffix)" | ||
| let unknownResponse = try call( | ||
| method: "browser.open_split", | ||
| params: [ | ||
| "workspace_id": fallbackWorkspace.id.uuidString, | ||
| "surface_id": fallbackSourceID.uuidString, | ||
| "profile": unknownSelector, | ||
| ] | ||
| ) | ||
| let unknownError = try errorPayload(unknownResponse) | ||
| #expect(unknownError["code"] as? String == "invalid_params") | ||
| #expect((unknownError["message"] as? String)?.contains(unknownSelector) == true) | ||
| #expect((unknownError["data"] as? [String: Any])?["profile"] as? String == unknownSelector) | ||
|
|
||
| let ambiguousResponse = try call( | ||
| method: "pane.create", | ||
| params: [ | ||
| "workspace_id": fallbackWorkspace.id.uuidString, | ||
| "surface_id": fallbackSourceID.uuidString, | ||
| "direction": "right", | ||
| "type": "browser", | ||
| "profile": ambiguousName.uppercased(), | ||
| ] | ||
| ) | ||
| let ambiguousError = try errorPayload(ambiguousResponse) | ||
| let ambiguousMessage = try #require(ambiguousError["message"] as? String) | ||
| let ambiguousCandidates = try #require( | ||
| (ambiguousError["data"] as? [String: Any])?["candidates"] as? [[String: Any]] | ||
| ) | ||
| #expect(ambiguousError["code"] as? String == "invalid_params") | ||
| #expect(ambiguousMessage.contains(ambiguousFirst.id.uuidString)) | ||
| #expect(ambiguousMessage.contains(ambiguousSecond.id.uuidString)) | ||
| #expect(Set(ambiguousCandidates.compactMap { $0["id"] as? String }) == [ | ||
| ambiguousFirst.id.uuidString, | ||
| ambiguousSecond.id.uuidString, | ||
| ]) | ||
| } | ||
|
|
||
| private func call(method: String, params: [String: Any]) throws -> [String: Any] { | ||
| let request: [String: Any] = [ | ||
| "jsonrpc": "2.0", | ||
| "id": UUID().uuidString, | ||
| "method": method, | ||
| "params": params, | ||
| ] | ||
| let data = try JSONSerialization.data(withJSONObject: request) | ||
| let line = try #require(String(data: data, encoding: .utf8)) | ||
| let response = TerminalController.shared.handleSocketLine(line) | ||
| return try #require( | ||
| JSONSerialization.jsonObject(with: Data(response.utf8)) as? [String: Any] | ||
| ) | ||
| } | ||
|
|
||
| private func successfulResult(_ response: [String: Any]) throws -> [String: Any] { | ||
| #expect(response["ok"] as? Bool == true) | ||
| return try #require(response["result"] as? [String: Any]) | ||
| } | ||
|
|
||
| private func errorPayload(_ response: [String: Any]) throws -> [String: Any] { | ||
| #expect(response["ok"] as? Bool == false) | ||
| return try #require(response["error"] as? [String: Any]) | ||
| } | ||
| } |
There was a problem hiding this comment.
@Test function hides partial failures
All five scenarios — name-based open_split, UUID-based pane.create, no-profile fallback, unknown selector, and ambiguous selector — share a single @Test function. Because the function uses try #require and try successfulResult(...), the first failing call short-circuits the rest. When the unknown-selector path breaks, the ambiguous-selector coverage silently disappears, and vice versa. The sibling unit tests in BrowserProfileRepositoryTests correctly use one @Test func per scenario.
There was a problem hiding this comment.
Keeping this as one serialized behavior flow is intentional: it shares one BrowserProfileStore lifecycle and verifies the complete socket contract (name, UUID, fallback, unknown, ambiguous) without multiplying mutation of persisted global profile state. Each scenario has localized #expect/#require diagnostics, while repository edge cases remain split into isolated unit tests.
— Claude Code
There was a problem hiding this comment.
Fair point — the shared profile lifecycle and the layered "each subsequent scenario builds on the previous state" structure are real constraints that would complicate splitting. The distinction between socket-level behavioral flow tests and isolated unit tests is a reasonable boundary to draw here.
The short-circuit risk from try #require is still present, but given that all five scenarios share a single profile store setup and teardown, the consolidation tradeoff makes sense. Acknowledged.
| ["browser", "open-split", "https://example.com", "--profile", PROFILE_ID], | ||
| ) | ||
| assert_last_call(state, "browser.open_split", PROFILE_ID) | ||
|
|
||
| run_cli( | ||
| cli, | ||
| socket_path, | ||
| [ | ||
| "new-pane", | ||
| "--type", | ||
| "browser", | ||
| "--url", | ||
| "https://example.com", | ||
| "--profile", | ||
| "Work Profile", | ||
| ], | ||
| ) | ||
| assert_last_call(state, "pane.create", "Work Profile") | ||
|
|
||
| run_cli(cli, socket_path, ["browser", "open", "https://example.com"]) | ||
| assert_last_call(state, "browser.open_split", None) | ||
|
|
||
| profiles = run_cli(cli, socket_path, ["browser", "profiles"]) | ||
| if "Work Profile" not in profiles or PROFILE_ID not in profiles: | ||
| raise AssertionError(f"profile list omitted name or UUID: {profiles!r}") | ||
| if "last used" not in profiles: | ||
| raise AssertionError(f"profile list did not mark last-used profile: {profiles!r}") | ||
| finally: | ||
| server.shutdown() | ||
| server.server_close() | ||
| thread.join(timeout=5) | ||
| print("PASS: browser profile CLI plumbing") | ||
| return 0 | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| raise SystemExit(main()) |
There was a problem hiding this comment.
The Python tests verify that a correctly plumbed --profile value is forwarded to the socket, but they don't exercise what the CLI prints when the socket returns an invalid_params error for an unknown or ambiguous selector. The acceptance criteria include "unknown and ambiguous selectors return clear errors", but run_cli raises AssertionError on any non-zero exit code without inspecting the error text. As a result, a regression where the CLI swallows the error or prints a raw internal message would not be caught here.
There was a problem hiding this comment.
The feature-specific unknown/ambiguous error code, selector, candidate IDs, and human-readable message are covered at the real socket handler boundary in BrowserProfileSocketTests. The Python test intentionally owns only CLI argument/output plumbing; making its fake server synthesize the same error would retest the unchanged generic sendV2 error printer rather than profile resolution.
— Claude Code
There was a problem hiding this comment.
That's a fair separation of concerns. The socket-level tests own error code correctness and message content; the Python test owns CLI argument forwarding. Retesting the generic error printer through a fake server would add noise without catching profile-resolution regressions.
Acknowledged — no change needed here.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/cmux.swift (1)
13396-13446: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd multilingual translations for the new profile localization keys.
Localizable.xcstringsonly includesenandjaforcli.browser.profile.error.emptySelector,cli.browser.profile.option,cli.newPane.help.profileDescription, andcli.browser.profiles.marker.lastUsed, whileInfoPlist.xcstringssupports 18 locales andLocalizable.xcstringssupports 20. New Swift localization keys need matching translations for every supported locale in the touched catalog.🤖 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 `@CLI/cmux.swift` around lines 13396 - 13446, Add translations for cli.browser.profile.error.emptySelector, cli.browser.profile.option, cli.newPane.help.profileDescription, and cli.browser.profiles.marker.lastUsed in Localizable.xcstrings for every locale supported by that catalog, not just en and ja. Preserve the existing English and Japanese entries and match the catalog’s established localization structure.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 `@CLI/cmux.swift`:
- Around line 4661-4670: Extract the duplicated profile selector trimming and
empty validation into a shared throwing helper, such as
resolvedBrowserProfileSelector(_ raw: String?) -> String?, preserving the
existing localized error and optional behavior. Replace the inline validation in
both the new-pane flow and the browser open/open-split/new flow with this helper
before assigning params["profile"].
In `@Sources/TerminalController`+ControlPaneContext.swift:
- Around line 170-193: Move the explicit browser profile selector resolution and
validation before the BrowserAvailabilitySettings.isDisabled() external-open
return path in Sources/TerminalController+ControlPaneContext.swift#L170-L193 and
Sources/TerminalController.swift#L6517-L6544. Preserve the existing no-selector
fallback, while ensuring unknown or ambiguous selectors return invalid_params
even when the browser is disabled and a URL is provided.
---
Outside diff comments:
In `@CLI/cmux.swift`:
- Around line 13396-13446: Add translations for
cli.browser.profile.error.emptySelector, cli.browser.profile.option,
cli.newPane.help.profileDescription, and cli.browser.profiles.marker.lastUsed in
Localizable.xcstrings for every locale supported by that catalog, not just en
and ja. Preserve the existing English and Japanese entries and match the
catalog’s established localization structure.
🪄 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 Plus
Run ID: f5bf7fc1-3890-46a6-9efa-174f80855109
📒 Files selected for processing (15)
.github/workflows/ci.ymlCLI/cmux.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Profiles/Repository/BrowserProfileRepository.swiftPackages/macOS/CmuxBrowser/Sources/CmuxBrowser/Profiles/Values/BrowserProfileSelectionResolution.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Pane/ControlCommandCoordinator+Pane.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Pane/ControlPaneBrowserProfileCandidate.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Pane/ControlPaneCreateInputs.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Pane/ControlPaneCreateResolution.swiftResources/Localizable.xcstringsSources/DockSplitStore.swiftSources/Panels/BrowserAutomation.swiftSources/Panels/BrowserPanel.swiftSources/TerminalController+ControlPaneContext.swiftSources/TerminalController+ControlPaneDock.swiftSources/TerminalController.swift
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/Profiles/BrowserProfileRepositoryTests.swift`:
- Line 266: Update the assertion in the profile candidate test to compare Set
values on both sides: wrap the expected first.id and second.id collection in Set
while preserving the existing candidate ID mapping.
🪄 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 Plus
Run ID: 60d2cf1c-851f-4428-9f4c-1f8d73ae8f0a
📒 Files selected for processing (2)
Packages/macOS/CmuxBrowser/Tests/CmuxBrowserTests/Profiles/BrowserProfileRepositoryTests.swiftcmuxTests/BrowserProfileSocketTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/TerminalController+ControlPaneContext.swift (1)
171-177: 🎯 Functional Correctness | 🟠 MajorValidate explicit profiles before disabled-browser fallback.
With browser creation disabled and a URL present, both methods return via external open before reaching these blocks. Unknown, ambiguous, or malformed explicit selectors therefore succeed instead of returning
invalid_params. Move selector validation before the disabled-browser return, while retaining the omitted-selector fallback; add disabled-browser coverage for invalid selectors.
Sources/TerminalController+ControlPaneContext.swift#L171-L177: resolve or reject an explicit selector beforebrowserDisabledCreateResolution(...).Sources/TerminalController.swift#L6517-L6526: resolve or reject an explicit selector beforev2BrowserDisabledExternalOpenResult(...).As per path instructions, an unavailable reliable selector must fail closed rather than fall back.
🤖 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`+ControlPaneContext.swift around lines 171 - 177, Validate explicit browser profile selectors before the disabled-browser fallback, rejecting unknown, ambiguous, malformed, or otherwise unavailable selectors with invalid_params rather than opening the URL externally; retain the omitted-selector fallback. Apply this in Sources/TerminalController+ControlPaneContext.swift lines 171-177 around browser profile resolution and in Sources/TerminalController.swift lines 6517-6526 before v2BrowserDisabledExternalOpenResult(...), and add disabled-browser coverage for invalid selectors.Source: Path instructions
🤖 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 `@CLI/cmux.swift`:
- Around line 13399-13413: Extract the duplicated `--profile` trimming and
empty-selector validation into a shared helper such as
`resolvedBrowserProfileSelector(_:)`, reusing the existing
`cli.browser.profile.error.emptySelector` error. Update both the `new-pane`
parsing flow and the `focus`/`profile` parsing flow to call the helper and
remove their local validation blocks, preserving the optional selector behavior.
---
Duplicate comments:
In `@Sources/TerminalController`+ControlPaneContext.swift:
- Around line 171-177: Validate explicit browser profile selectors before the
disabled-browser fallback, rejecting unknown, ambiguous, malformed, or otherwise
unavailable selectors with invalid_params rather than opening the URL
externally; retain the omitted-selector fallback. Apply this in
Sources/TerminalController+ControlPaneContext.swift lines 171-177 around browser
profile resolution and in Sources/TerminalController.swift lines 6517-6526
before v2BrowserDisabledExternalOpenResult(...), and add disabled-browser
coverage for invalid selectors.
🪄 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 Plus
Run ID: 24fd40cd-c170-4def-bc0f-a49eafb229ea
📒 Files selected for processing (9)
CLI/cmux.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Pane/ControlCommandCoordinator+Pane.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Pane/ControlPaneCreateInputs.swiftResources/Localizable.xcstringsSources/Panels/BrowserAutomation.swiftSources/TerminalController+ControlPaneContext.swiftSources/TerminalController.swiftcmuxTests/BrowserProfileSocketTests.swifttests/test_browser_profile_cli.py
1 participant
Closes #2720
Issue: #2720
Summary
CmuxBrowserdomain, preferring an existing UUID before exact case-insensitive display-name matching.browser.open_splitandpane.create, including Dock pane creation, while leaving an omitted selector asnilso the existing source/workspace/last-used fallback chain is unchanged.invalid_paramserrors for unknown and ambiguous selectors; ambiguous errors include every candidate display name and UUID.--profile <name-or-uuid>plumbing and help forbrowser open,browser open-split, andnew-pane --type browser.browser profilessocket API in the CLI and label its effective current profile aslast used.Acceptance criteria
cmux browser open <url> --profile <name-or-uuid>forwards and applies the selected profile.cmux browser open-split <url> --profile <name-or-uuid>forwards and applies the selected profile.cmux new-pane --type browser --url <url> --profile <name-or-uuid>forwards and applies the selected profile.cmux browser profileslists profile display names and UUIDs and marks the last-used profile.--profilepreserves the existing profile fallback behavior.Verification
swift test --package-path Packages/macOS/CmuxBrowser --filter BrowserProfileRepositoryTests(22 tests passed under native arm64)swift test --package-path Packages/macOS/CmuxControlSocket(278 tests passed under native arm64)./scripts/lint-pbxproj-test-wiring.sh./scripts/check-pbxproj.shpython3 scripts/check-package-resolved-policy.pypython3 scripts/check-workspace-package-groups.py --checkpython3 scripts/lint-feature-flags.pyResources/Localizable.xcstringswith duplicate-key detection and verified English/Japanese coverage for every changed key.Summary by CodeRabbit
--profile <name|uuid>support for browser pane creation, browser open/split flows, and split/tab creation with a resolved “preferred profile” carried through.invalid_paramsresponses (including candidate details when applicable).