From 23af18f5b7e176d61f8cfa9f17e1e09c87b2e7c2 Mon Sep 17 00:00:00 2001 From: Guillermo Casanova Date: Wed, 30 Sep 2026 22:45:43 -0300 Subject: [PATCH 1/3] fix(web): PR code tab no longer reloads on background refreshes --- .../pullRequest/PullRequestCodeTab.tsx | 48 ++++++++++++++++++- .../pullRequest/PullRequestDetailPanel.tsx | 9 ++-- 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 251f3cd49f95..7c22de506009 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -24,9 +24,17 @@ import { TextWrapIcon, TriangleAlertIcon, } from "lucide-react"; -import { useAtomRefresh } from "@effect/atom-react"; +import { RegistryContext, useAtomRefresh } from "@effect/atom-react"; import * as Schema from "effect/Schema"; -import { useCallback, useEffect, useMemo, useRef, useState, type ReactNode } from "react"; +import { + useCallback, + useContext, + useEffect, + useMemo, + useRef, + useState, + type ReactNode, +} from "react"; import { useLocalStorage } from "~/hooks/useLocalStorage"; import { useClientSettings, useUpdateClientSettings } from "~/hooks/useSettings"; @@ -203,6 +211,7 @@ function PullRequestCodeTab({ onAddToAgentSelection, onRefresh, refreshToken = 0, + backgroundRefreshToken = 0, }: { environmentId: EnvironmentId; reference: PullRequestRef; @@ -219,6 +228,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; }) { const { resolvedTheme } = useTheme(); const settings = useClientSettings(); @@ -437,6 +447,40 @@ function PullRequestCodeTab({ refreshFirstDiffPage(); refreshFilesViewed(); }, [refreshToken, scopeKey, refreshFirstDiffPage, refreshFilesViewed]); + // A background refresh keeps the first page on screen and re-reads it in place. Later pages + // are dropped and their cached answers invalidated, so scrolling reads them fresh rather than + // trusting an unchanged first page to vouch for the rest of the diff. + const registry = useContext(RegistryContext); + const appliedBackgroundRefreshToken = useRef(backgroundRefreshToken); + useEffect(() => { + if (appliedBackgroundRefreshToken.current === backgroundRefreshToken) return; + appliedBackgroundRefreshToken.current = backgroundRefreshToken; + for (const slice of loadedSlices.slice(1)) { + if (slice.cursor === null) continue; + registry.refresh( + pullRequestEnvironment.diff({ + environmentId, + input: { ...reference, cursor: slice.cursor, ...(commit === null ? {} : { commit }) }, + }), + ); + } + setSliceState((previous) => ({ + key: previous.key, + cursor: null, + slices: previous.slices.slice(0, 1), + })); + refreshFirstDiffPage(); + refreshFilesViewed(); + }, [ + backgroundRefreshToken, + commit, + environmentId, + loadedSlices, + reference, + refreshFirstDiffPage, + refreshFilesViewed, + registry, + ]); 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. diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index 1b42fdd4481a..45bd42f0b0e5 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -818,7 +818,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, ); @@ -830,7 +832,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]); @@ -2774,7 +2776,8 @@ export function PullRequestDetailPanel({ fixFindingLabel={handoffLabels.fixFinding} onFixFinding={startFixFinding} onRefresh={refreshDetail} - refreshToken={codeRefreshToken} + refreshToken={refreshToken} + backgroundRefreshToken={backgroundRefreshToken} /> From e537c0fd23b6d2f424999a81d2dd03dc2157212c Mon Sep 17 00:00:00 2001 From: Guillermo Casanova Date: Thu, 1 Oct 2026 00:10:48 -0300 Subject: [PATCH 2/3] fix(web): revalidate every loaded diff page in place on background refresh --- .../pullRequest/PullRequestCodeTab.tsx | 68 +++++-------------- .../pullRequest/pullRequestDiff.logic.test.ts | 38 +++++++++++ .../pullRequest/pullRequestDiff.logic.ts | 48 ++++++++++++- 3 files changed, 103 insertions(+), 51 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 7c22de506009..f9adf5a984c2 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -4,7 +4,6 @@ import type { EnvironmentId, PullRequestDetailView, PullRequestDiffSide, - PullRequestOmittedFileStat, PullRequestRef, PullRequestReviewPosition, PullRequestReviewThread, @@ -89,8 +88,10 @@ import { PendingReviewCommentCard, ReviewThreadCard } from "./PullRequestReviewA import { isFileDiffCollapsed, isLineInFileDiff, + reconcileDiffSlices, toggleFileDiffFoldForViewed, type DiffFoldOverride, + type DiffSlice, } from "./pullRequestDiff.logic"; import { PullRequestDiffStat, PullRequestMetaLine } from "./pullRequestPresentation"; import { usePullRequestFilesViewed } from "./usePullRequestFilesViewed"; @@ -116,16 +117,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; -} - /** * 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 @@ -300,44 +291,22 @@ function PullRequestCodeTab({ // text would cost more with every slice, which is the wall the slicing exists to remove. useEffect(() => { const data = diffQuery.data; - if (data === null) return; + // A refresh keeps answering with the previous value until the new one lands. + if (data === null || diffQuery.isPending) 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] }; + }); + return next.slices === slices && next.cursor === previous.cursor && previous.key === scopeKey + ? previous + : { key: scopeKey, ...next }; }); - }, [cursor, diffQuery.data, scopeKey]); + }, [cursor, diffQuery.data, diffQuery.isPending, 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( @@ -447,15 +416,15 @@ function PullRequestCodeTab({ refreshFirstDiffPage(); refreshFilesViewed(); }, [refreshToken, scopeKey, refreshFirstDiffPage, refreshFilesViewed]); - // A background refresh keeps the first page on screen and re-reads it in place. Later pages - // are dropped and their cached answers invalidated, so scrolling reads them fresh rather than - // trusting an unchanged first page to vouch for the rest of the diff. + // 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 registry = useContext(RegistryContext); const appliedBackgroundRefreshToken = useRef(backgroundRefreshToken); useEffect(() => { if (appliedBackgroundRefreshToken.current === backgroundRefreshToken) return; appliedBackgroundRefreshToken.current = backgroundRefreshToken; - for (const slice of loadedSlices.slice(1)) { + for (const slice of loadedSlices) { if (slice.cursor === null) continue; registry.refresh( pullRequestEnvironment.diff({ @@ -464,11 +433,7 @@ function PullRequestCodeTab({ }), ); } - setSliceState((previous) => ({ - key: previous.key, - cursor: null, - slices: previous.slices.slice(0, 1), - })); + setSliceState((previous) => ({ ...previous, cursor: null })); refreshFirstDiffPage(); refreshFilesViewed(); }, [ @@ -648,8 +613,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; diff --git a/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts index 181ebe6cfec1..85acf8d22eca 100644 --- a/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts +++ b/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")], + }); + }); +}); diff --git a/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts b/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts index b7b836cb3b5b..0657034d6e5a 100644 --- a/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts +++ b/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; +} + +/** + * 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, + answer: DiffSlice, +): { readonly cursor: string | null; readonly slices: ReadonlyArray } { + 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 + ); + }) + ); +} From 699de7dc8288d6a779ab3346db1f4cacde228b30 Mon Sep 17 00:00:00 2001 From: Guillermo Casanova Date: Thu, 1 Oct 2026 02:32:37 -0300 Subject: [PATCH 3/3] fix(web): accept only fresh answers while revalidating loaded diff pages --- .../pullRequest/PullRequestCodeTab.tsx | 55 ++++++------------- 1 file changed, 18 insertions(+), 37 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index f9adf5a984c2..a6b60e4bc083 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -23,17 +23,9 @@ import { TextWrapIcon, TriangleAlertIcon, } from "lucide-react"; -import { RegistryContext, useAtomRefresh } from "@effect/atom-react"; +import { useAtomRefresh } from "@effect/atom-react"; import * as Schema from "effect/Schema"; -import { - useCallback, - useContext, - useEffect, - useMemo, - useRef, - useState, - type ReactNode, -} from "react"; +import { useCallback, useEffect, useMemo, useRef, useState, type ReactNode } from "react"; import { useLocalStorage } from "~/hooks/useLocalStorage"; import { useClientSettings, useUpdateClientSettings } from "~/hooks/useSettings"; @@ -289,24 +281,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; // A refresh keeps answering with the previous value until the new one lands. - if (data === null || diffQuery.isPending) return; + 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 = reconcileDiffSlices(slices, { cursor, - patch: data.patch, - truncated: data.truncated, - nextCursor: data.nextCursor, - omittedFileStats: data.omittedFileStats ?? [], + 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, diffQuery.isPending, 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( @@ -419,33 +418,15 @@ function PullRequestCodeTab({ // 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 registry = useContext(RegistryContext); const appliedBackgroundRefreshToken = useRef(backgroundRefreshToken); useEffect(() => { if (appliedBackgroundRefreshToken.current === backgroundRefreshToken) return; appliedBackgroundRefreshToken.current = backgroundRefreshToken; - for (const slice of loadedSlices) { - if (slice.cursor === null) continue; - registry.refresh( - pullRequestEnvironment.diff({ - environmentId, - input: { ...reference, cursor: slice.cursor, ...(commit === null ? {} : { commit }) }, - }), - ); - } + revalidateAfter.current = Date.now(); setSliceState((previous) => ({ ...previous, cursor: null })); refreshFirstDiffPage(); refreshFilesViewed(); - }, [ - backgroundRefreshToken, - commit, - environmentId, - loadedSlices, - reference, - refreshFirstDiffPage, - refreshFilesViewed, - registry, - ]); + }, [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.