fix(web): PR code tab no longer reloads when turns finish or the PR updates

apps/web/src/components/pullRequest/PullRequestCodeTab.tsx

@@ -4,7 +4,6 @@ import type {
   EnvironmentId,
   PullRequestDetailView,
   PullRequestDiffSide,
-  PullRequestOmittedFileStat,
   PullRequestRef,
   PullRequestReviewPosition,
   PullRequestReviewThread,
@@ -90,8 +89,10 @@ import {
 import {
   isFileDiffCollapsed,
   isLineInFileDiff,
+  reconcileDiffSlices,
   toggleFileDiffFoldForViewed,
   type DiffFoldOverride,
+  type DiffSlice,
 } from "./pullRequestDiff.logic";
 import { PullRequestDiffStat, PullRequestMetaLine } from "./pullRequestPresentation";
 import { usePullRequestFilesViewed } from "./usePullRequestFilesViewed";
@@ -117,16 +118,6 @@ const COMMIT_PAGE_SIZE = 10;
 
 const PULL_REQUEST_FILE_TREE_STORAGE_KEY = "t3code.pullRequestFileTreeOpen";
 
-/** One answer from the host: a whole number of files, and where the next one carries on. */
-interface DiffSlice {
-  /** What was asked for, null being the first slice. Identifies the slice among the loaded ones. */
-  readonly cursor: string | null;
-  readonly patch: string;
-  readonly truncated: boolean;
-  readonly nextCursor: string | null;
-  readonly omittedFileStats: ReadonlyArray<PullRequestOmittedFileStat>;
-}
-
 /**
  * The viewer's own per-file counts are hidden and drawn from this side of its shadow root
  * instead: its counts are hunk sums, and a file whose hunks the host withheld would read as
@@ -191,6 +182,7 @@ function PullRequestCodeTab({
   onAddToAgentSelection,
   onRefresh,
   refreshToken = 0,
+  backgroundRefreshToken = 0,
   revealRequest = null,
 }: {
   scrollerRef: (node: HTMLDivElement | null) => void;
@@ -209,6 +201,7 @@ function PullRequestCodeTab({
   onRefresh: () => void;
   /** Bumped by the panel's refresh button: drop the accumulated pages and re-read the diff. */
   refreshToken?: number;
+  backgroundRefreshToken?: number;
   /** A file another tab asked to show. A new nonce asks again for the same path. */
   revealRequest?: { readonly path: string; readonly nonce: number } | null;
 }) {
@@ -282,46 +275,31 @@ function PullRequestCodeTab({
   );
   // Each answer is kept as its own slice. Concatenating the patches and re-parsing the growing
   // text would cost more with every slice, which is the wall the slicing exists to remove.
+  // When the last background refresh began. An answer older than that is a cached page, so it is
+  // re-read before it may vouch for the page on screen.
+  const revalidateAfter = useRef(0);
+  const { data: diffData, dataUpdatedAt, error: diffError, isPending, refresh } = diffQuery;
   useEffect(() => {
-    const data = diffQuery.data;
-    if (data === null) return;
+    // A refresh keeps answering with the previous value until the new one lands.
+    if (diffData === null || isPending || diffError !== null) return;
+    if ((dataUpdatedAt ?? 0) < revalidateAfter.current) {
+      refresh();
+      return;
+    }
     setSliceState((previous) => {
       const slices = previous.key === scopeKey ? previous.slices : NO_SLICES;
-      const next = {
+      const next = reconcileDiffSlices(slices, {
         cursor,
-        patch: data.patch,
-        truncated: data.truncated,
-        nextCursor: data.nextCursor,
-        omittedFileStats: data.omittedFileStats ?? [],
-      };
-      const index = slices.findIndex((slice) => slice.cursor === cursor);
-      if (index === -1) {
-        return { key: scopeKey, cursor, slices: [...slices, next] };
-      }
-      const existing = slices[index];
-      if (
-        existing !== undefined &&
-        existing.patch === next.patch &&
-        existing.truncated === next.truncated &&
-        existing.nextCursor === next.nextCursor &&
-        existing.omittedFileStats.length === next.omittedFileStats.length &&
-        existing.omittedFileStats.every((file, index) => {
-          const refreshed = next.omittedFileStats[index];
-          return (
-            refreshed !== undefined &&
-            refreshed.path === file.path &&
-            refreshed.additions === file.additions &&
-            refreshed.deletions === file.deletions
-          );
-        })
-      ) {
-        return previous;
-      }
-      // A page that came back different means the diff moved under the review. The slices
-      // after it go with the replacement: their cursors were positions in the old diff.
-      return { key: scopeKey, cursor, slices: [...slices.slice(0, index), next] };
+        patch: diffData.patch,
+        truncated: diffData.truncated,
+        nextCursor: diffData.nextCursor,
+        omittedFileStats: diffData.omittedFileStats ?? [],
+      });
+      return next.slices === slices && next.cursor === previous.cursor && previous.key === scopeKey
+        ? previous
+        : { key: scopeKey, ...next };
     });
-  }, [cursor, diffQuery.data, scopeKey]);
+  }, [cursor, dataUpdatedAt, diffData, diffError, isPending, refresh, scopeKey]);
   // The refresh button rereads from the first page rather than the page the reader is on:
   // pages are positions in one snapshot of the diff, and a fresh snapshot starts over.
   const refreshFirstDiffPage = useAtomRefresh(
@@ -431,6 +409,18 @@ function PullRequestCodeTab({
     refreshFirstDiffPage();
     refreshFilesViewed();
   }, [refreshToken, scopeKey, refreshFirstDiffPage, refreshFilesViewed]);
+  // A background refresh keeps every loaded page on screen and re-reads them in order from the
+  // first: each unchanged answer moves on to the next page, and a changed one replaces its page
+  // and drops the pages after it, whose cursors were positions in the old diff.
+  const appliedBackgroundRefreshToken = useRef(backgroundRefreshToken);
+  useEffect(() => {
+    if (appliedBackgroundRefreshToken.current === backgroundRefreshToken) return;
+    appliedBackgroundRefreshToken.current = backgroundRefreshToken;
+    revalidateAfter.current = Date.now();
+    setSliceState((previous) => ({ ...previous, cursor: null }));
+    refreshFirstDiffPage();
+    refreshFilesViewed();
+  }, [backgroundRefreshToken, refreshFirstDiffPage, refreshFilesViewed]);
   const nextCursor = loadedSlices.at(-1)?.nextCursor ?? null;
   // What a slice withheld: the host declining to inline part of it, or a patch the viewer could
   // not structure and so dropped. Neither says anything about there being more to fetch.
@@ -615,8 +605,11 @@ function PullRequestCodeTab({
 
   // A failed slice must not be asked for again on its own. The files already loaded keep the
   // sentinel on screen, so re-arming it after a failure would request the same slice forever.
+  // While a background refresh is still re-reading earlier pages, the cursor sits behind the last
+  // one and the next page waits for it to catch up.
   const canLoadNextSlice =
     nextCursor !== null &&
+    cursor === (loadedSlices.at(-1)?.cursor ?? null) &&
     nextCursor !== cursor &&
     !diffQuery.isPending &&
     diffQuery.error === null;

apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx

@@ -872,7 +872,9 @@ export function PullRequestDetailPanel({
     nativeStackQuery.refresh();
   }, [activityQuery.refresh, detailQuery.refresh, nativeStackQuery.refresh]);
   const [refreshToken, setRefreshToken] = useState(0);
-  const codeRefreshToken = refreshToken + (turnRefresh ?? 0);
+  // A new revision or a finished turn may have moved the diff, but nobody asked to start over.
+  const [revisionToken, setRevisionToken] = useState(0);
+  const backgroundRefreshToken = revisionToken + (turnRefresh ?? 0);
   const activityRevision = useRef<{ readonly key: string; readonly updatedAt: string } | null>(
     null,
   );
@@ -884,7 +886,7 @@ export function PullRequestDetailPanel({
       // mutation's activity refresh can leave SWR displaying its previous value.
       if (activityQuery.isPending) return;
       activityQuery.refresh();
-      setRefreshToken((token) => token + 1);
+      setRevisionToken((token) => token + 1);
     }
     activityRevision.current = next;
   }, [activityQuery.isPending, activityQuery.refresh, coreDetail, tabScopeKey]);
@@ -3005,7 +3007,8 @@ export function PullRequestDetailPanel({
                     fixFindingLabel={handoffLabels.fixFinding}
                     {...(canFixFindings ? { onFixFinding: startFixFinding } : {})}
                     onRefresh={refreshDetail}
-                    refreshToken={codeRefreshToken}
+                    refreshToken={refreshToken}
+                    backgroundRefreshToken={backgroundRefreshToken}
                     revealRequest={codeRevealRequest}
                   />
                 </Suspense>

apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts

@@ -4,6 +4,8 @@ import { describe, expect, it } from "vite-plus/test";
 import {
   isFileDiffCollapsed,
   isLineInFileDiff,
+  reconcileDiffSlices,
+  type DiffSlice,
   toggleFileDiffFoldForViewed,
 } from "./pullRequestDiff.logic";
 
@@ -110,3 +112,39 @@ describe("toggleFileDiffFoldForViewed", () => {
     expect([...toggleFileDiffFoldForViewed("a.ts", false, null, toggled)]).toEqual(["b.ts"]);
   });
 });
+
+describe("reconcileDiffSlices", () => {
+  const slice = (cursor: string | null, patch: string, nextCursor: string | null): DiffSlice => ({
+    cursor,
+    patch,
+    truncated: false,
+    nextCursor,
+    omittedFileStats: [],
+  });
+  const loaded = [slice(null, "a", "2"), slice("2", "b", "3"), slice("3", "c", null)];
+
+  it("appends a page it has not seen", () => {
+    expect(reconcileDiffSlices(loaded.slice(0, 1), slice("2", "b", "3"))).toEqual({
+      cursor: "2",
+      slices: loaded.slice(0, 2),
+    });
+  });
+
+  it("keeps every page and walks to the next one while answers are unchanged", () => {
+    const first = reconcileDiffSlices(loaded, slice(null, "a", "2"));
+    expect(first).toEqual({ cursor: "2", slices: loaded });
+    expect(first.slices).toBe(loaded);
+    expect(reconcileDiffSlices(loaded, slice("2", "b", "3")).cursor).toBe("3");
+    expect(reconcileDiffSlices(loaded, slice("3", "c", null))).toEqual({
+      cursor: "3",
+      slices: loaded,
+    });
+  });
+
+  it("replaces a changed later page and drops the pages after it", () => {
+    expect(reconcileDiffSlices(loaded, slice("2", "b2", "3"))).toEqual({
+      cursor: "2",
+      slices: [loaded[0], slice("2", "b2", "3")],
+    });
+  });
+});

apps/web/src/components/pullRequest/pullRequestDiff.logic.ts

@@ -1,5 +1,5 @@
 import type { FileDiffMetadata } from "@pierre/diffs";
-import type { PullRequestDiffSide } from "@t3tools/contracts";
+import type { PullRequestDiffSide, PullRequestOmittedFileStat } from "@t3tools/contracts";
 
 /**
  * Whether a conversation's line is really in this file's hunks.
@@ -62,3 +62,49 @@ export function toggleFileDiffFoldForViewed(
   else next.add(fileKey);
   return next;
 }
+
+/** One answer from the host: a whole number of files, and where the next one carries on. */
+export interface DiffSlice {
+  /** What was asked for, null being the first slice. Identifies the slice among the loaded ones. */
+  readonly cursor: string | null;
+  readonly patch: string;
+  readonly truncated: boolean;
+  readonly nextCursor: string | null;
+  readonly omittedFileStats: ReadonlyArray<PullRequestOmittedFileStat>;
+}
+
+/**
+ * Folds one answer into the loaded slices. A new cursor appends. An unchanged answer keeps every
+ * slice and moves the cursor to the next loaded one, so a background refresh re-reads them in
+ * turn. A changed answer replaces its slice and drops the ones after it.
+ */
+export function reconcileDiffSlices(
+  slices: ReadonlyArray<DiffSlice>,
+  answer: DiffSlice,
+): { readonly cursor: string | null; readonly slices: ReadonlyArray<DiffSlice> } {
+  const index = slices.findIndex((slice) => slice.cursor === answer.cursor);
+  if (index === -1) return { cursor: answer.cursor, slices: [...slices, answer] };
+  const existing = slices[index];
+  if (existing !== undefined && isSameDiffSlice(existing, answer)) {
+    return { cursor: slices[index + 1]?.cursor ?? answer.cursor, slices };
+  }
+  return { cursor: answer.cursor, slices: [...slices.slice(0, index), answer] };
+}
+
+function isSameDiffSlice(existing: DiffSlice, next: DiffSlice): boolean {
+  return (
+    existing.patch === next.patch &&
+    existing.truncated === next.truncated &&
+    existing.nextCursor === next.nextCursor &&
+    existing.omittedFileStats.length === next.omittedFileStats.length &&
+    existing.omittedFileStats.every((file, index) => {
+      const refreshed = next.omittedFileStats[index];
+      return (
+        refreshed !== undefined &&
+        refreshed.path === file.path &&
+        refreshed.additions === file.additions &&
+        refreshed.deletions === file.deletions
+      );
+    })
+  );
+}