diff --git a/packages/web/src/app/runs.$runId.files.tsx b/packages/web/src/app/runs.$runId.files.tsx index fc46a87..cb99880 100644 --- a/packages/web/src/app/runs.$runId.files.tsx +++ b/packages/web/src/app/runs.$runId.files.tsx @@ -1,18 +1,11 @@ import { createFileRoute } from "@tanstack/react-router"; -import { z } from "zod"; import { FilesPage } from "@/routes/files-page"; -const filesSearchSchema = z.object({ - scrollTo: z.string().optional(), -}); - export const Route = createFileRoute("/runs/$runId/files")({ - validateSearch: (search) => filesSearchSchema.parse(search), component: FilesRoute, }); function FilesRoute() { const { runId } = Route.useParams(); - const { scrollTo } = Route.useSearch(); - return ; + return ; } diff --git a/packages/web/src/app/runs.$runId.index.tsx b/packages/web/src/app/runs.$runId.index.tsx index 2e799c9..808aac6 100644 --- a/packages/web/src/app/runs.$runId.index.tsx +++ b/packages/web/src/app/runs.$runId.index.tsx @@ -36,7 +36,7 @@ function ChaptersRoute() {
- +
@@ -105,6 +106,7 @@ export function ChapterNavigator({ diff --git a/packages/web/src/components/chapter/file-header.tsx b/packages/web/src/components/chapter/file-header.tsx index 532aff1..b88e89c 100644 --- a/packages/web/src/components/chapter/file-header.tsx +++ b/packages/web/src/components/chapter/file-header.tsx @@ -8,11 +8,15 @@ import { } from "lucide-react"; import type { MouseEvent } from "react"; import { useCallback } from "react"; +import { ShortcutLabel } from "@/components/keyboard/shortcut-label"; import { LineCounts } from "@/components/shared/line-counts"; +import { ShortcutTooltip } from "@/components/shared/shortcut-tooltip"; import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip"; import { FILE_STATUS, type PullRequestFile } from "@/lib/diff-types"; import { FILE_STATUS_ICONS, FILE_STATUS_LABELS, FILE_STATUS_TEXT_COLORS } from "@/lib/file-status"; +import { SHORTCUT_KEY } from "@/lib/keyboard-shortcuts"; import { useIsMac } from "@/lib/use-is-mac"; +import { useShortcut } from "@/lib/use-shortcut"; import { cn } from "@/lib/utils"; function CopyableFilename({ @@ -71,6 +75,7 @@ export function FileHeader({ }: FileHeaderProps) { const isMac = useIsMac(); const altLabel = isMac ? "⌥" : "Alt"; + const { label: collapseShortcutLabel } = useShortcut(SHORTCUT_KEY.TOGGLE_FILE_COLLAPSED); const copyPath = useCallback( (path: string, label: string) => { @@ -146,7 +151,10 @@ export function FileHeader({ -

{isCollapsed ? "Expand file" : "Collapse file"}

+

+ {isCollapsed ? "Expand file" : "Collapse file"} + +

{altLabel}-click to {isCollapsed ? "expand" : "collapse"} all files

@@ -213,27 +221,26 @@ export function FileHeader({ className="relative z-10 shrink-0" /> {onToggleViewed && ( - - - - - {isViewed ? "Mark as unviewed" : "Mark as viewed"} - + + + )} {handleCommentClick && ( diff --git a/packages/web/src/components/files/file-diff-list.tsx b/packages/web/src/components/files/file-diff-list.tsx index 337a4de..94e4e74 100644 --- a/packages/web/src/components/files/file-diff-list.tsx +++ b/packages/web/src/components/files/file-diff-list.tsx @@ -5,6 +5,7 @@ import { PierreDiffViewer } from "@/components/chapter/pierre-diff-viewer"; import { findRenderedDiffLine } from "@/components/chapter/rendered-line-target"; import type { AnnotatedLineRef, DiffSide, LineRef } from "@/lib/diff-types"; import type { FileDiffEntry } from "@/lib/parse-diff"; +import { cn } from "@/lib/utils"; export interface FileDiffListHandle { scrollToFile: (filePath: string) => void; @@ -41,6 +42,8 @@ interface FileDiffListProps { onToggleViewed?: (path: string) => void; collapseState: CollapseState; chapterOverlay?: ChapterOverlayProps; + /** The keyboard-focused file, outlined to mark it as the active diff. */ + focusedFilePath?: string; } const FILE_TOP_PADDING = 16; @@ -48,7 +51,15 @@ const SCROLL_TO_LINE_POLL_MS = 100; const SCROLL_TO_LINE_TIMEOUT_MS = 3000; export const FileDiffList = forwardRef(function FileDiffList( - { entries, emptyMessage, viewedPathSet, onToggleViewed, collapseState, chapterOverlay }, + { + entries, + emptyMessage, + viewedPathSet, + onToggleViewed, + collapseState, + chapterOverlay, + focusedFilePath, + }, ref, ) { const scrollRequestRef = useRef(0); @@ -197,6 +208,7 @@ export const FileDiffList = forwardRef(fu key={entry.file.path} entry={entry} isViewed={viewedPathSet?.has(entry.file.path) ?? false} + isFocused={entry.file.path === focusedFilePath} onToggleViewed={onToggleViewed} collapseState={collapseState} chapterOverlay={chapterOverlay} @@ -209,6 +221,7 @@ export const FileDiffList = forwardRef(fu interface FileDiffSectionProps { entry: FileDiffEntry; isViewed: boolean; + isFocused: boolean; onToggleViewed?: (path: string) => void; collapseState: CollapseState; chapterOverlay?: ChapterOverlayProps; @@ -217,6 +230,7 @@ interface FileDiffSectionProps { function FileDiffSection({ entry, isViewed, + isFocused, onToggleViewed, collapseState, chapterOverlay, @@ -239,7 +253,11 @@ function FileDiffSection({ }, [onToggleViewed, file.path]); return ( -
+
{area.title} {area.locations[0] && ( - - {getFileName(area.locations[0])} → - + + {getFileName(area.locations[0])} + )}

{area.description}

@@ -103,10 +97,9 @@ function PrologueDisplay({ prologue, runId }: { prologue: Prologue; runId: strin interface PrologueSectionProps { prologue: Prologue | null | undefined; - runId: string; } -export function PrologueSection({ prologue, runId }: PrologueSectionProps) { +export function PrologueSection({ prologue }: PrologueSectionProps) { if (!prologue) return null; - return ; + return ; } diff --git a/packages/web/src/lib/__tests__/file-focus-shortcuts.test.ts b/packages/web/src/lib/__tests__/file-focus-shortcuts.test.ts new file mode 100644 index 0000000..7081b92 --- /dev/null +++ b/packages/web/src/lib/__tests__/file-focus-shortcuts.test.ts @@ -0,0 +1,250 @@ +// @vitest-environment happy-dom + +import { act, renderHook } from "@testing-library/react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { FILE_STATUS, type PullRequestFile } from "@/lib/diff-types"; +import { KEYBOARD_SHORTCUTS } from "@/lib/keyboard-shortcuts"; +import { useCurrentFile } from "@/lib/use-current-file"; +import { useFileCollapseKeys } from "@/lib/use-file-collapse-keys"; +import { useFileDiffNavigation } from "@/lib/use-file-diff-navigation"; +import { useFileKeyboardModeExitKey } from "@/lib/use-file-keyboard-mode-exit-key"; +import { useFileNavigationKeys } from "@/lib/use-file-navigation-keys"; +import { useFileViewedKey } from "@/lib/use-file-viewed-key"; + +const hotkeys = vi.hoisted(() => new Map void>()); + +vi.mock("react-hotkeys-hook", () => ({ + useHotkeys: (hotkey: string, callback: () => void) => { + hotkeys.set(hotkey, callback); + }, +})); + +function file(path: string): PullRequestFile { + return { + path, + filename: path, + status: FILE_STATUS.MODIFIED, + additions: 1, + deletions: 0, + hunks: [], + }; +} + +function press(hotkey: string) { + const callback = hotkeys.get(hotkey); + expect(callback).toBeDefined(); + callback?.(); +} + +describe("explicit file focus shortcuts", () => { + beforeEach(() => { + hotkeys.clear(); + }); + + it("keeps current file empty until explicitly set", () => { + const files = [file("a.ts"), file("b.ts")]; + const { result } = renderHook(() => useCurrentFile(files)); + + expect(result.current.currentFilePath).toBeUndefined(); + expect(result.current.keyboardFocusedFilePath).toBeUndefined(); + expect(result.current.isFileKeyboardMode).toBe(false); + }); + + it("tracks pointer-selected files without entering keyboard mode", () => { + const files = [file("a.ts"), file("b.ts")]; + const { result } = renderHook(() => useCurrentFile(files)); + + act(() => result.current.selectFile("b.ts")); + + expect(result.current.currentFilePath).toBe("b.ts"); + expect(result.current.keyboardFocusedFilePath).toBeUndefined(); + expect(result.current.isFileKeyboardMode).toBe(false); + }); + + it("tracks keyboard-focused files and exposes the diff focus target", () => { + const files = [file("a.ts"), file("b.ts")]; + const { result } = renderHook(() => useCurrentFile(files)); + + act(() => result.current.focusFile("b.ts")); + + expect(result.current.currentFilePath).toBe("b.ts"); + expect(result.current.keyboardFocusedFilePath).toBe("b.ts"); + expect(result.current.isFileKeyboardMode).toBe(true); + }); + + it("clears keyboard mode without clearing the current file", () => { + const files = [file("a.ts"), file("b.ts")]; + const { result } = renderHook(() => useCurrentFile(files)); + + act(() => result.current.focusFile("b.ts")); + act(() => result.current.clearKeyboardFocus()); + + expect(result.current.currentFilePath).toBe("b.ts"); + expect(result.current.keyboardFocusedFilePath).toBeUndefined(); + expect(result.current.isFileKeyboardMode).toBe(false); + }); + + it("clears current file and keyboard mode when the current file leaves the file set", () => { + const files = [file("a.ts"), file("b.ts")]; + const { result, rerender } = renderHook(({ currentFiles }) => useCurrentFile(currentFiles), { + initialProps: { currentFiles: files }, + }); + + act(() => result.current.focusFile("b.ts")); + expect(result.current.currentFilePath).toBe("b.ts"); + expect(result.current.isFileKeyboardMode).toBe(true); + + rerender({ currentFiles: [file("a.ts")] }); + expect(result.current.currentFilePath).toBeUndefined(); + expect(result.current.keyboardFocusedFilePath).toBeUndefined(); + expect(result.current.isFileKeyboardMode).toBe(false); + }); + + it("focuses the first file on j when no current file is selected", () => { + const onFocusFile = vi.fn(); + renderHook(() => useFileNavigationKeys([file("a.ts"), file("b.ts")], undefined, onFocusFile)); + + press(KEYBOARD_SHORTCUTS.NEXT_FILE.hotkey); + + expect(onFocusFile).toHaveBeenCalledWith("a.ts"); + }); + + it("focuses the last file on k when no current file is selected", () => { + const onFocusFile = vi.fn(); + renderHook(() => useFileNavigationKeys([file("a.ts"), file("b.ts")], undefined, onFocusFile)); + + press(KEYBOARD_SHORTCUTS.PREV_FILE.hotkey); + + expect(onFocusFile).toHaveBeenCalledWith("b.ts"); + }); + + it("moves relative to the current file for j and k", () => { + const onFocusFile = vi.fn(); + const { rerender } = renderHook( + ({ currentFilePath }) => + useFileNavigationKeys( + [file("a.ts"), file("b.ts"), file("c.ts")], + currentFilePath, + onFocusFile, + ), + { initialProps: { currentFilePath: "b.ts" } }, + ); + + press(KEYBOARD_SHORTCUTS.NEXT_FILE.hotkey); + expect(onFocusFile).toHaveBeenLastCalledWith("c.ts"); + + rerender({ currentFilePath: "b.ts" }); + press(KEYBOARD_SHORTCUTS.PREV_FILE.hotkey); + expect(onFocusFile).toHaveBeenLastCalledWith("a.ts"); + }); + + it("keeps keyboard mode target at the file list boundaries", () => { + const onFocusFile = vi.fn(); + const files = [file("a.ts"), file("b.ts")]; + const { rerender } = renderHook( + ({ currentFilePath }) => useFileNavigationKeys(files, currentFilePath, onFocusFile), + { initialProps: { currentFilePath: "a.ts" } }, + ); + + press(KEYBOARD_SHORTCUTS.PREV_FILE.hotkey); + expect(onFocusFile).toHaveBeenLastCalledWith("a.ts"); + + rerender({ currentFilePath: "b.ts" }); + press(KEYBOARD_SHORTCUTS.NEXT_FILE.hotkey); + expect(onFocusFile).toHaveBeenLastCalledWith("b.ts"); + }); + + it("marks the first file as viewed when no file is focused", () => { + const onMarkViewed = vi.fn(); + renderHook(() => useFileViewedKey([file("a.ts"), file("b.ts")], undefined, onMarkViewed)); + + press(KEYBOARD_SHORTCUTS.MARK_FILE_AS_VIEWED.hotkey); + + expect(onMarkViewed).toHaveBeenCalledWith("a.ts", "b.ts"); + }); + + it("marks the current file as viewed and reports the next file to advance to", () => { + const onMarkViewed = vi.fn(); + renderHook(() => + useFileViewedKey([file("a.ts"), file("b.ts"), file("c.ts")], "b.ts", onMarkViewed), + ); + + press(KEYBOARD_SHORTCUTS.MARK_FILE_AS_VIEWED.hotkey); + + expect(onMarkViewed).toHaveBeenCalledWith("b.ts", "c.ts"); + }); + + it("enables viewed keyboard toggling by default for writable viewed state", () => { + const onToggleViewed = vi.fn(); + renderHook(() => + useFileDiffNavigation({ + files: [file("a.ts"), file("b.ts")], + onToggleViewed, + collapse: { + collapsedFiles: new Set(), + toggleFileCollapsed: vi.fn(), + collapseAllFiles: vi.fn(), + expandAllFiles: vi.fn(), + }, + }), + ); + + press(KEYBOARD_SHORTCUTS.MARK_FILE_AS_VIEWED.hotkey); + + expect(onToggleViewed).toHaveBeenCalledWith("a.ts"); + }); + + it("reports no next file when marking the last file as viewed", () => { + const onMarkViewed = vi.fn(); + renderHook(() => useFileViewedKey([file("a.ts"), file("b.ts")], "b.ts", onMarkViewed)); + + press(KEYBOARD_SHORTCUTS.MARK_FILE_AS_VIEWED.hotkey); + + expect(onMarkViewed).toHaveBeenCalledWith("b.ts", undefined); + }); + + it("toggles the first file's collapse state when no file is focused", () => { + const onToggleCollapsed = vi.fn(); + renderHook(() => + useFileCollapseKeys( + [file("a.ts"), file("b.ts")], + undefined, + onToggleCollapsed, + vi.fn(), + vi.fn(), + false, + ), + ); + + press(KEYBOARD_SHORTCUTS.TOGGLE_FILE_COLLAPSED.hotkey); + + expect(onToggleCollapsed).toHaveBeenCalledWith("a.ts"); + }); + + it("toggles the current file's collapse state", () => { + const onToggleCollapsed = vi.fn(); + renderHook(() => + useFileCollapseKeys( + [file("a.ts"), file("b.ts")], + "b.ts", + onToggleCollapsed, + vi.fn(), + vi.fn(), + false, + ), + ); + + press(KEYBOARD_SHORTCUTS.TOGGLE_FILE_COLLAPSED.hotkey); + + expect(onToggleCollapsed).toHaveBeenCalledWith("b.ts"); + }); + + it("clears keyboard mode with Escape", () => { + const onExitKeyboardMode = vi.fn(); + renderHook(() => useFileKeyboardModeExitKey(onExitKeyboardMode)); + + press(KEYBOARD_SHORTCUTS.EXIT_FILE_KEYBOARD_MODE.hotkey); + + expect(onExitKeyboardMode).toHaveBeenCalled(); + }); +}); diff --git a/packages/web/src/lib/keyboard-shortcuts.ts b/packages/web/src/lib/keyboard-shortcuts.ts index 62dfa13..fdd33fa 100644 --- a/packages/web/src/lib/keyboard-shortcuts.ts +++ b/packages/web/src/lib/keyboard-shortcuts.ts @@ -28,6 +28,28 @@ export const KEYBOARD_SHORTCUTS = { mac: { label: "k", ariaKeyshortcuts: "K" }, nonMac: { label: "k", ariaKeyshortcuts: "K" }, }, + TOGGLE_FILE_COLLAPSED: { + hotkey: ";", + hotkeyOptions: { useKey: true }, + description: "Collapse/expand file", + group: "Navigation", + mac: { label: ";", ariaKeyshortcuts: "Semicolon" }, + nonMac: { label: ";", ariaKeyshortcuts: "Semicolon" }, + }, + TOGGLE_ALL_FILES_COLLAPSED: { + hotkey: "shift+semicolon", + description: "Collapse/expand all files", + group: "Navigation", + mac: { label: "⇧ ;", ariaKeyshortcuts: "Shift+Semicolon" }, + nonMac: { label: "Shift ;", ariaKeyshortcuts: "Shift+Semicolon" }, + }, + EXIT_FILE_KEYBOARD_MODE: { + hotkey: "esc", + description: "Hide file focus", + group: "Navigation", + mac: { label: "Esc", ariaKeyshortcuts: "Escape" }, + nonMac: { label: "Esc", ariaKeyshortcuts: "Escape" }, + }, NEXT_CHAPTER: { hotkey: "right", description: "Next chapter", @@ -42,14 +64,20 @@ export const KEYBOARD_SHORTCUTS = { mac: { label: "←", ariaKeyshortcuts: "ArrowLeft" }, nonMac: { label: "←", ariaKeyshortcuts: "ArrowLeft" }, }, - MARK_CHAPTER_AS_VIEWED: { + MARK_FILE_AS_VIEWED: { hotkey: "v", - hotkeyOptions: { useKey: true }, - description: "Toggle mark chapter as viewed", + description: "Toggle mark file as viewed", group: "Review", mac: { label: "v", ariaKeyshortcuts: "V" }, nonMac: { label: "v", ariaKeyshortcuts: "V" }, }, + MARK_CHAPTER_AS_VIEWED: { + hotkey: "shift+v", + description: "Toggle mark chapter as viewed", + group: "Review", + mac: { label: "⇧ V", ariaKeyshortcuts: "Shift+V" }, + nonMac: { label: "Shift V", ariaKeyshortcuts: "Shift+V" }, + }, COPY_BRANCH_NAME: { hotkey: "shift+c", description: "Copy branch name", diff --git a/packages/web/src/lib/use-active-file-on-scroll.ts b/packages/web/src/lib/use-active-file-on-scroll.ts deleted file mode 100644 index fe09d28..0000000 --- a/packages/web/src/lib/use-active-file-on-scroll.ts +++ /dev/null @@ -1,52 +0,0 @@ -import { useCallback, useEffect, useRef, useState } from "react"; -import type { PullRequestFile } from "./diff-types"; - -const HEADER_OFFSET = 96; - -export function useActiveFileOnScroll(files: PullRequestFile[]) { - const [activeFilePath, setActiveFilePath] = useState(); - const suppressUntilRef = useRef(0); - - const setActiveFileManually = useCallback((path: string) => { - setActiveFilePath(path); - suppressUntilRef.current = Date.now() + 100; - }, []); - - useEffect(() => { - if (files.length === 0) return; - - const handleScrollEnd = () => { - if (Date.now() < suppressUntilRef.current) return; - - const path = findActiveFile(files); - if (path) setActiveFilePath(path); - }; - - handleScrollEnd(); - - window.addEventListener("scrollend", handleScrollEnd, { passive: true }); - return () => window.removeEventListener("scrollend", handleScrollEnd); - }, [files]); - - return { activeFilePath, setActiveFileManually }; -} - -function findActiveFile(files: PullRequestFile[]): string | undefined { - let bestPath: string | undefined; - let bestDistance = Infinity; - - for (const file of files) { - const el = document.getElementById(`file-${file.path}`); - if (!el) continue; - - const rect = el.getBoundingClientRect(); - const distance = rect.top - HEADER_OFFSET; - // Element is a candidate if any part is still visible below the sticky header. - if (rect.bottom > HEADER_OFFSET && Math.abs(distance) < bestDistance) { - bestDistance = Math.abs(distance); - bestPath = file.path; - } - } - - return bestPath; -} diff --git a/packages/web/src/lib/use-current-file.ts b/packages/web/src/lib/use-current-file.ts new file mode 100644 index 0000000..fe56233 --- /dev/null +++ b/packages/web/src/lib/use-current-file.ts @@ -0,0 +1,47 @@ +import { useCallback, useEffect, useState } from "react"; +import type { PullRequestFile } from "@/lib/diff-types"; + +export function useCurrentFile(files: PullRequestFile[]) { + const [currentFilePath, setCurrentFilePath] = useState(); + const [isFileKeyboardMode, setIsFileKeyboardMode] = useState(false); + + useEffect(() => { + if (currentFilePath === undefined) return; + if (files.some((file) => file.path === currentFilePath)) return; + setCurrentFilePath(undefined); + setIsFileKeyboardMode(false); + }, [files, currentFilePath]); + + const selectFile = useCallback( + (filePath: string) => { + if (!files.some((file) => file.path === filePath)) return false; + setCurrentFilePath(filePath); + setIsFileKeyboardMode(false); + return true; + }, + [files], + ); + + const focusFile = useCallback( + (filePath: string) => { + if (!files.some((file) => file.path === filePath)) return false; + setCurrentFilePath(filePath); + setIsFileKeyboardMode(true); + return true; + }, + [files], + ); + + const clearKeyboardFocus = useCallback(() => { + setIsFileKeyboardMode(false); + }, []); + + return { + currentFilePath, + keyboardFocusedFilePath: isFileKeyboardMode ? currentFilePath : undefined, + isFileKeyboardMode, + selectFile, + focusFile, + clearKeyboardFocus, + }; +} diff --git a/packages/web/src/lib/use-file-collapse-keys.ts b/packages/web/src/lib/use-file-collapse-keys.ts new file mode 100644 index 0000000..023b6fd --- /dev/null +++ b/packages/web/src/lib/use-file-collapse-keys.ts @@ -0,0 +1,53 @@ +import { useHotkeys } from "react-hotkeys-hook"; +import type { PullRequestFile } from "@/lib/diff-types"; +import { KEYBOARD_SHORTCUTS } from "@/lib/keyboard-shortcuts"; + +/** + * Hook for collapse/expand shortcuts: + * - `;` toggles the focused file, defaulting to the first file when none is + * focused so the user doesn't have to press `j`/`k` first + * - `Shift+;` toggles all files (collapse if any expanded, expand if all collapsed) + */ +export function useFileCollapseKeys( + files: PullRequestFile[], + currentFilePath: string | undefined, + onToggleCollapsed: (filePath: string) => void, + collapseAllFiles: () => void, + expandAllFiles: () => void, + allCollapsed: boolean, + enabled = true, +) { + useHotkeys( + KEYBOARD_SHORTCUTS.TOGGLE_FILE_COLLAPSED.hotkey, + () => { + if (files.length === 0) return; + const targetPath = currentFilePath === undefined ? files[0]?.path : currentFilePath; + if (targetPath === undefined) return; + onToggleCollapsed(targetPath); + }, + { + enabled, + preventDefault: true, + enableOnFormTags: false, + ...KEYBOARD_SHORTCUTS.TOGGLE_FILE_COLLAPSED.hotkeyOptions, + }, + [files, currentFilePath, onToggleCollapsed], + ); + + useHotkeys( + KEYBOARD_SHORTCUTS.TOGGLE_ALL_FILES_COLLAPSED.hotkey, + () => { + if (allCollapsed) { + expandAllFiles(); + } else { + collapseAllFiles(); + } + }, + { + enabled, + preventDefault: true, + enableOnFormTags: false, + }, + [allCollapsed, collapseAllFiles, expandAllFiles], + ); +} diff --git a/packages/web/src/lib/use-file-diff-navigation.ts b/packages/web/src/lib/use-file-diff-navigation.ts new file mode 100644 index 0000000..ab32471 --- /dev/null +++ b/packages/web/src/lib/use-file-diff-navigation.ts @@ -0,0 +1,132 @@ +import { useCallback, useRef } from "react"; +import type { FileDiffListHandle } from "@/components/files/file-diff-list"; +import type { PullRequestFile } from "@/lib/diff-types"; +import { useCurrentFile } from "@/lib/use-current-file"; +import { useFileCollapseKeys } from "@/lib/use-file-collapse-keys"; +import { useFileKeyboardModeExitKey } from "@/lib/use-file-keyboard-mode-exit-key"; +import { useFileNavigationKeys } from "@/lib/use-file-navigation-keys"; +import { useFileViewedKey } from "@/lib/use-file-viewed-key"; + +interface FileCollapseControls { + collapsedFiles: Set; + toggleFileCollapsed: (filePath: string) => void; + collapseAllFiles: () => void; + expandAllFiles: () => void; +} + +interface UseFileDiffNavigationOptions { + /** Files in the order they should be navigated — tree order, reading order, etc. */ + files: PullRequestFile[]; + /** Marks a file viewed/unviewed. PR-wide on the Files tab, chapter-scoped on a chapter. */ + onToggleViewed: (filePath: string) => void; + /** Collapse controls for the same set of files (PR-level or chapter-scoped). */ + collapse: FileCollapseControls; + /** Base gate for every shortcut, ANDed with a non-empty file list. Defaults to true. */ + enabled?: boolean; + /** Extra gate for `v`: until viewed state is writable, toggling no-ops. Defaults to true. */ + canToggleViewed?: boolean; +} + +/** + * Wires keyboard-driven file navigation (j/k, v, ;, Shift+;, Esc) plus click-to-scroll + * for a list-of-file-diffs view, keeping focus/selection in sync with the diff viewer's + * scroll position. + * + * The Files-changed tab and the chapter detail page render the same {@link FileDiffList} + * over a list of files; they differ only in the file ordering, the viewed-state and + * collapse controls, and the enablement gates. This hook owns the shared wiring — the + * diff viewer ref and the selection state the view binds to — and takes those + * differences as inputs. + */ +export function useFileDiffNavigation({ + files, + onToggleViewed, + collapse, + enabled = true, + canToggleViewed = true, +}: UseFileDiffNavigationOptions) { + const diffListRef = useRef(null); + const { currentFilePath, keyboardFocusedFilePath, selectFile, focusFile, clearKeyboardFocus } = + useCurrentFile(files); + + const handleSelectFile = useCallback( + (filePath: string) => { + if (!selectFile(filePath)) return; + diffListRef.current?.scrollToFile(filePath); + }, + [selectFile], + ); + + const handleKeyboardFocusFile = useCallback( + (filePath: string) => { + focusFile(filePath); + diffListRef.current?.scrollToFile(filePath); + }, + [focusFile], + ); + + // `v` marks the file viewed and advances focus to the next file so the user + // can keep pressing `v`. The just-marked file stays pinned to the top so the + // view doesn't jump as files collapse. + const handleMarkViewedAndAdvance = useCallback( + (markedFilePath: string, nextFilePath: string | undefined) => { + onToggleViewed(markedFilePath); + focusFile(nextFilePath ?? markedFilePath); + diffListRef.current?.scrollToFile(markedFilePath); + }, + [onToggleViewed, focusFile], + ); + + const { collapsedFiles, toggleFileCollapsed, collapseAllFiles, expandAllFiles } = collapse; + const handleToggleCollapsed = useCallback( + (filePath: string) => { + toggleFileCollapsed(filePath); + focusFile(filePath); + }, + [toggleFileCollapsed, focusFile], + ); + + const hasFiles = files.length > 0; + const navEnabled = enabled && hasFiles; + const allCollapsed = hasFiles && collapsedFiles.size >= files.length; + + useFileNavigationKeys(files, currentFilePath, handleKeyboardFocusFile, navEnabled); + // Gate `v` on viewed-state readiness so advancing never walks past files + // without marking them. + useFileViewedKey( + files, + currentFilePath, + handleMarkViewedAndAdvance, + navEnabled && canToggleViewed, + ); + useFileCollapseKeys( + files, + currentFilePath, + handleToggleCollapsed, + collapseAllFiles, + expandAllFiles, + allCollapsed, + navEnabled, + ); + useFileKeyboardModeExitKey(clearKeyboardFocus, navEnabled); + + // Stable wrappers around the viewer's imperative scroll API so callers can drive + // line-level scrolling from effects without reaching into `diffListRef.current` + // themselves (which the dependency linter can't see through across the boundary). + const scrollToLine = useCallback((...args: Parameters) => { + diffListRef.current?.scrollToLine(...args); + }, []); + const cancelScrollToLine = useCallback(() => { + diffListRef.current?.cancelScrollToLine(); + }, []); + + return { + diffListRef, + currentFilePath, + keyboardFocusedFilePath, + selectFile, + handleSelectFile, + scrollToLine, + cancelScrollToLine, + }; +} diff --git a/packages/web/src/lib/use-file-keyboard-mode-exit-key.ts b/packages/web/src/lib/use-file-keyboard-mode-exit-key.ts new file mode 100644 index 0000000..8901e52 --- /dev/null +++ b/packages/web/src/lib/use-file-keyboard-mode-exit-key.ts @@ -0,0 +1,15 @@ +import { useHotkeys } from "react-hotkeys-hook"; +import { KEYBOARD_SHORTCUTS } from "@/lib/keyboard-shortcuts"; + +export function useFileKeyboardModeExitKey(onExitKeyboardMode: () => void, enabled = true) { + useHotkeys( + KEYBOARD_SHORTCUTS.EXIT_FILE_KEYBOARD_MODE.hotkey, + onExitKeyboardMode, + { + enabled, + preventDefault: false, + enableOnFormTags: false, + }, + [onExitKeyboardMode], + ); +} diff --git a/packages/web/src/lib/use-file-navigation-keys.ts b/packages/web/src/lib/use-file-navigation-keys.ts index 9881127..edafb4e 100644 --- a/packages/web/src/lib/use-file-navigation-keys.ts +++ b/packages/web/src/lib/use-file-navigation-keys.ts @@ -1,50 +1,64 @@ import { useHotkeys } from "react-hotkeys-hook"; +import type { PullRequestFile } from "@/lib/diff-types"; import { KEYBOARD_SHORTCUTS } from "@/lib/keyboard-shortcuts"; /** - * Bind j/k to step through `files` by path. Falls back to the first file when - * nothing is currently active. No-op at the ends of the list. + * Hook for keyboard navigation between files (j/k keys) + * Uses react-hotkeys-hook for automatic cleanup and input field exclusion */ export function useFileNavigationKeys( - files: { path: string }[], - activeFilePath: string | undefined, - onSelectFile: (filePath: string) => void, + files: PullRequestFile[], + currentFilePath: string | undefined, + onFocusFile: (filePath: string) => void, + enabled = true, ) { useHotkeys( KEYBOARD_SHORTCUTS.NEXT_FILE.hotkey, () => { - const next = stepFile(files, activeFilePath, 1); - if (next) onSelectFile(next); + if (files.length === 0) return; + + if (currentFilePath === undefined) { + const firstFile = files[0]; + if (!firstFile) return; + onFocusFile(firstFile.path); + return; + } + + const currentIndex = files.findIndex((f) => f.path === currentFilePath); + if (currentIndex === -1) return; + const targetFile = files[currentIndex + 1] ?? files[currentIndex]; + if (targetFile) onFocusFile(targetFile.path); + }, + { + enabled, + preventDefault: true, + enableOnFormTags: false, }, - { preventDefault: true, enableOnFormTags: false }, - [files, activeFilePath, onSelectFile], + [files, currentFilePath, onFocusFile], ); useHotkeys( KEYBOARD_SHORTCUTS.PREV_FILE.hotkey, () => { - const prev = stepFile(files, activeFilePath, -1); - if (prev) onSelectFile(prev); - }, - { preventDefault: true, enableOnFormTags: false }, - [files, activeFilePath, onSelectFile], - ); -} - -function stepFile( - files: { path: string }[], - activeFilePath: string | undefined, - delta: 1 | -1, -): string | undefined { - if (files.length === 0) return undefined; - const first = files[0]; - if (!first) return undefined; - if (!activeFilePath) return first.path; + if (files.length === 0) return; - const currentIndex = files.findIndex((f) => f.path === activeFilePath); - if (currentIndex === -1) return first.path; + if (currentFilePath === undefined) { + const lastFile = files.at(-1); + if (!lastFile) return; + onFocusFile(lastFile.path); + return; + } - const targetIndex = currentIndex + delta; - const target = files[targetIndex]; - return target?.path; + const currentIndex = files.findIndex((f) => f.path === currentFilePath); + if (currentIndex === -1) return; + const targetFile = files[currentIndex - 1] ?? files[currentIndex]; + if (targetFile) onFocusFile(targetFile.path); + }, + { + enabled, + preventDefault: true, + enableOnFormTags: false, + }, + [files, currentFilePath, onFocusFile], + ); } diff --git a/packages/web/src/lib/use-file-viewed-key.ts b/packages/web/src/lib/use-file-viewed-key.ts new file mode 100644 index 0000000..f1c4a69 --- /dev/null +++ b/packages/web/src/lib/use-file-viewed-key.ts @@ -0,0 +1,36 @@ +import { useHotkeys } from "react-hotkeys-hook"; +import type { PullRequestFile } from "@/lib/diff-types"; +import { KEYBOARD_SHORTCUTS } from "@/lib/keyboard-shortcuts"; + +/** + * Hook for marking a file as viewed via the `v` key. + * + * When no file is focused yet, the `v` key acts on the first file so the user + * doesn't have to press `j`/`k` first. `onMarkViewed` receives both the file + * that was marked and the file that follows it (or `undefined` at the end of + * the list) so the caller can advance focus and let the user keep pressing `v`. + */ +export function useFileViewedKey( + files: PullRequestFile[], + currentFilePath: string | undefined, + onMarkViewed: (markedFilePath: string, nextFilePath: string | undefined) => void, + enabled = true, +) { + useHotkeys( + KEYBOARD_SHORTCUTS.MARK_FILE_AS_VIEWED.hotkey, + () => { + if (files.length === 0) return; + const markedIndex = + currentFilePath === undefined ? 0 : files.findIndex((f) => f.path === currentFilePath); + const markedFile = files[markedIndex]; + if (!markedFile) return; + onMarkViewed(markedFile.path, files[markedIndex + 1]?.path); + }, + { + enabled, + preventDefault: true, + enableOnFormTags: false, + }, + [files, currentFilePath, onMarkViewed], + ); +} diff --git a/packages/web/src/routes/chapter-detail-page.tsx b/packages/web/src/routes/chapter-detail-page.tsx index 848fe9e..110e128 100644 --- a/packages/web/src/routes/chapter-detail-page.tsx +++ b/packages/web/src/routes/chapter-detail-page.tsx @@ -1,15 +1,10 @@ import type { Chapter, LineRef } from "@stagereview/types/chapters"; import type { FileContentsMap } from "@stagereview/types/diff"; import { Link, useNavigate } from "@tanstack/react-router"; -import { useCallback, useMemo, useRef, useState } from "react"; +import { useCallback, useLayoutEffect, useMemo, useRef, useState } from "react"; import { useHotkeys } from "react-hotkeys-hook"; import { ChapterSidePanel } from "@/components/chapter"; -import { - type ChapterOverlayProps, - FileDiffList, - type FileDiffListHandle, - SidebarLayout, -} from "@/components/files"; +import { type ChapterOverlayProps, FileDiffList, SidebarLayout } from "@/components/files"; import { Button } from "@/components/ui/button"; import { Skeleton } from "@/components/ui/skeleton"; import { useChapterContext } from "@/lib/chapter-context"; @@ -20,7 +15,6 @@ import { formatChapterAsMarkdown } from "@/lib/format-chapter-markdown"; import { KEYBOARD_SHORTCUTS } from "@/lib/keyboard-shortcuts"; import { groupAnnotatedLineRefsByFile, groupLineRefsByFile } from "@/lib/line-refs-by-file"; import { sortLineRefsByChapterOrder } from "@/lib/sort-line-refs"; -import { useActiveFileOnScroll } from "@/lib/use-active-file-on-scroll"; import { NAVIGATION_DIRECTION, type NavigationDirection, @@ -29,7 +23,7 @@ import { import { useChapters } from "@/lib/use-chapters"; import { useDiffPatch } from "@/lib/use-diff-patch"; import { useFileCollapseState } from "@/lib/use-file-collapse-state"; -import { useFileNavigationKeys } from "@/lib/use-file-navigation-keys"; +import { useFileDiffNavigation } from "@/lib/use-file-diff-navigation"; import { useViewState } from "@/lib/use-view-state"; interface ChapterDetailPageProps { @@ -113,21 +107,36 @@ function ChapterDetailContent({ [focusedLineRefs], ); - const diffListRef = useRef(null); - const chapterFiles = useMemo(() => chapterEntries.map((e) => e.file), [chapterEntries]); const chapterFilePaths = useMemo(() => chapterFiles.map((f) => f.path), [chapterFiles]); const chapterFilePathSet = useMemo(() => new Set(chapterFilePaths), [chapterFilePaths]); - const { activeFilePath, setActiveFileManually } = useActiveFileOnScroll(chapterFiles); + const navigate = useNavigate(); - const handleSelectFile = useCallback( - (filePath: string) => { - setActiveFileManually(filePath); - diffListRef.current?.scrollToFile(filePath); - }, - [setActiveFileManually], - ); + // After a chapter is marked viewed, advance to the next chapter — or, once + // every chapter is viewed, return to the run's chapter list. Mirrors the + // hosted app's mark-complete flow (minus the confetti the CLI omits). + const advanceAfterChapterComplete = useCallback(() => { + // markChapterViewed patches the cache on a later tick, so this snapshot + // still excludes the chapter just marked — treat it as about-to-be-viewed. + const willBeAllViewed = allChapters.every( + (ch) => ch.externalId === chapter.externalId || view.chapterIdSet.has(ch.externalId), + ); + if (willBeAllViewed) { + void navigate({ to: "/runs/$runId", params: { runId } }); + return; + } + const next = allChapters[chapterIndex + 1]; + if (next) { + void navigate({ + to: "/runs/$runId/chapters/$chapterNumber", + params: { runId, chapterNumber: String(next.order) }, + // Preserve scroll position when moving between chapters on the detail + // page (matches the hosted app); resetting would jump to the top. + resetScroll: false, + }); + } + }, [allChapters, chapter.externalId, view.chapterIdSet, chapterIndex, navigate, runId]); const handleToggleKeyChangeChecked = useCallback( (keyChangeId: string) => { @@ -139,33 +148,35 @@ function ChapterDetailContent({ const handleToggleChapterViewed = useCallback( (externalId: string) => { - if (view.chapterIdSet.has(externalId)) view.unmarkChapterViewed(externalId); - else view.markChapterViewed(externalId); + if (view.chapterIdSet.has(externalId)) { + view.unmarkChapterViewed(externalId); + return; + } + view.markChapterViewed(externalId); + // Advance only when completing the chapter currently on screen. + if (externalId === chapter.externalId) advanceAfterChapterComplete(); }, - [view], + [view, chapter.externalId, advanceAfterChapterComplete], ); const handleToggleFileViewed = useCallback( (filePath: string) => { - if (view.filePathSet.has(filePath)) view.unmarkFileViewed(filePath); - else view.markFileViewed(filePath); - }, - [view], - ); - - const handleFocusKeyChange = useCallback( - (keyChangeId: string | null, scrollTarget?: LineRef | null) => { - setFocusedKeyChangeId(keyChangeId); - if (!keyChangeId) { - diffListRef.current?.cancelScrollToLine(); + if (view.filePathSet.has(filePath)) { + view.unmarkFileViewed(filePath); return; } - const target = scrollTarget ?? findScrollTarget(chapter, keyChangeId); - if (target) { - diffListRef.current?.scrollToLine(target.filePath, target.side, target.startLine); + view.markFileViewed(filePath); + // Auto-complete the chapter once its last unviewed file is marked, so + // finishing a chapter's files also marks the chapter viewed and advances. + const willCompleteChapter = + !view.chapterIdSet.has(chapter.externalId) && + chapterFilePaths.every((path) => path === filePath || view.filePathSet.has(path)); + if (willCompleteChapter) { + view.markChapterViewed(chapter.externalId); + advanceAfterChapterComplete(); } }, - [chapter], + [view, chapter.externalId, chapterFilePaths, advanceAfterChapterComplete], ); const defaultCollapsedIds = useMemo(() => { @@ -187,9 +198,57 @@ function ChapterDetailContent({ ); useProvideCollapseActions(collapseState, chapterFilePaths.length); - useFileNavigationKeys(chapterFiles, activeFilePath, handleSelectFile); + const { + diffListRef, + currentFilePath, + keyboardFocusedFilePath, + selectFile, + handleSelectFile, + scrollToLine, + cancelScrollToLine, + } = useFileDiffNavigation({ + files: chapterFiles, + onToggleViewed: handleToggleFileViewed, + collapse: collapseState, + }); + + // On chapter change, realign the diff column to the new chapter's first file + // (matches the hosted app). `resetScroll: false` keeps the prior scroll + // offset, so when the user had scrolled down into the previous chapter the + // new first file lands above the sticky header — snap it back under the + // header. When already near the top (first file still below the header), the + // view is left alone so the chapter summary stays in sight. + const scrollAlignChapterRef = useRef(chapter.id); + useLayoutEffect(() => { + if (scrollAlignChapterRef.current === chapter.id) return; + scrollAlignChapterRef.current = chapter.id; + const firstPath = chapterFiles[0]?.path; + if (!firstPath) return; + const el = document.getElementById(`file-${firstPath}`); + if (!el) return; + const contentTop = Number.parseFloat(getComputedStyle(el).getPropertyValue("--content-top")); + if (el.getBoundingClientRect().top < contentTop) { + diffListRef.current?.scrollToFile(firstPath); + } + }, [chapter.id, chapterFiles, diffListRef]); + + const handleFocusKeyChange = useCallback( + (keyChangeId: string | null, scrollTarget?: LineRef | null) => { + setFocusedKeyChangeId(keyChangeId); + if (!keyChangeId) { + cancelScrollToLine(); + return; + } + const target = scrollTarget ?? findScrollTarget(chapter, keyChangeId); + if (target) { + scrollToLine(target.filePath, target.side, target.startLine); + // Focus the file the key change lives in so file shortcuts act on it. + selectFile(target.filePath); + } + }, + [chapter, scrollToLine, cancelScrollToLine, selectFile], + ); - const navigate = useNavigate(); const handleChapterNavigate = useCallback( (direction: NavigationDirection) => { const targetIndex = @@ -199,6 +258,9 @@ function ChapterDetailContent({ void navigate({ to: "/runs/$runId/chapters/$chapterNumber", params: { runId, chapterNumber: String(target.order) }, + // Keep scroll position when stepping chapters via the keyboard + // (matches the hosted app); the default would jump to the top. + resetScroll: false, }); }, [allChapters, chapterIndex, navigate, runId], @@ -211,7 +273,6 @@ function ChapterDetailContent({ { preventDefault: true, enableOnFormTags: false, - ...KEYBOARD_SHORTCUTS.MARK_CHAPTER_AS_VIEWED.hotkeyOptions, }, [handleToggleChapterViewed, chapter.externalId], ); @@ -249,7 +310,7 @@ function ChapterDetailContent({ chapter={chapter} chapterIndex={chapterIndex} files={chapterFiles} - focusedFilePath={activeFilePath} + focusedFilePath={currentFilePath} viewedChapterIds={view.chapterIdSet} checkedKeyChangeIds={view.keyChangeIdSet} viewedFilePathSet={view.filePathSet} @@ -272,6 +333,7 @@ function ChapterDetailContent({ onToggleViewed={handleToggleFileViewed} collapseState={collapseState} chapterOverlay={chapterOverlay} + focusedFilePath={keyboardFocusedFilePath} /> ); diff --git a/packages/web/src/routes/files-page.tsx b/packages/web/src/routes/files-page.tsx index 9e8636c..d6ea2e5 100644 --- a/packages/web/src/routes/files-page.tsx +++ b/packages/web/src/routes/files-page.tsx @@ -1,20 +1,13 @@ -import { useCallback, useEffect, useMemo, useRef } from "react"; -import { - FileDiffList, - type FileDiffListHandle, - FilePicker, - SidebarLayout, - type ViewedConfig, -} from "@/components/files"; +import { useCallback, useMemo } from "react"; +import { FileDiffList, FilePicker, SidebarLayout, type ViewedConfig } from "@/components/files"; import { Skeleton } from "@/components/ui/skeleton"; import { useProvideCollapseActions } from "@/lib/collapse-actions-context"; import { FILE_STATUS, FILE_VIEWED_STATE } from "@/lib/diff-types"; import { buildFileTree, flattenFileTree, sortFileTree } from "@/lib/file-tree"; import { type FileDiffEntry, useFileDiffEntries } from "@/lib/parse-diff"; -import { useActiveFileOnScroll } from "@/lib/use-active-file-on-scroll"; import { useDiffPatch } from "@/lib/use-diff-patch"; import { useFileCollapseState } from "@/lib/use-file-collapse-state"; -import { useFileNavigationKeys } from "@/lib/use-file-navigation-keys"; +import { useFileDiffNavigation } from "@/lib/use-file-diff-navigation"; import { useViewState } from "@/lib/use-view-state"; // The CLI has no review comments, so the file tree never renders comment badges. @@ -22,10 +15,9 @@ const NO_COMMENT_COUNTS: Map = new Map(); interface FilesPageProps { runId: string; - scrollTo?: string; } -export function FilesPage({ runId, scrollTo }: FilesPageProps) { +export function FilesPage({ runId }: FilesPageProps) { const { data: diffData, isLoading, error } = useDiffPatch(runId); const rawEntries = useFileDiffEntries(diffData?.patch, diffData?.fileContents); @@ -41,14 +33,17 @@ export function FilesPage({ runId, scrollTo }: FilesPageProps) { [filePathSet, markFileViewed, unmarkFileViewed], ); - // Deleted and viewed files start collapsed; useFileCollapseState lets the - // user override per-file while keeping these defaults reactive. + // Deleted and already-viewed files start collapsed; useFileCollapseState lets + // the user override per-file while keeping these defaults reactive. Scoped to + // the current files (a viewed path may linger in view-state for a file no + // longer in the diff) so collapsedFiles stays a subset of files. const defaultCollapsedFileIds = useMemo(() => { const ids = new Set(); for (const file of files) { - if (file.status === FILE_STATUS.DELETED) ids.add(file.path); + if (file.status === FILE_STATUS.DELETED || filePathSet.has(file.path)) { + ids.add(file.path); + } } - for (const path of filePathSet) ids.add(path); return ids; }, [files, filePathSet]); @@ -56,22 +51,12 @@ export function FilesPage({ runId, scrollTo }: FilesPageProps) { const collapseState = useFileCollapseState(defaultCollapsedFileIds, filePaths, runId); useProvideCollapseActions(collapseState, filePaths.length); - const diffListRef = useRef(null); - const { activeFilePath, setActiveFileManually } = useActiveFileOnScroll(files); - - const handleSelectFile = useCallback( - (filePath: string) => { - setActiveFileManually(filePath); - diffListRef.current?.scrollToFile(filePath); - }, - [setActiveFileManually], - ); - - useEffect(() => { - if (scrollTo && !isLoading) { - diffListRef.current?.scrollToFile(scrollTo); - } - }, [scrollTo, isLoading]); + const { diffListRef, currentFilePath, keyboardFocusedFilePath, handleSelectFile } = + useFileDiffNavigation({ + files, + onToggleViewed: handleToggleViewed, + collapse: collapseState, + }); const viewed = useMemo( () => ({ @@ -86,8 +71,6 @@ export function FilesPage({ runId, scrollTo }: FilesPageProps) { [files, filePathSet, handleToggleViewed], ); - useFileNavigationKeys(files, activeFilePath, handleSelectFile); - if (error) return ; if (isLoading || diffData === undefined) return ; @@ -96,7 +79,7 @@ export function FilesPage({ runId, scrollTo }: FilesPageProps) { sidebar={ );