diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx index 34a1c1ddfd6be..0115f96c8cef5 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx @@ -4,12 +4,13 @@ import { SourceControlProviderKind, type PullRequestDetailView, type PullRequestDiffResult, + type PullRequestFilesViewedResult, } from "@t3tools/contracts"; -import { expect, it, vi } from "vite-plus/test"; +import { expect, it, onTestFinished, vi } from "vite-plus/test"; import "~/index.css"; -const { diffAtom } = await vi.hoisted(async () => { +const { diffAtom, filesViewedAtom } = await vi.hoisted(async () => { const { AsyncResult, Atom } = await import("effect/reactivity"); const firstPage: PullRequestDiffResult = { patch: [ @@ -27,9 +28,26 @@ const { diffAtom } = await vi.hoisted(async () => { }; // An environment query whose refetch returns equal content hands back the previous value // itself, so a refresh publishes a new result around the very same page. - return { diffAtom: Atom.make(() => AsyncResult.success(firstPage)) }; + const filesViewed: PullRequestFilesViewedResult = { + files: [{ path: "a.ts", state: "viewed" }], + truncated: false, + }; + return { + diffAtom: Atom.make(() => AsyncResult.success(firstPage)), + filesViewedAtom: Atom.make(() => AsyncResult.success(filesViewed)), + }; }); +const settingsOverrides = vi.hoisted((): { diffFilesCollapsed?: boolean } => ({})); +vi.mock(import("~/hooks/useSettings"), async (original) => { + const actual = await original(); + type Settings = ReturnType; + function useClientSettings(select?: (settings: Settings) => T): T { + const settings = { ...actual.getClientSettings(), ...settingsOverrides }; + return select ? select(settings) : (settings as T); + } + return { ...actual, useClientSettings }; +}); vi.mock(import("~/state/use-atom-command"), async (original) => ({ ...(await original()), useAtomCommand: (() => vi.fn()) as never, @@ -41,6 +59,7 @@ vi.mock(import("~/state/pullRequests"), async (original) => { pullRequestEnvironment: { ...actual.pullRequestEnvironment, diff: (() => diffAtom) as never, + filesViewed: (() => filesViewedAtom) as never, }, }; }); @@ -100,12 +119,12 @@ const detail: PullRequestDetailView = { }, }; -const tab = (refreshToken: number) => ( +const tab = (refreshToken: number, tabDetail: PullRequestDetailView = detail) => ( {}} environmentId={EnvironmentId.make("environment")} - reference={detail} - detail={detail} + reference={tabDetail} + detail={tabDetail} selectedCommitOid={null} onSelectedCommitChange={() => {}} onRefresh={() => {}} @@ -126,3 +145,20 @@ it("keeps the diff after a refresh that brings back the same page", async () => .not.toBeInTheDocument(); await expect.element(screen.getByText("1 file", { exact: true })).toBeVisible(); }); + +it("opens a file already marked viewed folded when diffs open expanded", async () => { + settingsOverrides.diffFilesCollapsed = false; + onTestFinished(() => { + delete settingsOverrides.diffFilesCollapsed; + }); + const container = document.createElement("div"); + container.className = "flex h-[700px] w-[900px]"; + const viewedDetail: PullRequestDetailView = { + ...detail, + capabilities: { ...detail.capabilities, viewedFiles: "host" }, + }; + await using screen = await render(tab(0, viewedDetail), { container }); + + await expect.element(screen.getByRole("checkbox", { name: "Viewed" })).toBeChecked(); + await expect.element(screen.getByRole("button", { name: "Expand diff" })).toBeVisible(); +}); diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index ff7593d71b5e2..4e830adca9340 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -101,8 +101,10 @@ import { } from "./PullRequestReviewAnnotation"; import { isFileDiffCollapsed, + isFileDiffFoldedByDefault, isLineInFileDiff, - toggleFileDiffFoldForViewed, + setFileDiffFold, + setFileViewedFold, type DiffFoldOverride, } from "./pullRequestDiff.logic"; import { PullRequestDiffStat, PullRequestMetaLine } from "./pullRequestPresentation"; @@ -282,11 +284,10 @@ function PullRequestCodeTab({ diffLayout: settings.diffLayout, wordWrap: settings.wordWrap, })); - const [toggledFiles, setToggledFiles] = useState>(() => new Set()); + const [foldChoices, setFoldChoices] = useState>(() => new Map()); + const [viewedFolds, setViewedFolds] = useState>(() => new Set()); const [visibleCommitCount, setVisibleCommitCount] = useState(COMMIT_PAGE_SIZE); const [foldOverride, setFoldOverride] = useState(null); - const effectiveFoldOverride = - foldOverride ?? (settings.diffFilesCollapsed ? "folded" : "expanded"); const diffLayout = settings.diffLayout; const [wordWrap, setWordWrap] = useState(settings.wordWrap); const [ignoreWhitespace, setIgnoreWhitespace] = useState(settings.diffIgnoreWhitespace); @@ -323,7 +324,8 @@ function PullRequestCodeTab({ setStateScopeKey(scopeKey); setDraft(null); setSelectedLines(null); - setToggledFiles(new Set()); + setFoldChoices(new Map()); + setViewedFolds(new Set()); setFoldOverride(null); setVisibleCommitCount(COMMIT_PAGE_SIZE); setOrphansOpen(false); @@ -643,9 +645,17 @@ function PullRequestCodeTab({ ); + const isFoldedByDefault = (fileKey: string, path: string) => + isFileDiffFoldedByDefault({ + foldOverride, + filesCollapsedSetting: settings.diffFilesCollapsed, + viewed: isFileViewed(path), + viewedFold: viewedFolds.has(fileKey), + }); + const items: CodeViewDiffItem[] = annotatedFiles.map( ({ fileKey, path, fileDiff, annotations, annotationsVersion }) => { - const collapsed = isFileDiffCollapsed(fileKey, effectiveFoldOverride, toggledFiles); + const collapsed = isFileDiffCollapsed(fileKey, isFoldedByDefault(fileKey, path), foldChoices); const viewedMark = filesViewedEnabled ? `e${isFileViewed(path) ? "v" : ""}${isFileViewedStale(path) ? "s" : ""}` : ""; @@ -677,18 +687,35 @@ function PullRequestCodeTab({ setSliceState((previous) => ({ ...previous, cursor: nextCursor })); }; - const toggleFile = (fileKey: string) => - setToggledFiles((current) => { - const next = new Set(current); - if (next.has(fileKey)) next.delete(fileKey); - else next.add(fileKey); - return next; - }); + const setFileFold = (fileKey: string, collapse: (collapsed: boolean) => boolean) => { + const item = items.find((candidate) => candidate.id === fileKey); + if (item === undefined) return; + const foldedByDefault = isFoldedByDefault(fileKey, resolveFileDiffPath(item.fileDiff)); + setFoldChoices((current) => + setFileDiffFold( + fileKey, + collapse(isFileDiffCollapsed(fileKey, foldedByDefault, current)), + foldedByDefault, + current, + ), + ); + }; + const toggleFile = (fileKey: string) => setFileFold(fileKey, (collapsed) => !collapsed); const setFileViewed = (fileKey: string, path: string, viewed: boolean) => { setViewed(path, viewed); - setToggledFiles((current) => - toggleFileDiffFoldForViewed(fileKey, viewed, effectiveFoldOverride, current), + const foldedByDefault = isFileDiffFoldedByDefault({ + foldOverride, + filesCollapsedSetting: settings.diffFilesCollapsed, + viewed, + }); + // A tick the default would not fold is kept apart from the reader's own folds, so it lapses + // once the file stops being viewed rather than holding the file shut. + setViewedFolds((current) => setFileViewedFold(fileKey, viewed && !foldedByDefault, current)); + // Judged against the default the file will have once the press lands, so ticking folds and + // unticking unfolds whatever the reader did to the file before. + setFoldChoices((current) => + setFileDiffFold(fileKey, viewed, viewed || foldedByDefault, current), ); }; const fileViewedToggle = { @@ -701,13 +728,14 @@ function PullRequestCodeTab({ const revealFile = (path: string) => { const item = items.find((candidate) => resolveFileDiffPath(candidate.fileDiff) === path); if (item === undefined) return; - if (item.collapsed === true) toggleFile(item.id); + if (item.collapsed === true) setFileFold(item.id, () => false); requestTreeReveal(item.id); }; const toggleAllFiles = () => { setFoldOverride(areAllDiffFilesCollapsed(fileKeys, collapsedFileKeys) ? "expanded" : "folded"); - setToggledFiles(new Set()); + setFoldChoices(new Map()); + setViewedFolds(new Set()); }; const orderedCommits = // By instant, not by the text: GitLab keeps a commit's own UTC offset, so two commits @@ -1468,11 +1496,7 @@ function PullRequestCodeTab({ renderHeaderMetadata={renderHeaderMetadata} renderAnnotation={renderAnnotation} // Find can ask again before the unfolded file arrives, so this unfolds and never folds. - onRevealSearchMatch={(item) => - setToggledFiles((current) => - toggleFileDiffFoldForViewed(item.id, false, effectiveFoldOverride, current), - ) - } + onRevealSearchMatch={(item) => setFileFold(item.id, () => false)} unsafeCSSExtra={REPLACE_FILE_COUNTS_CSS} /> diff --git a/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts index 4e11e04e1d784..837fc325b0ad7 100644 --- a/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts @@ -3,8 +3,10 @@ import { describe, expect, it } from "vite-plus/test"; import { isFileDiffCollapsed, + isFileDiffFoldedByDefault, isLineInFileDiff, - toggleFileDiffFoldForViewed, + setFileDiffFold, + setFileViewedFold, } from "./pullRequestDiff.logic"; function fileWithHunks( @@ -39,18 +41,87 @@ describe("isLineInFileDiff", () => { }); }); +describe("isFileDiffFoldedByDefault", () => { + it("folds a viewed file every time the diff is drawn", () => { + expect( + isFileDiffFoldedByDefault({ foldOverride: null, filesCollapsedSetting: false, viewed: true }), + ).toBe(true); + expect( + isFileDiffFoldedByDefault({ + foldOverride: null, + filesCollapsedSetting: false, + viewed: false, + }), + ).toBe(false); + }); + + it("lets the toolbar fold or unfold every file, viewed or not", () => { + expect( + isFileDiffFoldedByDefault({ + foldOverride: "expanded", + filesCollapsedSetting: true, + viewed: true, + }), + ).toBe(false); + expect( + isFileDiffFoldedByDefault({ + foldOverride: "folded", + filesCollapsedSetting: false, + viewed: false, + }), + ).toBe(true); + }); +}); + +describe("setFileViewedFold", () => { + // regression: a tick after Expand all must not hold the file shut once it stops being viewed + it("folds a ticked file only while it stays viewed", () => { + const folds = setFileViewedFold("a.ts", true, new Set()); + const input = { foldOverride: "expanded", filesCollapsedSetting: false } as const; + expect( + isFileDiffFoldedByDefault({ ...input, viewed: true, viewedFold: folds.has("a.ts") }), + ).toBe(true); + expect( + isFileDiffFoldedByDefault({ ...input, viewed: false, viewedFold: folds.has("a.ts") }), + ).toBe(false); + }); + + it("drops the fold on untick and keeps the same set when nothing changes", () => { + const folds = setFileViewedFold("a.ts", true, new Set()); + expect(setFileViewedFold("a.ts", true, folds)).toBe(folds); + expect(setFileViewedFold("a.ts", false, folds).size).toBe(0); + }); +}); + describe("isFileDiffCollapsed", () => { // regression: #9174 diffs now expand by default; a reader's fold must survive new slices it("keeps a file the reader folded closed as the next slice arrives", () => { - const toggled = new Set(["b.ts"]); - expect(isFileDiffCollapsed("b.ts", null, toggled)).toBe(true); - expect(isFileDiffCollapsed("c.ts", null, toggled)).toBe(false); + const choices = new Map([["b.ts", true]]); + expect(isFileDiffCollapsed("b.ts", false, choices)).toBe(true); + expect(isFileDiffCollapsed("c.ts", false, choices)).toBe(false); + }); + + it("keeps a viewed file the reader opened open when a push unmarks it", () => { + const choices = setFileDiffFold("a.ts", false, true, new Map()); + expect(isFileDiffCollapsed("a.ts", true, choices)).toBe(false); + expect(isFileDiffCollapsed("a.ts", false, choices)).toBe(false); }); }); -describe("toggleFileDiffFoldForViewed", () => { - it("moves against whatever the toolbar last asked for", () => { - expect([...toggleFileDiffFoldForViewed("a.ts", true, "expanded", new Set())]).toEqual(["a.ts"]); - expect(toggleFileDiffFoldForViewed("a.ts", false, "expanded", new Set()).size).toBe(0); +describe("setFileDiffFold", () => { + it("drops a choice that agrees with the default so the file follows it again", () => { + const opened = setFileDiffFold("a.ts", false, true, new Map()); + expect([...opened]).toEqual([["a.ts", false]]); + expect(setFileDiffFold("a.ts", true, true, opened).size).toBe(0); + }); + + it("folds a file ticked after the toolbar unfolded everything", () => { + expect([...setFileDiffFold("a.ts", true, false, new Map())]).toEqual([["a.ts", true]]); + }); + + it("keeps the same map when nothing changes", () => { + const choices = new Map([["a.ts", true]]); + expect(setFileDiffFold("a.ts", true, false, choices)).toBe(choices); + expect(setFileDiffFold("b.ts", false, false, choices)).toBe(choices); }); }); diff --git a/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts b/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts index 7098b3c81f0c8..5639e1e99fc90 100644 --- a/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts @@ -22,30 +22,64 @@ export function isLineInFileDiff( export type DiffFoldOverride = "expanded" | "folded" | null; /** - * 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. + * A file the reader marked viewed opens folded every time the diff is drawn, not only in the + * moment the box was ticked, until the toolbar folds or unfolds every file at once. A file pushed + * to since it was marked is no longer viewed, so it opens again. + * + * `viewedFold` is a tick made after the toolbar unfolded everything. It folds the file only while + * the file stays viewed, so losing the tick some other way than unticking opens it again. + */ +export function isFileDiffFoldedByDefault(input: { + readonly foldOverride: DiffFoldOverride; + readonly filesCollapsedSetting: boolean; + readonly viewed: boolean; + readonly viewedFold?: boolean; +}): boolean { + if (input.viewed && input.viewedFold === true) return true; + if (input.foldOverride !== null) return input.foldOverride === "folded"; + return input.filesCollapsedSetting || input.viewed; +} + +/** + * A diff arrives a slice at a time, so only the files the reader folded or unfolded by hand are + * kept, and every other file follows its default: a file that has not loaded yet cannot be in the + * map, and would otherwise land expanded moments after the reader folded everything. Choices are + * kept as the state the reader asked for, not as flips of the default, because the default moves + * when a file's viewed state arrives or a push takes it away. */ export function isFileDiffCollapsed( fileKey: string, - foldOverride: DiffFoldOverride, - toggledFileKeys: ReadonlySet, + foldedByDefault: boolean, + foldChoices: ReadonlyMap, ): boolean { - const foldedByDefault = foldOverride === "folded"; - return toggledFileKeys.has(fileKey) ? !foldedByDefault : foldedByDefault; + return foldChoices.get(fileKey) ?? foldedByDefault; +} + +/** A choice that agrees with the default is dropped, so the file goes on following it. */ +export function setFileDiffFold( + fileKey: string, + collapsed: boolean, + foldedByDefault: boolean, + foldChoices: ReadonlyMap, +): ReadonlyMap { + if (collapsed === foldedByDefault) { + if (!foldChoices.has(fileKey)) return foldChoices; + const next = new Map(foldChoices); + next.delete(fileKey); + return next; + } + if (foldChoices.get(fileKey) === collapsed) return foldChoices; + return new Map(foldChoices).set(fileKey, collapsed); } -export function toggleFileDiffFoldForViewed( +export function setFileViewedFold( fileKey: string, - viewed: boolean, - foldOverride: DiffFoldOverride, - toggledFileKeys: ReadonlySet, + folded: boolean, + viewedFolds: ReadonlySet, ): ReadonlySet { - if (isFileDiffCollapsed(fileKey, foldOverride, toggledFileKeys) === viewed) - return toggledFileKeys; - const next = new Set(toggledFileKeys); - if (next.has(fileKey)) next.delete(fileKey); - else next.add(fileKey); + if (viewedFolds.has(fileKey) === folded) return viewedFolds; + const next = new Set(viewedFolds); + if (folded) next.add(fileKey); + else next.delete(fileKey); return next; }