diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index 0c7a4a28988d..77a0f75df28b 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -43,7 +43,13 @@ import { resolveFileDiffPath, } from "../lib/diffRendering"; import { PREFERRED_HIGHLIGHTER } from "../lib/syntaxHighlighting"; -import { areAllDiffFilesCollapsed, toggleAllDiffFiles } from "../lib/diffCollapse"; +import { + areAllDiffFilesCollapsed, + isFileDiffCollapsed, + setDiffFileCollapsed, + toggleAllDiffFiles, + type DiffFilesCollapseState, +} from "../lib/diffCollapse"; import { useTurnDiffSummaries } from "../hooks/useTurnDiffSummaries"; import { useWorkspaceMutationRefresh } from "../hooks/useWorkspaceMutationRefresh"; import { useProject, useThreadProjection, useThreadShell } from "../state/entities"; @@ -112,9 +118,8 @@ function getCachedFileEntry(fileDiff: FileDiffMetadata) { return entry; } -interface CollapsedDiffFilesState { +interface CollapsedDiffFilesState extends DiffFilesCollapseState { readonly scopeKey: string | null; - readonly fileKeys: ReadonlySet; } const EMPTY_COLLAPSED_DIFF_FILE_KEYS: ReadonlySet = new Set(); @@ -212,7 +217,8 @@ export default function DiffPanel({ const [baseRefQuery, setBaseRefQuery] = useState(""); const [collapsedDiffFiles, setCollapsedDiffFiles] = useState(() => ({ scopeKey: null, - fileKeys: EMPTY_COLLAPSED_DIFF_FILE_KEYS, + foldOverride: null, + toggledFileKeys: EMPTY_COLLAPSED_DIFF_FILE_KEYS, })); const [codeViewRevision, setCodeViewRevision] = useState(0); const [codeView, setCodeView] = useState(null); @@ -508,20 +514,41 @@ export default function DiffPanel({ const isRefreshingDiff = branchDiffPreview.isPending || areFilePatchesPending; const renderableFileEntries = useMemo( - () => renderableFiles.map(getCachedFileEntry), - [renderableFiles], - ); - const defaultCollapsedDiffFileKeys = useMemo( () => - settings.diffFilesCollapsed - ? new Set(renderableFileEntries.map((file) => file.fileKey)) - : EMPTY_COLLAPSED_DIFF_FILE_KEYS, - [renderableFileEntries, settings.diffFilesCollapsed], + renderableFiles.map((fileDiff) => { + const entry = getCachedFileEntry(fileDiff); + return lazySource + ? { ...entry, fileKey: buildFileDiffIdentityKey(fileDiff, "file") } + : entry; + }), + [renderableFiles, lazySource], + ); + const defaultCollapsedDiffFiles = useMemo( + () => ({ + foldOverride: settings.diffFilesCollapsed ? "folded" : "expanded", + toggledFileKeys: EMPTY_COLLAPSED_DIFF_FILE_KEYS, + }), + [settings.diffFilesCollapsed], ); - const collapsedDiffFileKeys = + const effectiveCollapsedDiffFiles = collapsedDiffFiles.scopeKey === collapseScopeKey - ? collapsedDiffFiles.fileKeys - : defaultCollapsedDiffFileKeys; + ? collapsedDiffFiles + : defaultCollapsedDiffFiles; + const collapsedDiffFileKeys = useMemo( + () => + new Set( + renderableFileEntries + .filter(({ fileKey }) => + isFileDiffCollapsed( + fileKey, + effectiveCollapsedDiffFiles.foldOverride, + effectiveCollapsedDiffFiles.toggledFileKeys, + ), + ) + .map(({ fileKey }) => fileKey), + ), + [renderableFileEntries, effectiveCollapsedDiffFiles], + ); const renderLoadingBoundary = useCallback( () => settledFileCount < renderableFiles.length ? ( @@ -594,11 +621,12 @@ export default function DiffPanel({ const file = renderableFileEntries[index]; if (!file) return; setCollapsedDiffFiles((current) => { - const next = new Set( - current.scopeKey === collapseScopeKey ? current.fileKeys : defaultCollapsedDiffFileKeys, + const next = setDiffFileCollapsed( + file.fileKey, + current.scopeKey === collapseScopeKey ? current : defaultCollapsedDiffFiles, + false, ); - next.delete(file.fileKey); - return { scopeKey: collapseScopeKey, fileKeys: next }; + return { ...next, scopeKey: collapseScopeKey }; }); if (lazySource && index >= settledFileCount) { requestFile(index); @@ -608,7 +636,7 @@ export default function DiffPanel({ [ renderableFileEntries, collapseScopeKey, - defaultCollapsedDiffFileKeys, + defaultCollapsedDiffFiles, requestTreeReveal, lazySource, settledFileCount, @@ -657,49 +685,44 @@ export default function DiffPanel({ }, [activeCwd, activeRepositoryRoot, openInPreferredEditor, routeThreadRef], ); - const collapseDefaultsRef = useRef({ collapseScopeKey, defaultCollapsedDiffFileKeys }); + const collapseDefaultsRef = useRef({ collapseScopeKey, defaultCollapsedDiffFiles }); useLayoutEffect(() => { - collapseDefaultsRef.current = { collapseScopeKey, defaultCollapsedDiffFileKeys }; - }, [collapseScopeKey, defaultCollapsedDiffFileKeys]); + collapseDefaultsRef.current = { collapseScopeKey, defaultCollapsedDiffFiles }; + }, [collapseScopeKey, defaultCollapsedDiffFiles]); const toggleDiffFileCollapsed = useCallback((fileKey: string) => { - const { collapseScopeKey, defaultCollapsedDiffFileKeys } = collapseDefaultsRef.current; + const { collapseScopeKey, defaultCollapsedDiffFiles } = collapseDefaultsRef.current; setCollapsedDiffFiles((current) => { - const next = new Set( - current.scopeKey === collapseScopeKey ? current.fileKeys : defaultCollapsedDiffFileKeys, + const state = current.scopeKey === collapseScopeKey ? current : defaultCollapsedDiffFiles; + const next = setDiffFileCollapsed( + fileKey, + state, + !isFileDiffCollapsed(fileKey, state.foldOverride, state.toggledFileKeys), ); - if (next.has(fileKey)) { - next.delete(fileKey); - } else { - next.add(fileKey); - } - return { scopeKey: collapseScopeKey, fileKeys: next }; + return { ...next, scopeKey: collapseScopeKey }; }); }, []); // Find can ask again before the unfolded file reaches the viewer, so this must never fold. const unfoldDiffFile = useCallback((fileKey: string) => { - const { collapseScopeKey, defaultCollapsedDiffFileKeys } = collapseDefaultsRef.current; + const { collapseScopeKey, defaultCollapsedDiffFiles } = collapseDefaultsRef.current; setCollapsedDiffFiles((current) => { - const fileKeys = - current.scopeKey === collapseScopeKey ? current.fileKeys : defaultCollapsedDiffFileKeys; - if (!fileKeys.has(fileKey)) return current; - const next = new Set(fileKeys); - next.delete(fileKey); - return { scopeKey: collapseScopeKey, fileKeys: next }; + const state = current.scopeKey === collapseScopeKey ? current : defaultCollapsedDiffFiles; + const next = setDiffFileCollapsed(fileKey, state, false); + if (next === state) return current; + return { ...next, scopeKey: collapseScopeKey }; }); }, []); const toggleDiffFileCollapse = useCallback(() => { setCodeViewRevision((current) => current + 1); setCollapsedDiffFiles((current) => { - const currentKeys = - current.scopeKey === collapseScopeKey ? current.fileKeys : defaultCollapsedDiffFileKeys; + const state = current.scopeKey === collapseScopeKey ? current : defaultCollapsedDiffFiles; return { + ...toggleAllDiffFiles(diffFileKeys, state), scopeKey: collapseScopeKey, - fileKeys: toggleAllDiffFiles(diffFileKeys, currentKeys), }; }); - }, [collapseScopeKey, defaultCollapsedDiffFileKeys, diffFileKeys]); + }, [collapseScopeKey, defaultCollapsedDiffFiles, diffFileKeys]); const selectTurn = (runId: RunId) => { if (!routeThreadRef) return; diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 25a8c10ad6b2..1380186a4b37 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -31,7 +31,11 @@ import { useCallback, useEffect, useMemo, useRef, useState, type ReactNode } fro import { useLocalStorage } from "~/hooks/useLocalStorage"; import { useClientSettings, useUpdateClientSettings } from "~/hooks/useSettings"; import { useTheme } from "~/hooks/useTheme"; -import { areAllDiffFilesCollapsed } from "~/lib/diffCollapse"; +import { + areAllDiffFilesCollapsed, + isFileDiffCollapsed, + type DiffFoldOverride, +} from "~/lib/diffCollapse"; import { pullRequestFindingKey, type PullRequestFinding } from "./pullRequestDetail.logic"; import { canEditPullRequestComment, canResolvePullRequestThread } from "./pullRequestEditing.logic"; import { orderDiffFiles } from "./pullRequestFileOrder.logic"; @@ -87,12 +91,7 @@ import { ReviewThreadCard, toViewerSide, } from "./PullRequestReviewAnnotation"; -import { - isFileDiffCollapsed, - isLineInFileDiff, - toggleFileDiffFoldForViewed, - type DiffFoldOverride, -} from "./pullRequestDiff.logic"; +import { isLineInFileDiff, toggleFileDiffFoldForViewed } from "./pullRequestDiff.logic"; import { PullRequestDiffStat, PullRequestMetaLine } from "./pullRequestPresentation"; import { usePullRequestFilesViewed } from "./usePullRequestFilesViewed"; import { diff --git a/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts index 181ebe6cfec1..6b91e78a5eca 100644 --- a/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts @@ -1,11 +1,8 @@ import type { FileDiffMetadata } from "@pierre/diffs"; import { describe, expect, it } from "vite-plus/test"; +import { isFileDiffCollapsed } from "~/lib/diffCollapse"; -import { - isFileDiffCollapsed, - isLineInFileDiff, - toggleFileDiffFoldForViewed, -} from "./pullRequestDiff.logic"; +import { isLineInFileDiff, toggleFileDiffFoldForViewed } from "./pullRequestDiff.logic"; /** Only the hunk ranges matter here; the viewer fills the rest in when it renders. */ function fileWithHunks( diff --git a/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts b/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts index b7b836cb3b5b..30bbd176376b 100644 --- a/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts @@ -1,5 +1,6 @@ import type { FileDiffMetadata } from "@pierre/diffs"; import type { PullRequestDiffSide } from "@t3tools/contracts"; +import { isFileDiffCollapsed, type DiffFoldOverride } from "~/lib/diffCollapse"; /** * Whether a conversation's line is really in this file's hunks. @@ -21,27 +22,6 @@ export function isLineInFileDiff( ); } -/** What the toolbar last asked of every file at once, null being the reader asking nothing yet. */ -export type DiffFoldOverride = "expanded" | "folded" | null; - -/** - * Whether a file is drawn folded. - * - * A diff arrives a slice at a time, so the reader's own choices are kept as the difference from - * what the toolbar last said rather than as the set of folded files: a file that has not loaded - * yet cannot be in a set, and would otherwise land expanded moments after the reader folded - * everything. The caller supplies the saved default until the toolbar overrides it; individual - * files can still be toggled independently. - */ -export function isFileDiffCollapsed( - fileKey: string, - foldOverride: DiffFoldOverride, - toggledFileKeys: ReadonlySet, -): boolean { - const foldedByDefault = foldOverride === "folded"; - return toggledFileKeys.has(fileKey) ? !foldedByDefault : foldedByDefault; -} - /** * The reader's fold choices after a file was ticked off, or put back. * diff --git a/apps/web/src/lib/diffCollapse.test.ts b/apps/web/src/lib/diffCollapse.test.ts index d46f6ec963e2..2a03d95a0e6f 100644 --- a/apps/web/src/lib/diffCollapse.test.ts +++ b/apps/web/src/lib/diffCollapse.test.ts @@ -1,9 +1,24 @@ import { describe, expect, it } from "vite-plus/test"; -import { areAllDiffFilesCollapsed, toggleAllDiffFiles } from "./diffCollapse"; +import { + areAllDiffFilesCollapsed, + isFileDiffCollapsed, + setDiffFileCollapsed, + toggleAllDiffFiles, + type DiffFilesCollapseState, +} from "./diffCollapse"; +import { buildFileDiffIdentityKey, getRenderablePatch } from "./diffRendering"; const FILE_KEYS = ["src/app.ts", "src/index.ts"]; const FIRST_FILE_KEY = FILE_KEYS[0]!; +const EXPANDED: DiffFilesCollapseState = { foldOverride: "expanded", toggledFileKeys: new Set() }; +const FOLDED: DiffFilesCollapseState = { foldOverride: "folded", toggledFileKeys: new Set() }; + +function collapsedKeys(fileKeys: ReadonlyArray, state: DiffFilesCollapseState) { + return new Set( + fileKeys.filter((key) => isFileDiffCollapsed(key, state.foldOverride, state.toggledFileKeys)), + ); +} describe("diff collapse controls", () => { it("reports whether every rendered file is collapsed", () => { @@ -13,10 +28,72 @@ describe("diff collapse controls", () => { }); it("collapses all files when any rendered file is expanded", () => { - expect(toggleAllDiffFiles(FILE_KEYS, new Set([FIRST_FILE_KEY]))).toEqual(new Set(FILE_KEYS)); + const partial = setDiffFileCollapsed(FIRST_FILE_KEY, EXPANDED, true); + const collapsed = toggleAllDiffFiles(FILE_KEYS, partial); + expect(areAllDiffFilesCollapsed(FILE_KEYS, collapsedKeys(FILE_KEYS, collapsed))).toBe(true); }); it("expands all files when every rendered file is collapsed", () => { - expect(toggleAllDiffFiles(FILE_KEYS, new Set(FILE_KEYS))).toEqual(new Set()); + const expanded = toggleAllDiffFiles(FILE_KEYS, FOLDED); + expect(collapsedKeys(FILE_KEYS, expanded)).toEqual(new Set()); + }); + + it("keeps a newly added file collapsed when its pending patch finishes loading", () => { + const pendingFileKey = "new.ts\u0000new.ts\u0000change"; + const loadedFileKey = "new.ts\u0000new.ts\u0000new"; + const collapsed = toggleAllDiffFiles([FIRST_FILE_KEY, pendingFileKey], EXPANDED); + + const loadedKeys = [FIRST_FILE_KEY, loadedFileKey]; + expect(areAllDiffFilesCollapsed(loadedKeys, collapsedKeys(loadedKeys, collapsed))).toBe(true); + }); + + it("applies collapse and expand all to files arriving on later pages", () => { + const collapsed = toggleAllDiffFiles([FIRST_FILE_KEY], EXPANDED); + const nextPage = [...FILE_KEYS, "src/next-page.ts"]; + expect(areAllDiffFilesCollapsed(nextPage, collapsedKeys(nextPage, collapsed))).toBe(true); + + const expanded = toggleAllDiffFiles(FILE_KEYS, collapsed); + expect(collapsedKeys(nextPage, expanded)).toEqual(new Set()); + }); + + it("keeps individual file choices when later pages arrive", () => { + const collapsed = toggleAllDiffFiles(FILE_KEYS, EXPANDED); + const revealed = setDiffFileCollapsed(FIRST_FILE_KEY, collapsed, false); + const nextPage = [...FILE_KEYS, "src/next-page.ts"]; + expect(collapsedKeys(nextPage, revealed)).toEqual(new Set(nextPage.slice(1))); + + const foldedAgain = setDiffFileCollapsed(FIRST_FILE_KEY, revealed, true); + expect(areAllDiffFilesCollapsed(nextPage, collapsedKeys(nextPage, foldedAgain))).toBe(true); + }); + + it("keeps a revealed lazy file expanded when its pending patch becomes a new file", () => { + const patch = getRenderablePatch( + [ + "diff --git a/new.ts b/new.ts", + "new file mode 100644", + "--- /dev/null", + "+++ b/new.ts", + "@@ -0,0 +1 @@", + "+export const value = 1;", + ].join("\n"), + ); + if (patch?.kind !== "files" || !patch.files[0]) throw new Error("Expected a new file diff"); + const loaded = patch.files[0]; + const pending = { ...loaded, type: "change" as const }; + const pendingKey = buildFileDiffIdentityKey(pending, "file"); + const loadedKey = buildFileDiffIdentityKey(loaded, "file"); + const collapsed = toggleAllDiffFiles([FIRST_FILE_KEY, pendingKey], EXPANDED); + const revealed = setDiffFileCollapsed(pendingKey, collapsed, false); + + expect(collapsedKeys([FIRST_FILE_KEY, loadedKey], revealed)).toEqual(new Set([FIRST_FILE_KEY])); + }); + + it("keeps repeated file reveals expanded and clears exceptions on the next bulk collapse", () => { + const revealed = setDiffFileCollapsed(FIRST_FILE_KEY, FOLDED, false); + const revealedAgain = setDiffFileCollapsed(FIRST_FILE_KEY, revealed, false); + expect(collapsedKeys(FILE_KEYS, revealedAgain)).toEqual(new Set([FILE_KEYS[1]])); + + const collapsed = toggleAllDiffFiles(FILE_KEYS, revealedAgain); + expect(areAllDiffFilesCollapsed(FILE_KEYS, collapsedKeys(FILE_KEYS, collapsed))).toBe(true); }); }); diff --git a/apps/web/src/lib/diffCollapse.ts b/apps/web/src/lib/diffCollapse.ts index 51bdcaf94ac8..c0c5e9e59270 100644 --- a/apps/web/src/lib/diffCollapse.ts +++ b/apps/web/src/lib/diffCollapse.ts @@ -1,3 +1,34 @@ +export type DiffFoldOverride = "expanded" | "folded" | null; + +export interface DiffFilesCollapseState { + readonly foldOverride: DiffFoldOverride; + readonly toggledFileKeys: ReadonlySet; +} + +/** Keep file choices relative to the bulk choice so patches arriving later inherit it. */ +export function isFileDiffCollapsed( + fileKey: string, + foldOverride: DiffFoldOverride, + toggledFileKeys: ReadonlySet, +): boolean { + const foldedByDefault = foldOverride === "folded"; + return toggledFileKeys.has(fileKey) ? !foldedByDefault : foldedByDefault; +} + +export function setDiffFileCollapsed( + fileKey: string, + state: DiffFilesCollapseState, + collapsed: boolean, +): DiffFilesCollapseState { + if (isFileDiffCollapsed(fileKey, state.foldOverride, state.toggledFileKeys) === collapsed) { + return state; + } + const toggledFileKeys = new Set(state.toggledFileKeys); + if (toggledFileKeys.has(fileKey)) toggledFileKeys.delete(fileKey); + else toggledFileKeys.add(fileKey); + return { ...state, toggledFileKeys }; +} + export function areAllDiffFilesCollapsed( fileKeys: ReadonlyArray, collapsedFileKeys: ReadonlySet, @@ -7,7 +38,15 @@ export function areAllDiffFilesCollapsed( export function toggleAllDiffFiles( fileKeys: ReadonlyArray, - collapsedFileKeys: ReadonlySet, -): ReadonlySet { - return areAllDiffFilesCollapsed(fileKeys, collapsedFileKeys) ? new Set() : new Set(fileKeys); + state: DiffFilesCollapseState, +): DiffFilesCollapseState { + const allCollapsed = + fileKeys.length > 0 && + fileKeys.every((fileKey) => + isFileDiffCollapsed(fileKey, state.foldOverride, state.toggledFileKeys), + ); + return { + foldOverride: allCollapsed ? "expanded" : "folded", + toggledFileKeys: new Set(), + }; } diff --git a/apps/web/src/lib/diffRendering.test.ts b/apps/web/src/lib/diffRendering.test.ts index 20114f6ef1b5..eea58d529107 100644 --- a/apps/web/src/lib/diffRendering.test.ts +++ b/apps/web/src/lib/diffRendering.test.ts @@ -292,7 +292,9 @@ describe("diff file reconciliation", () => { if (!deleted || !added) return; expect(buildFileDiffIdentityKey(deleted)).not.toBe(buildFileDiffIdentityKey(added)); - expect(new Set(parsed.files.map(buildFileDiffIdentityKey)).size).toBe(parsed.files.length); + expect(new Set(parsed.files.map((file) => buildFileDiffIdentityKey(file))).size).toBe( + parsed.files.length, + ); }); it("keeps identities stable and versions local to the changed file", () => { diff --git a/apps/web/src/lib/diffRendering.ts b/apps/web/src/lib/diffRendering.ts index dcc6bb2b79d7..51504d781948 100644 --- a/apps/web/src/lib/diffRendering.ts +++ b/apps/web/src/lib/diffRendering.ts @@ -218,9 +218,14 @@ export function resolveFileDiffPreviousPath(fileDiff: FileDiffMetadata): string /** * Stable across re-renders of the same file, distinct for every block in a * patch. A type change (regular file to symlink) arrives as a deletion and an - * addition of the same path, so the change type is part of the identity. + * addition of the same path, so block identities include the change type. + * Lazy sources have one entry per path; file identities survive placeholder replacement. */ -export function buildFileDiffIdentityKey(fileDiff: FileDiffMetadata): string { +export function buildFileDiffIdentityKey( + fileDiff: FileDiffMetadata, + identity: "block" | "file" = "block", +): string { + if (identity === "file") return resolveFileDiffPath(fileDiff); return `${resolveFileDiffPreviousPath(fileDiff)}\u0000${resolveFileDiffPath(fileDiff)}\u0000${fileDiff.type}`; }