diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index 15acf47b..ce5e35c6 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -37,7 +37,11 @@ import { resolvePathLinkTarget } from "../terminal-links"; import { parseDiffRouteSearch, stripDiffSearchParams } from "../diffRouteSearch"; import { useComposerDraftStore } from "../composerDraftStore"; import { useTheme } from "../hooks/useTheme"; -import { getRenderablePatch, resolveDiffThemeName } from "../lib/diffRendering"; +import { + buildFileDiffRenderKey, + getRenderablePatch, + resolveDiffThemeName, +} from "../lib/diffRendering"; import { useTurnDiffSummaries } from "../hooks/useTurnDiffSummaries"; import { useStore } from "../store"; import { createProjectSelectorByRef, createThreadSelectorByRef } from "../storeSelectors"; @@ -59,7 +63,6 @@ import { import { DiffPanelLoadingState, DiffPanelShell, type DiffPanelMode } from "./DiffPanelShell"; import { FileDiffHeader, - buildFileDiffRenderKey, getFileDiffStatusBadge, resolveFileDiffPath, } from "./diffs/fileDiffPresentation"; @@ -1598,7 +1601,7 @@ export default function DiffPanel({ mode = "inline", onClose, embedded = false } ) : ( 0; return (
{ diff --git a/apps/web/src/components/DiffPanelShell.tsx b/apps/web/src/components/DiffPanelShell.tsx index 44b20e22..174596e6 100644 --- a/apps/web/src/components/DiffPanelShell.tsx +++ b/apps/web/src/components/DiffPanelShell.tsx @@ -1,6 +1,7 @@ import type { KeyboardEvent, ReactNode } from "react"; import { cn } from "~/lib/utils"; +import { Skeleton } from "./ui/skeleton"; export type DiffPanelMode = "inline" | "sheet" | "sidebar"; @@ -57,20 +58,50 @@ export function DiffPanelShell(props: { } /** - * Waiting for the diff says so in one line, on the panel's gutter. It used to be - * a framed pane of skeleton bars, which drew a whole fake document over a delay - * that is usually shorter than reading the word "loading" -- and made the panel - * look like an embedded app rather than a sidebar. + * Placeholder rows in the shape of the list that is coming: one open file + * with a run of lines, then a few closed ones. Bars only, no frame, so it + * reads as the same edge-to-edge list before the data lands. The reveal is + * held back a beat (see `.diff-panel-loading`) so a fast load never flashes it. */ +const LOADING_ROWS: ReadonlyArray<{ + readonly path: string; + readonly lines?: readonly string[]; +}> = [ + { path: "w-44", lines: ["w-3/5", "w-2/5", "w-4/5", "w-1/3", "w-1/2", "w-3/4", "w-2/5"] }, + { path: "w-32" }, + { path: "w-52" }, + { path: "w-40" }, +]; + export function DiffPanelLoadingState(props: { label: string }) { return (
- {props.label} + {props.label} + {LOADING_ROWS.map((row, rowIndex) => ( + + ))}
); } diff --git a/apps/web/src/components/diffs/fileDiffPresentation.tsx b/apps/web/src/components/diffs/fileDiffPresentation.tsx index 3e72b6a5..7e70c1f6 100644 --- a/apps/web/src/components/diffs/fileDiffPresentation.tsx +++ b/apps/web/src/components/diffs/fileDiffPresentation.tsx @@ -24,14 +24,6 @@ export function resolveFileDiffPath(fileDiff: FileDiffMetadata): string { return raw; } -/** - * The instance hydrates once per mount and skips `fileDiff` swaps, so this is - * what a caller keys its wrapper on to force a remount when the file changes. - */ -export function buildFileDiffRenderKey(fileDiff: FileDiffMetadata): string { - return fileDiff.cacheKey ?? `${fileDiff.prevName ?? "none"}:${fileDiff.name}`; -} - /** Rename source path, only when it differs from the displayed path. */ export function resolveFileDiffPrevPath(fileDiff: FileDiffMetadata): string | null { const raw = fileDiff.prevName; diff --git a/apps/web/src/components/pull-requests/PullRequestCodeTab.tsx b/apps/web/src/components/pull-requests/PullRequestCodeTab.tsx index 018a17b4..cc7f8439 100644 --- a/apps/web/src/components/pull-requests/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pull-requests/PullRequestCodeTab.tsx @@ -18,17 +18,13 @@ import type { import { useMutation, useQueryClient } from "@tanstack/react-query"; import { useCallback, useMemo, useState } from "react"; -import { fnv1a32, getRenderablePatch } from "../../lib/diffRendering"; +import { buildFileDiffRenderKey, fnv1a32, getRenderablePatch } from "../../lib/diffRendering"; import { openExternalUrl } from "../../lib/externalLinks"; import { pullRequestReviewMutationOptions } from "../../lib/pullRequestsReactQuery"; import { AnnotatedDiffView } from "../diffs/AnnotatedDiffView"; import { DiffCommentDraft } from "../diffs/DiffCommentAnnotation"; import { useDiffWorkerReady } from "../diffs/useDiffWorkerReady"; -import { - buildFileDiffRenderKey, - resolveFileDiffPath, - resolveFileDiffPrevPath, -} from "../diffs/fileDiffPresentation"; +import { resolveFileDiffPath, resolveFileDiffPrevPath } from "../diffs/fileDiffPresentation"; import { PendingReviewCommentCard, ReviewThreadCard } from "./PullRequestReviewAnnotations"; import { PullRequestReviewBar } from "./PullRequestReviewBar"; import { diff --git a/apps/web/src/index.css b/apps/web/src/index.css index dbee86c5..c2710207 100644 --- a/apps/web/src/index.css +++ b/apps/web/src/index.css @@ -1008,21 +1008,14 @@ label:has(> select#reasoning-effort) select { background: color-mix(in srgb, var(--background) 94%, var(--card)); } -.diff-render-file { - border: 1px solid var(--border); - border-radius: var(--radius-2xl); - overflow: clip; - background: color-mix(in srgb, var(--card) 92%, var(--background)); - scroll-margin-top: 0.5rem; -} - -/* One-time emphasis on the file card the diff panel just scrolled to. */ +/* One-time emphasis on the file the diff panel just scrolled to. Files sit + flush against the scroller edges, so the ring is drawn inside the box. */ @keyframes diff-file-flash { 0% { - box-shadow: 0 0 0 1.5px color-mix(in srgb, var(--primary) 55%, transparent); + box-shadow: inset 0 0 0 1.5px color-mix(in srgb, var(--primary) 55%, transparent); } 100% { - box-shadow: 0 0 0 1.5px transparent; + box-shadow: inset 0 0 0 1.5px transparent; } } @@ -1030,6 +1023,17 @@ label:has(> select#reasoning-effort) select { animation: diff-file-flash 1.1s ease-out 150ms both; } +/* Loading rows hold back a beat so a diff that lands fast never flashes them. */ +@keyframes diff-panel-loading-reveal { + from { + opacity: 0; + } +} + +.diff-panel-loading { + animation: diff-panel-loading-reveal 160ms ease-out 120ms both; +} + /* Drill-in entrance for the diff panel body when it replaces source control. */ @keyframes diff-panel-enter { from { @@ -1709,6 +1713,7 @@ label:has(> select#reasoning-effort) select { @media (prefers-reduced-motion: reduce) { .diff-file-flash, + .diff-panel-loading, .diff-panel-enter, .work-row-enter, .work-meta-enter, diff --git a/apps/web/src/lib/diffRendering.test.ts b/apps/web/src/lib/diffRendering.test.ts index b7610fde..31ae8efe 100644 --- a/apps/web/src/lib/diffRendering.test.ts +++ b/apps/web/src/lib/diffRendering.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vite-plus/test"; -import { buildPatchCacheKey, getRenderablePatch } from "./diffRendering"; +import { buildFileDiffRenderKey, buildPatchCacheKey, getRenderablePatch } from "./diffRendering"; describe("buildPatchCacheKey", () => { it("returns a stable cache key for identical content", () => { @@ -63,3 +63,53 @@ describe("getRenderablePatch", () => { }); }); }); + +describe("getRenderablePatch identity across refetches", () => { + const fileA = [ + "diff --git a/src/a.ts b/src/a.ts", + "index 1111111..2222222 100644", + "--- a/src/a.ts", + "+++ b/src/a.ts", + "@@ -1,2 +1,2 @@", + " const a = 1;", + "-export const b = 2;", + "+export const b = 3;", + ].join("\n"); + const fileB = (value: string) => + [ + "diff --git a/src/b.ts b/src/b.ts", + `index 3333333..${value.length}444444 100644`, + "--- a/src/b.ts", + "+++ b/src/b.ts", + "@@ -1 +1 @@", + "-export const c = 0;", + `+export const c = ${value};`, + ].join("\n"); + + it("keeps the object for a file whose change did not move", () => { + const scope = `identity-test:${Math.random()}`; + const first = getRenderablePatch(`${fileA}\n${fileB("1")}`, scope); + const second = getRenderablePatch(`${fileA}\n${fileB("2")}`, scope); + if (first?.kind !== "files" || second?.kind !== "files") { + throw new Error("expected structured files"); + } + + expect(second.files[0]).toBe(first.files[0]); + expect(second.files[1]).not.toBe(first.files[1]); + expect(second.files[1]?.additionLines).toContain("export const c = 2;"); + }); + + it("keys a file by its path, not by the parse it came from", () => { + const scope = `identity-test:${Math.random()}`; + const first = getRenderablePatch(`${fileA}\n${fileB("1")}`, scope); + const second = getRenderablePatch(`${fileA}\n${fileB("2")}`, scope); + if (first?.kind !== "files" || second?.kind !== "files") { + throw new Error("expected structured files"); + } + + expect(second.files.map(buildFileDiffRenderKey)).toEqual( + first.files.map(buildFileDiffRenderKey), + ); + expect(second.files[1]?.cacheKey).not.toBe(first.files[1]?.cacheKey); + }); +}); diff --git a/apps/web/src/lib/diffRendering.ts b/apps/web/src/lib/diffRendering.ts index 3220f3df..22641465 100644 --- a/apps/web/src/lib/diffRendering.ts +++ b/apps/web/src/lib/diffRendering.ts @@ -62,6 +62,81 @@ export function buildPatchCacheKey(patch: string, scope = "diff-panel"): string return `${scope}:${normalizedPatch.length}:${primary}:${secondary}`; } +/** + * What a file is called across parses of a moving patch: the path it lands + * on, plus where it came from for a rename. The parser derives `cacheKey` + * from the whole patch, so it changes for every file whenever any file + * changes, and cannot key anything that should outlive a refetch (collapse + * state, the React element, the diff instance behind it). + */ +export function buildFileDiffRenderKey(fileDiff: FileDiffMetadata): string { + return `${fileDiff.prevName ?? "none"}:${fileDiff.name}`; +} + +function areStringArraysEqual(left: readonly string[], right: readonly string[]): boolean { + if (left.length !== right.length) return false; + for (let index = 0; index < left.length; index += 1) { + if (left[index] !== right[index]) return false; + } + return true; +} + +/** Whether two parses describe the same change to the same file. */ +function areFileDiffContentsEqual(left: FileDiffMetadata, right: FileDiffMetadata): boolean { + if ( + left.type !== right.type || + left.mode !== right.mode || + left.prevMode !== right.prevMode || + left.newObjectId !== right.newObjectId || + left.prevObjectId !== right.prevObjectId || + left.hunks.length !== right.hunks.length + ) { + return false; + } + const hunksEqual = left.hunks.every((hunk, index) => { + const other = right.hunks[index]; + return ( + other !== undefined && + hunk.additionStart === other.additionStart && + hunk.additionCount === other.additionCount && + hunk.additionLines === other.additionLines && + hunk.deletionStart === other.deletionStart && + hunk.deletionCount === other.deletionCount && + hunk.deletionLines === other.deletionLines && + hunk.hunkSpecs === other.hunkSpecs && + hunk.hunkContext === other.hunkContext + ); + }); + return ( + hunksEqual && + areStringArraysEqual(left.additionLines, right.additionLines) && + areStringArraysEqual(left.deletionLines, right.deletionLines) + ); +} + +/** + * The previous parse per scope, by file identity. A refetched patch is parsed + * from scratch, and downstream a fresh object reads as "this file changed": + * the diff instance rebuilds and re-highlights it. Files whose change did not + * move keep the object they already had, so identity means "same diff" and a + * save to one file leaves the others untouched. + */ +const lastParsedFilesByScope = new Map>(); + +function shareUnchangedFiles(files: FileDiffMetadata[], cacheScope: string): FileDiffMetadata[] { + const previous = lastParsedFilesByScope.get(cacheScope); + const next = new Map(); + const shared = files.map((file) => { + const identity = buildFileDiffRenderKey(file); + const prior = previous?.get(identity); + const kept = prior !== undefined && areFileDiffContentsEqual(prior, file) ? prior : file; + next.set(identity, kept); + return kept; + }); + lastParsedFilesByScope.set(cacheScope, next); + return shared; +} + export function getRenderablePatch( patch: string | undefined, cacheScope = "diff-panel", @@ -75,7 +150,10 @@ export function getRenderablePatch( normalizedPatch, buildPatchCacheKey(normalizedPatch, cacheScope), ); - const files = parsedPatches.flatMap((parsedPatch) => parsedPatch.files); + const files = shareUnchangedFiles( + parsedPatches.flatMap((parsedPatch) => parsedPatch.files), + cacheScope, + ); if (files.length > 0) { return { kind: "files", files }; }