Repository navigation
Move tabs into new workspaces - #3285
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:
📝 WalkthroughWalkthroughA comprehensive feature addition implementing "move tab to new workspace" functionality across the application, including CLI command routing, new AppDelegate APIs for workspace/panel management, UI components for drag-drop and context menus, localization entries, TerminalController v2 API support, and accompanying tests. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as UI Component<br/>(Menu/Drag-Drop)
participant AppDelegate
participant Workspace
participant TabManager
User->>UI: Initiate move<br/>(context menu or drag-drop)
UI->>AppDelegate: Check canMoveSurfaceToNewWorkspace(panelId)
AppDelegate-->>UI: true/false
alt Can Move
UI->>AppDelegate: moveSurfaceToNewWorkspace(panelId, focus, focusWindow)
AppDelegate->>Workspace: Create new workspace<br/>(with title & placement)
Workspace-->>AppDelegate: New workspace created
AppDelegate->>TabManager: Move panel from<br/>source to destination
TabManager-->>AppDelegate: Panel moved
AppDelegate-->>UI: SurfaceNewWorkspaceMoveResult
UI->>User: Display result<br/>(focus/window updates)
else Cannot Move
UI->>User: Play system beep
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryIntroduces a shared Confidence Score: 4/5Safe to merge; all findings are P2 style/defensive concerns with no runtime breakage on current call sites. Only P2 findings: a ?? true fallback that is safe today but fragile for future callers, a confusing property/method name collision, and a minor localization inconsistency. Core move logic, guard conditions, cleanup, and tests look correct. Sources/Panels/CmuxWebView.swift — ?? true default and property/method naming collision both live here. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant Entry as Entry Point<br/>(drag/menu/palette/CLI/socket)
participant AD as AppDelegate
participant TM as TabManager
participant WS as Workspace
U->>Entry: Trigger "Move Tab to New Workspace"
Entry->>AD: canMoveSurfaceToNewWorkspace(panelId)
AD-->>Entry: true/false (panels.count > 1)
Entry->>AD: moveSurfaceToNewWorkspace(panelId, focus, ...)
AD->>AD: locateSurface / resolve title
AD->>TM: addWorkspace(title, select: focus)
TM-->>AD: destinationWorkspace (+ bootstrapPanelIds)
AD->>AD: moveSurface(panelId -> destinationWorkspace)
alt moveSurface fails
AD->>TM: closeWorkspace(destinationWorkspace)
AD-->>Entry: nil
else moveSurface succeeds
AD->>WS: closePanel(bootstrapPanelId) for each bootstrap panel
AD-->>Entry: SurfaceNewWorkspaceMoveResult
end
Entry->>U: Update UI / return result
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| if contextMenuMoveTabToNewWorkspace != nil, |
There was a problem hiding this comment.
?? true fallback enables move item when no guard is set
contextMenuCanMoveTabToNewWorkspace?() ?? true means that if contextMenuMoveTabToNewWorkspace is set but contextMenuCanMoveTabToNewWorkspace is left as nil, the "Move Tab to New Workspace" menu item is unconditionally shown — even for a single-panel workspace where the move is invalid. BrowserPanel.swift always sets both closures together, so it doesn't trigger today, but any future callsite that wires only the action closure would silently skip the guard check.
Flipping the default to false is safer:
| } | |
| if contextMenuMoveTabToNewWorkspace != nil, | |
| contextMenuCanMoveTabToNewWorkspace?() ?? false, |
| } | ||
| } | ||
|
|
||
| @objc private func contextMenuMoveTabToNewWorkspace(_ sender: Any?) { | ||
| _ = sender | ||
| guard contextMenuMoveTabToNewWorkspace?() == true else { | ||
| NSSound.beep() | ||
| return |
There was a problem hiding this comment.
The @objc private func contextMenuMoveTabToNewWorkspace(_ sender: Any?) method has the same base name as the instance property var contextMenuMoveTabToNewWorkspace: (() -> Bool)? declared a few hundred lines above. Swift disambiguates them by signature, but the collision is confusing to read: inside the method body, contextMenuMoveTabToNewWorkspace?() reads as if it is a recursive call, when it is actually an optional-chained property invocation. Consider renaming the handler to something like contextMenuPerformMoveTabToNewWorkspace(_:) to make the two clearly distinct.
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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/ContentView.swift (1)
6743-6748:⚠️ Potential issue | 🟡 MinorUse
AppDelegate.canMoveSurfaceToNewWorkspace()to gate the palette command.
panelCanMoveToNewWorkspaceduplicates only the final check (workspace.panels.count > 1) while skipping the upstream guards inAppDelegate.canMoveSurfaceToNewWorkspace()(surface locatability, workspace validity, panel existence). To prevent command visibility and execution from drifting, delegate to the app-level helper instead of inline duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 6743 - 6748, The current snapshot boolean for CommandPaletteContextKeys.panelCanMoveToNewWorkspace is computed inline as workspace.panels.count > 1 which duplicates only the final check and skips upstream guards; replace that expression with a call to the app-level helper AppDelegate.canMoveSurfaceToNewWorkspace(...) passing the panel identifier (panelId) and the relevant workspace/surface context so the same locatability, workspace validity, and panel-existence checks are applied consistently (i.e., stop using workspace.panels.count > 1 and call AppDelegate.canMoveSurfaceToNewWorkspace with panelId and the workspace/surface).
🧹 Nitpick comments (1)
Sources/Panels/CmuxWebView.swift (1)
2175-2181: Re-check capability gate at action time before executing move.Line 2177 calls the move closure directly. If eligibility changes after menu construction (or a stale menu item is invoked), this can run execution without re-validating
contextMenuCanMoveTabToNewWorkspace.Suggested hardening
`@objc` private func contextMenuMoveTabToNewWorkspace(_ sender: Any?) { _ = sender + guard contextMenuCanMoveTabToNewWorkspace?() ?? true else { + NSSound.beep() + return + } guard contextMenuMoveTabToNewWorkspace?() == true else { NSSound.beep() return } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/CmuxWebView.swift` around lines 2175 - 2181, Before invoking the move closure, re-check the capability gate to avoid executing a stale action: call contextMenuCanMoveTabToNewWorkspace?() inside contextMenuMoveTabToNewWorkspace(_:) and only call contextMenuMoveTabToNewWorkspace?() if that returns true; otherwise beep/return as currently done. Reference the closures contextMenuCanMoveTabToNewWorkspace and contextMenuMoveTabToNewWorkspace when updating the guard logic so the capability is validated at action time rather than relying on the menu's previous state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 6743-6748: The current snapshot boolean for
CommandPaletteContextKeys.panelCanMoveToNewWorkspace is computed inline as
workspace.panels.count > 1 which duplicates only the final check and skips
upstream guards; replace that expression with a call to the app-level helper
AppDelegate.canMoveSurfaceToNewWorkspace(...) passing the panel identifier
(panelId) and the relevant workspace/surface context so the same locatability,
workspace validity, and panel-existence checks are applied consistently (i.e.,
stop using workspace.panels.count > 1 and call
AppDelegate.canMoveSurfaceToNewWorkspace with panelId and the
workspace/surface).
---
Nitpick comments:
In `@Sources/Panels/CmuxWebView.swift`:
- Around line 2175-2181: Before invoking the move closure, re-check the
capability gate to avoid executing a stale action: call
contextMenuCanMoveTabToNewWorkspace?() inside
contextMenuMoveTabToNewWorkspace(_:) and only call
contextMenuMoveTabToNewWorkspace?() if that returns true; otherwise beep/return
as currently done. Reference the closures contextMenuCanMoveTabToNewWorkspace
and contextMenuMoveTabToNewWorkspace when updating the guard logic so the
capability is validated at action time rather than relying on the menu's
previous state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c712f807-dde5-4c6f-9bc6-68936ae27647
📒 Files selected for processing (10)
CLI/cmux.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/GhosttyTerminalView.swiftSources/Panels/BrowserPanel.swiftSources/Panels/CmuxWebView.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/WindowAndDragTests.swift
248df2c to
a4af727
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Sources/ContentView.swift (1)
12210-12210: Route both UTTypes through a single delegate to avoid shadowing.The chained
.onDropmodifiers at lines 12200 and 12210 handleSidebarTabDragPayloadandBonsplitTabDragPayloadseparately. In SwiftUI on macOS, the second modifier overrides the first, leaving onlyBonsplitTabDragPayloaddrops active. Combine both UTTypes into a single.onDropcall with a unified delegate, or restructure to separate this into two independent views if the semantics require it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` at line 12210, The two chained .onDrop modifiers shadow each other so BonsplitTabDragPayload wins and SidebarTabDragPayload drops are ignored; fix by combining both UTType arrays into a single .onDrop call and use one delegate (or a new unified delegate) that handles both payload types. Update the view that currently calls .onDrop(of: SidebarTabDragPayload.dropContentTypes, delegate: SidebarTabNewWorkspaceDropDelegate(...)) and .onDrop(of: BonsplitTabDragPayload.dropContentTypes, delegate: SidebarBonsplitTabNewWorkspaceDropDelegate(tabManager:selectedTabIds:lastSidebarSelectionIndex:)) to instead call one .onDrop(of: combinedTypes, delegate: UnifiedSidebarDropDelegate(tabManager:selectedTabIds:lastSidebarSelectionIndex:)) where UnifiedSidebarDropDelegate inspects the incoming UTType and routes to the existing handling logic for SidebarTabDragPayload and BonsplitTabDragPayload.Sources/TerminalController.swift (1)
3324-3327:v2TabRefworks, but string replacement is a bit brittle.Consider emitting tab refs directly from a canonical helper (if available) to avoid coupling to
"surface:"text shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 3324 - 3327, The v2TabRef function currently builds a tab reference by replacing "surface:" in the string returned from v2EnsureHandleRef, which is brittle; change it to produce tab refs directly by calling a canonical helper instead of string replacement — either call v2EnsureHandleRef(kind: .tab, uuid: uuid) if that variant exists or add a small helper (e.g., v2HandleRef(kind: .tab, uuid:)) that returns the correct "tab:" handle and use that in v2TabRef (keep the same nil-check behavior and return NSNull() when uuid is nil).Sources/AppDelegate+MoveTabToNewWorkspace.swift (1)
14-20: Extract shared source-resolution logic to avoid guard drift.
canMoveSurfaceToNewWorkspace(...)andmoveSurfaceToNewWorkspace(...)repeat the same locate/workspace/panel validation pattern. A shared helper would keep eligibility checks and execution preconditions aligned.Also applies to: 57-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate`+MoveTabToNewWorkspace.swift around lines 14 - 20, Extract the repeated locate/workspace/panel validation into a single helper (e.g., resolveSourceWorkspace(forPanelId:)) that returns an optional tuple containing the located surface, its sourceWorkspace (the tab matching surface.workspaceId), and the panel (or nil if not found); then replace the guard blocks in canMoveSurfaceToNewWorkspace(panelId:) and moveSurfaceToNewWorkspace(panelId:) to call this helper and unwrap the tuple, using its values for the subsequent logic (keep the original return false/early-exit behavior when the helper returns nil and preserve the final check that the sourceWorkspace.panels.count > 1 in canMoveSurfaceToNewWorkspace).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/AppDelegateMoveTabToNewWorkspaceTests.swift`:
- Around line 29-39: The tests currently call window.orderOut(nil) but never
unregister the registered main-window context, causing leaked mainWindowContexts
across tests; after calling window.orderOut(nil) (or in the existing defer),
call the DEBUG helper unregisterMainWindowContextForTesting(windowId:) with the
same windowId used in registerMainWindow(...) to explicitly remove the context
and clear any active pointers; ensure both tests (the one around line 29 and the
one around lines 73-83) add this teardown so the
TabManager/windowId/registerMainWindow state is fully cleaned up between tests.
In `@Sources/AppDelegate`+MoveTabToNewWorkspace.swift:
- Around line 49-55: The socket handler currently passes the same boolean for
both in-app focus and window activation, causing socket/CLI requests to raise
the app; in the TerminalController+MoveTabToNewWorkspace call to
app.moveSurfaceToNewWorkspace (the invocation that uses v2FocusAllowed(...) and
v2Bool(params, "focus")), stop coupling these by leaving the in-app focus
argument as-is (focus) but always pass focusWindow: false for socket routes so
the window is not activated by default; update the call site that currently uses
focusWindow: focus to use focusWindow: false.
In `@Sources/TerminalController`+MoveTabToNewWorkspace.swift:
- Around line 22-29: The code currently defaults the computed focus to true
which can steal focus; change the default to false by updating the focus
assignment so v2FocusAllowed receives (v2Bool(params, "focus") ?? false) instead
of ... ?? true, and keep passing that focus into app.moveSurfaceToNewWorkspace
(both focus and focusWindow) so CLI/socket commands won't steal focus unless
explicitly requested.
---
Nitpick comments:
In `@Sources/AppDelegate`+MoveTabToNewWorkspace.swift:
- Around line 14-20: Extract the repeated locate/workspace/panel validation into
a single helper (e.g., resolveSourceWorkspace(forPanelId:)) that returns an
optional tuple containing the located surface, its sourceWorkspace (the tab
matching surface.workspaceId), and the panel (or nil if not found); then replace
the guard blocks in canMoveSurfaceToNewWorkspace(panelId:) and
moveSurfaceToNewWorkspace(panelId:) to call this helper and unwrap the tuple,
using its values for the subsequent logic (keep the original return
false/early-exit behavior when the helper returns nil and preserve the final
check that the sourceWorkspace.panels.count > 1 in
canMoveSurfaceToNewWorkspace).
In `@Sources/ContentView.swift`:
- Line 12210: The two chained .onDrop modifiers shadow each other so
BonsplitTabDragPayload wins and SidebarTabDragPayload drops are ignored; fix by
combining both UTType arrays into a single .onDrop call and use one delegate (or
a new unified delegate) that handles both payload types. Update the view that
currently calls .onDrop(of: SidebarTabDragPayload.dropContentTypes, delegate:
SidebarTabNewWorkspaceDropDelegate(...)) and .onDrop(of:
BonsplitTabDragPayload.dropContentTypes, delegate:
SidebarBonsplitTabNewWorkspaceDropDelegate(tabManager:selectedTabIds:lastSidebarSelectionIndex:))
to instead call one .onDrop(of: combinedTypes, delegate:
UnifiedSidebarDropDelegate(tabManager:selectedTabIds:lastSidebarSelectionIndex:))
where UnifiedSidebarDropDelegate inspects the incoming UTType and routes to the
existing handling logic for SidebarTabDragPayload and BonsplitTabDragPayload.
In `@Sources/TerminalController.swift`:
- Around line 3324-3327: The v2TabRef function currently builds a tab reference
by replacing "surface:" in the string returned from v2EnsureHandleRef, which is
brittle; change it to produce tab refs directly by calling a canonical helper
instead of string replacement — either call v2EnsureHandleRef(kind: .tab, uuid:
uuid) if that variant exists or add a small helper (e.g., v2HandleRef(kind:
.tab, uuid:)) that returns the correct "tab:" handle and use that in v2TabRef
(keep the same nil-check behavior and return NSNull() when uuid is nil).
🪄 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: 822680a4-ed2d-42d6-91d1-7e81817195f4
📒 Files selected for processing (17)
CLI/CMUXCLI+MoveTabToNewWorkspace.swiftCLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/AppDelegate+MoveTabToNewWorkspace.swiftSources/ContentView+MoveTabToNewWorkspace.swiftSources/ContentView.swiftSources/GhosttyNSView+MoveTabToNewWorkspace.swiftSources/GhosttyTerminalView.swiftSources/Panels/BrowserPanel+MoveTabToNewWorkspace.swiftSources/Panels/BrowserPanel.swiftSources/Panels/CmuxWebView+MoveTabToNewWorkspace.swiftSources/Panels/CmuxWebView.swiftSources/TerminalController+MoveTabToNewWorkspace.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/AppDelegateMoveTabToNewWorkspaceTests.swift
✅ Files skipped from review due to trivial changes (3)
- Resources/Localizable.xcstrings
- Sources/Panels/CmuxWebView.swift
- Sources/GhosttyTerminalView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Panels/BrowserPanel.swift
a4af727 to
0c35782
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
Sources/AppDelegate+MoveTabToNewWorkspace.swift (1)
33-35:⚠️ Potential issue | 🟠 MajorDefault focus parameters are unsafe for non-focus callers.
Both move helpers default
focusandfocusWindowtotrue. Any socket/CLI caller that omits args can unintentionally change in-app focus and activate windows. Prefer safe defaults (false) and require explicit opt-in where UI focus is intended.Proposed change
func moveBonsplitTabToNewWorkspace( tabId: UUID, destinationManager: TabManager? = nil, title: String? = nil, - focus: Bool = true, - focusWindow: Bool = true, + focus: Bool = false, + focusWindow: Bool = false, placementOverride: NewWorkspacePlacement? = nil ) -> SurfaceNewWorkspaceMoveResult? { func moveSurfaceToNewWorkspace( panelId: UUID, destinationManager: TabManager? = nil, title: String? = nil, - focus: Bool = true, - focusWindow: Bool = true, + focus: Bool = false, + focusWindow: Bool = false, placementOverride: NewWorkspacePlacement? = nil ) -> SurfaceNewWorkspaceMoveResult? {Based on learnings: “Socket/CLI commands must not steal macOS app focus … all non-focus commands should preserve current user focus context.”
Also applies to: 53-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate`+MoveTabToNewWorkspace.swift around lines 33 - 35, The default parameters for the move helpers currently default focus and focusWindow to true, which can unintentionally steal macOS focus for non-UI callers; update both occurrences of the helper signature in AppDelegate+MoveTabToNewWorkspace.swift to set focus: false and focusWindow: false (keep placementOverride: NewWorkspacePlacement? = nil) and ensure callers that expect UI focus explicitly pass focus: true and/or focusWindow: true to opt in.Sources/TerminalController+MoveTabToNewWorkspace.swift (1)
22-29:⚠️ Potential issue | 🟠 MajorDefault
focustofalsefor this socket/CLI action.Line 22 currently defaults missing
focustotrue, sotab-actioncan steal in-app focus without explicit focus intent.Proposed fix
- let focus = v2FocusAllowed(requested: v2Bool(params, "focus") ?? true) + let focus = v2FocusAllowed(requested: v2Bool(params, "focus") ?? false)Based on learnings: Socket/CLI commands must not steal macOS app focus unless focus is explicitly requested.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController`+MoveTabToNewWorkspace.swift around lines 22 - 29, The code defaults missing focus to true via v2Bool(params, "focus") ?? true and then passes that through v2FocusAllowed into the focus and focusWindow args of app.moveSurfaceToNewWorkspace; change the default to false so socket/CLI actions don't steal app focus by using v2Bool(params, "focus") ?? false (or otherwise ensure v2FocusAllowed receives false when the param is absent) and keep passing the resulting focus variable into moveSurfaceToNewWorkspace (referencing v2Bool, v2FocusAllowed, params, focus, and moveSurfaceToNewWorkspace).cmuxTests/AppDelegateMoveTabToNewWorkspaceTests.swift (1)
29-29:⚠️ Potential issue | 🟠 MajorUnregister main-window context during test teardown.
Line 29 and Line 73 only hide the window; they do not remove the registered main-window context, which can leak AppDelegate state between tests.
Proposed fix
- defer { window.orderOut(nil) } + defer { + app.unregisterMainWindowContextForTesting(windowId: windowId) + window.orderOut(nil) + } ... - defer { window.orderOut(nil) } + defer { + app.unregisterMainWindowContextForTesting(windowId: windowId) + window.orderOut(nil) + }Based on learnings: The DEBUG-only helper
unregisterMainWindowContextForTesting(windowId:)should mirror production teardown and repoint/clear active pointers to prevent stale test state.Also applies to: 73-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/AppDelegateMoveTabToNewWorkspaceTests.swift` at line 29, The test only calls window.orderOut(nil) which hides the window but leaves the AppDelegate main-window context registered; update the teardown to call the DEBUG helper unregisterMainWindowContextForTesting(windowId:) with the same window identifier used to register it (mirror production teardown) wherever you currently hide the window (references: window.orderOut(nil) lines and the DEBUG helper unregisterMainWindowContextForTesting(windowId:)); ensure this unregister call is placed in the defer/teardown for both occurrences so the AppDelegate’s active pointers are repointed/cleared and no stale state leaks between tests.
🤖 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`:
- Line 4636: For the "tab.action move-to-new-workspace" path, ensure a non-focus
default is sent when the CLI flag is omitted: after calling
applyTabActionFocusOption(focusOpt, to: ¶ms) (or in that handler), if
focusOpt is nil explicitly set params["focus"] = false so the server doesn't
default to true and steal app focus; reference applyTabActionFocusOption, the
params dictionary, and the tab.action move-to-new-workspace handler when making
this change.
---
Duplicate comments:
In `@cmuxTests/AppDelegateMoveTabToNewWorkspaceTests.swift`:
- Line 29: The test only calls window.orderOut(nil) which hides the window but
leaves the AppDelegate main-window context registered; update the teardown to
call the DEBUG helper unregisterMainWindowContextForTesting(windowId:) with the
same window identifier used to register it (mirror production teardown) wherever
you currently hide the window (references: window.orderOut(nil) lines and the
DEBUG helper unregisterMainWindowContextForTesting(windowId:)); ensure this
unregister call is placed in the defer/teardown for both occurrences so the
AppDelegate’s active pointers are repointed/cleared and no stale state leaks
between tests.
In `@Sources/AppDelegate`+MoveTabToNewWorkspace.swift:
- Around line 33-35: The default parameters for the move helpers currently
default focus and focusWindow to true, which can unintentionally steal macOS
focus for non-UI callers; update both occurrences of the helper signature in
AppDelegate+MoveTabToNewWorkspace.swift to set focus: false and focusWindow:
false (keep placementOverride: NewWorkspacePlacement? = nil) and ensure callers
that expect UI focus explicitly pass focus: true and/or focusWindow: true to opt
in.
In `@Sources/TerminalController`+MoveTabToNewWorkspace.swift:
- Around line 22-29: The code defaults missing focus to true via v2Bool(params,
"focus") ?? true and then passes that through v2FocusAllowed into the focus and
focusWindow args of app.moveSurfaceToNewWorkspace; change the default to false
so socket/CLI actions don't steal app focus by using v2Bool(params, "focus") ??
false (or otherwise ensure v2FocusAllowed receives false when the param is
absent) and keep passing the resulting focus variable into
moveSurfaceToNewWorkspace (referencing v2Bool, v2FocusAllowed, params, focus,
and moveSurfaceToNewWorkspace).
🪄 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: f3b45827-0170-493b-beed-62dc3174f8c9
📒 Files selected for processing (18)
CLI/CMUXCLI+MoveTabToNewWorkspace.swiftCLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/AppDelegate+MoveTabToNewWorkspace.swiftSources/ContentView+MoveTabToNewWorkspace.swiftSources/ContentView.swiftSources/GhosttyNSView+MoveTabToNewWorkspace.swiftSources/GhosttyTerminalView.swiftSources/Panels/BrowserPanel+MoveTabToNewWorkspace.swiftSources/Panels/BrowserPanel.swiftSources/Panels/CmuxWebView+MoveTabToNewWorkspace.swiftSources/Panels/CmuxWebView.swiftSources/TerminalController+MoveTabToNewWorkspace.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/AppDelegateMoveTabToNewWorkspaceTests.swiftvendor/bonsplit
✅ Files skipped from review due to trivial changes (5)
- vendor/bonsplit
- Sources/Panels/BrowserPanel.swift
- Resources/Localizable.xcstrings
- Sources/Panels/CmuxWebView+MoveTabToNewWorkspace.swift
- Sources/GhosttyTerminalView.swift
🚧 Files skipped from review as they are similar to previous changes (7)
- Sources/Panels/BrowserPanel+MoveTabToNewWorkspace.swift
- CLI/CMUXCLI+MoveTabToNewWorkspace.swift
- Sources/ContentView+MoveTabToNewWorkspace.swift
- Sources/Panels/CmuxWebView.swift
- Sources/Workspace.swift
- GhosttyTabs.xcodeproj/project.pbxproj
- Sources/TerminalController.swift
| params["url"] = urlOpt.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| } | ||
|
|
||
| try applyTabActionFocusOption(focusOpt, to: ¶ms) |
There was a problem hiding this comment.
Set a non-focus default when --focus is omitted for move-to-new-workspace.
Line 4636 only applies focus when provided. For tab.action move-to-new-workspace, this means no focus key is sent, and the server path defaults to true, which can steal in-app focus unexpectedly.
Suggested fix
- try applyTabActionFocusOption(focusOpt, to: ¶ms)
+ if action == "move-to-new-workspace", focusOpt == nil {
+ params["focus"] = false
+ } else {
+ try applyTabActionFocusOption(focusOpt, to: ¶ms)
+ }Based on learnings: "Socket/CLI commands must not steal macOS app focus ... only explicit focus-intent commands may mutate in-app focus/selection ... all non-focus commands should preserve current user focus context."
📝 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.
| try applyTabActionFocusOption(focusOpt, to: ¶ms) | |
| if action == "move-to-new-workspace", focusOpt == nil { | |
| params["focus"] = false | |
| } else { | |
| try applyTabActionFocusOption(focusOpt, to: ¶ms) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` at line 4636, For the "tab.action move-to-new-workspace"
path, ensure a non-focus default is sent when the CLI flag is omitted: after
calling applyTabActionFocusOption(focusOpt, to: ¶ms) (or in that handler),
if focusOpt is nil explicitly set params["focus"] = false so the server doesn't
default to true and steal app focus; reference applyTabActionFocusOption, the
params dictionary, and the tab.action move-to-new-workspace handler when making
this change.
…rkspace # Conflicts: # GhosttyTabs.xcodeproj/project.pbxproj # Sources/TerminalController.swift # vendor/bonsplit
…rkspace # Conflicts: # GhosttyTabs.xcodeproj/project.pbxproj
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 `@Sources/Workspace.swift`:
- Around line 13645-13646: The .moveToNewWorkspace branch currently calls
AppDelegate.shared?.moveBonsplitTabToNewWorkspace(...) directly and thus
bypasses the shared helper that handles failures and alerts; change this branch
to call the common mover (moveBonsplitTab(...)) with the appropriate parameters
so the shared logic runs and showMoveTabFailureAlert() will be invoked on
rejected moves (ensure you pass tab.id.uuid, focus: true, focusWindow: false or
the equivalent arguments expected by moveBonsplitTab to preserve behavior).
🪄 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: a4912754-9731-4fed-9f7e-38d21814093b
📒 Files selected for processing (11)
CLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/ContentView+MoveTabToNewWorkspace.swiftSources/ContentView.swiftSources/GhosttyTerminalView.swiftSources/Panels/BrowserPanel.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/WorkspaceUnitTests.swiftvendor/bonsplit
✅ Files skipped from review due to trivial changes (4)
- vendor/bonsplit
- Resources/Localizable.xcstrings
- GhosttyTabs.xcodeproj/project.pbxproj
- Sources/TerminalController.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- Sources/Panels/BrowserPanel.swift
- Sources/GhosttyTerminalView.swift
- CLI/cmux.swift
| case .moveToNewWorkspace: | ||
| _ = AppDelegate.shared?.moveBonsplitTabToNewWorkspace(tabId: tab.id.uuid, focus: true, focusWindow: false) |
There was a problem hiding this comment.
Route .moveToNewWorkspace through the shared move helper.
This branch bypasses moveBonsplitTab(...), so failures here skip showMoveTabFailureAlert(). That makes rejected moves, including the only-tab case, silently no-op on this UI path.
Suggested fix
- case .moveToNewWorkspace:
- _ = AppDelegate.shared?.moveBonsplitTabToNewWorkspace(tabId: tab.id.uuid, focus: true, focusWindow: false)
+ case .moveToNewWorkspace:
+ _ = moveBonsplitTab(tab.id, toMoveDestination: Self.bonsplitMoveNewWorkspaceDestinationId)📝 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.
| case .moveToNewWorkspace: | |
| _ = AppDelegate.shared?.moveBonsplitTabToNewWorkspace(tabId: tab.id.uuid, focus: true, focusWindow: false) | |
| case .moveToNewWorkspace: | |
| _ = moveBonsplitTab(tab.id, toMoveDestination: Self.bonsplitMoveNewWorkspaceDestinationId) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 13645 - 13646, The .moveToNewWorkspace
branch currently calls AppDelegate.shared?.moveBonsplitTabToNewWorkspace(...)
directly and thus bypasses the shared helper that handles failures and alerts;
change this branch to call the common mover (moveBonsplitTab(...)) with the
appropriate parameters so the shared logic runs and showMoveTabFailureAlert()
will be invoked on rejected moves (ensure you pass tab.id.uuid, focus: true,
focusWindow: false or the equivalent arguments expected by moveBonsplitTab to
preserve behavior).
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 `@CLI/CMUXCLI`+MoveTabToNewWorkspace.swift:
- Line 47: The usage synopsis string ("Usage: cmux move-tab-to-new-workspace
[--tab <id|ref>] [--surface <id|ref>] [--workspace <id|ref>] [--title <text>]
[--focus <true|false>]") is inconsistent with the flag docs which allow
"<id|ref|index>"; update the usage line to include "index" for the relevant
flags (--tab, --surface, --workspace) so it reads "<id|ref|index>" to match the
documented flag parsing in this file (CLI/CMUXCLI+MoveTabToNewWorkspace.swift)
and ensure any help/usage generator references the same token 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6d80599-d587-4bf2-bf27-2dd77e685af0
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (6)
CLI/CMUXCLI+MoveTabToNewWorkspace.swiftCLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojSources/TerminalController+MoveTabToNewWorkspace.swiftcmuxTests/AppDelegateMoveTabToNewWorkspaceTests.swiftcmuxTests/WorkspaceAdjacentPaneMoveTests.swift
✅ Files skipped from review due to trivial changes (1)
- GhosttyTabs.xcodeproj/project.pbxproj
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/AppDelegateMoveTabToNewWorkspaceTests.swift
This branch was successfully deployed
1 participant
Adds a shared move-tab-to-new-workspace path and wires it into drag/drop, context menus, command palette, socket actions, and CLI.
Follow-up fixes:
Verification:
Bonsplit submodule branch: https://github.com/manaflow-ai/bonsplit/tree/feat-detach-tab-to-workspace-menu
Summary by CodeRabbit
move-tab-to-new-workspaceanddetach-tabcommands.--focusflag support for tab action commands.