From baf0d2398f2892e6d1b26f0ed4c6629970555ece Mon Sep 17 00:00:00 2001 From: "(CJ) Chukwudi Nwobodo" <142016413+chuks-qua@users.noreply.github.com> Date: Tue, 22 Sep 2026 13:15:00 +0100 Subject: [PATCH] fix(review): reload patches when revision or comparison scope changes ReviewDiffView keyed its local patch map by file path only, so a cacheVersion bump or comparison/source/thread change left stale diff content rendered under new scope metadata, and obsolete in-flight responses could write into the current scope or poison the shared inline diff cache for a later remount. Tag the patch map with the thread/source/id/cacheVersion scope and reset it when the scope changes, drop async results that arrive for an obsolete scope, suppress duplicate fetches for paths already in flight, and include cacheVersion in the shared inline cache key so a stale response can never land under a key the new scope reads. Fixes #1732 --- apps/web/src/__tests__/diffStore.test.ts | 26 +++- .../src/components/diff/ReviewDiffView.tsx | 57 ++++++-- .../__tests__/ReviewDiffView.refresh.test.tsx | 138 ++++++++++++++++++ apps/web/src/stores/diffStore.ts | 24 +-- 4 files changed, 215 insertions(+), 30 deletions(-) create mode 100644 apps/web/src/components/diff/__tests__/ReviewDiffView.refresh.test.tsx diff --git a/apps/web/src/__tests__/diffStore.test.ts b/apps/web/src/__tests__/diffStore.test.ts index 4820149da..27f0d3079 100644 --- a/apps/web/src/__tests__/diffStore.test.ts +++ b/apps/web/src/__tests__/diffStore.test.ts @@ -322,26 +322,36 @@ describe("diffStore", () => { it("evicts inline diff cache entries for the refreshed mutable scope", () => { const { cacheInlineDiff, bumpDiffRevision } = useDiffStore.getState(); - cacheInlineDiff("thread-1", "branch", "origin/main...feat/x", "src/a.ts", "diff-a"); - cacheInlineDiff("thread-2", "branch", "origin/main...feat/y", "src/b.ts", "diff-b"); + cacheInlineDiff("thread-1", "branch", "origin/main...feat/x", "src/a.ts", "diff-a", 1); + cacheInlineDiff("thread-2", "branch", "origin/main...feat/y", "src/b.ts", "diff-b", 1); bumpDiffRevision("thread-1"); const state = useDiffStore.getState(); - expect(state.inlineDiffCache["thread-1:branch:origin/main...feat/x:src/a.ts"]).toBeUndefined(); - expect(state.inlineDiffCache["thread-2:branch:origin/main...feat/y:src/b.ts"]).toBe("diff-b"); + expect(state.inlineDiffCache["thread-1:branch:origin/main...feat/x:1:src/a.ts"]).toBeUndefined(); + expect(state.inlineDiffCache["thread-2:branch:origin/main...feat/y:1:src/b.ts"]).toBe("diff-b"); + }); + + it("keeps revisions of the same file under distinct keys", () => { + const { cacheInlineDiff } = useDiffStore.getState(); + cacheInlineDiff("thread-1", "branch", "origin/main...feat/x", "src/a.ts", "diff-v1", 1); + cacheInlineDiff("thread-1", "branch", "origin/main...feat/x", "src/a.ts", "diff-v2", 2); + + const state = useDiffStore.getState(); + expect(state.inlineDiffCache["thread-1:branch:origin/main...feat/x:1:src/a.ts"]).toBe("diff-v1"); + expect(state.inlineDiffCache["thread-1:branch:origin/main...feat/x:2:src/a.ts"]).toBe("diff-v2"); }); it("evicts cumulative inline diff cache when snapshots refresh", () => { const { cacheInlineDiff, setSnapshots } = useDiffStore.getState(); - cacheInlineDiff("thread-1", "cumulative", "thread-1", "src/a.ts", "old"); - cacheInlineDiff("thread-1", "snapshot", "s1", "src/a.ts", "snapshot"); + cacheInlineDiff("thread-1", "cumulative", "thread-1", "src/a.ts", "old", 0); + cacheInlineDiff("thread-1", "snapshot", "s1", "src/a.ts", "snapshot", 0); setSnapshots("thread-1", []); const state = useDiffStore.getState(); - expect(state.inlineDiffCache["thread-1:cumulative:thread-1:src/a.ts"]).toBeUndefined(); - expect(state.inlineDiffCache["thread-1:snapshot:s1:src/a.ts"]).toBe("snapshot"); + expect(state.inlineDiffCache["thread-1:cumulative:thread-1:0:src/a.ts"]).toBeUndefined(); + expect(state.inlineDiffCache["thread-1:snapshot:s1:0:src/a.ts"]).toBe("snapshot"); }); }); diff --git a/apps/web/src/components/diff/ReviewDiffView.tsx b/apps/web/src/components/diff/ReviewDiffView.tsx index f720d8112..dcaa95d70 100644 --- a/apps/web/src/components/diff/ReviewDiffView.tsx +++ b/apps/web/src/components/diff/ReviewDiffView.tsx @@ -11,7 +11,7 @@ import { type CodeViewReactOptions, } from "@pierre/diffs/react"; import { ChevronRight, MessageCircle } from "lucide-react"; -import { useDiffStore, type SelectedFile } from "@/stores/diffStore"; +import { inlineDiffCacheKey, useDiffStore, type SelectedFile } from "@/stores/diffStore"; import { useWorkspaceStore } from "@/features/projects/state/workspaceStore"; import { usePreviewAnnotationStore, @@ -39,6 +39,7 @@ type DiffRowMeta = type DiffItem = CodeViewItem; const EMPTY_ANNOTATIONS: SavedDiffAnnotation[] = []; +const EMPTY_PATCHES: Record = {}; /** Comparison sources whose old/new contents can be read from git refs. */ const HYDRATABLE_SOURCES: ReadonlySet = new Set([ @@ -253,15 +254,27 @@ export function ReviewDiffView({ const shikiTheme = useShikiTheme(); - const [patches, setPatches] = useState>(() => { + // Patch contents are scoped to the comparison identity: a revision bump or + // a source/id/thread switch must refetch rather than relabel the previous + // identity's bytes under the new metadata. + const patchScope = `${threadId}:${source}:${id}:${cacheVersion}`; + const seedPatches = (): Record => { const cache = useDiffStore.getState().inlineDiffCache; const seeded: Record = {}; for (const file of files) { - const cached = cache[`${threadId}:${source}:${id}:${file.path}`]; + const cached = cache[inlineDiffCacheKey(threadId, source, id, file.path, cacheVersion)]; if (cached !== undefined) seeded[file.path] = cached; } return seeded; - }); + }; + const [patchState, setPatchState] = useState(() => ({ + scope: patchScope, + byPath: seedPatches(), + })); + if (patchState.scope !== patchScope) { + setPatchState({ scope: patchScope, byPath: seedPatches() }); + } + const patches = patchState.scope === patchScope ? patchState.byPath : EMPTY_PATCHES; const [expanded, setExpanded] = useState>( () => new Set((bulkDiffExpand?.expand ?? defaultFilesExpanded) ? files.map((f) => f.path) : []), ); @@ -305,31 +318,49 @@ export function ReviewDiffView({ const diffCache = useRef(new FileDiffCache()).current; const fileDiffs = useMemo(() => { const out: Record = {}; - const scope = `${threadId}:${source}:${id}:${cacheVersion}`; for (const [path, patch] of Object.entries(patches)) { - const fileDiff = diffCache.get(scope, path, patch); + const fileDiff = diffCache.get(patchScope, path, patch); if (fileDiff) out[path] = fileDiff; } return out; - }, [patches, id, source, threadId, cacheVersion, diffCache]); + }, [patches, patchScope, diffCache]); - // Lazy-load the patch for every expanded file missing one. + // Lazy-load the patch for every expanded file missing one. In-flight keys + // dedupe requests: each resolution changes `patches`, which re-runs this + // effect while sibling fetches are still pending. + const pendingPatches = useRef(new Set()); useEffect(() => { const transport = getTransport(); + const scope = patchScope; for (const file of files) { if (!effectiveExpanded.has(file.path) || patches[file.path] !== undefined) continue; const path = file.path; + const pendingKey = `${scope}${path}`; + if (pendingPatches.current.has(pendingKey)) continue; + pendingPatches.current.add(pendingKey); void loadFileDiff(transport, source, id, path, threadId) .then((result) => { - setPatches((prev) => (prev[path] === undefined ? { ...prev, [path]: result } : prev)); - useDiffStore.getState().cacheInlineDiff(threadId, source, id, path, result); + // Responses from a superseded identity land after the scope reset; + // the scope tag keeps them out of the mounted comparison, and the + // versioned cache key keeps them out of any later seed. + setPatchState((prev) => + prev.scope === scope && prev.byPath[path] === undefined + ? { scope, byPath: { ...prev.byPath, [path]: result } } + : prev, + ); + useDiffStore.getState().cacheInlineDiff(threadId, source, id, path, result, cacheVersion); }) .catch((error) => { console.warn("[ReviewDiffView] load failed", path, error); - setPatches((prev) => (prev[path] === undefined ? { ...prev, [path]: "" } : prev)); - }); + setPatchState((prev) => + prev.scope === scope && prev.byPath[path] === undefined + ? { scope, byPath: { ...prev.byPath, [path]: "" } } + : prev, + ); + }) + .finally(() => pendingPatches.current.delete(pendingKey)); } - }, [effectiveExpanded, files, id, patches, source, threadId]); + }, [effectiveExpanded, files, patchScope, patches, source, id, threadId, cacheVersion]); // Bulk expand/collapse arrives as a store command; subscriptions run outside // the render pass, unlike an effect watching the nonce. diff --git a/apps/web/src/components/diff/__tests__/ReviewDiffView.refresh.test.tsx b/apps/web/src/components/diff/__tests__/ReviewDiffView.refresh.test.tsx new file mode 100644 index 000000000..ab33ee9f5 --- /dev/null +++ b/apps/web/src/components/diff/__tests__/ReviewDiffView.refresh.test.tsx @@ -0,0 +1,138 @@ +import { act, cleanup, render, screen, waitFor } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { ReviewDiffView } from "@/components/diff/ReviewDiffView"; +import { useDiffStore } from "@/stores/diffStore"; +import { useWorkspaceStore } from "@/features/projects/state/workspaceStore"; + +const transport = vi.hoisted(() => ({ + getWorkingTreeDiff: vi.fn(), + getBranchDiff: vi.fn(), +})); + +vi.mock("@/transport", async (original) => ({ + ...(await original()), + getTransport: () => transport, +})); + +// Observe the exact item contents delivered by the real component to its +// renderer; the bug under test lives in ReviewDiffView's patch state, not in +// pierre's paint pass. +vi.mock("@pierre/diffs/react", async (original) => ({ + ...(await original()), + CodeView: ({ items }: { items: unknown }) => ( + 
{JSON.stringify(items)}
+ ), +})); + +const patch = (text: string) => + `diff --git a/file.txt b/file.txt\nindex 1111111..2222222 100644\n--- a/file.txt\n+++ b/file.txt\n@@ -1 +1 @@\n-before\n+${text}\n`; + +const props = { + files: [{ path: "file.txt", previousPath: null, changeType: "modified" as const, binary: false }], + source: "unstaged" as const, + id: "workspace-fixture", + threadId: "thread-fixture", + cacheVersion: 1, + defaultFilesExpanded: true, + jumpTarget: null, + onJumpSettled: () => {}, + highlightPath: null, + renderMode: "unified" as const, + lineWrap: true, +}; + +beforeEach(() => { + vi.clearAllMocks(); + useWorkspaceStore.setState({ activeWorkspaceId: "workspace-fixture", threads: [] }); + useDiffStore.setState({ inlineDiffCache: {}, bulkDiffExpand: null }); + transport.getWorkingTreeDiff.mockResolvedValue(patch("FIRST_VERSION")); + transport.getBranchDiff.mockResolvedValue(patch("NEW_BRANCH")); +}); + +afterEach(cleanup); + +describe("ReviewDiffView refresh", () => { + it("refetches an expanded file when cacheVersion changes", async () => { + const view = render(); + await waitFor(() => + expect(screen.getByTestId("items").textContent).toContain("FIRST_VERSION"), + ); + + transport.getWorkingTreeDiff.mockResolvedValue(patch("SECOND_VERSION")); + act(() => useDiffStore.getState().bumpDiffRevision("thread-fixture")); + view.rerender(); + + await waitFor(() => + expect(screen.getByTestId("items").textContent).toContain("SECOND_VERSION"), + ); + expect(transport.getWorkingTreeDiff.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(screen.getByTestId("items").textContent).not.toContain("FIRST_VERSION"); + }); + + it("refetches a same-named file when the comparison range changes", async () => { + const view = render(); + await waitFor(() => + expect(screen.getByTestId("items").textContent).toContain("NEW_BRANCH"), + ); + + transport.getBranchDiff.mockResolvedValue(patch("BRANCH_B")); + view.rerender( + , + ); + + await waitFor(() => + expect(screen.getByTestId("items").textContent).toContain("BRANCH_B"), + ); + expect(transport.getBranchDiff.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(screen.getByTestId("items").textContent).not.toContain("NEW_BRANCH"); + }); + + it("drops an in-flight response from a superseded comparison", async () => { + let resolveFirst: (value: string) => void = () => {}; + transport.getBranchDiff.mockImplementationOnce( + () => new Promise((resolve) => { resolveFirst = resolve; }), + ); + const view = render(); + await waitFor(() => expect(transport.getBranchDiff).toHaveBeenCalledTimes(1)); + + transport.getBranchDiff.mockResolvedValue(patch("BRANCH_B")); + view.rerender( + , + ); + + // The superseded request resolves after the switch; its payload must not win. + await act(async () => resolveFirst(patch("STALE_A"))); + await waitFor(() => + expect(screen.getByTestId("items").textContent).toContain("BRANCH_B"), + ); + expect(screen.getByTestId("items").textContent).not.toContain("STALE_A"); + }); + + it("does not seed a superseded in-flight response on remount", async () => { + let resolveFirst: (value: string) => void = () => {}; + transport.getWorkingTreeDiff.mockImplementationOnce( + () => new Promise((resolve) => { resolveFirst = resolve; }), + ); + const view = render(); + await waitFor(() => expect(transport.getWorkingTreeDiff).toHaveBeenCalledTimes(1)); + + transport.getWorkingTreeDiff.mockResolvedValue(patch("SECOND_VERSION")); + act(() => useDiffStore.getState().bumpDiffRevision("thread-fixture")); + view.rerender(); + await waitFor(() => + expect(screen.getByTestId("items").textContent).toContain("SECOND_VERSION"), + ); + + // The revision-1 request resolves late; its payload lands under the + // revision-1 cache key, which a revision-2 remount must never seed from. + await act(async () => resolveFirst(patch("STALE_V1"))); + view.unmount(); + render(); + + await waitFor(() => + expect(screen.getByTestId("items").textContent).toContain("SECOND_VERSION"), + ); + expect(screen.getByTestId("items").textContent).not.toContain("STALE_V1"); + expect(transport.getWorkingTreeDiff).toHaveBeenCalledTimes(2); + }); +}); diff --git a/apps/web/src/stores/diffStore.ts b/apps/web/src/stores/diffStore.ts index 2ffa761a8..116c44324 100644 --- a/apps/web/src/stores/diffStore.ts +++ b/apps/web/src/stores/diffStore.ts @@ -245,14 +245,20 @@ export function createDefaultRightPanelState(): RightPanelState { }); } -/** Stable cache key for one inline diff payload. */ -function inlineDiffCacheKey( +/** + * Stable cache key for one inline diff payload. `cacheVersion` sits between + * the id and path so a response fetched under an older revision can never be + * read back under a newer one, while every existing `scopeId:` prefix eviction + * still matches. + */ +export function inlineDiffCacheKey( threadId: string, source: string, id: string, filePath: string, + cacheVersion: string | number, ): string { - return `${threadId}:${source}:${id}:${filePath}`; + return `${threadId}:${source}:${id}:${cacheVersion}:${filePath}`; } /** Drop inline diff cache entries matching a stable key prefix. */ @@ -675,9 +681,9 @@ interface DiffState { /** Set summary loading state. */ setSummaryLoading: (loading: boolean) => void; /** Cache a fetched inline diff so it survives component unmounts. */ - cacheInlineDiff: (threadId: string, source: string, id: string, filePath: string, data: string) => void; + cacheInlineDiff: (threadId: string, source: string, id: string, filePath: string, data: string, cacheVersion: string | number) => void; /** Retrieve a cached inline diff, or undefined if not cached. */ - getCachedInlineDiff: (threadId: string, source: string, id: string, filePath: string) => string | undefined; + getCachedInlineDiff: (threadId: string, source: string, id: string, filePath: string, cacheVersion: string | number) => string | undefined; /** Bump a mutable diff scope so mounted file rows refetch against the latest checkout. */ bumpDiffRevision: (scopeId: string) => void; /** Persist the omnibox URL for a thread's embedded preview. */ @@ -1063,12 +1069,12 @@ export const useDiffStore = create((set, get) => ({ setDiffLoading: (loading) => set({ diffLoading: loading }), setSummaryRecord: (record) => set({ summaryRecord: record }), setSummaryLoading: (loading) => set({ summaryLoading: loading }), - cacheInlineDiff: (threadId, source, id, filePath, data) => + cacheInlineDiff: (threadId, source, id, filePath, data, cacheVersion) => set((s) => ({ - inlineDiffCache: { ...s.inlineDiffCache, [inlineDiffCacheKey(threadId, source, id, filePath)]: data }, + inlineDiffCache: { ...s.inlineDiffCache, [inlineDiffCacheKey(threadId, source, id, filePath, cacheVersion)]: data }, })), - getCachedInlineDiff: (threadId, source, id, filePath) => - get().inlineDiffCache[inlineDiffCacheKey(threadId, source, id, filePath)], + getCachedInlineDiff: (threadId, source, id, filePath, cacheVersion) => + get().inlineDiffCache[inlineDiffCacheKey(threadId, source, id, filePath, cacheVersion)], bumpDiffRevision: (scopeId) => set((s) => ({ diffRevisionByScope: {