Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions apps/web/src/components/DiffPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -59,7 +63,6 @@ import {
import { DiffPanelLoadingState, DiffPanelShell, type DiffPanelMode } from "./DiffPanelShell";
import {
FileDiffHeader,
buildFileDiffRenderKey,
getFileDiffStatusBadge,
resolveFileDiffPath,
} from "./diffs/fileDiffPresentation";
Expand Down Expand Up @@ -1598,7 +1601,7 @@ export default function DiffPanel({ mode = "inline", onClose, embedded = false }
</div>
) : (
<Virtualizer
className="diff-render-surface h-full min-h-0 overflow-auto px-2 pb-2"
className="diff-render-surface h-full min-h-0 overflow-auto"
config={{
overscrollSize: 600,
intersectionObserverMargin: 1200,
Expand All @@ -1615,13 +1618,15 @@ export default function DiffPanel({ mode = "inline", onClose, embedded = false }
const showPreview = previewFileDiff != null && previewFileDiff.hunks.length > 0;
return (
<div
// The diff instance hydrates once per mount and skips
// fileDiff swaps unless options change, so the
// changes-only variant must remount.
// Keyed by path, so a refetched patch updates each file in
// place instead of rebuilding every instance. The changes-only
// cut is a separate parse, so switching it remounts.
key={`${themedFileKey}:${showPreview ? "changes" : "full"}`}
data-diff-file-path={filePath}
// Files stack edge to edge, split by hairlines. A collapsed
// file is only its header, whose own bottom rule is the divider.
className={cn(
"diff-render-file group/diff-file mb-2 rounded-md first:mt-2 last:mb-0",
!collapsed && "border-b border-border",
flashFilePath === filePath && "diff-file-flash",
)}
onContextMenu={(event) => {
Expand Down
43 changes: 37 additions & 6 deletions apps/web/src/components/DiffPanelShell.tsx
Original file line number Diff line number Diff line change
@@ -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";

Expand Down Expand Up @@ -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 (
<div
className="min-h-0 flex-1 px-3 py-2 text-[12px] text-muted-foreground/55"
className="diff-panel-loading min-h-0 flex-1 overflow-hidden"
role="status"
aria-live="polite"
data-diff-panel-loading="true"
>
{props.label}
<span className="sr-only">{props.label}</span>
{LOADING_ROWS.map((row, rowIndex) => (
<div key={rowIndex} className="border-b border-border" aria-hidden="true">
<div className="flex h-9 items-center gap-1.5 pl-1.5 pr-2">
<Skeleton className="size-4 shrink-0" />
<Skeleton className="size-4 shrink-0" />
<Skeleton className={cn("h-2.5", row.path)} />
<Skeleton className="ml-auto h-2.5 w-10 shrink-0" />
</div>
{row.lines ? (
<div className="space-y-2.5 px-2 pt-1.5 pb-3">
{row.lines.map((width, lineIndex) => (
<div key={lineIndex} className="flex items-center gap-3">
<Skeleton className="h-2.5 w-7 shrink-0" />
<Skeleton className={cn("h-2.5", width)} />
</div>
))}
</div>
) : null}
</div>
))}
</div>
);
}
8 changes: 0 additions & 8 deletions apps/web/src/components/diffs/fileDiffPresentation.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
8 changes: 2 additions & 6 deletions apps/web/src/components/pull-requests/PullRequestCodeTab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
27 changes: 16 additions & 11 deletions apps/web/src/index.css
Original file line number Diff line number Diff line change
Expand Up @@ -1008,28 +1008,32 @@ 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;
}
}

.diff-file-flash {
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 {
Expand Down Expand Up @@ -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,
Expand Down
52 changes: 51 additions & 1 deletion apps/web/src/lib/diffRendering.test.ts
Original file line number Diff line number Diff line change
@@ -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", () => {
Expand Down Expand Up @@ -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);
});
});
80 changes: 79 additions & 1 deletion apps/web/src/lib/diffRendering.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, ReadonlyMap<string, FileDiffMetadata>>();

function shareUnchangedFiles(files: FileDiffMetadata[], cacheScope: string): FileDiffMetadata[] {
const previous = lastParsedFilesByScope.get(cacheScope);
const next = new Map<string, FileDiffMetadata>();
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",
Expand All @@ -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 };
}
Expand Down
Loading