From 31fe58819e79bb5cb0fd605881cf8af35a39cc98 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Thu, 19 Mar 2026 16:04:25 +0000 Subject: [PATCH] fix: place diff comment box at end of selection and highlight selected lines (#23288) --- site/src/components/ai-elements/tool/utils.ts | 87 +++++++++- .../pages/AgentsPage/DiffViewer.stories.tsx | 152 ++++++++++++++++++ site/src/pages/AgentsPage/DiffViewer.tsx | 96 ++++++++++- .../AgentsPage/InlinePromptInput.stories.tsx | 59 +++++++ site/src/pages/AgentsPage/RemoteDiffPanel.tsx | 54 +++++-- 5 files changed, 425 insertions(+), 23 deletions(-) create mode 100644 site/src/pages/AgentsPage/DiffViewer.stories.tsx create mode 100644 site/src/pages/AgentsPage/InlinePromptInput.stories.tsx diff --git a/site/src/components/ai-elements/tool/utils.ts b/site/src/components/ai-elements/tool/utils.ts index 90b3962e87..c57365846e 100644 --- a/site/src/components/ai-elements/tool/utils.ts +++ b/site/src/components/ai-elements/tool/utils.ts @@ -150,8 +150,91 @@ export const formatResultOutput = (result: unknown): string | null => { export const fileViewerCSS = "pre, [data-line], [data-diffs-header] { background-color: transparent !important; }"; -export const diffViewerCSS = - "pre, [data-line], [data-diffs-header] { background-color: transparent !important; } [data-diffs-header] { border-left: 1px solid var(--border); }"; +// 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));", + "}", + // 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 = [ + // Transparent backgrounds so separators blend with the + // code area rather than forming a distinct band. + ":host {", + " --diffs-bg-separator-override: transparent;", + "}", + "[data-separator-content] {", + " border-radius: 0 !important;", + " background-color: transparent !important;", + "}", + "[data-separator-wrapper] {", + " border-radius: 0 !important;", + "}", + // Remove the inline padding that creates the inset pill look + // so separators span the full width of the diff. + "[data-unified] [data-separator='line-info'] [data-separator-wrapper] {", + " padding-inline: 0 !important;", + "}", + // The first separator in a file just says "N unmodified + // lines" before the first hunk — that's obvious context + // that adds no value, so hide it entirely. + "[data-separator='line-info'][data-separator-first] {", + " display: none !important;", + "}", + // Thin single border and muted text so collapsed-line + // indicators read as a quiet hint, not a landmark. + "[data-separator='line-info'] {", + " height: 28px !important;", + " border-top: 1px solid hsl(var(--border-default));", + " border-bottom: 1px solid hsl(var(--border-default));", + "}", + "[data-separator-content] {", + " font-size: 11px !important;", + " color: hsl(var(--content-secondary)) !important;", + " opacity: 0.8;", + "}", +].join(" "); + +export const diffViewerCSS = [ + "pre, [data-line]:not([data-selected-line]), [data-diffs-header] { background-color: transparent !important; }", + "[data-diffs-header] { border-left: 1px solid var(--border); }", + SELECTION_OVERRIDE_CSS, + SEPARATOR_CSS, +].join(" "); // Theme-aware option factories shared across tool renderers. export function getDiffViewerOptions(isDark: boolean) { diff --git a/site/src/pages/AgentsPage/DiffViewer.stories.tsx b/site/src/pages/AgentsPage/DiffViewer.stories.tsx new file mode 100644 index 0000000000..a3e117a492 --- /dev/null +++ b/site/src/pages/AgentsPage/DiffViewer.stories.tsx @@ -0,0 +1,152 @@ +import type { DiffLineAnnotation, SelectedLineRange } from "@pierre/diffs"; +import { parsePatchFiles } from "@pierre/diffs"; +import type { Meta, StoryObj } from "@storybook/react-vite"; +import { fn } from "storybook/test"; +import type { DiffStyle } from "./DiffViewer"; +import { DiffViewer } from "./DiffViewer"; +import { InlinePromptInput } from "./RemoteDiffPanel"; + +// biome-ignore format: raw diff string must preserve exact whitespace +const sampleDiff = [ +"diff --git a/src/main.ts b/src/main.ts", +"index abc1234..def5678 100644", +"--- a/src/main.ts", +"+++ b/src/main.ts", +"@@ -1,5 +1,7 @@", +" import { start } from \"./server\";", +"+import { logger } from \"./logger\";", +"", +" const port = 3000;", +"+logger.info(\"Starting server...\");", +" start(port);", +"diff --git a/src/server.ts b/src/server.ts", +"index 1111111..2222222 100644", +"--- a/src/server.ts", +"+++ b/src/server.ts", +"@@ -10,3 +10,5 @@", +" app.listen(port, () => {", +" console.log(\"Listening on port \" + port);", +" });", +"+", +"+ return app;", +" }", +].join("\n"); +const parsedFiles = parsePatchFiles(sampleDiff).flatMap((p) => p.files); +const firstFileName = parsedFiles[0]?.name ?? ""; + +const meta: Meta = { + title: "pages/AgentsPage/DiffViewer", + component: DiffViewer, + args: { + parsedFiles, + diffStyle: "unified" satisfies DiffStyle, + onLineNumberClick: fn(), + onLineSelected: fn(), + onScrollToFileComplete: fn(), + }, + decorators: [ + (Story) => ( +
+ +
+ ), + ], +}; +export default meta; +type Story = StoryObj; + +export const Default: Story = {}; + +export const SplitView: Story = { + args: { + diffStyle: "split", + }, +}; + +export const Loading: Story = { + args: { + parsedFiles: [], + isLoading: true, + }, +}; + +export const ErrorState: Story = { + name: "Error", + args: { + parsedFiles: [], + error: new Error("Failed to fetch diff"), + }, +}; + +export const Empty: Story = { + args: { + parsedFiles: [], + emptyMessage: "No file changes to display.", + }, +}; + +export const WithSelectedLines: Story = { + args: { + getSelectedLines: (fileName: string): SelectedLineRange | null => { + if (fileName === firstFileName) { + return { start: 2, end: 4, side: "additions" }; + } + return null; + }, + }, +}; + +// Diff with two non-adjacent hunks in one file, producing a +// mid-file separator that should remain visible even though +// leading separators are hidden. +// biome-ignore format: raw diff string must preserve exact whitespace +const multiHunkDiff = [ +"diff --git a/src/app.ts b/src/app.ts", +"index aaa1111..bbb2222 100644", +"--- a/src/app.ts", +"+++ b/src/app.ts", +"@@ -3,4 +3,5 @@", +" import { db } from \"./db\";", +" import { logger } from \"./logger\";", +"+import { metrics } from \"./metrics\";", +" ", +" const app = express();", +"@@ -20,3 +21,4 @@", +" app.listen(port, () => {", +" console.log(\"Listening on port \" + port);", +"+ metrics.record(\"server.start\");", +" });", +].join("\n"); +const multiHunkFiles = parsePatchFiles(multiHunkDiff).flatMap((p) => p.files); + +export const WithMidFileSeparator: Story = { + args: { + parsedFiles: multiHunkFiles, + }, +}; + +export const WithAnnotation: Story = { + args: { + getSelectedLines: (fileName: string): SelectedLineRange | null => { + if (fileName === firstFileName) { + return { start: 2, end: 4, side: "additions" }; + } + return null; + }, + getLineAnnotations: (fileName: string): DiffLineAnnotation[] => { + if (fileName === firstFileName) { + return [ + { + lineNumber: 4, + side: "additions", + metadata: "active-input", + }, + ]; + } + return []; + }, + renderAnnotation: () => ( + + ), + }, +}; diff --git a/site/src/pages/AgentsPage/DiffViewer.tsx b/site/src/pages/AgentsPage/DiffViewer.tsx index 5ff92bee36..1c196dca6d 100644 --- a/site/src/pages/AgentsPage/DiffViewer.tsx +++ b/site/src/pages/AgentsPage/DiffViewer.tsx @@ -1,5 +1,9 @@ import { useTheme } from "@emotion/react"; -import type { DiffLineAnnotation, FileDiffMetadata } from "@pierre/diffs"; +import type { + DiffLineAnnotation, + FileDiffMetadata, + SelectedLineRange, +} from "@pierre/diffs"; import { FileDiff } from "@pierre/diffs/react"; import { ErrorAlert } from "components/Alert/ErrorAlert"; import { @@ -69,6 +73,11 @@ interface DiffViewerProps { * 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`. */ @@ -98,15 +107,84 @@ const FILE_TREE_THRESHOLD = 1000; * file headers sticky and adjust metadata layout. */ const STICKY_HEADER_CSS = [ + // Layout and sticky behavior. "[data-diffs-header] {", " position: sticky; top: 0; z-index: 10;", " font-size: 13px;", + " min-height: 32px !important;", + " padding-block: 0 !important;", + " padding-inline: 12px !important;", " border-bottom: 1px solid hsl(var(--border-default));", - " background-color: hsl(var(--surface-quaternary)) !important;", + " background-color: hsl(var(--surface-secondary)) !important;", "}", - "[data-diffs-header] [data-metadata] { flex-direction: row-reverse; }", - "@media (prefers-color-scheme: dark) {", - " [data-diffs-header] { background-color: hsl(var(--surface-secondary)) !important; }", + + // Keep the title in the site's sans-serif font, just a + // touch smaller than the surrounding header text. + "[data-diffs-header] [data-title] {", + " font-size: 12px;", + " color: hsl(var(--content-primary));", + "}", + + // Hide the library's built-in change-type SVG icons and + // replace them with a single-letter badge (A/D/M/R) via + // CSS-generated content. The letter mirrors the file tree + // sidebar and works even when the tree is hidden in narrow + // layouts. + "[data-change-icon] { display: none !important; }", + "[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 matching the + // DiffStatBadge component used in the PR header. + "[data-diffs-header] [data-metadata] {", + " flex-direction: row-reverse;", + " gap: 0 !important;", + "}", + "[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: 6px;", + " border-radius: 3px;", + "}", + "[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));", + "}", + // When both counts are present, flatten the touching inner + // edges so they form one joined badge. DOM order is + // [deletions][additions]; row-reverse puts additions left. + "[data-deletions-count] + [data-additions-count] {", + " border-radius: 3px 0 0 3px;", + "}", + "[data-deletions-count]:has(+ [data-additions-count]) {", + " border-radius: 0 3px 3px 0;", "}", ].join(" "); @@ -346,12 +424,14 @@ const LazyFileDiff = memo<{ options: ComponentProps["options"]; lineAnnotations?: DiffLineAnnotation[]; renderAnnotation?: (annotation: DiffLineAnnotation) => ReactNode; + selectedLines?: SelectedLineRange | null; }>( ({ fileDiff, options, lineAnnotations, renderAnnotation: renderAnnotationProp, + selectedLines, }) => { const placeholderRef = useRef(null); const [visible, setVisible] = useState(false); @@ -399,6 +479,7 @@ const LazyFileDiff = memo<{ style={DIFFS_FONT_STYLE} lineAnnotations={lineAnnotations} renderAnnotation={renderAnnotationProp} + selectedLines={selectedLines} /> ); }, @@ -406,7 +487,8 @@ const LazyFileDiff = memo<{ if ( prev.fileDiff !== next.fileDiff || prev.options !== next.options || - prev.lineAnnotations !== next.lineAnnotations + prev.lineAnnotations !== next.lineAnnotations || + prev.selectedLines !== next.selectedLines ) { return false; } @@ -436,6 +518,7 @@ export const DiffViewer: FC = ({ onLineNumberClick, onLineSelected, getLineAnnotations, + getSelectedLines, renderAnnotation, scrollToFile, onScrollToFileComplete, @@ -778,6 +861,7 @@ export const DiffViewer: FC = ({ options={perFileOptions?.get(fileDiff.name) ?? fileOptions} lineAnnotations={perFileAnnotations?.get(fileDiff.name)} renderAnnotation={renderAnnotation} + selectedLines={getSelectedLines?.(fileDiff.name)} /> ))} diff --git a/site/src/pages/AgentsPage/InlinePromptInput.stories.tsx b/site/src/pages/AgentsPage/InlinePromptInput.stories.tsx new file mode 100644 index 0000000000..df6bf4139e --- /dev/null +++ b/site/src/pages/AgentsPage/InlinePromptInput.stories.tsx @@ -0,0 +1,59 @@ +import type { Meta, StoryObj } from "@storybook/react-vite"; +import { expect, fn, userEvent, within } from "storybook/test"; +import { InlinePromptInput } from "./RemoteDiffPanel"; + +const meta: Meta = { + title: "pages/AgentsPage/InlinePromptInput", + component: InlinePromptInput, + decorators: [ + (Story) => ( +
+ +
+ ), + ], + args: { + onSubmit: fn(), + onCancel: fn(), + }, +}; +export default meta; +type Story = StoryObj; + +export const Default: Story = {}; + +export const WithText: Story = { + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const textarea = canvas.getByPlaceholderText("Add a comment..."); + await userEvent.type(textarea, "Fix the race condition on line 42"); + }, +}; + +export const Submitting: Story = { + args: { + onSubmit: fn(), + }, + play: async ({ canvasElement, args }) => { + const canvas = within(canvasElement); + const textarea = canvas.getByPlaceholderText("Add a comment..."); + await userEvent.type(textarea, "Fix the race condition on line 42"); + await userEvent.keyboard("{Enter}"); + await expect(args.onSubmit).toHaveBeenCalledWith( + "Fix the race condition on line 42", + ); + }, +}; + +export const Cancelling: Story = { + args: { + onCancel: fn(), + }, + play: async ({ canvasElement, args }) => { + const canvas = within(canvasElement); + const textarea = canvas.getByPlaceholderText("Add a comment..."); + await userEvent.click(textarea); + await userEvent.keyboard("{Escape}"); + await expect(args.onCancel).toHaveBeenCalled(); + }, +}; diff --git a/site/src/pages/AgentsPage/RemoteDiffPanel.tsx b/site/src/pages/AgentsPage/RemoteDiffPanel.tsx index 24864b5588..51b3e2b37f 100644 --- a/site/src/pages/AgentsPage/RemoteDiffPanel.tsx +++ b/site/src/pages/AgentsPage/RemoteDiffPanel.tsx @@ -1,11 +1,15 @@ -import type { DiffLineAnnotation, FileDiffMetadata } from "@pierre/diffs"; +import type { + DiffLineAnnotation, + FileDiffMetadata, + SelectedLineRange, +} from "@pierre/diffs"; import { parsePatchFiles } from "@pierre/diffs"; import { chatDiffContents } from "api/queries/chats"; import type * as TypesGen from "api/typesGenerated"; import { Button } from "components/Button/Button"; import { ArrowLeftIcon, - CornerDownLeftIcon, + ArrowUpIcon, ExternalLinkIcon, GitBranchIcon, GitMergeIcon, @@ -165,7 +169,7 @@ const PullRequestStateBadge: FC<{ * line(s). Supports multiline via Shift+Enter. Enter submits, * Escape dismisses. */ -const InlinePromptInput: FC<{ +export const InlinePromptInput: FC<{ onSubmit: (text: string) => void; onCancel: () => void; }> = ({ onSubmit, onCancel }) => { @@ -183,12 +187,12 @@ const InlinePromptInput: FC<{ return (
-
+