From 987cbb1601b620e2b5413886b248d702be6d2737 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 5 Dec 2025 11:01:26 +0100 Subject: [PATCH 1/2] Add keyboard shortcuts help modal with centralized config - Add `?` hotkey to open a modal showing all available shortcuts - Create centralized shortcuts config in src/browser/lib/shortcuts.ts - Modal auto-generates from config (no manual sync needed) - Update all keyboard handlers to use matchesKey() utility - Shortcuts organized by category: Navigation, Actions, Go to Line, File Search, Tabs, Help Change-Id: I4c7d8b209ed70901f7b6e3c61c275a34207f7369 Signed-off-by: Thomas Kosiewski --- src/browser/components/app-shell.tsx | 28 ++ src/browser/components/command-palette.tsx | 6 +- .../components/keyboard-shortcuts-modal.tsx | 54 ++++ .../pr-review/useKeyboardNavigation.ts | 245 +++++++-------- src/browser/lib/shortcuts.ts | 283 ++++++++++++++++++ 5 files changed, 495 insertions(+), 121 deletions(-) create mode 100644 src/browser/components/keyboard-shortcuts-modal.tsx create mode 100644 src/browser/lib/shortcuts.ts diff --git a/src/browser/components/app-shell.tsx b/src/browser/components/app-shell.tsx index 1f80800..8a0422c 100644 --- a/src/browser/components/app-shell.tsx +++ b/src/browser/components/app-shell.tsx @@ -23,6 +23,8 @@ import { HoverCardContent, } from "../ui/hover-card"; import { version } from "../../../package.json"; +import { KeyboardShortcutsModal } from "./keyboard-shortcuts-modal"; +import { matchesKey } from "@/browser/lib/shortcuts"; // ============================================================================ // App Shell - Tab-based Layout @@ -40,6 +42,7 @@ export function AppShell() { } = useTabContext(); const params = useParams<{ owner: string; repo: string; number: string }>(); const navigate = useNavigate(); + const [shortcutsModalOpen, setShortcutsModalOpen] = useState(false); // URL is the source of truth - sync URL → Tab useEffect(() => { @@ -116,6 +119,26 @@ export function AppShell() { return () => window.removeEventListener("keydown", handleKeyDown); }, [tabs, activeTabId, handleTabSelect, closeTab]); + // Handle ? key for keyboard shortcuts modal + useEffect(() => { + const handleKeyDown = (e: KeyboardEvent) => { + const target = e.target as HTMLElement; + if ( + target.tagName === "INPUT" || + target.tagName === "TEXTAREA" || + target.isContentEditable + ) { + return; + } + if (matchesKey(e, "SHOW_SHORTCUTS")) { + e.preventDefault(); + setShortcutsModalOpen(true); + } + }; + window.addEventListener("keydown", handleKeyDown); + return () => window.removeEventListener("keydown", handleKeyDown); + }, []); + return (
{/* Native-style Tab Bar */} @@ -210,6 +233,11 @@ export function AppShell() {
)} + + ); } diff --git a/src/browser/components/command-palette.tsx b/src/browser/components/command-palette.tsx index fc16e95..ca38740 100644 --- a/src/browser/components/command-palette.tsx +++ b/src/browser/components/command-palette.tsx @@ -18,6 +18,7 @@ import { cn } from "../cn"; import { usePRReviewSelector, usePRReviewStore } from "../contexts/pr-review"; import { Keycap, KeycapGroup } from "../ui/keycap"; import type { PullRequestFile } from "@/api/types"; +import { matchesKey } from "@/browser/lib/shortcuts"; // ============================================================================ // Global Command Palette Context @@ -44,7 +45,10 @@ export function CommandPaletteProvider({ children }: { children: ReactNode }) { useEffect(() => { const handleKeyDown = (e: KeyboardEvent) => { // Ctrl+K or Ctrl+P to open command palette - if ((e.ctrlKey || e.metaKey) && (e.key === "k" || e.key === "p")) { + if ( + matchesKey(e, "OPEN_FILE_SEARCH_K") || + matchesKey(e, "OPEN_FILE_SEARCH_P") + ) { e.preventDefault(); e.stopPropagation(); setOpen((prev) => !prev); diff --git a/src/browser/components/keyboard-shortcuts-modal.tsx b/src/browser/components/keyboard-shortcuts-modal.tsx new file mode 100644 index 0000000..ff51737 --- /dev/null +++ b/src/browser/components/keyboard-shortcuts-modal.tsx @@ -0,0 +1,54 @@ +import { + Dialog, + DialogContent, + DialogHeader, + DialogTitle, + DialogDescription, +} from "@/browser/ui/dialog"; +import { KeycapGroup } from "@/browser/ui/keycap"; +import { getShortcutsByCategory } from "@/browser/lib/shortcuts"; + +interface KeyboardShortcutsModalProps { + open: boolean; + onOpenChange: (open: boolean) => void; +} + +export function KeyboardShortcutsModal({ + open, + onOpenChange, +}: KeyboardShortcutsModalProps) { + const categories = getShortcutsByCategory(); + + return ( + + + + Keyboard Shortcuts + + A list of all available keyboard shortcuts + + +
+ {categories.map(({ category, shortcuts }) => ( +
+

+ {category} +

+
+ {shortcuts.map((shortcut, idx) => ( +
+ {shortcut.description} + +
+ ))} +
+
+ ))} +
+ + + ); +} diff --git a/src/browser/contexts/pr-review/useKeyboardNavigation.ts b/src/browser/contexts/pr-review/useKeyboardNavigation.ts index a630609..073b6ab 100644 --- a/src/browser/contexts/pr-review/useKeyboardNavigation.ts +++ b/src/browser/contexts/pr-review/useKeyboardNavigation.ts @@ -1,5 +1,6 @@ import { startTransition, useEffect } from "react"; import { usePRReviewStore } from "."; +import { matchesKey } from "@/browser/lib/shortcuts"; export function useKeyboardNavigation() { const store = usePRReviewStore(); @@ -16,16 +17,14 @@ export function useKeyboardNavigation() { } // Handle Ctrl/Cmd+Arrow for jumping by 10 lines - if ( - (e.ctrlKey || e.metaKey) && - (e.key === "ArrowDown" || e.key === "ArrowUp") - ) { + if (matchesKey(e, "JUMP_UP")) { e.preventDefault(); - store.navigateLine( - e.key === "ArrowDown" ? "down" : "up", - e.shiftKey, - 10 - ); + store.navigateLine("up", e.shiftKey, 10); + return; + } + if (matchesKey(e, "JUMP_DOWN")) { + e.preventDefault(); + store.navigateLine("down", e.shiftKey, 10); return; } @@ -48,17 +47,17 @@ export function useKeyboardNavigation() { store.backspaceGotoInput(); return; } - if (e.key === "Tab") { + if (matchesKey(e, "GOTO_TOGGLE_SIDE")) { e.preventDefault(); store.toggleGotoLineSide(); return; } - if (e.key === "Enter" && state.gotoLineInput) { + if (matchesKey(e, "GOTO_EXECUTE") && state.gotoLineInput) { e.preventDefault(); store.executeGotoLine(); return; } - if (e.key === "Escape") { + if (matchesKey(e, "GOTO_EXIT")) { e.preventDefault(); store.exitGotoMode(); return; @@ -67,7 +66,10 @@ export function useKeyboardNavigation() { } // Enter to expand focused skip block - if (e.key === "Enter" && state.focusedSkipBlockIndex !== null) { + if ( + matchesKey(e, "EXPAND_SECTION") && + state.focusedSkipBlockIndex !== null + ) { e.preventDefault(); // Dispatch event to expand the skip block (handled by DiffViewer) const event = new CustomEvent("pr-review:expand-skip-block", { @@ -78,143 +80,146 @@ export function useKeyboardNavigation() { } // Arrow navigation - direct call for instant response - if (e.key === "ArrowDown") { + if (matchesKey(e, "NAVIGATE_DOWN")) { e.preventDefault(); store.navigateLine("down", e.shiftKey, 1); return; } - if (e.key === "ArrowUp") { + if (matchesKey(e, "NAVIGATE_UP")) { e.preventDefault(); store.navigateLine("up", e.shiftKey, 1); return; } // Left/Right arrows to switch between sides in split view - if (e.key === "ArrowLeft") { + if (matchesKey(e, "NAVIGATE_LEFT")) { e.preventDefault(); store.navigateSide("left"); return; } - if (e.key === "ArrowRight") { + if (matchesKey(e, "NAVIGATE_RIGHT")) { e.preventDefault(); store.navigateSide("right"); return; } // Shortcuts - switch (e.key.toLowerCase()) { - case "j": - e.preventDefault(); - // Use startTransition to allow React to interrupt rendering during rapid navigation - startTransition(() => { - store.navigateToNextUnviewedFile(); - }); - break; - case "k": - e.preventDefault(); - // Use startTransition to allow React to interrupt rendering during rapid navigation - startTransition(() => { - store.navigateToPrevUnviewedFile(); - }); - break; - case "v": - e.preventDefault(); - if (state.selectedFiles.size > 0) { - store.toggleViewedMultiple([...state.selectedFiles]); - } else if (state.selectedFile) { - store.toggleViewed(state.selectedFile); + if (matchesKey(e, "NEXT_UNVIEWED_FILE")) { + e.preventDefault(); + // Use startTransition to allow React to interrupt rendering during rapid navigation + startTransition(() => { + store.navigateToNextUnviewedFile(); + }); + return; + } + if (matchesKey(e, "PREV_UNVIEWED_FILE")) { + e.preventDefault(); + startTransition(() => { + store.navigateToPrevUnviewedFile(); + }); + return; + } + if (matchesKey(e, "TOGGLE_VIEWED")) { + e.preventDefault(); + if (state.selectedFiles.size > 0) { + store.toggleViewedMultiple([...state.selectedFiles]); + } else if (state.selectedFile) { + store.toggleViewed(state.selectedFile); + } + return; + } + if (matchesKey(e, "GOTO_LINE_MODE")) { + e.preventDefault(); + store.enterGotoMode(); + return; + } + if (matchesKey(e, "GOTO_OVERVIEW")) { + e.preventDefault(); + store.selectOverview(); + return; + } + if (matchesKey(e, "COMMENT")) { + e.preventDefault(); + store.startCommentingOnFocusedLine(); + return; + } + if (matchesKey(e, "EDIT_COMMENT")) { + if (state.focusedCommentId) { + // Check if user can edit this comment + // ADMIN and MAINTAIN can edit any comment, WRITE can only edit own comments + const commentToEdit = state.comments.find( + (c) => c.id === state.focusedCommentId + ); + const isOwnComment = + commentToEdit && state.currentUser === commentToEdit.user.login; + const canEditAny = + state.viewerPermission === "ADMIN" || + state.viewerPermission === "MAINTAIN"; + if (commentToEdit && (isOwnComment || canEditAny)) { + e.preventDefault(); + store.startEditing(state.focusedCommentId); } - break; - case "g": - e.preventDefault(); - store.enterGotoMode(); - break; - case "o": + } else if (state.focusedPendingCommentId) { + // Pending comments are always owned by current user e.preventDefault(); - store.selectOverview(); - break; - case "c": + store.startEditingPendingComment(state.focusedPendingCommentId); + } + return; + } + if (matchesKey(e, "REPLY_COMMENT")) { + if (state.focusedCommentId) { e.preventDefault(); - store.startCommentingOnFocusedLine(); - break; - case "e": - if (state.focusedCommentId) { - // Check if user can edit this comment - // ADMIN and MAINTAIN can edit any comment, WRITE can only edit own comments - const commentToEdit = state.comments.find( - (c) => c.id === state.focusedCommentId - ); - const isOwnComment = - commentToEdit && state.currentUser === commentToEdit.user.login; - const canEditAny = - state.viewerPermission === "ADMIN" || - state.viewerPermission === "MAINTAIN"; - if (commentToEdit && (isOwnComment || canEditAny)) { - e.preventDefault(); - store.startEditing(state.focusedCommentId); - } - } else if (state.focusedPendingCommentId) { - // Pending comments are always owned by current user - e.preventDefault(); - store.startEditingPendingComment(state.focusedPendingCommentId); - } - break; - case "r": - if (state.focusedCommentId) { - e.preventDefault(); - store.startReplying(state.focusedCommentId); - } - break; - case "d": - if (state.focusedCommentId) { - // Check if user can delete this comment - // ADMIN and MAINTAIN can delete any comment, WRITE can only delete own comments - const commentToDelete = state.comments.find( - (c) => c.id === state.focusedCommentId - ); - const isOwnCommentD = - commentToDelete && - state.currentUser === commentToDelete.user.login; - const canDeleteAny = - state.viewerPermission === "ADMIN" || - state.viewerPermission === "MAINTAIN"; - if (commentToDelete && (isOwnCommentD || canDeleteAny)) { - e.preventDefault(); - if ( - window.confirm("Are you sure you want to delete this comment?") - ) { - // Trigger delete via API - component handles this - const event = new CustomEvent("pr-review:delete-comment", { - detail: { commentId: state.focusedCommentId }, - }); - window.dispatchEvent(event); - } - } - } else if (state.focusedPendingCommentId) { - // Pending comments are always owned by current user + store.startReplying(state.focusedCommentId); + } + return; + } + if (matchesKey(e, "DELETE_COMMENT")) { + if (state.focusedCommentId) { + // Check if user can delete this comment + // ADMIN and MAINTAIN can delete any comment, WRITE can only delete own comments + const commentToDelete = state.comments.find( + (c) => c.id === state.focusedCommentId + ); + const isOwnCommentD = + commentToDelete && state.currentUser === commentToDelete.user.login; + const canDeleteAny = + state.viewerPermission === "ADMIN" || + state.viewerPermission === "MAINTAIN"; + if (commentToDelete && (isOwnCommentD || canDeleteAny)) { e.preventDefault(); if ( - window.confirm( - "Are you sure you want to delete this pending comment?" - ) + window.confirm("Are you sure you want to delete this comment?") ) { - const event = new CustomEvent( - "pr-review:delete-pending-comment", - { - detail: { commentId: state.focusedPendingCommentId }, - } - ); + // Trigger delete via API - component handles this + const event = new CustomEvent("pr-review:delete-comment", { + detail: { commentId: state.focusedCommentId }, + }); window.dispatchEvent(event); } } - break; - case "escape": + } else if (state.focusedPendingCommentId) { + // Pending comments are always owned by current user e.preventDefault(); - if (state.commentingOnLine) { - store.cancelCommenting(); - } else { - store.clearAllSelections(); + if ( + window.confirm( + "Are you sure you want to delete this pending comment?" + ) + ) { + const event = new CustomEvent("pr-review:delete-pending-comment", { + detail: { commentId: state.focusedPendingCommentId }, + }); + window.dispatchEvent(event); } - break; + } + return; + } + if (matchesKey(e, "CANCEL")) { + e.preventDefault(); + if (state.commentingOnLine) { + store.cancelCommenting(); + } else { + store.clearAllSelections(); + } + return; } }; diff --git a/src/browser/lib/shortcuts.ts b/src/browser/lib/shortcuts.ts new file mode 100644 index 0000000..c414faa --- /dev/null +++ b/src/browser/lib/shortcuts.ts @@ -0,0 +1,283 @@ +/** + * Centralized keyboard shortcuts configuration. + * This is the single source of truth for all keyboard shortcuts in the app. + * + * Both the keyboard handlers and the shortcuts modal read from this config. + */ + +// ============================================================================ +// Types +// ============================================================================ + +export type ShortcutCategory = + | "Navigation" + | "Actions" + | "Go to Line" + | "File Search" + | "Tabs" + | "Help"; + +export interface ShortcutDefinition { + /** The key(s) to display in the UI */ + keys: string[]; + /** Human-readable description */ + description: string; + /** Category for grouping in the modal */ + category: ShortcutCategory; + /** If true, requires Cmd (Mac) / Ctrl (other) modifier */ + withModifier?: boolean; +} + +// ============================================================================ +// Shortcuts Configuration +// ============================================================================ + +export const SHORTCUTS = { + // --------------------------------------------------------------------------- + // Navigation + // --------------------------------------------------------------------------- + NAVIGATE_UP: { + keys: ["↑"], + description: "Navigate up", + category: "Navigation", + }, + NAVIGATE_DOWN: { + keys: ["↓"], + description: "Navigate down", + category: "Navigation", + }, + NAVIGATE_LEFT: { + keys: ["←"], + description: "Switch to left side (split view)", + category: "Navigation", + }, + NAVIGATE_RIGHT: { + keys: ["→"], + description: "Switch to right side (split view)", + category: "Navigation", + }, + JUMP_UP: { + keys: ["cmd", "↑"], + description: "Jump 10 lines up", + category: "Navigation", + withModifier: true, + }, + JUMP_DOWN: { + keys: ["cmd", "↓"], + description: "Jump 10 lines down", + category: "Navigation", + withModifier: true, + }, + NEXT_UNVIEWED_FILE: { + keys: ["j"], + description: "Next unviewed file", + category: "Navigation", + }, + PREV_UNVIEWED_FILE: { + keys: ["k"], + description: "Previous unviewed file", + category: "Navigation", + }, + GOTO_LINE_MODE: { + keys: ["g"], + description: "Go to line mode", + category: "Navigation", + }, + GOTO_OVERVIEW: { + keys: ["o"], + description: "Go to overview", + category: "Navigation", + }, + + // --------------------------------------------------------------------------- + // Actions + // --------------------------------------------------------------------------- + TOGGLE_VIEWED: { + keys: ["v"], + description: "Toggle viewed status", + category: "Actions", + }, + COMMENT: { + keys: ["c"], + description: "Comment on line", + category: "Actions", + }, + EDIT_COMMENT: { + keys: ["e"], + description: "Edit comment", + category: "Actions", + }, + REPLY_COMMENT: { + keys: ["r"], + description: "Reply to comment", + category: "Actions", + }, + DELETE_COMMENT: { + keys: ["d"], + description: "Delete comment", + category: "Actions", + }, + EXPAND_SECTION: { + keys: ["enter"], + description: "Expand collapsed section", + category: "Actions", + }, + CANCEL: { + keys: ["esc"], + description: "Cancel / clear selection", + category: "Actions", + }, + + // --------------------------------------------------------------------------- + // Go to Line Mode + // --------------------------------------------------------------------------- + GOTO_INPUT_DIGITS: { + keys: ["0-9"], + description: "Enter line number", + category: "Go to Line", + }, + GOTO_TOGGLE_SIDE: { + keys: ["tab"], + description: "Toggle side", + category: "Go to Line", + }, + GOTO_EXECUTE: { + keys: ["enter"], + description: "Go to line", + category: "Go to Line", + }, + GOTO_EXIT: { + keys: ["esc"], + description: "Exit mode", + category: "Go to Line", + }, + + // --------------------------------------------------------------------------- + // File Search + // --------------------------------------------------------------------------- + OPEN_FILE_SEARCH_K: { + keys: ["cmd", "k"], + description: "Open file search", + category: "File Search", + withModifier: true, + }, + OPEN_FILE_SEARCH_P: { + keys: ["cmd", "p"], + description: "Open file search", + category: "File Search", + withModifier: true, + }, + + // --------------------------------------------------------------------------- + // Tabs + // --------------------------------------------------------------------------- + SWITCH_TAB: { + keys: ["cmd", "1-9"], + description: "Switch to tab", + category: "Tabs", + withModifier: true, + }, + CLOSE_TAB: { + keys: ["cmd", "w"], + description: "Close current tab", + category: "Tabs", + withModifier: true, + }, + + // --------------------------------------------------------------------------- + // Help + // --------------------------------------------------------------------------- + SHOW_SHORTCUTS: { + keys: ["?"], + description: "Show keyboard shortcuts", + category: "Help", + }, +} as const satisfies Record; + +export type ShortcutId = keyof typeof SHORTCUTS; + +// ============================================================================ +// Utilities +// ============================================================================ + +/** Order for displaying categories in the modal */ +const CATEGORY_ORDER: ShortcutCategory[] = [ + "Navigation", + "Actions", + "Go to Line", + "File Search", + "Tabs", + "Help", +]; + +/** + * Get all shortcuts grouped by category, ordered for display. + * Used by the keyboard shortcuts modal. + */ +export function getShortcutsByCategory(): Array<{ + category: ShortcutCategory; + shortcuts: ShortcutDefinition[]; +}> { + const map = new Map(); + + // Initialize with empty arrays in order + for (const category of CATEGORY_ORDER) { + map.set(category, []); + } + + // Group shortcuts by category + for (const shortcut of Object.values(SHORTCUTS)) { + const existing = map.get(shortcut.category); + if (existing) { + existing.push(shortcut); + } + } + + // Convert to array, filtering out empty categories + return CATEGORY_ORDER.filter((cat) => (map.get(cat)?.length ?? 0) > 0).map( + (category) => ({ + category, + shortcuts: map.get(category)!, + }) + ); +} + +/** + * Check if a keyboard event matches a shortcut's key. + * Handles modifier key requirements. + */ +export function matchesKey( + event: KeyboardEvent, + shortcutId: ShortcutId +): boolean { + const shortcut = SHORTCUTS[shortcutId]; + + // Check modifier requirement + if (shortcut.withModifier && !(event.metaKey || event.ctrlKey)) { + return false; + } + + // Get the actual key (last element if modifier is separate) + const targetKey = shortcut.keys[shortcut.keys.length - 1].toLowerCase(); + const eventKey = event.key.toLowerCase(); + + // Handle special key mappings + switch (targetKey) { + case "↑": + return eventKey === "arrowup"; + case "↓": + return eventKey === "arrowdown"; + case "←": + return eventKey === "arrowleft"; + case "→": + return eventKey === "arrowright"; + case "esc": + return eventKey === "escape"; + case "enter": + return eventKey === "enter"; + case "tab": + return eventKey === "tab"; + default: + return eventKey === targetKey; + } +} From 97ca37c9ba74832c9bb3de8fd338978e98b33e44 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 5 Dec 2025 11:05:48 +0100 Subject: [PATCH 2/2] Fix TypeScript error with optional withModifier property Change-Id: If0ba43b175821808ac897cd08e0a82f3af04c91e Signed-off-by: Thomas Kosiewski --- src/browser/lib/shortcuts.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/browser/lib/shortcuts.ts b/src/browser/lib/shortcuts.ts index c414faa..adf2798 100644 --- a/src/browser/lib/shortcuts.ts +++ b/src/browser/lib/shortcuts.ts @@ -253,7 +253,11 @@ export function matchesKey( const shortcut = SHORTCUTS[shortcutId]; // Check modifier requirement - if (shortcut.withModifier && !(event.metaKey || event.ctrlKey)) { + if ( + "withModifier" in shortcut && + shortcut.withModifier && + !(event.metaKey || event.ctrlKey) + ) { return false; }