Migrate web keyboard shortcuts to TanStack hotkeys manager#69
Migrate web keyboard shortcuts to TanStack hotkeys manager#69juliusmarminge wants to merge 1 commit intomainfrom
Conversation
- replace window keydown listeners with `@tanstack/react-hotkeys` registrations in chat, sidebar, and context menu flows - add `shortcutToRawHotkey` and `shortcutsForCommands` helpers for normalized, deduplicated hotkey mappings - add focused keybinding tests and lockfile/package updates for new hotkeys deps
WalkthroughThe changes introduce a centralized hotkey management system by integrating the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Migrate web keyboard shortcuts to TanStack hotkeys and replace global window listeners in
|
Greptile SummaryMigrates keyboard shortcut handling in the web app from ad-hoc
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["keybindings config<br/>(ResolvedKeybindingsConfig)"] --> B["shortcutsForCommands()"]
B --> C["RawHotkey[] (deduplicated)"]
C --> D["getHotkeyManager().register()"]
D --> E["ChatView: terminal hotkeys<br/>(toggle, split, close, new)"]
D --> F["ChatView: openFavoriteEditor hotkeys"]
D --> G["Sidebar: chat hotkeys<br/>(new, newLocal)"]
A --> H["shortcutToRawHotkey()"]
H --> I["normalizeKeyName() + modifier flags"]
I --> C
J["useHotkey('Escape')"] --> K["ChatView: close expanded image"]
D --> L["contextMenuFallback: Escape dismiss"]
E --> M["is*Shortcut() re-check<br/>+ when-clause evaluation"]
F --> M
G --> M
M --> N["Action dispatch"]
Last reviewed commit: db5681e |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const handles = terminalHotkeys.map((hotkey) => | ||
| manager.register( | ||
| hotkey, | ||
| (event) => { | ||
| if (!activeThreadId || event.defaultPrevented) return; | ||
| const shortcutContext = { | ||
| terminalFocus: isTerminalFocused(), | ||
| terminalOpen: Boolean(activeThread?.terminalOpen), | ||
| }; | ||
|
|
||
| if (isTerminalToggleShortcut(event, keybindings, { context: shortcutContext })) { | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| toggleTerminalVisibility(); | ||
| return; | ||
| } | ||
|
|
||
| if (isTerminalToggleShortcut(event, keybindings, { context: shortcutContext })) { | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| toggleTerminalVisibility(); | ||
| return; | ||
| } | ||
| if (isTerminalSplitShortcut(event, keybindings, { context: shortcutContext })) { | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| if (!activeThread?.terminalOpen) { | ||
| dispatch({ | ||
| type: "SET_THREAD_TERMINAL_OPEN", | ||
| threadId: activeThreadId, | ||
| open: true, | ||
| }); | ||
| } | ||
| splitTerminal(); | ||
| return; | ||
| } | ||
|
|
||
| if (isTerminalSplitShortcut(event, keybindings, { context: shortcutContext })) { | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| if (!activeThread?.terminalOpen) { | ||
| dispatch({ | ||
| type: "SET_THREAD_TERMINAL_OPEN", | ||
| threadId: activeThreadId, | ||
| open: true, | ||
| }); | ||
| } | ||
| splitTerminal(); | ||
| return; | ||
| } | ||
| if (isTerminalCloseShortcut(event, keybindings, { context: shortcutContext })) { | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| if (!activeThread?.terminalOpen) return; | ||
| closeTerminal(activeThread.activeTerminalId); | ||
| return; | ||
| } | ||
|
|
||
| if (isTerminalCloseShortcut(event, keybindings, { context: shortcutContext })) { | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| if (!activeThread?.terminalOpen) return; | ||
| closeTerminal(activeThread.activeTerminalId); | ||
| return; | ||
| } | ||
| if (!isTerminalNewShortcut(event, keybindings, { context: shortcutContext })) return; | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| if (!activeThread?.terminalOpen) { | ||
| dispatch({ | ||
| type: "SET_THREAD_TERMINAL_OPEN", | ||
| threadId: activeThreadId, | ||
| open: true, | ||
| }); | ||
| } | ||
| createNewTerminal(); | ||
| }, | ||
| { | ||
| enabled: Boolean(activeThreadId), | ||
| conflictBehavior: "allow", | ||
| ignoreInputs: false, | ||
| preventDefault: false, | ||
| stopPropagation: false, | ||
| target: window, | ||
| }, | ||
| ), |
There was a problem hiding this comment.
Shared handler re-checks every shortcut for each hotkey
Each RawHotkey from terminalHotkeys is registered with the same handler that sequentially checks isTerminalToggleShortcut, isTerminalSplitShortcut, isTerminalCloseShortcut, and isTerminalNewShortcut. Since shortcutsForCommands already extracts the distinct key combos, when e.g. the Mod+J hotkey fires, it still runs through the split/close/new checks unnecessarily before (or after) finding the toggle match.
This works correctly because only the matching is*Shortcut call will return true, but it's doing redundant work on every keypress. Consider registering each command's hotkeys separately with a dedicated handler to avoid the unnecessary checks and make the intent clearer:
const terminalCommands = [
{ commands: ["terminal.toggle"] as const, action: () => toggleTerminalVisibility() },
{ commands: ["terminal.split"] as const, action: () => { /* ... */ } },
// ...
] as const;This isn't a functional issue — just something to consider for clarity as more shortcuts are added.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| useEffect(() => { | ||
| const manager = getHotkeyManager(); | ||
| const handles = chatHotkeys.map((hotkey) => | ||
| manager.register( | ||
| hotkey, | ||
| (event) => { | ||
| const activeThread = state.threads.find((t) => t.id === state.activeThreadId); | ||
| if (isChatNewLocalShortcut(event, keybindings)) { | ||
| const projectId = activeThread?.projectId ?? state.projects[0]?.id; | ||
| if (!projectId) return; | ||
| event.preventDefault(); | ||
| handleNewThread(projectId); | ||
| return; | ||
| } | ||
|
|
||
| window.addEventListener("keydown", onWindowKeyDown); | ||
| if (!isChatNewShortcut(event, keybindings)) return; | ||
| const projectId = activeThread?.projectId ?? state.projects[0]?.id; | ||
| if (!projectId) return; | ||
| event.preventDefault(); | ||
| handleNewThread(projectId, { | ||
| branch: activeThread?.branch ?? null, | ||
| worktreePath: activeThread?.worktreePath ?? null, | ||
| }); | ||
| }, | ||
| { | ||
| conflictBehavior: "allow", | ||
| ignoreInputs: false, | ||
| preventDefault: false, | ||
| stopPropagation: false, | ||
| target: window, | ||
| }, | ||
| ), | ||
| ); | ||
| return () => { | ||
| window.removeEventListener("keydown", onWindowKeyDown); | ||
| for (const handle of handles) { | ||
| handle.unregister(); | ||
| } | ||
| }; | ||
| }, [handleNewThread, keybindings, state.activeThreadId, state.projects, state.threads]); | ||
| }, [chatHotkeys, handleNewThread, keybindings, state.activeThreadId, state.projects, state.threads]); |
There was a problem hiding this comment.
Same shared-handler pattern; consider per-command registration
Same note as the terminal hotkeys in ChatView.tsx: every hotkey in chatHotkeys (covering both chat.new and chat.newLocal) receives the same handler that checks isChatNewLocalShortcut then isChatNewShortcut. This works but is somewhat fragile if someone later changes shortcut config to overlap the two commands, since the check order implicitly gives chat.newLocal priority.
Registering each command's hotkeys with its own focused handler would make the precedence explicit and remove the dead-code paths per invocation.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| useEffect(() => { | ||
| const handler = (e: globalThis.KeyboardEvent) => { | ||
| if (!isOpenFavoriteEditorShortcut(e, keybindings)) return; | ||
| if (!api || !activeProject) return; | ||
|
|
||
| e.preventDefault(); | ||
| const cwd = activeThread?.worktreePath ?? activeProject.cwd; | ||
| void api.shell.openInEditor(cwd, lastEditor); | ||
| const manager = getHotkeyManager(); | ||
| const handles = openFavoriteHotkeys.map((hotkey) => | ||
| manager.register( | ||
| hotkey, | ||
| (event) => { | ||
| if (!isOpenFavoriteEditorShortcut(event, keybindings)) return; | ||
| if (!api || !activeProject) return; | ||
|
|
||
| event.preventDefault(); | ||
| const cwd = activeThread?.worktreePath ?? activeProject.cwd; | ||
| void api.shell.openInEditor(cwd, lastEditor); | ||
| }, | ||
| { | ||
| conflictBehavior: "allow", | ||
| ignoreInputs: false, | ||
| preventDefault: false, | ||
| stopPropagation: false, | ||
| target: window, | ||
| }, | ||
| ), | ||
| ); | ||
| return () => { | ||
| for (const handle of handles) { | ||
| handle.unregister(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Redundant isOpenFavoriteEditorShortcut re-check inside hotkey handler
The handler is only invoked when TanStack matches the registered openFavoriteHotkeys key combo. Then line 2137 re-checks the same event with isOpenFavoriteEditorShortcut(event, keybindings). Since shortcutsForCommands already filtered for "editor.openFavorite", this inner check will always pass when the correct key combo fires — unless the keybinding has a when clause that the TanStack registration doesn't evaluate. Given that editor.openFavorite bindings in this codebase don't use when clauses, this re-check is a no-op guard.
If when-clause awareness is intended to be preserved here, it would be worth adding a comment explaining that; otherwise the check can be removed to simplify the handler.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/components/Sidebar.tsx (1)
394-432: Correct but consider optimizing with refs for frequently-changing state.The effect correctly re-registers hotkeys when dependencies change, ensuring fresh state values. However, accessing
state.threads,state.projects, andstate.activeThreadIddirectly in the handler causes re-registration on every state change.If performance becomes a concern, consider using refs for state values that the handler reads but shouldn't trigger re-registration:
♻️ Optional optimization pattern
+ const stateRef = useRef({ threads: state.threads, projects: state.projects, activeThreadId: state.activeThreadId }); + useEffect(() => { + stateRef.current = { threads: state.threads, projects: state.projects, activeThreadId: state.activeThreadId }; + }, [state.threads, state.projects, state.activeThreadId]); useEffect(() => { const manager = getHotkeyManager(); const handles = chatHotkeys.map((hotkey) => manager.register( hotkey, (event) => { - const activeThread = state.threads.find((t) => t.id === state.activeThreadId); + const { threads, projects, activeThreadId } = stateRef.current; + const activeThread = threads.find((t) => t.id === activeThreadId); // ... rest of handler }, // options ), ); return () => { /* cleanup */ }; - }, [chatHotkeys, handleNewThread, keybindings, state.activeThreadId, state.projects, state.threads]); + }, [chatHotkeys, handleNewThread, keybindings]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/Sidebar.tsx` around lines 394 - 432, The effect re-registers hotkeys on any change to state.threads, state.projects, or state.activeThreadId; to prevent excessive re-registration, create refs (e.g., threadsRef, projectsRef, activeThreadIdRef) that you keep updated from state (via small useEffect(s) or immediately before registering) and read those refs inside the hotkey handler instead of reading state directly; leave only chatHotkeys, handleNewThread, and keybindings in the registration effect's dependency array so handlers use up-to-date values from refs while avoiding re-registering on frequent state updates; keep the existing getHotkeyManager registration and cleanup (handle.unregister) logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/components/Sidebar.tsx`:
- Around line 394-432: The effect re-registers hotkeys on any change to
state.threads, state.projects, or state.activeThreadId; to prevent excessive
re-registration, create refs (e.g., threadsRef, projectsRef, activeThreadIdRef)
that you keep updated from state (via small useEffect(s) or immediately before
registering) and read those refs inside the hotkey handler instead of reading
state directly; leave only chatHotkeys, handleNewThread, and keybindings in the
registration effect's dependency array so handlers use up-to-date values from
refs while avoiding re-registering on frequent state updates; keep the existing
getHotkeyManager registration and cleanup (handle.unregister) logic intact.
github-actions
bot
added
the
vouch:trusted
1 participant
Summary
window/documentkeyboard listeners with@tanstack/react-hotkeysregistrations inChatView,Sidebar, and context-menu fallback flows.RawHotkeyentries and deduplicate command hotkeys while preserving config order.@tanstack/react-hotkeysdependency and lockfile updates.Testing
apps/web/src/keybindings.test.ts: added checks forshortcutToRawHotkeyalias normalization andshortcutsForCommandsdedupe/order behavior.Note
Medium Risk
Changes global keyboard shortcut registration for terminal/chat/editor actions and Escape handling, which can subtly break or conflict with existing shortcuts across focus contexts. Scope is limited to the web UI and includes added unit tests to validate hotkey normalization/deduping.
Overview
Migrates web keyboard shortcut handling in
ChatView,Sidebar, andcontextMenuFallbackfrom imperativewindow/documentkeydownlisteners to@tanstack/react-hotkeys(useHotkey/getHotkeyManager().register) with explicit unregister cleanup.Adds keybinding helpers in
keybindings.ts(shortcutToRawHotkey,shortcutsForCommands) to convert configured shortcuts into TanStackRawHotkeys and dedupe them in config order, and expandskeybindings.test.tscoverage for these helpers. Updatesapps/webdependencies/lockfile to include@tanstack/react-hotkeys.Written by Cursor Bugbot for commit db5681e. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Release Notes
Refactor
Tests