Repository navigation
Second change - #3766
Second change#3766
Conversation
The remappable right-sidebar shortcuts had drifted: the action labeled Toggle Right Sidebar only moved focus back to the terminal, and Open File Explorer reused the last right-sidebar mode instead of selecting Files. Route those shortcuts through explicit shared actions so Cmd-Option-B toggles visibility and Cmd-Shift-E opens the Files mode regardless of the previous mode. Constraint: Do not include the pre-existing vendor/bonsplit submodule dirty state Rejected: Keep Cmd-Shift-E as a generic sidebar visibility toggle | it preserves stale Find mode and violates the action label Confidence: high Scope-risk: narrow Directive: Keep right-sidebar shortcut labels aligned with visibility and mode behavior; do not route Open File Explorer through a mode-preserving toggle Tested: ./scripts/reload.sh --tag fix-right-sidebar-toggle --launch Not-tested: Local unit test suite per repository testing policy
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
To use Codex here, create a Codex account and connect to github. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR refactors the right-sidebar toggle and focus API in AppDelegate by removing two keyboard-focus-based methods and introducing a new visibility-based toggle method. The new method directly manages ChangesRight Sidebar Toggle/Focus API Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
✨ 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 refactors the right sidebar shortcut behavior: the old 3-state focus cycle (
Confidence Score: 4/5Safe to merge; production logic is clean with only minor test coverage gaps and an undescribed submodule bump. The refactor to toggleRightSidebarVisibilityInActiveMainWindow is straightforward and the new tests cover the primary show/hide transitions. However, the semantic shift where a visible-sidebar + terminal-focused shortcut press now hides the sidebar (previously it moved focus to the sidebar) has no test coverage. The vendor/bonsplit submodule bump is also undescribed. Sources/AppDelegate.swift around the show/hide logic in toggleRightSidebarVisibilityInActiveMainWindow, and vendor/bonsplit for the undescribed upstream change. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Keyboard shortcut fired] --> B{Which action?}
B -->|.focusRightSidebar| C[toggleRightSidebarVisibilityInActiveMainWindow]
B -->|.toggleFileExplorer| D[Task at MainActor - focusRightSidebarInActiveMainWindow mode=.files]
C --> E{context available?}
E -->|No| F[return false]
E -->|Yes| G{state available?}
G -->|No| F
G -->|Yes| H{state.isVisible?}
H -->|Yes - hide| I[state.setVisible false + restoreTerminalFocus + return true]
H -->|No - show| J[focusForInWindowCommand + focusRightSidebar + return result]
D --> K[Shows sidebar in Files mode and focuses first item]
Reviews (1): 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}
| window.makeKeyAndOrderFront(nil) | ||
| RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.05)) |
There was a problem hiding this comment.
testFocusRightSidebarShortcutTogglesRightSidebarVisibility always begins with state.setVisible(false) before driving the shortcut, so it verifies the "hidden → show" and "visible → hide" transitions only. The previous toggleRightSidebarOrTerminalFocus() had a third state: sidebar visible with keyboard focus on the terminal, where the old behavior would have moved focus to the sidebar. The new toggleRightSidebarVisibilityInActiveMainWindow unconditionally hides the sidebar in that state instead — a user-visible change that has no test coverage. If that semantic change is intentional, a comment or a third test case would prevent a future accidental reversion.
| @@ -1 +1 @@ | |||
| Subproject commit f65eccb2e4bf3e77662902f7c44a650fba3b3540 | |||
| Subproject commit 90f4981f6a990a2282b2433a67f32edc89306c36 | |||
There was a problem hiding this comment.
The vendor/bonsplit submodule is advanced from f65eccb to 90f4981 with no description of what changed or why. Please add a brief note in the PR description explaining the relevant upstream commits (e.g., bug fix, API change, version tag) so reviewers and the audit trail can evaluate the change.
This branch was successfully deployed
1 participant
Summary
Testing
Demo Video
For UI or behavior changes, include a short demo video (GitHub upload, Loom, or other direct link).
Review Trigger (Copy/Paste as PR comment)
Checklist
Note
Medium Risk
Changes right-sidebar shortcut behavior and focus restoration logic in
AppDelegate, which can affect key handling and window focus across main windows. Risk is moderated by added shortcut routing tests covering the new toggle and file-explorer mode behavior.Overview
Updates right-sidebar shortcut handling so “Toggle Right Sidebar” now toggles visibility (hide if visible; otherwise reveal and focus the sidebar) via new
toggleRightSidebarVisibilityInActiveMainWindow, removing the oldertoggleRightSidebarInActiveMainWindowand the focus/terminal-toggle behavior.Changes the Open File Explorer shortcut to always reveal the right sidebar in Files mode (and focus the first item) instead of a generic toggle, and updates the corresponding SwiftUI menu command to use the new visibility toggle.
Adds tests asserting that the focus-right-sidebar shortcut flips
fileExplorer.isVisibleon successive presses, and that Open File Explorer selects.fileseven if.findwas previously active.Reviewed by Cursor Bugbot for commit 538e60a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes right-sidebar shortcuts to match labels: Cmd-Option-B now toggles visibility, and Cmd-Shift-E always opens the Files mode and focuses the first item. Refactors shortcut routing to explicit visibility/mode actions and adds tests to prevent regressions.
Bug Fixes
toggleRightSidebarVisibilityInActiveMainWindow(no longer just shifts focus back to the terminal).focusRightSidebarInActiveMainWindow(mode: .files, focusFirstItem: true)so Files is selected even if Find was last used.toggleRightSidebarInActiveMainWindowand updated related logs/labels from “focus” to “visibility”.Dependencies
vendor/bonsplitsubmodule pointer.Written for commit 538e60a. Summary will update on new commits.
Summary by CodeRabbit
Improvements
Tests