From e1e1db7b80eac2f8d23c0076482a048e7f08f5f3 Mon Sep 17 00:00:00 2001 From: Nathan Arthur Date: Mon, 29 Jun 2026 16:52:16 -0500 Subject: [PATCH 1/5] feat(review): collapse and expand files like a GitHub diff Long agent changesets force you to scroll past files you've already read. This adds GitHub-style per-file collapse: `x` collapses the selected file to its header, `Shift+X` collapses or expands every file, and the chevron in each file header toggles it by mouse. Collapse is a pure derivation: a collapsed file is swapped for a zero-hunk placeholder variant in buildReviewState, so the existing empty-file render, geometry, and hunk-cursor paths handle it with no new renderer flags. State is session-only and reconciled across watch reloads alongside gap expansion, so there is no on-disk persistence layer to gate by repo root or validate. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/collapse-files.md | 5 ++ src/core/types.ts | 3 + src/ui/App.tsx | 12 ++++ src/ui/AppHost.interactions.test.tsx | 3 +- src/ui/components/chrome/HelpDialog.tsx | 1 + src/ui/components/panes/DiffFileHeaderRow.tsx | 31 ++++++++- src/ui/components/panes/DiffPane.tsx | 19 ++++++ src/ui/components/panes/DiffSection.tsx | 5 ++ src/ui/components/ui-components.test.tsx | 5 +- src/ui/diff/renderRows.tsx | 4 ++ src/ui/hooks/useAppKeyboardShortcuts.ts | 23 +++++++ src/ui/hooks/useReviewController.test.tsx | 60 +++++++++++++++++ src/ui/hooks/useReviewController.ts | 45 +++++++++++++ src/ui/lib/appMenus.ts | 17 +++++ src/ui/lib/fileCollapse.test.ts | 47 +++++++++++++ src/ui/lib/fileCollapse.ts | 66 +++++++++++++++++++ src/ui/lib/reviewState.ts | 12 +++- src/ui/lib/ui-lib.test.ts | 2 + test/pty/collapse.test.ts | 51 ++++++++++++++ 19 files changed, 404 insertions(+), 7 deletions(-) create mode 100644 .changeset/collapse-files.md create mode 100644 src/ui/lib/fileCollapse.test.ts create mode 100644 src/ui/lib/fileCollapse.ts create mode 100644 test/pty/collapse.test.ts diff --git a/.changeset/collapse-files.md b/.changeset/collapse-files.md new file mode 100644 index 00000000..33431a8e --- /dev/null +++ b/.changeset/collapse-files.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": minor +--- + +Collapse and expand files in the review stream like a GitHub diff. Press `x` to collapse the selected file to its header, `Shift+X` to collapse or expand every file, or click the chevron in any file header. Collapse state is session-only and survives watch-mode reloads. diff --git a/src/core/types.ts b/src/core/types.ts index 4c6c5c95..5c8649c8 100644 --- a/src/core/types.ts +++ b/src/core/types.ts @@ -58,6 +58,9 @@ export interface DiffFile { isBinary?: boolean; isTooLarge?: boolean; statsTruncated?: boolean; + // Set on the placeholder variant when the user has collapsed this file in the + // review stream. Drives the collapsed-body message and the header chevron. + isCollapsed?: boolean; // Optional capability for fetching the file's full text on either side. // Loaders attach this when source content is reachable; absent when not. sourceFetcher?: FileSourceFetcher; diff --git a/src/ui/App.tsx b/src/ui/App.tsx index 8b5b0985..3b781c83 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -372,6 +372,11 @@ export function App({ setShowAgentNotes((current) => !current); }; + /** Collapse or expand the currently selected file in the review stream. */ + const toggleSelectedFileCollapsed = () => { + review.toggleFileCollapsed(review.selectedFileId); + }; + /** Toggle line-number gutters without changing the diff content itself. */ const toggleLineNumbers = () => { setShowLineNumbers((current) => !current); @@ -758,6 +763,8 @@ export function App({ toggleCopyDecorations, toggleAgentNotes, toggleFocusArea, + toggleSelectedFileCollapsed, + toggleAllFilesCollapsed: review.toggleAllFilesCollapsed, openAgentSkill, toggleHelp, toggleHunkHeaders, @@ -790,6 +797,8 @@ export function App({ renderSidebar, toggleAgentNotes, toggleFocusArea, + toggleSelectedFileCollapsed, + review.toggleAllFilesCollapsed, toggleHelp, toggleHunkHeaders, toggleLineNumbers, @@ -850,6 +859,8 @@ export function App({ toggleAgentNotes, toggleFocusArea, toggleGapForSelectedHunk: review.toggleSelectedHunkGap, + toggleSelectedFileCollapsed, + toggleAllFilesCollapsed: review.toggleAllFilesCollapsed, toggleHelp, toggleHunkHeaders, toggleLineNumbers, @@ -1047,6 +1058,7 @@ export function App({ onCopyFeedback={showTransientNotice} onSelectFile={jumpToFile} onToggleGap={review.toggleGap} + onToggleCollapse={review.toggleFileCollapsed} onViewportCenteredHunkChange={(fileId, hunkIndex) => review.selectHunk(fileId, hunkIndex, { preserveViewport: true }) } diff --git a/src/ui/AppHost.interactions.test.tsx b/src/ui/AppHost.interactions.test.tsx index 8dba4a6e..ffa7f957 100644 --- a/src/ui/AppHost.interactions.test.tsx +++ b/src/ui/AppHost.interactions.test.tsx @@ -532,7 +532,8 @@ function firstCrossFileHunkNavigationHeader(frame: string) { return ( frame .split("\n") - .map((line) => line.trim()) + // Strip the leading collapse chevron so header matching stays filename-first. + .map((line) => line.trim().replace(/^[▸▾]\s*/, "")) .find((line) => line.startsWith("long-file.txt") || line.startsWith("short-file.ts")) ?? "" ); } diff --git a/src/ui/components/chrome/HelpDialog.tsx b/src/ui/components/chrome/HelpDialog.tsx index 4e3d1835..e524c0bb 100644 --- a/src/ui/components/chrome/HelpDialog.tsx +++ b/src/ui/components/chrome/HelpDialog.tsx @@ -48,6 +48,7 @@ export function HelpDialog({ ["a", "toggle AI notes"], ["z", "toggle unchanged context"], ["l / w / m / M", "lines / wrap / metadata / menu"], + ["x / X", "collapse file / all files"], ["e", "open file in $EDITOR"], ], }, diff --git a/src/ui/components/panes/DiffFileHeaderRow.tsx b/src/ui/components/panes/DiffFileHeaderRow.tsx index 81b135bd..0f7cd144 100644 --- a/src/ui/components/panes/DiffFileHeaderRow.tsx +++ b/src/ui/components/panes/DiffFileHeaderRow.tsx @@ -1,3 +1,4 @@ +import type { MouseEvent as TuiMouseEvent } from "@opentui/core"; import type { DiffFile } from "../../../core/types"; import { fileLabelParts } from "../../lib/files"; import { fitText } from "../../lib/text"; @@ -8,20 +9,32 @@ interface DiffFileHeaderRowProps { headerLabelWidth: number; headerStatsWidth: number; theme: AppTheme; + collapsed?: boolean; onSelect?: () => void; + onToggleCollapse?: () => void; } +// Disclosure chevrons mirror GitHub's collapse affordance: ▸ when collapsed, +// ▾ when expanded. The trailing space keeps the filename aligned either way. +const COLLAPSE_CHEVRON = "▸ "; +const EXPAND_CHEVRON = "▾ "; + /** Render one file header row in the review stream or sticky overlay. */ export function DiffFileHeaderRow({ file, headerLabelWidth, headerStatsWidth, theme, + collapsed = false, onSelect, + onToggleCollapse, }: DiffFileHeaderRowProps) { const additionsText = `+${file.stats.additions}${file.statsTruncated ? "+" : ""}`; const deletionsText = `-${file.stats.deletions}`; const { filename, stateLabel } = fileLabelParts(file); + const chevron = collapsed ? COLLAPSE_CHEVRON : EXPAND_CHEVRON; + // The chevron consumes header width; reserve it so the filename doesn't overflow. + const labelWidth = Math.max(1, headerLabelWidth - chevron.length - (stateLabel?.length ?? 0)); return ( {/* Clicking the file header jumps the main stream selection without collapsing to a single-file view. */} - - {fitText(filename, Math.max(1, headerLabelWidth - (stateLabel?.length ?? 0)))} - + {/* The chevron toggles collapse on its own; stopping propagation keeps the surrounding header click as a plain select. */} + { + event.stopPropagation(); + onToggleCollapse(); + } + : undefined + } + > + {chevron} + + {fitText(filename, labelWidth)} {stateLabel && {stateLabel}} {}, onSelectFile, onToggleGap = NOOP_TOGGLE_GAP, + onToggleCollapse, onViewportCenteredHunkChange, }: { codeHorizontalOffset?: number; @@ -267,6 +268,7 @@ export function DiffPane({ onScrollCodeHorizontally?: (delta: number) => void; onSelectFile: (fileId: string) => void; onToggleGap?: (fileId: string, gapKey: string) => void; + onToggleCollapse?: (fileId: string) => void; onViewportCenteredHunkChange?: (fileId: string, hunkIndex: number) => void; }) { const renderTopChrome = showTopChrome ?? !pagerMode; @@ -315,6 +317,20 @@ export function DiffPane({ return callback; }, []); + // Same latest-ref + cached-closure pattern as selectFileCallback so collapsing + // a file from its header chevron never invalidates memoized sections. + const onToggleCollapseRef = useRef(onToggleCollapse); + onToggleCollapseRef.current = onToggleCollapse; + const toggleCollapseCallbacksRef = useRef(new Map void>()); + const toggleCollapseCallback = useCallback((fileId: string) => { + let callback = toggleCollapseCallbacksRef.current.get(fileId); + if (!callback) { + callback = () => onToggleCollapseRef.current?.(fileId); + toggleCollapseCallbacksRef.current.set(fileId, callback); + } + return callback; + }, []); + // Add-note row handlers are cached per file so mounted DiffSections keep a stable prop identity, // while the ref indirection ensures clicks still use the latest App/review callback after hunk // navigation changes the selected-file defaults upstream. @@ -1759,7 +1775,9 @@ export function DiffPane({ headerLabelWidth={headerLabelWidth} headerStatsWidth={headerStatsWidth} theme={theme} + collapsed={pinnedHeaderFile.isCollapsed ?? false} onSelect={() => onSelectFile(pinnedHeaderFile.id)} + onToggleCollapse={toggleCollapseCallback(pinnedHeaderFile.id)} /> ) : null} @@ -1849,6 +1867,7 @@ export function DiffPane({ } onSelect={selectFileCallback(file.id)} onToggleGap={(gapKey) => onToggleGap(file.id, gapKey)} + onToggleCollapse={toggleCollapseCallback(file.id)} /> ); })} diff --git a/src/ui/components/panes/DiffSection.tsx b/src/ui/components/panes/DiffSection.tsx index 7dc6af2c..61a72229 100644 --- a/src/ui/components/panes/DiffSection.tsx +++ b/src/ui/components/panes/DiffSection.tsx @@ -42,6 +42,7 @@ interface DiffSectionProps { onStartUserNoteAtHunk?: (hunkIndex: number, target?: UserNoteLineTarget) => void; onSelect: () => void; onToggleGap: (gapKey: string) => void; + onToggleCollapse?: () => void; } /** Render one file section in the main review stream. */ @@ -76,6 +77,7 @@ function DiffSectionComponent({ onStartUserNoteAtHunk, onSelect, onToggleGap, + onToggleCollapse, }: DiffSectionProps) { return ( ) : null} @@ -173,6 +177,7 @@ export const DiffSection = memo(DiffSectionComponent, (previous, next) => { previous.onMouseScroll === next.onMouseScroll && previous.onActiveAddNoteAffordanceChange === next.onActiveAddNoteAffordanceChange && previous.onStartUserNoteAtHunk === next.onStartUserNoteAtHunk && + previous.onToggleCollapse === next.onToggleCollapse && previous.theme === next.theme && previous.visibleAgentNotes === next.visibleAgentNotes && previous.visibleBodyBounds === next.visibleBodyBounds && diff --git a/src/ui/components/ui-components.test.tsx b/src/ui/components/ui-components.test.tsx index a7fa905c..a2af1fdd 100644 --- a/src/ui/components/ui-components.test.tsx +++ b/src/ui/components/ui-components.test.tsx @@ -2322,13 +2322,13 @@ describe("UI components", () => { const frame = await captureFrame( {}} />, 76, - 39, + 41, ); const expectedRows = [ @@ -2355,6 +2355,7 @@ describe("UI components", () => { "a toggle AI notes", "z toggle unchanged context", "l / w / m / M lines / wrap / metadata / menu", + "x / X collapse file / all files", "e open file in $EDITOR", "Review", "/ focus file filter", diff --git a/src/ui/diff/renderRows.tsx b/src/ui/diff/renderRows.tsx index 60bba1cf..17f5b41b 100644 --- a/src/ui/diff/renderRows.tsx +++ b/src/ui/diff/renderRows.tsx @@ -1100,6 +1100,10 @@ function renderWrappedStackCellLine( /** Explain why a file still appears in the review stream even when it has no textual hunks. */ export function diffMessage(file: DiffFile) { + if (file.isCollapsed) { + return "Collapsed. Press x or click the chevron to expand."; + } + if (file.metadata.type === "rename-pure") { return "No textual hunks. This change only renames the file."; } diff --git a/src/ui/hooks/useAppKeyboardShortcuts.ts b/src/ui/hooks/useAppKeyboardShortcuts.ts index 1f09de60..562cccf6 100644 --- a/src/ui/hooks/useAppKeyboardShortcuts.ts +++ b/src/ui/hooks/useAppKeyboardShortcuts.ts @@ -42,6 +42,14 @@ function isUppercaseGKey(key: KeyEvent) { ); } +/** Detect Shift-X without stealing the lowercase collapse-file toggle. */ +function isUppercaseXKey(key: KeyEvent) { + return ( + (key.sequence === "X" && !key.option && !key.ctrl && !key.meta) || + (key.name === "x" && key.shift && !key.option && !key.ctrl && !key.meta) + ); +} + /** Detect Shift-M without stealing the lowercase hunk metadata toggle. */ function isUppercaseMKey(key: KeyEvent) { return ( @@ -82,6 +90,8 @@ export interface UseAppKeyboardShortcutsOptions { toggleAgentNotes: () => void; toggleFocusArea: () => void; toggleGapForSelectedHunk: () => void; + toggleSelectedFileCollapsed: () => void; + toggleAllFilesCollapsed: () => void; toggleHelp: () => void; toggleHunkHeaders: () => void; toggleLineNumbers: () => void; @@ -126,6 +136,8 @@ export function useAppKeyboardShortcuts({ toggleAgentNotes, toggleFocusArea, toggleGapForSelectedHunk, + toggleSelectedFileCollapsed, + toggleAllFilesCollapsed, toggleHelp, themeSelectorOpen, toggleHunkHeaders, @@ -545,6 +557,17 @@ export function useAppKeyboardShortcuts({ return; } + // Shift-X collapses/expands every file; check it before the lowercase x binding. + if (isUppercaseXKey(key)) { + runAndCloseMenu(toggleAllFilesCollapsed); + return; + } + + if (key.name === "x" || key.sequence === "x") { + runAndCloseMenu(toggleSelectedFileCollapsed); + return; + } + if (key.name === "e" || key.sequence === "e") { runAndCloseMenu(triggerEditSelectedFile); return; diff --git a/src/ui/hooks/useReviewController.test.tsx b/src/ui/hooks/useReviewController.test.tsx index 8dd65666..8cb1b8e5 100644 --- a/src/ui/hooks/useReviewController.test.tsx +++ b/src/ui/hooks/useReviewController.test.tsx @@ -167,6 +167,66 @@ describe("useReviewController", () => { } }); + test("collapsing a file swaps in a zero-hunk variant and skips its hunk navigation", async () => { + const { controllerRef, setup } = await renderReviewController([ + createDiffFile("alpha", "alpha.ts", "export const alpha = 1;\n", "export const alpha = 2;\n"), + createDiffFile("beta", "beta.ts", "export const beta = 1;\n", "export const beta = 2;\n"), + ]); + + try { + await flush(setup); + + await act(async () => { + expectValue(controllerRef.current).toggleFileCollapsed("alpha"); + }); + await flush(setup); + + const collapsed = expectValue(controllerRef.current).visibleFiles.find( + (file) => file.id === "alpha", + ); + expect(collapsed?.isCollapsed).toBe(true); + expect(collapsed?.metadata.hunks).toEqual([]); + // The collapsed file contributes no hunk cursors, so [ / ] navigation skips it. + expect( + expectValue(controllerRef.current).visibleFiles.every((file) => + file.id === "alpha" ? file.metadata.hunks.length === 0 : true, + ), + ).toBe(true); + expect([...expectValue(controllerRef.current).collapsedFileIds]).toEqual(["alpha"]); + + // Toggling again expands it back to its real hunks. + await act(async () => { + expectValue(controllerRef.current).toggleFileCollapsed("alpha"); + }); + await flush(setup); + const expanded = expectValue(controllerRef.current).visibleFiles.find( + (file) => file.id === "alpha", + ); + expect(expanded?.isCollapsed).toBeFalsy(); + expect(expanded?.metadata.hunks.length).toBeGreaterThan(0); + + // Collapse-all marks every file, expand-all clears the set. + await act(async () => { + expectValue(controllerRef.current).toggleAllFilesCollapsed(); + }); + await flush(setup); + expect([...expectValue(controllerRef.current).collapsedFileIds].sort()).toEqual([ + "alpha", + "beta", + ]); + + await act(async () => { + expectValue(controllerRef.current).toggleAllFilesCollapsed(); + }); + await flush(setup); + expect([...expectValue(controllerRef.current).collapsedFileIds]).toEqual([]); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + test("clamps the selected hunk index when files update under a soft reload", async () => { const { controllerRef, setFilesRef, setup } = await renderReviewController([ createTwoHunkFile(), diff --git a/src/ui/hooks/useReviewController.ts b/src/ui/hooks/useReviewController.ts index 6e14f203..82903e29 100644 --- a/src/ui/hooks/useReviewController.ts +++ b/src/ui/hooks/useReviewController.ts @@ -39,6 +39,7 @@ import type { import type { FileSourceStatus } from "../diff/expandCollapsedRows"; import { selectGapForKeyboardToggle } from "../diff/expandCollapsedRows"; import { trailingCollapsedLines } from "../diff/pierre"; +import { pruneCollapsedFileIds } from "../lib/fileCollapse"; import { findNextHunkCursor } from "../lib/hunks"; import { reviewNoteSource } from "../lib/agentAnnotations"; import { @@ -127,6 +128,7 @@ export interface ReviewSelectionOptions { export interface ReviewController { allFiles: DiffFile[]; + collapsedFileIds: ReadonlySet; expandedGapsByFileId: Record>; filter: string; draftNote: DraftReviewNote | null; @@ -151,6 +153,8 @@ export interface ReviewController { sourceStatusByFileId: Record; toggleGap: (fileId: string, gapKey: string) => void; toggleSelectedHunkGap: () => void; + toggleFileCollapsed: (fileId: string) => void; + toggleAllFilesCollapsed: () => void; visibleFiles: DiffFile[]; addLiveComment: ( input: CommentToolInput, @@ -202,6 +206,11 @@ export function useReviewController({ files }: { files: DiffFile[] }): ReviewCon const [sourceStatusByFileId, setSourceStatusByFileId] = useState< Record >({}); + // Session-only set of collapsed file ids. Survives watch reloads via the same + // reconciliation as gap expansion, but is intentionally not persisted to disk. + const [collapsedFileIds, setCollapsedFileIds] = useState>( + () => new Set(), + ); // Mirror sourceStatusByFileId so toggleGap can dedup synchronously without // waiting for React's state updater to commit. const sourceStatusRef = useRef(sourceStatusByFileId); @@ -236,6 +245,7 @@ export function useReviewController({ files }: { files: DiffFile[] }): ReviewCon } setSourceStatusByFileId((prev) => removeKeys(prev, staleFileIds)); setExpandedGapsByFileId((prev) => removeKeys(prev, staleFileIds)); + setCollapsedFileIds((prev) => pruneCollapsedFileIds(prev, staleFileIds)); } } @@ -257,8 +267,10 @@ export function useReviewController({ files }: { files: DiffFile[] }): ReviewCon filterQuery: deferredFilter, selectedFileId, selectedHunkIndex, + collapsedFileIds, }), [ + collapsedFileIds, deferredFilter, files, liveCommentsByFileId, @@ -295,6 +307,36 @@ export function useReviewController({ files }: { files: DiffFile[] }): ReviewCon [selectHunk], ); + /** Toggle one file's collapsed state, re-pinning its header so the height change stays in view. */ + const toggleFileCollapsed = useCallback( + (fileId: string) => { + if (!fileId) { + return; + } + setCollapsedFileIds((prev) => { + const next = new Set(prev); + if (next.has(fileId)) { + next.delete(fileId); + } else { + next.add(fileId); + } + return next; + }); + // Anchor the toggled file's header to the top so collapsing tall files + // above the fold can't scroll the file out of view. + selectFile(fileId, 0, { alignFileHeaderTop: true }); + }, + [selectFile], + ); + + /** Collapse every file when any is expanded, otherwise expand them all. */ + const toggleAllFilesCollapsed = useCallback(() => { + setCollapsedFileIds((prev) => { + const anyExpanded = allFiles.some((file) => !prev.has(file.id)); + return anyExpanded ? new Set(allFiles.map((file) => file.id)) : new Set(); + }); + }, [allFiles]); + /** Reset selection to the first visible file when the current target disappears from the review stream. */ const reselectFirstVisibleFile = useCallback(() => { startTransition(() => { @@ -988,6 +1030,7 @@ export function useReviewController({ files }: { files: DiffFile[] }): ReviewCon return { allFiles, + collapsedFileIds, draftNote, expandedGapsByFileId, filter, @@ -1008,6 +1051,8 @@ export function useReviewController({ files }: { files: DiffFile[] }): ReviewCon sourceStatusByFileId, toggleGap, toggleSelectedHunkGap, + toggleFileCollapsed, + toggleAllFilesCollapsed, visibleFiles, addLiveComment, addLiveCommentBatch, diff --git a/src/ui/lib/appMenus.ts b/src/ui/lib/appMenus.ts index 6dcadf90..886d2713 100644 --- a/src/ui/lib/appMenus.ts +++ b/src/ui/lib/appMenus.ts @@ -22,6 +22,8 @@ export interface BuildAppMenusOptions { toggleCopyDecorations: () => void; toggleAgentNotes: () => void; toggleFocusArea: () => void; + toggleSelectedFileCollapsed: () => void; + toggleAllFilesCollapsed: () => void; openAgentSkill: () => void; toggleHelp: () => void; toggleHunkHeaders: () => void; @@ -55,6 +57,8 @@ export function buildAppMenus({ toggleCopyDecorations, toggleAgentNotes, toggleFocusArea, + toggleSelectedFileCollapsed, + toggleAllFilesCollapsed, openAgentSkill, toggleHelp, toggleHunkHeaders, @@ -214,6 +218,19 @@ export function buildAppMenus({ action: () => moveToAnnotatedHunk(1), }, { kind: "separator" }, + { + kind: "item", + label: "Collapse/expand file", + hint: "x", + action: toggleSelectedFileCollapsed, + }, + { + kind: "item", + label: "Collapse/expand all files", + hint: "X", + action: toggleAllFilesCollapsed, + }, + { kind: "separator" }, { kind: "item", label: "Focus filter", diff --git a/src/ui/lib/fileCollapse.test.ts b/src/ui/lib/fileCollapse.test.ts new file mode 100644 index 00000000..0df47dfb --- /dev/null +++ b/src/ui/lib/fileCollapse.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, test } from "bun:test"; +import { createTestDiffFile } from "../../../test/helpers/diff-helpers"; +import { applyFileCollapse, collapsedFileVariant, pruneCollapsedFileIds } from "./fileCollapse"; + +describe("collapsedFileVariant", () => { + test("empties hunks, flags collapse, and preserves stats/identity", () => { + const file = createTestDiffFile({ id: "a", path: "a.ts" }); + expect(file.metadata.hunks.length).toBeGreaterThan(0); + + const variant = collapsedFileVariant(file); + + expect(variant.metadata.hunks).toEqual([]); + expect(variant.isCollapsed).toBe(true); + expect(variant.id).toBe(file.id); + expect(variant.stats).toEqual(file.stats); + expect(variant.metadata.cacheKey).not.toBe(file.metadata.cacheKey); + }); + + test("returns a stable object so geometry caching can key on identity", () => { + const file = createTestDiffFile({ id: "a", path: "a.ts" }); + expect(collapsedFileVariant(file)).toBe(collapsedFileVariant(file)); + }); +}); + +describe("applyFileCollapse", () => { + test("swaps only collapsed ids and leaves the array untouched when none collapse", () => { + const a = createTestDiffFile({ id: "a", path: "a.ts" }); + const b = createTestDiffFile({ id: "b", path: "b.ts" }); + const files = [a, b]; + + expect(applyFileCollapse(files, new Set())).toBe(files); + + const result = applyFileCollapse(files, new Set(["a"])); + expect(result[0]!.isCollapsed).toBe(true); + expect(result[1]).toBe(b); + }); +}); + +describe("pruneCollapsedFileIds", () => { + test("drops ids that left the changeset and is a no-op otherwise", () => { + const ids = new Set(["a", "b"]); + expect(pruneCollapsedFileIds(ids, new Set())).toBe(ids); + + const pruned = pruneCollapsedFileIds(ids, new Set(["b"])); + expect([...pruned]).toEqual(["a"]); + }); +}); diff --git a/src/ui/lib/fileCollapse.ts b/src/ui/lib/fileCollapse.ts new file mode 100644 index 00000000..d84ecd3a --- /dev/null +++ b/src/ui/lib/fileCollapse.ts @@ -0,0 +1,66 @@ +/** + * Manual file collapse for the review stream. + * + * Collapsing a file swaps it for a zero-hunk placeholder variant so the existing + * empty-file render and geometry paths handle it as a single header-plus-message + * section. This keeps collapse out of the deep renderer: hunk cursors, windowing, + * and sticky headers all read the same `hunks: []` shape they already understand. + */ +import type { DiffFile } from "../../core/types"; + +// Cache the collapsed variant per original file object so its identity stays +// stable across renders. The geometry layer keys its measurement cache on the +// DiffFile object, so a fresh variant per render would thrash that cache. +const collapsedVariants = new WeakMap(); + +/** Build (or reuse) the zero-hunk placeholder variant for a collapsed file. */ +export function collapsedFileVariant(file: DiffFile): DiffFile { + const cached = collapsedVariants.get(file); + if (cached) { + return cached; + } + + const variant: DiffFile = { + ...file, + isCollapsed: true, + metadata: { + ...file.metadata, + hunks: [], + // Distinguish the collapsed geometry from the real file in any string-keyed cache. + cacheKey: `${file.metadata.cacheKey}:collapsed`, + }, + }; + collapsedVariants.set(file, variant); + return variant; +} + +/** Replace collapsed files in a review list with their header-only placeholder variant. */ +export function applyFileCollapse( + files: DiffFile[], + collapsedFileIds: ReadonlySet, +): DiffFile[] { + if (collapsedFileIds.size === 0) { + return files; + } + return files.map((file) => (collapsedFileIds.has(file.id) ? collapsedFileVariant(file) : file)); +} + +/** Drop ids that no longer exist in the current review so collapse state can't leak across reloads. */ +export function pruneCollapsedFileIds( + collapsedFileIds: ReadonlySet, + staleFileIds: ReadonlySet, +): ReadonlySet { + if (collapsedFileIds.size === 0 || staleFileIds.size === 0) { + return collapsedFileIds; + } + let changed = false; + const next = new Set(); + for (const id of collapsedFileIds) { + if (staleFileIds.has(id)) { + changed = true; + } else { + next.add(id); + } + } + return changed ? next : collapsedFileIds; +} diff --git a/src/ui/lib/reviewState.ts b/src/ui/lib/reviewState.ts index 794a74da..536c08a5 100644 --- a/src/ui/lib/reviewState.ts +++ b/src/ui/lib/reviewState.ts @@ -9,12 +9,15 @@ import { findDiffFileByPath, findHunkIndexForLine, hunkLineRange } from "../../core/liveComments"; import type { AgentAnnotation, DiffFile } from "../../core/types"; import type { NavigateToHunkToolInput, SelectedHunkSummary } from "../../hunk-session/types"; +import { applyFileCollapse } from "./fileCollapse"; import { buildSidebarEntries, filterReviewFiles, mergeFileAnnotationsByFileId, type SidebarEntry, } from "./files"; + +const EMPTY_COLLAPSED_FILE_IDS: ReadonlySet = new Set(); import { buildAnnotatedHunkCursors, buildHunkCursors, @@ -28,6 +31,7 @@ export interface BuildReviewStateOptions { filterQuery: string; selectedFileId: string; selectedHunkIndex: number; + collapsedFileIds?: ReadonlySet; } export interface ReviewState { @@ -53,9 +57,15 @@ export function buildReviewState({ filterQuery, selectedFileId, selectedHunkIndex, + collapsedFileIds, }: BuildReviewStateOptions): ReviewState { const allFiles = mergeFileAnnotationsByFileId(files, liveCommentsByFileId); - const visibleFiles = filterReviewFiles(allFiles, filterQuery); + // Collapse swaps each collapsed file for a zero-hunk variant, so the visible + // stream, sidebar, and hunk cursors all derive from one already-collapsed list. + const visibleFiles = applyFileCollapse( + filterReviewFiles(allFiles, filterQuery), + collapsedFileIds ?? EMPTY_COLLAPSED_FILE_IDS, + ); const selectedFile = resolveSelectedFile(allFiles, visibleFiles, selectedFileId); return { diff --git a/src/ui/lib/ui-lib.test.ts b/src/ui/lib/ui-lib.test.ts index 098ea5cb..4bc89847 100644 --- a/src/ui/lib/ui-lib.test.ts +++ b/src/ui/lib/ui-lib.test.ts @@ -150,6 +150,8 @@ describe("ui helpers", () => { toggleCopyDecorations: () => {}, toggleAgentNotes: () => {}, toggleFocusArea: () => {}, + toggleSelectedFileCollapsed: () => {}, + toggleAllFilesCollapsed: () => {}, openAgentSkill: () => {}, toggleHelp: () => {}, toggleHunkHeaders: () => {}, diff --git a/test/pty/collapse.test.ts b/test/pty/collapse.test.ts new file mode 100644 index 00000000..13f0bcd0 --- /dev/null +++ b/test/pty/collapse.test.ts @@ -0,0 +1,51 @@ +import { afterEach, describe, expect, setDefaultTimeout, test } from "bun:test"; +import { createPtyHarness } from "./harness"; + +const harness = createPtyHarness(); + +/** Give PTY-backed startup and redraws enough headroom for slower CI machines. */ +setDefaultTimeout(20_000); + +afterEach(() => { + harness.cleanup(); +}); + +describe("PTY file collapse", () => { + test("x collapses the selected file to a header placeholder and expands it again", async () => { + const fixture = harness.createMultiHunkFilePair(); + const session = await harness.launchHunk({ + args: ["diff", fixture.before, fixture.after, "--mode", "split"], + cols: 104, + rows: 12, + }); + + try { + const initial = await session.waitForText(/View\s+Navigate\s+Agent\s+Help/, { + timeout: 15_000, + }); + expect(initial).toContain("line1 = 100"); + + await session.press("x"); + const collapsed = await harness.waitForSnapshot( + session, + (text) => text.includes("Collapsed") && !text.includes("line1 = 100"), + 5_000, + ); + expect(collapsed).toContain("Collapsed"); + expect(collapsed).not.toContain("line1 = 100"); + // The header chevron flips to the collapsed glyph. + expect(collapsed).toContain("▸"); + + await session.press("x"); + const expanded = await harness.waitForSnapshot( + session, + (text) => text.includes("line1 = 100") && !text.includes("Collapsed"), + 5_000, + ); + expect(expanded).toContain("line1 = 100"); + expect(expanded).not.toContain("Collapsed"); + } finally { + session.close(); + } + }); +}); From 15796576b2822451038f4c5ccacbb153ddedd685 Mon Sep 17 00:00:00 2001 From: Nathan Arthur Date: Mon, 29 Jun 2026 17:40:35 -0500 Subject: [PATCH 2/5] refactor(review): unify collapse state with sibling per-file maps Back collapsed-file state with a fileId->true Record instead of a Set so it reconciles through the existing removeKeys stale-id pruning, deleting the parallel pruneCollapsedFileIds helper. Extract anchorFileHeaderTop so a bulk Shift+X collapse re-pins the selected file's header like the single-file toggle. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ui/hooks/useReviewController.test.tsx | 6 +-- src/ui/hooks/useReviewController.ts | 58 +++++++++++++---------- src/ui/lib/fileCollapse.test.ts | 16 ++----- src/ui/lib/fileCollapse.ts | 35 +++++--------- src/ui/lib/reviewState.ts | 6 +-- 5 files changed, 54 insertions(+), 67 deletions(-) diff --git a/src/ui/hooks/useReviewController.test.tsx b/src/ui/hooks/useReviewController.test.tsx index 8cb1b8e5..4a39b7c8 100644 --- a/src/ui/hooks/useReviewController.test.tsx +++ b/src/ui/hooks/useReviewController.test.tsx @@ -192,7 +192,7 @@ describe("useReviewController", () => { file.id === "alpha" ? file.metadata.hunks.length === 0 : true, ), ).toBe(true); - expect([...expectValue(controllerRef.current).collapsedFileIds]).toEqual(["alpha"]); + expect(Object.keys(expectValue(controllerRef.current).collapsedFileIds)).toEqual(["alpha"]); // Toggling again expands it back to its real hunks. await act(async () => { @@ -210,7 +210,7 @@ describe("useReviewController", () => { expectValue(controllerRef.current).toggleAllFilesCollapsed(); }); await flush(setup); - expect([...expectValue(controllerRef.current).collapsedFileIds].sort()).toEqual([ + expect(Object.keys(expectValue(controllerRef.current).collapsedFileIds).sort()).toEqual([ "alpha", "beta", ]); @@ -219,7 +219,7 @@ describe("useReviewController", () => { expectValue(controllerRef.current).toggleAllFilesCollapsed(); }); await flush(setup); - expect([...expectValue(controllerRef.current).collapsedFileIds]).toEqual([]); + expect(Object.keys(expectValue(controllerRef.current).collapsedFileIds)).toEqual([]); } finally { await act(async () => { setup.renderer.destroy(); diff --git a/src/ui/hooks/useReviewController.ts b/src/ui/hooks/useReviewController.ts index 82903e29..3eff1051 100644 --- a/src/ui/hooks/useReviewController.ts +++ b/src/ui/hooks/useReviewController.ts @@ -39,7 +39,6 @@ import type { import type { FileSourceStatus } from "../diff/expandCollapsedRows"; import { selectGapForKeyboardToggle } from "../diff/expandCollapsedRows"; import { trailingCollapsedLines } from "../diff/pierre"; -import { pruneCollapsedFileIds } from "../lib/fileCollapse"; import { findNextHunkCursor } from "../lib/hunks"; import { reviewNoteSource } from "../lib/agentAnnotations"; import { @@ -128,7 +127,7 @@ export interface ReviewSelectionOptions { export interface ReviewController { allFiles: DiffFile[]; - collapsedFileIds: ReadonlySet; + collapsedFileIds: Readonly>; expandedGapsByFileId: Record>; filter: string; draftNote: DraftReviewNote | null; @@ -206,11 +205,10 @@ export function useReviewController({ files }: { files: DiffFile[] }): ReviewCon const [sourceStatusByFileId, setSourceStatusByFileId] = useState< Record >({}); - // Session-only set of collapsed file ids. Survives watch reloads via the same - // reconciliation as gap expansion, but is intentionally not persisted to disk. - const [collapsedFileIds, setCollapsedFileIds] = useState>( - () => new Set(), - ); + // Session-only `fileId -> true` map of collapsed files. Keyed like the other + // per-file session maps so it reconciles through the same `removeKeys` pruning + // on reload, but is intentionally not persisted to disk. + const [collapsedFileIds, setCollapsedFileIds] = useState>>({}); // Mirror sourceStatusByFileId so toggleGap can dedup synchronously without // waiting for React's state updater to commit. const sourceStatusRef = useRef(sourceStatusByFileId); @@ -245,7 +243,7 @@ export function useReviewController({ files }: { files: DiffFile[] }): ReviewCon } setSourceStatusByFileId((prev) => removeKeys(prev, staleFileIds)); setExpandedGapsByFileId((prev) => removeKeys(prev, staleFileIds)); - setCollapsedFileIds((prev) => pruneCollapsedFileIds(prev, staleFileIds)); + setCollapsedFileIds((prev) => removeKeys(prev, staleFileIds)); } } @@ -307,35 +305,47 @@ export function useReviewController({ files }: { files: DiffFile[] }): ReviewCon [selectHunk], ); + // Single place that knows a collapse/expand height change must re-pin a file's + // header to the top, so a collapse above the fold can't scroll it out of view. + const anchorFileHeaderTop = useCallback( + (fileId: string) => { + if (fileId) { + selectFile(fileId, 0, { alignFileHeaderTop: true }); + } + }, + [selectFile], + ); + /** Toggle one file's collapsed state, re-pinning its header so the height change stays in view. */ const toggleFileCollapsed = useCallback( (fileId: string) => { if (!fileId) { return; } - setCollapsedFileIds((prev) => { - const next = new Set(prev); - if (next.has(fileId)) { - next.delete(fileId); - } else { - next.add(fileId); - } - return next; - }); - // Anchor the toggled file's header to the top so collapsing tall files - // above the fold can't scroll the file out of view. - selectFile(fileId, 0, { alignFileHeaderTop: true }); + setCollapsedFileIds((prev) => + prev[fileId] ? removeKeys(prev, new Set([fileId])) : { ...prev, [fileId]: true }, + ); + anchorFileHeaderTop(fileId); }, - [selectFile], + [anchorFileHeaderTop], ); /** Collapse every file when any is expanded, otherwise expand them all. */ const toggleAllFilesCollapsed = useCallback(() => { setCollapsedFileIds((prev) => { - const anyExpanded = allFiles.some((file) => !prev.has(file.id)); - return anyExpanded ? new Set(allFiles.map((file) => file.id)) : new Set(); + const anyExpanded = allFiles.some((file) => !prev[file.id]); + if (!anyExpanded) { + return {}; + } + const next: Record = {}; + for (const file of allFiles) { + next[file.id] = true; + } + return next; }); - }, [allFiles]); + // Re-pin the selected file too, so a bulk collapse keeps it in view like the single-file toggle. + anchorFileHeaderTop(selectedFileId); + }, [allFiles, anchorFileHeaderTop, selectedFileId]); /** Reset selection to the first visible file when the current target disappears from the review stream. */ const reselectFirstVisibleFile = useCallback(() => { diff --git a/src/ui/lib/fileCollapse.test.ts b/src/ui/lib/fileCollapse.test.ts index 0df47dfb..cf52ea49 100644 --- a/src/ui/lib/fileCollapse.test.ts +++ b/src/ui/lib/fileCollapse.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; import { createTestDiffFile } from "../../../test/helpers/diff-helpers"; -import { applyFileCollapse, collapsedFileVariant, pruneCollapsedFileIds } from "./fileCollapse"; +import { applyFileCollapse, collapsedFileVariant } from "./fileCollapse"; describe("collapsedFileVariant", () => { test("empties hunks, flags collapse, and preserves stats/identity", () => { @@ -28,20 +28,10 @@ describe("applyFileCollapse", () => { const b = createTestDiffFile({ id: "b", path: "b.ts" }); const files = [a, b]; - expect(applyFileCollapse(files, new Set())).toBe(files); + expect(applyFileCollapse(files, {})).toBe(files); - const result = applyFileCollapse(files, new Set(["a"])); + const result = applyFileCollapse(files, { a: true }); expect(result[0]!.isCollapsed).toBe(true); expect(result[1]).toBe(b); }); }); - -describe("pruneCollapsedFileIds", () => { - test("drops ids that left the changeset and is a no-op otherwise", () => { - const ids = new Set(["a", "b"]); - expect(pruneCollapsedFileIds(ids, new Set())).toBe(ids); - - const pruned = pruneCollapsedFileIds(ids, new Set(["b"])); - expect([...pruned]).toEqual(["a"]); - }); -}); diff --git a/src/ui/lib/fileCollapse.ts b/src/ui/lib/fileCollapse.ts index d84ecd3a..876481b7 100644 --- a/src/ui/lib/fileCollapse.ts +++ b/src/ui/lib/fileCollapse.ts @@ -34,33 +34,20 @@ export function collapsedFileVariant(file: DiffFile): DiffFile { return variant; } -/** Replace collapsed files in a review list with their header-only placeholder variant. */ +/** + * Replace collapsed files in a review list with their header-only placeholder variant. + * + * Collapse state is a `fileId -> true` map, the same shape as the controller's + * other per-file session state, so it reconciles through the shared `removeKeys` + * stale-id pruning on reload rather than needing its own pruning helper. + */ export function applyFileCollapse( files: DiffFile[], - collapsedFileIds: ReadonlySet, + collapsedFileIds: Readonly>, ): DiffFile[] { - if (collapsedFileIds.size === 0) { + const anyCollapsed = files.some((file) => collapsedFileIds[file.id]); + if (!anyCollapsed) { return files; } - return files.map((file) => (collapsedFileIds.has(file.id) ? collapsedFileVariant(file) : file)); -} - -/** Drop ids that no longer exist in the current review so collapse state can't leak across reloads. */ -export function pruneCollapsedFileIds( - collapsedFileIds: ReadonlySet, - staleFileIds: ReadonlySet, -): ReadonlySet { - if (collapsedFileIds.size === 0 || staleFileIds.size === 0) { - return collapsedFileIds; - } - let changed = false; - const next = new Set(); - for (const id of collapsedFileIds) { - if (staleFileIds.has(id)) { - changed = true; - } else { - next.add(id); - } - } - return changed ? next : collapsedFileIds; + return files.map((file) => (collapsedFileIds[file.id] ? collapsedFileVariant(file) : file)); } diff --git a/src/ui/lib/reviewState.ts b/src/ui/lib/reviewState.ts index 536c08a5..89c58d6a 100644 --- a/src/ui/lib/reviewState.ts +++ b/src/ui/lib/reviewState.ts @@ -16,8 +16,6 @@ import { mergeFileAnnotationsByFileId, type SidebarEntry, } from "./files"; - -const EMPTY_COLLAPSED_FILE_IDS: ReadonlySet = new Set(); import { buildAnnotatedHunkCursors, buildHunkCursors, @@ -25,13 +23,15 @@ import { type HunkCursor, } from "./hunks"; +const EMPTY_COLLAPSED_FILE_IDS: Readonly> = {}; + export interface BuildReviewStateOptions { files: DiffFile[]; liveCommentsByFileId: Record; filterQuery: string; selectedFileId: string; selectedHunkIndex: number; - collapsedFileIds?: ReadonlySet; + collapsedFileIds?: Readonly>; } export interface ReviewState { From da0a534145667f3489e10ef383e916a890c711d6 Mon Sep 17 00:00:00 2001 From: Nathan Arthur Date: Mon, 29 Jun 2026 17:45:48 -0500 Subject: [PATCH 3/5] test(review): cover collapse re-anchor and stale-id pruning Guard the two behaviors the collapse refactor changed: bulk Shift+X re-pins the selected file's header (not just single-file x), and collapse state keyed by an old patch is pruned through removeKeys when a reload swaps the file's fetcher. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ui/hooks/useReviewController.test.tsx | 65 +++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/src/ui/hooks/useReviewController.test.tsx b/src/ui/hooks/useReviewController.test.tsx index 4a39b7c8..7af06b19 100644 --- a/src/ui/hooks/useReviewController.test.tsx +++ b/src/ui/hooks/useReviewController.test.tsx @@ -227,6 +227,71 @@ describe("useReviewController", () => { } }); + test("re-pins the selected file's header when collapsing a single file or all files", async () => { + const { controllerRef, setup } = await renderReviewController([ + createDiffFile("alpha", "alpha.ts", "export const alpha = 1;\n", "export const alpha = 2;\n"), + createDiffFile("beta", "beta.ts", "export const beta = 1;\n", "export const beta = 2;\n"), + ]); + + try { + await flush(setup); + + // Collapsing one file anchors its header to the top so a tall file above the fold can't scroll it off. + const beforeSingle = expectValue(controllerRef.current).selectedFileTopAlignRequestId; + await act(async () => { + expectValue(controllerRef.current).toggleFileCollapsed("alpha"); + }); + await flush(setup); + expect(expectValue(controllerRef.current).selectedFileTopAlignRequestId).toBeGreaterThan( + beforeSingle, + ); + + // Bulk collapse re-pins the selected file too, matching the single-file toggle. + const beforeBulk = expectValue(controllerRef.current).selectedFileTopAlignRequestId; + await act(async () => { + expectValue(controllerRef.current).toggleAllFilesCollapsed(); + }); + await flush(setup); + expect(expectValue(controllerRef.current).selectedFileTopAlignRequestId).toBeGreaterThan( + beforeBulk, + ); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("prunes collapse state for a file whose patch is replaced on reload", async () => { + // A reload swaps the file's sourceFetcher; collapse state keyed by the old patch must not leak. + const firstFetcher = createTestSourceFetcher(() => null); + const { controllerRef, setFilesRef, setup } = await renderReviewController([ + createAlphaFile(firstFetcher), + ]); + + try { + await flush(setup); + + await act(async () => { + expectValue(controllerRef.current).toggleFileCollapsed("alpha"); + }); + await flush(setup); + expect(Object.keys(expectValue(controllerRef.current).collapsedFileIds)).toEqual(["alpha"]); + + // Same id, new fetcher (and thus a new patch) marks the old collapse entry stale. + const secondFetcher = createTestSourceFetcher(() => null); + await act(async () => { + expectValue(setFilesRef.current)([createAlphaFile(secondFetcher)]); + }); + await flush(setup); + expect(Object.keys(expectValue(controllerRef.current).collapsedFileIds)).toEqual([]); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + test("clamps the selected hunk index when files update under a soft reload", async () => { const { controllerRef, setFilesRef, setup } = await renderReviewController([ createTwoHunkFile(), From fbc10c89e7c6be9268b826cb867b1683111cbe9a Mon Sep 17 00:00:00 2001 From: Nathan Arthur Date: Mon, 29 Jun 2026 17:51:13 -0500 Subject: [PATCH 4/5] test(review): cover Shift-X all-files collapse and chevron click Add the two coverage gaps the review flagged on the feature: Shift-X must collapse/expand every file (distinguished from single-file x by asserting the second file also collapses), and the header chevron toggles collapse while its stopPropagation keeps a chevron click from also firing the header select. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ui/AppHost.interactions.test.tsx | 43 +++++++++++++++++++ src/ui/components/ui-components.test.tsx | 54 ++++++++++++++++++++++++ 2 files changed, 97 insertions(+) diff --git a/src/ui/AppHost.interactions.test.tsx b/src/ui/AppHost.interactions.test.tsx index ffa7f957..d2be44e4 100644 --- a/src/ui/AppHost.interactions.test.tsx +++ b/src/ui/AppHost.interactions.test.tsx @@ -763,6 +763,49 @@ describe("App interactions", () => { } }); + test("Shift-X collapses and expands every file, not just the selected one", async () => { + // Two files so the all-files binding is distinguishable from the single-file x binding: + // a single-file toggle would leave beta expanded, whereas Shift-X must collapse it too. + const setup = await testRender(, { + width: 240, + height: 24, + }); + + try { + await flush(setup); + + let frame = setup.captureCharFrame(); + expect(frame).toContain("add = true"); + expect(frame).toContain("betaValue"); + + await act(async () => { + await setup.mockInput.pressKey("x", { shift: true }); + }); + await flush(setup); + + // Both files collapse to placeholders; if Shift-X routed to the single-file + // toggle instead, beta's "betaValue" line would still be on screen. + frame = setup.captureCharFrame(); + expect(frame).toContain("Collapsed"); + expect(frame).not.toContain("add = true"); + expect(frame).not.toContain("betaValue"); + + await act(async () => { + await setup.mockInput.pressKey("x", { shift: true }); + }); + await flush(setup); + + frame = setup.captureCharFrame(); + expect(frame).not.toContain("Collapsed"); + expect(frame).toContain("add = true"); + expect(frame).toContain("betaValue"); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + test("configured hidden menu bar starts hidden while menus remain keyboard-accessible", async () => { const setup = await testRender( , diff --git a/src/ui/components/ui-components.test.tsx b/src/ui/components/ui-components.test.tsx index a2af1fdd..de36dcaa 100644 --- a/src/ui/components/ui-components.test.tsx +++ b/src/ui/components/ui-components.test.tsx @@ -528,6 +528,60 @@ describe("UI components", () => { expect(firstLine[statsIndex + "+2 -1".length + 1]).toBe(" "); }); + test("DiffFileHeaderRow chevron toggles collapse without triggering header select", async () => { + const theme = resolveTheme("github-dark-default", null); + const onSelect = mock(() => undefined); + const onToggleCollapse = mock(() => undefined); + const setup = await testRender( + , + { width: 40, height: 2 }, + ); + + try { + await act(async () => { + await setup.renderOnce(); + }); + const rows = setup.captureCharFrame().split("\n"); + const rowY = rows.findIndex((line) => line.includes("▾")); + const chevronX = rows[rowY]?.indexOf("▾") ?? -1; + const filenameX = rows[rowY]?.indexOf("hdr") ?? -1; + expect(rowY).toBeGreaterThanOrEqual(0); + expect(chevronX).toBeGreaterThanOrEqual(0); + expect(filenameX).toBeGreaterThanOrEqual(0); + + // Clicking the chevron toggles collapse and stops propagation, so the header's select never fires. + await act(async () => { + await setup.mockMouse.click(chevronX, rowY); + }); + expect(onToggleCollapse).toHaveBeenCalledTimes(1); + expect(onSelect).not.toHaveBeenCalled(); + + // Clicking the filename bubbles to the header's select without toggling collapse. + await act(async () => { + await setup.mockMouse.click(filenameX, rowY); + }); + expect(onSelect).toHaveBeenCalledTimes(1); + expect(onToggleCollapse).toHaveBeenCalledTimes(1); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + test("DiffRowView renders a clickable add-note affordance for a hovered diff row", async () => { const theme = resolveTheme("github-dark-default", null); const startUserNote = mock(() => undefined); From a0d7e52a5e97f2d0e129b8a7ec52387213f8f0e7 Mon Sep 17 00:00:00 2001 From: Nathan Arthur Date: Mon, 29 Jun 2026 17:58:12 -0500 Subject: [PATCH 5/5] refactor(review): drop test-only collapsedFileIds from controller surface The collapse map is internal: the UI reads collapse state through the swapped-in zero-hunk variant (isCollapsed), never the id map. Keep it as private state and assert collapse in tests through the observable visibleFiles stream instead of widening the public ReviewController contract for tests alone. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ui/hooks/useReviewController.test.tsx | 18 ++++++++++++------ src/ui/hooks/useReviewController.ts | 2 -- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/ui/hooks/useReviewController.test.tsx b/src/ui/hooks/useReviewController.test.tsx index 7af06b19..eab87e47 100644 --- a/src/ui/hooks/useReviewController.test.tsx +++ b/src/ui/hooks/useReviewController.test.tsx @@ -84,6 +84,11 @@ function expectValue(value: T): NonNullable { return value as NonNullable; } +/** Ids of files currently rendered as collapsed placeholders, read from the observable review stream. */ +function collapsedVisibleFileIds(controller: ReviewController): string[] { + return controller.visibleFiles.filter((file) => file.isCollapsed).map((file) => file.id); +} + function ReviewControllerHarness({ initialFiles, onController, @@ -192,7 +197,7 @@ describe("useReviewController", () => { file.id === "alpha" ? file.metadata.hunks.length === 0 : true, ), ).toBe(true); - expect(Object.keys(expectValue(controllerRef.current).collapsedFileIds)).toEqual(["alpha"]); + expect(collapsedVisibleFileIds(expectValue(controllerRef.current))).toEqual(["alpha"]); // Toggling again expands it back to its real hunks. await act(async () => { @@ -210,7 +215,7 @@ describe("useReviewController", () => { expectValue(controllerRef.current).toggleAllFilesCollapsed(); }); await flush(setup); - expect(Object.keys(expectValue(controllerRef.current).collapsedFileIds).sort()).toEqual([ + expect(collapsedVisibleFileIds(expectValue(controllerRef.current)).sort()).toEqual([ "alpha", "beta", ]); @@ -219,7 +224,7 @@ describe("useReviewController", () => { expectValue(controllerRef.current).toggleAllFilesCollapsed(); }); await flush(setup); - expect(Object.keys(expectValue(controllerRef.current).collapsedFileIds)).toEqual([]); + expect(collapsedVisibleFileIds(expectValue(controllerRef.current))).toEqual([]); } finally { await act(async () => { setup.renderer.destroy(); @@ -276,15 +281,16 @@ describe("useReviewController", () => { expectValue(controllerRef.current).toggleFileCollapsed("alpha"); }); await flush(setup); - expect(Object.keys(expectValue(controllerRef.current).collapsedFileIds)).toEqual(["alpha"]); + expect(collapsedVisibleFileIds(expectValue(controllerRef.current))).toEqual(["alpha"]); - // Same id, new fetcher (and thus a new patch) marks the old collapse entry stale. + // Same id, new fetcher (and thus a new patch) marks the old collapse entry stale, so the + // reloaded file renders expanded rather than inheriting the previous patch's collapse. const secondFetcher = createTestSourceFetcher(() => null); await act(async () => { expectValue(setFilesRef.current)([createAlphaFile(secondFetcher)]); }); await flush(setup); - expect(Object.keys(expectValue(controllerRef.current).collapsedFileIds)).toEqual([]); + expect(collapsedVisibleFileIds(expectValue(controllerRef.current))).toEqual([]); } finally { await act(async () => { setup.renderer.destroy(); diff --git a/src/ui/hooks/useReviewController.ts b/src/ui/hooks/useReviewController.ts index 3eff1051..027851ee 100644 --- a/src/ui/hooks/useReviewController.ts +++ b/src/ui/hooks/useReviewController.ts @@ -127,7 +127,6 @@ export interface ReviewSelectionOptions { export interface ReviewController { allFiles: DiffFile[]; - collapsedFileIds: Readonly>; expandedGapsByFileId: Record>; filter: string; draftNote: DraftReviewNote | null; @@ -1040,7 +1039,6 @@ export function useReviewController({ files }: { files: DiffFile[] }): ReviewCon return { allFiles, - collapsedFileIds, draftNote, expandedGapsByFileId, filter,