fix(desktop): show platform-specific hotkey symbols#348
fix(desktop): show platform-specific hotkey symbols#348saddlepaddle wants to merge 1 commit intomainfrom
Conversation
WalkthroughReplaces runtime hotkey formatting with precomputed display arrays in the shared HOTKEYS config and updates UI components to read Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used📓 Path-based instructions (6)apps/desktop/**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
Files:
apps/desktop/**/*.{ts,tsx}📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx,json}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/components/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/desktop/src/renderer/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-11-24T21:33:13.267ZApplied to files:
🧬 Code graph analysis (1)apps/desktop/src/shared/hotkeys.ts (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). (6)
🔇 Additional comments (11)
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 |
saddlepaddle
force-pushed
the
fix/hotkey-display-platform-specific
branch
from
e6188f3 to
65681b4
Compare
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 Preview resources have been processed for cleanup |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsx (1)
20-21: Consider exportingHotkeyWithDisplayfromshared/hotkeys.tsto avoid duplication.The
HotkeyWithDisplaytype mirrors the shape defined inhotkeys.tsvia thesatisfiesclause. Exporting this type from the shared module would centralize the definition and ensure consistency.// In apps/desktop/src/shared/hotkeys.ts, add: +export type HotkeyWithDisplay = HotkeyDefinition & { display: string[] }; // Then in KeyboardShortcutsSettings.tsx: -type HotkeyWithDisplay = HotkeyDefinition & { display: string[] }; +import { + getHotkeysByCategory, + HOTKEYS, + type HotkeyCategory, + type HotkeyDefinition, + type HotkeyWithDisplay, +} from "shared/hotkeys";apps/desktop/src/shared/hotkeys.ts (1)
6-6: Consider using path alias if available.The coding guidelines recommend using aliases as defined in
tsconfig.jsonwhen possible. If an alias exists for the shared directory (e.g.,@shared/constants), consider using it instead of the relative path.Apply this diff if an alias is available:
-import { PLATFORM } from "./constants"; +import { PLATFORM } from "@shared/constants";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsx(6 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/HelpMenu/HelpMenu.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/SettingsButton/SettingsButton.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/SidebarControl.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/EmptyTabView.tsx(2 hunks)apps/desktop/src/shared/hotkeys.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
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/TopBar/SettingsButton/SettingsButton.tsxapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/EmptyTabView.tsxapps/desktop/src/renderer/screens/main/components/TopBar/HelpMenu/HelpMenu.tsxapps/desktop/src/renderer/screens/main/components/TopBar/SidebarControl.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.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/TopBar/SettingsButton/SettingsButton.tsxapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/EmptyTabView.tsxapps/desktop/src/renderer/screens/main/components/TopBar/HelpMenu/HelpMenu.tsxapps/desktop/src/renderer/screens/main/components/TopBar/SidebarControl.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsx
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting, running at root level for speed
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/SettingsButton/SettingsButton.tsxapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/EmptyTabView.tsxapps/desktop/src/renderer/screens/main/components/TopBar/HelpMenu/HelpMenu.tsxapps/desktop/src/renderer/screens/main/components/TopBar/SidebarControl.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/SettingsButton/SettingsButton.tsxapps/desktop/src/shared/hotkeys.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/EmptyTabView.tsxapps/desktop/src/renderer/screens/main/components/TopBar/HelpMenu/HelpMenu.tsxapps/desktop/src/renderer/screens/main/components/TopBar/SidebarControl.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsx
**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/components/**/*.{ts,tsx}: Structure project folders as one folder per component with PascalCase naming (ComponentName/ComponentName.tsx + index.ts barrel export)
Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Use one component per file (no multi-component files)
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/SettingsButton/SettingsButton.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/EmptyTabView.tsxapps/desktop/src/renderer/screens/main/components/TopBar/HelpMenu/HelpMenu.tsxapps/desktop/src/renderer/screens/main/components/TopBar/SidebarControl.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Call IPC methods from renderer process using window.ipcRenderer.invoke with type-safe object parameters
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/SettingsButton/SettingsButton.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/EmptyTabView.tsxapps/desktop/src/renderer/screens/main/components/TopBar/HelpMenu/HelpMenu.tsxapps/desktop/src/renderer/screens/main/components/TopBar/SidebarControl.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Please use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsx
🧬 Code graph analysis (4)
apps/desktop/src/renderer/screens/main/components/TopBar/SettingsButton/SettingsButton.tsx (1)
apps/desktop/src/shared/hotkeys.ts (1)
HOTKEYS(65-205)
apps/desktop/src/renderer/screens/main/components/TopBar/HelpMenu/HelpMenu.tsx (1)
apps/desktop/src/shared/hotkeys.ts (1)
HOTKEYS(65-205)
apps/desktop/src/renderer/screens/main/components/TopBar/SidebarControl.tsx (1)
apps/desktop/src/shared/hotkeys.ts (1)
HOTKEYS(65-205)
apps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsx (1)
apps/desktop/src/shared/hotkeys.ts (2)
HotkeyDefinition(50-59)HOTKEYS(65-205)
⏰ 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). (6)
- GitHub Check: Deploy Web
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Docs
- GitHub Check: Deploy API
- GitHub Check: Build
🔇 Additional comments (10)
apps/desktop/src/renderer/screens/main/components/TopBar/SidebarControl.tsx (1)
32-34: LGTM!Clean refactor that aligns with the data-driven hotkey display approach. The import uses the correct alias as per
tsconfig.json.One minor note: using
keyas the React key assumes display arrays never contain duplicate entries. This is a safe assumption for typical hotkey combinations.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/EmptyTabView.tsx (1)
3-5: LGTM!The refactor correctly uses
shortcut.displaydirectly from the HOTKEYS configuration. The iteration pattern is consistent with other components in this PR.Also applies to: 20-22
apps/desktop/src/renderer/screens/main/components/TopBar/HelpMenu/HelpMenu.tsx (1)
19-19: LGTM!Clean simplification that removes the
formatKeysForDisplaydependency and directly uses the pre-computeddisplayarray fromHOTKEYS.SHOW_HOTKEYS.Also applies to: 76-78
apps/desktop/src/renderer/screens/main/components/TopBar/SettingsButton/SettingsButton.tsx (1)
5-5: LGTM!Consistent refactor that aligns with the other components. The Settings button correctly displays the
SHOW_HOTKEYSshortcut in its tooltip.Also applies to: 26-28
apps/desktop/src/renderer/screens/main/components/SettingsView/KeyboardShortcutsSettings.tsx (2)
59-66: LGTM!Good approach to extract the platform-specific meta key symbol from an existing hotkey's display array. This ensures the consolidated "1-9" shortcut displays correctly across all platforms without hardcoding symbols.
78-80: No action needed. The type safety is correct:getHotkeysByCategory()returnsRecord<HotkeyCategory, HotkeyWithDisplay[]>, andhotkeysByCategory[category]properly resolves toHotkeyWithDisplay[], which matches the expected parameter type forconsolidateWorkspaceJumps(hotkeys: HotkeyWithDisplay[]). Thedisplayfield is guaranteed by theHotkeyWithDisplaytype.apps/desktop/src/shared/hotkeys.ts (4)
30-41: LGTM! Clean pre-computation design.The
formatKeyshelper andhotkeywrapper provide an elegant solution for pre-computing display strings at module load time. The generic type preservation inhotkey<T>ensures type safety while augmenting the definition with thedisplayfield.
65-205: Excellent use ofas const satisfiespattern.The consistent wrapping of all hotkey definitions with
hotkey()ensures every entry gets a pre-computeddisplayfield. The type assertionas const satisfies Record<string, HotkeyDefinition & { display: string[] }>at line 205 provides both immutability and type safety, guaranteeing that all entries conform to the expected shape at compile time.
209-231: Type-safe category grouping.The introduction of
HotkeyWithDisplayand the updatedgetHotkeysByCategorysignature correctly reflect that all hotkeys now include the pre-computeddisplayfield. The implementation maintains type safety while preserving the existing grouping logic.
9-13: PLATFORM constants are properly available at module load time.PLATFORM is statically defined in
src/shared/constants.tsusingprocess.platformcomparisons, which are synchronously available during module evaluation. No async initialization is required, and MODIFIER_MAP can safely be computed at module load time.
saddlepaddle
force-pushed
the
fix/hotkey-display-platform-specific
branch
from
65681b4 to
3fb3448
Compare
- Mac: ⌘ ⌃ ⌥ ⇧ - Windows: Win Ctrl Alt Shift - Linux: Super Ctrl Alt Shift Refactored hotkey system to pre-compute display symbols at build time using PLATFORM constants instead of runtime navigator parsing. This: - Adds `.display` property to all HOTKEYS entries - Removes `formatKeysForDisplay` export (now internal) - Removes `useIsMac` hook from components - Centralizes all platform logic in hotkeys.ts 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
saddlepaddle
force-pushed
the
fix/hotkey-display-platform-specific
branch
from
3fb3448 to
188c470
Compare
|
Works on Linux! 
|
|
Thanks for checking! Closing this one in favor of one with shared credit at #353 |
2 participants
Summary
.displayat build time usingPLATFORMconstantsformatKeysForDisplayexport anduseIsMachook - consuming code now just usesHOTKEYS.X.displayhotkeys.tsReplaces #346 with a cleaner implementation that uses existing infrastructure (
PLATFORMconstants) instead of runtimenavigator.userAgentparsing.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.