Add surface and workspace reorder shortcuts - #8080
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:
📝 WalkthroughWalkthroughAdds four shortcut actions for moving surfaces and workspaces, shared relative-reordering helpers, menu and command-palette wiring, localized shortcut metadata, schema updates, and tests covering ordering, selection, shortcut consistency, and collisions. ChangesSurface and workspace shortcut reordering
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AppDelegate
participant Workspace
participant TabManager
User->>AppDelegate: Press movement shortcut
AppDelegate->>Workspace: moveSelectedSurface(by: offset)
AppDelegate->>TabManager: moveSelectedWorkspace(by: offset)
Workspace->>Workspace: reorderSurface(panelId:by:)
TabManager->>TabManager: reorderWorkspace(tabId:by:)
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 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 |
Greptile SummaryThis PR adds four customizable keyboard shortcuts — Move Surface Left/Right and Move Workspace Up/Down — and routes all entry points (keyboard, menus, command palette, socket) through shared adjacent-reorder helpers on
Confidence Score: 5/5Safe to merge — well-bounded refactor of duplicated move logic with no observable behavior change for existing callers and solid new test coverage. All three layout/entry-point paths (canvas, split, socket) are covered by tests. Bounds clamping is handled by the existing WorkspaceReorderCoordinator. No new concurrency patterns, no new ambient state, and all user-facing strings carry en/ja translations. No files require special attention. Important Files Changed
Reviews (8): Last reviewed commit: "Add Canvas reorder overflow regression t..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| case "move_up": | ||
| guard let currentIndex = tabManager.tabs.firstIndex(where: { $0.id == workspace.id }) else { | ||
| result = .err(code: "not_found", message: "Workspace not found", data: nil) | ||
| return | ||
| } | ||
| _ = tabManager.reorderWorkspace(tabId: workspace.id, toIndex: max(currentIndex - 1, 0)) | ||
| _ = tabManager.reorderWorkspace(tabId: workspace.id, by: -1) | ||
| finish(["index": v2OrNull(tabManager.tabs.firstIndex(where: { $0.id == workspace.id }))]) | ||
|
|
||
| case "move_down": | ||
| guard let currentIndex = tabManager.tabs.firstIndex(where: { $0.id == workspace.id }) else { | ||
| result = .err(code: "not_found", message: "Workspace not found", data: nil) | ||
| return | ||
| } | ||
| _ = tabManager.reorderWorkspace(tabId: workspace.id, toIndex: min(currentIndex + 1, tabManager.tabs.count - 1)) | ||
| _ = tabManager.reorderWorkspace(tabId: workspace.id, by: 1) | ||
| finish(["index": v2OrNull(tabManager.tabs.firstIndex(where: { $0.id == workspace.id }))]) |
There was a problem hiding this comment.
not_found error path silently removed for move_up/move_down
The old implementation guarded against the workspace being absent from tabManager.tabs and returned result = .err(code: "not_found", …) before calling finish. The refactored code drops that guard entirely: if reorderWorkspace(tabId:by:) returns false because the workspace is not in tabs, execution still falls through to finish(["index": v2OrNull(nil)]), sending a success response with null index instead of an error. Adjacent commands close_above and close_below (a few lines below) still retain their not_found guards, making this an inconsistency in the socket API contract. Any automation client that distinguishes the error vs. success-with-null response will silently get wrong behavior.
There was a problem hiding this comment.
The behavior remains not_found for an unknown workspace. The enclosing v2MainSync closure already resolves workspace with tabManager.tabs.first(where:) and returns not_found before entering this switch. This is one synchronous @mainactor closure with no suspension point or intervening mutation, so the removed per-case lookup was redundant; reorderWorkspace(tabId:by:) receives the same workspace ID that the outer guard just proved is present. I am leaving the shared reorder path unchanged.
— Claude Code
There was a problem hiding this comment.
The outer guard in v2WorkspaceAction already resolves workspace from tabManager.tabs inside the same @mainactor v2MainSync closure before entering the switch. Since no suspension occurs between that guard and move_up/move_down, the workspace cannot disappear concurrently; the removed inner guard was redundant. A genuinely missing workspace still returns not_found from the outer guard, while reorderWorkspace returning false here only represents an edge/tier no-op and preserves the existing success contract.
— Claude Code
There was a problem hiding this comment.
You're right — the @MainActor guarantee with no suspension between the outer resolution and the switch makes the inner guards provably redundant. The not_found contract is fully preserved by the outer guard, and reorderWorkspace returning false correctly maps to an edge/tier no-op rather than a missing-workspace error. I'll withdraw the concern.
![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+RightSidebarCommandPalette.swift (1)
919-924: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the new shortcut labels to every supported locale.
Resources/Localizable.xcstringsalready includesar, bs, da, de, es, fr, it, km, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, and zh-Hant, butshortcut.moveSurfaceLeft.label,shortcut.moveSurfaceRight.label,shortcut.moveWorkspaceDown.label, andshortcut.moveWorkspaceUp.labelonly haveenandja.🤖 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`+RightSidebarCommandPalette.swift around lines 919 - 924, Update the localized entries for shortcut.moveSurfaceLeft.label, shortcut.moveSurfaceRight.label, shortcut.moveWorkspaceDown.label, and shortcut.moveWorkspaceUp.label in Localizable.xcstrings so every supported locale has a translation, including ar, bs, da, de, es, fr, it, km, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, and zh-Hant, while preserving the existing en and ja values.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`+RightSidebarCommandPalette.swift:
- Around line 919-924: Update the localized entries for
shortcut.moveSurfaceLeft.label, shortcut.moveSurfaceRight.label,
shortcut.moveWorkspaceDown.label, and shortcut.moveWorkspaceUp.label in
Localizable.xcstrings so every supported locale has a translation, including ar,
bs, da, de, es, fr, it, km, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, and
zh-Hant, while preserving the existing en and ja values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 14184649-a1af-46e0-a909-0e3a1cccc703
📒 Files selected for processing (16)
Packages/macOS/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction+Defaults.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView+RightSidebarCommandPalette.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/TabManager+AdjacentWorkspaceReordering.swiftSources/TerminalController.swiftSources/Workspace+SurfaceNavigation.swiftSources/cmuxApp.swiftcmux.xcodeproj/project.pbxprojcmuxTests/ReorderShortcutActionTests.swiftskills/cmux-settings/references/shortcut-actions.mdweb/data/cmux-shortcuts.tsweb/data/cmux.schema.json
…ortcuts # Conflicts: # cmux.xcodeproj/project.pbxproj
…ortcuts # Conflicts: # Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohRelayCredentialCoordinatorTests.swift
reorderPanel(_:by:) computed currentIndex + offset before clamping, so offset = Int.max / Int.min (exactly what reorderPanelClampsExtremeOffsetsWithoutOverflow drives) trapped with a Swift runtime arithmetic-overflow crash and killed the whole CmuxCanvasUI test process before it printed its summary. Saturate via addingReportingOverflow, then clamp as before. This landed broken in manaflow-ai#8080, which merged while the repo's CI workflows were manually disabled, and has broken swift-package-tests on main since.
* Add workspace and surface reorder shortcuts * Split shortcut helpers out of oversized files * Add Canvas surface reorder regression test * Route surface reordering through Canvas layout * Add no-op Canvas reorder regression test * Skip Canvas refresh for clamped surface moves * Fix relay test determinism guard * Add Canvas reorder overflow regression test --------- Co-authored-by: cmux reload-cloud <cmux-reload-cloud@users.noreply.github.com> (cherry picked from commit 1ff14b8)
reorderPanel(_:by:) computed currentIndex + offset before clamping, so offset = Int.max / Int.min (exactly what reorderPanelClampsExtremeOffsetsWithoutOverflow drives) trapped with a Swift runtime arithmetic-overflow crash and killed the whole CmuxCanvasUI test process before it printed its summary. Saturate via addingReportingOverflow, then clamp as before. This landed broken in manaflow-ai#8080, which merged while the repo's CI workflows were manually disabled, and has broken swift-package-tests on main since. (cherry picked from commit 6a00674)
This branch was previously deployed
1 participant
Summary
Validation
Issue: #8041
Closes #8041
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds customizable shortcuts to reorder the selected surface and workspace across layouts and UIs. Preserves selection, clamps at edges (including extreme offsets), respects pinned tiers, and routes Canvas reorders through
CanvasModelwith no viewport refresh on clamped moves; implements #8041.New Features
palette.moveWorkspaceUp/palette.moveWorkspaceDownmapped.Refactors
handleAdjacentNavigationShortcut; addedTabManagerhelpers and action-visibility filter.CanvasModel.reorderPanel; clamps extreme offsets without overflow, and refreshes the viewport only when order changes.Written for commit 028aa00. Summary will update on new commits.
Summary by CodeRabbit