From 2bcc5ea11ffefed0d10c0133a4dd7692e1b73875 Mon Sep 17 00:00:00 2001 From: badcuban <108198679+badcuban@users.noreply.github.com> Date: Thu, 3 Sep 2026 00:08:39 -0400 Subject: [PATCH 1/2] feat(web): diff panel lists files edge to edge and keeps them across refetches Files in the right-panel diff sat in rounded cards inside the panel. They now run edge to edge with a hairline between them, matching the pull request diff viewer. The jump-to-file flash draws inside the row so the edges do not clip it. Loading shows placeholder rows in the shape of the list instead of a one-line label, held back a beat so a fast load never flashes them. The parser keys every file off the whole patch, so any change re-keyed every file: each diff instance remounted and re-highlighted, and collapse state was wiped on every working-tree refetch. Files are now keyed by path, and files whose change did not move keep their parsed object across refetches. --- apps/web/src/components/DiffPanel.tsx | 22 +++--- apps/web/src/components/DiffPanelShell.tsx | 43 ++++++++++-- apps/web/src/index.css | 27 ++++--- apps/web/src/lib/diffRendering.test.ts | 52 +++++++++++++- apps/web/src/lib/diffRendering.ts | 82 +++++++++++++++++++++- 5 files changed, 196 insertions(+), 30 deletions(-) diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index 1ab81e6de..d2ce68c6c 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -38,7 +38,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"; @@ -95,10 +99,6 @@ function resolveFileDiffPath(fileDiff: FileDiffMetadata): string { return raw; } -function buildFileDiffRenderKey(fileDiff: FileDiffMetadata): string { - return fileDiff.cacheKey ?? `${fileDiff.prevName ?? "none"}:${fileDiff.name}`; -} - /** Rename source path, only when it differs from the displayed path. */ function resolveFileDiffPrevPath(fileDiff: FileDiffMetadata): string | null { const raw = fileDiff.prevName; @@ -1636,7 +1636,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 44b20e221..174596e6d 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/index.css b/apps/web/src/index.css index 681ab2f65..d50a909b3 100644 --- a/apps/web/src/index.css +++ b/apps/web/src/index.css @@ -995,21 +995,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; } } @@ -1017,6 +1010,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 { @@ -1696,6 +1700,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 b7610fde5..31ae8efea 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 3220f3df7..b319b53d9 100644 --- a/apps/web/src/lib/diffRendering.ts +++ b/apps/web/src/lib/diffRendering.ts @@ -21,7 +21,7 @@ export type RenderablePatch = const ANSI_ESCAPE = String.fromCharCode(0x1b); const ANSI_CONTROL_SEQUENCE_PATTERN = new RegExp( - `${ANSI_ESCAPE}(?:[@-Z\\\\-_]|\\[[0-?]*[ -/]*[@-~])`, + `${ANSI_ESCAPE}(?:[@-Z\\-_]|\[[0-?]*[ -/]*[@-~])`, "gu", ); @@ -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 }; } From c62f074355cebc9c1ee8d231ca7e3d423b39e008 Mon Sep 17 00:00:00 2001 From: badcuban <108198679+badcuban@users.noreply.github.com> Date: Thu, 3 Sep 2026 00:32:19 -0400 Subject: [PATCH 2/2] fix(web): restore the ANSI escape pattern in diffRendering The file rewrite in the previous commit dropped one backslash level from the regular expression that strips ANSI color codes, which CodeQL flagged. The line now matches main again. --- apps/web/src/lib/diffRendering.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/web/src/lib/diffRendering.ts b/apps/web/src/lib/diffRendering.ts index b319b53d9..22641465d 100644 --- a/apps/web/src/lib/diffRendering.ts +++ b/apps/web/src/lib/diffRendering.ts @@ -21,7 +21,7 @@ export type RenderablePatch = const ANSI_ESCAPE = String.fromCharCode(0x1b); const ANSI_CONTROL_SEQUENCE_PATTERN = new RegExp( - `${ANSI_ESCAPE}(?:[@-Z\\-_]|\[[0-?]*[ -/]*[@-~])`, + `${ANSI_ESCAPE}(?:[@-Z\\\\-_]|\\[[0-?]*[ -/]*[@-~])`, "gu", );