fix(web): viewed files stay folded when a pull request reopens

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<typeof actual.getClientSettings>;
+  function useClientSettings<T = Settings>(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) => (
   <PullRequestCodeTab
     scrollerRef={() => {}}
     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();
+});

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<ReadonlySet<string>>(() => new Set());
+  const [foldChoices, setFoldChoices] = useState<ReadonlyMap<string, boolean>>(() => new Map());
+  const [viewedFolds, setViewedFolds] = useState<ReadonlySet<string>>(() => new Set());
   const [visibleCommitCount, setVisibleCommitCount] = useState(COMMIT_PAGE_SIZE);
   const [foldOverride, setFoldOverride] = useState<DiffFoldOverride>(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({
       </div>
     );
 
+  const isFoldedByDefault = (fileKey: string, path: string) =>
+    isFileDiffFoldedByDefault({
+      foldOverride,
+      filesCollapsedSetting: settings.diffFilesCollapsed,
+      viewed: isFileViewed(path),
+      viewedFold: viewedFolds.has(fileKey),
+    });
+
   const items: CodeViewDiffItem<ReviewAnnotationGroup>[] = 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}
             />
           </FileViewedToggleContext>

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);
   });
 });