From 039944c836a1a48b8692e98fe8be144e341437f6 Mon Sep 17 00:00:00 2001 From: Einar Andersson <72999+drdator@users.noreply.github.com> Date: Sun, 1 Nov 2026 11:27:11 +0100 Subject: [PATCH 1/2] fix(web): Diff pane expands unmodified lines again Pierre loads the full file into the parsed diff it was given and keeps the result only if that same object is still the item's diff. DiffPanel parsed the selected patch again whenever it re-rendered or refetched, so expanding a collapsed file handed Pierre a new object, and every "N unmodified lines" click after that loaded the contents and dropped them. Keep the last few parses keyed by the patch text and its parse options, so the same patch keeps the same parsed files. Fixes #27171 Co-authored-by: Claude Opus 5.5 --- .../src/components/DiffPanel.browser.test.tsx | 68 ++++++++++++++++--- apps/web/src/components/DiffPanel.tsx | 24 ++++++- 2 files changed, 80 insertions(+), 12 deletions(-) diff --git a/apps/web/src/components/DiffPanel.browser.test.tsx b/apps/web/src/components/DiffPanel.browser.test.tsx index 505c11130d692..8547b75cf0feb 100644 --- a/apps/web/src/components/DiffPanel.browser.test.tsx +++ b/apps/web/src/components/DiffPanel.browser.test.tsx @@ -32,6 +32,7 @@ const state = vi.hoisted(() => ({ localRefs: null as unknown as ResultAtom, remoteRefs: null as unknown as ResultAtom, commits: null as unknown as ResultAtom, + viewerFiles: [] as Array>, })); vi.mock("~/state/server", () => ({ @@ -76,8 +77,14 @@ vi.mock("~/state/use-atom-command", () => ({ useAtomCommand: () => async () => { vi.mock("~/editorPreferences", () => ({ useOpenInPreferredEditor: () => async () => {} })); vi.mock("~/fileContextMenu", () => ({ useFileContextMenuHandler: () => () => {} })); vi.mock("~/browser/useOpenLink", () => ({ useOpenLink: () => async () => {} })); -// The test drives comparison controls, without mounting virtualized diff viewers or their workers. -vi.mock("./diffs/AnnotatableCodeView", () => ({ AnnotatableCodeView: () => null })); +// The tests drive comparison controls and record the viewer's files, without mounting virtualized +// diff viewers or their workers. +vi.mock("./diffs/AnnotatableCodeView", () => ({ + AnnotatableCodeView: ({ files }: { files: (typeof state.viewerFiles)[number] }) => { + state.viewerFiles.push(files); + return null; + }, +})); vi.mock("./diffs/DiffFileTree", () => ({ DiffFileTree: () => null })); vi.mock("./DiffFilePathCopyButton", () => ({ DiffFilePathCopyButton: () => null })); @@ -87,7 +94,20 @@ import { useDiffPanelSessionStore, useDiffPanelStore } from "~/diffPanelStore"; const cwd = "/project-worktree"; const errorMessage = "Could not find a common commit between 'origin/orphan' and HEAD."; -function preview(): ReviewDiffPreviewResult { +const valuesPatch = [ + "diff --git a/src/values.ts b/src/values.ts", + "index 1111111..2222222 100644", + "--- a/src/values.ts", + "+++ b/src/values.ts", + "@@ -40,3 +40,3 @@", + " export const line40 = 40;", + "-export const line41 = 41;", + "+export const line41 = 410;", + " export const line42 = 42;", + "", +].join("\n"); + +function preview(diff: string): ReviewDiffPreviewResult { return { cwd, generatedAt: DateTime.makeUnsafe("2026-10-30T00:00:00Z"), @@ -98,15 +118,15 @@ function preview(): ReviewDiffPreviewResult { title: "Changes", baseRef: "main", headRef: "dev", - diff: "", - diffHash: "empty", + diff, + diffHash: diff === "" ? "empty" : "values", truncated: false, }, ], }; } -function setup(failedBaseRef: string | null) { +function setup({ failedBaseRef, diff = "" }: { failedBaseRef?: string | null; diff?: string }) { const reset = () => { useDiffPanelStore.setState({ byThreadKey: {}, @@ -128,9 +148,18 @@ function setup(failedBaseRef: string | null) { worktreePath: cwd, }); state.previews.clear(); - state.previews.set(null, Atom.make(AsyncResult.success(preview()))); - state.previews.set("main", Atom.make(AsyncResult.success(preview()))); - state.previews.set(failedBaseRef, Atom.make(AsyncResult.fail(new Error(errorMessage)))); + state.previews.set( + null, + Atom.make(() => AsyncResult.success(preview(diff))), + ); + state.previews.set( + "main", + Atom.make(() => AsyncResult.success(preview(diff))), + ); + if (failedBaseRef !== undefined) { + state.previews.set(failedBaseRef, Atom.make(AsyncResult.fail(new Error(errorMessage)))); + } + state.viewerFiles = []; state.config = Atom.make(null); state.status = Atom.make( AsyncResult.success({ @@ -163,7 +192,7 @@ function setup(failedBaseRef: string | null) { state.localRefs = Atom.make(AsyncResult.success(refs(["dev", "main"], false))); state.remoteRefs = Atom.make(AsyncResult.success(refs(["origin/orphan"], true))); state.commits = Atom.make(AsyncResult.success({ commits: [], truncated: false })); - useDiffPanelStore.getState().selectBranchBaseRef(state.threadRef, failedBaseRef); + useDiffPanelStore.getState().selectBranchBaseRef(state.threadRef, failedBaseRef ?? null); const registry = AtomRegistry.make(); return { registry, @@ -191,7 +220,7 @@ it.each([ ])( "recovers from $name without leaving the diff panel", async ({ failedBaseRef, recovery, baseRef }) => { - using context = setup(failedBaseRef); + using context = setup({ failedBaseRef }); await using screen = await render( @@ -212,3 +241,20 @@ it.each([ }); }, ); + +it("hands the viewer the same parsed file when it expands after a refetch", async () => { + using context = setup({ diff: valuesPatch }); + await using screen = await render( + + + , + ); + await vi.waitFor(() => expect(state.viewerFiles.at(-1)?.[0]?.collapsed).toBe(true)); + const collapsedFile = state.viewerFiles.at(-1)![0]!.fileDiff; + // A refetch returns the same patch in a new result. + context.registry.refresh(state.previews.get(null)!); + await userEvent.click(screen.getByRole("button", { name: "Expand all files" })); + await vi.waitFor(() => expect(state.viewerFiles.at(-1)?.[0]?.collapsed).toBe(false)); + // Pierre loads unmodified lines into the parsed file it was given; a re-parse would drop them. + expect(state.viewerFiles.at(-1)![0]!.fileDiff).toBe(collapsedFile); +}); diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index 0f1d1e9e28379..9363d27511672 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -50,6 +50,7 @@ import { getRenderablePatch, resolveDiffThemeName, resolveFileDiffPath, + type RenderablePatch, } from "../lib/diffRendering"; import { PREFERRED_HIGHLIGHTER } from "../lib/syntaxHighlighting"; import { areAllDiffFilesCollapsed, toggleAllDiffFiles } from "../lib/diffCollapse"; @@ -129,6 +130,27 @@ function getCachedFileEntry(fileDiff: FileDiffMetadata) { return entry; } +// Pierre loads expanded lines into the parsed file it was given, so a patch has to keep its parse +// across re-renders and refetches that return the same text. The four most recent are kept. +const recentPatchParses: Array<{ key: string; patch: string; parse: RenderablePatch | null }> = []; + +function getCachedRenderablePatch( + patch: string | undefined, + cacheScope: string, + options: { compactPartialHunkOffsets: boolean; ignoreWhitespace?: boolean }, +) { + if (patch === undefined) return null; + const key = `${cacheScope}:${options.compactPartialHunkOffsets}:${options.ignoreWhitespace}`; + const index = recentPatchParses.findIndex((entry) => entry.key === key && entry.patch === patch); + const entry = + index === -1 + ? { key, patch, parse: getRenderablePatch(patch, cacheScope, options) } + : recentPatchParses.splice(index, 1)[0]!; + recentPatchParses.unshift(entry); + recentPatchParses.splice(4); + return entry.parse; +} + const EMPTY_COLLAPSED_DIFF_FILE_KEYS: ReadonlySet = new Set(); function DiffFileCollapseToggle({ @@ -477,7 +499,7 @@ export default function DiffPanel({ isGitSelection && selectedGitSource?.truncated && selectedGitSource.files ? selectedGitSource : null; - const renderablePatch = getRenderablePatch( + const renderablePatch = getCachedRenderablePatch( lazySource ? undefined : selectedPatch, `diff-panel:${resolvedTheme}`, { From 4a05a9424fc52748493119e624d603f3efb84867 Mon Sep 17 00:00:00 2001 From: maria-rcks Date: Wed, 4 Nov 2026 08:00:28 +0000 Subject: [PATCH 2/2] fix(web): preserve refreshed lazy diff parses --- apps/web/src/components/DiffPanel.browser.test.tsx | 8 ++++++++ apps/web/src/components/diffs/useReviewFilePatches.ts | 7 +++++++ 2 files changed, 15 insertions(+) diff --git a/apps/web/src/components/DiffPanel.browser.test.tsx b/apps/web/src/components/DiffPanel.browser.test.tsx index ab1b9d63b71d5..3f53a9a4a0f18 100644 --- a/apps/web/src/components/DiffPanel.browser.test.tsx +++ b/apps/web/src/components/DiffPanel.browser.test.tsx @@ -262,6 +262,7 @@ it.each([ it("hands the viewer the same parsed file when it expands after a refetch", async () => { using context = setup({ diff: valuesPatch }); + state.diffFilesCollapsed = true; await using screen = await render(
@@ -313,6 +314,13 @@ it.each([false, true])( const loadedToggle = screen.getByRole("button", { name: `Collapse ${path}`, exact: true }); await expect.element(loadedToggle).toHaveAttribute("aria-expanded", "true"); await expect.element(screen.getByText("after", { exact: true })).toBeVisible(); + const loadedFile = state.viewerFiles.at(-1)![0]!.fileDiff; + const loaded = context.registry.get(context.patch); + if (!AsyncResult.isSuccess(loaded)) throw new Error("Expected a loaded patch"); + const renderCount = state.viewerFiles.length; + act(() => context.registry.set(context.patch, AsyncResult.success({ ...loaded.value }))); + await vi.waitFor(() => expect(state.viewerFiles.length).toBeGreaterThan(renderCount)); + await vi.waitFor(() => expect(state.viewerFiles.at(-1)![0]!.fileDiff).toBe(loadedFile)); await userEvent.click(loadedToggle); await expect.element(toggle).toHaveAttribute("aria-expanded", "false"); }, diff --git a/apps/web/src/components/diffs/useReviewFilePatches.ts b/apps/web/src/components/diffs/useReviewFilePatches.ts index 728f43433b010..e81e9467d39ae 100644 --- a/apps/web/src/components/diffs/useReviewFilePatches.ts +++ b/apps/web/src/components/diffs/useReviewFilePatches.ts @@ -223,8 +223,14 @@ type ReviewPatchTarget = Parameters[0]; const parsedPatchAtom = Atom.family((key: string) => { const [theme, target] = JSON.parse(key) as ["light" | "dark", ReviewPatchTarget]; + let cached: { key: string; patch: RenderablePatch | null } | undefined; return Atom.map(reviewEnvironment.diffFilePatch(target), (result) => AsyncResult.map(result, (source) => { + const contentKey = JSON.stringify([ + source.diff, + source.files?.map(({ path, previousPath }) => [path, previousPath]), + ]); + if (cached?.key === contentKey) return { source, patch: cached.patch }; let patch = getRenderablePatch(source.diff, `diff-panel:${theme}`, { compactPartialHunkOffsets: true, }); @@ -235,6 +241,7 @@ const parsedPatchAtom = Atom.family((key: string) => { else delete file.prevName; patch = { ...patch, files: [file] }; } + cached = { key: contentKey, patch }; return { source, patch }; }), );