hotkey pane#108
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a centralized global keyboard-shortcut hook and wires it into MainScreen; enforces boolean-return handlers in shortcut infra; replaces legacy split shortcuts with splitHorizontal/splitVertical and adds pane-focus handlers; implements tab-splitting actions in TabsState, updates UI components and tests to exercise splitting. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as MainScreen
participant Hook as useGlobalShortcuts
participant Stores as State Stores
participant Factories as Shortcut Factories
participant Builder as createShortcutHandler
participant DOM as Window (keydown)
Main->>Hook: mount / invoke hook
Hook->>Stores: read workspace, tabs, UI state
Note over Hook,Factories: Build workspace/tab/pane shortcut groups
Hook->>Factories: createWorkspaceShortcuts/createTabShortcuts/createSplitPaneShortcuts
Factories-->>Hook: KeyboardShortcutGroups
Hook->>Builder: merge groups -> handler fn
Builder-->>Hook: listener function
Hook->>DOM: register keydown listener
DOM->>Builder: keydown event
Builder->>Builder: match shortcut -> call handler
alt handler returns false
Builder-->>DOM: preventDefault & stopPropagation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 2
🧹 Nitpick comments (1)
apps/desktop/src/renderer/hooks/useGlobalShortcuts.ts (1)
54-61: TODO: Connect split handlers to store actions.The split handlers are currently placeholders. Once the
splitTabVerticalandsplitTabHorizontalstore methods are implemented, wire them here.Here's how to connect them after the store methods are implemented:
splitVertical: () => { - // TODO: Implement split vertical - console.log("Split vertical"); + if (!activeWorkspaceId) return; + const activeTabId = activeTabIds[activeWorkspaceId]; + useTabsStore.getState().splitTabVertical(activeWorkspaceId, activeTabId || undefined); }, splitHorizontal: () => { - // TODO: Implement split horizontal - console.log("Split horizontal"); + if (!activeWorkspaceId) return; + const activeTabId = activeTabIds[activeWorkspaceId]; + useTabsStore.getState().splitTabHorizontal(activeWorkspaceId, activeTabId || undefined); },Note: You'll need to import
useTabsStoredirectly to callgetState().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/desktop/src/renderer/hooks/useGlobalShortcuts.ts(1 hunks)apps/desktop/src/renderer/lib/keyboard-shortcuts.ts(1 hunks)apps/desktop/src/renderer/lib/shortcuts.ts(5 hunks)apps/desktop/src/renderer/screens/main/index.tsx(1 hunks)apps/desktop/src/renderer/stores/tabs/store.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/desktop/src/renderer/hooks/useGlobalShortcuts.ts (5)
apps/desktop/src/renderer/stores/workspaces.ts (1)
useWorkspacesStore(27-90)apps/desktop/src/renderer/stores/sidebar-state.ts (1)
useSidebarStore(17-37)apps/desktop/src/renderer/stores/tabs/store.ts (5)
useTabs(616-616)useActiveTabIds(617-618)useSetActiveTab(623-624)useAddTab(620-620)useRemoveTab(621-621)apps/desktop/src/renderer/lib/shortcuts.ts (3)
createWorkspaceShortcuts(37-102)createTabShortcuts(104-186)createSplitPaneShortcuts(188-239)apps/desktop/src/renderer/lib/keyboard-shortcuts.ts (1)
createShortcutHandler(50-65)
apps/desktop/src/renderer/screens/main/index.tsx (2)
apps/old-desktop/src/renderer/screens/main/MainScreen.tsx (1)
MainScreen(23-225)apps/desktop/src/renderer/hooks/useGlobalShortcuts.ts (1)
useGlobalShortcuts(22-163)
apps/desktop/src/renderer/lib/shortcuts.ts (1)
apps/desktop/src/renderer/lib/keyboard-shortcuts.ts (1)
KeyboardShortcutGroup(15-18)
🔇 Additional comments (7)
apps/desktop/src/renderer/lib/shortcuts.ts (2)
10-35: LGTM! Interface updates are clear and well-documented.The split and pane navigation handlers are well-organized with helpful iTerm2-style comments.
37-101: LGTM! Workspace shortcuts updated correctly.The split shortcuts follow iTerm2 conventions (Cmd+D for vertical, Cmd+Shift+D for horizontal) and integrate cleanly with the new handler interface.
apps/desktop/src/renderer/screens/main/index.tsx (1)
2-2: LGTM! Clean integration of global shortcuts.The hook is correctly invoked at the component's top level, enabling global keyboard shortcuts for the main screen.
Also applies to: 10-10
apps/desktop/src/renderer/lib/keyboard-shortcuts.ts (1)
12-12: LGTM! Handler return type now enforces boolean.The change from
boolean | voidtobooleanimproves type safety. All handlers in the codebase already return boolean, so this formalization is appropriate.apps/desktop/src/renderer/hooks/useGlobalShortcuts.ts (3)
73-111: LGTM! Tab handlers are correctly implemented.The tab navigation, creation, and jumping logic is sound with appropriate bounds checking and workspace guards. The
reopenClosedTabplaceholder is acceptable for initial implementation.
114-131: TODO implementations noted, but affected by shortcut conflicts.These pane focus handlers are placeholders, which is acceptable. However, due to the shortcut key conflicts identified in
apps/desktop/src/renderer/lib/shortcuts.ts(lines 188-239), these handlers won't be reachable until the conflicts are resolved.
32-162: LGTM! useEffect dependencies are correct.The dependency array includes all external values used in the effect. The derived values (
workspaceTabs,activeTabId) are correctly computed inside the effect from the tracked dependencies.
| export function createSplitPaneShortcuts( | ||
| handlers: Pick< | ||
| ShortcutHandlers, | ||
| "focusPaneLeft" | "focusPaneRight" | "focusPaneUp" | "focusPaneDown" | ||
| >, | ||
| ): KeyboardShortcutGroup { | ||
| return { | ||
| name: "Split Pane Navigation", | ||
| shortcuts: [ | ||
| { | ||
| key: "ArrowLeft", | ||
| modifiers: ["meta", "alt"], | ||
| description: "Focus left pane", | ||
| handler: (event) => { | ||
| event.preventDefault(); | ||
| handlers.focusPaneLeft(); | ||
| return false; | ||
| }, | ||
| }, | ||
| { | ||
| key: "ArrowRight", | ||
| modifiers: ["meta", "alt"], | ||
| description: "Focus right pane", | ||
| handler: (event) => { | ||
| event.preventDefault(); | ||
| handlers.focusPaneRight(); | ||
| return false; | ||
| }, | ||
| }, | ||
| { | ||
| key: "ArrowUp", | ||
| modifiers: ["meta", "alt"], | ||
| description: "Focus upper pane", | ||
| handler: (event) => { | ||
| event.preventDefault(); | ||
| handlers.focusPaneUp(); | ||
| return false; | ||
| }, | ||
| }, | ||
| { | ||
| key: "ArrowDown", | ||
| modifiers: ["meta", "alt"], | ||
| description: "Focus lower pane", | ||
| handler: (event) => { | ||
| event.preventDefault(); | ||
| handlers.focusPaneDown(); | ||
| return false; | ||
| }, | ||
| }, | ||
| ], | ||
| }; | ||
| } |
There was a problem hiding this comment.
Critical: Shortcut key conflicts with workspace and tab navigation.
The pane navigation shortcuts conflict with existing shortcuts:
- Meta+Alt+ArrowLeft/Right: Used by both workspace switching (lines 51, 61) and pane focus (lines 198, 208)
- Meta+Alt+ArrowUp/Down: Used by both tab switching (lines 117, 127) and pane focus (lines 218, 228)
When all shortcuts are registered together in useGlobalShortcuts, only the first matching shortcut will fire, making either workspace/tab navigation or pane navigation inaccessible.
Consider using different modifiers for pane navigation, such as:
- Option 1: Use Cmd+Opt+Ctrl+Arrows for pane focus (keeping workspace/tab nav as-is)
- Option 2: Use Cmd+[ / Cmd+] style shortcuts (similar to tmux/vim)
- Option 3: Make shortcuts contextual - pane navigation only when a split view is active
Apply this diff for Option 1:
export function createSplitPaneShortcuts(
handlers: Pick<
ShortcutHandlers,
"focusPaneLeft" | "focusPaneRight" | "focusPaneUp" | "focusPaneDown"
>,
): KeyboardShortcutGroup {
return {
name: "Split Pane Navigation",
shortcuts: [
{
key: "ArrowLeft",
- modifiers: ["meta", "alt"],
+ modifiers: ["meta", "alt", "ctrl"],
description: "Focus left pane",
handler: (event) => {
event.preventDefault();
handlers.focusPaneLeft();
return false;
},
},
{
key: "ArrowRight",
- modifiers: ["meta", "alt"],
+ modifiers: ["meta", "alt", "ctrl"],
description: "Focus right pane",
handler: (event) => {
event.preventDefault();
handlers.focusPaneRight();
return false;
},
},
{
key: "ArrowUp",
- modifiers: ["meta", "alt"],
+ modifiers: ["meta", "alt", "ctrl"],
description: "Focus upper pane",
handler: (event) => {
event.preventDefault();
handlers.focusPaneUp();
return false;
},
},
{
key: "ArrowDown",
- modifiers: ["meta", "alt"],
+ modifiers: ["meta", "alt", "ctrl"],
description: "Focus lower pane",
handler: (event) => {
event.preventDefault();
handlers.focusPaneDown();
return false;
},
},
],
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function createSplitPaneShortcuts( | |
| handlers: Pick< | |
| ShortcutHandlers, | |
| "focusPaneLeft" | "focusPaneRight" | "focusPaneUp" | "focusPaneDown" | |
| >, | |
| ): KeyboardShortcutGroup { | |
| return { | |
| name: "Split Pane Navigation", | |
| shortcuts: [ | |
| { | |
| key: "ArrowLeft", | |
| modifiers: ["meta", "alt"], | |
| description: "Focus left pane", | |
| handler: (event) => { | |
| event.preventDefault(); | |
| handlers.focusPaneLeft(); | |
| return false; | |
| }, | |
| }, | |
| { | |
| key: "ArrowRight", | |
| modifiers: ["meta", "alt"], | |
| description: "Focus right pane", | |
| handler: (event) => { | |
| event.preventDefault(); | |
| handlers.focusPaneRight(); | |
| return false; | |
| }, | |
| }, | |
| { | |
| key: "ArrowUp", | |
| modifiers: ["meta", "alt"], | |
| description: "Focus upper pane", | |
| handler: (event) => { | |
| event.preventDefault(); | |
| handlers.focusPaneUp(); | |
| return false; | |
| }, | |
| }, | |
| { | |
| key: "ArrowDown", | |
| modifiers: ["meta", "alt"], | |
| description: "Focus lower pane", | |
| handler: (event) => { | |
| event.preventDefault(); | |
| handlers.focusPaneDown(); | |
| return false; | |
| }, | |
| }, | |
| ], | |
| }; | |
| } | |
| export function createSplitPaneShortcuts( | |
| handlers: Pick< | |
| ShortcutHandlers, | |
| "focusPaneLeft" | "focusPaneRight" | "focusPaneUp" | "focusPaneDown" | |
| >, | |
| ): KeyboardShortcutGroup { | |
| return { | |
| name: "Split Pane Navigation", | |
| shortcuts: [ | |
| { | |
| key: "ArrowLeft", | |
| modifiers: ["meta", "alt", "ctrl"], | |
| description: "Focus left pane", | |
| handler: (event) => { | |
| event.preventDefault(); | |
| handlers.focusPaneLeft(); | |
| return false; | |
| }, | |
| }, | |
| { | |
| key: "ArrowRight", | |
| modifiers: ["meta", "alt", "ctrl"], | |
| description: "Focus right pane", | |
| handler: (event) => { | |
| event.preventDefault(); | |
| handlers.focusPaneRight(); | |
| return false; | |
| }, | |
| }, | |
| { | |
| key: "ArrowUp", | |
| modifiers: ["meta", "alt", "ctrl"], | |
| description: "Focus upper pane", | |
| handler: (event) => { | |
| event.preventDefault(); | |
| handlers.focusPaneUp(); | |
| return false; | |
| }, | |
| }, | |
| { | |
| key: "ArrowDown", | |
| modifiers: ["meta", "alt", "ctrl"], | |
| description: "Focus lower pane", | |
| handler: (event) => { | |
| event.preventDefault(); | |
| handlers.focusPaneDown(); | |
| return false; | |
| }, | |
| }, | |
| ], | |
| }; | |
| } |
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/lib/shortcuts.ts around lines 188 to 239, pane
navigation uses the same Meta+Alt+Arrow combinations as workspace/tab navigation
causing conflicts; update each pane-focus shortcut's modifiers array to include
"ctrl" so they become ["meta","alt","ctrl"] (Cmd+Opt+Ctrl+Arrow) for ArrowLeft,
ArrowRight, ArrowUp, and ArrowDown, leaving keys and handlers unchanged so pane
navigation no longer blocks workspace/tab shortcuts.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/renderer/lib/shortcuts.ts (1)
187-238: Pane-focus shortcuts still conflict with workspace/tab navigation
createSplitPaneShortcutsusesMeta+Alt+ArrowLeft/Right/Up/Down, which are already taken by workspace and tab navigation, so only one of these behaviors will fire for each combo when all groups are registered globally. This reintroduces the same conflict noted in the earlier review.To keep all shortcuts usable, consider updating pane-focus modifiers, for example:
export function createSplitPaneShortcuts( @@ name: "Split Pane Navigation", shortcuts: [ { key: "ArrowLeft", - modifiers: ["meta", "alt"], + modifiers: ["meta", "alt", "ctrl"], @@ { key: "ArrowRight", - modifiers: ["meta", "alt"], + modifiers: ["meta", "alt", "ctrl"], @@ { key: "ArrowUp", - modifiers: ["meta", "alt"], + modifiers: ["meta", "alt", "ctrl"], @@ { key: "ArrowDown", - modifiers: ["meta", "alt"], + modifiers: ["meta", "alt", "ctrl"],This preserves existing workspace/tab navigation while giving pane focus its own distinct chord.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx (1)
1-29: SingleTabView correctly delegates to tab store split/remove actionsHooking
useSplitTabHorizontal/useSplitTabVerticalanduseRemoveTabinto the three handlers aligns the single-tab view with the new split/close semantics and keeps the logic centralized in the store.You can later drop or wire up the unused
isDropZoneprop if it’s no longer needed.apps/desktop/src/renderer/stores/tabs/store.test.ts (1)
180-265: Splitting tests are thorough; consider adding in-group split coverage laterThe new
splitTabVertical/splitTabHorizontaltests nicely validate group creation, layout orientation, childparentIds, and active tab updates, as well as the “don’t split groups” and “split specific id” cases. This should catch most regressions in the split actions.In a follow-up, it may be worth adding a test that exercises splitting an existing group pane via the
pathargument, mirroring howGroupTabViewcalls the store.Also applies to: 267-364
apps/desktop/src/renderer/stores/tabs/store.ts (1)
364-369: Guarding against direct group-tab closure is good; consider surfacing via UI instead of consoleBlocking
removeTabfor group tabs avoids bypassing the group/ungroup logic, which is the right call. Depending on how this is surfaced, you may eventually want to route this through your UI/notification layer instead ofconsole.error, but that’s a polish item, not a blocker.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/desktop/CLAUDE.md(0 hunks)apps/desktop/src/renderer/hooks/useGlobalShortcuts.ts(1 hunks)apps/desktop/src/renderer/lib/shortcuts.ts(5 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/index.tsx(3 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx(2 hunks)apps/desktop/src/renderer/stores/tabs/store.test.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/store.ts(6 hunks)apps/desktop/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/hooks/useGlobalShortcuts.ts
🧰 Additional context used
🧬 Code graph analysis (5)
apps/desktop/src/renderer/lib/shortcuts.ts (1)
apps/desktop/src/renderer/lib/keyboard-shortcuts.ts (1)
KeyboardShortcutGroup(15-18)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/index.tsx (2)
apps/desktop/src/renderer/stores/tabs/store.ts (2)
useSplitTabHorizontal(842-843)useSplitTabVertical(840-841)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabContentContextMenu.tsx (1)
TabContentContextMenu(17-40)
apps/desktop/src/renderer/stores/tabs/store.test.ts (2)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
useTabsStore(326-822)apps/desktop/src/shared/types.ts (1)
TabType(23-30)
apps/desktop/src/renderer/stores/tabs/store.ts (3)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
Tab(25-25)apps/desktop/src/shared/types.ts (2)
TabType(23-30)MosaicNode(35-35)apps/desktop/src/renderer/stores/tabs/utils.ts (1)
createNewTab(10-36)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx (1)
apps/desktop/src/renderer/stores/tabs/store.ts (3)
useSplitTabHorizontal(842-843)useSplitTabVertical(840-841)useRemoveTab(829-829)
🔇 Additional comments (7)
apps/desktop/tsconfig.json (1)
17-17: Confirm Bun typings scope for the whole desktop appSetting
"types": ["bun-types"]means only Bun’s global type package is pulled in from@types. If any of the desktop code (Electron main, Node APIs, etc.) still relies on other global type packages (e.g.node,electron), you’ll need to add them here or split out a test-only tsconfig; otherwise those globals may lose their typings.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/index.tsx (1)
57-104: Split/close wiring in GroupTabView looks consistent with the store APIThe new split handlers correctly delegate to
splitTabHorizontal/splitTabVerticalwithworkspaceId,tabId, andpath, andhandleClosePanecleanly routes pane removal throughremoveChildTabFromGroup.renderPane’s use ofMosaicWindowplusTabContentContextMenuis memoized with the right dependencies and has a safe “Tab not found” fallback.Also applies to: 106-142
apps/desktop/src/renderer/lib/shortcuts.ts (1)
10-35: Workspace split shortcuts are wired cleanly to the new handlersExtending
ShortcutHandlerswithsplitVertical/splitHorizontaland routing the"⌘D"/"⇧⌘D"entries through those handlers keeps the workspace split behavior centralized and consistent with the new tab store actions.Also applies to: 37-45, 80-99
apps/desktop/src/renderer/stores/tabs/store.ts (4)
1-2: New imports for pane splitting look appropriateImporting
MosaicBranch/MosaicNodetypes andupdateTreelocally scopes the layout-specific concerns to this store without impacting existing behavior; no issues here.
38-47: TabsState API extensions for split actions are clearThe
splitTabVertical/splitTabHorizontalsignatures (workspace-scoped, optional source tab, optional path) are well-shaped for both “active tab” and targeted pane splitting use cases and align with the rest of the store API.
131-183: splitPaneInGroup maintains group invariantsThe helper correctly:
- Locates the owning group via
parentId,- Creates a new child with the same
workspaceIdandparentId,- Uses
updateTreeto replace the leaf atpathwith a split node,- Returns a partial state update that only mutates
tabs.This keeps the group’s
layoutand children in sync; I don’t see correctness issues here.
840-843: Hook exports for split actions are consistent with existing store hooks
useSplitTabVerticalanduseSplitTabHorizontalmirror the other selector hooks and keep the public surface tidy; no concerns here.
1 participant
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.