fix: Cmd+Left/Right tab shortcuts broken when sidebar has focus — scroll jumps instead#841
fix: Cmd+Left/Right tab shortcuts broken when sidebar has focus — scroll jumps instead#841scriptease wants to merge 1 commit intoRunMaestro:rcfrom
Cmd+Left/Right tab shortcuts broken when sidebar has focus — scroll jumps instead#841Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughUpdated the sidebar arrow-key shortcut suppression logic in the keyboard navigation hook. Modified the condition to skip command/control-modified arrow keys regardless of the Alt modifier state, and clarified that unmodified arrow keys remain reserved for group and bookmarks collapse/expand operations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
scriptease
force-pushed
the
feat/cmd-arrow-broken
branch
from
3ccb6ad to
80e121d
Compare
Greptile SummaryAdds a targeted Confidence Score: 5/5Safe to merge — the fix is minimal, correct, and does not regress existing sidebar navigation behaviour. The change is a single targeted guard that early-returns for Cmd/Ctrl+ArrowLeft/Right before any side-effecting code is reached. No logic is removed; plain arrows and all other modifiers behave identically to before. No new P0/P1 findings were identified beyond what previous threads already cover. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[KeyboardEvent fires] --> B{focus === sidebar?}
B -- No --> Z[return false]
B -- Yes --> C{Target is input element?}
C -- Yes --> Z
C -- No --> D{Cmd/Ctrl + ArrowLeft or ArrowRight?}
D -- Yes --> Z2[return false - pass to tab shortcut handler]
D -- No --> E{Key in arrow/space set?}
E -- No --> Z
E -- Yes --> F[e.preventDefault called]
F --> G{Which key?}
G -- ArrowLeft --> H[Collapse group or bookmarks]
G -- ArrowRight --> I[Expand group or bookmarks]
G -- Space --> J[Collapse group, jump to nearest visible]
G -- ArrowUp or ArrowDown --> K[Navigate sessions, auto-expand collapsed groups]
Reviews (2): Last reviewed commit: "`Cmd+Left/Right` tab shortcuts broken wh..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/index.ts`:
- Around line 606-607: The branch-name check using /rc/i.test(branch) is too
broad and can match strings like "feature/force-icon"; update the condition
around the icon selection (the if that checks branch and tag and sets iconFile =
'Maestro_rc.png') to only match an exact "rc" branch or "rc/..." pattern (e.g.,
test for branch === 'rc' or branch starting with 'rc/' or use a regex anchored
to the start like ^rc(?:\/|$) with case-insensitivity) so only true
release-candidate branches get the RC icon.
- Around line 609-611: The empty catch swallowing git resolution errors should
be replaced to capture the thrown error: change the catch block handling the
"git resolution / icon selection" logic to catch (err) and either log a clear
warning including the error (using the existing logger, e.g.,
processLogger.warn/error or console.warn) and continue with the dev-icon
fallback for expected failures, or re-throw unexpected errors so they surface to
Sentry; update the catch in the icon-selection try/catch in index.ts to log the
error message and stack and only swallow known/recoverable cases while
re-throwing others.
🪄 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: CHILL
Plan: Pro
Run ID: aaf30585-40e0-44a0-8f53-781fd391b35c
⛔ Files ignored due to path filters (12)
build/icon.icois excluded by!**/*.icobuild/icon.pngis excluded by!**/*.pngbuild/newest-icon/Maestro.pngis excluded by!**/*.pngbuild/newest-icon/Maestro_dev.icois excluded by!**/*.icobuild/newest-icon/Maestro_dev.pngis excluded by!**/*.pngbuild/newest-icon/Maestro_dev_dock.pngis excluded by!**/*.pngbuild/newest-icon/Maestro_dock.pngis excluded by!**/*.pngbuild/newest-icon/Maestro_rc.icois excluded by!**/*.icobuild/newest-icon/Maestro_rc.pngis excluded by!**/*.pngbuild/newest-icon/Maestro_rc_dock.pngis excluded by!**/*.pngbuild/old-icon/icon.icois excluded by!**/*.icobuild/old-icon/icon.pngis excluded by!**/*.png
📒 Files selected for processing (6)
build/icon.icnsbuild/newest-icon/Maestro_dev.icnsbuild/newest-icon/Maestro_rc.icnsbuild/old-icon/icon.icnssrc/main/index.tssrc/renderer/hooks/keyboard/useKeyboardNavigation.ts
|
@pedramamini I tried to set CMD+left and right but the navigation view would keep stealing the arrow keys |
2 participants
Summary
Cmd+Left/Right Arrowconfigured as previous/next tab shortcuts had no effect when the sidebar had focus — groups collapsed/expanded insteadRoot Cause
handleSidebarNavigationinuseKeyboardNavigation.tswas catching allArrowLeft/ArrowRightevents when the sidebar had focus, regardless of modifier keys. OnlyAlt+Cmd+Arrowwas previously excluded. The handler calledpreventDefault()and returnedtrue, preventing thenextTab/prevTabshortcut handlers from ever being reached.Fix
Broadened the guard in
handleSidebarNavigationto skip anyCmd/Ctrl+Arrowcombination (with or without Alt). PlainArrowLeft/ArrowRight(no modifiers) still handles group collapse/expand in the sidebar.Test plan
Cmd+Right Arrow, "Previous Tab" →Cmd+Left ArrowCmd+Right Arrow— should navigate to next tab, not collapse a groupCmd+Left Arrow— should navigate to previous tab, not collapse a groupArrowLeft/ArrowRight(no Cmd) still collapses/expands groups in sidebarAlt+Cmd+Arrowstill toggles layout as expectedSummary by CodeRabbit