Repository navigation
Add focus-neutral split-off layout command - #3484
Conversation
|
The latest updates on your projects. Learn more about Vercel for 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThreads a validated ChangesFocus-Flag, Split-Off, and V2 Focus Plumbing
Sequence DiagramsequenceDiagram
participant User as User
participant CLI as cmux CLI
participant Parser as CLI Parser
participant Policy as Socket Policy
participant Server as TerminalController (V2)
participant Workspace as Workspace
User->>CLI: cmux split-off --surface S --focus false
CLI->>Parser: parse args, validate --focus
Parser-->>CLI: params["focus"]=false
CLI->>Server: v2 call "surface.split_off"(params)
Server->>Policy: withSocketCommandPolicy(commandKey, isV2:true, params)
Policy-->>Server: allow/deny focus mutation (evaluates params)
Server->>Workspace: perform split (focus=false)
Workspace-->>Server: success + new pane/surface refs
Server-->>CLI: {ok, result}
CLI-->>User: success (focus unchanged)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 5/8 reviews remaining, refill in 19 minutes and 6 seconds.Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21f6528f18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
21f6528 to
fd7d017
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
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)
1-17331:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftPipeline failure: File length budget exceeded.
The CI reports this file has 17331 lines but the budget is 17229 lines. This must be addressed before merge—consider extracting related functionality (e.g., V2 socket handlers) into a separate file or extension.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 1 - 17331, File exceeds allowed line count; extract large V2 socket/handler code into a separate extension file. Create a new file (e.g. TerminalController+V2.swift) and move the V2-related types, helpers and methods (examples: V2CallResult, V2SocketRequest, v2MainSync, v2Ok/v2Error/v2Result, v2Encode, v2* methods such as v2Capabilities(), v2Identify(), v2SystemTree(), all v2Browser*, v2Surface*, v2Workspace*, v2Pane* handlers, and related helpers like v2RunJavaScript/v2AwaitCallback/v2Browser* state variables) into that extension; keep TerminalController core socket accept/start/stop and minimal dispatching (processSocketLine/processCommandUsingSocketExecutionPolicy/processCommand) in the original file. Ensure moved symbols retain access (private/nonisolated/@MainActor as required) and update any references/imports; run build/tests to verify no access-level or name collisions.
🧹 Nitpick comments (2)
Sources/TerminalController.swift (1)
6118-6122: 💤 Low valueMinor: Redundant TabManager check.
Line 6119 checks
v2ResolveTabManager(params:)but discards the result, then lines 6140-6141 uselocateSurfacewhich returns a potentially different TabManager. The initial check is harmless (quick bailout) but slightly redundant sincelocateSurfacewould also fail if no TabManager exists.Consider removing the redundant check or adding a comment explaining its purpose (e.g., "early exit if no active window").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 6118 - 6122, The initial guard in v2SurfaceSplitOff redundantly calls v2ResolveTabManager(params:) and discards its result before later calling locateSurface which will also fail without a TabManager; remove the early guard or replace it with a clarifying comment. Specifically, edit the v2SurfaceSplitOff function to either delete the guard that invokes v2ResolveTabManager(params:) (so only locateSurface is relied upon) or keep the guard but add a brief comment like "early exit if no active window" to explain its purpose, ensuring no change to subsequent logic that uses locateSurface and v2UUID.tests/test_cli_layout_focus_contract.py (1)
93-108: 💤 Low valueSubprocess S603 lint warning is a false positive here.
cliis sourced fromresolve_cmux_cli()(the controlledCMUX_CLI_BINenv var set by CI), andargsare all hardcoded list literals inmain(). No user-supplied or externally-controlled input reaches thesubprocess.runcall. Suppressing this with# noqa: S603on Line 97 is safe and would eliminate the static-analysis noise.🔧 Suppress the false-positive lint warning
- proc = subprocess.run( + proc = subprocess.run( # noqa: S603🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli_layout_focus_contract.py` around lines 93 - 108, The S603 false positive can be suppressed: in the run_cli function, add a "# noqa: S603" comment to the subprocess.run invocation line (the call that uses [cli, "--socket", socket_path, *args]) to silence the linter, since cli comes from resolve_cmux_cli() and args are hardcoded; ensure the comment is placed on the same line as the subprocess.run call so the S603 warning is ignored for that call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 13609-13614: The code currently lets "--no-focus" silently
override a prior "--focus" value; change the logic in the blocks using
focusRaw/applyFocusOption/params so that if commandArgs contains both "--focus"
and "--no-focus" you immediately reject the invocation (throw or return an
error/exit with a clear message) instead of letting "--no-focus" win; implement
the same mutual-exclusion check in the other identical block handling focus (the
block around applyFocusOption at the second occurrence) so both places validate
and fail fast when both flags are present.
- Around line 9125-9144: The per-command help blocks under the "split-off" and
"drag-surface-to-split" cases currently advertise flags as "--surface <id|ref>"
and "--panel <id|ref>" but the resolver (see runSplitOff and the top-level
synopsis) supports "index" as well; update the help text for both commands so
the Flags lines for --surface and --panel list "<id|ref|index>" (and any
duplicated occurrences in those command string literals) to match the
standardized resolver contract.
- Around line 2762-2770: The parser currently takes the first leftover token
rem3.first as direction and forwards it to params, which allows mistyped flags
(e.g. "--bogus") to be accepted; update the code path around rem3/params so you
validate rem3.first against the allowed set {"left","right","up","down"} and
explicitly reject tokens that start with "--" by throwing CLIError(message:
"new-split requires a direction") before constructing params; apply the same
validation and rejection logic to the other parser that uses
normalizeWorkspaceHandle, normalizeSurfaceHandle and applyFocusOption so both
locations check direction and special-case "--..." tokens prior to building
params.
- Around line 4356-4388: The runSplitOff function and its plumbing (runSplitOff,
its use of parseOption, normalizeWorkspaceHandle, normalizeSurfaceHandle,
applyFocusOption, client.sendV2 call, v2OKSummary and printV2Payload) should be
moved out of the large CLI file into a new companion Swift source file that
implements the split-off command handler; create a new file that imports the
same modules, make the handler function internal/public as needed (signature:
runSplitOff(commandName:commandArgs:client:jsonOutput:idFormat:)), update the
command dispatch in the original file to call this new function, and ensure the
new file is added to the build target so the project compiles; apply the same
extraction pattern to the other newly added layout command handlers referenced
in the review.
In `@CLI/CMUXCLI`+MoveTabToNewWorkspace.swift:
- Around line 15-17: applyTabActionFocusOption currently calls
applyFocusOption(focusOpt, to: ¶ms) without forwarding defaultValue, so when
--focus is omitted the 'focus' key isn't written and server defaults (likely
true) are used; change the call in applyTabActionFocusOption to pass
defaultValue: false (i.e., call applyFocusOption(focusOpt, defaultValue: false,
to: ¶ms)) so the params always include focus=false by default and match the
PR/help text contract.
In `@cmuxTests/TerminalControllerSocketSecurityTests.swift`:
- Around line 113-135: The test file TerminalControllerSocketSecurityTests.swift
grew past the repo's Swift file-length budget by 23 lines; update the budget
table entry for cmuxTests/TerminalControllerSocketSecurityTests.swift in
.github/swift-file-length-budget.tsv to at least 605 (current new length) so
CI's "Validate Swift file length budget" step passes; open the TSV, find the row
for TerminalControllerSocketSecurityTests.swift and increase the numeric limit
to 605 (or a slightly larger safe value), commit the change, and push.
In `@Sources/Workspace.swift`:
- Around line 10918-10923: Change the default behavior of reorderSurface so it
is focus-neutral: modify the function signature of reorderSurface(panelId: UUID,
toIndex index: Int, focus: Bool = true) to default focus to false, and ensure
existing behavior still applies when focus is explicitly true by keeping the
conditional call to applyTabSelection(tabId:inPane:) (and use
paneId(forPanelId:) and surfaceIdFromPanelId(panelId) as currently used). Update
any call sites that relied on implicit focusing to explicitly pass focus: true
where appropriate.
---
Outside diff comments:
In `@Sources/TerminalController.swift`:
- Around line 1-17331: File exceeds allowed line count; extract large V2
socket/handler code into a separate extension file. Create a new file (e.g.
TerminalController+V2.swift) and move the V2-related types, helpers and methods
(examples: V2CallResult, V2SocketRequest, v2MainSync, v2Ok/v2Error/v2Result,
v2Encode, v2* methods such as v2Capabilities(), v2Identify(), v2SystemTree(),
all v2Browser*, v2Surface*, v2Workspace*, v2Pane* handlers, and related helpers
like v2RunJavaScript/v2AwaitCallback/v2Browser* state variables) into that
extension; keep TerminalController core socket accept/start/stop and minimal
dispatching
(processSocketLine/processCommandUsingSocketExecutionPolicy/processCommand) in
the original file. Ensure moved symbols retain access
(private/nonisolated/@MainActor as required) and update any references/imports;
run build/tests to verify no access-level or name collisions.
---
Nitpick comments:
In `@Sources/TerminalController.swift`:
- Around line 6118-6122: The initial guard in v2SurfaceSplitOff redundantly
calls v2ResolveTabManager(params:) and discards its result before later calling
locateSurface which will also fail without a TabManager; remove the early guard
or replace it with a clarifying comment. Specifically, edit the
v2SurfaceSplitOff function to either delete the guard that invokes
v2ResolveTabManager(params:) (so only locateSurface is relied upon) or keep the
guard but add a brief comment like "early exit if no active window" to explain
its purpose, ensuring no change to subsequent logic that uses locateSurface and
v2UUID.
In `@tests/test_cli_layout_focus_contract.py`:
- Around line 93-108: The S603 false positive can be suppressed: in the run_cli
function, add a "# noqa: S603" comment to the subprocess.run invocation line
(the call that uses [cli, "--socket", socket_path, *args]) to silence the
linter, since cli comes from resolve_cmux_cli() and args are hardcoded; ensure
the comment is placed on the same line as the subprocess.run call so the S603
warning is ignored for that call.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59fa918e-ed2e-476f-85ca-eee70d5a24b5
📒 Files selected for processing (11)
.circleci/config.yml.github/workflows/ci.ymlCLI/CMUXCLI+MoveTabToNewWorkspace.swiftCLI/cmux.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/TerminalControllerSocketSecurityTests.swiftdocs/cli-contract.mdskills/cmux/SKILL.mdskills/cmux/references/panes-surfaces.mdtests/test_cli_layout_focus_contract.py
| func reorderSurface(panelId: UUID, toIndex index: Int, focus: Bool = true) -> Bool { | ||
| guard let tabId = surfaceIdFromPanelId(panelId) else { return false } | ||
| guard bonsplitController.reorderTab(tabId, toIndex: index) else { return false } | ||
|
|
||
| if let paneId = paneId(forPanelId: panelId) { | ||
| if focus, let paneId = paneId(forPanelId: panelId) { | ||
| applyTabSelection(tabId: tabId, inPane: paneId) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Make reorderSurface focus-neutral by default
Line 10918 defaults focus to true, which keeps focus mutation implicit for omitted call sites. For layout-command safety, this should default to opt-in focus.
♻️ Proposed change
- func reorderSurface(panelId: UUID, toIndex index: Int, focus: Bool = true) -> Bool {
+ func reorderSurface(panelId: UUID, toIndex index: Int, focus: Bool = false) -> Bool {As per coding guidelines: "Socket/CLI commands must not steal macOS app focus. Only explicit focus-intent commands may mutate in-app focus/selection (window.focus, workspace.select/next/previous/last, surface.focus, pane.focus/last)."
📝 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.
| func reorderSurface(panelId: UUID, toIndex index: Int, focus: Bool = true) -> Bool { | |
| guard let tabId = surfaceIdFromPanelId(panelId) else { return false } | |
| guard bonsplitController.reorderTab(tabId, toIndex: index) else { return false } | |
| if let paneId = paneId(forPanelId: panelId) { | |
| if focus, let paneId = paneId(forPanelId: panelId) { | |
| applyTabSelection(tabId: tabId, inPane: paneId) | |
| func reorderSurface(panelId: UUID, toIndex index: Int, focus: Bool = false) -> Bool { | |
| guard let tabId = surfaceIdFromPanelId(panelId) else { return false } | |
| guard bonsplitController.reorderTab(tabId, toIndex: index) else { return false } | |
| if focus, let paneId = paneId(forPanelId: panelId) { | |
| applyTabSelection(tabId: tabId, inPane: paneId) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 10918 - 10923, Change the default
behavior of reorderSurface so it is focus-neutral: modify the function signature
of reorderSurface(panelId: UUID, toIndex index: Int, focus: Bool = true) to
default focus to false, and ensure existing behavior still applies when focus is
explicitly true by keeping the conditional call to
applyTabSelection(tabId:inPane:) (and use paneId(forPanelId:) and
surfaceIdFromPanelId(panelId) as currently used). Update any call sites that
relied on implicit focusing to explicitly pass focus: true where appropriate.
Greptile SummaryThis PR introduces a focus-neutral
Confidence Score: 3/5Not safe to merge as-is — the unintentional removal of the surface.split graceful fallback could break real-time collaborative split workflows. One P1 finding (silent behaviour regression in surface.split fallback) pulls the score below the P1 ceiling of 4. The rest of the changes are well-structured and covered by new tests. Sources/TerminalController.swift — specifically the v2SurfaceSplit function around line 5978 where the fallback was removed. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as CLI (cmux.swift)
participant TC as TerminalController
participant App as AppDelegate
participant WS as Workspace
Note over CLI,WS: split-off / drag-surface-to-split (v2 path)
CLI->>TC: sendV2(surface.split_off, {surface_id, direction, focus:false})
TC->>TC: withSocketCommandPolicy(params) — focus gate
TC->>App: locateSurface(surfaceId)
App-->>TC: located {tabManager, workspaceId, windowId}
TC->>WS: bonsplitController.splitPane(orientation, movingTab, insertFirst)
alt focus == true
TC->>App: focusMainWindow(windowId)
TC->>WS: focusPanel(surfaceId)
else focus == false (default)
TC->>WS: focusPanel(previousFocusedPanelId)
end
TC-->>CLI: {window_id, workspace_id, pane_id, surface_id}
Note over CLI,WS: surface.split (new-split) — focus fallback REMOVED
CLI->>TC: sendV2(surface.split, {surface_id?, direction, focus:false})
alt surface_id provided but not found
TC-->>CLI: err(not_found) was: fallback to focused surface
else surface_id absent
TC->>WS: newSplit(from: focusedPanelId)
TC-->>CLI: {surface_id, pane_id, ...}
end
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd7d017b2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
Sources/Workspace.swift (1)
10918-10923:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefault
focusshould be opt-in (false) to preserve focus-neutral behavior.Line 10918 currently defaults
focustotrue, so omitted callers still mutate focus. That makes it easy for socket/CLI paths to regress focus-neutral behavior unintentionally.♻️ Proposed change
- func reorderSurface(panelId: UUID, toIndex index: Int, focus: Bool = true) -> Bool { + func reorderSurface(panelId: UUID, toIndex index: Int, focus: Bool = false) -> Bool {As per coding guidelines: "Socket/CLI commands must not steal macOS app focus. Only explicit focus-intent commands may mutate in-app focus/selection (
window.focus,workspace.select/next/previous/last,surface.focus,pane.focus/last)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 10918 - 10923, The reorderSurface function currently defaults its focus parameter to true causing implicit focus changes; change the default to false in func reorderSurface(panelId: UUID, toIndex index: Int, focus: Bool = true) so it becomes focus: Bool = false, and audit callers of reorderSurface to ensure only explicit focus-intent calls pass true (leave socket/CLI paths and other focus-neutral callers using the no-argument form so they remain focus-preserving); keep the existing applyTabSelection(tabId:inPane:) call unchanged so explicit focus=true still triggers selection.CLI/CMUXCLI+MoveTabToNewWorkspace.swift (1)
15-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
tab-actionstill drops the documentedfocus=falsedefault.This wrapper now delegates to
applyFocusOptionwithoutdefaultValue: false, sotab.action/move-tab-to-new-workspaceomitparams["focus"]whenever--focusis absent. That still contradicts the PR contract and the help text at Line 63.🐛 Proposed fix
func applyTabActionFocusOption(_ focusOpt: String?, to params: inout [String: Any]) throws { - try applyFocusOption(focusOpt, to: ¶ms) + try applyFocusOption(focusOpt, defaultValue: false, to: ¶ms) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/CMUXCLI`+MoveTabToNewWorkspace.swift around lines 15 - 17, The wrapper applyTabActionFocusOption currently calls applyFocusOption(focusOpt, to: ¶ms) which omits the documented default of false and thus leaves params["focus"] unset when --focus is not provided; update applyTabActionFocusOption to call applyFocusOption with the defaultValue: false argument so that applyFocusOption populates params["focus"] = false when focusOpt is nil, preserving the documented behavior for tab.action / move-tab-to-new-workspace.
🧹 Nitpick comments (1)
CLI/CMUXCLI+MoveTabToNewWorkspace.swift (1)
110-115: ⚡ Quick winReuse
applyFocusOptioninrunMoveSurfacetoo.This is the one layout-mutating path in this file that still hand-parses
--focus, so it can drift from the shared validation/defaulting behavior the rest of the focus-neutral commands now use.♻️ Proposed refactor
- if let focusRaw = optionValue(commandArgs, name: "--focus") { - guard let focus = parseBoolString(focusRaw) else { - throw CLIError(message: "--focus must be true|false") - } - params["focus"] = focus - } + try applyFocusOption(optionValue(commandArgs, name: "--focus"), defaultValue: false, to: ¶ms)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/CMUXCLI`+MoveTabToNewWorkspace.swift around lines 110 - 115, The runMoveSurface path currently hand-parses the --focus option (using optionValue + parseBoolString + CLIError) which can diverge from shared behavior; replace that manual block in runMoveSurface with a call to the shared helper applyFocusOption so focus validation/defaulting is centralized. Remove the conditional that inspects focusRaw/parseBoolString and instead invoke applyFocusOption with the same commandArgs/params context (ensuring params["focus"] is set the same way), and delete any now-unused references to parseBoolString/CLIError in that method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/CMUXCLI`+MoveTabToNewWorkspace.swift:
- Around line 78-80: The bug is that surfaceRaw currently falls back to
commandArgs.first (let surfaceRaw = optionValue(commandArgs, name: "--surface")
?? commandArgs.first), which lets a leading flag like "--workspace" be
misinterpreted as the surface token; change the logic so
optionValue(commandArgs, name: "--surface") is required (no fallback to
commandArgs.first) and throw the CLIError when the option is absent (or use the
safer parseOption(...) pattern used by runSplitOff(...) if you want to accept
positional surfaces after other options); apply the same fix to the other
handler referenced around lines 162-165 to avoid flags being parsed as the
surface.
In `@Sources/TerminalController`+MoveTabToNewWorkspace.swift:
- Around line 109-141: The handler currently uses v2ResolveTabManager(params:)
only as a presence check then ignores its workspace scope and falls back to
AppDelegate.shared?.locateSurface(surfaceId:), causing workspace selectors
(workspace_ref/index) to be ignored or misapplied; change the logic to first
call v2ResolveTabManager(params:) and if it returns a manager, verify that the
manager actually owns the target surfaceId (compare manager.tabs / workspace ids
to the resolved surface) and use that manager/workspace for the split; if
v2ResolveTabManager(params:) is nil or it does not own the surfaceId, fall back
to AppDelegate.shared?.locateSurface(surfaceId:) as now (and only then), and
ensure you respect an explicit workspace_id param by comparing
requestedWorkspaceId (from v2UUID(params,"workspace_id")) against the resolved
workspace before proceeding; alternatively remove the upfront guard that returns
"TabManager not available" so locateSurface can be used when no workspace
selector was supplied.
- Around line 110-117: The error return messages in
TerminalController+MoveTabToNewWorkspace.swift (the .err(...) calls after the
TabManager check, the surface_id guard, the direction guard, and the other
returns around lines referenced) are hard-coded English; replace each
user-facing message string with localized variants using String(localized:
"terminal.moveTab.error.<key>", defaultValue: "<English text>") (choose unique
keys per message) where these appear (e.g., the .err call after checking
TabManager availability, the guard failures using v2UUID and
v2String/parseSplitDirection, and the other returns at the indicated ranges),
and add corresponding entries to Resources/Localizable.xcstrings with the same
keys and default English text. Ensure keys are descriptive (e.g.,
"tabmanager_unavailable", "missing_surface_id", "invalid_direction") and keep
the error codes unchanged.
---
Duplicate comments:
In `@CLI/CMUXCLI`+MoveTabToNewWorkspace.swift:
- Around line 15-17: The wrapper applyTabActionFocusOption currently calls
applyFocusOption(focusOpt, to: ¶ms) which omits the documented default of
false and thus leaves params["focus"] unset when --focus is not provided; update
applyTabActionFocusOption to call applyFocusOption with the defaultValue: false
argument so that applyFocusOption populates params["focus"] = false when
focusOpt is nil, preserving the documented behavior for tab.action /
move-tab-to-new-workspace.
In `@Sources/Workspace.swift`:
- Around line 10918-10923: The reorderSurface function currently defaults its
focus parameter to true causing implicit focus changes; change the default to
false in func reorderSurface(panelId: UUID, toIndex index: Int, focus: Bool =
true) so it becomes focus: Bool = false, and audit callers of reorderSurface to
ensure only explicit focus-intent calls pass true (leave socket/CLI paths and
other focus-neutral callers using the no-argument form so they remain
focus-preserving); keep the existing applyTabSelection(tabId:inPane:) call
unchanged so explicit focus=true still triggers selection.
---
Nitpick comments:
In `@CLI/CMUXCLI`+MoveTabToNewWorkspace.swift:
- Around line 110-115: The runMoveSurface path currently hand-parses the --focus
option (using optionValue + parseBoolString + CLIError) which can diverge from
shared behavior; replace that manual block in runMoveSurface with a call to the
shared helper applyFocusOption so focus validation/defaulting is centralized.
Remove the conditional that inspects focusRaw/parseBoolString and instead invoke
applyFocusOption with the same commandArgs/params context (ensuring
params["focus"] is set the same way), and delete any now-unused references to
parseBoolString/CLIError in that method.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59d48a18-b9a9-4345-b53d-7498e87f9217
📒 Files selected for processing (9)
CLI/CMUXCLI+MoveTabToNewWorkspace.swiftCLI/cmux.swiftSources/TerminalController+MoveTabToNewWorkspace.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/TerminalControllerSocketSecurityTests.swiftdocs/cli-contract.mdskills/cmux/SKILL.mdskills/cmux/references/panes-surfaces.md
💤 Files with no reviewable changes (1)
- cmuxTests/TerminalControllerSocketSecurityTests.swift
✅ Files skipped from review due to trivial changes (3)
- docs/cli-contract.md
- skills/cmux/references/panes-surfaces.md
- skills/cmux/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- CLI/cmux.swift
…focus-neutral-split-off
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_cli_layout_focus_contract.py`:
- Around line 119-133: The failing test helper assert_cli_fails copies the
current environment but does not remove CMUX_* variables, so existing
CMUX_WORKSPACE_ID/CMUX_SURFACE_ID/CMUX_TAB_ID can change negative-path behavior;
update assert_cli_fails to pop "CMUX_WORKSPACE_ID", "CMUX_SURFACE_ID", and
"CMUX_TAB_ID" from the env dict (same as run_cli) before calling subprocess.run
so the CLI sees a clean environment and the negative tests reliably trigger the
expected errors.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc820a74-a3b4-4daa-84b3-40d7c018d634
📒 Files selected for processing (5)
CLI/CMUXCLI+MoveTabToNewWorkspace.swiftCLI/cmux.swiftResources/Localizable.xcstringsSources/TerminalController+MoveTabToNewWorkspace.swifttests/test_cli_layout_focus_contract.py
✅ Files skipped from review due to trivial changes (1)
- Resources/Localizable.xcstrings
🚧 Files skipped from review as they are similar to previous changes (2)
- CLI/CMUXCLI+MoveTabToNewWorkspace.swift
- Sources/TerminalController+MoveTabToNewWorkspace.swift
This branch was successfully deployed
1 participant
Summary
split-offand routedrag-surface-to-splitthrough v2 ID resolution.--focus true|falsehandling across layout CLI commands, defaulting to false.Testing
./scripts/reload.sh --tag c3481succeeded.CMUX_CLI_BIN="<tagged cmux CLI>" python3 tests/test_cli_layout_focus_contract.pypassed.python3 scripts/swift_file_length_budget.py --repo-root . --budget .github/swift-file-length-budget.tsvpassed.workflow-guard-tests,remote-daemon-tests, CircleCImacos-unit-tests,macos-debug-build, andmacos-release-build.Issues
split-offfor tab moves #3481Summary by cubic
Adds a focus‑neutral layout command,
split-off, and makes focus opt‑in via--focus <true|false>across layout and open commands (default false). Also routesdrag-surface-to-splitthrough v2 with stable ID resolution and improves focus handoff, validation, and test coverage. Closes #3481.New Features
split-off --surface <id|ref|index> <left|right|up|down>moves a surface into a new split without changing focus by default; validates direction, restores previous focus when not focusing, guards against empty source panes, and supports--workspaceand--panelaliases.drag-surface-to-splitnow uses v2surface.split_offwith workspace/ref/index resolution; supports--focus.--focus <true|false>added and defaulted tofalseon:new-workspace,new-split,new-pane,new-surface,reorder-surface,swap-pane,break-pane,join-pane,markdown open, andbrowser open|open-split|new(--no-focusremains as an alias onbreak-pane/join-pane; conflicts with--focusare rejected).focus=trueis passed to explicit v2 methods; otherwise previous focus is preserved. Improved CLI help, strict direction parsing, and unknown flag checks; localized error messages added. New CLI testtests/test_cli_layout_focus_contract.pyverifies defaultfocus=falseand v2 routing, runs in CI, and is hardened to avoid env leaks and use a fake cmux server.Migration
--focus trueexplicitly (e.g.,cmux split-off --surface surface:1 right --focus true,cmux browser open https://example.com --focus true).Written for commit fed46e2. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation