Add modal to view hotkeys, have hotkeys work even when focusing terminals#166
Add modal to view hotkeys, have hotkeys work even when focusing terminals#166saddlepaddle merged 8 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. WalkthroughAdds a centralized HOTKEYS registry and utilities, a HotkeyModal UI and Kbd UI primitives, wires HOTKEYS into main screens and terminal initialization, forwards app hotkeys from XTerm to global handlers, and replaces prior numeric workspace/tab hotkeys with consolidated meta+1..meta+9 handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant XTerm as XTerm (terminal)
participant Helper as Terminal Helper
participant Utils as Hotkey Utils
participant Document as Document (global)
participant App as App Handler
participant Modal as HotkeyModal
User->>XTerm: press keys (e.g., Meta+1 or Meta+T)
XTerm->>Helper: keydown event (terminal focused)
Helper->>Utils: isAppHotkey(event)?
alt matches app hotkey
Utils-->>Helper: true
Helper->>Document: re-dispatch event (global)
Helper-->>XTerm: prevent terminal handling
Document->>App: global hotkey handler invoked
alt show hotkeys
App->>Modal: set open = true
Modal-->>User: render shortcuts dialog
else perform action
App->>App: execute action (switch workspace / split / toggle)
end
else not an app hotkey
Utils-->>Helper: false
Helper-->>XTerm: allow XTerm to handle input
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
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 |
![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 (2)
apps/desktop/src/shared/hotkeys.ts (1)
147-164: Consider making category order explicit.The function iterates over
Object.values(HOTKEYS), but object iteration order in JavaScript is guaranteed only for integer-keyed properties. While modern engines preserve insertion order for string keys, you might want to define an explicit category order array if display order matters.apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx (1)
5-5: Remove unused import.
HOTKEYSis imported but not used in this file. Either remove the import or use the centralized constants (e.g., iterate overHOTKEYS.JUMP_TO_WORKSPACE_*definitions) instead of dynamically generating the key string on line 31.-import { HOTKEYS } from "shared/hotkeys";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsx(1 hunks)apps/desktop/src/renderer/components/HotkeyModal/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx(2 hunks)apps/desktop/src/renderer/screens/main/index.tsx(4 hunks)apps/desktop/src/shared/hotkeys.ts(1 hunks)packages/ui/package.json(1 hunks)packages/ui/src/components/kbd.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Apply Biome formatting, linting, and import organization at the root level for all code files
Files:
packages/ui/package.jsonapps/desktop/src/renderer/components/HotkeyModal/index.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsxapps/desktop/src/shared/hotkeys.tspackages/ui/src/components/kbd.tsxapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid usinganytype; use type-safe approaches instead, unless necessary
Ensure TypeScript type checking passes across all packages using bun run typecheck
Files:
apps/desktop/src/renderer/components/HotkeyModal/index.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsxapps/desktop/src/shared/hotkeys.tspackages/ui/src/components/kbd.tsxapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
apps/**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
apps/**/*.{tsx,ts}: Structure React applications with one folder per component following the pattern: ComponentName/ComponentName.tsx with index.ts barrel export
Co-locate component dependencies (hooks, utils, constants, tests, stories) next to the file using them
Files:
apps/desktop/src/renderer/components/HotkeyModal/index.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsxapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/renderer/components/HotkeyModal/index.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsxapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/renderer/components/HotkeyModal/index.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsxapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
apps/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use one component per file; do not create multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsxapps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsxapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
apps/desktop/src/renderer/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Define keyboard shortcuts using the centralized system in apps/desktop/src/renderer/lib/shortcuts.ts with Arc Browser-inspired shortcut definitions
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsxapps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsxapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
packages/ui/**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use TailwindCSS v4 and shadcn/ui for UI component styling and design
Files:
packages/ui/src/components/kbd.tsx
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/desktop/src/renderer/**/*.tsx : Define keyboard shortcuts using the centralized system in apps/desktop/src/renderer/lib/shortcuts.ts with Arc Browser-inspired shortcut definitions
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/**/*.{tsx,ts} : Co-locate component dependencies (hooks, utils, constants, tests, stories) next to the file using them
Applied to files:
packages/ui/package.json
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/**/*.{tsx,ts} : Structure React applications with one folder per component following the pattern: ComponentName/ComponentName.tsx with index.ts barrel export
Applied to files:
packages/ui/package.json
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Please use alias as defined in `tsconfig.json` when possible
Applied to files:
packages/ui/package.json
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to **/*.{ts,tsx,js,jsx,json} : Apply Biome formatting, linting, and import organization at the root level for all code files
Applied to files:
packages/ui/package.json
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/desktop/src/renderer/**/*.tsx : Define keyboard shortcuts using the centralized system in apps/desktop/src/renderer/lib/shortcuts.ts with Arc Browser-inspired shortcut definitions
Applied to files:
packages/ui/package.jsonapps/desktop/src/renderer/components/HotkeyModal/index.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsxapps/desktop/src/shared/hotkeys.tspackages/ui/src/components/kbd.tsxapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/**/*.test.{ts,tsx} : Co-locate tests with their corresponding components (e.g., Component.test.tsx next to Component.tsx)
Applied to files:
packages/ui/package.json
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/**/*.tsx : Use one component per file; do not create multi-component files
Applied to files:
packages/ui/package.json
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to @(apps/desktop/src/renderer|apps/desktop/src/shared)/**/*.{ts,tsx} : Never import Node.js modules in Electron renderer process or shared code; use IPC for communication between main and renderer processes
Applied to files:
packages/ui/package.json
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to **/*.{ts,tsx} : Ensure TypeScript type checking passes across all packages using bun run typecheck
Applied to files:
packages/ui/package.json
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to packages/ui/**/*.{tsx,ts} : Use TailwindCSS v4 and shadcn/ui for UI component styling and design
Applied to files:
packages/ui/package.jsonpackages/ui/src/components/kbd.tsx
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/desktop/src/main/lib/*-ipcs.ts : Implement Electron IPC handlers in dedicated files under apps/desktop/src/main/lib/ (e.g., workspace-ipcs.ts, terminal-ipcs.ts)
Applied to files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/desktop/src/shared/ipc-channels.ts : Define all Electron IPC channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Applied to files:
apps/desktop/src/shared/hotkeys.ts
🧬 Code graph analysis (5)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
apps/desktop/src/shared/hotkeys.ts (1)
isAppHotkey(253-255)
apps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsx (3)
apps/desktop/src/shared/hotkeys.ts (4)
HotkeyCategory(6-11)HotkeyDefinition(13-22)formatKeysForDisplay(169-191)getHotkeysByCategory(147-164)packages/ui/src/components/kbd.tsx (2)
KbdGroup(28-28)Kbd(28-28)packages/ui/src/components/dialog.tsx (4)
Dialog(132-132)DialogContent(134-134)DialogHeader(137-137)DialogTitle(140-140)
packages/ui/src/components/kbd.tsx (1)
packages/ui/src/lib/utils.ts (1)
cn(4-6)
apps/desktop/src/renderer/screens/main/index.tsx (2)
apps/desktop/src/shared/hotkeys.ts (1)
HOTKEYS(28-140)apps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsx (1)
HotkeyModal(90-111)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx (1)
apps/desktop/src/shared/hotkeys.ts (1)
HOTKEYS(28-140)
⏰ 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 (18)
apps/desktop/src/shared/hotkeys.ts (2)
1-23: Well-structured type definitions.The type system is clean with proper use of union types for
HotkeyCategoryand a well-defined interface. Theas const satisfiespattern on the HOTKEYS object preserves literal types while ensuring type safety.
28-140: LGTM - Centralized hotkey definitions.Good organization by category with consistent structure. The
as const satisfies Record<string, HotkeyDefinition>pattern ensures type safety while preserving literal key types forHotkeyId. This aligns with the coding guidelines for centralizing keyboard shortcuts.apps/desktop/src/renderer/components/HotkeyModal/index.ts (1)
1-1: LGTM - Proper barrel export.Follows the component folder pattern with
index.tsbarrel export as per coding guidelines.packages/ui/package.json (1)
25-25: LGTM - New kbd component export.The export follows the existing pattern for UI component exports and is correctly placed in the exports map.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (2)
110-111: Good placement of shortcut forwarding setup.Setting up hotkey forwarding after terminal is opened but before fitting addons ensures shortcuts work correctly when terminal has focus.
124-141: The concern aboutisTrusted: falseis technically valid but has no practical impact in this codebase.Web search confirmed that synthetic events created via
dispatchEvent(new KeyboardEvent(...))will haveisTrusted: false. However, examining the actual codebase reveals:
No handlers check
isTrusted: All 8useHotkeyshandlers found in the codebase are simple callbacks (toggling sidebar, opening modals, splitting terminals) with no event authenticity checks.react-hotkeys-hook v5.2.1 processes synthetic events normally: The library does not auto-filter events based on
isTrusted. It will process the re-dispatched synthetic events correctly.The implementation works as intended: The function successfully forwards app hotkeys from the terminal to React components, allowing
useHotkeyshandlers to execute when expected.Since no handlers rely on
isTrusted, the synthetic nature of the re-dispatched event is irrelevant to the current implementation. The code functions correctly.packages/ui/src/components/kbd.tsx (1)
3-16: Clean implementation of Kbd component.Good use of TailwindCSS with
cnutility for class composition. The contextual styling for tooltip content is a nice touch.apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx (2)
11-11: LGTM - Centralized hotkey import.Correctly uses the
shared/hotkeystsconfig alias as per coding guidelines.
38-66: LGTM - Proper migration to centralized HOTKEYS constants.All terminal management shortcuts now use
HOTKEYS.*.keysinstead of hardcoded strings, improving maintainability and ensuring consistency with the hotkey modal display.apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx (1)
30-40: LGTM!The workspace switching logic is clean and correctly maps
meta+1-9to workspace indices. The use ofNumber(event.key)with range validation is appropriate for this use case.apps/desktop/src/renderer/screens/main/index.tsx (3)
1-13: LGTM!Imports are well-organized, and the centralized
HOTKEYSconstants are properly used as per the coding guidelines.
42-52: Verify the split direction mapping is intentional.The hotkey-to-action mapping appears inverted:
SPLIT_HORIZONTAL("Split Horizontal") callssplitTabVerticalSPLIT_VERTICAL("Split Vertical") callssplitTabHorizontalThis could be intentional if "horizontal split" means splitting side-by-side (creating a vertical divider), but the naming is confusing. Please verify this matches the expected behavior or consider renaming for clarity.
27-27: LGTM!The
HotkeyModalintegration follows the controlled component pattern correctly. The empty dependency array inuseHotkeysis appropriate sincesetHotkeyModalOpenis a stable reference fromuseState.Also applies to: 36-36, 65-65
apps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsx (5)
28-41: LGTM!
HotkeyRowis a clean presentational component. Using array index as the key is acceptable here since the keys list is static and derived from the hotkey definition.
43-65: LGTM!
HotkeySectionhandles empty categories gracefully with an early return and useshotkey.keysas a stable unique key.
67-88: LGTM!The consolidation logic is clean and improves UX by displaying workspace shortcuts as a single "⌘+1-9" entry instead of nine separate rows. The synthetic
"meta+1-9"key string is display-only and won't conflict with actual hotkey registration.
90-111: LGTM!The
HotkeyModalcomponent is well-structured as a controlled dialog. WhilegetHotkeysByCategory()is called on each render, it operates on static data, so memoization would be premature optimization.
1-111: Note on file structure.The file contains multiple components (
HotkeyRow,HotkeySection,HotkeyModal), but since onlyHotkeyModalis exported and the others are tightly-coupled internal helpers, this is an acceptable pattern. The coding guideline for "one component per file" is primarily concerned with preventing multiple public exports.
| function KbdGroup({ className, ...props }: React.ComponentProps<"div">) { | ||
| return ( | ||
| <kbd | ||
| data-slot="kbd-group" | ||
| className={cn("inline-flex items-center gap-1", className)} | ||
| {...props} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Type mismatch: props typed as <div> but renders <kbd>.
KbdGroup accepts React.ComponentProps<"div"> but renders a <kbd> element. This mismatch could cause issues if div-specific attributes are passed that aren't valid on <kbd>.
-function KbdGroup({ className, ...props }: React.ComponentProps<"div">) {
+function KbdGroup({ className, ...props }: React.ComponentProps<"kbd">) {
return (
<kbd
data-slot="kbd-group"
className={cn("inline-flex items-center gap-1", className)}
{...props}
/>
);
}📝 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.
| function KbdGroup({ className, ...props }: React.ComponentProps<"div">) { | |
| return ( | |
| <kbd | |
| data-slot="kbd-group" | |
| className={cn("inline-flex items-center gap-1", className)} | |
| {...props} | |
| /> | |
| ); | |
| } | |
| function KbdGroup({ className, ...props }: React.ComponentProps<"kbd">) { | |
| return ( | |
| <kbd | |
| data-slot="kbd-group" | |
| className={cn("inline-flex items-center gap-1", className)} | |
| {...props} | |
| /> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In packages/ui/src/components/kbd.tsx around lines 18 to 26, the component types
props as React.ComponentProps<"div"> but renders a <kbd>, causing a mismatch;
change the prop type to React.ComponentProps<"kbd"> (or
React.HTMLAttributes<HTMLElement> if broader) and keep the destructuring of
className and ...props so only attributes valid on <kbd> are allowed; update any
imports/types if needed and run typecheck.
saddlepaddle
force-pushed
the
pearl-wind-69
branch
from
6766cdf to
e20f53c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/desktop/src/shared/hotkeys.ts (1)
196-236:matchesHotkeyparsesctrlbut never validates it, and meta/ctrl are conflatedYou strip
"ctrl"out ofpartswhen derivingkey, but never track arequiresCtrlflag. At the same timehasMetais computed asevent.metaKey || event.ctrlKey, soctrl-only combos can interfere withmeta-only definitions andctrl+...hotkeys can never be expressed cleanly. This was already raised on an earlier commit; re-raising here so it gets fixed.A minimal, self-contained fix that preserves “meta = Cmd on Mac, Ctrl on Windows/Linux” while adding proper
ctrlhandling would look like:export function matchesHotkey( event: KeyboardEvent, hotkeyString: string, ): boolean { const parts = hotkeyString.toLowerCase().split("+"); const requiresMeta = parts.includes("meta"); const requiresShift = parts.includes("shift"); const requiresAlt = parts.includes("alt"); + const requiresCtrl = parts.includes("ctrl"); // Get the actual key (last part that's not a modifier) const key = parts.find((p) => !["meta", "shift", "alt", "ctrl"].includes(p)); if (!key) return false; - const hasMeta = event.metaKey || event.ctrlKey; + const hasMeta = event.metaKey; + const hasCtrl = event.ctrlKey; const hasShift = event.shiftKey; const hasAlt = event.altKey; - if (requiresMeta !== hasMeta) return false; + // "meta" maps to Cmd on Mac, Ctrl on Windows/Linux + if (requiresMeta && !event.metaKey && !event.ctrlKey) return false; + if (!requiresMeta && (event.metaKey || event.ctrlKey) && !requiresCtrl) return false; + if (requiresCtrl !== hasCtrl) return false; if (requiresShift !== hasShift) return false; if (requiresAlt !== hasAlt) return false; // Match the key const eventKey = event.key.toLowerCase(); const eventCode = event.code.toLowerCase();This keeps existing
meta+…behaviour while makingctrl+…hotkeys first-class and avoids accidental matches when extra modifiers are held.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsx (1)
68-88: Workspace jump consolidation depends on labels; consider a more structural approach
consolidateWorkspaceJumpscurrently identifies jump shortcuts via the label regex^Switch to Workspace \d$. This makes the behaviour fragile if labels are ever edited or localized, since the modal logic is now coupled to exact copy:const workspaceJumpPattern = /^Switch to Workspace \d$/; … const filtered = hotkeys.filter((h) => !workspaceJumpPattern.test(h.label)); filtered.unshift({ keys: "meta+1-9", label: "Switch to Workspace 1-9", category: "Workspace", });If possible, consider matching on something more stable, e.g.:
- A
HotkeyId(by exposing the ID alongsideHotkeyDefinitionfromgetHotkeysByCategory), or- The
keyspattern (e.g./^meta\+[1-9]$/in combination withcategory === "Workspace").That would make consolidation resilient to copy and i18n changes.
Separately, this file currently defines three React components (
HotkeyRow,HotkeySection,HotkeyModal). To align more strictly with the “one component per file” guideline forapps/**/*.tsx, you could either extract the helper components into their own files or convert them to non-component helpers (e.g. lowercase functions returning JSX).As per coding guidelines, apps/**/*.tsx should prefer one component per file.
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx (1)
29-39: Reuse centralized HOTKEYS for workspace shortcuts to avoid duplicationThe workspace shortcut handler works correctly but hardcodes the key combinations. Since
JUMP_TO_WORKSPACE_1..9are already defined inHOTKEYS, derive the key list from those definitions to prevent drift if the central map is updated:+import { HOTKEYS } from "shared/hotkeys"; … -// Workspace switching shortcuts (⌘+1-9) - combined into single hook call -const workspaceKeys = Array.from({ length: 9 }, (_, i) => `meta+${i + 1}`).join(", "); +// Workspace switching shortcuts (⌘+1-9) - derived from HOTKEYS +const workspaceKeys = [ + HOTKEYS.JUMP_TO_WORKSPACE_1.keys, + HOTKEYS.JUMP_TO_WORKSPACE_2.keys, + HOTKEYS.JUMP_TO_WORKSPACE_3.keys, + HOTKEYS.JUMP_TO_WORKSPACE_4.keys, + HOTKEYS.JUMP_TO_WORKSPACE_5.keys, + HOTKEYS.JUMP_TO_WORKSPACE_6.keys, + HOTKEYS.JUMP_TO_WORKSPACE_7.keys, + HOTKEYS.JUMP_TO_WORKSPACE_8.keys, + HOTKEYS.JUMP_TO_WORKSPACE_9.keys, +]; useHotkeys(workspaceKeys, (event) => {This ensures the handler stays synchronized with centralized hotkey definitions without manual maintenance.
📜 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 (9)
apps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsx(1 hunks)apps/desktop/src/renderer/components/HotkeyModal/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx(2 hunks)apps/desktop/src/renderer/screens/main/index.tsx(4 hunks)apps/desktop/src/shared/hotkeys.ts(1 hunks)packages/ui/package.json(1 hunks)packages/ui/src/components/kbd.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/ui/src/components/kbd.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
- packages/ui/package.json
- apps/desktop/src/renderer/components/HotkeyModal/index.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Apply Biome formatting, linting, and import organization at the root level for all code files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsxapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid usinganytype; use type-safe approaches instead, unless necessary
Ensure TypeScript type checking passes across all packages using bun run typecheck
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsxapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/index.tsx
apps/**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
apps/**/*.{tsx,ts}: Structure React applications with one folder per component following the pattern: ComponentName/ComponentName.tsx with index.ts barrel export
Co-locate component dependencies (hooks, utils, constants, tests, stories) next to the file using them
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsxapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/index.tsx
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsxapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/index.tsx
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsxapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/index.tsx
apps/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use one component per file; do not create multi-component files
Files:
apps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsxapps/desktop/src/renderer/screens/main/index.tsx
apps/desktop/src/renderer/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Define keyboard shortcuts using the centralized system in apps/desktop/src/renderer/lib/shortcuts.ts with Arc Browser-inspired shortcut definitions
Files:
apps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsxapps/desktop/src/renderer/screens/main/index.tsx
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/desktop/src/renderer/**/*.tsx : Define keyboard shortcuts using the centralized system in apps/desktop/src/renderer/lib/shortcuts.ts with Arc Browser-inspired shortcut definitions
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/desktop/src/renderer/**/*.tsx : Define keyboard shortcuts using the centralized system in apps/desktop/src/renderer/lib/shortcuts.ts with Arc Browser-inspired shortcut definitions
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsxapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/index.tsx
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/desktop/src/main/lib/*-ipcs.ts : Implement Electron IPC handlers in dedicated files under apps/desktop/src/main/lib/ (e.g., workspace-ipcs.ts, terminal-ipcs.ts)
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsxapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/index.tsx
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/desktop/src/shared/ipc-channels.ts : Define all Electron IPC channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Applied to files:
apps/desktop/src/shared/hotkeys.ts
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Please use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/desktop/src/shared/hotkeys.ts
🧬 Code graph analysis (1)
apps/desktop/src/renderer/screens/main/index.tsx (2)
apps/desktop/src/shared/hotkeys.ts (1)
HOTKEYS(28-140)apps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsx (1)
HotkeyModal(90-111)
🪛 GitHub Actions: CI
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx
[error] 28-46: Prettier formatting check failed. Run 'prettier --write' to fix code style issues in this file.
🔇 Additional comments (5)
apps/desktop/src/shared/hotkeys.ts (1)
24-191: Centralized HOTKEYS + helpers are well-structured and strongly typedThe HOTKEYS map, category typing,
getHotkeysByCategory, andformatKeysForDisplayform a clean, strongly-typed API that should scale well as you add more shortcuts. Theas const+HotkeyIdpattern is especially nice for keeping usages in sync.apps/desktop/src/renderer/screens/main/index.tsx (2)
1-5: MainScreen hotkey wiring and HotkeyModal integration look solidUsing
HOTKEYSfor global shortcuts and gating actions onisWorkspaceView/activeWorkspaceIdkeeps behaviour predictable, and theHotkeyModalstate/placement at the root of MainScreen is appropriate. This should make it easy for users to discover shortcuts without impacting existing layout logic.Also applies to: 27-37, 65-66
42-52: I'll help you verify the review comment about the SPLIT_HORIZONTAL/SPLIT_VERTICAL mapping. Let me gather the necessary information from the codebase.
<function_calls>
#!/bin/bashFirst, find the shortcuts definition file
find apps/desktop -name "shortcuts.ts" -o -name "shortcuts.js"
</function_calls>I'll search for the shortcuts and split tab implementations:
#!/bin/bash # Find shortcuts file and examine HOTKEYS definition fd -t f shortcuts \. apps/desktop/src/renderer/lib/ </parameter> </invoke> </function_calls> ```shell #!/bin/bash # Search for SPLIT_HORIZONTAL and SPLIT_VERTICAL hotkey definitions rg -n "SPLIT_HORIZONTAL|SPLIT_VERTICAL" apps/desktop/src/renderer/lib/ -A 2 -B 2 </parameter> </invoke> </function_calls> ```shell #!/bin/bash # Search for splitTabVertical and splitTabHorizontal implementations rg -n "splitTabVertical|splitTabHorizontal" apps/desktop/src/renderer/ -A 5 -B 2 --type ts --type tsx </parameter> </invoke> </function_calls> ```shell #!/bin/bash # Get more context from the main/index.tsx file cat -n apps/desktop/src/renderer/screens/main/index.tsx | head -70 | tail -40 </parameter> </invoke> </function_calls> Let me run these scripts to understand the split tab behavior: </blockquote></details> <details> <summary>apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)</summary><blockquote> `110-141`: **Terminal hotkey forwarding to app handlers is well-contained** The `setupShortcutForwarding` handler cleanly forwards only meta/ctrl keydowns that match `isAppHotkey` to `document`, and returns `false` so XTerm ignores those events. This keeps terminal input behaviour intact while letting global app shortcuts continue to work when the terminal is focused, without introducing extra listeners or cleanup complexity. </blockquote></details> <details> <summary>apps/desktop/src/renderer/components/HotkeyModal/HotkeyModal.tsx (1)</summary><blockquote> `90-110`: **HotkeyModal composition and use of shared hotkey utilities look good** The modal correctly pulls from `getHotkeysByCategory`, uses a fixed `CATEGORY_ORDER`, and renders keys via `formatKeysForDisplay` + `KbdGroup`, which keeps the UI in sync with the centralized HOTKEYS map. The props surface (`open`, `onOpenChange`) also matches typical dialog patterns, making it easy to host from `MainScreen`. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/desktop/src/shared/hotkeys.ts (1)
197-241:ctrlmodifier is parsed but never enforced; ctrl‑only hotkeys won’t work.In
matchesHotkey,"ctrl"is excluded fromkey(Line 208) but there’s norequiresCtrl/hasCtrlcheck, andhasMetais currentlyevent.metaKey || event.ctrlKey. That means:
- A hotkey defined as
"ctrl+k"will never match (becauserequiresMetais false buthasMetabecomes true when Ctrl is pressed).- There’s no way to require Ctrl separately from Meta.
This is the same latent bug called out in the earlier review and still needs a fix.
Minimal patch to distinguish Meta vs Ctrl and actually support
"ctrl+...":export function matchesHotkey( event: KeyboardEvent, hotkeyString: string, ): boolean { const parts = hotkeyString.toLowerCase().split("+"); const requiresMeta = parts.includes("meta"); const requiresShift = parts.includes("shift"); const requiresAlt = parts.includes("alt"); + const requiresCtrl = parts.includes("ctrl"); // Get the actual key (last part that's not a modifier) const key = parts.find((p) => !["meta", "shift", "alt", "ctrl"].includes(p)); if (!key) return false; - const hasMeta = event.metaKey || event.ctrlKey; + const hasMeta = event.metaKey; + const hasCtrl = event.ctrlKey; const hasShift = event.shiftKey; const hasAlt = event.altKey; - if (requiresMeta !== hasMeta) return false; + // meta maps to Cmd on macOS and Ctrl on Windows/Linux + if (requiresMeta && !event.metaKey && !event.ctrlKey) return false; + if (!requiresMeta && (event.metaKey || event.ctrlKey) && !requiresCtrl) return false; + if (requiresCtrl !== hasCtrl) return false; if (requiresShift !== hasShift) return false; if (requiresAlt !== hasAlt) return false;This keeps the “meta = Cmd/Ctrl” semantics while allowing explicit
"ctrl+..."hotkeys to behave correctly.
🧹 Nitpick comments (2)
apps/desktop/src/shared/hotkeys.ts (2)
169-192: Display mapping is good; consider OS‑awaremetaand explicit space handling.The symbol map is nice, but today
metaalways displays as ⌘ andspacewould display as" "(since onlyevent.codeis “space” while display here is string‑based). If you ever addmetaorspacebindings on Windows/Linux, consider:
- Making the
metaglyph OS‑aware (e.g., “Ctrl” on non‑macOS).- Normalizing
"space"to a visible symbol consistently (you already have"space": "␣"; ensure callers don’t pass a literal" "instead).Not urgent, but worth tracking if you tune cross‑platform UX.
246-260:findMatchingHotkey/isAppHotkeyare fine for current scale.Linear scan over
HOTKEYSis acceptable given the small set, andisAppHotkeyis a clean wrapper for the terminal forwarding path. If the hotkey set grows substantially, you could later index by normalized key/modifier tuple to avoid scanning, but that’s not necessary now.
📜 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 (2)
apps/desktop/package.json(1 hunks)apps/desktop/src/shared/hotkeys.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/shared/hotkeys.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/shared/hotkeys.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid usinganytype - use explicit types instead for type safety
Use camelCase for variable and function names following existing codebase patterns
Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Follow existing patterns and match the codebase style when writing new code
Files:
apps/desktop/src/shared/hotkeys.ts
🧠 Learnings (1)
📚 Learning: 2025-11-28T01:03:47.951Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.951Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net, etc.) in renderer process code - browser environment only
Applied to files:
apps/desktop/package.json
🔇 Additional comments (3)
apps/desktop/src/shared/hotkeys.ts (2)
6-140: Centralized HOTKEYS shape and typing look solid.Typed
HotkeyCategory,HotkeyDefinition, andHOTKEYSwithas const satisfies Record<string, HotkeyDefinition>give you nice safety, andHotkeyIdis a good derived type. This is a clean foundation for the modal and for wiring shortcuts across the app.
147-164:getHotkeysByCategoryis straightforward and type‑safe.Initialization covers all
HotkeyCategoryvariants explicitly and the grouping loop is clear; no issues here.apps/desktop/package.json (1)
53-53:dnd-coreis correctly declared and necessary — it's directly imported in the codebase.The concern is resolved.
dnd-core@^16.0.1is appropriately declared because the project directly importscreateDragDropManagerfrom it inapps/desktop/src/renderer/lib/dnd.ts. The version aligns perfectly withreact-dnd@16.0.1, andreact-dnd16.x has peer dependencyreact >= 16.14, which is compatible with React 19.1.1 used in this project. No action needed.
- Add centralized HOTKEYS constants in shared/hotkeys.ts with categories - Add HotkeyModal component (⌘+/) to display all keyboard shortcuts - Add Kbd component from shadcn/ui for displaying key combinations - Forward app hotkeys from terminal to document so useHotkeys works when terminal is focused - Update shortcut scheme: ⌘+1-9 for workspaces, ⌘+Up/Down for terminals 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
saddlepaddle
force-pushed
the
pearl-wind-69
branch
from
b1b96e9 to
9474800
Compare
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Chore
✏️ Tip: You can customize this high-level summary in your review settings.