feat(shortcuts): surface hotkey tooltips across desktop UI#6439
feat(shortcuts): surface hotkey tooltips across desktop UI#6439wolfiesch wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (21)
💤 Files with no reviewable changes (21)
📝 WalkthroughWalkthroughThis PR adds three new workspace keyboard shortcut actions ( 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 (4)
src/renderer/src/components/tab-bar/TabBar.tsx (1)
997-1028: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winEffect re-runs on every render because
togglePinnedis a fresh dependency.
togglePinnedis recreated on each render, so the[activeItem, togglePinned]dependency array never compares equal and the effect tears down and re-adds thewindowlistener every render. WraptogglePinnedinuseCallback(or readactiveItemvia a ref and drop it from deps) so the listener is registered once.♻️ Suggested approach
- const togglePinned = (item: TabItem): void => { + const togglePinned = useCallback((item: TabItem): void => { // pinTab/unpinTab mirror the change to the host for remote-server tabs. if (item.isPinned) { unpinTab(item.unifiedTabId) return } if (item.type === 'editor' && onPinFile) { onPinFile(item.data.id, item.unifiedTabId) return } pinTab(item.unifiedTabId) - } + }, [onPinFile, pinTab, unpinTab])Source: Linters/SAST tools
src/shared/keybindings.ts (1)
325-356: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the shared pin-conflict behavior.
workspace.togglePinandtab.togglePinintentionally shareworkspace-pin, and only the workspace action gets a default chord. That coupling is easy to break later unless the reason lives next to the data. Add a shortWhy:comment by the shared conflict group/defaults.Suggested comment
{ id: 'workspace.togglePin', title: 'Pin / Unpin Workspace', group: 'Global', scope: 'global', + // Why: shared with tab.togglePin so settings cannot let one pin shortcut + // silently shadow the other across scopes. conflictGroup: 'workspace-pin', searchKeywords: ['shortcut', 'global', 'workspace', 'worktree', 'pin', 'unpin'], defaultBindings: { darwin: ['Mod+Alt+P'], linux: [], win32: [] }, allowInTerminal: true },As per coding guidelines, "When writing or modifying code driven by a design doc or non-obvious constraint, add a comment explaining why the code behaves the way it does."
Also applies to: 662-670
Source: Coding guidelines
src/shared/window-shortcut-policy.ts (1)
216-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the routing rationale next to these workspace actions.
These branches now depend on the same main-process interception path as
workspace.create, but that requirement is only documented for the create action. A shortWhy:comment here would make the browser-guest/contentEditable constraint much easier to preserve.Suggested comment
+ // Why: route these workspace actions through the main-process allowlist so + // they still fire from browser guests and contentEditable surfaces. if (actionMatches('workspace.togglePin', input, platform, keybindings, options)) { return { type: 'toggleCurrentWorkspacePin' } }As per coding guidelines, "When writing or modifying code driven by a design doc or non-obvious constraint, add a comment explaining why the code behaves the way it does."
Source: Coding guidelines
src/renderer/src/components/floating-terminal/FloatingTerminalWindowControls.tsx (1)
165-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: collapse the duplicated
ShortcutKeyComboblock.Both branches of the maximized/restore ternary render the identical
ShortcutKeyCombofrommaximizeShortcut; only the label text differs. You can keep the conditional on the label and hoist the combo so it isn't duplicated.♻️ Proposed simplification
- <TooltipContent side="bottom" sideOffset={6} className="flex items-center gap-2"> - {maximized ? ( - <> - <span> - {translate( - 'auto.components.floating.terminal.FloatingTerminalWindowControls.b5686fee1e', - 'Restore' - )} - </span> - {maximizeShortcut.keys.length > 0 && ( - <ShortcutKeyCombo - keys={maximizeShortcut.keys} - doubleTap={maximizeShortcut.doubleTap} - /> - )} - </> - ) : ( - <> - <span> - {translate( - 'auto.components.floating.terminal.FloatingTerminalWindowControls.109870e023', - 'Maximize' - )} - </span> - {maximizeShortcut.keys.length > 0 && ( - <ShortcutKeyCombo - keys={maximizeShortcut.keys} - doubleTap={maximizeShortcut.doubleTap} - /> - )} - </> - )} - </TooltipContent> + <TooltipContent side="bottom" sideOffset={6} className="flex items-center gap-2"> + <span> + {maximized + ? translate( + 'auto.components.floating.terminal.FloatingTerminalWindowControls.b5686fee1e', + 'Restore' + ) + : translate( + 'auto.components.floating.terminal.FloatingTerminalWindowControls.109870e023', + 'Maximize' + )} + </span> + {maximizeShortcut.keys.length > 0 && ( + <ShortcutKeyCombo + keys={maximizeShortcut.keys} + doubleTap={maximizeShortcut.doubleTap} + /> + )} + </TooltipContent>
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9c75dfdc-40eb-4577-99b7-d8e849b87fe3
📒 Files selected for processing (38)
src/main/window/createMainWindow.tssrc/preload/api-types.tssrc/preload/index.tssrc/renderer/src/App.tsxsrc/renderer/src/components/GitHubItemDialog.tsxsrc/renderer/src/components/LinearItemDrawer.tsxsrc/renderer/src/components/PullRequestPage.tsxsrc/renderer/src/components/TaskPage.tsxsrc/renderer/src/components/Terminal.tsxsrc/renderer/src/components/TerminalSearch.tsxsrc/renderer/src/components/WorktreeJumpPalette.tsxsrc/renderer/src/components/browser-pane/BrowserPane.tsxsrc/renderer/src/components/editor/EditorPanelHeader.tsxsrc/renderer/src/components/editor/RichMarkdownToolbar.tsxsrc/renderer/src/components/editor/RichMarkdownToolbarButton.tsxsrc/renderer/src/components/floating-terminal/FloatingTerminalPanel.tsxsrc/renderer/src/components/floating-terminal/FloatingTerminalToggleButton.test.tsxsrc/renderer/src/components/floating-terminal/FloatingTerminalToggleButton.tsxsrc/renderer/src/components/floating-terminal/FloatingTerminalWindowControls.tsxsrc/renderer/src/components/right-sidebar/activity-bar-buttons.tsxsrc/renderer/src/components/right-sidebar/index.tsxsrc/renderer/src/components/right-sidebar/right-sidebar-titlebar-drag-regions.render.test.tsxsrc/renderer/src/components/sidebar/SidebarNav.test.tsxsrc/renderer/src/components/sidebar/SidebarNav.tsxsrc/renderer/src/components/sidebar/SidebarToolbar.test.tsxsrc/renderer/src/components/sidebar/SidebarToolbar.tsxsrc/renderer/src/components/sidebar/WorktreeCard.tsxsrc/renderer/src/components/sidebar/WorktreeContextMenu.tsxsrc/renderer/src/components/tab-bar/BrowserTab.tsxsrc/renderer/src/components/tab-bar/EditorFileTabContextMenu.tsxsrc/renderer/src/components/tab-bar/SortableTabContextMenu.tsxsrc/renderer/src/components/tab-bar/TabBar.tsxsrc/renderer/src/hooks/useIpcEvents.tssrc/renderer/src/web/web-preload-api.tssrc/shared/keybindings.test.tssrc/shared/keybindings.tssrc/shared/window-shortcut-policy.test.tssrc/shared/window-shortcut-policy.ts
Expose shortcut chips and tooltips on shortcut-backed affordances across app chrome, tab strip, browser, editor, sidebar, right sidebar, floating workspace, terminal search, and the command palette. Add configurable actions: workspace.togglePin (default Cmd+Opt+P), workspace.copyPath, workspace.sleep, and tab.togglePin (unbound by default). Add conflictGroup workspace-pin so Settings cannot allow silent shortcut shadowing. Route workspace pin/copy/sleep through the App/main IPC path.
wolfiesch
force-pushed
the
wolfiesch/hotkey-tooltip-expansion
branch
from
e071774 to
825ed2e
Compare
2 participants
Summary
Surfaces keyboard-shortcut affordances (tooltips and menu shortcut chips) consistently across the desktop UI. Wherever an action already had a binding, the chrome now shows the shortcut on hover or in its context/dropdown menu; where an affordance existed without a configurable action, a configurable action was added so the chip can render.
Audited surfaces: app chrome / titlebar, tab strip (editor + browser tabs), browser pane (back / forward / reload), editor path menu + rich-markdown actions, left sidebar (nav, toolbar, worktree card + context menu), right sidebar activity bar, floating workspace controls, terminal search, and the Cmd-J command palette.
New configurable actions:
workspace.togglePin— defaultCmd+Opt+P(macOS).workspace.copyPath— unbound by default.workspace.sleep— unbound by default.tab.togglePin— unbound by default.conflictGroup: workspace-pinis added toworkspace.togglePinandtab.togglePinso Settings cannot allow one to silently shadow the other. Workspace pin/copy/sleep are routed through the App/main-process IPC path rather than duplicate handlers inTerminal.tsx. Unbound actions intentionally render no chip unless the user assigns a binding.Screenshots
A representative set is below. Captures were curated to one example per surface; redundant and unclear shots were dropped.
Ships by default — these chips/tooltips appear out of the box:
Mixed (default + demo binding) — the workspace context menu shows the real default
Pinchip (workspace.togglePin,Cmd+Opt+Pon macOS) alongsideCopy Path/Sleep, which are unbound by default and shown here with a temporary demo binding:Testing
pnpm lint— not run repo-wide; ranpnpm run lint:react-doctor:changed(warnings only, in pre-existing areas).pnpm typecheck—pnpm run typecheck:webpassed; LSP reported no TypeScript diagnostics on changed files.pnpm test— not run repo-wide; ran the targeted shortcut/menu Vitest suite (8 files, 149 tests passed).pnpm build— not run.Scoped checks were chosen to cover the changed files; repo-wide
lint/test/buildwere not run in this environment. Local Node may differ from the repo's pinned Node; CI behavior is not claimed.AI Review Report
Ran an agent self-review across the diff focused on shortcut wiring and cross-platform behavior. Checked:
Cmd+Opt+Pis mac-specific and degrades to unbound elsewhere rather than hardcoding a chord.conflictGroup: workspace-pinprevents silent shadowing between workspace and tab pin./vs\hardcoding introduced.Flagged and resolved during development: duplicate pin/copy/sleep handling was consolidated onto the App/main IPC path instead of
Terminal.tsx.Security Audit
src/preload. No raw/unvalidated payloads introduced.Notes
Cmd+Opt+P) forworkspace.togglePin; other platforms start unbound.X: @wolfie_