From 20c4ba6dfa4d30e5e95f31c50507bebd0e331bf8 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 1 Jun 2026 21:52:28 -0700 Subject: [PATCH 1/7] feat(web): vendor explicit file-focus navigation + viewed/collapse shortcuts Syncs the CLI with the monorepo's keyboard-navigation overhaul that landed after the fork: - #943: replace scroll-derived "active file" with an explicit focused-file state machine (useCurrentFile) + a shared orchestrator (useFileDiffNavigation) that owns the diff-viewer ref and wires every file shortcut. Adds a "file keyboard mode" so keyboard focus shows an outline ring while click/scroll selection only highlights the picker row. - #975: add v (mark file viewed + advance, pinning the marked file to top), ; / Shift+; (collapse file / all files), Esc (exit keyboard mode), and re-bind chapter-viewed from v to Shift+V (#910). file-header gains ShortcutTooltip hints on the viewed + collapse controls. New hooks (vendored, imports adapted to @/lib/*): use-current-file, use-file-viewed-key, use-file-collapse-keys, use-file-keyboard-mode-exit-key, use-file-diff-navigation; use-file-navigation-keys rewritten to the focus signature. The old scroll heuristic use-active-file-on-scroll is deleted. Both routes now delegate to useFileDiffNavigation; FileDiffList gains a focusedFilePath outline. Ported the monorepo's file-focus-shortcuts test (16 cases). CLI simplification: the canToggleViewed gate stays at its default (the CLI's viewed state is client-side and always writable). Typecheck, lint, web tests (105), and build all pass. --- .../src/components/chapter/file-header.tsx | 51 ++-- .../src/components/files/file-diff-list.tsx | 22 +- .../__tests__/file-focus-shortcuts.test.ts | 250 ++++++++++++++++++ packages/web/src/lib/keyboard-shortcuts.ts | 34 ++- .../web/src/lib/use-active-file-on-scroll.ts | 52 ---- packages/web/src/lib/use-current-file.ts | 47 ++++ .../web/src/lib/use-file-collapse-keys.ts | 53 ++++ .../web/src/lib/use-file-diff-navigation.ts | 132 +++++++++ .../lib/use-file-keyboard-mode-exit-key.ts | 15 ++ .../web/src/lib/use-file-navigation-keys.ts | 76 +++--- packages/web/src/lib/use-file-viewed-key.ts | 36 +++ .../web/src/routes/chapter-detail-page.tsx | 69 +++-- packages/web/src/routes/files-page.tsx | 36 +-- 13 files changed, 701 insertions(+), 172 deletions(-) create mode 100644 packages/web/src/lib/__tests__/file-focus-shortcuts.test.ts delete mode 100644 packages/web/src/lib/use-active-file-on-scroll.ts create mode 100644 packages/web/src/lib/use-current-file.ts create mode 100644 packages/web/src/lib/use-file-collapse-keys.ts create mode 100644 packages/web/src/lib/use-file-diff-navigation.ts create mode 100644 packages/web/src/lib/use-file-keyboard-mode-exit-key.ts create mode 100644 packages/web/src/lib/use-file-viewed-key.ts 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 ( -
+
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..cfe9b86 100644 --- a/packages/web/src/routes/chapter-detail-page.tsx +++ b/packages/web/src/routes/chapter-detail-page.tsx @@ -4,12 +4,7 @@ import { Link, useNavigate } from "@tanstack/react-router"; import { useCallback, 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,22 +107,10 @@ 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 handleSelectFile = useCallback( - (filePath: string) => { - setActiveFileManually(filePath); - diffListRef.current?.scrollToFile(filePath); - }, - [setActiveFileManually], - ); - const handleToggleKeyChangeChecked = useCallback( (keyChangeId: string) => { if (view.keyChangeIdSet.has(keyChangeId)) view.unmarkKeyChangeChecked(keyChangeId); @@ -153,21 +135,6 @@ function ChapterDetailContent({ [view], ); - const handleFocusKeyChange = useCallback( - (keyChangeId: string | null, scrollTarget?: LineRef | null) => { - setFocusedKeyChangeId(keyChangeId); - if (!keyChangeId) { - diffListRef.current?.cancelScrollToLine(); - return; - } - const target = scrollTarget ?? findScrollTarget(chapter, keyChangeId); - if (target) { - diffListRef.current?.scrollToLine(target.filePath, target.side, target.startLine); - } - }, - [chapter], - ); - const defaultCollapsedIds = useMemo(() => { const ids = new Set(); for (const file of chapterFiles) { @@ -187,7 +154,33 @@ function ChapterDetailContent({ ); useProvideCollapseActions(collapseState, chapterFilePaths.length); - useFileNavigationKeys(chapterFiles, activeFilePath, handleSelectFile); + const { + diffListRef, + currentFilePath, + keyboardFocusedFilePath, + handleSelectFile, + scrollToLine, + cancelScrollToLine, + } = useFileDiffNavigation({ + files: chapterFiles, + onToggleViewed: handleToggleFileViewed, + collapse: collapseState, + }); + + 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); + } + }, + [chapter, scrollToLine, cancelScrollToLine], + ); const navigate = useNavigate(); const handleChapterNavigate = useCallback( @@ -211,7 +204,6 @@ function ChapterDetailContent({ { preventDefault: true, enableOnFormTags: false, - ...KEYBOARD_SHORTCUTS.MARK_CHAPTER_AS_VIEWED.hotkeyOptions, }, [handleToggleChapterViewed, chapter.externalId], ); @@ -249,7 +241,7 @@ function ChapterDetailContent({ chapter={chapter} chapterIndex={chapterIndex} files={chapterFiles} - focusedFilePath={activeFilePath} + focusedFilePath={currentFilePath} viewedChapterIds={view.chapterIdSet} checkedKeyChangeIds={view.keyChangeIdSet} viewedFilePathSet={view.filePathSet} @@ -272,6 +264,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..588249d 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, useEffect, 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. @@ -56,22 +49,18 @@ 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], - ); + const { diffListRef, currentFilePath, keyboardFocusedFilePath, handleSelectFile } = + useFileDiffNavigation({ + files, + onToggleViewed: handleToggleViewed, + collapse: collapseState, + }); useEffect(() => { if (scrollTo && !isLoading) { diffListRef.current?.scrollToFile(scrollTo); } - }, [scrollTo, isLoading]); + }, [scrollTo, isLoading, diffListRef]); const viewed = useMemo( () => ({ @@ -86,8 +75,6 @@ export function FilesPage({ runId, scrollTo }: FilesPageProps) { [files, filePathSet, handleToggleViewed], ); - useFileNavigationKeys(files, activeFilePath, handleSelectFile); - if (error) return ; if (isLoading || diffData === undefined) return ; @@ -96,7 +83,7 @@ export function FilesPage({ runId, scrollTo }: FilesPageProps) { sidebar={ ); From 3b7e7977be4fda8bef3064d8c79536431db7f377 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Tue, 2 Jun 2026 15:54:56 -0700 Subject: [PATCH 2/7] feat(web): advance to next chapter on view + auto-mark chapter when all files viewed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mirrors the hosted app's chapter mark-complete flow (chapter-context.tsx handleMarkComplete / toggleChapterFileViewed), which the CLI lacked: - Marking a chapter viewed (Shift+V or the navigator toggle) now advances to the next chapter — or returns to the run's chapter list once every chapter is viewed. - Marking a chapter's last unviewed file now auto-marks the chapter viewed and advances, so finishing a chapter by its files behaves like Shift+V. Both routes through a shared advanceAfterChapterComplete helper. The all- viewed/next-chapter check reads the pre-mark view-state snapshot (the optimistic cache patch lands a tick later), matching the monorepo's current-snapshot logic. Confetti is intentionally omitted (CLI has none). Verified locally via stagereview show: completing a chapter's files marks it viewed and navigates to the next chapter. typecheck, lint, test (315) pass. --- .../web/src/routes/chapter-detail-page.tsx | 54 ++++++++++++++++--- 1 file changed, 47 insertions(+), 7 deletions(-) diff --git a/packages/web/src/routes/chapter-detail-page.tsx b/packages/web/src/routes/chapter-detail-page.tsx index cfe9b86..8a25d19 100644 --- a/packages/web/src/routes/chapter-detail-page.tsx +++ b/packages/web/src/routes/chapter-detail-page.tsx @@ -111,6 +111,30 @@ function ChapterDetailContent({ const chapterFilePaths = useMemo(() => chapterFiles.map((f) => f.path), [chapterFiles]); const chapterFilePathSet = useMemo(() => new Set(chapterFilePaths), [chapterFilePaths]); + const navigate = useNavigate(); + + // 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) }, + }); + } + }, [allChapters, chapter.externalId, view.chapterIdSet, chapterIndex, navigate, runId]); + const handleToggleKeyChangeChecked = useCallback( (keyChangeId: string) => { if (view.keyChangeIdSet.has(keyChangeId)) view.unmarkKeyChangeChecked(keyChangeId); @@ -121,18 +145,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); + if (view.filePathSet.has(filePath)) { + view.unmarkFileViewed(filePath); + return; + } + 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(); + } }, - [view], + [view, chapter.externalId, chapterFilePaths, advanceAfterChapterComplete], ); const defaultCollapsedIds = useMemo(() => { @@ -182,7 +223,6 @@ function ChapterDetailContent({ [chapter, scrollToLine, cancelScrollToLine], ); - const navigate = useNavigate(); const handleChapterNavigate = useCallback( (direction: NavigationDirection) => { const targetIndex = From a0d02ac8f4f0e723cd377c6ab7ba73c338790f34 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Tue, 2 Jun 2026 16:02:50 -0700 Subject: [PATCH 3/7] fix(web): preserve scroll position when navigating between chapters The chapter detail page scrolls the window, and TanStack Router defaults to resetScroll: true, so every chapter-to-chapter navigation jumped back to the top. The hosted app passes resetScroll: false for on-detail-page chapter nav (chapter-context.tsx navigateToPrev/Next/Chapter); the CLI's navigations omitted it. Add resetScroll={false} to the navigator's prev/next/jump s and to the keyboard + auto-advance navigate() calls. Navigating to the run's chapter list once all chapters are viewed keeps the default reset, matching the hosted app's mark-complete-all flow. Verified locally: scrolling down a chapter and advancing keeps the scroll position instead of jumping to the top. --- packages/web/src/components/chapter/chapter-navigator.tsx | 3 +++ packages/web/src/routes/chapter-detail-page.tsx | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/packages/web/src/components/chapter/chapter-navigator.tsx b/packages/web/src/components/chapter/chapter-navigator.tsx index 7f4334e..c5c0478 100644 --- a/packages/web/src/components/chapter/chapter-navigator.tsx +++ b/packages/web/src/components/chapter/chapter-navigator.tsx @@ -73,6 +73,7 @@ export function ChapterNavigator({ @@ -105,6 +106,7 @@ export function ChapterNavigator({ diff --git a/packages/web/src/routes/chapter-detail-page.tsx b/packages/web/src/routes/chapter-detail-page.tsx index 8a25d19..be15d81 100644 --- a/packages/web/src/routes/chapter-detail-page.tsx +++ b/packages/web/src/routes/chapter-detail-page.tsx @@ -131,6 +131,9 @@ function ChapterDetailContent({ 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]); @@ -232,6 +235,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], From a299c5e1f69db1c936d10ddcef4c74e08d9a0ba5 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Tue, 2 Jun 2026 16:34:12 -0700 Subject: [PATCH 4/7] fix(web): align to the new chapter's first file on chapter change resetScroll: false alone left the window at the prior chapter's offset, so advancing could strand the reader deep in (or past) the next chapter. The hosted app realigns the diff column to the new chapter's first file under the sticky header on chapter change, gated on the side panel being pinned (i.e. only when scrolled into the content). Port that to the CLI's window-scroll model: a useLayoutEffect keyed on chapter id scrolls to the first file when the carried-over offset left it above --content-top, and leaves the view alone when already near the top (so the chapter summary stays visible). Verified locally: scrolled to the bottom of a chapter then advanced -> the next chapter's first file snaps under the header; advancing from the top leaves the view in place. --- .../web/src/routes/chapter-detail-page.tsx | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/packages/web/src/routes/chapter-detail-page.tsx b/packages/web/src/routes/chapter-detail-page.tsx index be15d81..02f4f27 100644 --- a/packages/web/src/routes/chapter-detail-page.tsx +++ b/packages/web/src/routes/chapter-detail-page.tsx @@ -1,7 +1,7 @@ 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, SidebarLayout } from "@/components/files"; @@ -211,6 +211,26 @@ function ChapterDetailContent({ 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); From 6549860214ca7ac8499ea31a52538faae1da48c6 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Tue, 2 Jun 2026 16:51:57 -0700 Subject: [PATCH 5/7] fix(web): sync file focus on route-driven scrolls + robust collapse-all test Addresses PR review (Bugbot/codex): - Deep-link scrollTo on the Files tab now selects the linked file (not just scrolls), so the picker highlights it and j/k/v/; start from it. - Key-change jumps and the chapter-change first-file realign now select the file they scroll to, so shortcuts act on the file shown at the top instead of a stale path carried over from the previous chapter. This also makes the orchestrator's selectFile return value a live consumer (was unused). - allCollapsed now tests membership (files.every(has)) instead of size >= length, so Shift+; can't flip to expand-all when collapsedFiles carries paths outside the current file list. typecheck, lint, test (315) pass. --- packages/web/src/lib/use-file-diff-navigation.ts | 6 +++++- packages/web/src/routes/chapter-detail-page.tsx | 10 ++++++++-- packages/web/src/routes/files-page.tsx | 6 ++++-- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/packages/web/src/lib/use-file-diff-navigation.ts b/packages/web/src/lib/use-file-diff-navigation.ts index ab32471..8c44174 100644 --- a/packages/web/src/lib/use-file-diff-navigation.ts +++ b/packages/web/src/lib/use-file-diff-navigation.ts @@ -88,7 +88,11 @@ export function useFileDiffNavigation({ const hasFiles = files.length > 0; const navEnabled = enabled && hasFiles; - const allCollapsed = hasFiles && collapsedFiles.size >= files.length; + // Test membership rather than comparing sizes: `collapsedFiles` can carry + // paths that aren't in the current `files` list (e.g. viewed-state defaults + // for files outside this view), which would inflate a size comparison and + // flip Shift+; to expand-all while a visible file is still expanded. + const allCollapsed = hasFiles && files.every((file) => collapsedFiles.has(file.path)); useFileNavigationKeys(files, currentFilePath, handleKeyboardFocusFile, navEnabled); // Gate `v` on viewed-state readiness so advancing never walks past files diff --git a/packages/web/src/routes/chapter-detail-page.tsx b/packages/web/src/routes/chapter-detail-page.tsx index 02f4f27..05e8fb5 100644 --- a/packages/web/src/routes/chapter-detail-page.tsx +++ b/packages/web/src/routes/chapter-detail-page.tsx @@ -202,6 +202,7 @@ function ChapterDetailContent({ diffListRef, currentFilePath, keyboardFocusedFilePath, + selectFile, handleSelectFile, scrollToLine, cancelScrollToLine, @@ -228,8 +229,11 @@ function ChapterDetailContent({ const contentTop = Number.parseFloat(getComputedStyle(el).getPropertyValue("--content-top")); if (el.getBoundingClientRect().top < contentTop) { diffListRef.current?.scrollToFile(firstPath); + // Sync focus to the file now at the top so j/k/v/; act on it rather than + // a stale path carried over from the previous chapter. + selectFile(firstPath); } - }, [chapter.id, chapterFiles, diffListRef]); + }, [chapter.id, chapterFiles, diffListRef, selectFile]); const handleFocusKeyChange = useCallback( (keyChangeId: string | null, scrollTarget?: LineRef | null) => { @@ -241,9 +245,11 @@ function ChapterDetailContent({ 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], + [chapter, scrollToLine, cancelScrollToLine, selectFile], ); const handleChapterNavigate = useCallback( diff --git a/packages/web/src/routes/files-page.tsx b/packages/web/src/routes/files-page.tsx index 588249d..774a459 100644 --- a/packages/web/src/routes/files-page.tsx +++ b/packages/web/src/routes/files-page.tsx @@ -58,9 +58,11 @@ export function FilesPage({ runId, scrollTo }: FilesPageProps) { useEffect(() => { if (scrollTo && !isLoading) { - diffListRef.current?.scrollToFile(scrollTo); + // Select (not just scroll) so the picker highlights the linked file and + // j/k/v/; start from it instead of defaulting to the first/last file. + handleSelectFile(scrollTo); } - }, [scrollTo, isLoading, diffListRef]); + }, [scrollTo, isLoading, handleSelectFile]); const viewed = useMemo( () => ({ From 88059aca4af6cdb84a7955e8437515f4ae7dad30 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Tue, 2 Jun 2026 17:24:03 -0700 Subject: [PATCH 6/7] refactor(web): align collapse-state + chapter realign with hosted monorepo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Match the hosted app rather than diverging: - allCollapsed: revert to collapsedFiles.size >= files.length (the monorepo's test). Instead of a membership test, scope the Files-tab collapse defaults to the current files (only mark deleted/viewed paths that are in the diff), so collapsedFiles stays a subset of files exactly like the monorepo guarantees. - Chapter-change realign now only scrolls to the first file (no selectFile), matching the monorepo's reset effect which repositions without touching file-focus state. Key-change jumps still selectFile(target.filePath) — that DOES match the monorepo (its chapter route effect selects the focused key change's file). typecheck, lint, test (315) pass. --- packages/web/src/lib/use-file-diff-navigation.ts | 6 +----- packages/web/src/routes/chapter-detail-page.tsx | 5 +---- packages/web/src/routes/files-page.tsx | 11 +++++++---- 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/packages/web/src/lib/use-file-diff-navigation.ts b/packages/web/src/lib/use-file-diff-navigation.ts index 8c44174..ab32471 100644 --- a/packages/web/src/lib/use-file-diff-navigation.ts +++ b/packages/web/src/lib/use-file-diff-navigation.ts @@ -88,11 +88,7 @@ export function useFileDiffNavigation({ const hasFiles = files.length > 0; const navEnabled = enabled && hasFiles; - // Test membership rather than comparing sizes: `collapsedFiles` can carry - // paths that aren't in the current `files` list (e.g. viewed-state defaults - // for files outside this view), which would inflate a size comparison and - // flip Shift+; to expand-all while a visible file is still expanded. - const allCollapsed = hasFiles && files.every((file) => collapsedFiles.has(file.path)); + const allCollapsed = hasFiles && collapsedFiles.size >= files.length; useFileNavigationKeys(files, currentFilePath, handleKeyboardFocusFile, navEnabled); // Gate `v` on viewed-state readiness so advancing never walks past files diff --git a/packages/web/src/routes/chapter-detail-page.tsx b/packages/web/src/routes/chapter-detail-page.tsx index 05e8fb5..110e128 100644 --- a/packages/web/src/routes/chapter-detail-page.tsx +++ b/packages/web/src/routes/chapter-detail-page.tsx @@ -229,11 +229,8 @@ function ChapterDetailContent({ const contentTop = Number.parseFloat(getComputedStyle(el).getPropertyValue("--content-top")); if (el.getBoundingClientRect().top < contentTop) { diffListRef.current?.scrollToFile(firstPath); - // Sync focus to the file now at the top so j/k/v/; act on it rather than - // a stale path carried over from the previous chapter. - selectFile(firstPath); } - }, [chapter.id, chapterFiles, diffListRef, selectFile]); + }, [chapter.id, chapterFiles, diffListRef]); const handleFocusKeyChange = useCallback( (keyChangeId: string | null, scrollTarget?: LineRef | null) => { diff --git a/packages/web/src/routes/files-page.tsx b/packages/web/src/routes/files-page.tsx index 774a459..eb089ba 100644 --- a/packages/web/src/routes/files-page.tsx +++ b/packages/web/src/routes/files-page.tsx @@ -34,14 +34,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]); From 7280c6da6028f5ea9a0c54f3d49bccda8248a75c Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Tue, 2 Jun 2026 17:36:56 -0700 Subject: [PATCH 7/7] refactor(web): remove scrollTo deep-link, matching the hosted monorepo The hosted app has no scrollTo flow on the Files tab, so drop the CLI's: - files route: remove the scrollTo search param + validateSearch schema - FilesPage: remove the scrollTo prop and the select-on-scrollTo effect - prologue Review Focus: the concern location is now plain text (matching pull-request-summary-section), not a link into the Files tab PrologueSection no longer needs runId. typecheck, lint, test (315), build pass. --- packages/web/src/app/runs.$runId.files.tsx | 9 +-------- packages/web/src/app/runs.$runId.index.tsx | 2 +- .../components/prologue/prologue-section.tsx | 19 ++++++------------- packages/web/src/routes/files-page.tsx | 13 ++----------- 4 files changed, 10 insertions(+), 33 deletions(-) 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() {
- +
{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/routes/files-page.tsx b/packages/web/src/routes/files-page.tsx index eb089ba..d6ea2e5 100644 --- a/packages/web/src/routes/files-page.tsx +++ b/packages/web/src/routes/files-page.tsx @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useMemo } from "react"; +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"; @@ -15,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); @@ -59,14 +58,6 @@ export function FilesPage({ runId, scrollTo }: FilesPageProps) { collapse: collapseState, }); - useEffect(() => { - if (scrollTo && !isLoading) { - // Select (not just scroll) so the picker highlights the linked file and - // j/k/v/; start from it instead of defaulting to the first/last file. - handleSelectFile(scrollTo); - } - }, [scrollTo, isLoading, handleSelectFile]); - const viewed = useMemo( () => ({ stateByPath: new Map(