feat(desktop): shortcut through terminal panes#469
Conversation
WalkthroughAdds keyboard pane navigation: two hotkeys (meta+alt+left/right), utilities to compute next/previous pane IDs in mosaic layouts (visual order, circular), and handlers in WorkspaceView that use those utils and the tabs store to move focused pane. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
There was a problem hiding this comment.
Pane navigation is implemented cleanly, but there’s a suboptimal lookup for activeTab (searching allTabs instead of workspace-filtered tabs) and a potential avoidable traversal cost in getNextPaneId/getPreviousPaneId if extractPaneIdsFromLayout is non-trivial. Neither is likely to break functionality, but the first is worth fixing for correctness/robustness and performance.
Additional notes (1)
- Performance |
apps/desktop/src/renderer/stores/tabs/utils.ts:214-243
getNextPaneId/getPreviousPaneIdcallextractPaneIdsFromLayout(layout)on every hotkey press. IfextractPaneIdsFromLayoutwalks the full mosaic tree, this is O(n) traversal per keypress. That’s likely fine for small layouts, but it can become noticeable with larger/complex layouts.
Since layout only changes when the tab layout changes, consider memoizing the extracted pane ID list (in the caller) or providing a utility that can compute the “next” pane without flattening the entire tree each time.
Summary of changes
What changed
-
Added pane focus hotkeys in
WorkspaceView:- New handlers for
HOTKEYS.PREV_PANE/HOTKEYS.NEXT_PANEthat callsetFocusedPane(activeTabId, paneId). - Introduced
activeTabmemoization to accessactiveTab.layout. - Imported new helpers
getNextPaneId/getPreviousPaneIdfromrenderer/stores/tabs/utils.
- New handlers for
-
Extended tab layout utilities (
apps/desktop/src/renderer/stores/tabs/utils.ts):- Added
getNextPaneId(layout, currentPaneId)andgetPreviousPaneId(layout, currentPaneId)which traverse pane IDs (viaextractPaneIdsFromLayout) and wrap around.
- Added
-
Registered new hotkey definitions (
apps/desktop/src/shared/hotkeys.ts):meta+alt+left→ Previous Panemeta+alt+right→ Next Pane
Links: WorkspaceView → apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx, utilities → apps/desktop/src/renderer/stores/tabs/utils.ts, hotkeys → apps/desktop/src/shared/hotkeys.ts.
There was a problem hiding this comment.
Pane navigation is implemented cleanly, but activeTab is looked up via allTabs.find(...) instead of the already workspace-filtered tabs, which is both extra work and less robust. Additionally, getNextPaneId/getPreviousPaneId implicitly depend on the traversal order of extractPaneIdsFromLayout(layout); for a user-facing hotkey, the navigation order should be explicit and aligned with the UI’s visual ordering. Addressing these will improve correctness and predictability without changing the feature’s intent.
Summary of changes
Summary of changes
-
Added pane-navigation hotkeys to
WorkspaceView:- Registers
HOTKEYS.PREV_PANE/HOTKEYS.NEXT_PANEand callssetFocusedPane(activeTabId, paneId). - Introduces
activeTabmemoization to accessactiveTab.layout. - Uses new helpers
getNextPaneId/getPreviousPaneIdfromrenderer/stores/tabs/utils.
- Registers
-
Extended tab layout utilities in
apps/desktop/src/renderer/stores/tabs/utils.ts:- Adds
getNextPaneId(layout, currentPaneId)andgetPreviousPaneId(layout, currentPaneId). - Both functions flatten pane IDs via
extractPaneIdsFromLayout(layout)and wrap at ends.
- Adds
-
Registered new hotkey definitions in
apps/desktop/src/shared/hotkeys.ts:meta+alt+left→ “Previous Pane”meta+alt+right→ “Next Pane”
Files touched:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/shared/hotkeys.ts
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsxapps/desktop/src/renderer/stores/tabs/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid using any type in TypeScript - maintain type safety unless absolutely necessary
Files:
apps/desktop/src/renderer/stores/tabs/utils.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run Biome for formatting, linting, import organization, and safe fixes at the root level using bun run lint:fix
Files:
apps/desktop/src/renderer/stores/tabs/utils.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process or shared code - use only in main process (src/main/)
Files:
apps/desktop/src/renderer/stores/tabs/utils.ts
apps/desktop/src/{main,renderer,preload}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC communication - define channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Files:
apps/desktop/src/renderer/stores/tabs/utils.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/renderer/stores/tabs/utils.ts
🧬 Code graph analysis (1)
apps/desktop/src/renderer/stores/tabs/utils.ts (1)
apps/desktop/src/shared/types/mosaic.ts (1)
MosaicNode(1-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (2)
apps/desktop/src/renderer/stores/tabs/utils.ts (2)
244-260: LGTM! Clean implementation of circular next-pane navigation.The logic correctly handles:
- Single-pane layouts (returns null)
- Unknown current pane (fallback to first)
- Wraparound from last to first
The comprehensive documentation in
getPaneIdsInVisualOrderaddresses the previous review concern about visual order semantics.
262-278: LGTM! Correct implementation of circular previous-pane navigation.The logic correctly handles:
- Single-pane layouts (returns null)
- Unknown current pane (fallback to last)
- Wraparound from first to last with proper modulo arithmetic (
+ paneIds.lengthprevents negatives)The implementation appropriately mirrors
getNextPaneIdin reverse.
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.