From 815842c5f8a77c9bb5f234b3e11a72f758235c14 Mon Sep 17 00:00:00 2001 From: Rodrigo Brechard Date: Mon, 19 Oct 2026 15:45:42 +0200 Subject: [PATCH] fix(web): the pull request Code tab keeps its diff after a refresh A refresh emptied the accumulated diff slices and refetched the first page, but slices were only refilled when the query's data changed identity. An equal refetch keeps the previous value's identity, so the tab showed "no file changes" until the diff's content changed. Read whether a page is already applied from the slices instead of a separate identity gate. Co-Authored-By: Claude Opus 5.5 (cherry picked from commit 68b4e5a599e3561ddd5a23568bb5183a38fb342c) --- .../PullRequestCodeTab.browser.test.tsx | 147 ++++++++++++++++++ .../pullRequest/PullRequestCodeTab.tsx | 75 +++++---- 2 files changed, 182 insertions(+), 40 deletions(-) create mode 100644 apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx new file mode 100644 index 000000000000..4d28e968f266 --- /dev/null +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx @@ -0,0 +1,147 @@ +import { + EnvironmentId, + ProjectId, + SourceControlProviderKind, + type PullRequestDetailView, + type PullRequestDiffResult, +} from "@t3tools/contracts"; +import { act } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { afterEach, beforeEach, expect, it, vi } from "vite-plus/test"; +import { page } from "vite-plus/test/browser"; + +import "~/index.css"; + +const { diffAtom } = await vi.hoisted(async () => { + const { AsyncResult, Atom } = await import("effect/reactivity"); + const firstPage: PullRequestDiffResult = { + patch: [ + "diff --git a/a.ts b/a.ts", + "index 1111111..2222222 100644", + "--- a/a.ts", + "+++ b/a.ts", + "@@ -1 +1 @@", + "-before()", + "+after()", + "", + ].join("\n"), + truncated: false, + nextCursor: null, + }; + // 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. + return { diffAtom: Atom.make(() => AsyncResult.success(firstPage)) }; +}); + +vi.mock(import("~/state/use-atom-command"), async (original) => ({ + ...(await original()), + useAtomCommand: (() => vi.fn()) as never, +})); +vi.mock(import("~/state/pullRequests"), async (original) => { + const actual = await original(); + return { + ...actual, + pullRequestEnvironment: { + ...actual.pullRequestEnvironment, + diff: (() => diffAtom) as never, + }, + }; +}); + +import PullRequestCodeTab from "./PullRequestCodeTab"; + +const detail: PullRequestDetailView = { + provider: SourceControlProviderKind.make("github"), + projectId: ProjectId.make("project"), + projectTitle: "Project", + workspaceRoot: "/workspace", + repository: "owner/repo", + number: 1, + title: "Test pull request", + body: "", + url: "https://github.com/owner/repo/pull/1", + author: { login: "author", name: null, avatarUrl: null }, + viewer: "author", + state: "open", + isDraft: false, + mergeability: "mergeable", + additions: 1, + deletions: 1, + changedFiles: 1, + headBranch: "feature", + baseBranch: "main", + createdAt: "2026-09-01T00:00:00Z", + updatedAt: "2026-09-01T00:00:00Z", + mergedAt: null, + closedAt: null, + reviewers: [], + labels: [], + checks: [], + comments: [], + commentCount: 0, + commentsTruncated: false, + reviewThreads: [], + commits: [], + mergeCapabilities: { merge: false, squash: false, rebase: false }, + capabilities: { + diff: true, + comment: false, + search: true, + actions: [], + mergeMethods: [], + review: { inlineComment: false, reply: false, resolve: false, verdicts: [] }, + reviewers: { request: false, listCandidates: false }, + edit: { changeRequest: false, comment: false }, + }, + viewerPermissions: { + actions: [], + comment: false, + resolve: false, + verdicts: [], + requestReviewers: false, + }, +}; + +let root: Root; +let container: HTMLDivElement; +beforeEach(() => { + vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + container = document.createElement("div"); + container.className = "flex h-[700px] w-[900px]"; + document.body.append(container); + root = createRoot(container); +}); +afterEach(async () => { + await act(async () => root.unmount()); + container.remove(); + localStorage.clear(); + vi.unstubAllGlobals(); +}); + +const render = (refreshToken: number) => + act(async () => + root.render( + {}} + environmentId={EnvironmentId.make("environment")} + reference={detail} + detail={detail} + selectedCommitOid={null} + onSelectedCommitChange={() => {}} + onRefresh={() => {}} + refreshToken={refreshToken} + />, + ), + ); + +it("keeps the diff after a refresh that brings back the same page", async () => { + await render(0); + await expect.element(page.getByText("1 file", { exact: true })).toBeVisible(); + + await render(1); + + await expect + .element(page.getByText("This pull request has no file changes.")) + .not.toBeInTheDocument(); + await expect.element(page.getByText("1 file", { exact: true })).toBeVisible(); +}); diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 751b34ecdc04..fa2cb3651154 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -145,6 +145,25 @@ const REPLACE_FILE_COUNTS_CSS = ` const NO_SLICES: ReadonlyArray = []; +function sameSlice(slice: DiffSlice, page: DiffSlice) { + return ( + slice.cursor === page.cursor && + slice.patch === page.patch && + slice.truncated === page.truncated && + slice.nextCursor === page.nextCursor && + slice.omittedFileStats.length === page.omittedFileStats.length && + slice.omittedFileStats.every((file, index) => { + const refreshed = page.omittedFileStats[index]; + return ( + refreshed !== undefined && + refreshed.path === file.path && + refreshed.additions === file.additions && + refreshed.deletions === file.deletions + ); + }) + ); +} + // Each slice is parsed once per cache key and the parse lives as long as the slice does, so a // slice arriving costs one parse rather than one per slice already on screen. const sliceParses = new WeakMap>(); @@ -336,55 +355,31 @@ function PullRequestCodeTab({ }), ); // Pages accumulate into slices as each answer arrives, so the slice list is adjusted while - // rendering the answer rather than one commit later. - const [appliedPage, setAppliedPage] = useState<{ - readonly data: typeof diffQuery.data; - readonly cursor: string | null; - readonly scopeKey: string; - } | null>(null); + // rendering the answer rather than one commit later. Whether an answer is in is read from the + // slices: an equal refetch keeps the answer's identity, so it cannot signal a reset's refill. const data = diffQuery.data; - if ( - data !== null && - (appliedPage?.data !== data || - appliedPage.cursor !== cursor || - appliedPage.scopeKey !== scopeKey) - ) { - setAppliedPage({ data, cursor, scopeKey }); + const page: DiffSlice | null = + data === null + ? null + : { + cursor, + patch: data.patch, + truncated: data.truncated, + nextCursor: data.nextCursor, + omittedFileStats: data.omittedFileStats ?? [], + }; + if (page !== null && !loadedSlices.some((slice) => sameSlice(slice, page))) { setSliceState((previous) => { const slices = previous.key === scopeKey ? previous.slices : NO_SLICES; - const next = { - cursor, - patch: data.patch, - truncated: data.truncated, - nextCursor: data.nextCursor, - omittedFileStats: data.omittedFileStats ?? [], - }; const index = slices.findIndex((slice) => slice.cursor === cursor); if (index === -1) { - return { key: scopeKey, cursor, slices: [...slices, next] }; + return { key: scopeKey, cursor, slices: [...slices, page] }; } const existing = slices[index]; - if ( - existing !== undefined && - existing.patch === next.patch && - existing.truncated === next.truncated && - existing.nextCursor === next.nextCursor && - existing.omittedFileStats.length === next.omittedFileStats.length && - existing.omittedFileStats.every((file, index) => { - const refreshed = next.omittedFileStats[index]; - return ( - refreshed !== undefined && - refreshed.path === file.path && - refreshed.additions === file.additions && - refreshed.deletions === file.deletions - ); - }) - ) { - return previous; - } + if (existing !== undefined && sameSlice(existing, page)) return previous; // A page that came back different means the diff moved under the review. The slices // after it go with the replacement: their cursors were positions in the old diff. - return { key: scopeKey, cursor, slices: [...slices.slice(0, index), next] }; + return { key: scopeKey, cursor, slices: [...slices.slice(0, index), page] }; }); } const refreshFirstDiffPage = useAtomRefresh(