diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 25a8c10ad6b2..e6123ef0bd19 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, @@ -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; -} - /** * 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; diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index d68983937f19..17b385ee307e 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/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} /> 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 + ); + }) + ); +}