Remove custom hotkeys handler in favor of react-hotkeys-hook#114
Remove custom hotkeys handler in favor of react-hotkeys-hook#114saddlepaddle merged 1 commit intomainfrom
Conversation
WalkthroughThis PR migrates keyboard shortcut handling from a centralized custom system (useGlobalShortcuts hook with keyboard-shortcuts and shortcuts modules) to a distributed component-based approach using react-hotkeys-hook library. The old infrastructure is removed entirely and hotkey registration is moved into individual components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/index.tsx (1)
2-34: Align modifiers withmodand considerpreventDefaultfor the sidebar shortcutThe wiring itself looks correct, but two details are worth tightening up:
meta+...vsmod+...:react-hotkeys-hooksupports amodmodifier that maps toctrlon Windows/Linux andmetaon macOS, which is usually what users expect for editor‑style shortcuts. Usingmetadirectly means Windows users need the OS key instead of Ctrl. (react-hotkeys-hook.vercel.app)meta+sdefault behavior: in Chromium environments,cmd/ctrl+sis typically bound to “Save page”. The library docs recommend preventing the default if you are overriding that shortcut; otherwise you can end up both toggling the sidebar and triggering the browser/Electron save behavior, depending on menu configuration. (react-hotkeys-hook.vercel.app)You might want to update along these lines:
- useHotkeys('meta+s', toggleSidebar, [toggleSidebar]); + useHotkeys('mod+s', toggleSidebar, { preventDefault: true }, [toggleSidebar]); - useHotkeys('meta+d', () => { + useHotkeys('mod+d', () => { if (activeWorkspaceId) { splitTabVertical(activeWorkspaceId); } - }, [activeWorkspaceId, splitTabVertical]); + }, [activeWorkspaceId, splitTabVertical]); - useHotkeys('meta+shift+d', () => { + useHotkeys('mod+shift+d', () => { if (activeWorkspaceId) { splitTabHorizontal(activeWorkspaceId); } }, [activeWorkspaceId, splitTabHorizontal]);apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx (1)
3-42: Workspace hotkey logic looks solid; considermodfor cross‑platform ergonomicsThe index/bounds checks around
setActiveWorkspace.mutatelook correct and safe even if the active workspace disappears or the list changes underneath.The only thing I’d reconsider is the modifier choice:
- Using
'meta+alt+left/right'will map to Cmd+Opt+Arrow on macOS, but to Super+Alt+Arrow on Windows/Linux, which is an unusual combo for app‑level navigation.- Since
react-hotkeys-hooksupportsmod(Ctrl on Win/Linux, Meta on macOS),'mod+alt+left/right'would better match common expectations for “previous/next workspace” on all platforms. (react-hotkeys-hook.vercel.app)If you don’t have strong reasons to stick with the OS key on non‑mac platforms, swapping
meta→modhere would improve consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
apps/desktop/package.json(1 hunks)apps/desktop/src/renderer/hooks/useGlobalShortcuts.ts(0 hunks)apps/desktop/src/renderer/lib/keyboard-shortcuts.ts(0 hunks)apps/desktop/src/renderer/lib/shortcuts.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx(3 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/index.tsx(1 hunks)
💤 Files with no reviewable changes (3)
- apps/desktop/src/renderer/hooks/useGlobalShortcuts.ts
- apps/desktop/src/renderer/lib/keyboard-shortcuts.ts
- apps/desktop/src/renderer/lib/shortcuts.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx (1)
apps/desktop/src/renderer/react-query/workspaces/useSetActiveWorkspace.ts (1)
useSetActiveWorkspace(7-23)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx (1)
apps/desktop/src/renderer/stores/tabs/hooks.ts (5)
useTabs(3-3)useActiveTabIds(4-5)useAddTab(7-7)useSetActiveTab(10-11)useRemoveTab(8-8)
apps/desktop/src/renderer/screens/main/index.tsx (2)
apps/desktop/src/renderer/stores/sidebar-state.ts (1)
useSidebarStore(17-37)apps/desktop/src/renderer/stores/tabs/hooks.ts (2)
useSplitTabVertical(19-20)useSplitTabHorizontal(21-22)
🔇 Additional comments (1)
apps/desktop/package.json (1)
55-61: Hotkeys dependency addition looks good; just confirm peer/engine compatibility locallyAdding
react-hotkeys-hookat^5.2.1aligns with the current stable, non‑vulnerable release and its documented API (including the overload where the third arg is a deps array) matches how it’s used elsewhere in this PR. (secure.software)Since you’re on React 19.1.1 and Electron 39, it should work fine, but please run your normal install flow and glance at any peer‑dependency warnings to be safe.
| // Tab management shortcuts - work even when sidebar is closed | ||
| useHotkeys('meta+t', () => { | ||
| if (activeWorkspaceId) { | ||
| addTab(activeWorkspaceId); | ||
| } | ||
| }, [activeWorkspaceId, addTab]); | ||
|
|
||
| useHotkeys('meta+w', () => { | ||
| if (activeTabId) { | ||
| removeTab(activeTabId); | ||
| } | ||
| }, [activeTabId, removeTab]); | ||
|
|
||
| useHotkeys('meta+alt+up', () => { | ||
| if (!activeWorkspaceId || !activeTabId) return; | ||
| const index = tabs.findIndex((t) => t.id === activeTabId); | ||
| if (index > 0) { | ||
| setActiveTab(activeWorkspaceId, tabs[index - 1].id); | ||
| } | ||
| }, [activeWorkspaceId, activeTabId, tabs, setActiveTab]); | ||
|
|
||
| useHotkeys('meta+alt+down', () => { | ||
| if (!activeWorkspaceId || !activeTabId) return; | ||
| const index = tabs.findIndex((t) => t.id === activeTabId); | ||
| if (index < tabs.length - 1) { | ||
| setActiveTab(activeWorkspaceId, tabs[index + 1].id); | ||
| } | ||
| }, [activeWorkspaceId, activeTabId, tabs, setActiveTab]); | ||
|
|
||
| // Jump to tab by number (Cmd+1 through Cmd+9) | ||
| useHotkeys('meta+1,meta+2,meta+3,meta+4,meta+5,meta+6,meta+7,meta+8,meta+9', (_, handler) => { | ||
| if (!activeWorkspaceId) return; | ||
| const key = handler.keys?.join(''); | ||
| const num = key ? Number.parseInt(key, 10) : null; | ||
| if (num && tabs[num - 1]) { | ||
| setActiveTab(activeWorkspaceId, tabs[num - 1].id); | ||
| } | ||
| }, [activeWorkspaceId, tabs, setActiveTab]); |
There was a problem hiding this comment.
🧩 Analysis chain
Meta+T / Meta+W / Meta+1–9 hotkeys are likely to conflict with browser/Electron defaults
The tab management logic itself is clean:
- Filtering
tabsbyworkspaceIdand!parentIdis straightforward. meta+alt+up/downnavigation and the numbered tab jump usinghandler.keys?.join('')follow the patterns shown in thereact-hotkeys-hookdocs. (react-hotkeys-hook.vercel.app)
However, the specific keybindings chosen are risky with this library:
- The docs explicitly call out that browsers don’t allow overriding several “safe browsing” shortcuts such as
meta+W,meta+N,meta+T, theirmeta+shift+...variants, and themeta+1..9tab-selection shortcuts; key events for these combinations may never reach JavaScript at all. (react-hotkeys-hook.vercel.app) - Here, those exact combos are used for:
meta+t→ add tab.meta+w→ close tab.meta+1..9→ jump to tab.
- In a Chrome-based environment (including Electron unless you intercept at
webContentslevel), users are very likely to get the default behavior (closing the window, opening a new tab/window, switching OS/browser tabs) instead of your in‑app tab actions, or the handlers may never fire.
That’s a significant UX and reliability problem, especially for cmd+w, which can close the entire window instead of just the active in‑app tab.
I’d strongly recommend:
- Verifying on macOS and Windows in both dev (Vite in a browser, if applicable) and packaged Electron builds that these handlers actually fire and don’t accidentally close the app/window.
- If they don’t behave reliably, either:
- Move just these reserved combos (
cmd+w,cmd+t,cmd+1..9) back to Electron‑level handling (before-input-event/globalShortcut) and dispatch actions into the renderer instead of usingreact-hotkeys-hook; or - Change the bindings to non‑reserved sequences (for example
mod+alt+wfor close tab,mod+alt+tfor new tab, and another pattern for numbered jumps), and surface them in whatever “keyboard shortcuts” UI you expose.
- Move just these reserved combos (
Separately, using meta instead of mod here means Windows/Linux users must press the OS key rather than Ctrl. If you want the usual “Cmd on mac, Ctrl elsewhere” behavior, consider swapping to mod for all these tab shortcuts as well. (react-hotkeys-hook.vercel.app)
🏁 Script executed:
#!/bin/bash
# Step 1: Locate and examine the target file and its imports
find . -name "index.tsx" -path "*/WorkspaceView/*" -type f | head -1 | xargs catLength of output: 4394
🏁 Script executed:
#!/bin/bash
# Step 2: Check package.json for react-hotkeys-hook version and Electron
find . -name "package.json" -type f | head -5 | xargs grep -l "react-hotkeys-hook\|electron" 2>/dev/null || echo "Not found in initial scan"Length of output: 59
🏁 Script executed:
#!/bin/bash
# Step 3: Search for Electron main process configuration and before-input-event handlers
rg -n "before-input-event|globalShortcut|ipcMain" --type=ts --type=js -C3 | head -40Length of output: 2613
🏁 Script executed:
#!/bin/bash
# Step 4: Check for any existing meta vs mod usage in hotkey definitions
rg -n "useHotkeys|meta\+|mod\+" --type=tsx --type=ts -C2 | grep -E "(useHotkeys|meta\+|mod\+)" | head -30Length of output: 27
🌐 Web query:
react-hotkeys-hook reserved shortcuts meta+w meta+t limitations 2025
💡 Result:
Short answer: You generally cannot override meta+W or meta+T in browsers using react-hotkeys-hook — the library’s docs and README warn those browser-level shortcuts (close tab / new tab) are not preventable, so preventDefault won’t stop them. For special cases there’s the experimental Keyboard Lock API (navigator.keyboard.lock) but it has limited support and typically only works in fullscreen. [1][2][3]
Sources:
[1] react-hotkeys-hook docs (preventDefault warning).
[2] react-hotkeys-hook GitHub README (note about certain keystrokes not preventable).
[3] Discussion/experiments about navigator.keyboard.lock (Stack Overflow).
---
🏁 Script executed:
```shell
#!/bin/bash
# Find all index.tsx files under apps/desktop and filter for WorkspaceView
fd "index.tsx" apps/desktop --exec grep -l "useHotkeys" {} \;
Length of output: 203
🏁 Script executed:
#!/bin/bash
# Examine the apps/desktop/package.json for react-hotkeys-hook version and Electron
cat apps/desktop/package.json | grep -A2 -B2 "react-hotkeys-hook\|electron"Length of output: 1236
🏁 Script executed:
#!/bin/bash
# Search for Electron main process and hotkey handlers in active desktop app
find apps/desktop/src/main -type f \( -name "*.ts" -o -name "*.js" \) 2>/dev/null | xargs grep -l "before-input-event\|globalShortcut\|ipcMain" 2>/dev/null || echo "No main process files found"Length of output: 41
🏁 Script executed:
#!/bin/bash
# Correct search for useHotkeys usage with proper file types
rg "useHotkeys.*meta\+|useHotkeys.*mod\+" -t ts -C2 | head -50Length of output: 4316
🏁 Script executed:
#!/bin/bash
# Search for the specific hotkey patterns mentioned in the review
rg "meta\+t|meta\+w|meta\+1" -t ts -t js -C3 | head -50Length of output: 2165
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the full WorkspaceView/index.tsx to confirm lines 29-66 and context
cat apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
Length of output: 2384
🏁 Script executed:
#!/bin/bash
# Check if there are any Electron main process files at all
find apps/desktop/src/main -type f -name "*.ts" -o -name "*.js" 2>/dev/null | head -10Length of output: 309
🏁 Script executed:
#!/bin/bash
# Check for any conditional event handlers or preventDefault logic
rg "preventDefault|keyboard.lock|registerKeyboardEvent" apps/desktop/src --type ts --type js -C2Length of output: 3670
Reserved hotkeys (meta+t, meta+w, meta+1–9) will not work reliably in Electron
The tab management logic itself is clean:
- Filtering
tabsbyworkspaceIdand!parentIdis straightforward. meta+alt+up/downnavigation and the numbered tab jump usinghandler.keys?.join('')follow the patterns shown in thereact-hotkeys-hookdocs.
However, the specific keybindings chosen are risky with this library:
- The docs explicitly warn that browsers and Electron do not allow overriding several "safe browsing" shortcuts such as
meta+w,meta+t, theirmeta+shift+...variants, and themeta+1..9tab-selection shortcuts; key events for these combinations may never reach JavaScript at all. - Here, those exact combos are used for:
meta+t→ add tab.meta+w→ close tab.meta+1..9→ jump to tab.
- In this Electron environment, users will likely get the default behavior (closing the window, opening a new tab, switching OS/browser tabs) instead of your in-app tab actions, or the handlers may never fire.
This is a significant UX and reliability problem, especially for cmd+w, which can close the entire window instead of just the active in-app tab.
I'd strongly recommend:
- Verifying on macOS and Windows in both dev and packaged Electron builds that these handlers actually fire and don't accidentally close the app/window.
- If they don't behave reliably, either:
- Move just these reserved combos (
cmd+w,cmd+t,cmd+1..9) back to Electron-level handling (before-input-event/globalShortcut) and dispatch actions into the renderer instead of usingreact-hotkeys-hook; or - Change the bindings to non-reserved sequences (for example
mod+alt+wfor close tab,mod+alt+tfor new tab, and another pattern for numbered jumps).
- Move just these reserved combos (
Separately, using meta instead of mod means Windows/Linux users must press the OS key rather than Ctrl. If you want the usual "Cmd on macOS, Ctrl elsewhere" behavior, consider swapping to mod for all these tab shortcuts.
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
around lines 29–66, the current hotkeys use reserved combos (meta+t, meta+w,
meta+1..9) which may be intercepted by the OS/Electron and never reach
react-hotkeys-hook; replace these usages by either moving handling for these
reserved combos to the Electron main process (use before-input-event or
globalShortcut and send an IPC to the renderer to add/close/switch tabs) or
change the bindings to non-reserved sequences (e.g., mod+alt+t, mod+alt+w,
mod+alt+1..9) and switch from literal "meta" to "mod" so Ctrl is used on
Windows/Linux; ensure you update the useHotkeys calls accordingly and test on
macOS and Windows in both dev and packaged builds to confirm the handlers fire
and the window isn’t closed unexpectedly.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.