bug(review): refreshed revisions and changed comparisons retain stale patch contents #1732

Summary

Priority: P1-high. Audit finding: B3.

Verified on 8384f3013f46c2ab9b0583681a90f30c04a07012 (Windows 11; Bun 1.4.0; Vitest 4.1.4). This is a report of executed behavior, not a static-analysis suspicion.

Location: apps/web/src/components/diff/ReviewDiffView.tsx:256–264,301–330.

Reproduction

1. Render an expanded modified file.txt through the real ReviewDiffView; the initial patch contains FIRST_VERSION.
2. Change the transport's next response to SECOND_VERSION.
3. Invoke the real diff-revision invalidation and rerender with the updated cache version.
4. Separately switch a mounted branch comparison from main...branchA to main...branchB, retaining the same file path but supplying different content.

Expected: request and render the new patch.

Observed: one fetch total in both cases. Old content remains, but its cache key/version is relabelled with the new revision or comparison. For example, a version-2 cache key still contains FIRST_VERSION.

Root cause: the component-local patches state is keyed only by path. Its loader skips any path already present; changed cache/comparison props do not clear that state. Real parents pass changing props without forcing remount.

Impact: users can review stale code under current revision/comparison metadata. This is incorrect data supplied to the renderer, not a cosmetic repaint issue.

Origin check: #1616/#1650 concern delivering invalidation; #479 concerns thread-scoped cache keys; #781 concerns snapshot refresh. Here explicit invalidation reaches a mounted component and it still refuses to refetch. #1726 introduced the current renderer. No exact duplicate found.

Pinned source

apps/web/src/components/diff/ReviewDiffView.tsx

Verification boundary

Two independent runs exercised real ReviewDiffView, React, diff store, and Pierre parser. Only transport responses and the final CodeView renderer are controlled. Both assertions fail; fetchCount remains 1. This is a component/renderer-input reproduction, not a claimed full-browser Review incident.

Self-contained regression reproduction

Read AGENTS.md first and install the repository dependencies with its documented setup. Use a disposable checkout and fixture data, never a user branch/project/database. The source below is embedded so another agent does not need access to the audit author's local files. Tests assert correct behavior and are expected to fail on the audited commit.

Save the test as apps/web/src/__tests__/hermes-audit-8384-diff.test.tsx:

import { act, cleanup, render, screen, waitFor } from "@testing-library/react";
import { afterEach, beforeEach, 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<object>(), getTransport: () => transport }));
// Observe the exact item contents delivered by the real component to its renderer.
vi.mock("@pierre/diffs/react", async (original) => ({
  ...await original<object>(),
  CodeView: ({ items }: { items: unknown }) => <pre data-testid="items">{JSON.stringify(items)}</pre>,
}));
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);
it("refreshing the same file invalidates the rendered patch when cacheVersion changes", async () => {
  const view = render(<ReviewDiffView {...props} />);
  await waitFor(() => expect(screen.getByTestId("items").textContent).toContain("FIRST_VERSION"));
  transport.getWorkingTreeDiff.mockResolvedValue(patch("SECOND_VERSION"));
  act(() => useDiffStore.getState().bumpDiffRevision("thread-fixture"));
  view.rerender(<ReviewDiffView {...props} cacheVersion={2} />);
  await act(async () => {});
  console.log(JSON.stringify({ fetchCount: transport.getWorkingTreeDiff.mock.calls.length, containsOld: screen.getByTestId("items").textContent?.includes("FIRST_VERSION") }));
  expect(screen.getByTestId("items").textContent).toContain("SECOND_VERSION");
});
it("changing a comparison range replaces the patch for a same-named file", async () => {
  const view = render(<ReviewDiffView {...props} source="branch" id="main...branchA" />);
  await waitFor(() => expect(screen.getByTestId("items").textContent).toContain("NEW_BRANCH"));
  transport.getBranchDiff.mockResolvedValue(patch("BRANCH_B"));
  view.rerender(<ReviewDiffView {...props} source="branch" id="main...branchB" cacheVersion={2} />);
  await act(async () => {});
  console.log(JSON.stringify({ fetchCount: transport.getBranchDiff.mock.calls.length, containsOld: screen.getByTestId("items").textContent?.includes("NEW_BRANCH") }));
  expect(screen.getByTestId("items").textContent).toContain("BRANCH_B");
});

For the audit's tested focused harness, save this config as apps/web/hermes-audit-8384.config.ts:

import { defineConfig } from "vitest/config";
import { resolve } from "node:path";
export default defineConfig({
  resolve: { alias: { "@": resolve(import.meta.dirname, "src") } },
  esbuild: { jsx: "automatic" },
  test: { environment: "jsdom", setupFiles: "./src/test-setup.ts", pool: "forks", maxWorkers: 1, testTimeout: 10000 },
});

Run from repository root:

bun run --cwd apps/web test -- --config hermes-audit-8384.config.ts src/__tests__/hermes-audit-8384-diff.test.tsx

This harness uses the repository's aliases, jsdom and test setup but omits React compiler/Tailwind build plugins. Normal-config audit runs timed out; those timeouts are not evidence of this bug. The focused harness completed with semantic assertion failures. Integrate permanent regression coverage into the normal test setup as part of the fix.

Acceptance criteria

• Changing the revision for an expanded same-path file requests and renders the fresh patch.
• Changing comparison/source/thread identity cannot reuse patch contents from the previous identity.
• Do not relabel stale data with the new revision/comparison metadata.
• Protect against in-flight responses from an obsolete comparison replacing the current one.
• Keep lazy loading and expand/collapse behavior; add both refresh and same-path comparison-change regression tests.

Implementation constraints / handoff

• Keep the fix scoped to this root cause; follow repository instructions and workspace boundaries.
• Preserve the reproduction as a regression test and show red-before/green-after results.
• Run the relevant existing tests plus targeted lint/typecheck for touched files; do not use bun test or run the repo-wide suite contrary to AGENTS.md.
• Do not mutate live projects/databases during verification. Do not conceal test failures by weakening the assertions.
• Recheck current source if the branch has advanced from the pinned audit commit.