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: {