Repository navigation
Fix #3662: toggle right sidebar with CMD+SHIFT+E instead of just focusing - #3875
austinywang wants to merge 7 commits into
Conversation
The Cmd+Shift+E regression needs a small decision seam so tests can describe the intended tri-state behavior without driving AppKit focus state. This commit introduces the seam and the failing expectations while preserving the current visible-focused behavior for the fix commit to change. Constraint: Tests must exercise behavior through a runtime seam, not source text.\nRejected: Add an end-to-end AppKit shortcut test only | too much focus/window setup for the decision being locked.\nConfidence: high\nScope-risk: narrow\nDirective: Keep Cmd+Shift+E behavior expressed as one tri-state decision: hidden shows and focuses, visible-unfocused focuses, visible-focused hides.\nTested: Not run per task instructions.\nNot-tested: CI has not run this failing checkpoint yet.
Cmd+Shift+E already owned the right-sidebar focus path, but the focused-sidebar branch restored terminal focus while leaving the sidebar visible. Keep the visibility decision inside MainWindowFocusController, so shortcut, menu, and command palette dispatchers share one MainActor decision surface. Constraint: Preserve the committed failing test as the first commit in the regression pair. Constraint: Do not run local tests or bare xcodebuild for this task. Rejected: Add a separate close-only shortcut path | it would split right-sidebar visibility behavior across entrypoints. Confidence: high Scope-risk: narrow Directive: Keep Cmd+Shift+E behavior expressed as hidden shows and focuses, visible-unfocused focuses, and visible-focused hides. Tested: git diff --check Not-tested: Local unit tests and local app build, per task instruction to rely on CI before tagged reload.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a stateful right-sidebar keyboard-focus action (hide/focus/show-and-focus), implements execution in MainWindowFocusController, wires a dedicated command-palette command and ContentView handler, routes AppDelegate to the new performer, and adds tests verifying action resolution and command mapping. ChangesRight Sidebar Toggle Shortcut
Sequence DiagramsequenceDiagram
participant User as User / Keyboard
participant ContentView
participant CommandPalette
participant AppDelegate
participant MainWindowFocusController
User->>ContentView: invoke "Toggle Right Sidebar" command
ContentView->>CommandPalette: resolve command ID -> shortcut action (.focusRightSidebar)
CommandPalette->>AppDelegate: call toggleRightSidebarKeyboardFocusInActiveMainWindow(...)
AppDelegate->>MainWindowFocusController: performRightSidebarFocusShortcut(currentResponder: window?.firstResponder)
MainWindowFocusController->>MainWindowFocusController: compute RightSidebarFocusShortcutAction (hide/focus/showAndFocus)
MainWindowFocusController->>MainWindowFocusController: execute action (hide / focus / showAndFocus)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
origin/main introduced a separate pure right-sidebar toggle action while this branch changes the focus shortcut semantics. The merge keeps both entrypoints: Toggle Right Sidebar performs visibility toggle, while Toggle Right Sidebar Focus uses Cmd+Shift+E and now hides only when the right sidebar already owns focus. Constraint: PR merge state was DIRTY after creation. Rejected: Collapse the new pure toggle action into the focus shortcut | it would discard upstream's clearer visibility command. Confidence: medium Scope-risk: broad Directive: Keep pure visibility toggle and focus-shortcut tri-state separate after this merge. Tested: git diff --cached --check Not-tested: Local tests and local app build, per task instruction to rely on CI before tagged reload.
![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/ContentView`+RightSidebarCommandPalette.swift:
- Around line 85-96: Extract the duplicated local closure into a single private
static helper and have both
commandPaletteRightSidebarToggleCommandContribution() and
commandPaletteRightSidebarModeCommandContributions() call it; specifically, add
a private static function (e.g., constantTitle(_: String) ->
(CommandPaletteContextSnapshot) -> String) that returns the closure used to
produce constant titles/subtitles, then replace the inline constant closures in
both functions with calls to this new helper so duplication is removed while
keeping the exact return type and 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3c6bac8-7aba-430a-8643-fa26139ea422
📒 Files selected for processing (9)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView+RightSidebarCommandPalette.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/MainWindowFocusController.swiftSources/cmuxApp.swiftcmuxTests/RightSidebarCommandPaletteTests.swiftweb/data/cmux-shortcuts.ts
Greptile SummaryThis PR implements a tri-state toggle for Cmd+Shift+E: show+focus when the right sidebar is hidden, focus-only when visible but unfocused, and hide when already focused. It also adds a
Confidence Score: 5/5Safe to merge — the tri-state toggle is well-scoped and the focus-restoration path has been corrected to handle both terminal and browser panels. The refactor cleanly centralises the hide/focus/show decision in RightSidebarFocusShortcutAction.resolve, the hide arm delegates to restoreFocusedPanelFocusFromRightSidebarIfNeeded (which already handles non-terminal panels), and the command palette routing goes through the same tri-state path as the keyboard shortcut. All previously raised concerns have been addressed in the current HEAD, and new tests cover the three resolve branches and the command-palette mapping. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Cmd+Shift+E\nor palette.toggleRightSidebar"] --> B["toggleRightSidebarKeyboardFocusInActiveMainWindow"]
B --> C["performRightSidebarFocusShortcut(currentResponder:)"]
C --> D["rightSidebarFocusShortcutAction(isVisible:, currentResponder:)"]
D --> E["RightSidebarFocusShortcutAction.resolve(isVisible:, isFocused:)"]
E -->|"!isVisible"| F[".showAndFocus"]
E -->|"isVisible && isFocused"| G[".hide"]
E -->|"isVisible && !isFocused"| H[".focus"]
F --> I["focusRightSidebar()"]
H --> I
G --> J["state.setVisible(false)"]
J --> K["restoreFocusedPanelFocusFromRightSidebarIfNeeded(currentResponder:)"]
K -->|"panel is TerminalPanel"| L["focusTerminal()"]
K -->|"panel is BrowserPanel"| M["workspace.focusPanel + restoreFocusIntent"]
K -->|"no panel available"| N["focusTerminalOrReleaseRightSidebarFocus"]
Reviews (3): Last reviewed commit: "Restore focused panel after right sideba..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| registry.register(commandId: Self.commandPaletteToggleRightSidebarCommandID) { | ||
| if AppDelegate.shared?.toggleRightSidebarInActiveMainWindow( | ||
| preferredWindow: observedWindow ?? NSApp.keyWindow ?? NSApp.mainWindow | ||
| ) != true { | ||
| NSSound.beep() | ||
| } | ||
| } |
There was a problem hiding this comment.
The command palette handler calls toggleRightSidebarInActiveMainWindow, which is a pure visibility flip (visible→hide, hidden→show) with no focus awareness. The keyboard shortcut path calls toggleRightSidebarKeyboardFocusInActiveMainWindow → performRightSidebarFocusShortcut, which implements the new tri-state logic (focus when visible-but-unfocused, hide when focused, show+focus when hidden). Because the wrong function is used here, invoking "Toggle Right Sidebar" from the command palette when the sidebar is visible but unfocused will hide it instead of focusing it — the exact opposite of the intended PR behavior and inconsistent with the keyboard shortcut.
There was a problem hiding this comment.
Fixed in f4af32a by routing the command palette entry through toggleRightSidebarKeyboardFocusInActiveMainWindow, so it now shares the same tri-state path as Cmd+Shift+E.
— Claude Code
| case .hide: | ||
| state.setVisible(false) | ||
| _ = restoreTerminalFocusAfterRightSidebarHiddenIfNeeded() | ||
| return true | ||
| case .focus, .showAndFocus: | ||
| return focusRightSidebar() | ||
| } |
There was a problem hiding this comment.
.focus and .showAndFocus arms collapse to identical code
Both case .focus and case .showAndFocus resolve to the same return focusRightSidebar() call, making RightSidebarFocusShortcutAction.showAndFocus a dead branch in performRightSidebarFocusShortcut. focusRightSidebar() already calls state.setVisible(true) unconditionally, so the distinction is correct at the resolve level, but the switch loses it immediately. If a future caller needs to behave differently for "focus only" vs "show then focus", the current collapsed arms offer no leverage. Consider at minimum a comment explaining why both arms reduce to the same call.
There was a problem hiding this comment.
Addressed in f4af32a with an inline comment explaining why .focus and .showAndFocus intentionally share focusRightSidebar().
— Claude Code
CodeRabbit flagged the duplicated local constant closure in the new right-sidebar palette contribution. Extract one private helper in the extension and keep the visibility-toggle command and mode commands on the same small text factory. Constraint: Address high-priority review feedback before CI handoff. Rejected: Leave duplicate local closures | review requested removal and the helper is behavior-preserving. Confidence: high Scope-risk: narrow Directive: Keep palette contribution labels constant through this helper unless a command becomes context-sensitive. Tested: git diff --check Not-tested: Local tests and local app build, per task instruction to rely on CI before tagged reload.
The command palette entry must exercise the same focus-aware right-sidebar path as Cmd+Shift+E. A plain visibility toggle would hide a visible but unfocused sidebar instead of focusing it, which reintroduces split behavior across entrypoints. Constraint: Shared behavior policy requires keyboard shortcut and command palette entrypoints to use one action path. Rejected: Keep the palette on the pure visibility toggle | it diverges from the issue's focus-aware toggle semantics and review feedback. Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local tests/builds per user instruction; CI will verify.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f4af32a. Configure here.
| case .hide: | ||
| state.setVisible(false) | ||
| _ = restoreTerminalFocusAfterRightSidebarHiddenIfNeeded() | ||
| return true |
There was a problem hiding this comment.
.hide case calls restoreTerminalFocusAfterRightSidebarHiddenIfNeeded(), which routes through focusTerminalOrReleaseRightSidebarFocus and only knows how to focus a TerminalPanel. If the panel that was active before the user invoked Cmd+Shift+E is a browser panel, focusTerminal() will either jump to an unrelated terminal or fail entirely, leaving focus adrift. The pre-PR path for the same key event used restoreFocusedPanelFocusFromRightSidebarIfNeeded, which inspects workspace.focusedPanelId, handles non-terminal panels explicitly, and only falls back to focusTerminalOrReleaseRightSidebarFocus when no panel is available.
| case .hide: | |
| state.setVisible(false) | |
| _ = restoreTerminalFocusAfterRightSidebarHiddenIfNeeded() | |
| return true | |
| case .hide: | |
| state.setVisible(false) | |
| _ = restoreFocusedPanelFocusFromRightSidebarIfNeeded(currentResponder: currentResponder) | |
| return true |
The right-sidebar shortcut now has one production execution path through performRightSidebarFocusShortcut. Keeping the older terminal/sidebar focus-toggle helper left tests covering behavior that production no longer used. Constraint: Shared behavior policy requires tests and entrypoints to exercise the production action path. Rejected: Keep the helper for tests only | it would preserve a dead branch and confuse future shortcut changes. Confidence: high Scope-risk: narrow Tested: rg confirmed no remaining references; git diff --check Not-tested: Local tests/builds per user instruction; CI will verify.
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/ContentView`+RightSidebarCommandPalette.swift:
- Around line 32-33: The command palette entry for
Self.commandPaletteToggleRightSidebarCommandID currently returns the
.focusRightSidebar action but the UI uses .toggleRightSidebar.label for the
title, causing a mismatch; fix it by returning .toggleRightSidebar instead of
.focusRightSidebar (or alternatively update the title to use
.focusRightSidebar.label), updating the switch/case that maps
Self.commandPaletteToggleRightSidebarCommandID so the action and label
(.toggleRightSidebar or .focusRightSidebar.label) are consistent.
🪄 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: 81958575-3a9c-476e-a468-16ea98965832
📒 Files selected for processing (4)
Sources/AppDelegate.swiftSources/ContentView+RightSidebarCommandPalette.swiftSources/ContentView.swiftSources/MainWindowFocusController.swift
The focus-aware right-sidebar toggle should preserve the user's previous main-panel context when hiding the sidebar. The command palette entry also now uses the same shortcut action for its title and shortcut hint. Constraint: Shortcut, menu, and palette behavior share one tri-state focus path. Rejected: Terminal-only restoration | loses focus context for browser or other non-terminal panels. Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local tests/builds per user instruction; CI will verify.
|
Closing — the underlying behavior (Cmd+Shift+E toggles the right sidebar dock) was already shipped through a different change, so this branch is no longer needed. |
This branch was successfully deployed
1 participant
Summary
Repro
Before this change, the second press only refocused/restored focus while leaving the sidebar visible. Now it closes the focused right sidebar.
Fixes #3662
Note
Medium Risk
Moderate risk because it changes core keyboard focus/visibility behavior for the right sidebar and routes multiple entry points through new toggle logic; regressions would be user-visible but limited in scope.
Overview
Fixes the right-sidebar keyboard shortcut to behave as a true toggle: show+focus when hidden, focus when visible but unfocused, and hide when already focused, via a new tri-state
RightSidebarFocusShortcutActionandperformRightSidebarFocusShortcut()inMainWindowFocusController.Adds a command palette command (
palette.toggleRightSidebar) wired to the same toggle behavior, and updates command palette contribution plumbing (shared constant text helper, new contribution for the toggle). Tests are updated/added to cover the new toggle decision logic and command palette mapping, and the obsoleteMainWindowFocusToggleDestination/toggle path is removed.Reviewed by Cursor Bugbot for commit f13b247. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Cmd+Shift+E now toggles the right sidebar based on focus: show+focus when hidden, focus when visible, and hide when focused—and it restores the previously focused main panel when hiding. The command palette adds
palette.toggleRightSidebarwith the same focus-aware behavior. Fixes #3662.New Features
palette.toggleRightSidebar) that routes through the focus-aware toggle.Refactors
MainWindowFocusController.performRightSidebarFocusShortcutwith a tri-stateRightSidebarFocusShortcutAction; removed the old focus toggle path andMainWindowFocusToggleDestination, and routed AppDelegate and the command palette through the new path.Written for commit f13b247. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Tests