Repository navigation
Move active surfaces between panes with automatic directional splits - #8764
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds configurable actions for moving the active surface to previous, next, or directional panes. The implementation centralizes pane transfer logic, exposes keyboard shortcuts through settings, command palette, and View menu, updates schemas and localization, and adds movement and routing coverage. ChangesSurface movement
Runtime and deployment validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AppDelegate
participant Workspace
participant CommandPalette
User->>AppDelegate: Trigger configured movement shortcut
AppDelegate->>Workspace: moveFocusedSurface(to: movement)
Workspace->>Workspace: Resolve destination and move surface
User->>CommandPalette: Select movement command
CommandPalette->>AppDelegate: performSurfacePaneMovement(...)
AppDelegate->>Workspace: moveFocusedSurface(to: movement)
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 errors, 3 warnings)
✅ Passed checks (17 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR adds six configurable actions for moving the active surface between panes: previous/next (spatial wrapping) and left/right/above/below (directional). All six actions route through a shared
Confidence Score: 5/5Safe to merge. The change is well-scoped: new movement actions flow through the existing same-workspace surface transfer path, all guards (Canvas, remote-tmux, Dock focus, key-repeat edge) are exercised by the 30 new tests, and zoom rollback on failure has been explicitly verified. The core movement logic, zoom snapshot/restore, directional-split creation, insertion ordering, and identity preservation for terminal/browser/WebView instances are all covered by focused behavior tests that passed on the merged branch head. Localization is complete in both supported locales across Swift xcstrings and web data. No new global state, no test/debug seams in production source, and no actor isolation regressions are introduced. Files Needing Attention: No files require special attention. The Important Files Changed
Sequence DiagramsequenceDiagram
participant KB as Keyboard/Menu/Palette
participant AD as AppDelegate
participant WS as Workspace
participant BS as BonsplitController
KB->>AD: handleAdjacentNavigationShortcut / performSurfacePaneMovement
AD->>AD: focusedDockStoreForShortcut (Dock gate)
AD->>WS: moveFocusedSurface(to: movement, allowMissingDestinationSplit:)
WS->>WS: "guard layoutMode != .canvas && !isRemoteTmuxMirror"
WS->>BS: adjacentPane / spatiallyOrderedPaneIds (resolve destination)
alt Existing destination pane found
WS->>WS: snapshot zoomedPaneId then clearSplitZoom()
WS->>WS: insertionIndexAfterSelectedSurface(in: destinationPaneId)
WS->>BS: moveSurface(panelId:toPane:atIndex:focus:)
BS-->>WS: didMove Bool
alt didMove is false
WS->>BS: togglePaneZoom restore zoom
end
else No adjacent pane AND allowMissingDestinationSplit
WS->>WS: snapshot zoomedPaneId then clearSplitZoom()
WS->>BS: splitPane(sourcePaneId, orientation, movingTab, insertFirst)
BS-->>WS: newPaneId atomic moves tab collapses empty source
WS->>BS: focusPane(newPaneId) and selectTab(tabId)
WS->>WS: focusPanel(panelId)
else No destination AND not allowMissingDestinationSplit
WS-->>AD: false
AD->>AD: NSSound.beep()
end
WS-->>AD: Bool success
Reviews (11): Last reviewed commit: "Merge branch 'main' of https://github.co..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Author
|
CI note: the first manual full-CI run was blocked by the repository-wide optional-Iroh-limiter test regression tracked in #8761. Commit a0d2aac is the exact reviewed, test-only #8761 fix (original author preserved) so this PR’s macOS build/test matrix can run. Once #8761 lands on main, this file will drop out of the PR content diff. |
![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+SurfaceNavigationCommands.swift (1)
65-70: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the owning workspace window for the Dock-focus gate.
In
registerSurfaceNavigationCommandHandlers(...)atSources/ContentView+SurfaceNavigationCommands.swift:69,observedWindowcan belong to another panel, andNSApp.keyWindow ?? NSApp.mainWindowcan be the command palette or another transient window. That can makeperformSurfacePaneMovement(...)route the movement to the wrong workspace context and bypass Dock-focus protection. Pass the command’s owning workspace window explicitly, or fail closed when it is unavailable.🤖 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/ContentView`+SurfaceNavigationCommands.swift around lines 65 - 70, Update registerSurfaceNavigationCommandHandlers and its performSurfacePaneMovement call to use the command’s owning workspace window for preferredWindow, rather than observedWindow or global key/main windows; if no owning workspace window is available, fail closed instead of routing the movement through a transient or unrelated window.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/ContentView`+SurfaceNavigationCommands.swift:
- Around line 65-70: Update registerSurfaceNavigationCommandHandlers and its
performSurfacePaneMovement call to use the command’s owning workspace window for
preferredWindow, rather than observedWindow or global key/main windows; if no
owning workspace window is available, fail closed instead of routing the
movement through a transient or unrelated window.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8cf04e15-61a5-431d-ba8e-99eb55dcd980
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/ContentView+SurfaceNavigationCommands.swiftSources/Workspace+SurfaceNavigation.swiftcmuxTests/WorkspaceAdjacentPaneMoveTests.swift
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Contributor
Author
|
Review follow-up: f17accd addresses the Command Palette window-routing finding. Surface-movement handlers now use only the owning ContentView window and fail closed (with the existing beep) when that window is unavailable; they no longer fall back to an ambient key/main window. The shared AppDelegate Dock-focus gate remains the single mutation guard. |
Co-authored-by: Kyler <leejianhui260901@gmail.com>
Co-authored-by: Kyler <leejianhui260901@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/SurfacePaneMovement.swift (1)
88-96: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLocalize command-palette keywords.
These search aliases are English-only, so Japanese users cannot discover these commands through equivalent directional terms. Source them from localized keys/catalog entries alongside the titles.
As per coding guidelines, user-facing text and data must use locale-specific sources and update supported locales.
🤖 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/SurfacePaneMovement.swift` around lines 88 - 96, Update the keywords property on SurfacePaneMovement to source each command-palette alias from the existing localized keys or catalog entries used by the command titles, including equivalent directional terms for supported locales. Extend the locale resources as needed while preserving the current keyword semantics for previous, next, left, right, up, and down.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Workspace`+SurfaceNavigation.swift:
- Around line 59-60: Replace the split-move calls in
Sources/Workspace+SurfaceNavigation.swift lines 59-60 with a single
lifecycle-owned Bonsplit/host-reparent completion that reattaches the terminal,
reconciles geometry, and refreshes once without delayed asyncAfter retries. In
Sources/Workspace.swift line 10505, do not widen
scheduleMovedTerminalRefresh(panelId:); expose or reuse a lifecycle completion
API for this path instead.
---
Outside diff comments:
In `@Sources/SurfacePaneMovement.swift`:
- Around line 88-96: Update the keywords property on SurfacePaneMovement to
source each command-palette alias from the existing localized keys or catalog
entries used by the command titles, including equivalent directional terms for
supported locales. Extend the locale resources as needed while preserving the
current keyword semantics for previous, next, left, right, up, and down.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b391b728-f053-4e4a-baf9-25eff33c47a1
📒 Files selected for processing (4)
Sources/SurfacePaneMovement.swiftSources/Workspace+SurfaceNavigation.swiftSources/Workspace.swiftcmuxTests/WorkspaceAdjacentPaneMoveTests.swift
…eds it Building main at 4dc00ae fails with a single error: Sources/DockSplitStore+RestoredAgentLifecycle.swift:13:62: error: cannot find type 'PanelShellActivityState' in scope That file arrived with #8690 and declares updatePanelShellActivityState(panelId:state:), whose state parameter is PanelShellActivityState. That type is a public enum in the CmuxWorkspaces package, and Swift imports are per-file, so the app target linking the package is not enough on its own. Every other file in Sources that names the type imports CmuxWorkspaces; this one imports only Foundation. ci.yml is dispatch-only, so no PR build runs and nothing caught it.
Second and third errors from the same build, after the missing import: Sources/DockSplitStore+SessionSnapshot.swift:44:43: error: generic parameter 'Key' could not be inferred Sources/DockSplitStore+SessionSnapshot.swift:44:43: error: generic parameter 'Value' could not be inferred Sources/DockSplitStore+SessionSnapshot.swift:332:47: error: generic parameter 'U' could not be inferred Both are the same shape: a multi-statement closure whose bail-out is a bare `return nil`, handed to something generic. At line 44 that is Dictionary(uniqueKeysWithValues:), which has to solve Key and Value; at line 332 it is Optional.flatMap, which has to solve U. A bare `return nil` carries no type, so the closure result and the generic parameters each depend on the other and the solver gives up. Naming the return types breaks the cycle. The types are the ones already required by the surrounding code: SessionSplitContainerSnapshot declares sourceWorkspaceIdsByPanelId as [UUID: UUID]?, DetachedSurfaceTransfer's sessionRestoreWorkspaceId is a UUID, and observation is a RestorableAgentSessionIndex.Entry?. Behaviour is unchanged.
Fourth error from the same build: Sources/ControlSurfaceResumeTarget.swift:282:40: error: reference to member 'auto' cannot be resolved without a contextual type Sources/ControlSurfaceResumeTarget.swift:283:41: error: reference to member 'prompt' cannot be resolved without a contextual type Sources/ControlSurfaceResumeTarget.swift:284:19: error: reference to member 'manual' cannot be resolved without a contextual type surfacePromptForResumeApproval builds an NSAlert over several statements and then ends with a bare `switch alert.runModal()` whose cases are `.auto`, `.prompt` and `.manual`. Swift gives a function body an implicit return only when the body is a single expression, so in a multi-statement body that switch is an expression statement with nothing to type it, and the leading-dot members have no SurfaceResumeApprovalPolicy to resolve against. Returning it supplies the contextual type. Every other trailing switch of this shape under Sources is the single-expression body of a computed property or a one-statement function, which is why this is the only one that fails.
…ce-between-panes # Conflicts: # cmux.xcodeproj/project.pbxproj
austinywang
changed the title
added 4 commits
July 24, 2026 03:37
…rface-between-panes
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
5 of 6 tasks
…-8752-move-surface-between-panes # Conflicts: # Packages/macOS/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swift # Sources/ControlSurfaceResumeTarget.swift # Sources/DockSplitStore+SessionSnapshot.swift # cmux.xcodeproj/project.pbxproj
…-8752-move-surface-between-panes # Conflicts: # Packages/macOS/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swift # Sources/KeyboardShortcutSettings.swift # Sources/cmuxApp.swift # cmux.xcodeproj/project.pbxproj # cmuxTests/DockShortcutRoutingTests.swift
…-8752-move-surface-between-panes
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cmux.jsonBehavior
SurfacePaneMovementis the shared six-action model.Workspaceresolves spatial wrapping or Bonsplit directional adjacency, computes the insertion point, and delegates to the existing same-workspace surface transfer machinery used by drag-and-drop. Keyboard routing, Command Palette handlers, View-menu items, and tab context actions converge on the same movement path.When a directional destination is missing, cmux creates an equal split in that direction and moves the existing surface instance into it. Keyboard auto-repeat can still move into an existing adjacent pane, but cannot create another split while the key is held at an edge. Successful moves insert after the destination pane's selected surface, focus the moved surface, and allow Bonsplit to collapse an emptied source pane. Failed moves restore the previous layout and split-zoom state.
Canvas and remote-tmux mirror layouts reject the action without mutation. Previous/next is a no-op with one pane, and Dock keyboard focus cannot mutate a background workspace.
The existing within-pane actions are labeled “Reorder Surface Left/Right” so they remain distinct from cross-pane movement.
Default shortcuts
The directional family adds Shift to the existing Option+Command+Arrow pane-focus bindings. Previous/next adds Control to the existing Command+Shift+[/] surface-navigation family. The defaults are collision-free across current cmux and Ghostty bindings, include Command so they do not steal PTY control sequences, and remain rebindable or unbindable.
Coverage
Behavior-level tests cover:
Validation on the merged branch head:
WorkspaceAdjacentPaneMoveTests,DockShortcutRoutingTests, andReorderShortcutActionTests./scripts/reload.sh --tag issue-8752-splitPackage.resolvedpolicy, localization/JSON parsing, locale completeness, andgit diff --checkpassedAt validation time,
origin/mainhad a test-target compile regression. The branch includes the exact reviewed head of upstream repair PR #8857 and a minimal fixture update adding the newly required nil transfer field.Closes #8752