Repository navigation
Add cmux window display to place a window on a named display - #5804
Merged
Merged
Conversation
New v2 socket commands `window.display` / `window.displays` plus a `cmux window` CLI namespace. `window display <name|index>` moves the instance's window(s) onto a display matched by name (case-insensitive exact, then substring) or zero-based index, preserving the window's size and centering it. `window displays` lists connected displays. The move is deliberately not a focus-intent command: it stays out of `focusIntentV2Methods` and never calls activate/makeKeyAndOrderFront, so it repositions without stealing macOS focus. This lets agent sessions place dev builds on a chosen monitor (the app moves its own window) without an external tiling window manager fighting cmux's window management. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a new window namespace: ChangesWindow Display Management
Sequence Diagram(s)sequenceDiagram
participant User as CLI (cmux)
participant Terminal as TerminalController
participant App as AppDelegate
User->>Terminal: request window.displays
Terminal->>App: availableDisplays()
App-->>Terminal: list of DisplayInfo
Terminal-->>User: ok {displays: [...]}
User->>Terminal: request window.display(display, window_id?)
Terminal->>App: moveMainWindow / moveAllMainWindows
App-->>Terminal: {display, window_ids}
Terminal-->>User: ok {display, window_ids} / not_found
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (18 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/cli-contract.md`:
- Line 95: Update the CLI docs line describing the window display command to
replace the awkward phrase "`--list` aliases `window displays`" with a clearer
wording; locate the entry for the `window display <name|index>` command and
change the trailing clause to one of the suggested rephrasings such as "`Use
--list to show available displays (same as the 'window displays' command).`" or
"`--list: show available displays (equivalent to the 'window displays'
command).`" to make the intent unambiguous.
- Around line 94-95: The docs mix new namespaced commands (window displays,
window display) with legacy hyphenated commands (list-windows, current-window,
new-window, focus-window, close-window) without explanation; add a "Window
subcommands:" section in the "Command Families" area (after the Auth/VM sections
around where other subcommand families are documented) that lists the window
namespace (window displays — List connected displays; window display
<name|index> — move instance windows, with --window and --list behaviors) and
include a brief note that the hyphenated top-level commands remain for backward
compatibility and point readers to the window subcommands for the new pattern,
or alternatively add an inline compatibility note next to each hyphenated entry
linking to the new namespace.
In `@Sources/AppDelegate.swift`:
- Around line 17937-17945: The isMain logic in the DisplayInfo mapping uses only
cmuxDisplayID equality and thus marks no main screen when cmuxDisplayID is nil;
update the isMain computation in the NSScreen.screens.enumerated() map so it
first checks if both displayID and mainID are non-nil and equal, otherwise fall
back to comparing the screen object to NSScreen.main (i.e., use (displayID !=
nil && displayID == mainID) || screen == NSScreen.main) to reliably detect the
main display; update the DisplayInfo creation site where isMain is set.
🪄 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: b740fe3c-ded3-4257-a80d-cb1a6e43fdac
📒 Files selected for processing (4)
CLI/cmux.swiftSources/AppDelegate.swiftSources/TerminalController.swiftdocs/cli-contract.md
Comment on lines
+94
to
+95
| | `window displays` | List connected displays (name, index, main flag). | | ||
| | `window display <name\|index>` | Move the instance's window(s) onto a display by name (exact, substring) or index, preserving size. Does not steal focus. With `--window`, targets that window; otherwise moves all main windows. `--list` aliases `window displays`. | |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Document the window namespace pattern alongside legacy hyphenated commands.
The new window displays and window display commands follow a namespaced subcommand pattern (like auth status, vm ls), while existing window-related commands (list-windows, current-window, new-window, focus-window, close-window at lines 89-93) use the hyphenated top-level pattern.
This creates two different command styles for window operations without explanation. Consider either:
- Preferred: Add a "Window subcommands:" section in the "Command Families" area (after line 176) to document the
windownamespace, similar to Auth subcommands (line 178) and VM subcommands (line 186). - Alternative: Add an inline note explaining that
window <subcommand>is the new pattern while legacy hyphenated commands remain for compatibility.
This will help users understand the migration path and keep the documentation organized consistently with other namespaced commands.
📋 Example "Window subcommands:" section
Add after line 176 (before "Auth subcommands:"):
+Window subcommands:
+
+| Command | Contract |
+| --- | --- |
+| `window displays` | List connected displays (name, index, main flag). |
+| `window display <name\|index>` | Move the instance's window(s) onto a display by name (exact, substring) or index, preserving size. Does not steal focus. With `--window`, targets that window; otherwise moves all main windows. `--list` aliases `window displays`. |
+
Auth subcommands:Then optionally add a note in the top-level table entries pointing to the Window subcommands section, or keep them in both places for discoverability.
🤖 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 `@docs/cli-contract.md` around lines 94 - 95, The docs mix new namespaced
commands (window displays, window display) with legacy hyphenated commands
(list-windows, current-window, new-window, focus-window, close-window) without
explanation; add a "Window subcommands:" section in the "Command Families" area
(after the Auth/VM sections around where other subcommand families are
documented) that lists the window namespace (window displays — List connected
displays; window display <name|index> — move instance windows, with --window and
--list behaviors) and include a brief note that the hyphenated top-level
commands remain for backward compatibility and point readers to the window
subcommands for the new pattern, or alternatively add an inline compatibility
note next to each hyphenated entry linking to the new namespace.
| | `focus-window` | Focus a window by handle. | | ||
| | `close-window` | Close a window by handle. | | ||
| | `window displays` | List connected displays (name, index, main flag). | | ||
| | `window display <name\|index>` | Move the instance's window(s) onto a display by name (exact, substring) or index, preserving size. Does not steal focus. With `--window`, targets that window; otherwise moves all main windows. `--list` aliases `window displays`. | |
There was a problem hiding this comment.
Clarify the --list alias phrasing.
The phrase "--list aliases window displays" is grammatically awkward and could be misinterpreted.
✏️ Suggested rephrasings
Choose one of these clearer alternatives:
-| `window display <name\|index>` | Move the instance's window(s) onto a display by name (exact, substring) or index, preserving size. Does not steal focus. With `--window`, targets that window; otherwise moves all main windows. `--list` aliases `window displays`. |
+| `window display <name\|index>` | Move the instance's window(s) onto a display by name (exact, substring) or index, preserving size. Does not steal focus. With `--window`, targets that window; otherwise moves all main windows. `window display --list` is an alias for `window displays`. |Or:
-| `window display <name\|index>` | Move the instance's window(s) onto a display by name (exact, substring) or index, preserving size. Does not steal focus. With `--window`, targets that window; otherwise moves all main windows. `--list` aliases `window displays`. |
+| `window display <name\|index>` | Move the instance's window(s) onto a display by name (exact, substring) or index, preserving size. Does not steal focus. With `--window`, targets that window; otherwise moves all main windows. Supports `--list` to list displays (equivalent to `window displays`). |📝 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
| | `window display <name\|index>` | Move the instance's window(s) onto a display by name (exact, substring) or index, preserving size. Does not steal focus. With `--window`, targets that window; otherwise moves all main windows. `--list` aliases `window displays`. | | |
| | `window display <name\|index>` | Move the instance's window(s) onto a display by name (exact, substring) or index, preserving size. Does not steal focus. With `--window`, targets that window; otherwise moves all main windows. `window display --list` is an alias for `window displays`. | |
🤖 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 `@docs/cli-contract.md` at line 95, Update the CLI docs line describing the
window display command to replace the awkward phrase "`--list` aliases `window
displays`" with a clearer wording; locate the entry for the `window display
<name|index>` command and change the trailing clause to one of the suggested
rephrasings such as "`Use --list to show available displays (same as the 'window
displays' command).`" or "`--list: show available displays (equivalent to the
'window displays' command).`" to make the intent unambiguous.
Comment on lines
+17937
to
+17945
| let mainID = NSScreen.main?.cmuxDisplayID | ||
| return NSScreen.screens.enumerated().map { index, screen in | ||
| let displayID = screen.cmuxDisplayID | ||
| return DisplayInfo( | ||
| name: screen.localizedName, | ||
| index: index, | ||
| displayID: displayID, | ||
| isMain: displayID != nil && displayID == mainID, | ||
| frame: screen.frame |
There was a problem hiding this comment.
isMain detection should not depend only on displayID.
Line 17944 marks main display only when both IDs are non-nil and equal. If cmuxDisplayID is unavailable, isMain becomes false for every display. Add a fallback to screen == NSScreen.main.
Suggested fix
- isMain: displayID != nil && displayID == mainID,
+ isMain: (displayID != nil && displayID == mainID) || screen == NSScreen.main,🤖 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/AppDelegate.swift` around lines 17937 - 17945, The isMain logic in
the DisplayInfo mapping uses only cmuxDisplayID equality and thus marks no main
screen when cmuxDisplayID is nil; update the isMain computation in the
NSScreen.screens.enumerated() map so it first checks if both displayID and
mainID are non-nil and equal, otherwise fall back to comparing the screen object
to NSScreen.main (i.e., use (displayID != nil && displayID == mainID) || screen
== NSScreen.main) to reliably detect the main display; update the DisplayInfo
creation site where isMain is set.
Contributor
Greptile SummaryAdds
Confidence Score: 5/5Safe to merge; the window-placement logic is well-scoped, correctly dispatched through the existing v2MainSync/MainActor pattern, and deliberately kept out of the focus-intent path. The actor-isolation model is correct: all AppKit calls happen inside v2MainSync on @mainactor AppDelegate. The focus-steal concern is properly addressed — window.display is absent from focusIntentV2Methods and setFrame is called without activate/makeKeyAndOrderFront. The only new finding is an edge-case where the all-windows path returns a success response with an empty moved list, which would print "Moved 0 windows to X." in the CLI; that can't happen in normal usage but is worth tightening. Sources/TerminalController.swift — the all-windows branch of v2WindowDisplay can return .ok with an empty moved array. Important Files Changed
Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+7137
to
7210
| throw CLIError(message: "window requires a subcommand. Try: display, displays") | ||
| } | ||
| let rest = Array(commandArgs.dropFirst()) | ||
| switch sub { | ||
| case "displays": | ||
| try runWindowDisplaysCommand(client: client, jsonOutput: jsonOutput) | ||
| case "display": | ||
| try runWindowDisplayCommand( | ||
| commandArgs: rest, | ||
| client: client, | ||
| jsonOutput: jsonOutput, | ||
| idFormat: idFormat, | ||
| windowOverride: windowOverride | ||
| ) | ||
| default: | ||
| throw CLIError(message: "Unknown window subcommand: \(sub). Try: display, displays") | ||
| } | ||
| } | ||
|
|
||
| /// `cmux window displays` — list connected displays (name + index). | ||
| private func runWindowDisplaysCommand(client: SocketClient, jsonOutput: Bool) throws { | ||
| let response = try client.sendV2(method: "window.displays") | ||
| if jsonOutput { | ||
| print(jsonString(response)) | ||
| return | ||
| } | ||
| let displays = (response["displays"] as? [[String: Any]]) ?? [] | ||
| if displays.isEmpty { | ||
| print("No displays found.") | ||
| return | ||
| } | ||
| for display in displays { | ||
| let name = (display["name"] as? String) ?? "(unknown)" | ||
| let index = (display["index"] as? Int) ?? -1 | ||
| let isMain = (display["main"] as? Bool) ?? false | ||
| print("\(index): \(name)\(isMain ? " (main)" : "")") | ||
| } | ||
| } | ||
|
|
||
| /// `cmux window display "<name>"` — move this instance's window(s) onto the | ||
| /// named display, preserving size. `--list` is an alias for `window displays`. | ||
| private func runWindowDisplayCommand( | ||
| commandArgs: [String], | ||
| client: SocketClient, | ||
| jsonOutput: Bool, | ||
| idFormat: CLIIDFormat, | ||
| windowOverride: String? | ||
| ) throws { | ||
| if commandArgs.contains("--list") || commandArgs.contains("-l") { | ||
| try runWindowDisplaysCommand(client: client, jsonOutput: jsonOutput) | ||
| return | ||
| } | ||
| let positional = commandArgs.filter { !$0.hasPrefix("-") } | ||
| guard let displayName = positional.first, !displayName.isEmpty else { | ||
| throw CLIError(message: "window display requires a display name. Usage: cmux window display \"LG HDR 4K\" (list names with: cmux window displays)") | ||
| } | ||
| var params: [String: Any] = ["display": displayName] | ||
| if let windowOverride { | ||
| let normalized = try normalizeWindowHandle(windowOverride, client: client) ?? windowOverride | ||
| params["window_id"] = normalized | ||
| } | ||
| let response = try client.sendV2(method: "window.display", params: params) | ||
| if jsonOutput { | ||
| print(jsonString(formatIDs(response, mode: idFormat))) | ||
| return | ||
| } | ||
| let resolvedDisplay = (response["display"] as? String) ?? displayName | ||
| let movedCount = (response["moved"] as? [Any])?.count ?? 0 | ||
| print("Moved \(movedCount) window\(movedCount == 1 ? "" : "s") to \(resolvedDisplay).") | ||
| } | ||
|
|
||
| private func runWorkspaceNamespace( | ||
| commandArgs: [String], | ||
| client: SocketClient, |
Contributor
There was a problem hiding this comment.
String(localized:)
runWindowNamespace, runWindowDisplaysCommand, and runWindowDisplayCommand all produce user-visible output with raw string literals — "window requires a subcommand. Try: display, displays", "No displays found.", "Moved \(movedCount) window\(movedCount == 1 ? "" : "s") to \(resolvedDisplay).", etc. The existing notification commands in this same file (e.g. dismiss-notification, mark-notification-read) use String(localized:defaultValue:) with catalog entries, so there is an established pattern to follow. Raw literals here skip localization for every non-English locale the app supports.
Rule Used: Flag production user-facing text that is not fully... (source)
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!
Comment on lines
+4102
to
+4120
|
|
||
| private func v2WindowDisplay(params: [String: Any]) -> V2CallResult { | ||
| guard let displayQuery = (params["display"] as? String)? | ||
| .trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !displayQuery.isEmpty else { | ||
| return .err(code: "invalid_params", message: "Missing or invalid display", data: nil) | ||
| } | ||
|
|
||
| // Explicit window target moves just that window; otherwise move every main | ||
| // window of this instance (a dev build usually has one). | ||
| if let windowId = v2UUID(params, "window_id") { | ||
| let resolved = v2MainSync { | ||
| AppDelegate.shared?.moveMainWindow(windowId: windowId, toDisplayMatching: displayQuery) | ||
| } | ||
| if let display = resolved { | ||
| return .ok([ | ||
| "display": display, | ||
| "window_id": windowId.uuidString, | ||
| "window_ref": v2Ref(kind: .window, uuid: windowId), |
Contributor
There was a problem hiding this comment.
v2MainSync calls in error-discrimination path create a TOCTOU window
When moveMainWindow(windowId:toDisplayMatching:) returns nil, the code issues a second independent v2MainSync to check windowForMainWindowId. Between the two calls the window can be created or destroyed, causing the wrong error code: if a window is created between the calls, the response says "not_found" for the display even though the window didn't exist when the move was attempted. Folding both checks into a single v2MainSync closure — for example returning an enum that distinguishes windowNotFound/displayNotFound — would eliminate the race and remove the redundant round-trip to the main actor.
- TerminalController v2WindowDisplay: the windowExists closure already returns a non-optional Bool, so the outer `?? false` was dead code and tripped the zero-warning budget. Removed it. - Refresh .github/swift-file-length-budget.tsv for the three files this feature grew (CLI/cmux.swift, TerminalController.swift, AppDelegate.swift). The additions are a cohesive ~90-line feature on already-large files; splitting into new files would require hand-wiring the Xcode project for three files across two targets, so the sanctioned budget refresh is the lower-risk fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts: # .github/swift-file-length-budget.tsv
This was referenced Jun 10, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds a
cmux window displayCLI command (andwindow.display/window.displaysv2 socket methods) that move a running cmux instance's window(s) onto a chosen monitor by name or index.cmux window displays— list connected displays (name, index, main flag).cmux window display "<name>"— move the window(s) onto that display, preserving size and centering. Matches by case-insensitive exact name, then substring, then zero-based index.--listaliaseswindow displays;--window <id>targets one window instead of all.Why
To place dev builds on a specific monitor (e.g. an external 4K) without an external tiling window manager. A tiling WM continuously moves/resizes cmux windows and fights cmux's own window management. Here the app moves its own window once, on command, so there is no external manager and no fight.
How
The CLI sends a v2 socket command; the app resolves the
NSScreenby name/index and callssetFrameto reposition the window onto that display's visible frame at its current size. The command is deliberately not a focus-intent method: it is kept out offocusIntentV2Methodsand never callsactivate/makeKeyAndOrderFront, so it repositions without stealing macOS focus (per the socket focus policy).Testing
Dogfooded on a two-display Mac: lists both displays; moves by exact name, substring, and index; graceful not-found error listing available displays; exactly one window moved (no spawning); and a focus-steal check confirmed the frontmost app was unchanged across the move. No automated test yet (window placement needs real displays); the display-name resolution logic is a good unit-test follow-up.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds
cmux windowcommands to list displays and move the app’s window(s) to a chosen monitor by name or index without stealing macOS focus. Implements v2 socket methods to list and move displays and prevents pre-focus forwindowcommands.New Features
cmux window displays— list connected displays (name, index, main).cmux window display "<name|index>"— move window(s) to a display; matches case-insensitive exact name, then substring, then zero-based index; preserves size and centers. Use--window <id>to target one window;--listaliaseswindow displays. Backed by v2 methodswindow.displaysandwindow.display; commands are not focus-intent and won’t activate or key the window.Bug Fixes
v2WindowDisplayand refreshed.github/swift-file-length-budget.tsvfor touched files.Written for commit 60e2359. Summary will update on new commits.
Summary by CodeRabbit
window displaysto list connected displays (name, index, main marker).window display <name|index>to move one or all main windows to a specified display by name or index, preserving size and focus.windowcommand so moves don’t steal focus.windowsubcommands and a--listalias.