From 36b69c5b3d076e22c205da6fd7d90f406aea4c33 Mon Sep 17 00:00:00 2001 From: Jake Leventhal Date: Sun, 4 Oct 2026 10:35:53 -0400 Subject: [PATCH 01/17] fix(web): keep the review scope across agent turns An agent turn rebuilds the thread shell, and the completed-turn path always selected Changes. A review left on Uncommitted therefore snapped back on every turn. Keep the current scope while the diff stays open on the same thread, and select Changes only when the diff newly opens. --- .../web/src/components/ChatView.logic.test.ts | 87 +++++++++++++++++++ apps/web/src/components/ChatView.logic.ts | 36 ++++++++ apps/web/src/hooks/useProactivePanels.ts | 6 +- 3 files changed, 128 insertions(+), 1 deletion(-) diff --git a/apps/web/src/components/ChatView.logic.test.ts b/apps/web/src/components/ChatView.logic.test.ts index 8223e03540405..e132c43dac562 100644 --- a/apps/web/src/components/ChatView.logic.test.ts +++ b/apps/web/src/components/ChatView.logic.test.ts @@ -1,4 +1,5 @@ import { shouldShowOpenInPicker } from "./chat/OpenInPicker.logic"; +import { scopeThreadRef } from "@t3tools/client-runtime/environment"; import { boundAttachmentIds, buildExpiredTerminalContextToastCopy, @@ -51,6 +52,8 @@ import { resetHeldThreadTimeline, resolveThreadSwitchTimeline, timelineHasEphemeralPreviewUrls, + shouldApplyProactiveChangesScope, + shouldResetDiffSelectionToChanges, createLocalDispatchSnapshot, deriveCommittedServerUserMessageIds, deriveComposerSendState, @@ -408,6 +411,90 @@ describe("deriveCommittedServerUserMessageIds", () => { }); }); +describe("diff selection across agent turns", () => { + const threadRef = scopeThreadRef(EnvironmentId.make("env-1"), ThreadId.make("thread-1")); + const otherThreadRef = scopeThreadRef(EnvironmentId.make("env-1"), ThreadId.make("thread-2")); + const openThread = { threadKey: "env-1:thread-1", diffOpen: true }; + + it("keeps Uncommitted when the same thread refreshes", () => { + expect( + shouldResetDiffSelectionToChanges({ + diffOpen: true, + activeThreadRef: threadRef, + explicitThreadRef: null, + previous: openThread, + threadKey: "env-1:thread-1", + }), + ).toBe(false); + }); + + it("shows Changes when the diff opens or the thread changes", () => { + expect( + shouldResetDiffSelectionToChanges({ + diffOpen: true, + activeThreadRef: threadRef, + explicitThreadRef: null, + previous: { threadKey: "env-1:thread-1", diffOpen: false }, + threadKey: "env-1:thread-1", + }), + ).toBe(true); + expect( + shouldResetDiffSelectionToChanges({ + diffOpen: true, + activeThreadRef: otherThreadRef, + explicitThreadRef: null, + previous: openThread, + threadKey: "env-1:thread-2", + }), + ).toBe(true); + expect( + shouldResetDiffSelectionToChanges({ + diffOpen: true, + activeThreadRef: threadRef, + explicitThreadRef: null, + previous: null, + threadKey: "env-1:thread-1", + }), + ).toBe(true); + }); + + it("leaves a timeline turn selection in place", () => { + expect( + shouldResetDiffSelectionToChanges({ + diffOpen: true, + activeThreadRef: threadRef, + explicitThreadRef: threadRef, + previous: { threadKey: "env-1:thread-1", diffOpen: false }, + threadKey: "env-1:thread-1", + }), + ).toBe(false); + }); + + it("does nothing while the diff is closed", () => { + expect( + shouldResetDiffSelectionToChanges({ + diffOpen: false, + activeThreadRef: threadRef, + explicitThreadRef: null, + previous: openThread, + threadKey: "env-1:thread-1", + }), + ).toBe(false); + }); + + it("shows Changes only when a completed turn newly opens the diff", () => { + expect( + shouldApplyProactiveChangesScope({ openedDiff: true, alreadyReviewingDiff: false }), + ).toBe(true); + expect(shouldApplyProactiveChangesScope({ openedDiff: true, alreadyReviewingDiff: true })).toBe( + false, + ); + expect( + shouldApplyProactiveChangesScope({ openedDiff: false, alreadyReviewingDiff: false }), + ).toBe(false); + }); +}); + describe("resolveThreadSwitchTimeline", () => { afterEach(() => { resetHeldThreadTimeline(); diff --git a/apps/web/src/components/ChatView.logic.ts b/apps/web/src/components/ChatView.logic.ts index b1d44d95441c5..eb25344cbc0fc 100644 --- a/apps/web/src/components/ChatView.logic.ts +++ b/apps/web/src/components/ChatView.logic.ts @@ -199,6 +199,42 @@ export function shouldRetargetThreadPullRequestPanel( ); } +export interface DiffPanelOpenContext { + readonly threadKey: string | null; + readonly diffOpen: boolean; +} + +/** + * Generic openings show Changes: the first time the diff is on screen, a tab + * fallback, or a switch to another thread. A timeline click passes the same + * thread ref it just selected and keeps that turn. An agent turn replaces the + * thread shell without closing the diff, and that refresh must leave Uncommitted + * or the selected turn alone. + */ +export function shouldResetDiffSelectionToChanges(input: { + readonly diffOpen: boolean; + readonly activeThreadRef: ScopedThreadRef | null; + readonly explicitThreadRef: ScopedThreadRef | null; + readonly previous: DiffPanelOpenContext | null; + readonly threadKey: string | null; +}): boolean { + if (!input.diffOpen || input.activeThreadRef === null) return false; + if (input.explicitThreadRef === input.activeThreadRef) return false; + return !( + input.previous !== null && + input.previous.diffOpen && + input.previous.threadKey === input.threadKey + ); +} + +/** Opening the diff for a completed turn shows Changes. A review already on screen stays put. */ +export function shouldApplyProactiveChangesScope(input: { + readonly openedDiff: boolean; + readonly alreadyReviewingDiff: boolean; +}): boolean { + return input.openedDiff && !input.alreadyReviewingDiff; +} + export function shouldOpenProactiveTurnDiff(input: { previousRunningTurnId: RunId | null | undefined; runningTurnId: RunId | null; diff --git a/apps/web/src/hooks/useProactivePanels.ts b/apps/web/src/hooks/useProactivePanels.ts index 60c78a850c25f..1fe137872ba08 100644 --- a/apps/web/src/hooks/useProactivePanels.ts +++ b/apps/web/src/hooks/useProactivePanels.ts @@ -1,3 +1,4 @@ +import { shouldApplyProactiveChangesScope } from "../components/ChatView.logic"; import type { RunId, ScopedThreadRef, ThreadLinkedPullRequest } from "@t3tools/contracts"; import { scopedThreadKey } from "@t3tools/client-runtime/environment"; import { useEffect, useRef } from "react"; @@ -176,6 +177,7 @@ export function useProactivePanels(input: { diffAction === "defer" || shouldDeferLink ? previousRunningTurnId : activeRunningTurnId, }; if (diffAction !== "open" || newlyCompletedTurnId === null) return; + const alreadyReviewingDiff = openSurface?.kind === "diff"; if ( !panels.openProactive( threadRef, @@ -185,7 +187,9 @@ export function useProactivePanels(input: { ) { return; } - useDiffPanelStore.getState().selectGitScope(threadRef, "branch"); + if (shouldApplyProactiveChangesScope({ openedDiff: true, alreadyReviewingDiff })) { + useDiffPanelStore.getState().selectGitScope(threadRef, "branch"); + } onDiffPanelOpen?.(); }, [ activeLatestRun?.runId, From 490f8ac8e65809ccb1a31bfab108e9dd243793d4 Mon Sep 17 00:00:00 2001 From: Jake Leventhal Date: Sun, 4 Oct 2026 11:10:07 -0400 Subject: [PATCH 02/17] fix(web): keep diff scroll across review refreshes A new diff hash remounted the viewer and dropped the pages already on screen, so an agent turn sent the reader back to the top of Uncommitted, Changes, and pull request review. Co-authored-by: Cursor --- apps/web/src/components/DiffPanel.tsx | 5 +- .../diffs/reviewFileDiffRetention.test.ts | 94 +++++++ .../diffs/reviewFileDiffRetention.ts | 88 ++++++ .../components/diffs/useReviewFilePatches.ts | 254 ++++++++++-------- .../pullRequest/PullRequestCodeTab.tsx | 9 +- .../pullRequest/pullRequestDiff.logic.test.ts | 25 ++ .../pullRequest/pullRequestDiff.logic.ts | 21 ++ 7 files changed, 386 insertions(+), 110 deletions(-) create mode 100644 apps/web/src/components/diffs/reviewFileDiffRetention.test.ts create mode 100644 apps/web/src/components/diffs/reviewFileDiffRetention.ts diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index 4231b669aa76b..1c518821ea54d 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -105,6 +105,7 @@ import { buildBaseRefChoices, filterBaseRefChoices } from "../lib/baseRefChoices import { createGitDiffFileContentsLoader } from "../lib/diffFileContents"; import { observeIntersection } from "~/lib/observeIntersection"; +import { reviewDiffViewerKey } from "./diffs/reviewFileDiffRetention"; import { useReviewFilePatches } from "./diffs/useReviewFilePatches"; import { DiffFileLoadingBoundary } from "./diffs/DiffFileLoadingBoundary"; import { DiffFileStatus } from "./diffs/DiffFileStatus"; @@ -598,6 +599,8 @@ export default function DiffPanel({ ...matchingBaseRefChoices.map(valueForBaseRefChoice), ]; const { + scope: filePatchScope, + family: filePatchFamily, isPending: areFilePatchesPending, fileStates, retry, @@ -1450,7 +1453,7 @@ export default function DiffPanel({ projectId={activeThread?.projectId} viewerRef={attachCodeView} onScroll={codeViewScroll.onScroll} - codeViewKey={codeViewMountKey} + codeViewKey={reviewDiffViewerKey(codeViewMountKey, lazySource ? filePatchFamily : null)} className="h-full min-h-0 overflow-auto" files={codeViewFiles} pendingFiles={pendingFiles} diff --git a/apps/web/src/components/diffs/reviewFileDiffRetention.test.ts b/apps/web/src/components/diffs/reviewFileDiffRetention.test.ts new file mode 100644 index 0000000000000..8f44c5e7eb74a --- /dev/null +++ b/apps/web/src/components/diffs/reviewFileDiffRetention.test.ts @@ -0,0 +1,94 @@ +import type { FileDiffMetadata } from "@pierre/diffs"; +import { describe, expect, it } from "vite-plus/test"; + +import { + isPendingReviewFile, + retainLoadedReviewFiles, + retainedReviewFile, + reviewDiffViewerKey, + reviewPatchIndicesForFamily, + reviewSnapshotFamily, + reviewSnapshotScope, +} from "./reviewFileDiffRetention"; + +const identity = { + environmentId: "env-1", + cwd: "/repo", + kind: "working-tree", + baseRef: "HEAD", + ignoreWhitespace: false, +}; + +function file(name: string, cacheKey?: string): FileDiffMetadata { + return { name, ...(cacheKey ? { cacheKey } : {}) } as FileDiffMetadata; +} + +describe("review snapshot identity", () => { + it("keeps the same review across git snapshots", () => { + const family = reviewSnapshotFamily(identity); + expect(reviewSnapshotFamily(identity)).toBe(family); + expect(reviewSnapshotScope(family, "hash-a")).not.toBe(reviewSnapshotScope(family, "hash-b")); + expect(reviewDiffViewerKey("section", family)).toBe(`section:${family}`); + expect(reviewDiffViewerKey("section", null)).toBe("section:preview"); + }); + + it("keeps loaded file indexes when the snapshot family is unchanged", () => { + const family = reviewSnapshotFamily(identity); + expect(reviewPatchIndicesForFamily({ family, indices: [0, 1, 2, 3, 8] }, family)).toEqual([ + 0, 1, 2, 3, 8, + ]); + expect( + reviewPatchIndicesForFamily({ family, indices: [0, 1, 2, 3, 8] }, `${family}:other`), + ).toEqual([0, 1, 2, 3]); + }); +}); + +describe("retained review files", () => { + const loaded = file("src/app.ts", "hash:src/app.ts"); + const placeholder = file("src/app.ts", "next:src/app.ts:pending"); + + it("keeps the open file until the next patch arrives", () => { + expect( + retainedReviewFile({ + loaded: null, + previous: loaded, + placeholder, + sameFamily: true, + }), + ).toBe(loaded); + }); + + it("uses the newly loaded patch", () => { + const next = file("src/app.ts", "hash-b:src/app.ts"); + expect( + retainedReviewFile({ + loaded: next, + previous: loaded, + placeholder, + sameFamily: true, + }), + ).toBe(next); + }); + + it("does not carry a file into a different review", () => { + expect( + retainedReviewFile({ + loaded: null, + previous: loaded, + placeholder, + sameFamily: false, + }), + ).toBe(placeholder); + expect(isPendingReviewFile(placeholder)).toBe(true); + }); + + it("stores loaded files and ignores placeholders", () => { + const previous = new Map([["src/app.ts", loaded]]); + expect(retainLoadedReviewFiles(previous, [loaded, placeholder], (entry) => entry.name)).toBe( + previous, + ); + const next = file("src/app.ts", "hash-b:src/app.ts"); + const retained = retainLoadedReviewFiles(previous, [next], (entry) => entry.name); + expect(retained.get("src/app.ts")).toBe(next); + }); +}); diff --git a/apps/web/src/components/diffs/reviewFileDiffRetention.ts b/apps/web/src/components/diffs/reviewFileDiffRetention.ts new file mode 100644 index 0000000000000..81eb4be4e331f --- /dev/null +++ b/apps/web/src/components/diffs/reviewFileDiffRetention.ts @@ -0,0 +1,88 @@ +import type { FileDiffMetadata } from "@pierre/diffs"; + +const INITIAL_PATCH_INDICES = [0, 1, 2, 3] as const; +const EMPTY_RETAINED_FILES: ReadonlyMap = new Map(); + +export interface ReviewSnapshotIdentity { + readonly environmentId: string | undefined; + readonly cwd: string | undefined; + readonly kind: string | undefined; + readonly baseRef: string | null; + readonly ignoreWhitespace: boolean; +} + +/** Identity of a review that survives a new git snapshot. The diff hash is not part of it. */ +export function reviewSnapshotFamily(input: ReviewSnapshotIdentity): string { + return JSON.stringify([ + input.environmentId, + input.cwd, + input.kind, + input.baseRef, + input.ignoreWhitespace, + ]); +} + +export function reviewSnapshotScope(family: string, diffHash: string | undefined): string { + return JSON.stringify([family, diffHash ?? null]); +} + +/** + * Keep requesting the files already on screen when the snapshot changes. A different + * review starts over at the first page. + */ +export function reviewPatchIndicesForFamily( + requested: { readonly family: string; readonly indices: readonly number[] }, + family: string, +): readonly number[] { + return requested.family === family ? requested.indices : INITIAL_PATCH_INDICES; +} + +export function isPendingReviewFile(file: { readonly cacheKey?: string | undefined }): boolean { + return file.cacheKey?.endsWith(":pending") === true; +} + +/** Show the file already on screen until its replacement patch arrives. */ +export function retainedReviewFile(input: { + readonly loaded: T | null; + readonly previous: T | undefined; + readonly placeholder: T; + readonly sameFamily: boolean; +}): T { + if (input.loaded) return input.loaded; + if (input.sameFamily && input.previous && !isPendingReviewFile(input.previous)) { + return input.previous; + } + return input.placeholder; +} + +export function retainLoadedReviewFiles( + previous: ReadonlyMap, + files: readonly FileDiffMetadata[], + pathOf: (file: FileDiffMetadata) => string, +): ReadonlyMap { + const next = new Map(); + for (const file of files) { + if (!isPendingReviewFile(file)) next.set(pathOf(file), file); + } + if (next.size === previous.size) { + let unchanged = true; + for (const [path, file] of next) { + if (previous.get(path) !== file) { + unchanged = false; + break; + } + } + if (unchanged) return previous; + } + return next.size === 0 ? EMPTY_RETAINED_FILES : next; +} + +/** The viewer remounts for a new review section, not for a new git snapshot of the same one. */ +export function reviewDiffViewerKey( + sectionMountKey: string, + snapshotFamily: string | null, +): string { + return snapshotFamily === null + ? `${sectionMountKey}:preview` + : `${sectionMountKey}:${snapshotFamily}`; +} diff --git a/apps/web/src/components/diffs/useReviewFilePatches.ts b/apps/web/src/components/diffs/useReviewFilePatches.ts index 728f43433b010..3e7f365c72b3f 100644 --- a/apps/web/src/components/diffs/useReviewFilePatches.ts +++ b/apps/web/src/components/diffs/useReviewFilePatches.ts @@ -5,19 +5,28 @@ import type { ReviewDiffPreviewInput, ReviewDiffPreviewSource, } from "@t3tools/contracts"; -import * as Option from "effect/Option"; import * as AsyncResult from "effect/reactivity/AsyncResult"; import * as Atom from "effect/reactivity/Atom"; -import { useContext, useState } from "react"; +import { useCallback, useContext, useEffect, useMemo, useRef, useState } from "react"; import { useStableCallback } from "~/hooks/useStableCallback"; import { getRenderablePatch, resolveFileDiffPath, type RenderablePatch } from "~/lib/diffRendering"; import { reviewEnvironment } from "~/state/review"; +import { + isPendingReviewFile, + retainLoadedReviewFiles, + retainedReviewFile, + reviewPatchIndicesForFamily, + reviewSnapshotFamily, + reviewSnapshotScope, +} from "./reviewFileDiffRetention"; + +const EMPTY_RETAINED_FILES: ReadonlyMap = new Map(); const filePathCollator = new Intl.Collator(undefined, { numeric: true, sensitivity: "base", }); -const INITIAL_FILE_COUNT = 4; +const DEFAULT_INDICES = [0, 1, 2, 3]; export function useReviewFilePatches({ environmentId, @@ -40,46 +49,96 @@ export function useReviewFilePatches({ preview: RenderablePatch | null; }) { const registry = useContext(RegistryContext); - const scope = JSON.stringify([ - environmentId, - cwd, - source?.kind, - source?.baseRef, - source?.headRef, - baseRef, - ignoreWhitespace, - ]); - const files = sortedReviewFiles(source); - const automaticPaths = files.slice(0, INITIAL_FILE_COUNT).map((file) => file.path); - const [requested, setRequested] = useState(() => ({ scope, paths: automaticPaths })); - const previousPaths = requested.scope === scope ? requested.paths : []; - const paths = [...new Set([...previousPaths, ...automaticPaths])]; - if (requested.scope !== scope || paths.length !== previousPaths.length) { - setRequested({ scope, paths }); - } - const queries = reviewFileQueries({ + const snapshotFamily = reviewSnapshotFamily({ environmentId, cwd, - source, + kind: source?.kind, baseRef, ignoreWhitespace, - previewInput, - scope, - requested, }); + const scope = reviewSnapshotScope(snapshotFamily, source?.diffHash); + const [requested, setRequested] = useState({ family: snapshotFamily, indices: [0, 1, 2, 3] }); + const [retainedFiles, setRetainedFiles] = useState<{ + readonly family: string; + readonly byPath: ReadonlyMap; + }>({ family: snapshotFamily, byPath: EMPTY_RETAINED_FILES }); + const previousByPath = retainedFiles.family === snapshotFamily ? retainedFiles.byPath : EMPTY_RETAINED_FILES; + const files = sortedReviewFiles(source); + const queries = reviewFileQueries({ environmentId, cwd, source, baseRef, ignoreWhitespace, previewInput, scope, + requested: { scope, indices: [...reviewPatchIndicesForFamily(requested, snapshotFamily)] } }); const queriedPatchesAtom = reviewPatchesAtom( JSON.stringify([theme, queries.map(({ index, target }) => [index, target])]), ); const patches = useAtomValue(queriedPatchesAtom); const retry = useStableCallback((path: string) => { - const files = sortedReviewFiles(source); - const query = queries.find(({ index }) => files[index]?.path === path)?.query; - if (query) registry.refresh(query); - }); - const pendingIndex = files.findIndex((_, index) => { - const patch = patches.get(index); - return !patch || patch._tag === "Initial"; - }); + const requestFiles = useCallback( + (nextIndices: number[]) => + setRequested((current) => { + const previous = reviewPatchIndicesForFamily(current, snapshotFamily); + const added = nextIndices.filter((index) => !previous.includes(index)); + if (added.length === 0 && current.family === snapshotFamily) return current; + return { family: snapshotFamily, indices: [...previous, ...added] }; + }), + [snapshotFamily], + ); + const requestFile = useCallback((index: number) => requestFiles([index]), [requestFiles]); + const renderableFiles = useMemo( + () => + source + ? files.map((file, index): FileDiffMetadata => { + const result = patches.get(index); + const loaded = + result?._tag === "Success" && result.value.patch?.kind === "files" + ? (result.value.patch.files.find( + (candidate) => resolveFileDiffPath(candidate) === file.path, + ) ?? null) + : null; + return retainedReviewFile({ + loaded, + previous: previousByPath.get(file.path), + sameFamily: true, + placeholder: { + name: file.path, + ...(file.previousPath ? { prevName: file.previousPath } : {}), + type: file.previousPath ? "rename-changed" : "change", + hunks: [], + additionLines: [], + deletionLines: [], + additionCharacterCount: 0, + deletionCharacterCount: 0, + splitLineCount: 0, + unifiedLineCount: 0, + isPartial: true, + cacheKey: `${scope}:${file.path}:pending`, + }, + }); + }) + : (preview?.kind === "files" ? preview.files : []).toSorted((a, b) => + filePathCollator.compare(resolveFileDiffPath(a), resolveFileDiffPath(b)), + ), + [source, files, patches, scope, preview, previousByPath], + ); + const nextRetainedFiles = source + ? retainLoadedReviewFiles(previousByPath, renderableFiles, resolveFileDiffPath) + : EMPTY_RETAINED_FILES; + useEffect(() => { + if (!source) return; + if (retainedFiles.family === snapshotFamily && retainedFiles.byPath === nextRetainedFiles) + return; + setRetainedFiles({ family: snapshotFamily, byPath: nextRetainedFiles }); + }, [nextRetainedFiles, retainedFiles.byPath, retainedFiles.family, snapshotFamily, source]); + const pendingIndex = source + ? files.findIndex((file, index) => { + const patch = patches.get(index); + if (patch && patch._tag !== "Initial") return false; + const rendered = renderableFiles[index]; + return ( + rendered === undefined || + isPendingReviewFile(rendered) || + resolveFileDiffPath(rendered) !== file.path + ); + }) + : -1; const settledFileCount = source ? pendingIndex < 0 ? files.length @@ -87,77 +146,63 @@ export function useReviewFilePatches({ : preview?.kind === "files" ? preview.files.length : 0; - const requestFiles = (indices: number[]) => - setRequested((current) => { - const previous = current.scope === scope ? current.paths : []; - const added = indices.flatMap((index) => { - const path = files[index]?.path; - return path !== undefined && !previous.includes(path) ? [path] : []; - }); - return added.length === 0 ? current : { scope, paths: [...previous, ...added] }; - }); - // The first unsettled file already requested means a batch is in flight; wait for it. - const loadNextFiles = () => { - if (queries.some(({ index }) => index === settledFileCount)) return; - requestFiles(Array.from({ length: 4 }, (_, index) => settledFileCount + index)); - }; - const requestFile = (path: string) => - requestFiles([files.findIndex((file) => file.path === path)]); - const renderableFiles = source - ? files.flatMap((file, index): FileDiffMetadata[] => { - const result = patches.get(index); - const value = result && Option.getOrUndefined(AsyncResult.value(result)); - if (value?.patch?.kind === "files") { - const loaded = value.patch.files.filter( - (candidate) => resolveFileDiffPath(candidate) === file.path, - ); - if (loaded.length > 0) return loaded; - } - return [ - { - name: file.path, - ...(file.previousPath ? { prevName: file.previousPath } : {}), - type: file.previousPath ? "rename-changed" : "change", - hunks: [], - additionLines: [], - deletionLines: [], - additionCharacterCount: 0, - deletionCharacterCount: 0, - splitLineCount: 0, - unifiedLineCount: 0, - isPartial: true, - cacheKey: `${scope}:${file.path}:pending`, - }, - ]; - }) - : (preview?.kind === "files" ? preview.files : []).toSorted((a, b) => - filePathCollator.compare(resolveFileDiffPath(a), resolveFileDiffPath(b)), - ); - const fileStates = new Map( - files.map((file, index) => { - const patch = patches.get(index); - return [ - file.path, - { - error: - patch?._tag === "Failure" || - (patch?._tag === "Success" && - (patch.value.patch?.kind !== "files" || - !patch.value.patch.files.some( - (candidate) => resolveFileDiffPath(candidate) === file.path, - ))), - truncated: patch?._tag === "Success" && patch.value.source.truncated, - }, - ] as const; - }), + const loadNextFiles = useCallback( + () => { + if (queries.some(({ index }) => index === settledFileCount)) return; + requestFiles(Array.from({ length: 4 }, (_, index) => settledFileCount + index)); + }, + [queries, requestFiles, settledFileCount], + ); + const fileStates = useMemo( + () => + new Map( + files.map((file, index) => { + const patch = patches.get(index); + return [ + file.path, + { + error: + patch?._tag === "Failure" || + (patch?._tag === "Success" && + (patch.value.patch?.kind !== "files" || + !patch.value.patch.files.some( + (candidate) => resolveFileDiffPath(candidate) === file.path, + ))), + truncated: patch?._tag === "Success" && patch.value.source.truncated, + }, + ] as const; + }), + ), + [files, patches], + ); + const readyFilePaths = useMemo( + () => + new Set( + files + .filter((file, index) => { + const patch = patches.get(index); + if (patch && patch._tag !== "Initial") return true; + const rendered = renderableFiles[index]; + return ( + rendered !== undefined && + !isPendingReviewFile(rendered) && + resolveFileDiffPath(rendered) === file.path + ); + }) + .map((file) => file.path), + ), + [files, patches, renderableFiles], ); return { + scope, + family: snapshotFamily, fileStates, isPending: [...patches.values()].some((patch) => patch._tag === "Initial" || patch.waiting), retry, requestFile, + readyFilePaths, renderableFiles, - remainingFileCount: source ? files.length - settledFileCount : 0, + settledFileCount, loadNextFiles, }; } @@ -179,19 +224,17 @@ function reviewFileQueries({ ignoreWhitespace: boolean; previewInput: ReviewDiffPreviewInput | undefined; scope: string; - requested: { scope: string; paths: string[] }; + requested: { scope: string; indices: number[] }; }) { + const indices = requested.scope === scope ? requested.indices : DEFAULT_INDICES; const files = sortedReviewFiles(source); - const paths = new Set([ - ...files.slice(0, INITIAL_FILE_COUNT).map((file) => file.path), - ...(requested.scope === scope ? requested.paths : []), - ]); const queries = !environmentId || !cwd || !source ? [] - : files - .flatMap((file, index) => (paths.has(file.path) ? [{ file, index }] : [])) - .map(({ file, index }) => { + : indices + .filter((index) => index < files.length) + .map((index) => { + const file = files[index]!; const target: ReviewPatchTarget = { environmentId, input: { @@ -200,7 +243,6 @@ function reviewFileQueries({ request: { cwd, ...(baseRef ? { baseRef } : {}), - ...(source.kind === "commit" ? { commitSha: source.id } : {}), ignoreWhitespace, file: { path: file.path, diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index febf82441f514..6c6411c352e23 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -126,6 +126,7 @@ import { isFileDiffCollapsed, isLineInFileDiff, withFileDiffFold, + retainReviewDiffSlices, type DiffFoldOverride, type FileDiffFoldChoices, } from "./pullRequestDiff.logic"; @@ -539,14 +540,16 @@ function PullRequestCodeTab({ isViewed: isFileViewed, isStale: isFileViewedStale, } = filesViewed; + // Reread the first page and the viewed ticks. The pages already on screen stay until that + // page arrives: blanking them remounts the viewer and sends the reader back to the top, + // including on every agent turn. const appliedRefreshToken = useRef(refreshToken); // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- a new refresh token clears loaded slices and refetches the first diff page and viewed files useEffect(() => { if (appliedRefreshToken.current === refreshToken) return; appliedRefreshToken.current = refreshToken; - setSliceState({ key: scopeKey, cursor: null, slices: NO_SLICES }); - // An equal refetch keeps the answer's identity, so the emptied slices refill only if the - // applied page is forgotten too. + setSliceState((previous) => retainReviewDiffSlices(previous, scopeKey)); + // An equal refetch keeps the answer's identity, so forget the applied page to reapply it. setAppliedPage(null); refreshFirstDiffPage(); refreshFilesViewed(); diff --git a/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts index 47f309b3e32c0..155c6c7a76581 100644 --- a/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts @@ -6,6 +6,7 @@ import { isFileDiffCollapsed, isLineInFileDiff, withFileDiffFold, + retainReviewDiffSlices, } from "./pullRequestDiff.logic"; function fileWithHunks( @@ -19,6 +20,30 @@ function fileWithHunks( return { name: "src/app.ts", hunks } as unknown as FileDiffMetadata; } +describe("retainReviewDiffSlices", () => { + const slices = [{ cursor: null, patch: "page-1" }]; + + it("keeps the open pages when the same review refreshes", () => { + const previous = { key: "pr-1", cursor: "page-2", slices }; + expect(retainReviewDiffSlices(previous, "pr-1")).toEqual({ + key: "pr-1", + cursor: null, + slices, + }); + expect(retainReviewDiffSlices({ key: "pr-1", cursor: null, slices }, "pr-1").slices).toBe( + slices, + ); + }); + + it("starts over when the review itself changes", () => { + expect(retainReviewDiffSlices({ key: "pr-1", cursor: null, slices }, "pr-2")).toEqual({ + key: "pr-2", + cursor: null, + slices: [], + }); + }); +}); + describe("isLineInFileDiff", () => { const file = fileWithHunks([ { deletionStart: 10, deletionCount: 3, additionStart: 10, additionCount: 5 }, diff --git a/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts b/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts index da3844e90e1bb..8ed81fab6d956 100644 --- a/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts @@ -1,6 +1,27 @@ import type { FileDiffMetadata } from "@pierre/diffs"; import type { PullRequestDiffSide } from "@t3tools/contracts"; +export interface ReviewDiffSliceState { + readonly key: string; + readonly cursor: string | null; + readonly slices: ReadonlyArray; +} + +/** + * A refresh rereads the first page, and the pages already on screen stay until that page + * arrives. Clearing them remounts the viewer and sends the reader back to the top. + */ +export function retainReviewDiffSlices( + previous: ReviewDiffSliceState, + scopeKey: string, +): ReviewDiffSliceState { + if (previous.key !== scopeKey) { + return { key: scopeKey, cursor: null, slices: [] }; + } + if (previous.cursor === null) return previous; + return { key: previous.key, cursor: null, slices: previous.slices }; +} + /** Only rendered hunks can anchor comments; list others off-diff. */ export function isLineInFileDiff( file: FileDiffMetadata, From 3b4ffda04307518d67326b59727829e29e1606e8 Mon Sep 17 00:00:00 2001 From: Jake Leventhal Date: Sun, 4 Oct 2026 11:57:41 -0400 Subject: [PATCH 03/17] fix(web): reread later diff pages and drop failed patches A refresh that left the first page unchanged never fetched the pages after it, and a failed file patch stayed on screen as the current diff. Co-authored-by: Cursor --- .../web/src/components/ChatView.logic.test.ts | 9 +++++++++ apps/web/src/components/ChatView.logic.ts | 13 +++++++++--- .../diffs/reviewFileDiffRetention.test.ts | 15 ++++++++++++++ .../diffs/reviewFileDiffRetention.ts | 8 ++++++-- .../components/diffs/useReviewFilePatches.ts | 1 + .../pullRequest/PullRequestCodeTab.tsx | 20 ++++++++++++++++--- .../pullRequest/pullRequestDiff.logic.test.ts | 15 +++++++------- .../pullRequest/pullRequestDiff.logic.ts | 13 ++++++++---- 8 files changed, 74 insertions(+), 20 deletions(-) diff --git a/apps/web/src/components/ChatView.logic.test.ts b/apps/web/src/components/ChatView.logic.test.ts index e132c43dac562..313777571dac9 100644 --- a/apps/web/src/components/ChatView.logic.test.ts +++ b/apps/web/src/components/ChatView.logic.test.ts @@ -468,6 +468,15 @@ describe("diff selection across agent turns", () => { threadKey: "env-1:thread-1", }), ).toBe(false); + expect( + shouldResetDiffSelectionToChanges({ + diffOpen: true, + activeThreadRef: scopeThreadRef(EnvironmentId.make("env-1"), ThreadId.make("thread-1")), + explicitThreadRef: scopeThreadRef(EnvironmentId.make("env-1"), ThreadId.make("thread-1")), + previous: { threadKey: "env-1:thread-1", diffOpen: false }, + threadKey: "env-1:thread-1", + }), + ).toBe(false); }); it("does nothing while the diff is closed", () => { diff --git a/apps/web/src/components/ChatView.logic.ts b/apps/web/src/components/ChatView.logic.ts index eb25344cbc0fc..589d854726e03 100644 --- a/apps/web/src/components/ChatView.logic.ts +++ b/apps/web/src/components/ChatView.logic.ts @@ -206,8 +206,9 @@ export interface DiffPanelOpenContext { /** * Generic openings show Changes: the first time the diff is on screen, a tab - * fallback, or a switch to another thread. A timeline click passes the same - * thread ref it just selected and keeps that turn. An agent turn replaces the + * fallback, or a switch to another thread. A timeline click names the thread it + * just selected and keeps that turn, matched by id because the shell hands out + * a new ref object whenever the thread updates. An agent turn replaces the * thread shell without closing the diff, and that refresh must leave Uncommitted * or the selected turn alone. */ @@ -219,7 +220,13 @@ export function shouldResetDiffSelectionToChanges(input: { readonly threadKey: string | null; }): boolean { if (!input.diffOpen || input.activeThreadRef === null) return false; - if (input.explicitThreadRef === input.activeThreadRef) return false; + if ( + input.explicitThreadRef !== null && + input.explicitThreadRef.environmentId === input.activeThreadRef.environmentId && + input.explicitThreadRef.threadId === input.activeThreadRef.threadId + ) { + return false; + } return !( input.previous !== null && input.previous.diffOpen && diff --git a/apps/web/src/components/diffs/reviewFileDiffRetention.test.ts b/apps/web/src/components/diffs/reviewFileDiffRetention.test.ts index 8f44c5e7eb74a..059705dc26f77 100644 --- a/apps/web/src/components/diffs/reviewFileDiffRetention.test.ts +++ b/apps/web/src/components/diffs/reviewFileDiffRetention.test.ts @@ -54,10 +54,23 @@ describe("retained review files", () => { previous: loaded, placeholder, sameFamily: true, + pending: true, }), ).toBe(loaded); }); + it("drops the open file once its replacement fails or no longer includes it", () => { + expect( + retainedReviewFile({ + loaded: null, + previous: loaded, + placeholder, + sameFamily: true, + pending: false, + }), + ).toBe(placeholder); + }); + it("uses the newly loaded patch", () => { const next = file("src/app.ts", "hash-b:src/app.ts"); expect( @@ -66,6 +79,7 @@ describe("retained review files", () => { previous: loaded, placeholder, sameFamily: true, + pending: false, }), ).toBe(next); }); @@ -77,6 +91,7 @@ describe("retained review files", () => { previous: loaded, placeholder, sameFamily: false, + pending: true, }), ).toBe(placeholder); expect(isPendingReviewFile(placeholder)).toBe(true); diff --git a/apps/web/src/components/diffs/reviewFileDiffRetention.ts b/apps/web/src/components/diffs/reviewFileDiffRetention.ts index 81eb4be4e331f..28c9c1dd10de2 100644 --- a/apps/web/src/components/diffs/reviewFileDiffRetention.ts +++ b/apps/web/src/components/diffs/reviewFileDiffRetention.ts @@ -41,15 +41,19 @@ export function isPendingReviewFile(file: { readonly cacheKey?: string | undefin return file.cacheKey?.endsWith(":pending") === true; } -/** Show the file already on screen until its replacement patch arrives. */ +/** + * Show the file already on screen while its replacement is still loading. + * A failure, or a snapshot that no longer contains the file, must not keep the old diff. + */ export function retainedReviewFile(input: { readonly loaded: T | null; readonly previous: T | undefined; readonly placeholder: T; readonly sameFamily: boolean; + readonly pending: boolean; }): T { if (input.loaded) return input.loaded; - if (input.sameFamily && input.previous && !isPendingReviewFile(input.previous)) { + if (input.pending && input.sameFamily && input.previous && !isPendingReviewFile(input.previous)) { return input.previous; } return input.placeholder; diff --git a/apps/web/src/components/diffs/useReviewFilePatches.ts b/apps/web/src/components/diffs/useReviewFilePatches.ts index 3e7f365c72b3f..286881e8ec421 100644 --- a/apps/web/src/components/diffs/useReviewFilePatches.ts +++ b/apps/web/src/components/diffs/useReviewFilePatches.ts @@ -97,6 +97,7 @@ export function useReviewFilePatches({ loaded, previous: previousByPath.get(file.path), sameFamily: true, + pending: result === undefined || result._tag === "Initial" || result.waiting, placeholder: { name: file.path, ...(file.previousPath ? { prevName: file.previousPath } : {}), diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 6c6411c352e23..9c8d470d7a7ad 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -540,14 +540,18 @@ function PullRequestCodeTab({ isViewed: isFileViewed, isStale: isFileViewedStale, } = filesViewed; - // Reread the first page and the viewed ticks. The pages already on screen stay until that - // page arrives: blanking them remounts the viewer and sends the reader back to the top, - // including on every agent turn. + // Reread from the first page. It stays on screen so the reader does not jump; later pages + // belong to the snapshot being replaced, so they are fetched again. Each of those pages is + // its own cached query, so the walk asks for it again instead of replaying the old snapshot. + const refreshGeneration = useRef(0); + const refreshedCursor = useRef(undefined); const appliedRefreshToken = useRef(refreshToken); // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- a new refresh token clears loaded slices and refetches the first diff page and viewed files useEffect(() => { if (appliedRefreshToken.current === refreshToken) return; appliedRefreshToken.current = refreshToken; + refreshGeneration.current += 1; + refreshedCursor.current = undefined; setSliceState((previous) => retainReviewDiffSlices(previous, scopeKey)); // An equal refetch keeps the answer's identity, so forget the applied page to reapply it. setAppliedPage(null); @@ -559,11 +563,21 @@ function PullRequestCodeTab({ useEffect(() => { if (appliedBackgroundRefreshToken.current === backgroundRefreshToken) return; appliedBackgroundRefreshToken.current = backgroundRefreshToken; + refreshGeneration.current += 1; + refreshedCursor.current = undefined; // Keeping slices mounted preserves scroll; the page merge invalidates changed pages. setSliceState((previous) => ({ ...previous, cursor: null })); refreshFirstDiffPage(); refreshFilesViewed(); }, [backgroundRefreshToken, refreshFirstDiffPage, refreshFilesViewed]); + useEffect(() => { + if (refreshGeneration.current === 0 || cursor === null) return; + if (refreshedCursor.current === cursor) return; + refreshedCursor.current = cursor; + diffQuery.refresh(); + }, [cursor, diffQuery.refresh]); + const nextCursor = loadedSlices.at(-1)?.nextCursor ?? null; + const codeViewReveal = useCodeViewFileReveal(); const answeredRevealNonce = useRef(null); diff --git a/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts index 155c6c7a76581..4a08e97ff6c1d 100644 --- a/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts @@ -21,18 +21,17 @@ function fileWithHunks( } describe("retainReviewDiffSlices", () => { - const slices = [{ cursor: null, patch: "page-1" }]; + const first = { cursor: null, patch: "page-1" }; + const slices = [first, { cursor: "page-2", patch: "page-2" }]; - it("keeps the open pages when the same review refreshes", () => { - const previous = { key: "pr-1", cursor: "page-2", slices }; - expect(retainReviewDiffSlices(previous, "pr-1")).toEqual({ + it("keeps the first page and drops later ones when the same review refreshes", () => { + expect(retainReviewDiffSlices({ key: "pr-1", cursor: "page-2", slices }, "pr-1")).toEqual({ key: "pr-1", cursor: null, - slices, + slices: [first], }); - expect(retainReviewDiffSlices({ key: "pr-1", cursor: null, slices }, "pr-1").slices).toBe( - slices, - ); + const open = { key: "pr-1", cursor: null, slices: [first] }; + expect(retainReviewDiffSlices(open, "pr-1")).toBe(open); }); it("starts over when the review itself changes", () => { diff --git a/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts b/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts index 8ed81fab6d956..c9774dc316ba4 100644 --- a/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts @@ -8,8 +8,9 @@ export interface ReviewDiffSliceState { } /** - * A refresh rereads the first page, and the pages already on screen stay until that page - * arrives. Clearing them remounts the viewer and sends the reader back to the top. + * A refresh rereads from the first page. That page stays on screen so the reader does not + * jump. Later pages are positions in the snapshot being replaced: keeping them when the + * first page comes back unchanged would leave those files stale for good. */ export function retainReviewDiffSlices( previous: ReviewDiffSliceState, @@ -18,8 +19,12 @@ export function retainReviewDiffSlices( if (previous.key !== scopeKey) { return { key: scopeKey, cursor: null, slices: [] }; } - if (previous.cursor === null) return previous; - return { key: previous.key, cursor: null, slices: previous.slices }; + const first = previous.slices[0]; + if (first === undefined) { + return previous.cursor === null ? previous : { key: scopeKey, cursor: null, slices: [] }; + } + if (previous.slices.length === 1 && previous.cursor === null) return previous; + return { key: scopeKey, cursor: null, slices: [first] }; } /** Only rendered hunks can anchor comments; list others off-diff. */ From fc023b2afb01608a0a2456979c2c95efeb43e991 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Mon, 19 Oct 2026 20:59:55 +0000 Subject: [PATCH 04/17] fix(web): preserve review context through refreshed diffs --- .../web/src/components/ChatView.logic.test.ts | 41 +-- apps/web/src/components/ChatView.logic.ts | 9 - apps/web/src/components/DiffPanel.tsx | 12 +- .../diffs/reviewFileDiffRetention.test.ts | 109 ------- .../diffs/reviewFileDiffRetention.ts | 92 ------ .../components/diffs/useReviewFilePatches.ts | 305 +++++++++--------- .../pullRequest/PullRequestCodeTab.tsx | 170 ++++++---- .../pullRequest/pullRequestDiff.logic.test.ts | 4 +- .../pullRequest/pullRequestDiff.logic.ts | 16 +- apps/web/src/hooks/useProactivePanels.ts | 2 +- apps/web/src/lib/diffRendering.test.ts | 73 +++++ apps/web/src/lib/diffRendering.ts | 34 ++ 12 files changed, 398 insertions(+), 469 deletions(-) delete mode 100644 apps/web/src/components/diffs/reviewFileDiffRetention.test.ts delete mode 100644 apps/web/src/components/diffs/reviewFileDiffRetention.ts diff --git a/apps/web/src/components/ChatView.logic.test.ts b/apps/web/src/components/ChatView.logic.test.ts index 313777571dac9..7f1ab534f9780 100644 --- a/apps/web/src/components/ChatView.logic.test.ts +++ b/apps/web/src/components/ChatView.logic.test.ts @@ -416,7 +416,7 @@ describe("diff selection across agent turns", () => { const otherThreadRef = scopeThreadRef(EnvironmentId.make("env-1"), ThreadId.make("thread-2")); const openThread = { threadKey: "env-1:thread-1", diffOpen: true }; - it("keeps Uncommitted when the same thread refreshes", () => { + it("keeps the active review when an agent updates or completes a turn", () => { expect( shouldResetDiffSelectionToChanges({ diffOpen: true, @@ -426,6 +426,9 @@ describe("diff selection across agent turns", () => { threadKey: "env-1:thread-1", }), ).toBe(false); + expect(shouldApplyProactiveChangesScope({ openedDiff: true, alreadyReviewingDiff: true })).toBe( + false, + ); }); it("shows Changes when the diff opens or the thread changes", () => { @@ -456,18 +459,12 @@ describe("diff selection across agent turns", () => { threadKey: "env-1:thread-1", }), ).toBe(true); + expect( + shouldApplyProactiveChangesScope({ openedDiff: true, alreadyReviewingDiff: false }), + ).toBe(true); }); it("leaves a timeline turn selection in place", () => { - expect( - shouldResetDiffSelectionToChanges({ - diffOpen: true, - activeThreadRef: threadRef, - explicitThreadRef: threadRef, - previous: { threadKey: "env-1:thread-1", diffOpen: false }, - threadKey: "env-1:thread-1", - }), - ).toBe(false); expect( shouldResetDiffSelectionToChanges({ diffOpen: true, @@ -478,30 +475,6 @@ describe("diff selection across agent turns", () => { }), ).toBe(false); }); - - it("does nothing while the diff is closed", () => { - expect( - shouldResetDiffSelectionToChanges({ - diffOpen: false, - activeThreadRef: threadRef, - explicitThreadRef: null, - previous: openThread, - threadKey: "env-1:thread-1", - }), - ).toBe(false); - }); - - it("shows Changes only when a completed turn newly opens the diff", () => { - expect( - shouldApplyProactiveChangesScope({ openedDiff: true, alreadyReviewingDiff: false }), - ).toBe(true); - expect(shouldApplyProactiveChangesScope({ openedDiff: true, alreadyReviewingDiff: true })).toBe( - false, - ); - expect( - shouldApplyProactiveChangesScope({ openedDiff: false, alreadyReviewingDiff: false }), - ).toBe(false); - }); }); describe("resolveThreadSwitchTimeline", () => { diff --git a/apps/web/src/components/ChatView.logic.ts b/apps/web/src/components/ChatView.logic.ts index 589d854726e03..6775878538623 100644 --- a/apps/web/src/components/ChatView.logic.ts +++ b/apps/web/src/components/ChatView.logic.ts @@ -204,14 +204,6 @@ export interface DiffPanelOpenContext { readonly diffOpen: boolean; } -/** - * Generic openings show Changes: the first time the diff is on screen, a tab - * fallback, or a switch to another thread. A timeline click names the thread it - * just selected and keeps that turn, matched by id because the shell hands out - * a new ref object whenever the thread updates. An agent turn replaces the - * thread shell without closing the diff, and that refresh must leave Uncommitted - * or the selected turn alone. - */ export function shouldResetDiffSelectionToChanges(input: { readonly diffOpen: boolean; readonly activeThreadRef: ScopedThreadRef | null; @@ -234,7 +226,6 @@ export function shouldResetDiffSelectionToChanges(input: { ); } -/** Opening the diff for a completed turn shows Changes. A review already on screen stays put. */ export function shouldApplyProactiveChangesScope(input: { readonly openedDiff: boolean; readonly alreadyReviewingDiff: boolean; diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index 1c518821ea54d..5473734da0e00 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -105,7 +105,6 @@ import { buildBaseRefChoices, filterBaseRefChoices } from "../lib/baseRefChoices import { createGitDiffFileContentsLoader } from "../lib/diffFileContents"; import { observeIntersection } from "~/lib/observeIntersection"; -import { reviewDiffViewerKey } from "./diffs/reviewFileDiffRetention"; import { useReviewFilePatches } from "./diffs/useReviewFilePatches"; import { DiffFileLoadingBoundary } from "./diffs/DiffFileLoadingBoundary"; import { DiffFileStatus } from "./diffs/DiffFileStatus"; @@ -419,7 +418,8 @@ export default function DiffPanel({ ? `${routeThreadRef.environmentId}:${routeThreadRef.threadId}` : null; - const selectedGitSource = branchDiffPreview.data?.sources.find( + const preview = branchDiffPreview.isSuccess ? branchDiffPreview.data : null; + const selectedGitSource = preview?.sources.find( (source) => source.kind === (selectedGitScope === "unstaged" @@ -471,7 +471,6 @@ export default function DiffPanel({ const refreshPreviewQuery = branchDiffPreview.refresh; const refreshDiffFromUserAction = refreshPreviewQuery; - const preview = branchDiffPreview.data; const gitDiff = selectedGitSource?.diff; const toolCallPreview = @@ -479,7 +478,9 @@ export default function DiffPanel({ const selectedPatch = toolCallSelection ? toolCallPreview?.patch : selectedTurn - ? activeCheckpointDiff.data?.diff + ? activeCheckpointDiff.error === null + ? activeCheckpointDiff.data?.diff + : undefined : gitDiff; const isSelectedPatchTruncated = toolCallPreview?.truncated === true || @@ -609,6 +610,7 @@ export default function DiffPanel({ remainingFileCount, loadNextFiles, } = useReviewFilePatches({ + reviewKey: collapseScopeKey, environmentId: activeThread?.environmentId, cwd: branchDiffPreview.data?.cwd, source: lazySource, @@ -1453,7 +1455,7 @@ export default function DiffPanel({ projectId={activeThread?.projectId} viewerRef={attachCodeView} onScroll={codeViewScroll.onScroll} - codeViewKey={reviewDiffViewerKey(codeViewMountKey, lazySource ? filePatchFamily : null)} + codeViewKey={`${codeViewMountKey}:${filePatchFamily}`} className="h-full min-h-0 overflow-auto" files={codeViewFiles} pendingFiles={pendingFiles} diff --git a/apps/web/src/components/diffs/reviewFileDiffRetention.test.ts b/apps/web/src/components/diffs/reviewFileDiffRetention.test.ts deleted file mode 100644 index 059705dc26f77..0000000000000 --- a/apps/web/src/components/diffs/reviewFileDiffRetention.test.ts +++ /dev/null @@ -1,109 +0,0 @@ -import type { FileDiffMetadata } from "@pierre/diffs"; -import { describe, expect, it } from "vite-plus/test"; - -import { - isPendingReviewFile, - retainLoadedReviewFiles, - retainedReviewFile, - reviewDiffViewerKey, - reviewPatchIndicesForFamily, - reviewSnapshotFamily, - reviewSnapshotScope, -} from "./reviewFileDiffRetention"; - -const identity = { - environmentId: "env-1", - cwd: "/repo", - kind: "working-tree", - baseRef: "HEAD", - ignoreWhitespace: false, -}; - -function file(name: string, cacheKey?: string): FileDiffMetadata { - return { name, ...(cacheKey ? { cacheKey } : {}) } as FileDiffMetadata; -} - -describe("review snapshot identity", () => { - it("keeps the same review across git snapshots", () => { - const family = reviewSnapshotFamily(identity); - expect(reviewSnapshotFamily(identity)).toBe(family); - expect(reviewSnapshotScope(family, "hash-a")).not.toBe(reviewSnapshotScope(family, "hash-b")); - expect(reviewDiffViewerKey("section", family)).toBe(`section:${family}`); - expect(reviewDiffViewerKey("section", null)).toBe("section:preview"); - }); - - it("keeps loaded file indexes when the snapshot family is unchanged", () => { - const family = reviewSnapshotFamily(identity); - expect(reviewPatchIndicesForFamily({ family, indices: [0, 1, 2, 3, 8] }, family)).toEqual([ - 0, 1, 2, 3, 8, - ]); - expect( - reviewPatchIndicesForFamily({ family, indices: [0, 1, 2, 3, 8] }, `${family}:other`), - ).toEqual([0, 1, 2, 3]); - }); -}); - -describe("retained review files", () => { - const loaded = file("src/app.ts", "hash:src/app.ts"); - const placeholder = file("src/app.ts", "next:src/app.ts:pending"); - - it("keeps the open file until the next patch arrives", () => { - expect( - retainedReviewFile({ - loaded: null, - previous: loaded, - placeholder, - sameFamily: true, - pending: true, - }), - ).toBe(loaded); - }); - - it("drops the open file once its replacement fails or no longer includes it", () => { - expect( - retainedReviewFile({ - loaded: null, - previous: loaded, - placeholder, - sameFamily: true, - pending: false, - }), - ).toBe(placeholder); - }); - - it("uses the newly loaded patch", () => { - const next = file("src/app.ts", "hash-b:src/app.ts"); - expect( - retainedReviewFile({ - loaded: next, - previous: loaded, - placeholder, - sameFamily: true, - pending: false, - }), - ).toBe(next); - }); - - it("does not carry a file into a different review", () => { - expect( - retainedReviewFile({ - loaded: null, - previous: loaded, - placeholder, - sameFamily: false, - pending: true, - }), - ).toBe(placeholder); - expect(isPendingReviewFile(placeholder)).toBe(true); - }); - - it("stores loaded files and ignores placeholders", () => { - const previous = new Map([["src/app.ts", loaded]]); - expect(retainLoadedReviewFiles(previous, [loaded, placeholder], (entry) => entry.name)).toBe( - previous, - ); - const next = file("src/app.ts", "hash-b:src/app.ts"); - const retained = retainLoadedReviewFiles(previous, [next], (entry) => entry.name); - expect(retained.get("src/app.ts")).toBe(next); - }); -}); diff --git a/apps/web/src/components/diffs/reviewFileDiffRetention.ts b/apps/web/src/components/diffs/reviewFileDiffRetention.ts deleted file mode 100644 index 28c9c1dd10de2..0000000000000 --- a/apps/web/src/components/diffs/reviewFileDiffRetention.ts +++ /dev/null @@ -1,92 +0,0 @@ -import type { FileDiffMetadata } from "@pierre/diffs"; - -const INITIAL_PATCH_INDICES = [0, 1, 2, 3] as const; -const EMPTY_RETAINED_FILES: ReadonlyMap = new Map(); - -export interface ReviewSnapshotIdentity { - readonly environmentId: string | undefined; - readonly cwd: string | undefined; - readonly kind: string | undefined; - readonly baseRef: string | null; - readonly ignoreWhitespace: boolean; -} - -/** Identity of a review that survives a new git snapshot. The diff hash is not part of it. */ -export function reviewSnapshotFamily(input: ReviewSnapshotIdentity): string { - return JSON.stringify([ - input.environmentId, - input.cwd, - input.kind, - input.baseRef, - input.ignoreWhitespace, - ]); -} - -export function reviewSnapshotScope(family: string, diffHash: string | undefined): string { - return JSON.stringify([family, diffHash ?? null]); -} - -/** - * Keep requesting the files already on screen when the snapshot changes. A different - * review starts over at the first page. - */ -export function reviewPatchIndicesForFamily( - requested: { readonly family: string; readonly indices: readonly number[] }, - family: string, -): readonly number[] { - return requested.family === family ? requested.indices : INITIAL_PATCH_INDICES; -} - -export function isPendingReviewFile(file: { readonly cacheKey?: string | undefined }): boolean { - return file.cacheKey?.endsWith(":pending") === true; -} - -/** - * Show the file already on screen while its replacement is still loading. - * A failure, or a snapshot that no longer contains the file, must not keep the old diff. - */ -export function retainedReviewFile(input: { - readonly loaded: T | null; - readonly previous: T | undefined; - readonly placeholder: T; - readonly sameFamily: boolean; - readonly pending: boolean; -}): T { - if (input.loaded) return input.loaded; - if (input.pending && input.sameFamily && input.previous && !isPendingReviewFile(input.previous)) { - return input.previous; - } - return input.placeholder; -} - -export function retainLoadedReviewFiles( - previous: ReadonlyMap, - files: readonly FileDiffMetadata[], - pathOf: (file: FileDiffMetadata) => string, -): ReadonlyMap { - const next = new Map(); - for (const file of files) { - if (!isPendingReviewFile(file)) next.set(pathOf(file), file); - } - if (next.size === previous.size) { - let unchanged = true; - for (const [path, file] of next) { - if (previous.get(path) !== file) { - unchanged = false; - break; - } - } - if (unchanged) return previous; - } - return next.size === 0 ? EMPTY_RETAINED_FILES : next; -} - -/** The viewer remounts for a new review section, not for a new git snapshot of the same one. */ -export function reviewDiffViewerKey( - sectionMountKey: string, - snapshotFamily: string | null, -): string { - return snapshotFamily === null - ? `${sectionMountKey}:preview` - : `${sectionMountKey}:${snapshotFamily}`; -} diff --git a/apps/web/src/components/diffs/useReviewFilePatches.ts b/apps/web/src/components/diffs/useReviewFilePatches.ts index 286881e8ec421..196e14526bd73 100644 --- a/apps/web/src/components/diffs/useReviewFilePatches.ts +++ b/apps/web/src/components/diffs/useReviewFilePatches.ts @@ -7,18 +7,16 @@ import type { } from "@t3tools/contracts"; import * as AsyncResult from "effect/reactivity/AsyncResult"; import * as Atom from "effect/reactivity/Atom"; -import { useCallback, useContext, useEffect, useMemo, useRef, useState } from "react"; +import { useContext, useEffect, useRef, useState } from "react"; import { useStableCallback } from "~/hooks/useStableCallback"; -import { getRenderablePatch, resolveFileDiffPath, type RenderablePatch } from "~/lib/diffRendering"; -import { reviewEnvironment } from "~/state/review"; import { - isPendingReviewFile, - retainLoadedReviewFiles, + getRenderablePatch, + resolveFileDiffPath, retainedReviewFile, - reviewPatchIndicesForFamily, - reviewSnapshotFamily, - reviewSnapshotScope, -} from "./reviewFileDiffRetention"; + retainLoadedReviewFiles, + type RenderablePatch, +} from "~/lib/diffRendering"; +import { reviewEnvironment } from "~/state/review"; const EMPTY_RETAINED_FILES: ReadonlyMap = new Map(); @@ -26,9 +24,10 @@ const filePathCollator = new Intl.Collator(undefined, { numeric: true, sensitivity: "base", }); -const DEFAULT_INDICES = [0, 1, 2, 3]; +const PAGE_SIZE = 4; export function useReviewFilePatches({ + reviewKey, environmentId, cwd, source, @@ -38,6 +37,7 @@ export function useReviewFilePatches({ previewInput, preview, }: { + reviewKey: string | null; environmentId: EnvironmentId | undefined; cwd: string | undefined; source: ReviewDiffPreviewSource | null; @@ -49,165 +49,170 @@ export function useReviewFilePatches({ preview: RenderablePatch | null; }) { const registry = useContext(RegistryContext); - const snapshotFamily = reviewSnapshotFamily({ + const family = JSON.stringify([ + reviewKey, + environmentId, + cwd, + source?.id, + source?.kind, + source?.baseRef, + baseRef, + source?.headRef, + ignoreWhitespace, + ]); + const scope = JSON.stringify([family, source?.diffHash]); + const lazy = source?.truncated === true && source.files !== undefined; + const files = lazy ? sortedReviewFiles(source) : []; + const [requested, setRequested] = useState<{ family: string; paths: readonly string[] }>({ + family, + paths: [], + }); + const [retained, setRetained] = useState(() => ({ + family, + source, + byPath: + !lazy && preview?.kind === "files" + ? retainLoadedReviewFiles(EMPTY_RETAINED_FILES, preview.files) + : EMPTY_RETAINED_FILES, + })); + const previousByPath = retained.family === family ? retained.byPath : EMPTY_RETAINED_FILES; + const requestedPaths = new Set([ + ...files.slice(0, PAGE_SIZE).map((file) => file.path), + ...(requested.family === family ? requested.paths : []), + ...previousByPath.keys(), + ]); + const queries = reviewFileQueries({ environmentId, cwd, - kind: source?.kind, + source, baseRef, ignoreWhitespace, + previewInput, + scope, + files, + requestedPaths, }); - const scope = reviewSnapshotScope(snapshotFamily, source?.diffHash); - const [requested, setRequested] = useState({ family: snapshotFamily, indices: [0, 1, 2, 3] }); - const [retainedFiles, setRetainedFiles] = useState<{ - readonly family: string; - readonly byPath: ReadonlyMap; - }>({ family: snapshotFamily, byPath: EMPTY_RETAINED_FILES }); - const previousByPath = retainedFiles.family === snapshotFamily ? retainedFiles.byPath : EMPTY_RETAINED_FILES; - const files = sortedReviewFiles(source); - const queries = reviewFileQueries({ environmentId, cwd, source, baseRef, ignoreWhitespace, previewInput, scope, - requested: { scope, indices: [...reviewPatchIndicesForFamily(requested, snapshotFamily)] } }); const queriedPatchesAtom = reviewPatchesAtom( JSON.stringify([theme, queries.map(({ index, target }) => [index, target])]), ); const patches = useAtomValue(queriedPatchesAtom); const retry = useStableCallback((path: string) => { - const requestFiles = useCallback( - (nextIndices: number[]) => - setRequested((current) => { - const previous = reviewPatchIndicesForFamily(current, snapshotFamily); - const added = nextIndices.filter((index) => !previous.includes(index)); - if (added.length === 0 && current.family === snapshotFamily) return current; - return { family: snapshotFamily, indices: [...previous, ...added] }; - }), - [snapshotFamily], - ); - const requestFile = useCallback((index: number) => requestFiles([index]), [requestFiles]); - const renderableFiles = useMemo( - () => - source - ? files.map((file, index): FileDiffMetadata => { - const result = patches.get(index); - const loaded = - result?._tag === "Success" && result.value.patch?.kind === "files" - ? (result.value.patch.files.find( - (candidate) => resolveFileDiffPath(candidate) === file.path, - ) ?? null) - : null; - return retainedReviewFile({ - loaded, - previous: previousByPath.get(file.path), - sameFamily: true, - pending: result === undefined || result._tag === "Initial" || result.waiting, - placeholder: { - name: file.path, - ...(file.previousPath ? { prevName: file.previousPath } : {}), - type: file.previousPath ? "rename-changed" : "change", - hunks: [], - additionLines: [], - deletionLines: [], - additionCharacterCount: 0, - deletionCharacterCount: 0, - splitLineCount: 0, - unifiedLineCount: 0, - isPartial: true, - cacheKey: `${scope}:${file.path}:pending`, - }, - }); - }) - : (preview?.kind === "files" ? preview.files : []).toSorted((a, b) => - filePathCollator.compare(resolveFileDiffPath(a), resolveFileDiffPath(b)), - ), - [source, files, patches, scope, preview, previousByPath], - ); - const nextRetainedFiles = source - ? retainLoadedReviewFiles(previousByPath, renderableFiles, resolveFileDiffPath) - : EMPTY_RETAINED_FILES; - useEffect(() => { - if (!source) return; - if (retainedFiles.family === snapshotFamily && retainedFiles.byPath === nextRetainedFiles) - return; - setRetainedFiles({ family: snapshotFamily, byPath: nextRetainedFiles }); - }, [nextRetainedFiles, retainedFiles.byPath, retainedFiles.family, snapshotFamily, source]); - const pendingIndex = source - ? files.findIndex((file, index) => { - const patch = patches.get(index); - if (patch && patch._tag !== "Initial") return false; - const rendered = renderableFiles[index]; + const query = queries.find(({ index }) => files[index]?.path === path)?.query; + if (query) registry.refresh(query); + }); + const requestFiles = (indices: number[]) => + setRequested((current) => { + const paths = new Set(current.family === family ? current.paths : []); + for (const index of indices) { + const file = files[index]; + if (file) paths.add(file.path); + } + return current.family === family && paths.size === current.paths.length + ? current + : { family, paths: [...paths] }; + }); + const renderableFiles = lazy + ? files.map((file, index): FileDiffMetadata => { + const result = patches.get(index); + const loaded = + result?._tag === "Success" && result.value.patch?.kind === "files" + ? (result.value.patch.files.find( + (candidate) => resolveFileDiffPath(candidate) === file.path, + ) ?? null) + : null; + return retainedReviewFile({ + loaded, + previous: previousByPath.get(file.path), + sameFamily: true, + pending: + result === undefined || + result._tag === "Initial" || + (result._tag === "Success" && result.waiting), + placeholder: { + name: file.path, + ...(file.previousPath ? { prevName: file.previousPath } : {}), + type: file.previousPath ? "rename-changed" : "change", + hunks: [], + additionLines: [], + deletionLines: [], + additionCharacterCount: 0, + deletionCharacterCount: 0, + splitLineCount: 0, + unifiedLineCount: 0, + isPartial: true, + cacheKey: `${scope}:${file.path}:pending`, + }, + }); + }) + : (preview?.kind === "files" ? preview.files : []).toSorted((a, b) => + filePathCollator.compare(resolveFileDiffPath(a), resolveFileDiffPath(b)), + ); + const nextRetained = !source + ? EMPTY_RETAINED_FILES + : !lazy && retained.family === family && retained.source === source + ? previousByPath + : retainLoadedReviewFiles(previousByPath, renderableFiles); + if ( + retained.family !== family || + retained.source !== source || + retained.byPath !== nextRetained + ) { + setRetained({ family, source, byPath: nextRetained }); + } + const readyFilePaths = new Set( + files + .filter((_, index) => { + const result = patches.get(index); return ( - rendered === undefined || - isPendingReviewFile(rendered) || - resolveFileDiffPath(rendered) !== file.path + (result !== undefined && result._tag !== "Initial") || + !renderableFiles[index]?.cacheKey?.endsWith(":pending") ); }) - : -1; - const settledFileCount = source + .map((file) => file.path), + ); + const pendingIndex = files.findIndex((file) => !readyFilePaths.has(file.path)); + const settledFileCount = lazy ? pendingIndex < 0 ? files.length : pendingIndex - : preview?.kind === "files" - ? preview.files.length - : 0; - const loadNextFiles = useCallback( - () => { - if (queries.some(({ index }) => index === settledFileCount)) return; - requestFiles(Array.from({ length: 4 }, (_, index) => settledFileCount + index)); - }, - [queries, requestFiles, settledFileCount], - ); - const fileStates = useMemo( - () => - new Map( - files.map((file, index) => { - const patch = patches.get(index); - return [ - file.path, - { - error: - patch?._tag === "Failure" || - (patch?._tag === "Success" && - (patch.value.patch?.kind !== "files" || - !patch.value.patch.files.some( - (candidate) => resolveFileDiffPath(candidate) === file.path, - ))), - truncated: patch?._tag === "Success" && patch.value.source.truncated, - }, - ] as const; - }), - ), - [files, patches], - ); - const readyFilePaths = useMemo( - () => - new Set( - files - .filter((file, index) => { - const patch = patches.get(index); - if (patch && patch._tag !== "Initial") return true; - const rendered = renderableFiles[index]; - return ( - rendered !== undefined && - !isPendingReviewFile(rendered) && - resolveFileDiffPath(rendered) === file.path - ); - }) - .map((file) => file.path), - ), - [files, patches, renderableFiles], + : renderableFiles.length; + const fileStates = new Map( + files.map((file, index) => { + const patch = patches.get(index); + return [ + file.path, + { + error: + patch?._tag === "Failure" || + (patch?._tag === "Success" && + !patch.waiting && + (patch.value.patch?.kind !== "files" || + !patch.value.patch.files.some( + (candidate) => resolveFileDiffPath(candidate) === file.path, + ))), + truncated: patch?._tag === "Success" && patch.value.source.truncated, + }, + ] as const; + }), ); return { scope, - family: snapshotFamily, + family, fileStates, isPending: [...patches.values()].some((patch) => patch._tag === "Initial" || patch.waiting), retry, - requestFile, + requestFile: (index: number) => requestFiles([index]), readyFilePaths, renderableFiles, settledFileCount, - loadNextFiles, + loadNextFiles: () => { + if (queries.some(({ index }) => index === settledFileCount)) return; + requestFiles(Array.from({ length: PAGE_SIZE }, (_, index) => settledFileCount + index)); + }, }; } +// The refresh effect keys on the identity of these queries. function reviewFileQueries({ environmentId, cwd, @@ -216,7 +221,8 @@ function reviewFileQueries({ ignoreWhitespace, previewInput, scope, - requested, + files, + requestedPaths, }: { environmentId: EnvironmentId | undefined; cwd: string | undefined; @@ -225,17 +231,16 @@ function reviewFileQueries({ ignoreWhitespace: boolean; previewInput: ReviewDiffPreviewInput | undefined; scope: string; - requested: { scope: string; indices: number[] }; + files: ReturnType; + requestedPaths: ReadonlySet; }) { - const indices = requested.scope === scope ? requested.indices : DEFAULT_INDICES; - const files = sortedReviewFiles(source); const queries = !environmentId || !cwd || !source ? [] - : indices - .filter((index) => index < files.length) - .map((index) => { - const file = files[index]!; + : files + .map((file, index) => ({ file, index })) + .filter(({ file }) => requestedPaths.has(file.path)) + .map(({ file, index }) => { const target: ReviewPatchTarget = { environmentId, input: { @@ -264,6 +269,7 @@ function sortedReviewFiles(source: ReviewDiffPreviewSource | null) { type ReviewPatchTarget = Parameters[0]; +// Derived atoms parse each query result once, even when another file finishes loading. const parsedPatchAtom = Atom.family((key: string) => { const [theme, target] = JSON.parse(key) as ["light" | "dark", ReviewPatchTarget]; return Atom.map(reviewEnvironment.diffFilePatch(target), (result) => @@ -283,6 +289,7 @@ const parsedPatchAtom = Atom.family((key: string) => { ); }); +// Keyed by theme and query targets so every render with the same files reads the same atom. const reviewPatchesAtom = Atom.family((key: string) => { const [theme, entries] = JSON.parse(key) as [ "light" | "dark", diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 9c8d470d7a7ad..d4522316408dd 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -7,8 +7,8 @@ import { capitalizeTerm, sourceControlClients, } from "@t3tools/client-runtime/source-control-clients"; -import type { CodeViewItem, DiffLineAnnotation, SelectedLineRange } from "@pierre/diffs"; -import type { CodeViewAnnotationArgs, CodeViewDiffItem } from "@pierre/diffs/react"; +import type { CodeViewItem, DiffLineAnnotation, FileDiffMetadata, SelectedLineRange } from "@pierre/diffs"; +import type { CodeViewAnnotationArgs, CodeViewDiffItem, CodeViewHandle } from "@pierre/diffs/react"; import type { EnvironmentId, PullRequestDetailView, @@ -36,7 +36,7 @@ import { ChevronsDownUpIcon, ChevronsUpDownIcon, } from "lucide-react"; -import { useAtomRefresh } from "@effect/atom-react"; +import { RegistryContext } from "@effect/atom-react"; import * as Schema from "effect/Schema"; import { createContext, @@ -66,7 +66,8 @@ import { fileDiffRows } from "@t3tools/client-runtime/review-guide-tour"; import { isTestChangedFile } from "@t3tools/shared/changedFileKind"; import { orderDiffFiles } from "./pullRequestFileOrder.logic"; import { - buildFileDiffRenderKey, + buildFileDiffIdentityKey, + buildFileDiffContentVersion, fnv1a32, getDiffLineStat, getRenderablePatch, @@ -174,6 +175,7 @@ const NO_SLICES: ReadonlyArray = []; // Query results outlive the panel's slices, so their parses survive closing and reopening // without retaining anything beyond the query cache's lifetime. const sliceParses = new WeakMap>(); +const fileContentVersions = new WeakMap(); function parseSlice(slice: DiffSlice, cacheKey: string, ignoreWhitespace: boolean) { let parses = sliceParses.get(slice.source); @@ -416,7 +418,11 @@ function PullRequestCodeTab({ readonly key: string; readonly cursor: string | null; readonly slices: ReadonlyArray; - }>({ key: "", cursor: null, slices: NO_SLICES }); + readonly refreshing: ReadonlyArray | null; + }>({ key: "", cursor: null, slices: NO_SLICES, refreshing: null }); + const [viewer, setViewer] = useState | null>( + null, + ); const referenceKey = pullRequestReviewKey(reference); // A commit opened from the timeline or left selected across a force-push may no longer be @@ -438,11 +444,12 @@ function PullRequestCodeTab({ setFoldOverride(null); setVisibleCommitCount(COMMIT_PAGE_SIZE); setOrphansOpen(false); - setSliceState({ key: scopeKey, cursor: null, slices: NO_SLICES }); + setSliceState({ key: scopeKey, cursor: null, slices: NO_SLICES, refreshing: null }); } const loadedSlices = sliceState.key === scopeKey ? sliceState.slices : NO_SLICES; const cursor = sliceState.key === scopeKey ? sliceState.cursor : null; + const registry = use(RegistryContext); const diffQuery = useEnvironmentQuery( pullRequestEnvironment.diff({ environmentId, @@ -458,7 +465,7 @@ function PullRequestCodeTab({ readonly cursor: string | null; readonly scopeKey: string; } | null>(null); - const data = diffQuery.data; + const data = diffQuery.isSuccess && !diffQuery.isPending ? diffQuery.data : null; if ( data !== null && (appliedPage?.data !== data || @@ -476,9 +483,19 @@ function PullRequestCodeTab({ nextCursor: data.nextCursor, omittedFileStats: data.omittedFileStats ?? [], }; + if (previous.key === scopeKey && previous.refreshing !== null) { + const index = previous.refreshing.findIndex((slice) => slice.cursor === cursor); + const refreshing = [ + ...(index < 0 ? previous.refreshing : previous.refreshing.slice(0, index)), + next, + ]; + return refreshing.length >= previous.slices.length || next.nextCursor === null + ? { key: scopeKey, cursor, slices: refreshing, refreshing: null } + : { ...previous, refreshing }; + } const index = slices.findIndex((slice) => slice.cursor === cursor); if (index === -1) { - return { key: scopeKey, cursor, slices: [...slices, next] }; + return { key: scopeKey, cursor, slices: [...slices, next], refreshing: null }; } const existing = slices[index]; if ( @@ -500,15 +517,22 @@ function PullRequestCodeTab({ return previous; } // Drop later slices when replacing a changed page; their cursors target the old diff. - return { key: scopeKey, cursor, slices: [...slices.slice(0, index), next] }; + return { key: scopeKey, cursor, slices: [...slices.slice(0, index), next], refreshing: null }; }); } - const refreshFirstDiffPage = useAtomRefresh( - pullRequestEnvironment.diff({ - environmentId, - input: { ...reference, ...(commit === null ? {} : { commit }) }, - }), - ); + const failedSliceIndex = loadedSlices.findIndex((slice) => slice.cursor === cursor); + if ( + diffQuery.error !== null && + !diffQuery.isPending && + (sliceState.refreshing !== null || failedSliceIndex !== -1) + ) { + setAppliedPage(null); + setSliceState((previous) => ({ + ...previous, + slices: previous.refreshing ?? previous.slices.slice(0, failedSliceIndex), + refreshing: null, + })); + } const reviewKey = referenceKey; const pendingComments = usePendingReviewComments(reference); const addComment = usePullRequestReviewStore((store) => store.addComment); @@ -540,44 +564,67 @@ function PullRequestCodeTab({ isViewed: isFileViewed, isStale: isFileViewedStale, } = filesViewed; - // Reread from the first page. It stays on screen so the reader does not jump; later pages - // belong to the snapshot being replaced, so they are fetched again. Each of those pages is - // its own cached query, so the walk asks for it again instead of replaying the old snapshot. - const refreshGeneration = useRef(0); - const refreshedCursor = useRef(undefined); - const appliedRefreshToken = useRef(refreshToken); - // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- a new refresh token clears loaded slices and refetches the first diff page and viewed files + const appliedRefresh = useRef({ refreshToken, backgroundRefreshToken }); + // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- refreshes mounted review pages from the environment query cache useEffect(() => { - if (appliedRefreshToken.current === refreshToken) return; - appliedRefreshToken.current = refreshToken; - refreshGeneration.current += 1; - refreshedCursor.current = undefined; - setSliceState((previous) => retainReviewDiffSlices(previous, scopeKey)); - // An equal refetch keeps the answer's identity, so forget the applied page to reapply it. + const previous = appliedRefresh.current; + appliedRefresh.current = { refreshToken, backgroundRefreshToken }; + if ( + previous.refreshToken === refreshToken && + previous.backgroundRefreshToken === backgroundRefreshToken + ) + return; + registry.refresh( + pullRequestEnvironment.diff({ + environmentId, + input: { ...reference, ...(commit === null ? {} : { commit }) }, + }), + ); setAppliedPage(null); - refreshFirstDiffPage(); - refreshFilesViewed(); - }, [refreshToken, scopeKey, refreshFirstDiffPage, refreshFilesViewed]); - const appliedBackgroundRefreshToken = useRef(backgroundRefreshToken); - // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- a background refresh token refetches the first diff page and viewed files while keeping slices - useEffect(() => { - if (appliedBackgroundRefreshToken.current === backgroundRefreshToken) return; - appliedBackgroundRefreshToken.current = backgroundRefreshToken; - refreshGeneration.current += 1; - refreshedCursor.current = undefined; - // Keeping slices mounted preserves scroll; the page merge invalidates changed pages. - setSliceState((previous) => ({ ...previous, cursor: null })); - refreshFirstDiffPage(); + // Stage a fresh walk while all displayed pages keep the reader's scroll geometry. + setSliceState((current) => ({ + ...retainReviewDiffSlices(current, scopeKey), + refreshing: [], + })); refreshFilesViewed(); - }, [backgroundRefreshToken, refreshFirstDiffPage, refreshFilesViewed]); + }, [ + refreshToken, + backgroundRefreshToken, + environmentId, + reference, + commit, + scopeKey, + registry, + refreshFilesViewed, + ]); useEffect(() => { - if (refreshGeneration.current === 0 || cursor === null) return; - if (refreshedCursor.current === cursor) return; - refreshedCursor.current = cursor; - diffQuery.refresh(); - }, [cursor, diffQuery.refresh]); - const nextCursor = loadedSlices.at(-1)?.nextCursor ?? null; - + const last = sliceState.refreshing?.at(-1); + if ( + !last || + last.cursor !== cursor || + last.nextCursor === null || + diffQuery.isPending || + diffQuery.error !== null + ) + return; + registry.refresh( + pullRequestEnvironment.diff({ + environmentId, + input: { ...reference, cursor: last.nextCursor, ...(commit === null ? {} : { commit }) }, + }), + ); + // oxlint-disable-next-line react/set-state-in-effect -- Advance only after invalidating the cached next page. + setSliceState((current) => ({ ...current, cursor: last.nextCursor })); + }, [ + sliceState.refreshing, + cursor, + diffQuery.isPending, + diffQuery.error, + registry, + environmentId, + reference, + commit, + ]); const codeViewReveal = useCodeViewFileReveal(); const answeredRevealNonce = useRef(null); @@ -654,7 +701,7 @@ function PullRequestCodeTab({ })(); const annotatedFiles = files.map((fileDiff) => { - const fileKey = buildFileDiffRenderKey(fileDiff); + const fileKey = buildFileDiffIdentityKey(fileDiff); const path = resolveFileDiffPath(fileDiff); const groups = new Map(); // Stacked view draws a context row once, so its old side shares the new side's annotation. @@ -777,6 +824,11 @@ function PullRequestCodeTab({ foldOverride === null && filesViewedEnabled && isFileViewed(path) && !isFileViewedStale(path); const items: CodeViewDiffItem[] = annotatedFiles.map( ({ fileKey, path, fileDiff, annotations, annotationsVersion }) => { + let contentVersion = fileContentVersions.get(fileDiff); + if (contentVersion === undefined) { + contentVersion = buildFileDiffContentVersion(fileDiff); + fileContentVersions.set(fileDiff, contentVersion); + } const collapsed = isFileDiffCollapsed( fileKey, effectiveFoldOverride, @@ -792,7 +844,9 @@ function PullRequestCodeTab({ fileDiff, annotations, collapsed, - version: fnv1a32(`${collapsed ? "1" : "0"}:${viewedMark}:${annotationsVersion}`), + version: fnv1a32( + `${contentVersion}:${collapsed ? "1" : "0"}:${viewedMark}:${annotationsVersion}`, + ), }; }, ); @@ -804,12 +858,19 @@ function PullRequestCodeTab({ // Do not re-arm the visible sentinel after failure; it would retry indefinitely. const canLoadNextSlice = + sliceState.refreshing === null && nextCursor !== null && nextCursor !== cursor && !diffQuery.isPending && diffQuery.error === null; const loadNextSlice = () => { - if (nextCursor === null) return; + if (nextCursor === null || sliceState.refreshing !== null) return; + registry.refresh( + pullRequestEnvironment.diff({ + environmentId, + input: { ...reference, cursor: nextCursor, ...(commit === null ? {} : { commit }) }, + }), + ); setSliceState((previous) => ({ ...previous, cursor: nextCursor })); }; const sentinelRef = (node: HTMLDivElement | null) => { @@ -883,7 +944,7 @@ function PullRequestCodeTab({ if (!range || !canSelectLines) return; const item = context.item; if (item.type !== "diff") return; - const file = files.find((candidate) => buildFileDiffRenderKey(candidate) === item.id); + const file = files.find((candidate) => buildFileDiffIdentityKey(candidate) === item.id); if (!file) return; // On a host without ranged comments a range collapses to its last line: one that silently // lost its first line there would be worse than one line. @@ -910,7 +971,7 @@ function PullRequestCodeTab({ text: string, onFinish: (comment: ReviewCommentContext) => void, ) => { - const file = files.find((candidate) => buildFileDiffRenderKey(candidate) === anchor.fileKey); + const file = files.find((candidate) => buildFileDiffIdentityKey(candidate) === anchor.fileKey); const comment = file === undefined ? null @@ -1678,6 +1739,7 @@ function PullRequestCodeTab({ {/* The viewer must own its scroll element to position virtual rows within it. */} + key={scopeKey} className="@container h-full overflow-auto [scrollbar-gutter:stable]" containerRef={scrollerRef} viewerRef={codeViewReveal.attach} diff --git a/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts index 4a08e97ff6c1d..494b39f0ef650 100644 --- a/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDiff.logic.test.ts @@ -24,11 +24,11 @@ describe("retainReviewDiffSlices", () => { const first = { cursor: null, patch: "page-1" }; const slices = [first, { cursor: "page-2", patch: "page-2" }]; - it("keeps the first page and drops later ones when the same review refreshes", () => { + it("keeps every displayed page while the same review refreshes", () => { expect(retainReviewDiffSlices({ key: "pr-1", cursor: "page-2", slices }, "pr-1")).toEqual({ key: "pr-1", cursor: null, - slices: [first], + slices, }); const open = { key: "pr-1", cursor: null, slices: [first] }; expect(retainReviewDiffSlices(open, "pr-1")).toBe(open); diff --git a/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts b/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts index c9774dc316ba4..30c7053ce6fc9 100644 --- a/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDiff.logic.ts @@ -7,24 +7,12 @@ export interface ReviewDiffSliceState { readonly slices: ReadonlyArray; } -/** - * A refresh rereads from the first page. That page stays on screen so the reader does not - * jump. Later pages are positions in the snapshot being replaced: keeping them when the - * first page comes back unchanged would leave those files stale for good. - */ export function retainReviewDiffSlices( previous: ReviewDiffSliceState, scopeKey: string, ): ReviewDiffSliceState { - if (previous.key !== scopeKey) { - return { key: scopeKey, cursor: null, slices: [] }; - } - const first = previous.slices[0]; - if (first === undefined) { - return previous.cursor === null ? previous : { key: scopeKey, cursor: null, slices: [] }; - } - if (previous.slices.length === 1 && previous.cursor === null) return previous; - return { key: scopeKey, cursor: null, slices: [first] }; + if (previous.key !== scopeKey) return { key: scopeKey, cursor: null, slices: [] }; + return previous.cursor === null ? previous : { ...previous, cursor: null }; } /** Only rendered hunks can anchor comments; list others off-diff. */ diff --git a/apps/web/src/hooks/useProactivePanels.ts b/apps/web/src/hooks/useProactivePanels.ts index 1fe137872ba08..d2b68f101f5f3 100644 --- a/apps/web/src/hooks/useProactivePanels.ts +++ b/apps/web/src/hooks/useProactivePanels.ts @@ -1,4 +1,3 @@ -import { shouldApplyProactiveChangesScope } from "../components/ChatView.logic"; import type { RunId, ScopedThreadRef, ThreadLinkedPullRequest } from "@t3tools/contracts"; import { scopedThreadKey } from "@t3tools/client-runtime/environment"; import { useEffect, useRef } from "react"; @@ -11,6 +10,7 @@ import { import type { TurnDiffSummary } from "../types"; import { observeProactivePanelUserChoice, + shouldApplyProactiveChangesScope, resolveProactiveTurnDiffAction, shouldOpenProactivePullRequest, shouldOpenProactiveTurnDiff, diff --git a/apps/web/src/lib/diffRendering.test.ts b/apps/web/src/lib/diffRendering.test.ts index dc8ced6d25738..1ea81f441bc73 100644 --- a/apps/web/src/lib/diffRendering.test.ts +++ b/apps/web/src/lib/diffRendering.test.ts @@ -7,6 +7,8 @@ import { buildFileDiffRenderKey, getDiffLineStat, getRenderablePatch, + retainLoadedReviewFiles, + retainedReviewFile, resolveFileDiffPath, resolveFileDiffPreviousPath, } from "./diffRendering"; @@ -266,6 +268,77 @@ describe("getRenderablePatch", () => { }); describe("diff file reconciliation", () => { + const reviewFile = (line: string) => { + const patch = getRenderablePatch( + [ + "diff --git a/app.ts b/app.ts", + "--- a/app.ts", + "+++ b/app.ts", + "@@ -1 +1 @@", + "-before", + `+${line}`, + ].join("\n"), + "review", + ); + if (patch?.kind !== "files" || !patch.files[0]) throw new Error("Expected a file patch"); + return patch.files[0]; + }; + const loaded = reviewFile("after"); + const placeholder = { + ...loaded, + cacheKey: "refresh:app.ts:pending", + hunks: [], + additionLines: [], + deletionLines: [], + splitLineCount: 0, + unifiedLineCount: 0, + }; + + it("keeps loaded hunks during a same-review refresh until fresh content arrives", () => { + const previous = retainLoadedReviewFiles(new Map(), [loaded]); + const pending = retainedReviewFile({ + loaded: null, + previous: previous.get("app.ts"), + placeholder, + sameFamily: true, + pending: true, + }); + expect(pending).toBe(loaded); + expect(getDiffLineStat([pending])).toEqual({ additions: 1, deletions: 1 }); + + const refreshed = retainedReviewFile({ + loaded: reviewFile("newer"), + previous: pending, + placeholder, + sameFamily: true, + pending: false, + }); + expect(refreshed.additionLines.join("")).toBe("newer"); + expect(buildFileDiffContentVersion(refreshed)).not.toBe(buildFileDiffContentVersion(pending)); + }); + + it("drops old hunks after a failed refresh or a switch to another review", () => { + const failed = retainedReviewFile({ + loaded: null, + previous: loaded, + placeholder, + sameFamily: true, + pending: false, + }); + expect(failed.hunks).toEqual([]); + const retained = retainLoadedReviewFiles(new Map([["app.ts", loaded]]), [failed]); + expect(retained.has("app.ts")).toBe(false); + + const switched = retainedReviewFile({ + loaded: null, + previous: loaded, + placeholder, + sameFamily: false, + pending: true, + }); + expect(switched.hunks).toEqual([]); + }); + it("keeps Pierre's render key stable when a partial diff hydrates", () => { const patch = [ "diff --git a/example.ts b/example.ts", diff --git a/apps/web/src/lib/diffRendering.ts b/apps/web/src/lib/diffRendering.ts index 09b4d792b0e52..e2007c46fb696 100644 --- a/apps/web/src/lib/diffRendering.ts +++ b/apps/web/src/lib/diffRendering.ts @@ -209,6 +209,40 @@ export function buildFileDiffRenderKey(fileDiff: FileDiffMetadata): string { return cacheKey.endsWith(":hydrated") ? cacheKey.slice(0, -":hydrated".length) : cacheKey; } +export function retainedReviewFile(input: { + readonly loaded: FileDiffMetadata | null; + readonly previous: FileDiffMetadata | undefined; + readonly placeholder: FileDiffMetadata; + readonly sameFamily: boolean; + readonly pending: boolean; +}): FileDiffMetadata { + if (input.loaded) return input.loaded; + if ( + input.pending && + input.sameFamily && + input.previous && + !input.previous.cacheKey?.endsWith(":pending") + ) { + return input.previous; + } + return input.placeholder; +} + +export function retainLoadedReviewFiles( + previous: ReadonlyMap, + files: readonly FileDiffMetadata[], +): ReadonlyMap { + const next = new Map( + files + .filter((file) => !file.cacheKey?.endsWith(":pending")) + .map((file) => [resolveFileDiffPath(file), file]), + ); + return next.size === previous.size && + [...next].every(([path, file]) => previous.get(path) === file) + ? previous + : next; +} + function hashFileDiffPart(hash: number, value: string | number | boolean | undefined): number { const serialized = value === undefined ? "undefined" : String(value); const withLength = fnv1a32(`${typeof value}:${serialized.length}:`, hash); From 3f37b057c12b858d747bc67d6923702354aef076 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Mon, 19 Oct 2026 21:01:16 +0000 Subject: [PATCH 05/17] fix(web): retain requested review paths after a failed refresh --- apps/web/src/components/diffs/useReviewFilePatches.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/apps/web/src/components/diffs/useReviewFilePatches.ts b/apps/web/src/components/diffs/useReviewFilePatches.ts index 196e14526bd73..e926e8bfacd22 100644 --- a/apps/web/src/components/diffs/useReviewFilePatches.ts +++ b/apps/web/src/components/diffs/useReviewFilePatches.ts @@ -81,6 +81,9 @@ export function useReviewFilePatches({ ...(requested.family === family ? requested.paths : []), ...previousByPath.keys(), ]); + if (requested.family !== family || requested.paths.length !== requestedPaths.size) { + setRequested({ family, paths: [...requestedPaths] }); + } const queries = reviewFileQueries({ environmentId, cwd, From 6990f97b984bb419321068c49ac2c02961b85085 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Mon, 19 Oct 2026 21:24:25 +0000 Subject: [PATCH 06/17] fix(web): discard review drafts when their file changes --- .../pullRequest/PullRequestCodeTab.tsx | 65 +++++++++++++++++-- 1 file changed, 58 insertions(+), 7 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index d4522316408dd..8aa137635afa1 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -50,6 +50,7 @@ import { import { observeIntersection } from "~/lib/observeIntersection"; import { useLocalStorage } from "~/hooks/useLocalStorage"; +import { useStableCallback } from "~/hooks/useStableCallback"; import { useClientSettings, updateClientSettings } from "~/hooks/useSettings"; import { useRenderedAppearance } from "~/hooks/useTheme"; import { areAllDiffFilesCollapsed } from "~/lib/diffCollapse"; @@ -177,6 +178,15 @@ const NO_SLICES: ReadonlyArray = []; const sliceParses = new WeakMap>(); const fileContentVersions = new WeakMap(); +function fileContentVersion(file: FileDiffMetadata) { + let version = fileContentVersions.get(file); + if (version === undefined) { + version = buildFileDiffContentVersion(file); + fileContentVersions.set(file, version); + } + return version; +} + function parseSlice(slice: DiffSlice, cacheKey: string, ignoreWhitespace: boolean) { let parses = sliceParses.get(slice.source); if (parses === undefined) { @@ -331,6 +341,8 @@ interface MutableAnnotationGroup { interface DraftAnchor { readonly fileKey: string; + readonly contentVersion: number; + readonly scopeKey: string; readonly path: string; readonly oldPath: string | null; readonly position: PullRequestReviewPosition; @@ -409,6 +421,8 @@ function PullRequestCodeTab({ const [selectedLines, setSelectedLines] = useState<{ id: string; range: SelectedLineRange; + contentVersion: number; + scopeKey: string; } | null>(null); const [draft, setDraft] = useState(null); const [threadPending, setThreadPending] = useState(false); @@ -677,6 +691,23 @@ function PullRequestCodeTab({ }) : loadedFiles; const hiddenTestFileCount = loadedFiles.length - files.length; + const contentVersions = new Map( + loadedFiles.map((file) => [buildFileDiffIdentityKey(file), fileContentVersion(file)]), + ); + const isCurrentFile = (fileKey: string, version: number, anchorScopeKey: string) => + anchorScopeKey === scopeKey && + contentVersions.get(fileKey) === version && + !isFileDiffCollapsed(fileKey, effectiveFoldOverride, toggledFiles); + // Stable viewer identities preserve scroll, but line coordinates belong to one file version. + if (draft !== null && !isCurrentFile(draft.fileKey, draft.contentVersion, draft.scopeKey)) { + setDraft(null); + } + if ( + selectedLines !== null && + !isCurrentFile(selectedLines.id, selectedLines.contentVersion, selectedLines.scopeKey) + ) { + setSelectedLines(null); + } const filePaths = files.map((file) => resolveFileDiffPath(file)); const omittedFileStats = new Map( loadedSlices.flatMap((slice) => @@ -824,11 +855,7 @@ function PullRequestCodeTab({ foldOverride === null && filesViewedEnabled && isFileViewed(path) && !isFileViewedStale(path); const items: CodeViewDiffItem[] = annotatedFiles.map( ({ fileKey, path, fileDiff, annotations, annotationsVersion }) => { - let contentVersion = fileContentVersions.get(fileDiff); - if (contentVersion === undefined) { - contentVersion = buildFileDiffContentVersion(fileDiff); - fileContentVersions.set(fileDiff, contentVersion); - } + const contentVersion = fileContentVersion(fileDiff); const collapsed = isFileDiffCollapsed( fileKey, effectiveFoldOverride, @@ -945,7 +972,12 @@ function PullRequestCodeTab({ const item = context.item; if (item.type !== "diff") return; const file = files.find((candidate) => buildFileDiffIdentityKey(candidate) === item.id); - if (!file) return; + if ( + !file || + item.collapsed === true || + !isCurrentFile(item.id, fileContentVersion(item.fileDiff), scopeKey) + ) + return; // On a host without ranged comments a range collapses to its last line: one that silently // lost its first line there would be worse than one line. const path = resolveFileDiffPath(file); @@ -959,6 +991,8 @@ function PullRequestCodeTab({ if (anchor === null) return; setDraft({ fileKey: item.id, + contentVersion: fileContentVersion(file), + scopeKey, path, oldPath: previousPath === path ? null : previousPath, ...anchor, @@ -966,11 +1000,19 @@ function PullRequestCodeTab({ }); }; + // Portals can retain a submit handler from before a refresh invalidated its draft. + const isCurrentDraft = useStableCallback( + (anchor: DraftAnchor) => + draft === anchor && + canCommentOnLines && + isCurrentFile(anchor.fileKey, anchor.contentVersion, anchor.scopeKey), + ); const finishSelection = ( anchor: DraftAnchor, text: string, onFinish: (comment: ReviewCommentContext) => void, ) => { + if (!isCurrentDraft(anchor)) return; const file = files.find((candidate) => buildFileDiffIdentityKey(candidate) === anchor.fileKey); const comment = file === undefined @@ -1196,6 +1238,7 @@ function PullRequestCodeTab({ setSelectedLines(null); }} onComment={(body) => { + if (!isCurrentDraft(draft)) return; if (!canCommentOnLines) { if (onAddToAgentSelection) { finishSelection(draft, body, (comment) => @@ -1745,7 +1788,15 @@ function PullRequestCodeTab({ viewerRef={codeViewReveal.attach} items={items} selectedLines={selectedLines} - onSelectedLinesChange={setSelectedLines} + onSelectedLinesChange={(selection) => { + const contentVersion = + selection === null ? undefined : contentVersions.get(selection.id); + setSelectedLines( + selection !== null && contentVersion !== undefined + ? { ...selection, contentVersion, scopeKey } + : null, + ); + }} options={diffViewOptions} renderCodeViewFooter={renderCodeViewFooter} renderHeaderPrefix={renderHeaderPrefix} From 9332b3c02109ffb0643ef7127d59738ec6f7a9d4 Mon Sep 17 00:00:00 2001 From: Jake Leventhal Date: Sat, 24 Oct 2026 14:17:21 -0400 Subject: [PATCH 07/17] fix(web): drop the unused file patch scope binding The diff viewer now keys its mount on the patch family, so the scope destructured from useReviewFilePatches was left unused after the rebase. Co-Authored-By: Claude Opus 5.5 --- apps/web/src/components/DiffPanel.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index 5473734da0e00..2830e7dee4d13 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -600,7 +600,6 @@ export default function DiffPanel({ ...matchingBaseRefChoices.map(valueForBaseRefChoice), ]; const { - scope: filePatchScope, family: filePatchFamily, isPending: areFilePatchesPending, fileStates, From 170d57bdf81d25ab9b9fa5943774d1464af32f7b Mon Sep 17 00:00:00 2001 From: Jake Leventhal Date: Mon, 26 Oct 2026 09:22:42 -0400 Subject: [PATCH 08/17] fix(review): allow agent selection without review posting access --- .../src/components/pullRequest/PullRequestCodeTab.tsx | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 8aa137635afa1..c2242e4d067cd 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -7,7 +7,12 @@ import { capitalizeTerm, sourceControlClients, } from "@t3tools/client-runtime/source-control-clients"; -import type { CodeViewItem, DiffLineAnnotation, FileDiffMetadata, SelectedLineRange } from "@pierre/diffs"; +import type { + CodeViewItem, + DiffLineAnnotation, + FileDiffMetadata, + SelectedLineRange, +} from "@pierre/diffs"; import type { CodeViewAnnotationArgs, CodeViewDiffItem, CodeViewHandle } from "@pierre/diffs/react"; import type { EnvironmentId, @@ -1004,7 +1009,7 @@ function PullRequestCodeTab({ const isCurrentDraft = useStableCallback( (anchor: DraftAnchor) => draft === anchor && - canCommentOnLines && + canSelectLines && isCurrentFile(anchor.fileKey, anchor.contentVersion, anchor.scopeKey), ); const finishSelection = ( From 13ebfda5c8ba1eff154e72612900ad9477012245 Mon Sep 17 00:00:00 2001 From: Jake Leventhal Date: Tue, 27 Oct 2026 15:11:24 -0400 Subject: [PATCH 09/17] fix: preserve feature behavior after upstream rebase --- apps/web/src/components/diffs/useReviewFilePatches.ts | 2 +- apps/web/src/components/pullRequest/PullRequestCodeTab.tsx | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/apps/web/src/components/diffs/useReviewFilePatches.ts b/apps/web/src/components/diffs/useReviewFilePatches.ts index e926e8bfacd22..28bcb4e951e1f 100644 --- a/apps/web/src/components/diffs/useReviewFilePatches.ts +++ b/apps/web/src/components/diffs/useReviewFilePatches.ts @@ -7,7 +7,7 @@ import type { } from "@t3tools/contracts"; import * as AsyncResult from "effect/reactivity/AsyncResult"; import * as Atom from "effect/reactivity/Atom"; -import { useContext, useEffect, useRef, useState } from "react"; +import { useContext, useState } from "react"; import { useStableCallback } from "~/hooks/useStableCallback"; import { getRenderablePatch, diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index c2242e4d067cd..549d6ba84732e 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -13,7 +13,7 @@ import type { FileDiffMetadata, SelectedLineRange, } from "@pierre/diffs"; -import type { CodeViewAnnotationArgs, CodeViewDiffItem, CodeViewHandle } from "@pierre/diffs/react"; +import type { CodeViewAnnotationArgs, CodeViewDiffItem } from "@pierre/diffs/react"; import type { EnvironmentId, PullRequestDetailView, @@ -439,9 +439,6 @@ function PullRequestCodeTab({ readonly slices: ReadonlyArray; readonly refreshing: ReadonlyArray | null; }>({ key: "", cursor: null, slices: NO_SLICES, refreshing: null }); - const [viewer, setViewer] = useState | null>( - null, - ); const referenceKey = pullRequestReviewKey(reference); // A commit opened from the timeline or left selected across a force-push may no longer be From 37463ad28deb8ee83a6c07963908d537f81d9dd4 Mon Sep 17 00:00:00 2001 From: Jake Leventhal Date: Wed, 28 Oct 2026 09:00:19 -0400 Subject: [PATCH 10/17] fix: preserve feature behavior on current upstream --- apps/web/src/components/DiffPanel.tsx | 4 ++-- apps/web/src/components/pullRequest/PullRequestCodeTab.tsx | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index 2830e7dee4d13..94a0167c2a6aa 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -479,8 +479,8 @@ export default function DiffPanel({ ? toolCallPreview?.patch : selectedTurn ? activeCheckpointDiff.error === null - ? activeCheckpointDiff.data?.diff - : undefined + ? activeCheckpointDiff.data?.diff + : undefined : gitDiff; const isSelectedPatchTruncated = toolCallPreview?.truncated === true || diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 549d6ba84732e..210f2e0de945c 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -613,6 +613,7 @@ function PullRequestCodeTab({ registry, refreshFilesViewed, ]); + // oxlint-disable-next-line t3code/no-unreasoned-state-sync -- invalidates the next cached environment diff page before advancing its refresh walk useEffect(() => { const last = sliceState.refreshing?.at(-1); if ( From 39437d385b69333266681fdd5c612f14ed6d1f83 Mon Sep 17 00:00:00 2001 From: Jake Leventhal Date: Wed, 28 Oct 2026 10:23:45 -0400 Subject: [PATCH 11/17] fix(web): invalidate local diff drafts after content changes --- .../AnnotatableCodeView.browser.test.tsx | 15 +-- .../diffs/AnnotatableCodeView.logic.test.ts | 63 +++++++++ .../diffs/AnnotatableCodeView.logic.ts | 30 +++++ .../components/diffs/AnnotatableCodeView.tsx | 124 +++++++++++------- 4 files changed, 172 insertions(+), 60 deletions(-) create mode 100644 apps/web/src/components/diffs/AnnotatableCodeView.logic.test.ts create mode 100644 apps/web/src/components/diffs/AnnotatableCodeView.logic.ts diff --git a/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx b/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx index 3ae6c1d524651..e514b7ab2ec90 100644 --- a/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx +++ b/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx @@ -6,7 +6,7 @@ import { describe, expect, it, vi } from "vite-plus/test"; import { act, render } from "../../../test/render"; import { DraftId, useComposerDraftStore } from "~/composerDraftStore"; -import { buildFileDiffIdentityKey } from "~/lib/diffRendering"; +import { buildFileDiffContentVersion, buildFileDiffIdentityKey } from "~/lib/diffRendering"; type ViewerProps = Omit>, "options"> & { options: { @@ -54,12 +54,7 @@ const target = DraftId.make("diff-draft-test"); const range: SelectedLineRange = { start: 1, end: 1, side: "additions" }; let container: HTMLDivElement; -function file( - name: string, - fileVersion = 1, - type: FileDiffMetadata["type"] = "change", - prevName?: string, -) { +function file(name: string, type: FileDiffMetadata["type"] = "change", prevName?: string) { const fileDiff = parsePatchFiles( `diff --git a/${name} b/${name}\n--- a/${name}\n+++ b/${name}\n@@ -1 +1 @@\n-old\n+new\n`, { cacheKeyPrefix: name }, @@ -70,7 +65,7 @@ function file( fileDiff, filePath: name, fileKey: buildFileDiffIdentityKey(fileDiff), - fileVersion, + fileVersion: buildFileDiffContentVersion(fileDiff), collapsed: false, }; } @@ -132,11 +127,11 @@ function setup(): Disposable { describe("diff comment drafts", () => { it("keeps the text and selected line through refresh and a provisional lazy patch", async () => { using _drafts = setup(); - const original = file("shared.ts", 1, "rename-pure", "old.ts"); + const original = file("shared.ts", "rename-pure", "old.ts"); await using screen = await render(view([original]), { container }); await click("Comment on shared.ts"); await typeComment("Keep this draft"); - const refreshed = file("shared.ts", 2, "rename-pure", "old.ts"); + const refreshed = file("shared.ts", "rename-pure", "old.ts"); await screen.rerender(view([refreshed], "refreshed")); expect(editor().value).toBe("Keep this draft"); expect(container.querySelector("mark")?.textContent).toBe("Line 1"); diff --git a/apps/web/src/components/diffs/AnnotatableCodeView.logic.test.ts b/apps/web/src/components/diffs/AnnotatableCodeView.logic.test.ts new file mode 100644 index 0000000000000..1a2739d85a3f6 --- /dev/null +++ b/apps/web/src/components/diffs/AnnotatableCodeView.logic.test.ts @@ -0,0 +1,63 @@ +import { parsePatchFiles } from "@pierre/diffs/utils/parsePatchFiles"; +import { describe, expect, it } from "vite-plus/test"; + +import { buildFileDiffContentVersion, buildFileDiffIdentityKey } from "~/lib/diffRendering"; + +import { isDiffCommentAnchorCurrent } from "./AnnotatableCodeView.logic"; + +function file(contents: string) { + return parsePatchFiles( + `diff --git a/shared.ts b/shared.ts\n--- a/shared.ts\n+++ b/shared.ts\n@@ -1 +1 @@\n-old\n+${contents}\n`, + )[0]!.files[0]!; +} + +describe("diff comment anchors", () => { + it("invalidates old line coordinates when the same file changes", () => { + const original = file("new"); + const refreshed = file("newer"); + const anchor = { + fileKey: buildFileDiffIdentityKey(original), + filePath: "shared.ts", + previousFilePath: "shared.ts", + fileVersion: buildFileDiffContentVersion(original), + }; + expect(buildFileDiffIdentityKey(refreshed)).toBe(anchor.fileKey); + const changedFiles = new Map([ + [ + anchor.fileKey, + { fileVersion: buildFileDiffContentVersion(refreshed), fileDiff: refreshed }, + ], + ]); + expect(isDiffCommentAnchorCurrent(anchor, changedFiles, [])).toBe(false); + expect( + isDiffCommentAnchorCurrent( + anchor, + new Map([[anchor.fileKey, { ...anchor, fileDiff: original }]]), + [], + ), + ).toBe(true); + const pending = { ...original, type: "rename-changed" as const, hunks: [] }; + expect(isDiffCommentAnchorCurrent(anchor, new Map(), [pending])).toBe(true); + expect(isDiffCommentAnchorCurrent(anchor, changedFiles, [pending])).toBe(false); + const sameIdentityPending = { ...original, hunks: [], cacheKey: "shared:pending" }; + expect( + isDiffCommentAnchorCurrent( + anchor, + new Map([ + [ + anchor.fileKey, + { + fileDiff: sameIdentityPending, + fileVersion: buildFileDiffContentVersion(sameIdentityPending), + }, + ], + ]), + [sameIdentityPending], + ), + ).toBe(true); + expect( + isDiffCommentAnchorCurrent(anchor, new Map(), [{ ...pending, prevName: "other.ts" }]), + ).toBe(false); + expect(isDiffCommentAnchorCurrent(anchor, new Map(), [])).toBe(false); + }); +}); diff --git a/apps/web/src/components/diffs/AnnotatableCodeView.logic.ts b/apps/web/src/components/diffs/AnnotatableCodeView.logic.ts new file mode 100644 index 0000000000000..21a144abe27dc --- /dev/null +++ b/apps/web/src/components/diffs/AnnotatableCodeView.logic.ts @@ -0,0 +1,30 @@ +import type { FileDiffMetadata } from "@pierre/diffs"; + +import { resolveFileDiffPath, resolveFileDiffPreviousPath } from "~/lib/diffRendering"; + +export interface DiffCommentAnchor { + readonly fileKey: string; + readonly filePath: string; + readonly previousFilePath: string; + readonly fileVersion: number; +} + +export function isDiffCommentAnchorCurrent( + anchor: DiffCommentAnchor, + filesByKey: ReadonlyMap< + string, + { readonly fileVersion: number; readonly fileDiff: FileDiffMetadata } + >, + pendingFiles: ReadonlyArray, +): boolean { + const file = filesByKey.get(anchor.fileKey); + if (file !== undefined && !file.fileDiff.cacheKey?.endsWith(":pending")) { + return file.fileVersion === anchor.fileVersion; + } + // Lazy placeholders have provisional change types, so match both paths instead. + return pendingFiles.some( + (file) => + resolveFileDiffPath(file) === anchor.filePath && + resolveFileDiffPreviousPath(file) === anchor.previousFilePath, + ); +} diff --git a/apps/web/src/components/diffs/AnnotatableCodeView.tsx b/apps/web/src/components/diffs/AnnotatableCodeView.tsx index f5a0da4b1a903..fddb0f33fb11b 100644 --- a/apps/web/src/components/diffs/AnnotatableCodeView.tsx +++ b/apps/web/src/components/diffs/AnnotatableCodeView.tsx @@ -11,7 +11,13 @@ import type { EnvironmentId, ProjectId, ScopedThreadRef } from "@t3tools/contrac import { useState, type ReactNode, type Ref } from "react"; import { type DraftId, useComposerDraftStore } from "~/composerDraftStore"; -import { fnv1a32, resolveFileDiffPath, resolveFileDiffPreviousPath } from "~/lib/diffRendering"; +import { + buildFileDiffContentVersion, + fnv1a32, + resolveFileDiffPath, + resolveFileDiffPreviousPath, +} from "~/lib/diffRendering"; +import { useStableCallback } from "~/hooks/useStableCallback"; import { buildDiffReviewComment, restoreDiffReviewCommentRange, @@ -20,6 +26,7 @@ import { import { nextFileCommentId } from "../files/fileCommentAnnotations"; import { DiffCommentAnnotation } from "./DiffCommentAnnotation"; +import { isDiffCommentAnchorCurrent, type DiffCommentAnchor } from "./AnnotatableCodeView.logic"; import { StyledDiffCodeView, type StyledDiffCodeViewOptions } from "./StyledDiffCodeView"; interface DiffCommentAnnotationEntry { @@ -134,28 +141,25 @@ export function AnnotatableCodeView({ const [selectedLines, setSelectedLines] = useState<{ id: string; range: SelectedLineRange; + anchor: DiffCommentAnchor; } | null>(null); - const [draft, setDraft] = useState<{ - fileKey: string; - filePath: string; - previousFilePath: string; - annotation: DiffCommentLineAnnotation; - } | null>(null); + const [draft, setDraft] = useState< + | (DiffCommentAnchor & { + annotation: DiffCommentLineAnnotation; + }) + | null + >(null); const [draftText, setDraftText] = useState(""); const filesByKey = new Map(files.map((file) => [file.fileKey, file])); - if ( - draft !== null && - !filesByKey.has(draft.fileKey) && - // Lazy placeholders have provisional change types, so match both paths instead. - !pendingFiles.some( - (file) => - resolveFileDiffPath(file) === draft.filePath && - resolveFileDiffPreviousPath(file) === draft.previousFilePath, - ) - ) { + if (draft !== null && !isDiffCommentAnchorCurrent(draft, filesByKey, pendingFiles)) { setDraft(null); setDraftText(""); + } + if ( + selectedLines !== null && + !isDiffCommentAnchorCurrent(selectedLines.anchor, filesByKey, pendingFiles) + ) { setSelectedLines(null); } const items: CodeViewDiffItem[] = files.map( @@ -208,10 +212,10 @@ export function AnnotatableCodeView({ } }; - const submitEntry = (entryId: string, text: string) => { + const submitEntry = useStableCallback((entryId: string, text: string) => { const entry = draft?.annotation.metadata.entries.find((candidate) => candidate.id === entryId); const file = draft ? filesByKey.get(draft.fileKey) : undefined; - if (!entry || !file) return; + if (!entry || !file || draft?.fileVersion !== file.fileVersion) return; const comment = buildDiffReviewComment({ id: entry.id, sectionId, @@ -225,38 +229,43 @@ export function AnnotatableCodeView({ setSelectedLines(null); setDraft(null); setDraftText(""); - }; + }); - const beginComment = (range: SelectedLineRange | null, context: DiffSelectionContext) => { - if (!range) return; - const item = context.item; - if (item.type !== "diff") return; - const id = nextFileCommentId(); - const filePath = resolveFileDiffPath(item.fileDiff); - const comment = buildDiffReviewComment({ - id, - sectionId, - sectionTitle, - filePath, - fileDiff: item.fileDiff, - range, - text: "", - }); - if (!comment) return; - setDraftText(""); - setDraft({ - fileKey: item.id, - filePath, - previousFilePath: resolveFileDiffPreviousPath(item.fileDiff), - annotation: { - side: annotationSide(range), - lineNumber: range.end, - metadata: { - entries: [{ id, kind: "draft", range, rangeLabel: comment.rangeLabel, text: "" }], + const beginComment = useStableCallback( + (range: SelectedLineRange | null, context: DiffSelectionContext) => { + if (!range) return; + const item = context.item; + if (item.type !== "diff") return; + const file = filesByKey.get(item.id); + if (!file || file.fileVersion !== buildFileDiffContentVersion(item.fileDiff)) return; + const id = nextFileCommentId(); + const filePath = resolveFileDiffPath(item.fileDiff); + const comment = buildDiffReviewComment({ + id, + sectionId, + sectionTitle, + filePath, + fileDiff: item.fileDiff, + range, + text: "", + }); + if (!comment) return; + setDraftText(""); + setDraft({ + fileKey: item.id, + fileVersion: file.fileVersion, + filePath, + previousFilePath: resolveFileDiffPreviousPath(item.fileDiff), + annotation: { + side: annotationSide(range), + lineNumber: range.end, + metadata: { + entries: [{ id, kind: "draft", range, rangeLabel: comment.rangeLabel, text: "" }], + }, }, - }, - }); - }; + }); + }, + ); return ( @@ -274,7 +283,22 @@ export function AnnotatableCodeView({ {...(renderCodeViewFooter ? { renderCodeViewFooter } : {})} items={items} selectedLines={selectedLines} - onSelectedLinesChange={setSelectedLines} + onSelectedLinesChange={(selection) => { + const file = selection === null ? undefined : filesByKey.get(selection.id); + setSelectedLines( + selection !== null && file !== undefined + ? { + ...selection, + anchor: { + fileKey: file.fileKey, + fileVersion: file.fileVersion, + filePath: file.filePath, + previousFilePath: resolveFileDiffPreviousPath(file.fileDiff), + }, + } + : null, + ); + }} options={{ ...options, enableGutterUtility: draft === null, From cc6c55e39a66f585eea84caebbbbad2d965fd2b6 Mon Sep 17 00:00:00 2001 From: Jake Leventhal Date: Wed, 28 Oct 2026 10:58:46 -0400 Subject: [PATCH 12/17] fix(web): guard diff comments until refreshed content renders --- apps/web/src/components/DiffPanel.tsx | 1 + .../AnnotatableCodeView.browser.test.tsx | 275 ++++++++++++------ .../components/diffs/AnnotatableCodeView.tsx | 49 +++- 3 files changed, 219 insertions(+), 106 deletions(-) diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index 94a0167c2a6aa..9267dc26b5dba 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -1458,6 +1458,7 @@ export default function DiffPanel({ className="h-full min-h-0 overflow-auto" files={codeViewFiles} pendingFiles={pendingFiles} + isRefreshingDiff={isRefreshingDiff} renderCodeViewFooter={renderLoadingBoundary} sectionId={reviewSectionId} sectionTitle={reviewSectionTitle} diff --git a/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx b/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx index e514b7ab2ec90..05e386444b798 100644 --- a/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx +++ b/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx @@ -1,6 +1,7 @@ -import type { CodeViewDiffItem, FileDiffMetadata, SelectedLineRange } from "@pierre/diffs"; +import type { FileDiffMetadata, SelectedLineRange } from "@pierre/diffs"; import { parsePatchFiles } from "@pierre/diffs/utils/parsePatchFiles"; -import { StrictMode, type ComponentProps } from "react"; +import { WorkerPoolManager } from "@pierre/diffs/worker"; +import { createRef, StrictMode } from "react"; import { describe, expect, it, vi } from "vite-plus/test"; import { act, render } from "../../../test/render"; @@ -8,56 +9,20 @@ import { act, render } from "../../../test/render"; import { DraftId, useComposerDraftStore } from "~/composerDraftStore"; import { buildFileDiffContentVersion, buildFileDiffIdentityKey } from "~/lib/diffRendering"; -type ViewerProps = Omit>, "options"> & { - options: { - enableLineSelection: boolean; - enableGutterUtility: boolean; - onGutterUtilityClick: ( - range: SelectedLineRange, - context: { item: CodeViewDiffItem }, - ) => void; - }; -}; - -// Replace only the virtualized viewer; comment editing and submission use the real components. -vi.mock("./StyledDiffCodeView", () => ({ - StyledDiffCodeView: (props: ViewerProps) => - props.items - ?.filter((item) => item.type === "diff") - .map((item) => ( -
- - {props.selectedLines?.id === item.id ? ( - Line {props.selectedLines.range.start} - ) : null} - {item.annotations?.map((annotation) => ( -
- {props.renderAnnotation?.(annotation, item.fileDiff, item)} -
- ))} -
- )), -})); - -import { AnnotatableCodeView } from "./AnnotatableCodeView"; -import type { StyledDiffCodeView } from "./StyledDiffCodeView"; +import { AnnotatableCodeView, type AnnotatableCodeViewHandle } from "./AnnotatableCodeView"; const target = DraftId.make("diff-draft-test"); const range: SelectedLineRange = { start: 1, end: 1, side: "additions" }; -let container: HTMLDivElement; -function file(name: string, type: FileDiffMetadata["type"] = "change", prevName?: string) { +function file( + name: string, + type: FileDiffMetadata["type"] = "change", + prevName?: string, + contents = "new", +) { const fileDiff = parsePatchFiles( - `diff --git a/${name} b/${name}\n--- a/${name}\n+++ b/${name}\n@@ -1 +1 @@\n-old\n+new\n`, - { cacheKeyPrefix: name }, + `diff --git a/${name} b/${name}\n--- a/${name}\n+++ b/${name}\n@@ -1 +1,${contents.split("\n").length} @@\n-old\n+${contents}\n`, + { cacheKeyPrefix: `${name}:${contents}` }, )[0]!.files[0]!; fileDiff.type = type; if (prevName) fileDiff.prevName = prevName; @@ -70,17 +35,25 @@ function file(name: string, type: FileDiffMetadata["type"] = "change", prevName? }; } -function view( - files: ReturnType[], - codeViewKey = "initial", - pendingFiles: FileDiffMetadata[] = [], -) { - return ( +function setup() { + const reset = () => useComposerDraftStore.setState(useComposerDraftStore.getInitialState(), true); + reset(); + const container = document.createElement("div"); + Object.assign(container.style, { width: "800px", height: "400px", display: "grid" }); + const viewerRef = createRef(); + const view = ( + files: ReturnType[], + codeViewKey = "initial", + pendingFiles: FileDiffMetadata[] = [], + isRefreshingDiff = false, + ) => ( ); -} - -function click(label: string) { - const button = Array.from(container.querySelectorAll("button")).find( - (node) => node.textContent === label, - ); - if (!button) throw new Error(`Missing button: ${label}`); - return act(() => button.click()); -} - -function editor() { - const textarea = container.querySelector("textarea"); - if (!textarea) throw new Error("Comment editor is not available"); - return textarea; -} - -function typeComment(text: string) { - return act(() => { - Object.getOwnPropertyDescriptor(HTMLTextAreaElement.prototype, "value")!.set!.call( - editor(), - text, + const renderedFile = () => { + const item = viewerRef.current?.getInstance()?.getRenderedItems()[0]; + if (item?.type !== "diff") throw new Error("Diff is not rendered"); + return item; + }; + const select = () => + act(() => { + // Invoke Pierre's own wrappers, which resolve their context from the latest item. + const { instance } = renderedFile(); + instance.options.onLineSelectionChange?.(range); + instance.options.onGutterUtilityClick?.(range); + }); + const editor = () => { + const textarea = container.querySelector("textarea"); + if (!textarea) throw new Error("Comment editor is not available"); + return textarea; + }; + const click = (label: string) => { + const button = Array.from(container.querySelectorAll("button")).find( + (node) => node.textContent === label, ); - editor().dispatchEvent(new Event("input", { bubbles: true })); - }); -} - -function setup(): Disposable { - const reset = () => useComposerDraftStore.setState(useComposerDraftStore.getInitialState(), true); - reset(); - container = document.createElement("div"); - return { [Symbol.dispose]: reset }; + if (!button) throw new Error(`Missing button: ${label}`); + return act(() => button.click()); + }; + const typeComment = (text: string) => + act(() => { + Object.getOwnPropertyDescriptor(HTMLTextAreaElement.prototype, "value")!.set!.call( + editor(), + text, + ); + editor().dispatchEvent(new Event("input", { bubbles: true })); + }); + return { + container, + viewerRef, + view, + renderedFile, + select, + editor, + click, + typeComment, + [Symbol.dispose]: reset, + }; } describe("diff comment drafts", () => { it("keeps the text and selected line through refresh and a provisional lazy patch", async () => { - using _drafts = setup(); + using test = setup(); const original = file("shared.ts", "rename-pure", "old.ts"); - await using screen = await render(view([original]), { container }); - await click("Comment on shared.ts"); - await typeComment("Keep this draft"); + await using screen = await render(test.view([original]), { container: test.container }); + await vi.waitFor(() => expect(test.renderedFile().instance.getLineIndex(1)).toBeDefined()); + await test.select(); + await vi.waitFor(() => expect(test.editor().value).toBe("")); + await test.typeComment("Keep this draft"); const refreshed = file("shared.ts", "rename-pure", "old.ts"); - await screen.rerender(view([refreshed], "refreshed")); - expect(editor().value).toBe("Keep this draft"); - expect(container.querySelector("mark")?.textContent).toBe("Line 1"); + await screen.rerender(test.view([refreshed], "refreshed", [], true)); + await vi.waitFor(() => expect(test.editor().value).toBe("Keep this draft")); + expect(test.viewerRef.current?.getSelectedLines()).toEqual( + expect.objectContaining({ id: original.fileKey, range }), + ); + await test.click("Comment"); + expect(useComposerDraftStore.getState().getComposerDraft(target)?.reviewComments ?? []).toEqual( + [], + ); + await act(() => { + test + .editor() + .dispatchEvent( + new KeyboardEvent("keydown", { key: "Enter", metaKey: true, bubbles: true }), + ); + }); + expect(useComposerDraftStore.getState().getComposerDraft(target)?.reviewComments ?? []).toEqual( + [], + ); + expect(test.editor().value).toBe("Keep this draft"); await screen.rerender( - view([], "loading", [ + test.view([], "loading", [ { ...refreshed.fileDiff, type: "rename-changed", hunks: [], cacheKey: "shared:pending" }, ]), ); - expect(container.querySelector("textarea")).toBeNull(); - await screen.rerender(view([refreshed], "loaded")); - expect(editor().value).toBe("Keep this draft"); - expect(container.querySelector("mark")?.textContent).toBe("Line 1"); - await click("Comment"); + expect(test.container.querySelector("textarea")).toBeNull(); + await screen.rerender(test.view([refreshed], "loaded")); + await vi.waitFor(() => expect(test.editor().value).toBe("Keep this draft")); + expect(test.viewerRef.current?.getSelectedLines()).toEqual( + expect.objectContaining({ id: original.fileKey, range }), + ); + await test.click("Comment"); expect(useComposerDraftStore.getState().getComposerDraft(target)?.reviewComments).toEqual([ expect.objectContaining({ filePath: "shared.ts", @@ -153,6 +159,81 @@ describe("diff comment drafts", () => { selection: { ...range, endSide: "additions" }, }), ]); - expect(container.querySelector("p")?.textContent).toBe("Keep this draft"); + }); + + it("rejects old line ranges and retained submission until Pierre adopts refreshed content", async () => { + using test = setup(); + const original = file("adoption.ts"); + const refreshed = file("adoption.ts", "change", undefined, "inserted\n+new"); + const highlight = WorkerPoolManager.prototype.highlightDiffAST; + let resumeHighlight: (() => void) | undefined; + let pool: WorkerPoolManager | undefined; + using _highlight = vi + .spyOn(WorkerPoolManager.prototype, "highlightDiffAST") + .mockImplementation(function (this: WorkerPoolManager, instance, diff) { + pool = this; + if (diff === refreshed.fileDiff) { + resumeHighlight = () => highlight.call(this, instance, diff); + return; + } + return highlight.call(this, instance, diff); + }); + await using screen = await render(test.view([original]), { container: test.container }); + await vi.waitFor(() => { + expect(pool?.getDiffResultCache(original.fileDiff)).toBeDefined(); + expect(test.renderedFile().instance.getLineIndex(1)).toBeDefined(); + }); + await act(() => test.viewerRef.current?.getInstance()?.render(true)); + await test.select(); + await vi.waitFor(() => expect(test.editor().value).toBe("")); + await test.typeComment("Old coordinates"); + const retainedSubmit = Array.from(test.container.querySelectorAll("button")).find( + (node) => node.textContent === "Comment", + )!; + const previousInstance = test.renderedFile().instance; + + await screen.rerender(test.view([refreshed])); + await vi.waitFor(() => expect(resumeHighlight).toBeDefined()); + const pending = test.renderedFile(); + expect(pending.item.fileDiff).toBe(refreshed.fileDiff); + expect(pending.instance).toBe(previousInstance); + expect(pending.element.shadowRoot?.textContent).not.toContain("inserted"); + expect(retainedSubmit.isConnected).toBe(true); + expect( + pending.instance.__getNextRenderData({ + fileDiff: refreshed.fileDiff, + lineAnnotations: undefined, + }).fileDiff, + ).toBe(original.fileDiff); + await test.typeComment("Still visible old draft"); + expect(retainedSubmit.disabled).toBe(false); + await act(() => retainedSubmit.click()); + expect(useComposerDraftStore.getState().getComposerDraft(target)?.reviewComments ?? []).toEqual( + [], + ); + await test.select(); + expect(test.viewerRef.current?.getSelectedLines()).toBeNull(); + + await act(() => resumeHighlight?.()); + await vi.waitFor(() => { + expect( + test.renderedFile().instance.__getNextRenderData({ + fileDiff: refreshed.fileDiff, + lineAnnotations: undefined, + }).fileDiff, + ).toBe(refreshed.fileDiff); + expect(test.renderedFile().element.shadowRoot?.textContent).toContain("inserted"); + expect(test.container.querySelector("textarea")).toBeNull(); + }); + await test.select(); + await vi.waitFor(() => expect(test.editor().value).toBe("")); + await test.typeComment("Current coordinates"); + await test.click("Comment"); + expect(useComposerDraftStore.getState().getComposerDraft(target)?.reviewComments).toEqual([ + expect.objectContaining({ + text: "Current coordinates", + diff: expect.stringContaining("+inserted"), + }), + ]); }); }); diff --git a/apps/web/src/components/diffs/AnnotatableCodeView.tsx b/apps/web/src/components/diffs/AnnotatableCodeView.tsx index fddb0f33fb11b..aaac3f3ec2598 100644 --- a/apps/web/src/components/diffs/AnnotatableCodeView.tsx +++ b/apps/web/src/components/diffs/AnnotatableCodeView.tsx @@ -8,13 +8,12 @@ import type { } from "@pierre/diffs"; import type { CodeViewHandle, CodeViewProps } from "@pierre/diffs/react"; import type { EnvironmentId, ProjectId, ScopedThreadRef } from "@t3tools/contracts"; -import { useState, type ReactNode, type Ref } from "react"; +import { useRef, useState, type ReactNode, type Ref } from "react"; import { type DraftId, useComposerDraftStore } from "~/composerDraftStore"; import { buildFileDiffContentVersion, fnv1a32, - resolveFileDiffPath, resolveFileDiffPreviousPath, } from "~/lib/diffRendering"; import { useStableCallback } from "~/hooks/useStableCallback"; @@ -90,6 +89,7 @@ interface AnnotatableCodeViewProps { collapsed: boolean; }>; pendingFiles: ReadonlyArray; + isRefreshingDiff: boolean; sectionId: string; sectionTitle: string; composerDraftTarget: ScopedThreadRef | DraftId; @@ -119,6 +119,7 @@ export function AnnotatableCodeView({ codeViewKey, files, pendingFiles, + isRefreshingDiff, sectionId, sectionTitle, composerDraftTarget, @@ -150,8 +151,19 @@ export function AnnotatableCodeView({ | null >(null); const [draftText, setDraftText] = useState(""); + const renderedFiles = useRef(new Map()); const filesByKey = new Map(files.map((file) => [file.fileKey, file])); + const isCurrentFileRendered = (fileKey: string) => { + const file = filesByKey.get(fileKey); + const rendered = renderedFiles.current.get(fileKey); + return ( + !isRefreshingDiff && + file !== undefined && + rendered !== undefined && + (file.fileDiff === rendered || file.fileVersion === buildFileDiffContentVersion(rendered)) + ); + }; if (draft !== null && !isDiffCommentAnchorCurrent(draft, filesByKey, pendingFiles)) { setDraft(null); setDraftText(""); @@ -215,7 +227,7 @@ export function AnnotatableCodeView({ const submitEntry = useStableCallback((entryId: string, text: string) => { const entry = draft?.annotation.metadata.entries.find((candidate) => candidate.id === entryId); const file = draft ? filesByKey.get(draft.fileKey) : undefined; - if (!entry || !file || draft?.fileVersion !== file.fileVersion) return; + if (isRefreshingDiff || !entry || !file || draft?.fileVersion !== file.fileVersion) return; const comment = buildDiffReviewComment({ id: entry.id, sectionId, @@ -237,15 +249,15 @@ export function AnnotatableCodeView({ const item = context.item; if (item.type !== "diff") return; const file = filesByKey.get(item.id); - if (!file || file.fileVersion !== buildFileDiffContentVersion(item.fileDiff)) return; + if (!file || !isCurrentFileRendered(item.id)) return; const id = nextFileCommentId(); - const filePath = resolveFileDiffPath(item.fileDiff); + const filePath = file.filePath; const comment = buildDiffReviewComment({ id, sectionId, sectionTitle, filePath, - fileDiff: item.fileDiff, + fileDiff: file.fileDiff, range, text: "", }); @@ -255,7 +267,7 @@ export function AnnotatableCodeView({ fileKey: item.id, fileVersion: file.fileVersion, filePath, - previousFilePath: resolveFileDiffPreviousPath(item.fileDiff), + previousFilePath: resolveFileDiffPreviousPath(file.fileDiff), annotation: { side: annotationSide(range), lineNumber: range.end, @@ -284,6 +296,7 @@ export function AnnotatableCodeView({ items={items} selectedLines={selectedLines} onSelectedLinesChange={(selection) => { + if (selection !== null && !isCurrentFileRendered(selection.id)) return; const file = selection === null ? undefined : filesByKey.get(selection.id); setSelectedLines( selection !== null && file !== undefined @@ -301,10 +314,27 @@ export function AnnotatableCodeView({ }} options={{ ...options, - enableGutterUtility: draft === null, - enableLineSelection: draft === null, + enableGutterUtility: draft === null && !isRefreshingDiff, + enableLineSelection: draft === null && !isRefreshingDiff, onGutterUtilityClick: beginComment, onRevealSearchMatch: (item) => onRevealSearchMatch(item.id), + onPostRender: (node, _instance, phase, context) => { + if (context.type === "diff") { + if (phase === "unmount") { + renderedFiles.current.delete(context.item.id); + } else { + // At post-render this is the data Pierre just adopted, including retained rows. + const { fileDiff } = context.instance.__getNextRenderData({ + fileDiff: context.item.fileDiff, + lineAnnotations: undefined, + }); + renderedFiles.current.set(context.item.id, fileDiff); + } + options.onPostRender?.(node, context.instance, phase, context); + } else { + options.onPostRender?.(node, context.instance, phase, context); + } + }, }} renderHeaderFilenameSuffix={(item) => item.type === "diff" ? renderHeaderFilenameSuffix(item.fileDiff) : null @@ -337,6 +367,7 @@ export function AnnotatableCodeView({ onTextChange={setDraftText} onCancel={() => removeEntry(entry.id)} onComment={(text) => submitEntry(entry.id, text)} + pending={isRefreshingDiff} onDelete={() => removeEntry(entry.id)} /> ))} From a4fda5fd5d2e4d46c62a9b5ef3a6b214fc6bdc19 Mon Sep 17 00:00:00 2001 From: Jake Leventhal Date: Wed, 28 Oct 2026 11:17:28 -0400 Subject: [PATCH 13/17] test(web): await diff viewer readiness in comment regressions --- .../diffs/AnnotatableCodeView.browser.test.tsx | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx b/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx index 05e386444b798..e9e1a25ad958c 100644 --- a/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx +++ b/apps/web/src/components/diffs/AnnotatableCodeView.browser.test.tsx @@ -50,6 +50,7 @@ function setup() { { using test = setup(); const original = file("shared.ts", "rename-pure", "old.ts"); await using screen = await render(test.view([original]), { container: test.container }); - await vi.waitFor(() => expect(test.renderedFile().instance.getLineIndex(1)).toBeDefined()); + await expect.element(screen.getByText("new", { exact: true })).toBeVisible(); + expect(test.renderedFile().instance.getLineIndex(1)).toBeDefined(); await test.select(); await vi.waitFor(() => expect(test.editor().value).toBe("")); await test.typeComment("Keep this draft"); @@ -179,10 +181,12 @@ describe("diff comment drafts", () => { return highlight.call(this, instance, diff); }); await using screen = await render(test.view([original]), { container: test.container }); - await vi.waitFor(() => { - expect(pool?.getDiffResultCache(original.fileDiff)).toBeDefined(); - expect(test.renderedFile().instance.getLineIndex(1)).toBeDefined(); - }); + await expect.element(screen.getByText("new", { exact: true })).toBeVisible(); + const initialPool = pool; + if (!initialPool) throw new Error("Diff worker pool is unavailable"); + await act(() => initialPool.primeDiffHighlightCache(original.fileDiff)); + expect(initialPool.getDiffResultCache(original.fileDiff)).toBeDefined(); + expect(test.renderedFile().instance.getLineIndex(1)).toBeDefined(); await act(() => test.viewerRef.current?.getInstance()?.render(true)); await test.select(); await vi.waitFor(() => expect(test.editor().value).toBe("")); From 919cedb85a5fdfd9f40f80ab12c0cc54dfc6166d Mon Sep 17 00:00:00 2001 From: Jake Leventhal Date: Thu, 12 Nov 2026 09:31:43 -0500 Subject: [PATCH 14/17] fix(web): preserve current diff navigation and multi-block refreshes --- .../web/src/components/ChatView.logic.test.ts | 73 ++------------ apps/web/src/components/ChatView.logic.ts | 27 ------ .../useReviewFilePatches.browser.test.tsx | 17 +++- .../components/diffs/useReviewFilePatches.ts | 95 +++++++++++-------- .../pullRequest/PullRequestCodeTab.tsx | 71 +++++++++++--- apps/web/src/lib/diffRendering.test.ts | 11 ++- apps/web/src/lib/diffRendering.ts | 2 +- 7 files changed, 142 insertions(+), 154 deletions(-) diff --git a/apps/web/src/components/ChatView.logic.test.ts b/apps/web/src/components/ChatView.logic.test.ts index 7f1ab534f9780..dafb9a5f1788f 100644 --- a/apps/web/src/components/ChatView.logic.test.ts +++ b/apps/web/src/components/ChatView.logic.test.ts @@ -1,5 +1,4 @@ import { shouldShowOpenInPicker } from "./chat/OpenInPicker.logic"; -import { scopeThreadRef } from "@t3tools/client-runtime/environment"; import { boundAttachmentIds, buildExpiredTerminalContextToastCopy, @@ -53,7 +52,6 @@ import { resolveThreadSwitchTimeline, timelineHasEphemeralPreviewUrls, shouldApplyProactiveChangesScope, - shouldResetDiffSelectionToChanges, createLocalDispatchSnapshot, deriveCommittedServerUserMessageIds, deriveComposerSendState, @@ -412,69 +410,14 @@ describe("deriveCommittedServerUserMessageIds", () => { }); describe("diff selection across agent turns", () => { - const threadRef = scopeThreadRef(EnvironmentId.make("env-1"), ThreadId.make("thread-1")); - const otherThreadRef = scopeThreadRef(EnvironmentId.make("env-1"), ThreadId.make("thread-2")); - const openThread = { threadKey: "env-1:thread-1", diffOpen: true }; - - it("keeps the active review when an agent updates or completes a turn", () => { - expect( - shouldResetDiffSelectionToChanges({ - diffOpen: true, - activeThreadRef: threadRef, - explicitThreadRef: null, - previous: openThread, - threadKey: "env-1:thread-1", - }), - ).toBe(false); - expect(shouldApplyProactiveChangesScope({ openedDiff: true, alreadyReviewingDiff: true })).toBe( - false, - ); - }); - - it("shows Changes when the diff opens or the thread changes", () => { - expect( - shouldResetDiffSelectionToChanges({ - diffOpen: true, - activeThreadRef: threadRef, - explicitThreadRef: null, - previous: { threadKey: "env-1:thread-1", diffOpen: false }, - threadKey: "env-1:thread-1", - }), - ).toBe(true); - expect( - shouldResetDiffSelectionToChanges({ - diffOpen: true, - activeThreadRef: otherThreadRef, - explicitThreadRef: null, - previous: openThread, - threadKey: "env-1:thread-2", - }), - ).toBe(true); - expect( - shouldResetDiffSelectionToChanges({ - diffOpen: true, - activeThreadRef: threadRef, - explicitThreadRef: null, - previous: null, - threadKey: "env-1:thread-1", - }), - ).toBe(true); - expect( - shouldApplyProactiveChangesScope({ openedDiff: true, alreadyReviewingDiff: false }), - ).toBe(true); - }); - - it("leaves a timeline turn selection in place", () => { - expect( - shouldResetDiffSelectionToChanges({ - diffOpen: true, - activeThreadRef: scopeThreadRef(EnvironmentId.make("env-1"), ThreadId.make("thread-1")), - explicitThreadRef: scopeThreadRef(EnvironmentId.make("env-1"), ThreadId.make("thread-1")), - previous: { threadKey: "env-1:thread-1", diffOpen: false }, - threadKey: "env-1:thread-1", - }), - ).toBe(false); - }); + it.each([false, true])( + "only selects Changes when opening a new review (already open: %s)", + (alreadyReviewingDiff) => { + expect(shouldApplyProactiveChangesScope({ openedDiff: true, alreadyReviewingDiff })).toBe( + !alreadyReviewingDiff, + ); + }, + ); }); describe("resolveThreadSwitchTimeline", () => { diff --git a/apps/web/src/components/ChatView.logic.ts b/apps/web/src/components/ChatView.logic.ts index 6775878538623..cd59345deba10 100644 --- a/apps/web/src/components/ChatView.logic.ts +++ b/apps/web/src/components/ChatView.logic.ts @@ -199,33 +199,6 @@ export function shouldRetargetThreadPullRequestPanel( ); } -export interface DiffPanelOpenContext { - readonly threadKey: string | null; - readonly diffOpen: boolean; -} - -export function shouldResetDiffSelectionToChanges(input: { - readonly diffOpen: boolean; - readonly activeThreadRef: ScopedThreadRef | null; - readonly explicitThreadRef: ScopedThreadRef | null; - readonly previous: DiffPanelOpenContext | null; - readonly threadKey: string | null; -}): boolean { - if (!input.diffOpen || input.activeThreadRef === null) return false; - if ( - input.explicitThreadRef !== null && - input.explicitThreadRef.environmentId === input.activeThreadRef.environmentId && - input.explicitThreadRef.threadId === input.activeThreadRef.threadId - ) { - return false; - } - return !( - input.previous !== null && - input.previous.diffOpen && - input.previous.threadKey === input.threadKey - ); -} - export function shouldApplyProactiveChangesScope(input: { readonly openedDiff: boolean; readonly alreadyReviewingDiff: boolean; diff --git a/apps/web/src/components/diffs/useReviewFilePatches.browser.test.tsx b/apps/web/src/components/diffs/useReviewFilePatches.browser.test.tsx index 46edb4186a744..88ae6c23eb49e 100644 --- a/apps/web/src/components/diffs/useReviewFilePatches.browser.test.tsx +++ b/apps/web/src/components/diffs/useReviewFilePatches.browser.test.tsx @@ -46,6 +46,7 @@ function source(paths: string[], hash: string): ReviewDiffPreviewSource { function Probe({ source }: { source: ReviewDiffPreviewSource }) { const files = useReviewFilePatches({ + reviewKey: "review", environmentId: EnvironmentId.make("environment"), cwd: "/project", source, @@ -108,14 +109,17 @@ it("retains automatic and explicitly opened files through metadata insertion, re ); await expect.element(screen.getByText("e.txt:loaded")).toBeVisible(); await expect.element(screen.getByText("f.txt:loaded")).toBeVisible(); + const refreshedAtom = [...state.patches] + .filter(([key]) => JSON.parse(key)[1] === "f.txt") + .at(-1)![1]; await act(() => registry.set( - atom, + refreshedAtom, AsyncResult.fail(new Error("refresh failed"), { previousSuccess: Option.some(loaded) }), ), ); - await expect.element(screen.getByText("f.txt:loaded")).toBeVisible(); - await act(() => registry.set(atom, loaded)); + await expect.element(screen.getByText("f.txt:pending")).toBeVisible(); + await act(() => registry.set(refreshedAtom, loaded)); await expect.element(screen.getByText("f.txt:loaded")).toBeVisible(); }); @@ -142,4 +146,11 @@ it("preserves deletion and addition patches for a file replaced by a symlink", a await expect.element(screen.getByText("deleted", { exact: true })).toBeVisible(); await expect.element(screen.getByText("new", { exact: true })).toBeVisible(); await expect.element(screen.getByText("Pending files: 0")).toBeVisible(); + await screen.rerender( + + + , + ); + await expect.element(screen.getByText("deleted", { exact: true })).toBeVisible(); + await expect.element(screen.getByText("new", { exact: true })).toBeVisible(); }); diff --git a/apps/web/src/components/diffs/useReviewFilePatches.ts b/apps/web/src/components/diffs/useReviewFilePatches.ts index 28bcb4e951e1f..f6ce8604bf082 100644 --- a/apps/web/src/components/diffs/useReviewFilePatches.ts +++ b/apps/web/src/components/diffs/useReviewFilePatches.ts @@ -61,7 +61,8 @@ export function useReviewFilePatches({ ignoreWhitespace, ]); const scope = JSON.stringify([family, source?.diffHash]); - const lazy = source?.truncated === true && source.files !== undefined; + const lazy = + (source?.truncated === true || source?.patchesDeferred === true) && source.files !== undefined; const files = lazy ? sortedReviewFiles(source) : []; const [requested, setRequested] = useState<{ family: string; paths: readonly string[] }>({ family, @@ -70,12 +71,19 @@ export function useReviewFilePatches({ const [retained, setRetained] = useState(() => ({ family, source, - byPath: + byKey: !lazy && preview?.kind === "files" ? retainLoadedReviewFiles(EMPTY_RETAINED_FILES, preview.files) : EMPTY_RETAINED_FILES, })); - const previousByPath = retained.family === family ? retained.byPath : EMPTY_RETAINED_FILES; + const previousByKey = retained.family === family ? retained.byKey : EMPTY_RETAINED_FILES; + const previousByPath = new Map(); + for (const file of previousByKey.values()) { + const path = resolveFileDiffPath(file); + const entries = previousByPath.get(path) ?? []; + entries.push(file); + previousByPath.set(path, entries); + } const requestedPaths = new Set([ ...files.slice(0, PAGE_SIZE).map((file) => file.path), ...(requested.family === family ? requested.paths : []), @@ -115,37 +123,42 @@ export function useReviewFilePatches({ : { family, paths: [...paths] }; }); const renderableFiles = lazy - ? files.map((file, index): FileDiffMetadata => { + ? files.flatMap((file, index): FileDiffMetadata[] => { const result = patches.get(index); const loaded = result?._tag === "Success" && result.value.patch?.kind === "files" - ? (result.value.patch.files.find( + ? result.value.patch.files.filter( (candidate) => resolveFileDiffPath(candidate) === file.path, - ) ?? null) - : null; - return retainedReviewFile({ - loaded, - previous: previousByPath.get(file.path), - sameFamily: true, - pending: - result === undefined || - result._tag === "Initial" || - (result._tag === "Success" && result.waiting), - placeholder: { - name: file.path, - ...(file.previousPath ? { prevName: file.previousPath } : {}), - type: file.previousPath ? "rename-changed" : "change", - hunks: [], - additionLines: [], - deletionLines: [], - additionCharacterCount: 0, - deletionCharacterCount: 0, - splitLineCount: 0, - unifiedLineCount: 0, - isPartial: true, - cacheKey: `${scope}:${file.path}:pending`, - }, - }); + ) + : []; + if (loaded.length > 0) return loaded; + const pending = + result === undefined || + result._tag === "Initial" || + (result._tag === "Success" && result.waiting); + const previous = pending ? previousByPath.get(file.path) : undefined; + return (previous ?? [undefined]).map((previous) => + retainedReviewFile({ + loaded: null, + previous, + sameFamily: true, + pending, + placeholder: { + name: file.path, + ...(file.previousPath ? { prevName: file.previousPath } : {}), + type: file.previousPath ? "rename-changed" : "change", + hunks: [], + additionLines: [], + deletionLines: [], + additionCharacterCount: 0, + deletionCharacterCount: 0, + splitLineCount: 0, + unifiedLineCount: 0, + isPartial: true, + cacheKey: `${scope}:${file.path}:pending`, + }, + }), + ); }) : (preview?.kind === "files" ? preview.files : []).toSorted((a, b) => filePathCollator.compare(resolveFileDiffPath(a), resolveFileDiffPath(b)), @@ -153,22 +166,21 @@ export function useReviewFilePatches({ const nextRetained = !source ? EMPTY_RETAINED_FILES : !lazy && retained.family === family && retained.source === source - ? previousByPath - : retainLoadedReviewFiles(previousByPath, renderableFiles); - if ( - retained.family !== family || - retained.source !== source || - retained.byPath !== nextRetained - ) { - setRetained({ family, source, byPath: nextRetained }); + ? previousByKey + : retainLoadedReviewFiles(previousByKey, renderableFiles); + if (retained.family !== family || retained.source !== source || retained.byKey !== nextRetained) { + setRetained({ family, source, byKey: nextRetained }); } + const retainedPaths = new Set( + renderableFiles.filter((file) => !file.cacheKey?.endsWith(":pending")).map(resolveFileDiffPath), + ); const readyFilePaths = new Set( files .filter((_, index) => { const result = patches.get(index); return ( (result !== undefined && result._tag !== "Initial") || - !renderableFiles[index]?.cacheKey?.endsWith(":pending") + retainedPaths.has(files[index]!.path) ); }) .map((file) => file.path), @@ -204,10 +216,10 @@ export function useReviewFilePatches({ fileStates, isPending: [...patches.values()].some((patch) => patch._tag === "Initial" || patch.waiting), retry, - requestFile: (index: number) => requestFiles([index]), + requestFile: (path: string) => requestFiles([files.findIndex((file) => file.path === path)]), readyFilePaths, renderableFiles, - settledFileCount, + remainingFileCount: lazy ? files.length - settledFileCount : 0, loadNextFiles: () => { if (queries.some(({ index }) => index === settledFileCount)) return; requestFiles(Array.from({ length: PAGE_SIZE }, (_, index) => settledFileCount + index)); @@ -252,6 +264,7 @@ function reviewFileQueries({ request: { cwd, ...(baseRef ? { baseRef } : {}), + ...(source.kind === "commit" ? { commitSha: source.id } : {}), ignoreWhitespace, file: { path: file.path, diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 210f2e0de945c..f435fa6d62d8d 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -103,7 +103,7 @@ import { FileExplorerPane } from "../files/FileExplorerPane"; import { useCodeViewFileReveal } from "../diffs/useCodeViewFileReveal"; import { useHideTestFiles } from "../diffs/useHideTestFiles"; import { diffFileTreeEntries, orderDiffFilesLikeTree } from "../diffs/diffFileTree.logic"; -import { StyledDiffCodeView } from "../diffs/StyledDiffCodeView"; +import { StyledDiffCodeView, type StyledDiffCodeViewOptions } from "../diffs/StyledDiffCodeView"; import { Button } from "../ui/button"; import { Checkbox } from "../ui/checkbox"; import { Collapsible, CollapsiblePanel, CollapsibleTrigger } from "../ui/collapsible"; @@ -246,7 +246,7 @@ function diffModel( ? parsed.files.map( (file, index) => [ - buildFileDiffRenderKey(file), + buildFileDiffIdentityKey(file), getDiffLineStat([parsed.sourceFiles[index] ?? file]), ] as const, ) @@ -431,6 +431,7 @@ function PullRequestCodeTab({ } | null>(null); const [draft, setDraft] = useState(null); const [threadPending, setThreadPending] = useState(false); + const renderedFiles = useRef(new Map()); const [orphansOpen, setOrphansOpen] = useState(false); // Scope slices before reset to prevent using a previous pull request's data or cursor. const [sliceState, setSliceState] = useState<{ @@ -694,13 +695,35 @@ function PullRequestCodeTab({ }) : loadedFiles; const hiddenTestFileCount = loadedFiles.length - files.length; + // A stale mark means the file changed since it was viewed; Fold all and Expand all outrank marks. + const foldsAsViewed = (path: string) => + foldOverride === null && filesViewedEnabled && isFileViewed(path) && !isFileViewedStale(path); const contentVersions = new Map( loadedFiles.map((file) => [buildFileDiffIdentityKey(file), fileContentVersion(file)]), ); - const isCurrentFile = (fileKey: string, version: number, anchorScopeKey: string) => - anchorScopeKey === scopeKey && - contentVersions.get(fileKey) === version && - !isFileDiffCollapsed(fileKey, effectiveFoldOverride, toggledFiles); + const isCurrentFileRendered = (fileKey: string) => { + const rendered = renderedFiles.current.get(fileKey); + return ( + !diffQuery.isPending && + sliceState.refreshing === null && + rendered !== undefined && + contentVersions.get(fileKey) === fileContentVersion(rendered) + ); + }; + const isCurrentFile = (fileKey: string, version: number, anchorScopeKey: string) => { + const file = loadedFiles.find((candidate) => buildFileDiffIdentityKey(candidate) === fileKey); + return ( + anchorScopeKey === scopeKey && + contentVersions.get(fileKey) === version && + file !== undefined && + !isFileDiffCollapsed( + fileKey, + effectiveFoldOverride, + foldChoices, + foldsAsViewed(resolveFileDiffPath(file)), + ) + ); + }; // Stable viewer identities preserve scroll, but line coordinates belong to one file version. if (draft !== null && !isCurrentFile(draft.fileKey, draft.contentVersion, draft.scopeKey)) { setDraft(null); @@ -853,9 +876,6 @@ function PullRequestCodeTab({ ); - // A stale mark means the file changed since it was viewed; Fold all and Expand all outrank marks. - const foldsAsViewed = (path: string) => - foldOverride === null && filesViewedEnabled && isFileViewed(path) && !isFileViewedStale(path); const items: CodeViewDiffItem[] = annotatedFiles.map( ({ fileKey, path, fileDiff, annotations, annotationsVersion }) => { const contentVersion = fileContentVersion(fileDiff); @@ -953,7 +973,13 @@ function PullRequestCodeTab({ if (item === undefined) return; // Record the unfold even when already open, so a Viewed mark that loads later cannot fold it. setFoldChoices((current) => withFileDiffFold(current, item.id, false)); - if (range) setSelectedLines({ id: item.id, range }); + if (range) + setSelectedLines({ + id: item.id, + range, + contentVersion: fileContentVersion(item.fileDiff), + scopeKey, + }); codeViewReveal.request(item.id, range); }; @@ -977,6 +1003,7 @@ function PullRequestCodeTab({ const file = files.find((candidate) => buildFileDiffIdentityKey(candidate) === item.id); if ( !file || + !isCurrentFileRendered(item.id) || item.collapsed === true || !isCurrentFile(item.id, fileContentVersion(item.fileDiff), scopeKey) ) @@ -1008,6 +1035,7 @@ function PullRequestCodeTab({ (anchor: DraftAnchor) => draft === anchor && canSelectLines && + isCurrentFileRendered(anchor.fileKey) && isCurrentFile(anchor.fileKey, anchor.contentVersion, anchor.scopeKey), ); const finishSelection = ( @@ -1109,15 +1137,29 @@ function PullRequestCodeTab({ themeType: resolvedTheme, stickyHeaders: true, loadDiffFiles, - enableGutterUtility: canSelectLines && draft === null, - enableLineSelection: canSelectLines && draft === null, + enableGutterUtility: + canSelectLines && draft === null && !diffQuery.isPending && sliceState.refreshing === null, + enableLineSelection: + canSelectLines && draft === null && !diffQuery.isPending && sliceState.refreshing === null, onGutterUtilityClick: beginComment, onLineSelectionEnd: beginComment, - onPostRender: codeViewReveal.flush, + onPostRender: (_node, _instance, phase, context) => { + if (context.type === "diff") { + if (phase === "unmount") renderedFiles.current.delete(context.item.id); + else { + const { fileDiff } = context.instance.__getNextRenderData({ + fileDiff: context.item.fileDiff, + lineAnnotations: undefined, + }); + renderedFiles.current.set(context.item.id, fileDiff); + } + } + codeViewReveal.flush(); + }, // Find may repeat requests before render; only unfold, never toggle. onRevealSearchMatch: (item: CodeViewItem) => setFoldChoices((current) => withFileDiffFold(current, item.id, false)), - }; + } satisfies StyledDiffCodeViewOptions; const runThreadCommand = async ( label: string, @@ -1792,6 +1834,7 @@ function PullRequestCodeTab({ items={items} selectedLines={selectedLines} onSelectedLinesChange={(selection) => { + if (selection !== null && !isCurrentFileRendered(selection.id)) return; const contentVersion = selection === null ? undefined : contentVersions.get(selection.id); setSelectedLines( diff --git a/apps/web/src/lib/diffRendering.test.ts b/apps/web/src/lib/diffRendering.test.ts index 1ea81f441bc73..493ca7f2802c0 100644 --- a/apps/web/src/lib/diffRendering.test.ts +++ b/apps/web/src/lib/diffRendering.test.ts @@ -298,7 +298,7 @@ describe("diff file reconciliation", () => { const previous = retainLoadedReviewFiles(new Map(), [loaded]); const pending = retainedReviewFile({ loaded: null, - previous: previous.get("app.ts"), + previous: previous.get(buildFileDiffIdentityKey(loaded)), placeholder, sameFamily: true, pending: true, @@ -326,8 +326,11 @@ describe("diff file reconciliation", () => { pending: false, }); expect(failed.hunks).toEqual([]); - const retained = retainLoadedReviewFiles(new Map([["app.ts", loaded]]), [failed]); - expect(retained.has("app.ts")).toBe(false); + const retained = retainLoadedReviewFiles( + new Map([[buildFileDiffIdentityKey(loaded), loaded]]), + [failed], + ); + expect(retained.has(buildFileDiffIdentityKey(loaded))).toBe(false); const switched = retainedReviewFile({ loaded: null, @@ -386,6 +389,8 @@ describe("diff file reconciliation", () => { expect(buildFileDiffIdentityKey(deleted)).not.toBe(buildFileDiffIdentityKey(added)); expect(new Set(parsed.files.map(buildFileDiffIdentityKey)).size).toBe(parsed.files.length); + const retained = retainLoadedReviewFiles(new Map(), parsed.files); + expect([...retained.values()]).toEqual([deleted, added]); }); it("keeps identities stable and versions local to the changed file", () => { diff --git a/apps/web/src/lib/diffRendering.ts b/apps/web/src/lib/diffRendering.ts index e2007c46fb696..f91d9e23bad72 100644 --- a/apps/web/src/lib/diffRendering.ts +++ b/apps/web/src/lib/diffRendering.ts @@ -235,7 +235,7 @@ export function retainLoadedReviewFiles( const next = new Map( files .filter((file) => !file.cacheKey?.endsWith(":pending")) - .map((file) => [resolveFileDiffPath(file), file]), + .map((file) => [buildFileDiffIdentityKey(file), file]), ); return next.size === previous.size && [...next].every(([path, file]) => previous.get(path) === file) From 9e9aec4a35ef57badd75555359afaacecb276caa Mon Sep 17 00:00:00 2001 From: Jake Leventhal Date: Thu, 12 Nov 2026 09:36:18 -0500 Subject: [PATCH 15/17] refactor(web): keep proactive review selection guard local --- apps/web/src/components/ChatView.logic.test.ts | 12 ------------ apps/web/src/components/ChatView.logic.ts | 7 ------- apps/web/src/hooks/useProactivePanels.ts | 3 +-- 3 files changed, 1 insertion(+), 21 deletions(-) diff --git a/apps/web/src/components/ChatView.logic.test.ts b/apps/web/src/components/ChatView.logic.test.ts index dafb9a5f1788f..8223e03540405 100644 --- a/apps/web/src/components/ChatView.logic.test.ts +++ b/apps/web/src/components/ChatView.logic.test.ts @@ -51,7 +51,6 @@ import { resetHeldThreadTimeline, resolveThreadSwitchTimeline, timelineHasEphemeralPreviewUrls, - shouldApplyProactiveChangesScope, createLocalDispatchSnapshot, deriveCommittedServerUserMessageIds, deriveComposerSendState, @@ -409,17 +408,6 @@ describe("deriveCommittedServerUserMessageIds", () => { }); }); -describe("diff selection across agent turns", () => { - it.each([false, true])( - "only selects Changes when opening a new review (already open: %s)", - (alreadyReviewingDiff) => { - expect(shouldApplyProactiveChangesScope({ openedDiff: true, alreadyReviewingDiff })).toBe( - !alreadyReviewingDiff, - ); - }, - ); -}); - describe("resolveThreadSwitchTimeline", () => { afterEach(() => { resetHeldThreadTimeline(); diff --git a/apps/web/src/components/ChatView.logic.ts b/apps/web/src/components/ChatView.logic.ts index cd59345deba10..b1d44d95441c5 100644 --- a/apps/web/src/components/ChatView.logic.ts +++ b/apps/web/src/components/ChatView.logic.ts @@ -199,13 +199,6 @@ export function shouldRetargetThreadPullRequestPanel( ); } -export function shouldApplyProactiveChangesScope(input: { - readonly openedDiff: boolean; - readonly alreadyReviewingDiff: boolean; -}): boolean { - return input.openedDiff && !input.alreadyReviewingDiff; -} - export function shouldOpenProactiveTurnDiff(input: { previousRunningTurnId: RunId | null | undefined; runningTurnId: RunId | null; diff --git a/apps/web/src/hooks/useProactivePanels.ts b/apps/web/src/hooks/useProactivePanels.ts index d2b68f101f5f3..5f1a2840abf60 100644 --- a/apps/web/src/hooks/useProactivePanels.ts +++ b/apps/web/src/hooks/useProactivePanels.ts @@ -10,7 +10,6 @@ import { import type { TurnDiffSummary } from "../types"; import { observeProactivePanelUserChoice, - shouldApplyProactiveChangesScope, resolveProactiveTurnDiffAction, shouldOpenProactivePullRequest, shouldOpenProactiveTurnDiff, @@ -187,7 +186,7 @@ export function useProactivePanels(input: { ) { return; } - if (shouldApplyProactiveChangesScope({ openedDiff: true, alreadyReviewingDiff })) { + if (!alreadyReviewingDiff) { useDiffPanelStore.getState().selectGitScope(threadRef, "branch"); } onDiffPanelOpen?.(); From 5700e755010ccb6445382490885e2e43248c1ea9 Mon Sep 17 00:00:00 2001 From: Jake Leventhal Date: Thu, 12 Nov 2026 10:48:01 -0500 Subject: [PATCH 16/17] fix(web): ignore stale pull request refresh cursors --- .../PullRequestCodeTab.browser.test.tsx | 44 ++++++++++++++++--- .../pullRequest/PullRequestCodeTab.tsx | 4 +- 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx index 5b9d0af32a129..1987ff5ea95c0 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx @@ -1,5 +1,7 @@ import { sha256 } from "@noble/hashes/sha2"; import { bytesToHex } from "@noble/hashes/utils"; +import { RegistryContext } from "@effect/atom-react"; +import { AsyncResult, AtomRegistry } from "effect/reactivity"; import { EnvironmentId, ProjectId, @@ -13,7 +15,7 @@ import "~/index.css"; import { __setClientSettingsForTests } from "../../hooks/useSettings"; import { DEFAULT_CLIENT_SETTINGS } from "@t3tools/contracts/settings"; -const { diffAtom, pagedDiffAtom } = await vi.hoisted(async () => { +const { diffAtom, pagedDiffAtom, pageResults, pages } = await vi.hoisted(async () => { const { AsyncResult, Atom } = await import("effect/reactivity"); const filePatch = (path: string) => [ @@ -46,19 +48,23 @@ const { diffAtom, pagedDiffAtom } = await vi.hoisted(async () => { }, "page-2": { patch: filePatch("late.test.ts"), truncated: false, nextCursor: null }, }; + const pageResults = Atom.family((cursor: string) => + Atom.make>(AsyncResult.success(pages[cursor]!)), + ); return { // An environment query whose refetch returns equal content hands back the previous value // itself, so a refresh publishes a new result around the very same page. diffAtom: Atom.make(() => AsyncResult.success(firstPage)), - pagedDiffAtom: Atom.family((cursor: string) => - Atom.make(() => AsyncResult.success(pages[cursor]!)), - ), + pagedDiffAtom: Atom.family((cursor: string) => Atom.make((get) => get(pageResults(cursor)))), + pageResults, + pages, }; }); const mocks = vi.hoisted(() => ({ paged: false, diffCommits: [] as Array, + diffCursors: [] as Array, viewedPaths: null as ReadonlySet | null, viewedListeners: new Set<() => void>(), })); @@ -101,13 +107,14 @@ vi.mock(import("~/state/pullRequests"), async (original) => { ...actual.pullRequestEnvironment, diff: (({ input }: { input: { cursor?: string; commit?: string } }) => { mocks.diffCommits.push(input.commit); + mocks.diffCursors.push(input.cursor); return mocks.paged ? pagedDiffAtom(input.cursor ?? "first") : diffAtom; }) as never, }, }; }); -import { render } from "../../../test/render"; +import { act, render } from "../../../test/render"; import PullRequestCodeTab from "./PullRequestCodeTab"; const detail: PullRequestDetailView = { @@ -167,6 +174,7 @@ function setup({ paged }: { paged: boolean }) { __setClientSettingsForTests(DEFAULT_CLIENT_SETTINGS); mocks.paged = paged; mocks.diffCommits = []; + mocks.diffCursors = []; mocks.viewedPaths = null; mocks.viewedListeners.clear(); } @@ -207,6 +215,32 @@ it("keeps the diff after a refresh that brings back the same page", async () => await expect.element(screen.getByText("1 file", { exact: true })).toBeVisible(); }); +it("restarts a refresh without advancing the cursor of the replaced page walk", async () => { + setup({ paged: true }); + const registry = AtomRegistry.make(); + using _registry = { [Symbol.dispose]: () => registry.dispose() }; + using _firstPage = { [Symbol.dispose]: registry.mount(pageResults("first")) }; + const content = (token: number) => ( + + {tab(token, { path: "late.test.ts", nonce: 1 })} + + ); + await using screen = await render(content(0), { container: pagedContainer() }); + await expect.element(screen.getByText("41 files", { exact: true })).toBeVisible(); + + await act(() => registry.set(pageResults("first"), AsyncResult.initial())); + await screen.rerender(content(1)); + await act(async () => { + registry.set(pageResults("first"), AsyncResult.success(pages.first!)); + await screen.rerender(content(2)); + }); + + await vi.waitFor(() => expect(mocks.diffCursors.at(-1)).toBe("page-2")); + await expect.element(screen.getByText("41 files", { exact: true })).toBeVisible(); + await screen.rerender(content(3)); + await expect.element(screen.getByText("41 files", { exact: true })).toBeVisible(); +}); + // regression: #13279 a Viewed answer that lands after a link reveals a file must not fold it it("keeps a revealed file open when its Viewed mark loads afterwards", async () => { setup({ paged: false }); diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index f435fa6d62d8d..bda256ff17ea0 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -632,7 +632,9 @@ function PullRequestCodeTab({ }), ); // oxlint-disable-next-line react/set-state-in-effect -- Advance only after invalidating the cached next page. - setSliceState((current) => ({ ...current, cursor: last.nextCursor })); + setSliceState((current) => + current.refreshing?.at(-1) === last ? { ...current, cursor: last.nextCursor } : current, + ); }, [ sliceState.refreshing, cursor, From dd220d04270a15e8ad2394c5a43644ab803a920f Mon Sep 17 00:00:00 2001 From: Jake Leventhal Date: Thu, 12 Nov 2026 11:33:38 -0500 Subject: [PATCH 17/17] fix(web): defer diff links until refresh completes --- .../pullRequest/PullRequestCodeTab.browser.test.tsx | 11 ++++++++--- .../src/components/pullRequest/PullRequestCodeTab.tsx | 1 + 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx index 1987ff5ea95c0..33f1b915cb93b 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx @@ -220,9 +220,11 @@ it("restarts a refresh without advancing the cursor of the replaced page walk", const registry = AtomRegistry.make(); using _registry = { [Symbol.dispose]: () => registry.dispose() }; using _firstPage = { [Symbol.dispose]: registry.mount(pageResults("first")) }; - const content = (token: number) => ( + const url = `${detail.url}/files#diff-${bytesToHex(sha256(new TextEncoder().encode("late.test.ts")))}R4-R5`; + const settled = vi.fn(); + const content = (token: number, revealUrl?: string) => ( - {tab(token, { path: "late.test.ts", nonce: 1 })} + {tab(token, { path: "late.test.ts", nonce: 1 }, revealUrl, settled)} ); await using screen = await render(content(0), { container: pagedContainer() }); @@ -230,13 +232,16 @@ it("restarts a refresh without advancing the cursor of the replaced page walk", await act(() => registry.set(pageResults("first"), AsyncResult.initial())); await screen.rerender(content(1)); + await screen.rerender(content(1, url)); + expect(settled).not.toHaveBeenCalled(); await act(async () => { registry.set(pageResults("first"), AsyncResult.success(pages.first!)); - await screen.rerender(content(2)); + await screen.rerender(content(2, url)); }); await vi.waitFor(() => expect(mocks.diffCursors.at(-1)).toBe("page-2")); await expect.element(screen.getByText("41 files", { exact: true })).toBeVisible(); + await vi.waitFor(() => expect(settled).toHaveBeenCalledWith(url)); await screen.rerender(content(3)); await expect.element(screen.getByText("41 files", { exact: true })).toBeVisible(); }); diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index bda256ff17ea0..44b9faa926f51 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -1655,6 +1655,7 @@ function PullRequestCodeTab({ const revealLink = useEffectEvent(() => { if (revealUrl === undefined) answeredLink.current = null; if (revealUrl === undefined || linkAnchor?.kind !== "diff") return; + if (sliceState.refreshing !== null) return; // The panel selects the link's commit after this tab's effects run, so the files on screen // can still be another commit's. const linkCommit = pullRequestLinkCommit(revealUrl);