diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx index 96ef692c4aa13..b06f3d117b9ba 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx @@ -56,12 +56,43 @@ const { diffAtom, pagedDiffAtom } = await vi.hoisted(async () => { }; }); -const mocks = vi.hoisted(() => ({ paged: false, diffCommits: [] as Array })); +const mocks = vi.hoisted(() => ({ + paged: false, + diffCommits: [] as Array, + viewedPaths: null as ReadonlySet | null, + viewedListeners: new Set<() => void>(), +})); + +function deliverViewedPaths(paths: ReadonlySet | null) { + mocks.viewedPaths = paths; + for (const listener of mocks.viewedListeners) listener(); +} vi.mock(import("~/state/use-atom-command"), async (original) => ({ ...(await original()), useAtomCommand: (() => vi.fn()) as never, })); +vi.mock(import("./usePullRequestFilesViewed"), async () => { + const { useSyncExternalStore } = await import("react"); + const subscribe = (listener: () => void) => { + mocks.viewedListeners.add(listener); + return () => mocks.viewedListeners.delete(listener); + }; + return { + usePullRequestFilesViewed: () => { + const viewedPaths = useSyncExternalStore(subscribe, () => mocks.viewedPaths); + return { + enabled: viewedPaths !== null, + isViewed: (path: string) => viewedPaths?.has(path) === true, + isStale: () => false, + setViewed: () => {}, + truncated: false, + error: null, + refresh: () => {}, + }; + }, + }; +}); vi.mock(import("~/state/pullRequests"), async (original) => { const actual = await original(); return { @@ -136,6 +167,8 @@ function setup({ paged }: { paged: boolean }) { __setClientSettingsForTests(DEFAULT_CLIENT_SETTINGS); mocks.paged = paged; mocks.diffCommits = []; + mocks.viewedPaths = null; + mocks.viewedListeners.clear(); } const tab = ( @@ -174,6 +207,30 @@ it("keeps the diff after a refresh that brings back the same page", async () => await expect.element(screen.getByText("1 file", { exact: true })).toBeVisible(); }); +// regression: #13279 a Viewed answer that lands after a link reveals a file must not fold it +it("keeps a revealed file open when its Viewed mark loads afterwards", async () => { + setup({ paged: false }); + __setClientSettingsForTests({ ...DEFAULT_CLIENT_SETTINGS, diffFilesCollapsed: false }); + using _preferences = { + [Symbol.dispose]() { + __setClientSettingsForTests(DEFAULT_CLIENT_SETTINGS); + }, + }; + mocks.viewedPaths = new Set(); + const container = document.createElement("div"); + container.className = "flex h-[700px] w-[900px]"; + await using screen = await render(tab(0, { path: "a.ts", nonce: 1 }), { container }); + await expect.element(screen.getByText("three()", { exact: true })).toBeVisible(); + + deliverViewedPaths(new Set(["a.ts"])); + await expect.element(screen.getByRole("checkbox", { name: "Viewed" })).toBeChecked(); + + await expect + .element(screen.getByRole("button", { name: "Collapse diff" })) + .toHaveAttribute("aria-expanded", "true"); + await expect.element(screen.getByText("three()", { exact: true })).toBeVisible(); +}); + function pagedContainer() { const container = document.createElement("div"); container.className = "flex h-[500px] w-[900px]"; diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index e25725023ae24..68525756e2d99 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -123,8 +123,9 @@ import { getFileViewedRange, isFileDiffCollapsed, isLineInFileDiff, - toggleFileDiffFoldForViewed, + withFileDiffFold, type DiffFoldOverride, + type FileDiffFoldChoices, } from "./pullRequestDiff.logic"; import { PullRequestDiffStat, PullRequestMetaLine } from "./pullRequestPresentation"; import { PullRequestDiffUnavailableState } from "./PullRequestDiffUnavailableState"; @@ -385,7 +386,7 @@ function PullRequestCodeTab({ diffLayout: settings.diffLayout, wordWrap: settings.wordWrap, })); - const [toggledFiles, setToggledFiles] = useState>(() => new Set()); + const [foldChoices, setFoldChoices] = useState(() => new Map()); const [lastViewedFileKey, setLastViewedFileKey] = useState(null); const [visibleCommitCount, setVisibleCommitCount] = useState(COMMIT_PAGE_SIZE); const [foldOverride, setFoldOverride] = useState(null); @@ -430,7 +431,7 @@ function PullRequestCodeTab({ setLastViewedFileKey(null); setDraft(null); setSelectedLines(null); - setToggledFiles(new Set()); + setFoldChoices(new Map()); setFoldOverride(null); setVisibleCommitCount(COMMIT_PAGE_SIZE); setOrphansOpen(false); @@ -740,9 +741,17 @@ function PullRequestCodeTab({ ); + // A stale mark means the file changed since it was viewed; Fold all and Expand all outrank marks. + const foldsAsViewed = (path: string) => + foldOverride === null && filesViewedEnabled && isFileViewed(path) && !isFileViewedStale(path); const items: CodeViewDiffItem[] = annotatedFiles.map( ({ fileKey, path, fileDiff, annotations, annotationsVersion }) => { - const collapsed = isFileDiffCollapsed(fileKey, effectiveFoldOverride, toggledFiles); + const collapsed = isFileDiffCollapsed( + fileKey, + effectiveFoldOverride, + foldChoices, + foldsAsViewed(path), + ); const viewedMark = filesViewedEnabled ? `e${isFileViewed(path) ? "v" : ""}${isFileViewedStale(path) ? "s" : ""}` : ""; @@ -783,13 +792,17 @@ function PullRequestCodeTab({ ); }; - 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 toggleFile = (fileKey: string) => { + const path = annotatedFiles.find((file) => file.fileKey === fileKey)?.path; + const viewed = path !== undefined && foldsAsViewed(path); + setFoldChoices((current) => + withFileDiffFold( + current, + fileKey, + !isFileDiffCollapsed(fileKey, effectiveFoldOverride, current, viewed), + ), + ); + }; const setFileViewed = (fileKey: string, viewed: boolean, extendRange: boolean) => { const range = getFileViewedRange( @@ -799,12 +812,8 @@ function PullRequestCodeTab({ ); setLastViewedFileKey(fileKey); for (const file of range) setViewed(file.path, viewed); - setToggledFiles((current) => - range.reduce( - (next, file) => - toggleFileDiffFoldForViewed(file.fileKey, viewed, effectiveFoldOverride, next), - current, - ), + setFoldChoices((current) => + range.reduce((next, file) => withFileDiffFold(next, file.fileKey, viewed), current), ); }; const fileViewedToggle = { @@ -817,19 +826,15 @@ function PullRequestCodeTab({ const revealFile = (path: string, range?: SelectedLineRange) => { const item = items.find((candidate) => resolveFileDiffPath(candidate.fileDiff) === path); if (item === undefined) return; - // Unfold only: a second reveal before the unfold renders must not fold the file again. - if (item.collapsed === true) { - setToggledFiles((current) => - toggleFileDiffFoldForViewed(item.id, false, effectiveFoldOverride, current), - ); - } + // Record the unfold even when already open, so a Viewed mark that loads later cannot fold it. + setFoldChoices((current) => withFileDiffFold(current, item.id, false)); if (range) setSelectedLines({ id: item.id, range }); codeViewReveal.request(item.id, range); }; const toggleAllFiles = () => { setFoldOverride(areAllDiffFilesCollapsed(fileKeys, collapsedFileKeys) ? "expanded" : "folded"); - setToggledFiles(new Set()); + setFoldChoices(new Map()); }; const orderedCommits = // GitLab preserves UTC offsets, so compare instants. @@ -971,9 +976,7 @@ function PullRequestCodeTab({ onPostRender: codeViewReveal.flush, // Find may repeat requests before render; only unfold, never toggle. onRevealSearchMatch: (item: CodeViewItem) => - setToggledFiles((current) => - toggleFileDiffFoldForViewed(item.id, false, effectiveFoldOverride, current), - ), + setFoldChoices((current) => withFileDiffFold(current, item.id, false)), }; const runThreadCommand = async ( diff --git a/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts index 552fa748f07f6..47f309b3e32c0 100644 --- a/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts @@ -5,7 +5,7 @@ import { getFileViewedRange, isFileDiffCollapsed, isLineInFileDiff, - toggleFileDiffFoldForViewed, + withFileDiffFold, } from "./pullRequestDiff.logic"; function fileWithHunks( @@ -43,16 +43,29 @@ describe("isLineInFileDiff", () => { 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", "expanded", choices)).toBe(true); + expect(isFileDiffCollapsed("c.ts", "expanded", choices)).toBe(false); + }); + + it("folds a file marked Viewed when the Code tab mounts again with no fold choices", () => { + expect(isFileDiffCollapsed("a.ts", "expanded", new Map(), true)).toBe(true); + expect(isFileDiffCollapsed("a.ts", "expanded", new Map(), false)).toBe(false); + }); + + it("keeps a reader's choice when the file's Viewed mark loads later", () => { + const choices = new Map([["a.ts", false]]); + expect(isFileDiffCollapsed("a.ts", "expanded", choices, false)).toBe(false); + expect(isFileDiffCollapsed("a.ts", "expanded", choices, true)).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("withFileDiffFold", () => { + it("records an absolute choice and keeps the same map when nothing changes", () => { + const folded = withFileDiffFold(new Map(), "a.ts", true); + expect(folded.get("a.ts")).toBe(true); + expect(withFileDiffFold(folded, "a.ts", true)).toBe(folded); + expect(withFileDiffFold(folded, "a.ts", false).get("a.ts")).toBe(false); }); }); diff --git a/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts b/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts index f175c0151431a..da3844e90e1bb 100644 --- a/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts @@ -16,28 +16,29 @@ export function isLineInFileDiff( export type DiffFoldOverride = "expanded" | "folded" | null; -/** Keep fold choices relative to the toolbar so later pages inherit Fold all. */ +/** The reader's own fold choice per file. It is absolute, so a Viewed mark loading later keeps it. */ +export type FileDiffFoldChoices = ReadonlyMap; + +/** + * A reader's own choice wins. Otherwise the toolbar decides, and a file still marked Viewed + * starts folded. Fold all and Expand all clear the choices, so later pages follow the toolbar. + */ export function isFileDiffCollapsed( fileKey: string, foldOverride: DiffFoldOverride, - toggledFileKeys: ReadonlySet, + choices: FileDiffFoldChoices, + viewed = false, ): boolean { - const foldedByDefault = foldOverride === "folded"; - return toggledFileKeys.has(fileKey) ? !foldedByDefault : foldedByDefault; + return choices.get(fileKey) ?? (foldOverride === "folded" || viewed); } -export function toggleFileDiffFoldForViewed( +export function withFileDiffFold( + choices: FileDiffFoldChoices, fileKey: string, - viewed: boolean, - foldOverride: DiffFoldOverride, - toggledFileKeys: 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); - return next; + collapsed: boolean, +): FileDiffFoldChoices { + if (choices.get(fileKey) === collapsed) return choices; + return new Map(choices).set(fileKey, collapsed); } export function getFileViewedRange(