Repository navigation
Fix Canvas keyboard shortcut routing - #6704
Merged
Merged
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 ChangesCanvas layout shortcut context and routing
Sequence Diagram(s)sequenceDiagram
participant handleCustomShortcut as AppDelegate.handleCustomShortcut
participant performToggleSplitZoom as performToggleSplitZoomShortcut
participant performSplit as performSplitShortcut
participant performBrowserSplit as performBrowserSplitShortcut
participant CanvasActionExecutor
participant Workspace
participant TabManager
handleCustomShortcut->>performToggleSplitZoom: .toggleSplitZoom
alt canvas layout
performToggleSplitZoom->>CanvasActionExecutor: perform(.toggleOverview)
else non-canvas
performToggleSplitZoom->>TabManager: toggleFocusedSplitZoom()
end
handleCustomShortcut->>performSplit: direction
alt canvas layout
performSplit->>Workspace: openNewCanvasPane(type: .terminal, direction: canvasDirection)
Workspace->>CanvasModel: syncPanes(preferredDirection: direction)
CanvasModel->>CanvasPlacer: frameForNewPane(preferredDirection: direction)
else non-canvas
performSplit->>TabManager: createTerminalSplit
end
handleCustomShortcut->>performBrowserSplit: direction
alt canvas layout
performBrowserSplit->>Workspace: openNewCanvasPane(type: .browser, direction: canvasDirection)
else non-canvas
performBrowserSplit->>TabManager: createBrowserSplit
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/KeyboardShortcutContext.swift`:
- Line 38: The `canvasLayout` case in the KeyboardShortcutContext needs to be
marked as overlapping with focus contexts. Update the `overlaps(_:)` method to
return true when comparing `canvasLayout` with focus-related context cases (such
as browser, markdown, sidebar, or other focus-scoped contexts), since canvas
layout is orthogonal to focus and can coexist with these other contexts. This
ensures that conflict detection properly acknowledges both when-clauses can
match simultaneously and routing order determines the winner.
🪄 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: b7b22c7d-4c97-4983-ab90-50e8de97e148
📒 Files selected for processing (13)
Packages/macOS/CmuxSettings/README.mdPackages/macOS/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Values/ShortcutContextKnownKey.swiftSources/AppDelegate.swiftSources/ContentView.swiftSources/KeyboardShortcutContext.swiftcmuxTests/AppDelegateEqualizeSplitsShortcutTests.swiftcmuxTests/KeyboardShortcutContextTests.swiftskills/cmux-settings/references/shortcut-actions.mdweb/app/[locale]/docs/configuration/page.tsxweb/data/cmux.schema.jsonweb/messages/en.jsonweb/messages/ja.json
| case browserPanel | ||
| case markdownPanel | ||
| case rightSidebarFocus | ||
| case canvasLayout |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Mark canvasLayout as overlapping focus contexts.
workspaceCanvasLayout is orthogonal to focus: in a canvas workspace, browser, markdown, sidebar, or non-browser contexts can also be true. Leaving overlaps(_:) unchanged lets conflict detection accept the same chord for a canvas action and a focus-scoped action even though both when-clauses can match, so routing order decides the winner.
Proposed fix
func overlaps(_ other: ShortcutContext) -> Bool {
if self == .application || other == .application {
return true
}
if self == other {
return true
}
+ if self == .canvasLayout || other == .canvasLayout {
+ return true
+ }
// A focused markdown viewer also satisfies `.nonBrowserPanel`, so the
// two contexts can be active at the same time. Treat them as
// overlapping so shortcut conflict detection rejects a chord bound toAlso applies to: 150-155
🤖 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/KeyboardShortcutContext.swift` at line 38, The `canvasLayout` case in
the KeyboardShortcutContext needs to be marked as overlapping with focus
contexts. Update the `overlaps(_:)` method to return true when comparing
`canvasLayout` with focus-related context cases (such as browser, markdown,
sidebar, or other focus-scoped contexts), since canvas layout is orthogonal to
focus and can coexist with these other contexts. This ensures that conflict
detection properly acknowledges both when-clauses can match simultaneously and
routing order determines the winner.
Contributor
Greptile SummaryThis PR gates Canvas-only keyboard shortcuts behind the new
Confidence Score: 5/5The routing logic is well-isolated behind the new workspaceCanvasLayout context key, the two implementation layers produce matching when-clauses, and every changed shortcut path has direct regression coverage. The context-gating, routing branches, and command-palette hint path are all consistent. All locale files were updated. The canvasZoomReset rebinding to Cmd+0 is correctly scoped away from browser and markdown panels. Tests cover both negative and positive paths for every new routing decision. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Key Event] --> B[shortcutEventFocusContext]
B --> C{workspaceCanvasLayout?}
C -- true --> D[canvasSurfaceDigitShortcutIsActive?]
D -- true --> E[selectSurface canvas order]
D -- false --> F[rightSidebarModeShortcut?]
F -- yes --> G[Focus sidebar]
F -- no --> H[matchConfiguredShortcut router]
C -- false --> F
H --> I{toggleSplitZoom?}
I -- canvas layout --> J[CanvasActionExecutor toggleOverview]
I -- split layout --> K[toggleFocusedSplitZoom]
H --> L{splitRight / splitDown?}
L -- canvas layout --> M[openNewCanvasPane preferred direction]
L -- split layout --> N[createSplit / createBrowserSplit]
H --> O{equalizeSplits?}
O -- canvas layout --> P[equalizeWidths + equalizeHeights]
O -- split layout --> Q[bonsplit equalize]
H --> R{canvasZoomReset Cmd+0?}
R -- canvas no browser/markdown --> S[CanvasViewport resetZoom]
R -- browser focused --> T[browserZoomReset]
R -- markdown focused --> U[markdownZoomReset]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Key Event] --> B[shortcutEventFocusContext]
B --> C{workspaceCanvasLayout?}
C -- true --> D[canvasSurfaceDigitShortcutIsActive?]
D -- true --> E[selectSurface canvas order]
D -- false --> F[rightSidebarModeShortcut?]
F -- yes --> G[Focus sidebar]
F -- no --> H[matchConfiguredShortcut router]
C -- false --> F
H --> I{toggleSplitZoom?}
I -- canvas layout --> J[CanvasActionExecutor toggleOverview]
I -- split layout --> K[toggleFocusedSplitZoom]
H --> L{splitRight / splitDown?}
L -- canvas layout --> M[openNewCanvasPane preferred direction]
L -- split layout --> N[createSplit / createBrowserSplit]
H --> O{equalizeSplits?}
O -- canvas layout --> P[equalizeWidths + equalizeHeights]
O -- split layout --> Q[bonsplit equalize]
H --> R{canvasZoomReset Cmd+0?}
R -- canvas no browser/markdown --> S[CanvasViewport resetZoom]
R -- browser focused --> T[browserZoomReset]
R -- markdown focused --> U[markdownZoomReset]
Reviews (8): Last reviewed commit: "Keep Canvas zoom test hook private" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }, | ||
| "shortcuts": { | ||
| "when": "Optional per-action context predicates (VS Code-style `when` clauses), keyed by cmux action id. Each value is a boolean expression over context keys combined with !, &&, ||, and parentheses. Boolean keys: sidebarFocus, browserFocus, markdownFocus, terminalFocus, commandPaletteVisible, terminalFindVisible. Typed keys support comparisons: the string sidebarMode (files, find, sessions, feed, or dock) and the integers paneCount and workspaceCount. Comparison operators are ==, !=, =~ (regex), <, <=, >, >=, and `in [a, b]`; an unknown or absent key reads as false. The boolean literals true and false are also accepted; `key == false` is the same as `!key`. The action's shortcut only fires (and only conflicts with other shortcuts) when the clause holds. Examples: { \"selectWorkspaceByNumber\": \"!sidebarFocus\" } selects workspaces with Ctrl+1–9 everywhere except when the right sidebar is focused; { \"selectSurfaceByNumber\": \"sidebarMode == 'find' && paneCount > 1\" } scopes a binding to the Find sidebar when the workspace has multiple panes.", | ||
| "when": "Optional per-action context predicates (VS Code-style `when` clauses), keyed by cmux action id. Each value is a boolean expression over context keys combined with !, &&, ||, and parentheses. Boolean keys: sidebarFocus, browserFocus, markdownFocus, terminalFocus, commandPaletteVisible, terminalFindVisible, workspaceCanvasLayout. Typed keys support comparisons: the string sidebarMode (files, find, sessions, feed, or dock) and the integers paneCount and workspaceCount. Comparison operators are ==, !=, =~ (regex), <, <=, >, >=, and `in [a, b]`; an unknown or absent key reads as false. The boolean literals true and false are also accepted; `key == false` is the same as `!key`. The action's shortcut only fires (and only conflicts with other shortcuts) when the clause holds. Examples: { \"selectWorkspaceByNumber\": \"!sidebarFocus\" } selects workspaces with Ctrl+1–9 everywhere except when the right sidebar is focused; { \"selectSurfaceByNumber\": \"sidebarMode == 'find' && paneCount > 1\" } scopes a binding to the Find sidebar when the workspace has multiple panes.", |
Contributor
There was a problem hiding this comment.
workspaceCanvasLayout in 18 message files
The shortcuts.when description key was updated in en.json and ja.json to include workspaceCanvasLayout in the boolean-keys list, but 18 other locale files (ar, bs, da, de, es, fr, it, km, ko, no, pl, pt-BR, ru, th, tr, uk, zh-CN, zh-TW) still have the old text ending with terminalFindVisible only. Users reading the docs in any of those locales will not see workspaceCanvasLayout listed among the available boolean context keys and will be unable to use it in their shortcuts.when clauses without guessing.
Rule Used: Flag production user-facing text that is not fully... (source)
Collaborator
Author
There was a problem hiding this comment.
Current head db2b10f updates all supported locale message files. Verification: 20 returns 20, and passes.
— Claude Code
Collaborator
Author
There was a problem hiding this comment.
Comment on lines
+89
to
+98
| #if DEBUG | ||
| XCTAssertTrue(appDelegate.debugHandleCustomShortcut(event: event)) | ||
| #else | ||
| XCTFail("debugHandleCustomShortcut is only available in DEBUG") | ||
| #endif | ||
| XCTAssertFalse( | ||
| workspace.bonsplitController.isSplitZoomed, | ||
| "In canvas mode, the split-zoom shortcut should drive canvas overview instead of Bonsplit zoom" | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
testCmdShiftReturnInCanvasModeDoesNotToggleBonsplitSplitZoom asserts that bonsplitController.isSplitZoomed stays false after the shortcut fires, but it does not assert that canvas overview actually toggled. If workspace.canvasModel.viewport is nil at test time (common for a newly created window before layout), CanvasActionExecutor.perform(.toggleOverview) silently returns false, debugHandleCustomShortcut still returns true, and all assertions pass — hiding the regression the test intends to catch. A complementary assertion on the canvas overview state (or XCTAssertTrue(canvasExecutorResult)) is needed to make the positive path observable.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/AppDelegate.swift`:
- Around line 14567-14574: The canvas browser split path using
`openNewCanvasPane(...)` returns immediately without calling
`focusBrowserAddressBar(panelId:)`, while the non-canvas path applies this
function. This creates inconsistent omnibar focus behavior between layouts.
Refactor the canvas path to capture the result from `openNewCanvasPane(...)`
and, when successful, ensure `focusBrowserAddressBar(panelId:)` is called with
the appropriate panel identifier before returning, so both canvas and non-canvas
paths maintain consistent focus behavior.
In `@Sources/Canvas/Workspace`+CanvasLayout.swift:
- Around line 193-203: The selectCanvasTab(at:) function incorrectly applies the
1-indexed shortcut convention (9 = last) directly to its zero-based parameter,
causing index 8 to always select the last tab regardless of actual tab count.
Remove the special case logic that maps index 8 to tabs.count - 1 in
selectCanvasTab(at:), keeping the function purely zero-based, and move the index
conversion logic (handling 9 = last) to the caller (likely a shortcut handler or
Workspace.selectSurface(at:)) so it performs the necessary conversion before
invoking selectCanvasTab(at:).
🪄 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: af074529-7462-4ff7-8de9-74b6548493e2
📒 Files selected for processing (13)
Packages/macOS/CmuxCanvas/Sources/CmuxCanvas/CanvasPlacer.swiftPackages/macOS/CmuxCanvasUI/Sources/CmuxCanvasUI/CanvasModel.swiftSources/AppDelegate+CanvasShortcutRouting.swiftSources/AppDelegate+EqualizeSplitsShortcut.swiftSources/AppDelegate.swiftSources/Canvas/Workspace+CanvasLayout.swiftSources/ContentView.swiftSources/KeyboardShortcutActionContext.swiftSources/KeyboardShortcutContext.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AppDelegateEqualizeSplitsShortcutTests.swiftcmuxTests/CanvasShortcutContextTests.swift
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.
Summary
workspaceCanvasLayoutso Canvas-only shortcuts are active only in Canvas layout across runtime, Settings/package metadata, command palette hints, docs, and schema.toggleSplitZoomthrough Canvas overview when the selected workspace is in Canvas layout, while keeping Bonsplit zoom in split layout.Testing
./scripts/setup.shswift test --package-path Packages/macOS/CmuxSettings --filter Shortcutpython3 -m json.tool web/data/cmux.schema.json >/dev/null && python3 -m json.tool web/messages/en.json >/dev/null && python3 -m json.tool web/messages/ja.json >/dev/null./scripts/lint-pbxproj-test-wiring.shxcodebuild -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-canvas-shortcuts-test -only-testing:cmuxTests/KeyboardShortcutContextTests/testCanvasOnlyShortcutDefaultWhenClausesRequireCanvasLayout -only-testing:cmuxTests/AppDelegateEqualizeSplitsShortcutTests/testCmdShiftReturnInCanvasModeDoesNotToggleBonsplitSplitZoom test\n-git diff --check origin/main...HEAD\n-./scripts/reload-cloud.sh --tag canvsk(fleet unavailable, fell back to local./scripts/reload.sh --tag canvsk)\n- Tagged preflight:debug.shortcut.simulate {"combo":"cmd+shift+return"}toggled Bonsplit zoom in split layout, then toggled Canvas overview in Canvas layout while Bonsplit panels stayed visible.\n\n## Notes\n- Regression test is commit 1; implementation is commit 2 so CI shows the intended red/green structure.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes Canvas keyboard shortcut routing and focus. Shortcuts now respect
workspaceCanvasLayout; Cmd+Shift+Return toggles the Canvas overview in Canvas (keeps Bonsplit zoom in split layout), and Cmd+0 resets Canvas zoom only in Canvas, not in browser/markdown..splitRight/.splitDown) and browser splits open a floating Canvas pane beside the focused pane, honoring the requested direction; browser splits focus the address bar.workspaceCanvasLayout;canvasZoomResetis Cmd+0 and scoped away from focused browser/markdown; Equalize Splits in Canvas equalizes widths and heights only; docs/schema add Canvas bindings andworkspaceCanvasLayout; command palette hints use full context; tests cover gating, directional placement, surface digits, equalize, and Cmd+Shift+Return/Cmd+0 routing.Written for commit d695c60. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
workspaceCanvasLayoutboolean context key and updated examples for per-actionwhenclauses.