From 00d1f1c59625e97dbe3d00b934ff35117bd1e4e6 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Wed, 10 Jun 2026 22:31:47 +0100 Subject: [PATCH] chore(site): upgrade @pierre/diffs and restore diff file tree (#25879) --- site/package.json | 3 +- site/pnpm-lock.yaml | 48 +- .../ChatElements/Response.stories.tsx | 59 +- .../ChatElements/tools/DiffFileHeader.tsx | 52 + .../ChatElements/tools/EditFilesTool.tsx | 4 + .../ChatElements/tools/Tool.stories.tsx | 73 +- .../components/ChatElements/tools/Tool.tsx | 12 +- .../ChatElements/tools/WriteFileTool.tsx | 4 + .../ChatElements/tools/utils.test.ts | 100 +- .../components/ChatElements/tools/utils.ts | 166 +-- .../DiffViewer/CommentableDiffViewer.tsx | 25 +- .../DiffViewer/DiffViewer.stories.tsx | 39 +- .../components/DiffViewer/DiffViewer.tsx | 1133 +++++++---------- .../DiffViewer/annotationsVersion.test.ts | 44 + .../DiffViewer/compareTreePaths.test.ts | 98 ++ .../DiffViewer/useActiveFileTracking.test.ts | 65 + .../DiffViewer/useActiveFileTracking.ts | 192 +-- .../components/GitPanel/GitPanel.stories.tsx | 40 +- .../AgentsPage/utils/countChangedLines.ts | 12 + 19 files changed, 1018 insertions(+), 1151 deletions(-) create mode 100644 site/src/pages/AgentsPage/components/ChatElements/tools/DiffFileHeader.tsx create mode 100644 site/src/pages/AgentsPage/components/DiffViewer/annotationsVersion.test.ts create mode 100644 site/src/pages/AgentsPage/components/DiffViewer/compareTreePaths.test.ts create mode 100644 site/src/pages/AgentsPage/components/DiffViewer/useActiveFileTracking.test.ts create mode 100644 site/src/pages/AgentsPage/utils/countChangedLines.ts diff --git a/site/package.json b/site/package.json index 519ec47024..6025d7ca34 100644 --- a/site/package.json +++ b/site/package.json @@ -60,7 +60,8 @@ "@mui/material": "5.18.0", "@mui/system": "5.18.0", "@novnc/novnc": "^1.5.0", - "@pierre/diffs": "1.1.19", + "@pierre/diffs": "1.2.4", + "@pierre/trees": "1.0.0-beta.4", "@tanstack/react-query-devtools": "5.77.0", "@xterm/addon-canvas": "0.7.0", "@xterm/addon-fit": "0.11.0", diff --git a/site/pnpm-lock.yaml b/site/pnpm-lock.yaml index b1b8fa8a40..a05af01e68 100644 --- a/site/pnpm-lock.yaml +++ b/site/pnpm-lock.yaml @@ -96,8 +96,11 @@ importers: specifier: ^1.5.0 version: 1.5.0 '@pierre/diffs': - specifier: 1.1.19 - version: 1.1.19(react-dom@19.2.6(react@19.2.6))(react@19.2.6) + specifier: 1.2.4 + version: 1.2.4(react-dom@19.2.6(react@19.2.6))(react@19.2.6) + '@pierre/trees': + specifier: 1.0.0-beta.4 + version: 1.0.0-beta.4(react-dom@19.2.6(react@19.2.6))(react@19.2.6) '@tanstack/react-query-devtools': specifier: 5.77.0 version: 5.77.0(@tanstack/react-query@5.77.0(react@19.2.6))(react@19.2.6) @@ -1414,16 +1417,22 @@ packages: cpu: [x64] os: [win32] - '@pierre/diffs@1.1.19': - resolution: {integrity: sha512-eYyDW69heXd7i9zdkWogGYosHzoYF2dstV6uDcmnQAf72uRChs3hrpf/7ym/ayTiwD8a+TQ7oZ5vNNb0tstJvA==, tarball: https://registry.npmjs.org/@pierre/diffs/-/diffs-1.1.19.tgz} + '@pierre/diffs@1.2.4': + resolution: {integrity: sha512-SEuYxGpSCHVvfoLly/Q/OYpJSBLWaVLV3M3wI/VBW7aZmzYenNe4aXjOf5sIKJMWW5gbZe9WdLvtKUt6cQ1k1A==, tarball: https://registry.npmjs.org/@pierre/diffs/-/diffs-1.2.4.tgz} peerDependencies: react: ^18.3.1 || ^19.0.0 react-dom: ^18.3.1 || ^19.0.0 - '@pierre/theme@0.0.28': - resolution: {integrity: sha512-1j/H/fECBuc9dEvntdWI+l435HZapw+RCJTlqCA6BboQ5TjlnE005j/ROWutXIs8aq5OAc82JI2Kwk4A1WWBgw==, tarball: https://registry.npmjs.org/@pierre/theme/-/theme-0.0.28.tgz} + '@pierre/theme@1.0.3': + resolution: {integrity: sha512-sWHv11TMoqKxKDgTIk5VbhQjdPhs8DCcBxbjh3mRlS3YOM/OcrWoGX6MM8eBGn9cUu3M46Py0JnxsG2nJaFTuA==, tarball: https://registry.npmjs.org/@pierre/theme/-/theme-1.0.3.tgz} engines: {vscode: ^1.0.0} + '@pierre/trees@1.0.0-beta.4': + resolution: {integrity: sha512-OfT1yk9ne8Te5+GB5zUY8yqE6B8BqjBHQJleH4lu8ltwNpoocZl4vXt1AzlEExpxI/pp+AFX5QG+lR3JjtTEag==, tarball: https://registry.npmjs.org/@pierre/trees/-/trees-1.0.0-beta.4.tgz} + peerDependencies: + react: ^18.3.1 || ^19.0.0 + react-dom: ^18.3.1 || ^19.0.0 + '@pkgjs/parseargs@0.11.0': resolution: {integrity: sha512-+1VkjdD0QBLPodGrJUeqarH8VAIvQODIbwh9XpP5Syisf7YoQgsJKPNFoqqLQlu+VQ/tVSshMR6loPMn8U+dPg==, tarball: https://registry.npmjs.org/@pkgjs/parseargs/-/parseargs-0.11.0.tgz} engines: {node: '>=14'} @@ -5179,6 +5188,14 @@ packages: resolution: {integrity: sha512-dM0jVuXJPsDN6DvRpea484tCUaMiXWjuCn++HGTqUWzGDjv5tZkEZldAJ/UMlqRYGFrD/etByo4/xOuC/snX2A==, tarball: https://registry.npmjs.org/powershell-utils/-/powershell-utils-0.1.0.tgz} engines: {node: '>=20'} + preact-render-to-string@6.6.5: + resolution: {integrity: sha512-O6MHzYNIKYaiSX3bOw0gGZfEbOmlIDtDfWwN1JJdc/T3ihzRT6tGGSEWE088dWrEDGa1u7101q+6fzQnO9XCPA==, tarball: https://registry.npmjs.org/preact-render-to-string/-/preact-render-to-string-6.6.5.tgz} + peerDependencies: + preact: '>=10 || >= 11.0.0-0' + + preact@11.0.0-beta.0: + resolution: {integrity: sha512-IcODoASASYwJ9kxz7+MJeiJhvLriwSb4y4mHIyxdgaRZp6kPUud7xytrk/6GZw8U3y6EFJaRb5wi9SrEK+8+lg==, tarball: https://registry.npmjs.org/preact/-/preact-11.0.0-beta.0.tgz} + prelude-ls@1.2.1: resolution: {integrity: sha512-vkcDPrRZo1QZLbn5RLGPpg/WmIQ65qoWWhcGKf/b5eplkkarX0m9z8ppCat4mlOqUsWpyNuYgO3VRyrYHSzX5g==, tarball: https://registry.npmjs.org/prelude-ls/-/prelude-ls-1.2.1.tgz} engines: {node: '>= 0.8.0'} @@ -7491,9 +7508,9 @@ snapshots: '@oxc-resolver/binding-win32-x64-msvc@11.14.0': optional: true - '@pierre/diffs@1.1.19(react-dom@19.2.6(react@19.2.6))(react@19.2.6)': + '@pierre/diffs@1.2.4(react-dom@19.2.6(react@19.2.6))(react@19.2.6)': dependencies: - '@pierre/theme': 0.0.28 + '@pierre/theme': 1.0.3 '@shikijs/transformers': 3.23.0 diff: 8.0.3 hast-util-to-html: 9.0.5 @@ -7502,7 +7519,14 @@ snapshots: react-dom: 19.2.6(react@19.2.6) shiki: 3.23.0 - '@pierre/theme@0.0.28': {} + '@pierre/theme@1.0.3': {} + + '@pierre/trees@1.0.0-beta.4(react-dom@19.2.6(react@19.2.6))(react@19.2.6)': + dependencies: + preact: 11.0.0-beta.0 + preact-render-to-string: 6.6.5(preact@11.0.0-beta.0) + react: 19.2.6 + react-dom: 19.2.6(react@19.2.6) '@pkgjs/parseargs@0.11.0': optional: true @@ -11612,6 +11636,12 @@ snapshots: powershell-utils@0.1.0: {} + preact-render-to-string@6.6.5(preact@11.0.0-beta.0): + dependencies: + preact: 11.0.0-beta.0 + + preact@11.0.0-beta.0: {} + prelude-ls@1.2.1: optional: true diff --git a/site/src/pages/AgentsPage/components/ChatElements/Response.stories.tsx b/site/src/pages/AgentsPage/components/ChatElements/Response.stories.tsx index 17b6101448..622c65eee4 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/Response.stories.tsx +++ b/site/src/pages/AgentsPage/components/ChatElements/Response.stories.tsx @@ -104,73 +104,18 @@ const expectCodeBlock = async ( expect(host.style.getPropertyValue("--diffs-font-size")).toBe("12px"); expect(host.style.getPropertyValue("--diffs-line-height")).toBe("20px"); + expect(canvasElement.textContent ?? "").not.toContain("```"); + const shadowRoot = host.shadowRoot; if (!shadowRoot) { throw new Error("Expected FileViewer to render code in its shadow root."); } - expect(shadowRoot.textContent ?? "").not.toContain("```"); - - const pre = shadowRoot.querySelector( - "pre[data-file][data-disable-line-numbers]", - ); - expect(pre).toBeInTheDocument(); - if (!(pre instanceof HTMLElement)) { - throw new Error("Expected FileViewer to render a pre element."); - } - - const code = shadowRoot.querySelector("[data-code]"); - expect(code).toBeInTheDocument(); - if (!(code instanceof HTMLElement)) { - throw new Error("Expected FileViewer to render a code container."); - } - - const line = shadowRoot.querySelector("[data-line]"); - expect(line).toBeInTheDocument(); - if (!(line instanceof HTMLElement)) { - throw new Error("Expected FileViewer to render code lines."); - } - - const gutter = shadowRoot.querySelector("[data-column-number]"); - expect(gutter).toBeInTheDocument(); - if (!(gutter instanceof HTMLElement)) { - throw new Error("Expected FileViewer to render its line-number gutter."); - } - - const preStyles = getComputedStyle(pre); - expect(preStyles.fontSize).toBe("12px"); - expect(preStyles.lineHeight).toBe("20px"); - - const codeStyles = getComputedStyle(code); - expect(codeStyles.paddingTop).toBe("8px"); - expect(codeStyles.paddingBottom).toBe("8px"); - expect(codeStyles.paddingBottom).toBe(codeStyles.paddingTop); - expect(codeStyles.overflow).toBe("visible"); - - const lineStyles = getComputedStyle(line); - expect(lineStyles.paddingLeft).toBe("12px"); - expect(lineStyles.paddingRight).toBe("12px"); - expect(lineStyles.paddingRight).toBe(lineStyles.paddingLeft); - expect(lineStyles.minHeight).toBe("20px"); - - const gutterStyles = getComputedStyle(gutter); - expect(gutterStyles.minWidth).toBe("0px"); - expect(gutterStyles.paddingLeft).toBe("0px"); - expect(gutterStyles.paddingRight).toBe("0px"); if (options.highlighted) { - let highlightedToken: HTMLElement | null = null; await waitFor(() => { const token = shadowRoot.querySelector("span[style*='color']"); expect(token).toBeInTheDocument(); - if (!(token instanceof HTMLElement)) { - throw new Error("Expected FileViewer to render highlighted tokens."); - } - highlightedToken = token; }); - if (!highlightedToken) { - throw new Error("Expected FileViewer to render highlighted tokens."); - } - expect(getComputedStyle(highlightedToken).color).not.toBe(lineStyles.color); } return host; diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/DiffFileHeader.tsx b/site/src/pages/AgentsPage/components/ChatElements/tools/DiffFileHeader.tsx new file mode 100644 index 0000000000..3e6493a371 --- /dev/null +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/DiffFileHeader.tsx @@ -0,0 +1,52 @@ +import type { FileContents, FileDiffMetadata } from "@pierre/diffs"; +import { cn } from "#/utils/cn"; +import { countChangedLines } from "../../../utils/countChangedLines"; +import { changeColor, changeLabel } from "../../../utils/diffColors"; + +export function DiffFileHeader({ + file, +}: { + file: FileDiffMetadata | FileContents; +}) { + const isDiff = "type" in file; + const stats = isDiff ? countChangedLines(file) : null; + + return ( +
+
+ {isDiff && ( + + {changeLabel(file.type)} + + )} + {isDiff && file.prevName && file.prevName !== file.name && ( + + {file.prevName} + + )} + + {file.name} + +
+ {stats && (stats.additions > 0 || stats.deletions > 0) && ( + + {stats.deletions > 0 && ( + + −{stats.deletions} + + )} + {stats.additions > 0 && ( + + +{stats.additions} + + )} + + )} +
+ ); +} diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/EditFilesTool.tsx b/site/src/pages/AgentsPage/components/ChatElements/tools/EditFilesTool.tsx index 93b8d5f177..0df55ff068 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/EditFilesTool.tsx +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/EditFilesTool.tsx @@ -4,6 +4,7 @@ import { FileDiff } from "@pierre/diffs/react"; import type React from "react"; import type * as TypesGen from "#/api/typesGenerated"; import { ScrollArea } from "#/components/ScrollArea/ScrollArea"; +import { DiffFileHeader } from "./DiffFileHeader"; import { type AgentDisplayState, isAgentDisplayFullyExpanded, @@ -85,6 +86,9 @@ export const EditFilesTool: React.FC<{ fileDiff={stripNoNewline(diff)} options={getDiffViewerOptions(isDark)} style={DIFFS_FONT_STYLE} + renderCustomHeader={(fileDiff) => ( + + )} /> ) : null, diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.stories.tsx b/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.stories.tsx index 92fc966d9e..a3295b5f63 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.stories.tsx +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.stories.tsx @@ -23,10 +23,15 @@ const longExecuteCommand = const TEST_PNG_B64 = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR4nGP4n539HwAHFwLVF8kc1wAAAABJRU5ErkJggg=="; -const getDiffsText = (element: HTMLElement) => - Array.from(element.querySelectorAll("diffs-container")) - .map((container) => container.shadowRoot?.textContent ?? "") - .join("\n"); +const expectDiffText = async (element: HTMLElement, text: string) => { + await waitFor(() => + expect( + Array.from(element.querySelectorAll("diffs-container")).some((host) => + host.shadowRoot?.textContent?.includes(text), + ), + ).toBe(true), + ); +}; const meta: Meta = { title: "pages/AgentsPage/ChatElements/tools/Tool", @@ -1266,11 +1271,7 @@ export const MCPToolCompleted: Story = { await userEvent.click(toggle); expect(canvas.getByText("Input")).toBeVisible(); expect(canvas.getByText("Output")).toBeVisible(); - await waitFor(() => { - const diffsText = getDiffsText(canvasElement); - expect(diffsText).toContain("backend"); - expect(diffsText).toContain("Fix auth flow"); - }); + await expectDiffText(canvasElement, "Fix auth flow"); }, }; @@ -1306,9 +1307,7 @@ export const MCPToolNoResult: Story = { const canvas = within(canvasElement); await userEvent.click(canvas.getByRole("button")); expect(canvas.getByText("Input")).toBeVisible(); - await waitFor(() => { - expect(getDiffsText(canvasElement)).toContain("New issue"); - }); + await expectDiffText(canvasElement, "New issue"); }, }; @@ -1392,11 +1391,8 @@ export const WorkspaceMCPToolCompleted: Story = { await userEvent.click(canvas.getByRole("button")); expect(canvas.getByText("Input")).toBeVisible(); expect(canvas.getByText("Output")).toBeVisible(); - await waitFor(() => { - const diffsText = getDiffsText(canvasElement); - expect(diffsText).toContain("message"); - expect(diffsText).toContain("hello from workspace MCP"); - }); + await expectDiffText(canvasElement, "message"); + await expectDiffText(canvasElement, "hello from workspace MCP"); }, }; @@ -2079,17 +2075,7 @@ export const ReadFileLongLine: Story = { await userEvent.click( canvas.getByRole("button", { name: /Read config.ts/i }), ); - await waitFor(() => - expect(getDiffsText(canvasElement)).toContain("apiUrl"), - ); - await waitFor(() => { - const host = canvasElement.querySelector("diffs-container"); - const code = host?.shadowRoot?.querySelector("[data-code]"); - expect(code).toBeInstanceOf(HTMLElement); - if (code instanceof HTMLElement) { - expect(getComputedStyle(code).overflow).toBe("visible"); - } - }); + await expectDiffText(canvasElement, "apiUrl"); }, }; @@ -2104,19 +2090,20 @@ export const ReadFileTallAndWide: Story = { await userEvent.click( canvas.getByRole("button", { name: /Read config.ts/i }), ); - await waitFor(() => - expect(getDiffsText(canvasElement)).toContain("apiUrl"), - ); - const viewport = [ - ...canvasElement.querySelectorAll( - "[data-radix-scroll-area-viewport]", - ), - ].find( - (v) => v.scrollWidth > v.clientWidth && v.scrollHeight > v.clientHeight, - ); - if (!viewport) { - throw new Error("Expected a viewport overflowing on both axes."); - } + await expectDiffText(canvasElement, "apiUrl"); + const viewport = await waitFor(() => { + const target = [ + ...canvasElement.querySelectorAll( + "[data-radix-scroll-area-viewport]", + ), + ].find( + (v) => v.scrollWidth > v.clientWidth && v.scrollHeight > v.clientHeight, + ); + if (!target) { + throw new Error("Expected a viewport overflowing on both axes."); + } + return target; + }); viewport.dispatchEvent( new WheelEvent("wheel", { deltaY: 200, bubbles: true, cancelable: true }), ); @@ -2136,9 +2123,7 @@ export const GenericToolLongOutput: Story = { await userEvent.click( canvas.getByRole("button", { name: /some_custom_tool/i }), ); - await waitFor(() => - expect(getDiffsText(canvasElement)).toContain("apiUrl"), - ); + await expectDiffText(canvasElement, "apiUrl"); }, }; diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx b/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx index 9aa8c36c1e..619a4b1929 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx @@ -12,6 +12,7 @@ import { import { ChatSummarizedTool } from "./ChatSummarizedTool"; import { ComputerTool } from "./ComputerTool"; import { CreateWorkspaceTool } from "./CreateWorkspaceTool"; +import { DiffFileHeader } from "./DiffFileHeader"; import { EditFilesTool } from "./EditFilesTool"; import { ExecuteAuthRequiredTool, @@ -828,7 +829,16 @@ const ToolFileViewer: FC = ({ label, file, options }) => ( scrollBarClassName="w-1.5" horizontalScrollBarClassName="h-1.5" > - + + } + /> ); diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/WriteFileTool.tsx b/site/src/pages/AgentsPage/components/ChatElements/tools/WriteFileTool.tsx index 6923e9b50c..54fb1a8381 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/WriteFileTool.tsx +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/WriteFileTool.tsx @@ -4,6 +4,7 @@ import { FileDiff } from "@pierre/diffs/react"; import type React from "react"; import type * as TypesGen from "#/api/typesGenerated"; import { ScrollArea } from "#/components/ScrollArea/ScrollArea"; +import { DiffFileHeader } from "./DiffFileHeader"; import { type AgentDisplayState, isAgentDisplayFullyExpanded, @@ -66,6 +67,9 @@ export const WriteFileTool: React.FC<{ fileDiff={stripNoNewline(diff)} options={getDiffViewerOptions(isDark)} style={DIFFS_FONT_STYLE} + renderCustomHeader={(fileDiff) => ( + + )} /> )} diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/utils.test.ts b/site/src/pages/AgentsPage/components/ChatElements/tools/utils.test.ts index a3785989d8..44ff5c75df 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/utils.test.ts +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/utils.test.ts @@ -990,24 +990,35 @@ describe("constants", () => { expect(DIFFS_FONT_STYLE).toHaveProperty("--diffs-line-height", "1.5"); }); - it("fileViewerCSS is a non-empty string", () => { - expect(typeof fileViewerCSS).toBe("string"); - expect(fileViewerCSS.length).toBeGreaterThan(0); + it("DIFFS_FONT_STYLE uses theme-aware diff variables", () => { + expect(DIFFS_FONT_STYLE).toHaveProperty( + "--diffs-addition-color-override", + "hsl(var(--git-added))", + ); + expect(DIFFS_FONT_STYLE).toHaveProperty( + "--diffs-deletion-color-override", + "hsl(var(--git-deleted))", + ); + expect(DIFFS_FONT_STYLE).toHaveProperty( + "--diffs-bg-addition-override", + "hsl(var(--surface-git-added))", + ); + expect(DIFFS_FONT_STYLE).toHaveProperty( + "--diffs-bg-deletion-override", + "hsl(var(--surface-git-deleted))", + ); }); - it("diffViewerCSS includes border-left style", () => { - expect(diffViewerCSS).toContain("border-left"); + it("fileViewerCSS keeps file viewer backgrounds transparent", () => { + expect(fileViewerCSS).toContain("background-color: transparent"); + expect(fileViewerCSS).toContain("[data-diffs-header]"); + expect(fileViewerCSS).not.toContain("[data-code]"); }); - it("diffViewerCSS uses theme-aware changed line backgrounds", () => { - expect(diffViewerCSS).toContain("--diffs-addition-color-override"); - expect(diffViewerCSS).toContain("--diffs-deletion-color-override"); - expect(diffViewerCSS).toContain("--diffs-bg-addition-override"); - expect(diffViewerCSS).toContain("--diffs-bg-deletion-override"); - expect(diffViewerCSS).toContain("var(--surface-git-added)"); - expect(diffViewerCSS).toContain("var(--surface-git-deleted)"); - expect(diffViewerCSS).toContain("[data-line-type='change-addition']"); - expect(diffViewerCSS).toContain("[data-line-type='change-deletion']"); + it("diffViewerCSS keeps hunk separator styling scoped", () => { + expect(diffViewerCSS).toContain("[data-separator='line-info']"); + expect(diffViewerCSS).toContain("[data-separator-content]"); + expect(diffViewerCSS).not.toContain("[data-diffs-header]"); }); }); @@ -1063,6 +1074,27 @@ describe("parseServerEditResults", () => { }); describe("parseServerEditDiffText", () => { + const changedLineContents = ( + diff: NonNullable>, + ) => + diff.hunks.flatMap((hunk) => + hunk.hunkContent.flatMap((content) => { + if (content.type !== "change") { + return []; + } + return [ + ...diff.deletionLines.slice( + content.deletionLineIndex, + content.deletionLineIndex + content.deletions, + ), + ...diff.additionLines.slice( + content.additionLineIndex, + content.additionLineIndex + content.additions, + ), + ].map((line) => line.trimEnd()); + }), + ); + it("returns null for an empty string (no-op edit)", () => { expect(parseServerEditDiffText("")).toBeNull(); }); @@ -1074,6 +1106,46 @@ describe("parseServerEditDiffText", () => { expect(diff).not.toBeNull(); expect(diff?.name).toBe("/abs/a.txt"); }); + + it("parses quoted git diff headers", () => { + const diff = parseServerEditDiffText( + [ + 'diff --git "a/path with spaces.ts" "b/path with spaces.ts"', + "index 1111111..2222222 100644", + '--- "a/path with spaces.ts"', + '+++ "b/path with spaces.ts"', + "@@ -1 +1 @@", + "-old value", + "+new value", + "", + ].join("\n"), + ); + + expect(diff).not.toBeNull(); + expect(diff?.name).toBe("path with spaces.ts"); + expect(changedLineContents(diff!)).toEqual(["old value", "new value"]); + }); + + it("parses diffs that include git patch footer metadata", () => { + const diff = parseServerEditDiffText( + [ + "diff --git a/example.ts b/example.ts", + "index 1111111..2222222 100644", + "--- a/example.ts", + "+++ b/example.ts", + "@@ -1 +1 @@", + "-old value", + "+new value", + "-- ", + "2.45.0", + "", + ].join("\n"), + ); + + expect(diff).not.toBeNull(); + expect(diff?.name).toBe("example.ts"); + expect(changedLineContents(diff!)).toEqual(["old value", "new value"]); + }); }); describe("summarizeParsedCommands", () => { diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts b/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts index 2995058fe6..b8a4314525 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts @@ -291,52 +291,11 @@ export const formatResultOutput = (result: unknown): string | null => { export const fileViewerCSS = [ "pre, [data-line], [data-diffs-header] { background-color: transparent !important; }", - "[data-code] { overflow: visible !important; }", -].join(" "); - -// Selection override CSS maps the library's gold/yellow selection -// palette to the Coder blue accent (`--content-link`) so line -// highlighting feels native to the rest of the page. -// -// The library has two selection code paths: context lines use -// `--diffs-bg-selection`, but change-addition/deletion lines -// use a separate `color-mix()` against `--diffs-line-bg`. To -// guarantee a uniform highlight across all line types we set -// the CSS variables for annotations AND apply direct rules -// with `!important` for line and gutter elements. -const SELECTION_OVERRIDE_CSS = [ - // Variable overrides for annotation areas and library internals. - ":host {", - " --diffs-bg-selection-override: hsl(var(--content-link) / 0.08);", - " --diffs-bg-selection-number-override: hsl(var(--content-link) / 0.13);", - " --diffs-selection-number-fg: hsl(var(--content-link));", - " --diffs-gap-style: 1px solid hsl(var(--border-default));", - "}", - // Direct rules that override both context and change-line - // selection backgrounds so every selected line looks the same. - "[data-selected-line][data-line] {", - " background-color: hsl(var(--content-link) / 0.08) !important;", - "}", - "[data-selected-line][data-column-number] {", - " background-color: hsl(var(--content-link) / 0.13) !important;", - " color: hsl(var(--content-link)) !important;", - "}", - // Clear the selection tint from annotation rows so the inline - // prompt input stands out clearly against the selected lines. - "[data-line-annotation][data-selected-line] [data-annotation-content] {", - " background-color: transparent !important;", - "}", - "[data-line-annotation][data-selected-line]::before {", - " background-color: transparent !important;", - "}", - "[data-selected-line][data-gutter-buffer='annotation'] {", - " background-color: transparent !important;", - "}", ].join(" "); // Restyled separators: quiet, full-width dividers that fade // into the background instead of drawing attention. -const SEPARATOR_CSS = [ +export const SEPARATOR_CSS = [ // Transparent backgrounds so separators blend with the // code area rather than forming a distinct band. ":host {", @@ -379,119 +338,8 @@ const SEPARATOR_CSS = [ "}", ].join(" "); -// Shared header styling applied to all diff viewers (both the -// conversation-inline diffs and the right-tab panel). This gives -// every diff header the same font sizing, change-type badges, -// and stat-count pills regardless of where it appears. -const DIFF_HEADER_CSS = [ - // Header layout: consistent sizing and padding across contexts. - "[data-diffs-header] {", - " font-size: 13px;", - " min-height: 32px !important;", - " padding-block: 8px !important;", - " padding-inline: 10px 6px !important;", - " border-bottom: 1px solid hsl(var(--border-default));", - "}", - - // Title text: sans-serif, slightly smaller than header chrome. - "[data-diffs-header] [data-title] {", - " font-size: 12px;", - " color: hsl(var(--content-primary));", - "}", - - // Replace the library's built-in SVG change-type icons with - // single-letter badges (A/D/M/R) via CSS-generated content. - "[data-change-icon] { display: none !important; }", - // Baseline-align the badge letter with the filename so their - // text baselines match despite different font sizes (11px vs - // 12px). Without this the box-centering default shifts the - // badge a fraction of a pixel above the title. - "[data-diffs-header] [data-header-content] { align-items: baseline; overflow: hidden; }", - "[data-diffs-header] [data-rename-icon] { align-self: center; }", - "[data-diffs-header] [data-header-content]::before {", - " font-size: 11px;", - " font-weight: 600;", - " flex-shrink: 0;", - "}", - "[data-diffs-header][data-change-type='new'] [data-header-content]::before {", - " content: 'A';", - " color: hsl(var(--git-added));", - "}", - "[data-diffs-header][data-change-type='change'] [data-header-content]::before {", - " content: 'M';", - " color: hsl(var(--git-modified));", - "}", - "[data-diffs-header][data-change-type='deleted'] [data-header-content]::before {", - " content: 'D';", - " color: hsl(var(--git-deleted));", - "}", - "[data-diffs-header][data-change-type='rename-pure'] [data-header-content]::before,", - "[data-diffs-header][data-change-type='rename-changed'] [data-header-content]::before {", - " content: 'R';", - " color: hsl(var(--git-modified));", - "}", - - // Stat counts styled as compact pill badges. - "[data-diffs-header] [data-metadata] {", - " flex-shrink: 0;", - " flex-direction: row-reverse;", - " align-items: stretch;", - " gap: 0 !important;", - " padding: 0;", - " border: 1px solid hsl(var(--border-default));", - " border-radius: 3px;", - " overflow: hidden;", - "}", - "[data-diffs-header] [data-additions-count],", - "[data-diffs-header] [data-deletions-count] {", - " font-family: var(--diffs-font-family, var(--diffs-font-fallback));", - " font-size: 12px;", - " font-weight: 500;", - " line-height: 20px;", - " padding-inline: 4px;", - " border-radius: 0;", - "}", - "[data-diffs-header] [data-additions-count] {", - " color: hsl(var(--git-added-bright)) !important;", - " background-color: hsl(var(--surface-git-added));", - "}", - "[data-diffs-header] [data-deletions-count] {", - " color: hsl(var(--git-deleted-bright)) !important;", - " background-color: hsl(var(--surface-git-deleted));", - "}", -].join(" "); - -const CHANGE_LINE_CSS = [ - ":host {", - " --diffs-addition-color-override: hsl(var(--git-added));", - " --diffs-deletion-color-override: hsl(var(--git-deleted));", - " --diffs-bg-addition-override: hsl(var(--surface-git-added));", - " --diffs-bg-deletion-override: hsl(var(--surface-git-deleted));", - " --diffs-bg-addition-number-override: hsl(var(--surface-git-added));", - " --diffs-bg-deletion-number-override: hsl(var(--surface-git-deleted));", - "}", - "[data-line-type='change-addition']:not([data-selected-line]) {", - " background-color: hsl(var(--surface-git-added)) !important;", - "}", - "[data-line-type='change-deletion']:not([data-selected-line]) {", - " background-color: hsl(var(--surface-git-deleted)) !important;", - "}", -].join(" "); - export const diffViewerCSS = [ - // Make context lines transparent so they blend with the page, - // while changed lines use the same theme-aware git surfaces as - // the file headers and stats. - "pre, [data-line]:not([data-selected-line]):not([data-line-type='change-addition']):not([data-line-type='change-deletion']), [data-diffs-header] { background-color: transparent !important; }", - "[data-diffs-header] { border-left: 1px solid var(--border); }", - // The library reserves a 6 px horizontal scrollbar track on - // [data-code] via overflow: scroll clip. In wrap mode lines - // never overflow, so hide the track to remove the phantom gap. - "[data-code] { scrollbar-width: none !important; }", - "[data-code]::-webkit-scrollbar { height: 0 !important; }", - DIFF_HEADER_CSS, - CHANGE_LINE_CSS, - SELECTION_OVERRIDE_CSS, + "pre, [data-line]:not([data-line-type='change-addition']):not([data-line-type='change-deletion']) { background-color: transparent !important; }", SEPARATOR_CSS, ].join(" "); @@ -566,6 +414,16 @@ export const DIFFS_FONT_STYLE = { "--diffs-header-font-family": '"Geist Variable", system-ui, sans-serif', "--diffs-font-size": "11px", "--diffs-line-height": "1.5", + "--diffs-addition-color-override": "hsl(var(--git-added))", + "--diffs-deletion-color-override": "hsl(var(--git-deleted))", + "--diffs-bg-addition-override": "hsl(var(--surface-git-added))", + "--diffs-bg-deletion-override": "hsl(var(--surface-git-deleted))", + "--diffs-bg-addition-number-override": "hsl(var(--surface-git-added))", + "--diffs-bg-deletion-number-override": "hsl(var(--surface-git-deleted))", + "--diffs-bg-selection-override": "hsl(var(--content-link) / 0.08)", + "--diffs-bg-selection-number-override": "hsl(var(--content-link) / 0.13)", + "--diffs-selection-number-fg": "hsl(var(--content-link))", + "--diffs-gap-style": "1px solid hsl(var(--border-default))", } as CSSProperties; /** diff --git a/site/src/pages/AgentsPage/components/DiffViewer/CommentableDiffViewer.tsx b/site/src/pages/AgentsPage/components/DiffViewer/CommentableDiffViewer.tsx index c2fa76a22b..68e8ecb91d 100644 --- a/site/src/pages/AgentsPage/components/DiffViewer/CommentableDiffViewer.tsx +++ b/site/src/pages/AgentsPage/components/DiffViewer/CommentableDiffViewer.tsx @@ -210,6 +210,10 @@ export const CommentableDiffViewer: FC = ({ // --------------------------------------------------------------- const [activeCommentBox, setActiveCommentBox] = useState(null); + const [previewSelection, setPreviewSelection] = useState<{ + fileName: string; + range: SelectedLineRange; + } | null>(null); const activeCommentBoxRef = useRef(null); @@ -228,6 +232,7 @@ export const CommentableDiffViewer: FC = ({ annotationSide: "additions" | "deletions"; }, ) => { + setPreviewSelection(null); updateCommentBox({ fileName, start: props.lineNumber, @@ -239,18 +244,21 @@ export const CommentableDiffViewer: FC = ({ const handleLineSelected = ( fileName: string, - range: { - start: number; - end: number; - side?: "additions" | "deletions"; - endSide?: "additions" | "deletions"; - } | null, + range: SelectedLineRange | null, ) => { const result = commentBoxFromRange(fileName, range); if (result === "ignore") return; + setPreviewSelection(null); updateCommentBox(result); }; + const handleLineSelectionChange = ( + fileName: string, + range: SelectedLineRange | null, + ) => { + setPreviewSelection(range ? { fileName, range } : null); + }; + // --------------------------------------------------------------- // Annotation helpers // --------------------------------------------------------------- @@ -273,10 +281,14 @@ export const CommentableDiffViewer: FC = ({ if (activeCommentBox && activeCommentBox.fileName === fileName) { return selectedLinesForBox(activeCommentBox); } + if (previewSelection && previewSelection.fileName === fileName) { + return previewSelection.range; + } return null; }; const handleCancelComment = () => { + setPreviewSelection(null); updateCommentBox(null); }; @@ -327,6 +339,7 @@ export const CommentableDiffViewer: FC = ({ parsedFiles={parsedFiles} onLineNumberClick={handleLineNumberClick} onLineSelected={handleLineSelected} + onLineSelectionChange={handleLineSelectionChange} getLineAnnotations={getLineAnnotations} getSelectedLines={getSelectedLines} renderAnnotation={renderAnnotation} diff --git a/site/src/pages/AgentsPage/components/DiffViewer/DiffViewer.stories.tsx b/site/src/pages/AgentsPage/components/DiffViewer/DiffViewer.stories.tsx index 2025291bf3..140711ccae 100644 --- a/site/src/pages/AgentsPage/components/DiffViewer/DiffViewer.stories.tsx +++ b/site/src/pages/AgentsPage/components/DiffViewer/DiffViewer.stories.tsx @@ -446,42 +446,11 @@ export const LargeDiff: Story = { ), ], play: async ({ canvasElement }) => { - // Wait for the file tree sidebar to render, proving that - // isExpanded activates the tree + observer code path. + // The @pierre/trees file tree mounts a `file-tree-container` custom + // element once the sidebar is shown (isExpanded). Assert it appears. await waitFor(() => { - const nav = canvasElement.querySelector("nav"); - expect(nav).not.toBeNull(); - }); - - // Find the diff content viewport (the one containing file - // sections) rather than the file-tree sidebar viewport. - const fileSection = canvasElement.querySelector("[data-file-name]"); - const viewport = fileSection?.closest( - "[data-radix-scroll-area-viewport]", - ); - if (!viewport) throw new Error("diff viewport not found"); - - // Capture the initial active file (whichever the observer picked - // up at mount), then scroll and verify it changed. - let initialFile: string | undefined; - await waitFor(() => { - const btn = canvasElement.querySelector( - 'nav button[aria-current="true"]', - ); - expect(btn).not.toBeNull(); - initialFile = btn!.title; - }); - - // Scroll to roughly the middle of the diff content. - viewport.scrollTop = viewport.scrollHeight / 2; - - // The observer should fire and highlight a different file. - await waitFor(() => { - const btn = canvasElement.querySelector( - 'nav button[aria-current="true"]', - ); - expect(btn).not.toBeNull(); - expect(btn!.title).not.toBe(initialFile); + const tree = canvasElement.querySelector("file-tree-container"); + expect(tree).not.toBeNull(); }); }, }; diff --git a/site/src/pages/AgentsPage/components/DiffViewer/DiffViewer.tsx b/site/src/pages/AgentsPage/components/DiffViewer/DiffViewer.tsx index 8e6267bfd3..1be5667847 100644 --- a/site/src/pages/AgentsPage/components/DiffViewer/DiffViewer.tsx +++ b/site/src/pages/AgentsPage/components/DiffViewer/DiffViewer.tsx @@ -1,127 +1,324 @@ import { useTheme } from "@emotion/react"; import type { + CodeViewHandle, + CodeViewItem, DiffLineAnnotation, FileDiffMetadata, SelectedLineRange, VirtualFileMetrics, -} from "@pierre/diffs"; -import { Virtualizer } from "@pierre/diffs"; -import { FileDiff, VirtualizerContext } from "@pierre/diffs/react"; -import { ChevronRightIcon } from "lucide-react"; +} from "@pierre/diffs/react"; +import { CodeView } from "@pierre/diffs/react"; +import type { FileTreeSortComparator, GitStatusEntry } from "@pierre/trees"; +import { FileTree, useFileTree } from "@pierre/trees/react"; import { type ComponentProps, + type CSSProperties, type FC, - memo, + Fragment, type ReactNode, - useCallback, useEffect, useRef, useState, } from "react"; import { ErrorAlert } from "#/components/Alert/ErrorAlert"; -import { FileIcon } from "#/components/FileIcon/FileIcon"; -import { ScrollArea } from "#/components/ScrollArea/ScrollArea"; import { Skeleton } from "#/components/Skeleton/Skeleton"; import { cn } from "#/utils/cn"; +import { countChangedLines } from "../../utils/countChangedLines"; import { changeColor, changeLabel } from "../../utils/diffColors"; -import { - DIFFS_FONT_STYLE, - getDiffViewerOptions, -} from "../ChatElements/tools/utils"; +import { SEPARATOR_CSS } from "../ChatElements/tools/utils"; import { useActiveFileTracking } from "./useActiveFileTracking"; -// ------------------------------------------------------------------- -// Public interface -// ------------------------------------------------------------------- - interface DiffViewerProps { - /** Parsed file diffs to render. */ parsedFiles: readonly FileDiffMetadata[]; - /** Cache key prefix for parsePatchFiles worker pool LRU cache. */ - cacheKeyPrefix?: string; - /** Whether the panel is in expanded mode (affects file tree threshold). */ isExpanded?: boolean; - /** Loading state. */ isLoading?: boolean; - /** Error state. */ error?: unknown; - /** Empty state message. */ emptyMessage?: string; - /** Which diff rendering style to use. */ diffStyle: DiffStyle; - /** - * Called when a line number gutter element is clicked. - * Receives the file name and click metadata. - */ onLineNumberClick?: ( fileName: string, props: { lineNumber: number; annotationSide: "additions" | "deletions" }, ) => void; - /** - * Called when a range of lines is selected (e.g. shift-click). - * Receives the file name and the selected range (or null on - * deselection). - */ - onLineSelected?: ( + /** Fires when a line selection is committed (e.g. on pointer up). */ + onLineSelected?: (fileName: string, range: SelectedLineRange | null) => void; + /** Fires continuously as the selection range changes during a drag. */ + onLineSelectionChange?: ( fileName: string, - range: { - start: number; - end: number; - side?: "additions" | "deletions"; - endSide?: "additions" | "deletions"; - } | null, + range: SelectedLineRange | null, ) => void; - /** - * Returns line annotations for the given file. Used to render - * inline widgets such as comment inputs. - */ getLineAnnotations?: (fileName: string) => DiffLineAnnotation[]; - /** - * Returns the selected line range for the given file, if any. - * Used to visually highlight the lines being commented on. - */ getSelectedLines?: (fileName: string) => SelectedLineRange | null; - /** - * Renderer for line annotations returned by `getLineAnnotations`. - */ renderAnnotation?: (annotation: DiffLineAnnotation) => ReactNode; - /** - * When set to a file name, DiffViewer scrolls to that file and - * highlights it in the tree. The parent should reset this to - * null via `onScrollToFileComplete` after the scroll completes. - */ scrollToFile?: string | null; - /** Called after scrollToFile has been processed. */ onScrollToFileComplete?: () => void; } -// ------------------------------------------------------------------- -// Constants -// ------------------------------------------------------------------- +export type DiffStyle = "unified" | "split"; +const DIFF_STYLE_KEY = "agents.diff-view-style"; -/** - * Minimum container width (px) at which the file tree sidebar - * is shown alongside the diff list. - */ +const DIFF_VIEWER_LINE_HEIGHT = 16.5; +const DIFF_HEADER_HEIGHT = 32; +const HUNK_SEPARATOR_HEIGHT = 28; + +// Minimum width (px) of the diff container at which the file tree sidebar is +// shown alongside the diff. Below this the diff takes the full width unless the +// viewer is explicitly expanded. const FILE_TREE_THRESHOLD = 1000; -/** - * Extra CSS injected via the diff viewer's `unsafeCSS` option to make - * file headers sticky with a solid background. The shared header - * styling (font sizing, change-type badges, stat-count pills) lives - * in `diffViewerCSS` from utils.ts and is already included in the - * base options returned by `getDiffViewerOptions`. - */ -const STICKY_HEADER_CSS = [ - "[data-diffs-header] {", - " position: sticky; top: 0; z-index: 10;", - " background-color: hsl(var(--surface-secondary)) !important;", - " padding-block: 0 !important;", +const diffViewerStyle = { + "--diffs-font-family": '"Geist Mono Variable", monospace, monospace', + "--diffs-header-font-family": '"Geist Variable", system-ui, sans-serif', + "--diffs-font-size": "11px", + "--diffs-line-height": `${DIFF_VIEWER_LINE_HEIGHT}px`, +} satisfies CSSProperties; + +const diffViewerMetrics: Partial = { + diffHeaderHeight: DIFF_HEADER_HEIGHT, + hunkSeparatorHeight: HUNK_SEPARATOR_HEIGHT, + lineHeight: DIFF_VIEWER_LINE_HEIGHT, +}; + +const fileTreeStyle = { + height: "100%", + "--trees-font-family-override": '"Geist Variable", system-ui, sans-serif', + "--trees-font-size-override": "13px", + "--trees-border-color-override": "hsl(var(--border-default))", + "--trees-bg-override": "hsl(var(--surface-primary))", + "--trees-fg-override": "hsl(var(--content-primary))", + "--trees-muted-fg-override": "hsl(var(--content-secondary))", + "--trees-selected-bg-override": "hsl(var(--surface-secondary))", + "--trees-padding-inline-override": "0px", + "--trees-item-margin-x-override": "0px", + "--trees-border-radius-override": "0px", + "--trees-git-added-color-override": "hsl(var(--git-added))", + "--trees-git-deleted-color-override": "hsl(var(--git-deleted))", + "--trees-git-modified-color-override": "hsl(var(--git-modified))", + "--trees-git-renamed-color-override": "hsl(var(--git-modified))", +} satisfies CSSProperties; + +// Single full-path ordering rule shared by the sidebar tree and the flat diff +// list so the two cannot drift apart. useFileTree applies the sort comparator +// to the whole flat set of path entries (directories included), so it must be +// a full-path comparator, not a basename one: at the first differing segment a +// directory sorts before a file, dot-prefixed names first, then +// case-insensitive locale order; a shorter path sorts before its descendants. +function compareTreeEntries( + aSegments: readonly string[], + aIsDirectory: boolean, + bSegments: readonly string[], + bIsDirectory: boolean, +): number { + const shared = Math.min(aSegments.length, bSegments.length); + for (let i = 0; i < shared; i++) { + const aSegment = aSegments[i]; + const bSegment = bSegments[i]; + if (aSegment === bSegment) { + continue; + } + // A segment is a directory when it is not the last one, or when the + // entry itself is a directory whose final segment this is. + const aSegmentIsDir = i < aSegments.length - 1 || aIsDirectory; + const bSegmentIsDir = i < bSegments.length - 1 || bIsDirectory; + if (aSegmentIsDir !== bSegmentIsDir) { + return aSegmentIsDir ? -1 : 1; + } + const aIsDot = aSegment.charCodeAt(0) === 46; + const bIsDot = bSegment.charCodeAt(0) === 46; + if (aIsDot !== bIsDot) { + return aIsDot ? -1 : 1; + } + return aSegment.toLowerCase().localeCompare(bSegment.toLowerCase()); + } + if (aSegments.length !== bSegments.length) { + return aSegments.length - bSegments.length; + } + if (aIsDirectory === bIsDirectory) { + return 0; + } + return aIsDirectory ? -1 : 1; +} + +// Passed to useFileTree so the sidebar uses compareTreeEntries rather than the +// library's undocumented default sort. The entry carries the full segment +// list. Exported for unit tests that verify the tree order matches the diff. +export const treeSortComparator: FileTreeSortComparator = (left, right) => + compareTreeEntries( + left.segments, + left.isDirectory, + right.segments, + right.isDirectory, + ); + +// Orders the flat diff list to the sidebar tree's leaf order so scrolling moves +// monotonically down the tree. Diff entries are always files, so it runs the +// same compareTreeEntries the tree uses, keeping the two orders identical by +// construction. Exported for unit tests that pin the shared ordering. +export function compareTreePaths(a: string, b: string): number { + return compareTreeEntries(a.split("/"), false, b.split("/"), false); +} + +// CodeView's syncItemRecord skips reusing a record when item.version is +// unchanged, so the version must reflect annotation content rather than count. +// Moving the active comment box to another line in the same file keeps the +// count at 1 but must still re-render, so fold each annotation's side and line +// into the version. Exported for unit tests. +export function annotationsVersion( + annotations: readonly DiffLineAnnotation[] | undefined, +): number { + if (!annotations || annotations.length === 0) { + return 0; + } + return annotations.reduce( + (version, annotation) => + version * 31 + + annotation.lineNumber * 2 + + (annotation.side === "additions" ? 1 : 0), + annotations.length, + ); +} + +// The library forces classic, space-reserving scrollbars via +// scrollbar-gutter: stable, leaving a permanent empty strip on the right. +// Restore the default gutter so the scrollbar overlays content while scrolling +// instead of reserving width, and keep it slim when it does appear. +const fileTreeUnsafeCSS = [ + "[data-file-tree-virtualized-scroll='true'] {", + " scrollbar-gutter: auto !important;", + " scrollbar-width: thin;", "}", ].join(" "); -export type DiffStyle = "unified" | "split"; -const DIFF_STYLE_KEY = "agents.diff-view-style"; +function gitStatusForFile( + fileDiff: FileDiffMetadata, +): GitStatusEntry["status"] { + switch (fileDiff.type) { + case "new": + return "added"; + case "deleted": + return "deleted"; + case "rename-pure": + case "rename-changed": + return "renamed"; + default: + return "modified"; + } +} + +function HeaderContent({ fileDiff }: { fileDiff: FileDiffMetadata }) { + const { additions, deletions } = countChangedLines(fileDiff); + return ( +
+
+ + {changeLabel(fileDiff.type)} + + {fileDiff.prevName && fileDiff.prevName !== fileDiff.name && ( + + {fileDiff.prevName} + + )} + + {fileDiff.name} + +
+ {(additions > 0 || deletions > 0) && ( + + {deletions > 0 && ( + + −{deletions} + + )} + {additions > 0 && ( + + +{additions} + + )} + + )} +
+ ); +} + +function DiffFileTree({ + files, + activePath, + onUserSelectPath, +}: { + files: readonly FileDiffMetadata[]; + activePath: string | null; + onUserSelectPath: (fileName: string) => void; +}) { + const paths = files.map((file) => file.name); + const gitStatus = files.map((file) => ({ + path: file.name, + status: gitStatusForFile(file), + })); + // Tracks the row currently selected in the model so scroll-driven syncing + // can skip redundant selects. + const selectedPathRef = useRef(null); + // Set while we replace the selection programmatically. The model emits + // selection events synchronously, so this drops the echoes from our own + // deselect/select calls in one window instead of looping back into + // onUserSelectPath. A user click can only land between effect runs, when + // the flag is already false. + const isSyncingSelectionRef = useRef(false); + const { model } = useFileTree({ + density: "compact", + flattenEmptyDirectories: false, + gitStatus, + initialExpansion: "open", + sort: treeSortComparator, + unsafeCSS: fileTreeUnsafeCSS, + onSelectionChange: (selectedPaths) => { + const selectedPath = selectedPaths.at(-1) ?? null; + selectedPathRef.current = selectedPath; + if (isSyncingSelectionRef.current) { + return; + } + if (selectedPath) { + onUserSelectPath(selectedPath); + } + }, + paths, + }); + + useEffect(() => { + model.resetPaths(paths); + model.setGitStatus(gitStatus); + }, [gitStatus, model, paths]); + + // Drive the tree selection from the active file. The @pierre/trees item + // handle's select() is additive, so a scrolled-past file stays + // highlighted unless its selection is cleared. Replace the whole + // selection with just the active file, suppressing the echoed events. + useEffect(() => { + if (!activePath || selectedPathRef.current === activePath) return; + const item = model.getItem(activePath); + if (!item) return; + isSyncingSelectionRef.current = true; + for (const path of model.getSelectedPaths()) { + if (path !== activePath) { + model.getItem(path)?.deselect(); + } + } + item.select(); + isSyncingSelectionRef.current = false; + selectedPathRef.current = activePath; + }, [activePath, model]); + + return ( + + ); +} export function loadDiffStyle(): DiffStyle { const stored = localStorage.getItem(DIFF_STYLE_KEY); @@ -135,383 +332,76 @@ export function saveDiffStyle(style: DiffStyle): void { localStorage.setItem(DIFF_STYLE_KEY, style); } -/** Width of the file tree sidebar in pixels. */ -const FILE_TREE_WIDTH = 300; - -// ------------------------------------------------------------------- -// Estimated diff height for lazy loading -// ------------------------------------------------------------------- - -/** - * Estimated height per line in the diff viewer (px). The - * library's shadow DOM applies box-sizing: border-box to all - * elements, and code lines have no padding or border, so the - * rendered height equals the CSS line-height: 11px × 1.5 = 16.5. - */ -const LINE_HEIGHT_PX = 16.5; - -/** Height of the file header row rendered by @pierre/diffs. */ -const HEADER_HEIGHT_PX = 36; - -/** - * Metrics that tell the @pierre/diffs virtualizer how tall each - * element actually is after our CSS overrides. Without these the - * library falls back to its built-in defaults (20 px lines, - * 44 px headers, 32 px separators) which are larger than our - * custom styling, causing visible blank buffers in the viewport. - */ -const VIRTUALIZER_METRICS: VirtualFileMetrics = { - hunkLineCount: 50, - lineHeight: LINE_HEIGHT_PX, - diffHeaderHeight: 32, // 32 px min-height (border-box includes borders) - hunkSeparatorHeight: 28, // height: 28px !important in SEPARATOR_CSS - fileGap: 2, // padding-bottom: max(0, gap-block - 6px) = 2px -}; - -/** - * Estimate the rendered pixel height of a file diff so the - * placeholder occupies roughly the same space. This keeps the - * scroll position stable as files are lazily mounted. - */ -function estimateDiffHeight(fileDiff: FileDiffMetadata): number { - return HEADER_HEIGHT_PX + fileDiff.unifiedLineCount * LINE_HEIGHT_PX; -} - -// ------------------------------------------------------------------- -// File tree data model -// ------------------------------------------------------------------- - -interface FileTreeNode { - name: string; - fullPath: string; - type: "file" | "directory"; - children: FileTreeNode[]; - fileDiff?: FileDiffMetadata; -} - -/** - * Builds a nested tree from a flat list of file diffs. Directory - * nodes are created for every intermediate path segment. The - * result is sorted with directories first, then alphabetically. - * Single-child directory chains are collapsed so that e.g. - * `src/pages/AgentsPage` renders as one row. - */ -function buildFileTree(files: readonly FileDiffMetadata[]): FileTreeNode[] { - const root: FileTreeNode[] = []; - - for (const file of files) { - const segments = file.name.split("/"); - let children = root; - - // Walk / create intermediate directory nodes. - for (let i = 0; i < segments.length - 1; i++) { - const seg = segments[i]; - let dir = children.find((n) => n.type === "directory" && n.name === seg); - if (!dir) { - dir = { - name: seg, - fullPath: segments.slice(0, i + 1).join("/"), - type: "directory", - children: [], - }; - children.push(dir); - } - children = dir.children; - } - - // Leaf file node. - const fileName = segments[segments.length - 1]; - children.push({ - name: fileName, - fullPath: file.name, - type: "file", - children: [], - fileDiff: file, - }); - } - - const sortNodes = (nodes: FileTreeNode[]): FileTreeNode[] => { - for (const node of nodes) { - if (node.children.length > 0) { - node.children = sortNodes(node.children); - } - } - return nodes.sort((a, b) => { - if (a.type !== b.type) { - return a.type === "directory" ? -1 : 1; - } - return a.name.localeCompare(b.name); - }); - }; - - // Collapse single-child directory chains into one node whose - // name uses path separators, e.g. "src/pages/AgentsPage". - const collapse = (nodes: FileTreeNode[]): FileTreeNode[] => { - for (const node of nodes) { - if (node.type === "directory") { - node.children = collapse(node.children); - // If this directory has exactly one child and it is also - // a directory, merge them. - while ( - node.children.length === 1 && - node.children[0].type === "directory" - ) { - const child = node.children[0]; - node.name = `${node.name}/${child.name}`; - node.fullPath = child.fullPath; - node.children = child.children; - } - } - } - return nodes; - }; - - return collapse(sortNodes(root)); -} - -// ------------------------------------------------------------------- -// Tree node renderer -// ------------------------------------------------------------------- - -interface FileTreeNodeViewProps { - node: FileTreeNode; - depth: number; - activeFile: string | null; - onFileClick: (fullPath: string) => void; -} - -function FileTreeNodeViewInner({ - node, - depth, - activeFile, - onFileClick, -}: FileTreeNodeViewProps) { - const [expanded, setExpanded] = useState(true); - - if (node.type === "directory") { - return ( -
- - {expanded && - node.children.map((child) => ( - - ))} -
- ); - } - - const isActive = activeFile === node.fullPath; - +// The loading state mirrors the real diff layout: flat, full-width +// file headers with a change badge and stat pill, gutter-aligned code +// lines, and centered hunk separators. Keeping the same shape avoids a +// jarring swap when the parsed diff replaces the placeholder. +function SkeletonLine({ width }: { width: string }) { return ( - + + + ); } -// memo requires stable props; the React Compiler provides them here. -const FileTreeNodeView = memo(FileTreeNodeViewInner); - -// ------------------------------------------------------------------- -// Virtualized scroll container -// ------------------------------------------------------------------- - -/** - * Wraps the diff list in a Radix ScrollArea and wires up the - * @pierre/diffs Virtualizer. Extracted into its own component so - * that the useCallback for ref-callback stability lives here - * instead of in DiffViewer. This lets the React Compiler skip - * only this small wrapper (where it can't preserve useCallback) - * while still optimizing the much larger parent. - * - * The ref callback is placed on a content div *inside* the - * ScrollArea, then walks up with closest() to the Radix viewport. - * React fires children's refs bottom-up during commit, so every - * VirtualizedFileDiff instance has already connected to the - * virtualizer by the time this ref fires and calls setup(). - */ -const DiffScrollContainer: FC<{ - children: ReactNode; - className?: string; - diffViewportRef: React.RefObject; -}> = ({ children, className, diffViewportRef }) => { - const [virtualizer] = useState(() => new Virtualizer()); - - // useCallback is required for correctness: in React 19 an - // unstable ref callback triggers old-cleanup → new-callback - // on every render, which calls virtualizer.cleanUp() and - // wipes the observer map. The compiler can't preserve this - // useCallback, but that only causes it to skip this small - // wrapper, not the entire DiffViewer. - const contentRef = useCallback( - (node: HTMLDivElement | null) => { - const viewport = node?.closest( - "[data-radix-scroll-area-viewport]", - ); - if (!viewport) return; - - diffViewportRef.current = viewport; - virtualizer.setup(viewport); - - return () => { - virtualizer.cleanUp(); - diffViewportRef.current = null; - }; - }, - [virtualizer, diffViewportRef], - ); - +function SkeletonSeparator() { return ( - - -
- {children} +
+
+ +
+
+ ); +} + +function SkeletonFile({ groups }: { groups: readonly (readonly string[])[] }) { + return ( +
+
+
+ +
- - - ); -}; - -// ------------------------------------------------------------------- -// Lazy file diff wrapper -// ------------------------------------------------------------------- - -/** - * Wraps a single `` with an IntersectionObserver so the - * heavy component (Shadow DOM + shiki highlighting) is only mounted - * once the placeholder scrolls into or near the viewport. - * - * Once mounted the component stays mounted. We never unmount a - * FileDiff that the user has already scrolled past, which avoids - * layout shifts and repeated highlighting work. - */ -interface LazyFileDiffProps { - fileDiff: FileDiffMetadata; - options: ComponentProps["options"]; - lineAnnotations?: DiffLineAnnotation[]; - renderAnnotation?: (annotation: DiffLineAnnotation) => ReactNode; - selectedLines?: SelectedLineRange | null; -} - -function LazyFileDiffInner({ - fileDiff, - options, - lineAnnotations, - renderAnnotation: renderAnnotationProp, - selectedLines, -}: LazyFileDiffProps) { - const placeholderRef = useRef(null); - const [visible, setVisible] = useState(false); - - useEffect(() => { - const el = placeholderRef.current; - if (!el || visible) { - return; - } - const observer = new IntersectionObserver( - ([entry]) => { - if (entry.isIntersecting) { - setVisible(true); - observer.disconnect(); - } - }, - // Pre-load files that are within one viewport-height of - // the visible area so they are ready before the user - // scrolls to them. - { rootMargin: "100% 0px" }, - ); - observer.observe(el); - return () => observer.disconnect(); - }, [visible]); - - if (!visible) { - return ( -
- - - - +
- ); - } - - return ( - +
+ {groups.map((lines, groupIndex) => ( + + {groupIndex > 0 && } + {lines.map((width, lineIndex) => ( + + ))} + + ))} +
+
); } -// memo requires stable props; the React Compiler provides them here. -const LazyFileDiff = memo(LazyFileDiffInner); +const DIFF_SKELETON_FILES = [ + [ + ["w-3/4", "w-1/2", "w-5/6"], + ["w-2/3", "w-2/5", "w-3/4", "w-1/3"], + ], + [ + ["w-1/2", "w-5/6"], + ["w-2/3", "w-1/3"], + ], +] as const; -// ------------------------------------------------------------------- -// Main component -// ------------------------------------------------------------------- +function DiffViewerSkeleton() { + return ( +
+ {DIFF_SKELETON_FILES.map((groups, fileIndex) => ( + + ))} +
+ ); +} export const DiffViewer: FC = ({ parsedFiles, @@ -522,6 +412,7 @@ export const DiffViewer: FC = ({ diffStyle, onLineNumberClick, onLineSelected, + onLineSelectionChange, getLineAnnotations, getSelectedLines, renderAnnotation, @@ -529,260 +420,164 @@ export const DiffViewer: FC = ({ onScrollToFileComplete, }) => { const theme = useTheme(); + const codeViewRef = useRef>(null); const isDark = theme.palette.mode === "dark"; + const [activeFile, setActiveFile] = useState(null); - const base = getDiffViewerOptions(isDark); - const diffOptions = { - ...base, - diffStyle, - // Extend the base CSS to make file headers sticky so they - // remain visible while scrolling through long diffs. - unsafeCSS: `${base.unsafeCSS ?? ""} ${STICKY_HEADER_CSS}`, - }; - - const fileOptions = { - ...diffOptions, - overflow: "wrap" as const, - enableLineSelection: true, - enableHoverUtility: true, - onLineSelected() { - // TODO: Make this add context to the input so the - // user can type. - }, - }; - - // When the parent provides per-file callbacks (e.g. line click - // handlers for comment inputs), build options per file. Otherwise - // share a single stable object to avoid unnecessary re-highlights. - const hasPerFileCallbacks = Boolean(onLineNumberClick || onLineSelected); - - const getOptionsForFile = (fileName: string) => ({ - ...diffOptions, - overflow: "wrap" as const, - enableLineSelection: true, - enableHoverUtility: true, - ...(onLineNumberClick && { - onLineNumberClick: (props: { - lineNumber: number; - annotationSide: "additions" | "deletions"; - }) => onLineNumberClick(fileName, props), - }), - onLineSelected: onLineSelected - ? ( - range: { - start: number; - end: number; - side?: "additions" | "deletions"; - endSide?: "additions" | "deletions"; - } | null, - ) => onLineSelected(fileName, range) - : () => { - // TODO: Make this add context to the input. - }, - }); - - const fileTree = buildFileTree(parsedFiles); - - // Sort diff blocks in the same order the file tree displays them - // (directories first, then alphabetical) so the rendering is - // consistent regardless of whether the sidebar is visible. - const sortedFiles = (() => { - const order = new Map(); - const walk = (nodes: FileTreeNode[]) => { - for (const node of nodes) { - if (node.type === "file") { - order.set(node.fullPath, order.size); - } else { - walk(node.children); - } - } - }; - walk(fileTree); - return [...parsedFiles].sort( - (a, b) => (order.get(a.name) ?? 0) - (order.get(b.name) ?? 0), - ); - })(); - - // Pre-compute per-file options so each LazyFileDiff receives a - // stable reference and avoids re-highlighting on parent re-render. - const perFileOptions = (() => { - if (!hasPerFileCallbacks) return null; - const map = new Map["options"]>(); - for (const file of sortedFiles) { - map.set(file.name, getOptionsForFile(file.name)); - } - return map; - })(); - - // Pre-compute per-file line annotations for the same reason. - const perFileAnnotations = (() => { - if (!getLineAnnotations) return null; - return new Map( - sortedFiles - .map((f) => [f.name, getLineAnnotations(f.name)] as const) - .filter( - (entry): entry is [string, DiffLineAnnotation[]] => - entry[1].length > 0, - ), - ); - })(); - - // Pre-compute per-file selected lines so each LazyFileDiff - // receives a stable reference. Without this, calling - // getSelectedLines during render returns a new object every - // time, which busts the memo comparator and forces an - // expensive Shadow DOM + shiki re-highlight. - const perFileSelectedLines = (() => { - if (!getSelectedLines) return null; - return new Map( - sortedFiles - .map((f) => [f.name, getSelectedLines(f.name)] as const) - .filter( - (entry): entry is [string, SelectedLineRange] => entry[1] != null, - ), - ); - })(); - - // --------------------------------------------------------------- - // Container width measurement via ResizeObserver so we can decide - // whether to show the file tree sidebar without a prop from the - // parent. - // --------------------------------------------------------------- - const [containerWidth, setContainerWidth] = useState(0); + // Measure the diff container so the file tree only appears when there is + // enough horizontal room, rather than keying off the viewport width. const [containerEl, setContainerEl] = useState(null); - + const [containerWidth, setContainerWidth] = useState(0); useEffect(() => { if (!containerEl) return; setContainerWidth(containerEl.getBoundingClientRect().width); - const ro = new ResizeObserver(([entry]) => { + const observer = new ResizeObserver(([entry]) => { setContainerWidth(entry.contentRect.width); }); - ro.observe(containerEl); - return () => ro.disconnect(); + observer.observe(containerEl); + return () => observer.disconnect(); }, [containerEl]); - const showTree = - (isExpanded || containerWidth >= FILE_TREE_THRESHOLD) && - sortedFiles.length > 0; + const showTree = isExpanded || containerWidth >= FILE_TREE_THRESHOLD; + const handleScroll = useActiveFileTracking({ + enabled: showTree, + onActiveFileChange: (path) => + setActiveFile((current) => (current === path ? current : path)), + }); - const diffViewportRef = useRef(null); - const { treeActiveFile, setFileRef, handleFileClick } = useActiveFileTracking( - { - viewportRef: diffViewportRef, - sortedFiles, - enabled: showTree, - scrollToFile, - onScrollToFileComplete, + const options: ComponentProps>["options"] = { + diffStyle, + diffIndicators: "bars", + overflow: "scroll", + stickyHeaders: true, + layout: { paddingTop: 0, paddingBottom: 0, gap: 0 }, + hunkSeparators: "line-info", + itemMetrics: diffViewerMetrics, + unsafeCSS: SEPARATOR_CSS, + themeType: isDark ? "dark" : "light", + theme: isDark ? "github-dark-high-contrast" : "github-light", + enableLineSelection: true, + enableGutterUtility: true, + onLineNumberClick: (props, item) => { + if (item.type === "diff" && props.type === "diff-line") { + onLineNumberClick?.(item.item.id, props); + } }, + onLineSelected: (range, item) => { + if (item.type === "diff") { + onLineSelected?.(item.item.id, range); + } + }, + onLineSelectionChange: (range, item) => { + if (item.type === "diff") { + onLineSelectionChange?.(item.item.id, range); + } + }, + onGutterUtilityClick: (range, item) => { + if (item.type === "diff") { + onLineSelected?.(item.item.id, range); + } + }, + }; + + // Render the diff in the same order the sidebar tree lays out its leaves so + // scrolling moves monotonically down the tree. + const sortedFiles = [...parsedFiles].sort((a, b) => + compareTreePaths(a.name, b.name), ); - // --------------------------------------------------------------- - // Loading state - // --------------------------------------------------------------- + const items: CodeViewItem[] = sortedFiles.map((fileDiff) => { + const annotations = getLineAnnotations?.(fileDiff.name); + return { + id: fileDiff.name, + type: "diff", + fileDiff, + annotations, + version: annotationsVersion(annotations), + }; + }); + + const selectedLines = (() => { + if (!getSelectedLines) return undefined; + for (const fileDiff of sortedFiles) { + const range = getSelectedLines(fileDiff.name); + if (range) return { id: fileDiff.name, range }; + } + return null; + })(); + + const canScroll = !isLoading && !error && items.length > 0; + + useEffect(() => { + if (!scrollToFile) return; + // CodeView is not mounted while loading/erroring/empty, so the ref is + // null. Skip the scroll without signalling completion so the parent + // keeps the pending target and retries once the view is ready. + if (!canScroll || !codeViewRef.current) return; + codeViewRef.current.scrollTo({ + type: "item", + id: scrollToFile, + align: "start", + behavior: "instant", + }); + onScrollToFileComplete?.(); + }, [scrollToFile, onScrollToFileComplete, canScroll]); + if (isLoading) { - return ( -
-
- {Array.from({ length: 3 }, (_, i) => ( -
- - - - -
- ))} -
-
- ); + return ; } - // --------------------------------------------------------------- - // Error state - // --------------------------------------------------------------- if (error) { + return ; + } + + if (items.length === 0) { return ( -
- +
+ {emptyMessage}
); } - // --------------------------------------------------------------- - // Main render - // --------------------------------------------------------------- return (
- {/* Diff contents */} - {sortedFiles.length === 0 ? ( -
- {emptyMessage} -
- ) : ( -
- {/* File tree sidebar */} - {showTree && ( - - - - )} - - {sortedFiles.map((fileDiff, i) => { - const isLast = i === sortedFiles.length - 1; - return ( -
setFileRef(fileDiff.name, el)} - className={cn( - "[contain:layout_style]", - i > 0 && - "border-0 border-t border-solid border-border-default", - )} - > - - {isLast && ( -
- {`${sortedFiles.length} ${sortedFiles.length === 1 ? "file" : "files"} changed`} -
- )} -
- ); - })} - -
+ {showTree && ( + )} + + item.type === "diff" ? ( + + ) : null + } + renderAnnotation={(annotation) => + "side" in annotation ? renderAnnotation?.(annotation) : null + } + />
); }; diff --git a/site/src/pages/AgentsPage/components/DiffViewer/annotationsVersion.test.ts b/site/src/pages/AgentsPage/components/DiffViewer/annotationsVersion.test.ts new file mode 100644 index 0000000000..ba8724715f --- /dev/null +++ b/site/src/pages/AgentsPage/components/DiffViewer/annotationsVersion.test.ts @@ -0,0 +1,44 @@ +import type { DiffLineAnnotation } from "@pierre/diffs/react"; +import { describe, expect, it } from "vitest"; +import { annotationsVersion } from "./DiffViewer"; + +const annotation = ( + lineNumber: number, + side: "additions" | "deletions" = "additions", +): DiffLineAnnotation => ({ + side, + lineNumber, + metadata: "active-input", +}); + +describe("annotationsVersion", () => { + it("is 0 when there are no annotations", () => { + expect(annotationsVersion(undefined)).toBe(0); + expect(annotationsVersion([])).toBe(0); + }); + + it("changes when the line moves but the count stays the same", () => { + // The regression: a single active comment box moving between lines keeps + // the count at 1, so a count-based version would not change and CodeView + // would skip the update. + expect(annotationsVersion([annotation(5)])).not.toBe( + annotationsVersion([annotation(10)]), + ); + }); + + it("changes when only the side flips", () => { + expect(annotationsVersion([annotation(5, "additions")])).not.toBe( + annotationsVersion([annotation(5, "deletions")]), + ); + }); + + it("is stable for identical annotation content", () => { + expect(annotationsVersion([annotation(7, "deletions")])).toBe( + annotationsVersion([annotation(7, "deletions")]), + ); + }); + + it("differs from the empty state for any annotation", () => { + expect(annotationsVersion([annotation(0)])).not.toBe(0); + }); +}); diff --git a/site/src/pages/AgentsPage/components/DiffViewer/compareTreePaths.test.ts b/site/src/pages/AgentsPage/components/DiffViewer/compareTreePaths.test.ts new file mode 100644 index 0000000000..50c512278d --- /dev/null +++ b/site/src/pages/AgentsPage/components/DiffViewer/compareTreePaths.test.ts @@ -0,0 +1,98 @@ +import { describe, expect, it } from "vitest"; +import { compareTreePaths, treeSortComparator } from "./DiffViewer"; + +// Mirrors how @pierre/trees feeds its sort comparator: it sorts one flat list +// of every path entry (files and the intermediate directories), then builds +// the tree. Expands file paths into that entry set so a test can sort it with +// treeSortComparator and read back the resulting file (leaf) order. +function treeFileOrder(files: readonly string[]): string[] { + const entries = new Map< + string, + { + basename: string; + depth: number; + isDirectory: boolean; + path: string; + segments: string[]; + } + >(); + for (const file of files) { + const segments = file.split("/"); + for (let i = 0; i < segments.length; i++) { + const slice = segments.slice(0, i + 1); + const path = slice.join("/"); + entries.set(path, { + basename: slice[slice.length - 1], + depth: slice.length, + isDirectory: i < segments.length - 1, + path, + segments: slice, + }); + } + } + return [...entries.values()] + .sort(treeSortComparator) + .filter((entry) => !entry.isDirectory) + .map((entry) => entry.path); +} + +describe("compareTreePaths", () => { + it("orders directories before sibling files and keeps dot-prefixed first", () => { + const sorted = [".config/a.ts", "b.ts", "a/z.ts", "b/c/d.ts"].sort( + compareTreePaths, + ); + + expect(sorted).toEqual([".config/a.ts", "a/z.ts", "b/c/d.ts", "b.ts"]); + }); + + it("sorts dot-prefixed names before other names at the same level", () => { + expect([".env", "app.ts", ".gitignore"].sort(compareTreePaths)).toEqual([ + ".env", + ".gitignore", + "app.ts", + ]); + }); + + it("breaks ties case-insensitively", () => { + expect(["Beta.ts", "alpha.ts", "Alpha.ts"].sort(compareTreePaths)).toEqual([ + "alpha.ts", + "Alpha.ts", + "Beta.ts", + ]); + }); + + it("is a stable total order regardless of input order", () => { + const files = [ + "src/zeta.ts", + "src/alpha/index.ts", + "README.md", + ".github/workflows/ci.yml", + "src/alpha.ts", + ]; + const forward = [...files].sort(compareTreePaths); + const reversed = [...files].reverse().sort(compareTreePaths); + expect(reversed).toEqual(forward); + expect(forward).toEqual([ + ".github/workflows/ci.yml", + "src/alpha/index.ts", + "src/alpha.ts", + "src/zeta.ts", + "README.md", + ]); + }); + + it("matches the tree's leaf order so the diff and sidebar stay in sync", () => { + const files = [ + "src/zeta.ts", + "lib/a.ts", + "src/alpha/index.ts", + "README.md", + ".github/workflows/ci.yml", + "src/alpha.ts", + "lib/b/c.ts", + ]; + // The sidebar tree (treeSortComparator over the full entry set) and the + // flat diff list (compareTreePaths) must produce the same file order. + expect([...files].sort(compareTreePaths)).toEqual(treeFileOrder(files)); + }); +}); diff --git a/site/src/pages/AgentsPage/components/DiffViewer/useActiveFileTracking.test.ts b/site/src/pages/AgentsPage/components/DiffViewer/useActiveFileTracking.test.ts new file mode 100644 index 0000000000..955a5709ad --- /dev/null +++ b/site/src/pages/AgentsPage/components/DiffViewer/useActiveFileTracking.test.ts @@ -0,0 +1,65 @@ +import { describe, expect, it } from "vitest"; +import { getActiveFile, type ScrollViewer } from "./useActiveFileTracking"; + +// Builds a ScrollViewer stub from a path -> top map. A top of `undefined` +// models an item the library has not measured yet. +function viewerFrom( + tops: ReadonlyArray, +): ScrollViewer { + const map = new Map(tops); + return { + getRenderedItems: () => tops.map(([id]) => ({ id })), + getTopForItem: (id) => map.get(id), + }; +} + +describe("getActiveFile", () => { + it("picks the item closest to the top that has crossed it", () => { + const viewer = viewerFrom([ + ["a.ts", 0], + ["b.ts", 300], + ["c.ts", 900], + ]); + // At scrollTop 500, b.ts (top 300) is the last file whose start has + // scrolled past the fold; c.ts (top 900) is still below it. + expect(getActiveFile(500, viewer)).toBe("b.ts"); + }); + + it("treats an item within the threshold slack as already active", () => { + const viewer = viewerFrom([ + ["a.ts", 0], + // 4px below the fold, inside ACTIVE_FILE_SCROLL_THRESHOLD. + ["b.ts", 104], + ]); + expect(getActiveFile(100, viewer)).toBe("b.ts"); + }); + + it("excludes items past the threshold slack", () => { + const viewer = viewerFrom([ + ["a.ts", 0], + // 5px below the fold, just outside the threshold. + ["b.ts", 105], + ]); + expect(getActiveFile(100, viewer)).toBe("a.ts"); + }); + + it("falls back to the first rendered item when none have crossed", () => { + const viewer = viewerFrom([ + ["a.ts", 300], + ["b.ts", 400], + ]); + expect(getActiveFile(0, viewer)).toBe("a.ts"); + }); + + it("ignores unmeasured items", () => { + const viewer = viewerFrom([ + ["a.ts", -10], + ["b.ts", undefined], + ]); + expect(getActiveFile(0, viewer)).toBe("a.ts"); + }); + + it("returns undefined when nothing is rendered", () => { + expect(getActiveFile(0, viewerFrom([]))).toBeUndefined(); + }); +}); diff --git a/site/src/pages/AgentsPage/components/DiffViewer/useActiveFileTracking.ts b/site/src/pages/AgentsPage/components/DiffViewer/useActiveFileTracking.ts index 2e659f7c66..6ccc0adf8c 100644 --- a/site/src/pages/AgentsPage/components/DiffViewer/useActiveFileTracking.ts +++ b/site/src/pages/AgentsPage/components/DiffViewer/useActiveFileTracking.ts @@ -1,146 +1,72 @@ -import type { FileDiffMetadata } from "@pierre/diffs"; -import { type RefObject, useEffect, useRef, useState } from "react"; +import { useEffect, useRef } from "react"; -// Leaves a 5% strip at the top of the viewport as the "active file" band. -const VIEWPORT_BOTTOM_MARGIN_RATIO = 0.95; +// Pixels of slack added when matching a file against the scroll offset, so a +// file counts as active while its sticky header is still pinned at the top. +const ACTIVE_FILE_SCROLL_THRESHOLD = 4; -interface UseActiveFileTrackingOptions { - viewportRef: RefObject; - sortedFiles: readonly FileDiffMetadata[]; - enabled: boolean; - scrollToFile?: string | null; - onScrollToFileComplete?: () => void; +// Minimal view of the @pierre/diffs CodeView instance handed to `onScroll`. +// `getRenderedItems` only returns the small set of currently virtualized +// items, so deriving the active file from it avoids scanning every file. +export interface ScrollViewer { + getRenderedItems(): readonly { id: string }[]; + getTopForItem(id: string): number | undefined; } -interface UseActiveFileTrackingReturn { - treeActiveFile: string | null; - setFileRef: (name: string, el: HTMLDivElement | null) => void; - handleFileClick: (name: string) => void; +// The active file is the rendered item closest to the top edge that has +// already crossed it (largest top still at or above the fold). Exported for +// unit tests that pin the closest-to-top selection logic. +export function getActiveFile( + scrollTop: number, + viewer: ScrollViewer, +): string | undefined { + const rendered = viewer.getRenderedItems(); + const limit = scrollTop + ACTIVE_FILE_SCROLL_THRESHOLD; + let activePath: string | undefined; + let activeTop = Number.NEGATIVE_INFINITY; + for (const item of rendered) { + const top = viewer.getTopForItem(item.id); + if (top !== undefined && top <= limit && top > activeTop) { + activeTop = top; + activePath = item.id; + } + } + return activePath ?? rendered[0]?.id; } +/** + * Reports the diff file scrolled to the top as the user scrolls. Returns a + * CodeView `onScroll` handler. Tree-agnostic: callers decide what to do with + * the active path (the diff viewer feeds it to the sidebar selection). + */ export function useActiveFileTracking({ - viewportRef, - sortedFiles, enabled, - scrollToFile, - onScrollToFileComplete, -}: UseActiveFileTrackingOptions): UseActiveFileTrackingReturn { - const fileRefs = useRef>(new Map()); - const [treeActiveFile, setTreeActiveFile] = useState(null); - - const [viewportHeight, setViewportHeight] = useState(0); - - // viewportRef is a stable RefObject whose identity never changes, so - // an effect that depends on it won't re-run when .current transitions - // from null to the actual DOM node (e.g. after a loading state). - // Keep a state mirror that flips exactly once when the element mounts. - const [viewportEl, setViewportEl] = useState(null); - useEffect(() => { - setViewportEl(viewportRef.current); - }); + onActiveFileChange, +}: { + enabled: boolean; + onActiveFileChange: (path: string) => void; +}) { + const rafRef = useRef(null); useEffect(() => { - if (!viewportEl) return; - setViewportHeight(viewportEl.clientHeight); - const ro = new ResizeObserver(([entry]) => { - setViewportHeight(Math.round(entry.contentRect.height)); + return () => { + if (rafRef.current !== null) { + cancelAnimationFrame(rafRef.current); + } + }; + }, []); + + return (scrollTop: number, viewer: ScrollViewer) => { + if (!enabled) return; + if (rafRef.current !== null) { + cancelAnimationFrame(rafRef.current); + } + // Coalesce bursts of scroll events into one update per frame. + rafRef.current = requestAnimationFrame(() => { + rafRef.current = null; + const next = getActiveFile(scrollTop, viewer); + if (next) { + onActiveFileChange(next); + } }); - ro.observe(viewportEl); - return () => ro.disconnect(); - }, [viewportEl]); - - const sortedFilesRef = useRef(sortedFiles); - useEffect(() => { - sortedFilesRef.current = sortedFiles; - }); - - const fileListKey = sortedFiles.map((f) => f.name).join("\0"); - - const setFileRef = (name: string, el: HTMLDivElement | null) => { - if (el) { - fileRefs.current.set(name, el); - } else { - fileRefs.current.delete(name); - } - }; - - useEffect(() => { - if (!enabled || fileListKey === "" || viewportHeight === 0) return; - if (!viewportEl) return; - - const bottomMargin = Math.round( - viewportHeight * VIEWPORT_BOTTOM_MARGIN_RATIO, - ); - - const intersecting = new Set(); - - const observer = new IntersectionObserver( - (entries) => { - for (const entry of entries) { - const name = (entry.target as HTMLElement).dataset.fileName; - if (!name) continue; - if (entry.isIntersecting) { - intersecting.add(name); - } else { - intersecting.delete(name); - } - } - for (const file of sortedFilesRef.current) { - if (intersecting.has(file.name)) { - setTreeActiveFile(file.name); - break; - } - } - }, - { - root: viewportEl, - // Observe only the top ~5% strip of the viewport height. - rootMargin: `0px 0px -${bottomMargin}px 0px`, - threshold: 0, - }, - ); - - for (const [, el] of fileRefs.current.entries()) { - observer.observe(el); - } - - return () => observer.disconnect(); - }, [enabled, fileListKey, viewportEl, viewportHeight]); - - const handleFileClick = (name: string) => { - const el = fileRefs.current.get(name); - if (el) { - el.scrollIntoView({ block: "start", behavior: "instant" }); - setTreeActiveFile(name); - } - }; - - // biome-ignore lint/correctness/useExhaustiveDependencies: fileListKey is an intentional trigger dep. The effect reads fileRefs (a mutable ref) and must retry when the file list changes so a previously-unmounted element can be found. - useEffect(() => { - if (!scrollToFile) return; - const el = fileRefs.current.get(scrollToFile); - if (el) { - el.scrollIntoView({ block: "start", behavior: "instant" }); - setTreeActiveFile(scrollToFile); - onScrollToFileComplete?.(); - return; - } - // Element not found. If the target isn't even in the current file - // list (e.g. stale chip after the diff changed), complete the - // request so the parent can clear its scroll target. Otherwise - // the target is present but not yet mounted; wait for fileListKey - // to change again. - const existsInFileList = sortedFilesRef.current.some( - (f) => f.name === scrollToFile, - ); - if (!existsInFileList) { - onScrollToFileComplete?.(); - } - }, [scrollToFile, onScrollToFileComplete, fileListKey]); - - return { - treeActiveFile, - setFileRef, - handleFileClick, }; } diff --git a/site/src/pages/AgentsPage/components/GitPanel/GitPanel.stories.tsx b/site/src/pages/AgentsPage/components/GitPanel/GitPanel.stories.tsx index c7b7789381..1bf84e69c4 100644 --- a/site/src/pages/AgentsPage/components/GitPanel/GitPanel.stories.tsx +++ b/site/src/pages/AgentsPage/components/GitPanel/GitPanel.stories.tsx @@ -1,5 +1,5 @@ import type { Meta, StoryObj } from "@storybook/react-vite"; -import { expect, fn, spyOn, userEvent, within } from "storybook/test"; +import { expect, fn, spyOn, userEvent, waitFor, within } from "storybook/test"; import { API } from "#/api/api"; import type { ChatDiffContents, @@ -334,35 +334,19 @@ export const InlineCommentInput: Story = { }); }, play: async ({ canvasElement }) => { - // Wait for the diff to load and render inside Shadow DOM. - // The line numbers live inside @pierre/diffs FileDiff web - // components, so we need to wait a bit for them to mount. - await new Promise((resolve) => setTimeout(resolve, 2000)); - - // Find a line number element inside a Shadow DOM diff viewer. - // The diff renders in shadow roots, so we look for the - // host elements and query inside their shadow DOMs. - const diffHosts = canvasElement.querySelectorAll("[data-diffs]"); - - for (const host of diffHosts) { - const shadow = host.shadowRoot; - if (!shadow) continue; - - // Look for a line number cell — they have data-line-number. - const lineNumber = shadow.querySelector( - "[data-line-number]", - ) as HTMLElement | null; - if (lineNumber) { - await userEvent.click(lineNumber); - break; + const canvas = within(canvasElement); + const lineNumber = await waitFor(() => { + for (const host of canvasElement.querySelectorAll("diffs-container")) { + const target = host.shadowRoot?.querySelector( + "[data-column-number]", + ) as HTMLElement | null; + if (target) return target; } - } + throw new Error("No rendered diff line number found"); + }); - // Verify the inline prompt appeared. - const textarea = canvasElement.querySelector("textarea"); - if (textarea) { - expect(textarea).toBeInTheDocument(); - } + await userEvent.click(lineNumber); + expect(canvas.getByRole("textbox")).toBeInTheDocument(); }, }; diff --git a/site/src/pages/AgentsPage/utils/countChangedLines.ts b/site/src/pages/AgentsPage/utils/countChangedLines.ts new file mode 100644 index 0000000000..e27b2b32d4 --- /dev/null +++ b/site/src/pages/AgentsPage/utils/countChangedLines.ts @@ -0,0 +1,12 @@ +import type { FileDiffMetadata } from "@pierre/diffs"; + +/** Sums added and deleted line counts across every hunk in a file diff. */ +export function countChangedLines(fileDiff: FileDiffMetadata) { + let additions = 0; + let deletions = 0; + for (const hunk of fileDiff.hunks) { + additions += hunk.additionLines; + deletions += hunk.deletionLines; + } + return { additions, deletions }; +}