From 92f8ba5e1032d95e7bda42a5aa8541704dcad15d Mon Sep 17 00:00:00 2001 From: maria-rcks <254055478+maria-rcks@users.noreply.github.com> Date: Thu, 6 Aug 2026 13:34:14 -0400 Subject: [PATCH 01/10] refactor(web): share diff rendering foundations --- apps/web/src/components/DiffPanel.tsx | 314 +----------------- .../diffs/AnnotatableCodeView.test.tsx | 4 +- .../components/diffs/AnnotatableCodeView.tsx | 13 +- .../DiffCommentAnnotation.test.tsx} | 38 ++- .../DiffCommentAnnotation.tsx} | 76 +++-- .../diffs/StyledDiffCodeView.test.tsx | 58 ++++ .../components/diffs/StyledDiffCodeView.tsx | 310 +++++++++++++++++ .../src/components/files/FilePreviewPanel.tsx | 4 +- apps/web/src/lib/diffFileContents.test.ts | 80 +++++ apps/web/src/lib/diffFileContents.ts | 124 +++++++ packages/contracts/src/pullRequest.ts | 17 + vite.config.ts | 6 + 12 files changed, 699 insertions(+), 345 deletions(-) rename apps/web/src/components/{files/LocalCommentAnnotation.test.tsx => diffs/DiffCommentAnnotation.test.tsx} (61%) rename apps/web/src/components/{files/LocalCommentAnnotation.tsx => diffs/DiffCommentAnnotation.tsx} (60%) create mode 100644 apps/web/src/components/diffs/StyledDiffCodeView.test.tsx create mode 100644 apps/web/src/components/diffs/StyledDiffCodeView.tsx create mode 100644 apps/web/src/lib/diffFileContents.test.ts create mode 100644 apps/web/src/lib/diffFileContents.ts diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index a10cdafd783..1855990aa1a 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -74,6 +74,7 @@ import { serverEnvironment } from "../state/server"; import { reviewEnvironment } from "../state/review"; import { vcsEnvironment } from "../state/vcs"; import { buildBaseRefChoices, filterBaseRefChoices } from "../lib/baseRefChoices"; +import { createGitDiffFileContentsLoader } from "../lib/diffFileContents"; type DiffRenderMode = "stacked" | "split"; type DiffThemeType = "light" | "dark"; @@ -86,262 +87,6 @@ interface CollapsedDiffFilesState { const EMPTY_COLLAPSED_DIFF_FILE_KEYS: ReadonlySet = new Set(); -const DIFF_PANEL_UNSAFE_CSS = ` -[data-diffs-header], -[data-diff], -[data-file], -[data-error-wrapper], -[data-virtualizer-buffer] { - --diffs-header-font-family: var(--font-sans) !important; - --diffs-font-family: var(--font-mono) !important; - --diffs-bg: var(--background) !important; - --diffs-light-bg: var(--background) !important; - --diffs-dark-bg: var(--background) !important; - --diffs-token-light-bg: transparent; - --diffs-token-dark-bg: transparent; - - --diffs-bg-context-override: color-mix(in srgb, var(--background) 97%, var(--foreground)); - --diffs-bg-hover-override: color-mix(in srgb, var(--background) 94%, var(--foreground)); - --diffs-bg-separator-override: color-mix(in srgb, var(--background) 95%, var(--foreground)); - --diffs-bg-buffer-override: color-mix(in srgb, var(--background) 90%, var(--foreground)); - - --diffs-bg-addition-override: light-dark( - color-mix(in srgb, var(--background) 50%, var(--success)), - color-mix(in srgb, var(--background) 70%, var(--success)) - ); - --diffs-bg-addition-number-override: light-dark( - color-mix(in srgb, var(--background) 35%, var(--success)), - color-mix(in srgb, var(--background) 60%, var(--success)) - ); - --diffs-bg-addition-hover-override: color-mix(in srgb, var(--background) 85%, var(--success)); - --diffs-bg-addition-emphasis-override: color-mix(in srgb, var(--background) 80%, var(--success)); - - --diffs-bg-deletion-override: light-dark( - color-mix(in srgb, var(--background) 50%, var(--destructive)), - color-mix(in srgb, var(--background) 70%, var(--destructive)) - ); - --diffs-bg-deletion-number-override: light-dark( - color-mix(in srgb, var(--background) 35%, var(--destructive)), - color-mix(in srgb, var(--background) 60%, var(--destructive)) - ); - --diffs-bg-deletion-hover-override: color-mix(in srgb, var(--background) 85%, var(--destructive)); - --diffs-bg-deletion-emphasis-override: color-mix( - in srgb, - var(--background) 80%, - var(--destructive) - ); - - background-color: var(--diffs-bg) !important; -} - -:is( - [data-line], - [data-line-annotation], - [data-merge-conflict], - [data-merge-conflict-actions], - [data-no-newline] -)[data-selected-line] { - --diffs-line-bg: light-dark( - color-mix( - in lab, - var(--background) 88%, - color-mix(in srgb, var(--background) 50%, var(--diffs-modified-base)) - ), - color-mix( - in lab, - var(--background) 80%, - color-mix(in srgb, var(--background) 70%, var(--diffs-modified-base)) - ) - ) !important; -} - -:is([data-gutter-buffer], [data-column-number])[data-selected-line] { - --diffs-line-bg: light-dark( - color-mix( - in lab, - var(--background) 91%, - color-mix(in srgb, var(--background) 35%, var(--diffs-modified-base)) - ), - color-mix( - in lab, - var(--background) 85%, - color-mix(in srgb, var(--background) 60%, var(--diffs-modified-base)) - ) - ) !important; -} - -[data-indicators="bars"] - :is([data-column-number], [data-gutter-buffer="annotation"])[data-selected-line] { - position: relative; -} - -[data-indicators="bars"] - :is([data-column-number], [data-gutter-buffer="annotation"])[data-selected-line]::before { - position: absolute !important; - inset-block: 0 !important; - inset-inline-start: 0 !important; - display: block !important; - width: 4px !important; - min-width: 4px !important; - max-width: 4px !important; - height: auto !important; - padding: 0 !important; - content: "" !important; - background-color: var(--diffs-modified-base) !important; - background-image: none !important; -} - -[data-file-info] { - background-color: var(--background) !important; - border-block-color: transparent !important; - color: var(--foreground) !important; -} - -[data-diffs-header] { - position: sticky !important; - top: 0; - z-index: 4; - background-color: var(--background) !important; - border-bottom-color: transparent !important; - align-items: center !important; - font-family: var(--font-sans) !important; - font-size: 12px !important; - line-height: 1 !important; - min-height: 32px !important; - padding-block: 6px !important; - padding-inline: 8px 12px !important; -} - -[data-diffs-header]:hover { - background-color: color-mix(in srgb, var(--background) 97%, var(--foreground)) !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]) { - height: 24px !important; - margin-block: 0 !important; - background-color: var(--background) !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]) - [data-separator-wrapper] { - padding-inline: 8px 12px !important; - background-color: transparent !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]) - [data-separator-content] { - gap: 8px; - padding-inline: 0 !important; - background-color: transparent !important; - color: color-mix(in srgb, var(--foreground) 52%, var(--background)) !important; - font-family: var(--font-sans) !important; - font-size: 11px !important; - text-decoration: none !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]) - [data-unmodified-lines] { - display: flex !important; - min-width: 0; - flex: 1 1 auto; - align-items: center; - gap: 8px; - cursor: pointer; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]) - [data-unmodified-lines]::before, -:is([data-separator="line-info"], [data-separator="line-info-basic"]) - [data-unmodified-lines]::after { - width: auto; - height: 1px; - flex: 1 1 auto; - content: ""; - background-color: color-mix(in srgb, var(--background) 92%, var(--foreground)); -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"])[data-expand-index] - [data-separator-wrapper] { - grid-template-columns: 0 minmax(0, 1fr) !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"])[data-expand-index] - [data-separator-content] { - grid-column: 2 !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]) - [data-expand-button] { - display: none !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( - [data-expand-button] - ) - [data-separator-content] { - cursor: pointer; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( - [data-expand-button] - ):hover - [data-separator-content] { - color: color-mix(in srgb, var(--foreground) 76%, var(--background)) !important; -} - -:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( - [data-expand-button] - ):hover - [data-unmodified-lines]::before, -:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( - [data-expand-button] - ):hover - [data-unmodified-lines]::after { - background-color: color-mix(in srgb, var(--background) 84%, var(--foreground)); -} - -[data-diffs-header] [data-header-content] { - align-items: center !important; - line-height: 1 !important; -} - -[data-diffs-header] [data-metadata] { - align-items: center !important; - line-height: 1 !important; - font-variant-numeric: tabular-nums; -} - -[data-diffs-header] [data-additions-count], -[data-diffs-header] [data-deletions-count] { - font-family: var(--font-mono) !important; - font-size: 11px !important; - font-variant-numeric: tabular-nums; - line-height: 1 !important; -} - -[data-diffs-header] [data-change-icon], -[data-diffs-header] [data-rename-icon] { - display: block; - flex-shrink: 0; -} - -[data-title] { - cursor: pointer; - transition: - color 120ms ease, - text-decoration-color 120ms ease; - text-decoration: underline; - text-decoration-color: transparent; - text-underline-offset: 2px; - font-family: var(--font-sans) !important; -} - -[data-title]:hover { - color: color-mix(in srgb, var(--foreground) 84%, var(--primary)) !important; - text-decoration-color: currentColor; -} -`; - interface DiffPanelProps { mode?: DiffPanelMode; composerDraftTarget: ScopedThreadRef | DraftId; @@ -569,45 +314,14 @@ export default function DiffPanel({ return undefined; } - const source = selectedGitSource; - return async (fileDiff) => { - const newPath = resolveFileDiffPath(fileDiff); - const oldPath = fileDiff.prevName - ? resolveFileDiffPath({ ...fileDiff, name: fileDiff.prevName }) - : newPath; - const result = await getDiffFileContents({ - environmentId: activeThread.environmentId, - input: { - cwd: preview.cwd, - sourceKind: source.kind, - changeType: fileDiff.type, - baseRef: source.baseRef, - headRef: source.headRef, - oldPath, - newPath, - }, - }); - if (result._tag !== "Success") { - throw squashAtomCommandFailure(result); - } - - const newFile = { - name: newPath, - contents: result.value.newContents, - cacheKey: `${source.diffHash}:new:${newPath}`, - }; - if (fileDiff.type === "rename-pure") { - return { oldFile: null, newFile }; - } - return { - oldFile: { - name: oldPath, - contents: result.value.oldContents, - cacheKey: `${source.diffHash}:old:${oldPath}`, - }, - newFile, - }; - }; + return createGitDiffFileContentsLoader(getDiffFileContents, { + environmentId: activeThread.environmentId, + cwd: preview.cwd, + sourceKind: selectedGitSource.kind, + baseRef: selectedGitSource.baseRef, + headRef: selectedGitSource.headRef, + cacheKey: selectedGitSource.diffHash, + }); }, [ activeThread, branchDiffPreview.data, @@ -1160,7 +874,7 @@ export default function DiffPanel({ key={collapseScopeKey ?? reviewSectionId} viewerRef={codeViewRef} codeViewKey={codeViewMountKey} - className="diff-render-surface h-full min-h-0 overflow-auto" + className="h-full min-h-0 overflow-auto" files={codeViewFiles} sectionId={reviewSectionId} sectionTitle={reviewSectionTitle} @@ -1204,16 +918,8 @@ export default function DiffPanel({ overflow: wordWrap ? "wrap" : "scroll", theme: resolveDiffThemeName(resolvedTheme), themeType: resolvedTheme as DiffThemeType, - unsafeCSS: DIFF_PANEL_UNSAFE_CSS, stickyHeaders: true, ...(loadDiffFiles ? { loadDiffFiles } : {}), - itemMetrics: { - diffHeaderHeight: 32, - hunkSeparatorHeight: 24, - paddingTop: 0, - paddingBottom: 0, - }, - layout: { paddingTop: 0, paddingBottom: 0, gap: 0 }, }} /> diff --git a/apps/web/src/components/diffs/AnnotatableCodeView.test.tsx b/apps/web/src/components/diffs/AnnotatableCodeView.test.tsx index bb8f8ebd9db..878ac230409 100644 --- a/apps/web/src/components/diffs/AnnotatableCodeView.test.tsx +++ b/apps/web/src/components/diffs/AnnotatableCodeView.test.tsx @@ -21,8 +21,8 @@ vi.mock("~/composerDraftStore", () => ({ }), })); -vi.mock("../files/LocalCommentAnnotation", () => ({ - LocalCommentAnnotation: () => null, +vi.mock("./DiffCommentAnnotation", () => ({ + DiffCommentAnnotation: () => null, })); vi.mock("../files/fileCommentAnnotations", () => ({ diff --git a/apps/web/src/components/diffs/AnnotatableCodeView.tsx b/apps/web/src/components/diffs/AnnotatableCodeView.tsx index 0decaa3acf5..f0d989a7d5b 100644 --- a/apps/web/src/components/diffs/AnnotatableCodeView.tsx +++ b/apps/web/src/components/diffs/AnnotatableCodeView.tsx @@ -6,7 +6,7 @@ import type { FileDiffMetadata, SelectedLineRange, } from "@pierre/diffs"; -import { CodeView, type CodeViewHandle, type CodeViewProps } from "@pierre/diffs/react"; +import type { CodeViewHandle } from "@pierre/diffs/react"; import type { ScopedThreadRef } from "@t3tools/contracts"; import { useCallback, useMemo, useState, type ReactNode, type Ref } from "react"; @@ -18,8 +18,9 @@ import { type ReviewCommentContext, } from "~/reviewCommentContext"; -import { LocalCommentAnnotation } from "../files/LocalCommentAnnotation"; import { nextFileCommentId } from "../files/fileCommentAnnotations"; +import { DiffCommentAnnotation } from "./DiffCommentAnnotation"; +import { StyledDiffCodeView, type StyledDiffCodeViewOptions } from "./StyledDiffCodeView"; interface DiffCommentAnnotationEntry { id: string; @@ -81,7 +82,7 @@ interface AnnotatableCodeViewProps { sectionId: string; sectionTitle: string; composerDraftTarget: ScopedThreadRef | DraftId; - options: NonNullable["options"]>; + options: StyledDiffCodeViewOptions; viewerRef?: Ref; className?: string; renderHeaderPrefix: ( @@ -237,9 +238,9 @@ export function AnnotatableCodeView({ const hasOpenComment = draft !== null; return ( - + key={codeViewKey} - {...(viewerRef ? { ref: viewerRef } : {})} + {...(viewerRef ? { viewerRef } : {})} {...(className ? { className } : {})} items={items} selectedLines={selectedLines} @@ -262,7 +263,7 @@ export function AnnotatableCodeView({ className={hasDraft ? "py-1" : "divide-y divide-border/30 border-y border-border/30"} > {annotation.metadata.entries.map((entry) => ( - { - it("renders the draft composer directly in the selected diff", () => { +describe("DiffCommentAnnotation", () => { + it("renders the shared draft composer directly in the selected diff", () => { const markup = renderToStaticMarkup( - , + , ); expect(markup).toContain("font-sans"); @@ -31,9 +31,35 @@ describe("LocalCommentAnnotation", () => { expect(markup).toContain("cursor-text"); }); + it("lets a pull-request diff configure actions without replacing the composer", () => { + const markup = renderToStaticMarkup( + , + allowEmpty: true, + onAction: vi.fn(), + }} + />, + ); + + expect(markup).toContain("Add a comment…"); + expect(markup).toContain(">Ask"); + expect(markup).toContain(">Add to review"); + expect(markup.match(/]*disabled[^>]*>Add to review<\/button>/)).not.toBeNull(); + const askButton = markup.match(/]*>.*?Ask<\/button>/)?.[0]; + expect(askButton).toBeDefined(); + expect(askButton).not.toContain(' disabled=""'); + }); + it("renders a saved comment without a nested card or redundant range label", () => { const markup = renderToStaticMarkup( - { it("renders draft text owned by the annotation wrapper", () => { const markup = renderToStaticMarkup( - void; +} + +interface DiffCommentAnnotationProps { kind: "draft" | "comment"; rangeLabel: string; text: string; onTextChange?: (text: string) => void; onCancel: () => void; onComment: (text: string) => void; - onDelete: () => void; + onDelete?: () => void; + placeholder?: string; + submitLabel?: string; + pending?: boolean; + secondaryAction?: DiffCommentSecondaryAction; } -export function LocalCommentAnnotation({ +/** The shared inline comment treatment for file previews, thread diffs, and pull-request diffs. */ +export function DiffCommentAnnotation({ kind, rangeLabel, text, @@ -22,36 +34,43 @@ export function LocalCommentAnnotation({ onCancel, onComment, onDelete, -}: LocalCommentAnnotationProps) { + placeholder = "Add a comment…", + submitLabel = "Comment", + pending = false, + secondaryAction, +}: DiffCommentAnnotationProps) { const [localDraftText, setLocalDraftText] = useState(""); const displayedText = kind === "draft" && !onTextChange ? localDraftText : text; + const trimmedText = displayedText.trim(); if (kind === "comment") { return (
event.stopPropagation()} >
); } return (
event.stopPropagation()} @@ -62,7 +81,7 @@ export function LocalCommentAnnotation({ className="relative inline-flex w-full rounded-md border border-border/50 bg-background/20 font-sans text-foreground transition-colors focus-within:border-border/70 [&_[data-slot=textarea]]:min-h-12 [&_[data-slot=textarea]]:cursor-text [&_[data-slot=textarea]]:px-2.5 [&_[data-slot=textarea]]:py-1.5 [&_[data-slot=textarea]]:font-sans [&_[data-slot=textarea]]:text-xs [&_[data-slot=textarea]]:leading-5 max-sm:[&_[data-slot=textarea]]:min-h-12" size="sm" value={displayedText} - placeholder="Add a comment…" + placeholder={placeholder} aria-label={`Comment on lines ${rangeLabel}`} onChange={(event) => (onTextChange ?? setLocalDraftText)(event.target.value)} onFocus={(event) => { @@ -74,9 +93,9 @@ export function LocalCommentAnnotation({ event.preventDefault(); onCancel(); } - if ((event.metaKey || event.ctrlKey) && event.key === "Enter" && displayedText.trim()) { + if ((event.metaKey || event.ctrlKey) && event.key === "Enter" && trimmedText) { event.preventDefault(); - onComment(displayedText.trim()); + onComment(trimmedText); } }} /> @@ -90,12 +109,19 @@ export function LocalCommentAnnotation({ > Cancel - + ) : null} +
diff --git a/apps/web/src/components/diffs/StyledDiffCodeView.test.tsx b/apps/web/src/components/diffs/StyledDiffCodeView.test.tsx new file mode 100644 index 00000000000..1baf3fcba98 --- /dev/null +++ b/apps/web/src/components/diffs/StyledDiffCodeView.test.tsx @@ -0,0 +1,58 @@ +import { renderToStaticMarkup } from "react-dom/server"; +import { beforeEach, describe, expect, it, vi } from "vite-plus/test"; + +const testState = vi.hoisted(() => ({ + codeViewClassName: null as string | null, + codeViewOptions: null as Record | null, +})); + +vi.mock("@pierre/diffs/react", () => ({ + CodeView: (props: { className: string; options: Record }) => { + testState.codeViewClassName = props.className; + testState.codeViewOptions = props.options; + return null; + }, +})); + +import { StyledDiffCodeView } from "./StyledDiffCodeView"; + +describe("StyledDiffCodeView", () => { + beforeEach(() => { + testState.codeViewClassName = null; + testState.codeViewOptions = null; + }); + + it("always pairs the shared diff styling with its virtualized geometry", () => { + const loadDiffFiles = vi.fn(async () => ({ + oldFile: { name: "before.ts", contents: "before\n" }, + newFile: { name: "after.ts", contents: "after\n" }, + })); + renderToStaticMarkup( + , + ); + + expect(testState.codeViewClassName).toBe("diff-render-surface min-h-0"); + expect(testState.codeViewOptions).toMatchObject({ + theme: "pierre-dark", + stickyHeaders: true, + loadDiffFiles, + itemMetrics: { + diffHeaderHeight: 32, + hunkSeparatorHeight: 24, + paddingTop: 0, + paddingBottom: 0, + }, + layout: { paddingTop: 0, paddingBottom: 0, gap: 0 }, + }); + expect(testState.codeViewOptions?.unsafeCSS).toEqual( + expect.stringContaining("[data-unmodified-lines]::before"), + ); + expect(testState.codeViewOptions?.unsafeCSS).toEqual( + expect.stringContaining(")[data-expand-index]\n [data-unmodified-lines]"), + ); + }); +}); diff --git a/apps/web/src/components/diffs/StyledDiffCodeView.tsx b/apps/web/src/components/diffs/StyledDiffCodeView.tsx new file mode 100644 index 00000000000..5714eb244f2 --- /dev/null +++ b/apps/web/src/components/diffs/StyledDiffCodeView.tsx @@ -0,0 +1,310 @@ +/* oxlint-disable eslint/no-restricted-imports -- This is the single styled adapter around Pierre's raw viewer. */ +import { + CodeView, + type CodeViewHandle, + type CodeViewProps, + type ControlledCodeViewProps, + type UncontrolledCodeViewProps, +} from "@pierre/diffs/react"; +/* oxlint-enable eslint/no-restricted-imports */ +import type { Ref } from "react"; + +const DIFF_VIEW_UNSAFE_CSS = ` +[data-diffs-header], +[data-diff], +[data-file], +[data-error-wrapper], +[data-virtualizer-buffer] { + --diffs-header-font-family: var(--font-sans) !important; + --diffs-font-family: var(--font-mono) !important; + --diffs-bg: var(--background) !important; + --diffs-light-bg: var(--background) !important; + --diffs-dark-bg: var(--background) !important; + --diffs-token-light-bg: transparent; + --diffs-token-dark-bg: transparent; + + --diffs-bg-context-override: color-mix(in srgb, var(--background) 97%, var(--foreground)); + --diffs-bg-hover-override: color-mix(in srgb, var(--background) 94%, var(--foreground)); + --diffs-bg-separator-override: color-mix(in srgb, var(--background) 95%, var(--foreground)); + --diffs-bg-buffer-override: color-mix(in srgb, var(--background) 90%, var(--foreground)); + + --diffs-bg-addition-override: light-dark( + color-mix(in srgb, var(--background) 50%, var(--success)), + color-mix(in srgb, var(--background) 70%, var(--success)) + ); + --diffs-bg-addition-number-override: light-dark( + color-mix(in srgb, var(--background) 35%, var(--success)), + color-mix(in srgb, var(--background) 60%, var(--success)) + ); + --diffs-bg-addition-hover-override: color-mix(in srgb, var(--background) 85%, var(--success)); + --diffs-bg-addition-emphasis-override: color-mix(in srgb, var(--background) 80%, var(--success)); + + --diffs-bg-deletion-override: light-dark( + color-mix(in srgb, var(--background) 50%, var(--destructive)), + color-mix(in srgb, var(--background) 70%, var(--destructive)) + ); + --diffs-bg-deletion-number-override: light-dark( + color-mix(in srgb, var(--background) 35%, var(--destructive)), + color-mix(in srgb, var(--background) 60%, var(--destructive)) + ); + --diffs-bg-deletion-hover-override: color-mix(in srgb, var(--background) 85%, var(--destructive)); + --diffs-bg-deletion-emphasis-override: color-mix( + in srgb, + var(--background) 80%, + var(--destructive) + ); + + background-color: var(--diffs-bg) !important; +} + +:is( + [data-line], + [data-line-annotation], + [data-merge-conflict], + [data-merge-conflict-actions], + [data-no-newline] +)[data-selected-line] { + --diffs-line-bg: light-dark( + color-mix( + in lab, + var(--background) 88%, + color-mix(in srgb, var(--background) 50%, var(--diffs-modified-base)) + ), + color-mix( + in lab, + var(--background) 80%, + color-mix(in srgb, var(--background) 70%, var(--diffs-modified-base)) + ) + ) !important; +} + +:is([data-gutter-buffer], [data-column-number])[data-selected-line] { + --diffs-line-bg: light-dark( + color-mix( + in lab, + var(--background) 91%, + color-mix(in srgb, var(--background) 35%, var(--diffs-modified-base)) + ), + color-mix( + in lab, + var(--background) 85%, + color-mix(in srgb, var(--background) 60%, var(--diffs-modified-base)) + ) + ) !important; +} + +[data-indicators="bars"] + :is([data-column-number], [data-gutter-buffer="annotation"])[data-selected-line] { + position: relative; +} + +[data-indicators="bars"] + :is([data-column-number], [data-gutter-buffer="annotation"])[data-selected-line]::before { + position: absolute !important; + inset-block: 0 !important; + inset-inline-start: 0 !important; + display: block !important; + width: 4px !important; + min-width: 4px !important; + max-width: 4px !important; + height: auto !important; + padding: 0 !important; + content: "" !important; + background-color: var(--diffs-modified-base) !important; + background-image: none !important; +} + +[data-file-info] { + background-color: var(--background) !important; + border-block-color: transparent !important; + color: var(--foreground) !important; +} + +[data-diffs-header] { + position: sticky !important; + top: 0; + z-index: 4; + background-color: var(--background) !important; + border-bottom-color: transparent !important; + align-items: center !important; + font-family: var(--font-sans) !important; + font-size: 12px !important; + line-height: 1 !important; + min-height: 32px !important; + padding-block: 6px !important; + padding-inline: 8px 12px !important; +} + +[data-diffs-header]:hover { + background-color: color-mix(in srgb, var(--background) 97%, var(--foreground)) !important; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"]) { + height: 24px !important; + margin-block: 0 !important; + background-color: var(--background) !important; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"]) + [data-separator-wrapper] { + padding-inline: 8px 12px !important; + background-color: transparent !important; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"]) + [data-separator-content] { + gap: 8px; + padding-inline: 0 !important; + background-color: transparent !important; + color: color-mix(in srgb, var(--foreground) 52%, var(--background)) !important; + font-family: var(--font-sans) !important; + font-size: 11px !important; + text-decoration: none !important; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"]) + [data-unmodified-lines] { + display: flex !important; + min-width: 0; + flex: 1 1 auto; + align-items: center; + gap: 8px; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"])[data-expand-index] + [data-unmodified-lines] { + cursor: pointer; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"]) + [data-unmodified-lines]::before, +:is([data-separator="line-info"], [data-separator="line-info-basic"]) + [data-unmodified-lines]::after { + width: auto; + height: 1px; + flex: 1 1 auto; + content: ""; + background-color: color-mix(in srgb, var(--background) 92%, var(--foreground)); +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"])[data-expand-index] + [data-separator-wrapper] { + grid-template-columns: 0 minmax(0, 1fr) !important; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"])[data-expand-index] + [data-separator-content] { + grid-column: 2 !important; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"]) + [data-expand-button] { + display: none !important; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( + [data-expand-button] + ) + [data-separator-content] { + cursor: pointer; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( + [data-expand-button] + ):hover + [data-separator-content] { + color: color-mix(in srgb, var(--foreground) 76%, var(--background)) !important; +} + +:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( + [data-expand-button] + ):hover + [data-unmodified-lines]::before, +:is([data-separator="line-info"], [data-separator="line-info-basic"]):has( + [data-expand-button] + ):hover + [data-unmodified-lines]::after { + background-color: color-mix(in srgb, var(--background) 84%, var(--foreground)); +} + +[data-diffs-header] [data-header-content] { + align-items: center !important; + line-height: 1 !important; +} + +[data-diffs-header] [data-metadata] { + align-items: center !important; + line-height: 1 !important; + font-variant-numeric: tabular-nums; +} + +[data-diffs-header] [data-additions-count], +[data-diffs-header] [data-deletions-count] { + font-family: var(--font-mono) !important; + font-size: 11px !important; + font-variant-numeric: tabular-nums; + line-height: 1 !important; +} + +[data-diffs-header] [data-change-icon], +[data-diffs-header] [data-rename-icon] { + display: block; + flex-shrink: 0; +} + +[data-title] { + cursor: pointer; + transition: + color 120ms ease, + text-decoration-color 120ms ease; + text-decoration: underline; + text-decoration-color: transparent; + text-underline-offset: 2px; + font-family: var(--font-sans) !important; +} + +[data-title]:hover { + color: color-mix(in srgb, var(--foreground) 84%, var(--primary)) !important; + text-decoration-color: currentColor; +} +`; + +export type StyledDiffCodeViewOptions = Omit< + NonNullable["options"]>, + "unsafeCSS" | "itemMetrics" | "layout" +>; + +type StyledDiffCodeViewProps = ( + | Omit, "options"> + | Omit, "options"> +) & { + readonly options?: StyledDiffCodeViewOptions; + readonly viewerRef?: Ref>; +}; + +/** The shared web CodeView surface: app styling and virtualized geometry stay paired here. */ +export function StyledDiffCodeView({ + options, + viewerRef, + className, + ...props +}: StyledDiffCodeViewProps) { + return ( + + {...props} + {...(viewerRef ? { ref: viewerRef } : {})} + className={className ? `diff-render-surface ${className}` : "diff-render-surface"} + options={{ + ...options, + unsafeCSS: DIFF_VIEW_UNSAFE_CSS, + itemMetrics: { + diffHeaderHeight: 32, + hunkSeparatorHeight: 24, + paddingTop: 0, + paddingBottom: 0, + }, + layout: { paddingTop: 0, paddingBottom: 0, gap: 0 }, + }} + /> + ); +} diff --git a/apps/web/src/components/files/FilePreviewPanel.tsx b/apps/web/src/components/files/FilePreviewPanel.tsx index a736cf96cd3..67ba43d9c2a 100644 --- a/apps/web/src/components/files/FilePreviewPanel.tsx +++ b/apps/web/src/components/files/FilePreviewPanel.tsx @@ -52,7 +52,7 @@ import { } from "./fileCommentAnnotations"; import { installFileEditorDismissal } from "./fileEditorDismissal"; import { resolveCenteredFileLineScrollTop } from "./fileLineReveal"; -import { LocalCommentAnnotation } from "./LocalCommentAnnotation"; +import { DiffCommentAnnotation } from "../diffs/DiffCommentAnnotation"; import { projectFileCacheKey, projectFileEditorCacheKey } from "./fileContentRevision"; import { fileBreadcrumbs } from "./filePath"; import { isMarkdownPreviewFile, setMarkdownTaskChecked } from "./filePreviewMode"; @@ -668,7 +668,7 @@ function EditableFileSurface({ renderAnnotation={(annotation) => (
{annotation.metadata.entries.map((entry) => ( - { + it("loads both sides with normalized paths and comparison-scoped cache keys", async () => { + const getDiffFileContents = vi.fn(async () => + AsyncResult.success({ oldContents: "before\n", newContents: "after\n" }), + ); + const load = createGitDiffFileContentsLoader(getDiffFileContents, SOURCE); + + await expect(load(fileDiff())).resolves.toEqual({ + oldFile: { + name: "src/old-name.ts", + contents: "before\n", + cacheKey: "comparison-1:old:src/old-name.ts", + }, + newFile: { + name: "src/new-name.ts", + contents: "after\n", + cacheKey: "comparison-1:new:src/new-name.ts", + }, + }); + expect(getDiffFileContents).toHaveBeenCalledWith({ + environmentId: "environment-1", + input: { + cwd: "/workspace", + sourceKind: "branch-range", + changeType: "rename-changed", + baseRef: "main", + headRef: "feature", + oldPath: "src/old-name.ts", + newPath: "src/new-name.ts", + }, + }); + }); + + it("loads a pure rename from its one shared file", async () => { + const getDiffFileContents = vi.fn(async () => + AsyncResult.success({ oldContents: "same\n", newContents: "same\n" }), + ); + const load = createGitDiffFileContentsLoader(getDiffFileContents, SOURCE); + + await expect(load(fileDiff("rename-pure"))).resolves.toMatchObject({ + oldFile: null, + newFile: { name: "src/new-name.ts", contents: "same\n" }, + }); + }); + + it("passes command failures through to Pierre's expansion handling", async () => { + const failure = new Error("revision is not available locally"); + const getDiffFileContents = vi.fn(async () => + AsyncResult.failure(Cause.fail(failure)), + ); + const load = createGitDiffFileContentsLoader(getDiffFileContents, SOURCE); + + await expect(load(fileDiff())).rejects.toBe(failure); + }); +}); diff --git a/apps/web/src/lib/diffFileContents.ts b/apps/web/src/lib/diffFileContents.ts new file mode 100644 index 00000000000..2dc4e5558bd --- /dev/null +++ b/apps/web/src/lib/diffFileContents.ts @@ -0,0 +1,124 @@ +import type { FileDiffContentsLoader } from "@pierre/diffs"; +import { + squashAtomCommandFailure, + type AtomCommandResult, +} from "@t3tools/client-runtime/state/runtime"; +import type { + EnvironmentId, + PullRequestDiffFileContentsInput, + PullRequestDiffFileContentsResult, + PullRequestRef, + ReviewDiffFileContentsInput, + ReviewDiffFileContentsResult, + ReviewDiffPreviewSourceKind, +} from "@t3tools/contracts"; + +import { resolveFileDiffPath } from "./diffRendering"; + +interface GitDiffFileContentsSource { + readonly environmentId: EnvironmentId; + readonly cwd: string; + readonly sourceKind: ReviewDiffPreviewSourceKind; + readonly baseRef: string | null; + readonly headRef: string | null; + /** The comparison identity Pierre carries into its hydrated render cache. */ + readonly cacheKey: string; +} + +interface PullRequestDiffFileContentsSource { + readonly environmentId: EnvironmentId; + readonly reference: PullRequestRef; + readonly commit: string | null; + readonly cacheKey: string; +} + +type GetDiffFileContents = (request: { + readonly environmentId: EnvironmentId; + readonly input: ReviewDiffFileContentsInput; +}) => Promise>; + +type GetPullRequestDiffFileContents = (request: { + readonly environmentId: EnvironmentId; + readonly input: PullRequestDiffFileContentsInput; +}) => Promise>; + +function createDiffFileContentsLoader( + load: (input: { + readonly changeType: PullRequestDiffFileContentsInput["changeType"]; + readonly oldPath: string; + readonly newPath: string; + }) => Promise<{ readonly oldContents: string; readonly newContents: string }>, + cacheKey: string, +): FileDiffContentsLoader { + return async (fileDiff) => { + const newPath = resolveFileDiffPath(fileDiff); + const oldPath = fileDiff.prevName + ? resolveFileDiffPath({ ...fileDiff, name: fileDiff.prevName }) + : newPath; + const contents = await load({ changeType: fileDiff.type, oldPath, newPath }); + const newFile = { + name: newPath, + contents: contents.newContents, + cacheKey: `${cacheKey}:new:${newPath}`, + }; + if (fileDiff.type === "rename-pure") { + return { oldFile: null, newFile }; + } + return { + oldFile: { + name: oldPath, + contents: contents.oldContents, + cacheKey: `${cacheKey}:old:${oldPath}`, + }, + newFile, + }; + }; +} + +/** Turns the host's Git file-content RPC into the full-file loader Pierre uses for hunk expansion. */ +export function createGitDiffFileContentsLoader( + getDiffFileContents: GetDiffFileContents, + source: GitDiffFileContentsSource, +): FileDiffContentsLoader { + return createDiffFileContentsLoader(async ({ changeType, oldPath, newPath }) => { + const result = await getDiffFileContents({ + environmentId: source.environmentId, + input: { + cwd: source.cwd, + sourceKind: source.sourceKind, + changeType, + baseRef: source.baseRef, + headRef: source.headRef, + oldPath, + newPath, + }, + }); + if (result._tag !== "Success") { + throw squashAtomCommandFailure(result); + } + return result.value; + }, source.cacheKey); +} + +/** Loads host-backed PR files, which may name revisions this checkout has never fetched. */ +export function createPullRequestDiffFileContentsLoader( + getDiffFileContents: GetPullRequestDiffFileContents, + source: PullRequestDiffFileContentsSource, +): FileDiffContentsLoader { + return createDiffFileContentsLoader(async ({ changeType, oldPath, newPath }) => { + const result = await getDiffFileContents({ + environmentId: source.environmentId, + input: { + ...source.reference, + ...(source.commit === null ? {} : { commit: source.commit }), + changeType, + oldPath, + newPath, + }, + }); + if (result._tag !== "Success") { + throw squashAtomCommandFailure(result); + } + return result.value; + }, source.cacheKey); +} diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index 1656da584ae..eabd058f6be 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -541,6 +541,23 @@ export const PullRequestDiffResult = Schema.Struct({ }); export type PullRequestDiffResult = typeof PullRequestDiffResult.Type; +/** The complete old and new files Pierre needs to open omitted context in a host-backed patch. */ +export const PullRequestDiffFileContentsInput = Schema.Struct({ + ...PullRequestRef.fields, + /** One commit's own comparison; absent means the whole change request. */ + commit: Schema.optional(TrimmedNonEmptyString), + changeType: Schema.Literals(["change", "rename-pure", "rename-changed", "new", "deleted"]), + oldPath: TrimmedNonEmptyString, + newPath: TrimmedNonEmptyString, +}); +export type PullRequestDiffFileContentsInput = typeof PullRequestDiffFileContentsInput.Type; + +export const PullRequestDiffFileContentsResult = Schema.Struct({ + oldContents: Schema.String, + newContents: Schema.String, +}); +export type PullRequestDiffFileContentsResult = typeof PullRequestDiffFileContentsResult.Type; + export const PullRequestActionInput = Schema.Struct({ ...PullRequestRef.fields, action: PullRequestAction, diff --git a/vite.config.ts b/vite.config.ts index b2498611198..585428028d3 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -109,6 +109,12 @@ export default defineConfig({ message: "Import from an explicit @t3tools/client-runtime/* subpath. The package has no root export.", }, + { + name: "@pierre/diffs/react", + importNames: ["CodeView"], + message: + "Use StyledDiffCodeView so web diff surfaces share styling and virtualized geometry.", + }, ], }, ], From 8b10e5bf095d3a624908feb7e2b136cc211ddbf2 Mon Sep 17 00:00:00 2001 From: maria-rcks <254055478+maria-rcks@users.noreply.github.com> Date: Thu, 6 Aug 2026 13:34:31 -0400 Subject: [PATCH 02/10] feat(pull-requests): expand omitted diff context --- apps/server/src/auth/RpcAuthorization.ts | 1 + .../src/pullRequest/GitHubPullRequestCli.ts | 110 ++++++++++++++++++ .../pullRequest/GitHubPullRequestProvider.ts | 5 + .../src/pullRequest/GitLabPullRequestCli.ts | 105 +++++++++++++++++ .../src/pullRequest/PullRequestProvider.ts | 17 +++ .../src/pullRequest/PullRequestService.ts | 30 +++++ .../src/pullRequest/gitLabMergeRequestJson.ts | 17 +++ apps/server/src/ws.ts | 6 + .../pullRequest/PullRequestCodeTab.tsx | 53 ++++++--- .../PullRequestReviewAnnotation.tsx | 70 +---------- .../pullRequest/pullRequestDiff.logic.test.ts | 5 +- .../client-runtime/src/state/pullRequests.ts | 19 +++ packages/contracts/src/rpc.ts | 13 +++ 13 files changed, 367 insertions(+), 84 deletions(-) diff --git a/apps/server/src/auth/RpcAuthorization.ts b/apps/server/src/auth/RpcAuthorization.ts index 9629ef1c8b2..96f5a131b72 100644 --- a/apps/server/src/auth/RpcAuthorization.ts +++ b/apps/server/src/auth/RpcAuthorization.ts @@ -54,6 +54,7 @@ export const RPC_REQUIRED_SCOPES = { [WS_METHODS.pullRequestsListStats]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsDetail]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsDiff]: AuthOrchestrationReadScope, + [WS_METHODS.pullRequestsDiffFileContents]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsRunAction]: AuthOrchestrationOperateScope, [WS_METHODS.pullRequestsComment]: AuthOrchestrationOperateScope, [WS_METHODS.pullRequestsSubmitReview]: AuthOrchestrationOperateScope, diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.ts index 682e243b66c..a3fe7c71313 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.ts @@ -165,6 +165,8 @@ export type GitHubPullRequestCliError = /** A large pull request can produce a multi-megabyte patch; past this it is truncated. */ const DIFF_MAX_OUTPUT_BYTES = 8 * 1024 * 1024; const DIFF_TIMEOUT_MS = 60_000; +/** Pierre expansion is for source files, not blobs large enough to stall a review surface. */ +const DIFF_FILE_MAX_OUTPUT_BYTES = 1024 * 1024; /** What the files API serves at most in one response, which is what one slice is made of. */ const DIFF_FILES_PAGE_SIZE = 100; @@ -287,6 +289,20 @@ export class GitHubPullRequestCli extends Context.Service< readonly commit?: string | undefined; }) => Effect.Effect; + readonly getPullRequestDiffFileContents: (input: { + readonly cwd: string; + readonly repository: string; + readonly host: string; + readonly number: number; + readonly commit?: string | undefined; + readonly changeType: "change" | "rename-pure" | "rename-changed" | "new" | "deleted"; + readonly oldPath: string; + readonly newPath: string; + }) => Effect.Effect< + { readonly oldContents: string; readonly newContents: string }, + GitHubPullRequestCliError + >; + readonly listReviewThreadComments: (input: { readonly cwd: string; readonly repository: string; @@ -708,6 +724,98 @@ export const make = Effect.gen(function* () { ); }; + const getPullRequestDiffFileContents: GitHubPullRequestCli["Service"]["getPullRequestDiffFileContents"] = + (input) => + Effect.gen(function* () { + if (input.commit !== undefined && !isCommitSha(input.commit)) { + return yield* new GitHubDiffCommitError({ command: "gh", cwd: input.cwd }); + } + const { owner, name } = parseRepositorySelector(input.repository); + const refsResult = yield* github.execute({ + cwd: input.cwd, + args: [ + "api", + "--hostname", + input.host, + input.commit === undefined + ? `repos/${owner}/${name}/pulls/${input.number}` + : `repos/${owner}/${name}/commits/${input.commit}`, + "--jq", + input.commit === undefined + ? "[.base.sha, .head.sha] | @tsv" + : "[.parents[0].sha, .sha] | @tsv", + ], + maxOutputBytes: 1024, + timeoutMs: DIFF_TIMEOUT_MS, + }); + const [baseRef, headRef, ...extraRefs] = refsResult.stdout.trim().split("\t"); + if ( + refsResult.stdoutTruncated || + !baseRef || + !headRef || + extraRefs.length > 0 || + !isCommitSha(baseRef) || + !isCommitSha(headRef) + ) { + return yield* new GitHubPullRequestReadError({ + command: "gh", + cwd: input.cwd, + operation: "getPullRequestDiffFileContents", + cause: new Error("GitHub returned no usable base and head revisions."), + }); + } + + const readFile = (revision: string, filePath: string) => + github + .execute({ + cwd: input.cwd, + args: [ + "api", + "--hostname", + input.host, + "--header", + "Accept: application/vnd.github.raw+json", + `repos/${owner}/${name}/contents/${filePath + .split("/") + .map(encodeURIComponent) + .join("/")}?ref=${encodeURIComponent(revision)}`, + ], + maxOutputBytes: DIFF_FILE_MAX_OUTPUT_BYTES, + timeoutMs: DIFF_TIMEOUT_MS, + }) + .pipe( + Effect.flatMap((result) => + result.stdoutTruncated || result.stdout.includes("\0") + ? Effect.fail( + new GitHubPullRequestReadError({ + command: "gh", + cwd: input.cwd, + operation: "getPullRequestDiffFileContents", + cause: new Error( + result.stdoutTruncated + ? `The diff file '${filePath}' exceeds the 1 MB expansion limit.` + : `The diff file '${filePath}' is binary.`, + ), + }), + ) + : Effect.succeed(result.stdout), + ), + ); + + const [oldContents, newContents] = yield* Effect.all( + [ + input.changeType === "new" + ? Effect.succeed("") + : readFile(baseRef, input.oldPath), + input.changeType === "deleted" + ? Effect.succeed("") + : readFile(headRef, input.newPath), + ], + { concurrency: 2 }, + ); + return { oldContents, newContents }; + }); + return GitHubPullRequestCli.of({ getViewerLogin: (input) => github.execute({ cwd: input.cwd, args: ["api", "user", "--jq", ".login"] }).pipe( @@ -947,6 +1055,8 @@ export const make = Effect.gen(function* () { ); }, + getPullRequestDiffFileContents, + listReviewThreadComments: (input) => Effect.gen(function* () { const { owner, name } = parseRepositorySelector(input.repository); diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts index 7202f162c4e..ec50319783b 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts @@ -263,6 +263,11 @@ export const make = Effect.gen(function* () { getDiff: (input) => cli.getPullRequestDiff(input).pipe(Effect.mapError(fail("getDiff"))), + getDiffFileContents: (input) => + cli + .getPullRequestDiffFileContents(input) + .pipe(Effect.mapError(fail("getDiffFileContents"))), + listReviewerCandidates: (input) => cli.listReviewerCandidates(input).pipe(Effect.mapError(fail("listReviewerCandidates"))), diff --git a/apps/server/src/pullRequest/GitLabPullRequestCli.ts b/apps/server/src/pullRequest/GitLabPullRequestCli.ts index a7c968c80cb..2cb8e2757fa 100644 --- a/apps/server/src/pullRequest/GitLabPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitLabPullRequestCli.ts @@ -19,6 +19,7 @@ import type { import * as GitLabCli from "../sourceControl/GitLabCli.ts"; import { + decodeCommitDiffRefsJson, decodeCommitsJson, decodeDiffRefsJson, decodeDiscussionsJson, @@ -148,6 +149,7 @@ const COMMIT_PAGE_SIZE = 100; const CONVERSATION_PAGES = 10; const DIFF_MAX_OUTPUT_BYTES = 8 * 1024 * 1024; const DIFF_TIMEOUT_MS = 60_000; +const DIFF_FILE_MAX_OUTPUT_BYTES = 1024 * 1024; export interface GitLabMergeRequestListBatch { readonly items: ReadonlyArray; @@ -213,6 +215,19 @@ export class GitLabPullRequestCli extends Context.Service< readonly commit?: string | undefined; }) => Effect.Effect; + readonly getMergeRequestDiffFileContents: (input: { + readonly cwd: string; + readonly repository: string; + readonly number: number; + readonly commit?: string | undefined; + readonly changeType: "change" | "rename-pure" | "rename-changed" | "new" | "deleted"; + readonly oldPath: string; + readonly newPath: string; + }) => Effect.Effect< + { readonly oldContents: string; readonly newContents: string }, + GitLabPullRequestCliError + >; + readonly getProjectMergeCapabilities: (input: { readonly cwd: string; readonly repository: string; @@ -672,6 +687,40 @@ export const make = Effect.gen(function* () { }), ); + const getCommitDiffRefs = (input: { + readonly cwd: string; + readonly repository: string; + readonly commit: string; + }): Effect.Effect => + api({ + cwd: input.cwd, + path: `projects/${projectPath(input.repository)}/repository/commits/${input.commit}`, + }).pipe( + Effect.flatMap((result) => { + const decoded = decodeCommitDiffRefsJson(result.stdout.trim()); + if (!Result.isSuccess(decoded)) { + return Effect.fail( + new GitLabMergeRequestReadError({ + command: "glab", + cwd: input.cwd, + operation: "getMergeRequestDiffFileContents", + cause: decoded.failure, + }), + ); + } + return decoded.success === null + ? Effect.fail( + new GitLabMergeRequestReadError({ + command: "glab", + cwd: input.cwd, + operation: "getMergeRequestDiffFileContents", + cause: new Error("GitLab returned no usable parent for the commit."), + }), + ) + : Effect.succeed(decoded.success); + }), + ); + /** * The merge request itself, which several calls need for different parts of it: the detail for * everything, and the reviewer paths for the ids GitLab writes a reviewer set with. @@ -794,6 +843,62 @@ export const make = Effect.gen(function* () { : diffPage({ ...target, page }); }, + getMergeRequestDiffFileContents: (input) => + Effect.gen(function* () { + if (input.commit !== undefined && !isCommitSha(input.commit)) { + return yield* Effect.fail( + new GitLabDiffCommitError({ command: "glab", cwd: input.cwd }), + ); + } + const refs = yield* input.commit === undefined + ? getDiffRefs(input) + : getCommitDiffRefs({ + cwd: input.cwd, + repository: input.repository, + commit: input.commit, + }); + + const readFile = (revision: string, filePath: string) => + api({ + cwd: input.cwd, + path: `projects/${projectPath(input.repository)}/repository/files/${encodeURIComponent( + filePath, + )}/raw?ref=${encodeURIComponent(revision)}`, + maxOutputBytes: DIFF_FILE_MAX_OUTPUT_BYTES, + timeoutMs: DIFF_TIMEOUT_MS, + }).pipe( + Effect.flatMap((result) => + result.stdoutTruncated || result.stdout.includes("\0") + ? Effect.fail( + new GitLabMergeRequestReadError({ + command: "glab", + cwd: input.cwd, + operation: "getMergeRequestDiffFileContents", + cause: new Error( + result.stdoutTruncated + ? `The diff file '${filePath}' exceeds the 1 MB expansion limit.` + : `The diff file '${filePath}' is binary.`, + ), + }), + ) + : Effect.succeed(result.stdout), + ), + ); + + const [oldContents, newContents] = yield* Effect.all( + [ + input.changeType === "new" + ? Effect.succeed("") + : readFile(refs.baseSha, input.oldPath), + input.changeType === "deleted" + ? Effect.succeed("") + : readFile(refs.headSha, input.newPath), + ], + { concurrency: 2 }, + ); + return { oldContents, newContents }; + }), + getProjectMergeCapabilities: (input) => api({ cwd: input.cwd, diff --git a/apps/server/src/pullRequest/PullRequestProvider.ts b/apps/server/src/pullRequest/PullRequestProvider.ts index 127ca6b4e87..6492cdc2803 100644 --- a/apps/server/src/pullRequest/PullRequestProvider.ts +++ b/apps/server/src/pullRequest/PullRequestProvider.ts @@ -157,6 +157,11 @@ export interface ProviderDiffSlice { readonly nextCursor: string | null; } +export interface ProviderDiffFileContents { + readonly oldContents: string; + readonly newContents: string; +} + export interface ProviderRepositoryRef { readonly cwd: string; /** Provider-native repository identity, e.g. `owner/repo` or `group/subgroup/project`. */ @@ -276,6 +281,18 @@ export interface PullRequestProviderApi { }, ) => Effect.Effect; + /** + * Full files at the exact revisions the host used for its patch. Optional where the provider + * exposes no diff at all; the service refuses expansion there just as it refuses the patch. + */ + readonly getDiffFileContents?: (input: ProviderRepositoryRef & { + readonly number: number; + readonly commit?: string | undefined; + readonly changeType: "change" | "rename-pure" | "rename-changed" | "new" | "deleted"; + readonly oldPath: string; + readonly newPath: string; + }) => Effect.Effect; + readonly runAction: ( input: ProviderRepositoryRef & { readonly number: number; diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 612784dc2a5..cc193018337 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -14,6 +14,8 @@ import { type PullRequestActionInput, type PullRequestCommentInput, type PullRequestDetail, + type PullRequestDiffFileContentsInput, + type PullRequestDiffFileContentsResult, type PullRequestDiffStat, type PullRequestDiffInput, type PullRequestDiffResult, @@ -104,6 +106,9 @@ export class PullRequestService extends Context.Service< readonly diff: ( input: PullRequestDiffInput, ) => Effect.Effect; + readonly diffFileContents: ( + input: PullRequestDiffFileContentsInput, + ) => Effect.Effect; readonly runAction: (input: PullRequestActionInput) => Effect.Effect; readonly comment: (input: PullRequestCommentInput) => Effect.Effect; readonly submitReview: ( @@ -895,6 +900,30 @@ export const make = Effect.gen(function* () { ), ); + const diffFileContents: PullRequestService["Service"]["diffFileContents"] = (input) => + requireProject(input).pipe( + Effect.flatMap((project) => { + const read = project.api.getDiffFileContents; + return project.api.capabilities.diff && read + ? read({ + cwd: project.project.workspaceRoot, + repository: project.repository, + host: project.host, + number: input.number, + ...(input.commit === undefined ? {} : { commit: input.commit }), + changeType: input.changeType, + oldPath: input.oldPath, + newPath: input.newPath, + }).pipe(Effect.mapError(toPullRequestError("diffFileContents"))) + : Effect.fail( + new PullRequestOperationError({ + operation: "diffFileContents", + detail: "This host cannot expand unchanged pull request lines.", + }), + ); + }), + ); + const runAction: PullRequestService["Service"]["runAction"] = (input) => requireProject(input).pipe( Effect.flatMap((project): Effect.Effect => { @@ -1453,6 +1482,7 @@ export const make = Effect.gen(function* () { listStats, detail, diff, + diffFileContents, runAction: invalidatedByMutation(runAction), comment: invalidatedByMutation(comment), submitReview: invalidatedByMutation(submitReview), diff --git a/apps/server/src/pullRequest/gitLabMergeRequestJson.ts b/apps/server/src/pullRequest/gitLabMergeRequestJson.ts index 34cd51f000f..a42f12f3213 100644 --- a/apps/server/src/pullRequest/gitLabMergeRequestJson.ts +++ b/apps/server/src/pullRequest/gitLabMergeRequestJson.ts @@ -137,6 +137,7 @@ const RawCommitSchema = Schema.Struct({ title: Schema.optional(Schema.NullOr(Schema.String)), committed_date: Schema.optional(Schema.NullOr(Schema.String)), created_at: Schema.optional(Schema.NullOr(Schema.String)), + parent_ids: Schema.optional(Schema.Array(Schema.String)), }); const RawDiffSchema = Schema.Struct({ @@ -347,6 +348,7 @@ const decodeMergeRequest = decodeJsonResult(RawMergeRequestSchema); const decodeNoteEntry = Schema.decodeUnknownExit(RawNoteSchema); const decodeUserEntry = Schema.decodeUnknownExit(RawUserSchema); const decodeCommitEntry = Schema.decodeUnknownExit(RawCommitSchema); +const decodeCommit = decodeJsonResult(RawCommitSchema); const decodeDiffEntry = Schema.decodeUnknownExit(RawDiffSchema); const decodeDiscussionEntry = Schema.decodeUnknownExit(RawDiscussionSchema); const decodeDiffRefs = decodeJsonResult(RawDiffRefsSchema); @@ -594,6 +596,21 @@ export function decodeCommitsJson( return Result.succeed(commits.toReversed()); } +/** The exact comparison GitLab uses for a commit-scoped diff. */ +export function decodeCommitDiffRefsJson( + raw: string, +): Result.Result { + const decoded = decodeCommit(raw); + if (!Result.isSuccess(decoded)) return Result.fail(decoded.failure); + const baseSha = trimmed(decoded.success.parent_ids?.[0]); + const headSha = trimmed(decoded.success.id); + return Result.succeed( + baseSha === null || headSha === null + ? null + : { baseSha, headSha, startSha: baseSha }, + ); +} + function diffHeaderPaths(raw: Schema.Schema.Type): { readonly from: string; readonly to: string; diff --git a/apps/server/src/ws.ts b/apps/server/src/ws.ts index 6a299aa49b7..3b796f594c3 100644 --- a/apps/server/src/ws.ts +++ b/apps/server/src/ws.ts @@ -1593,6 +1593,12 @@ const makeWsRpcLayer = ( observeRpcEffect(WS_METHODS.pullRequestsDiff, pullRequests.diff(input), { "rpc.aggregate": "pull-requests", }), + [WS_METHODS.pullRequestsDiffFileContents]: (input) => + observeRpcEffect( + WS_METHODS.pullRequestsDiffFileContents, + pullRequests.diffFileContents(input), + { "rpc.aggregate": "pull-requests" }, + ), [WS_METHODS.pullRequestsRunAction]: (input) => observeRpcEffect(WS_METHODS.pullRequestsRunAction, pullRequests.runAction(input), { "rpc.aggregate": "pull-requests", diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index f75a6301167..e50f70a5c52 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -1,5 +1,5 @@ import type { CodeViewItem, DiffLineAnnotation, SelectedLineRange } from "@pierre/diffs"; -import { CodeView, type CodeViewDiffItem } from "@pierre/diffs/react"; +import type { CodeViewDiffItem } from "@pierre/diffs/react"; import type { EnvironmentId, PullRequestDetail, @@ -15,6 +15,7 @@ import { Columns2Icon, MessageSquareOffIcon, Rows3Icon, + SparklesIcon, TextWrapIcon, TriangleAlertIcon, } from "lucide-react"; @@ -36,12 +37,15 @@ import { type RenderablePatch, } from "~/lib/diffRendering"; import { cn } from "~/lib/utils"; +import { createPullRequestDiffFileContentsLoader } from "~/lib/diffFileContents"; import { buildDiffReviewComment, type ReviewCommentContext } from "~/reviewCommentContext"; import { pullRequestEnvironment } from "~/state/pullRequests"; import { useEnvironmentQuery } from "~/state/query"; import { useAtomCommand } from "~/state/use-atom-command"; import { DiffWorkerPoolProvider } from "../DiffWorkerPoolProvider"; +import { DiffCommentAnnotation } from "../diffs/DiffCommentAnnotation"; +import { StyledDiffCodeView } from "../diffs/StyledDiffCodeView"; import { Button } from "../ui/button"; import { Collapsible, CollapsiblePanel, CollapsibleTrigger } from "../ui/collapsible"; import { @@ -54,11 +58,7 @@ import { Skeleton } from "../ui/skeleton"; import { toastManager } from "../ui/toast"; import { Toggle, ToggleGroup } from "../ui/toggle-group"; import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; -import { - PendingReviewCommentCard, - ReviewCommentComposer, - ReviewThreadCard, -} from "./PullRequestReviewAnnotation"; +import { PendingReviewCommentCard, ReviewThreadCard } from "./PullRequestReviewAnnotation"; import { PullRequestReviewBar } from "./PullRequestReviewBar"; import { isFileDiffCollapsed, @@ -287,6 +287,17 @@ export function PullRequestCodeTab({ const setThreadResolution = useAtomCommand(pullRequestEnvironment.setThreadResolution, { reportFailure: false, }); + const getDiffFileContents = useAtomCommand(pullRequestEnvironment.diffFileContents); + const loadDiffFiles = useMemo( + () => + createPullRequestDiffFileContentsLoader(getDiffFileContents, { + environmentId, + reference, + commit, + cacheKey: `pull-request:${referenceKey}:${detail.updatedAt}:${commit ?? "all"}`, + }), + [commit, detail.updatedAt, environmentId, getDiffFileContents, reference, referenceKey], + ); // What is offered is the intersection of two different questions: what this host can do at // all, and what this account may do on this repository. Either one saying no means a control @@ -315,7 +326,9 @@ export function PullRequestCodeTab({ const cacheKey = `pull-request:${scopeKey}:${resolvedTheme}:${slice.cursor ?? "first"}:${fnv1a32(slice.patch)}`; const cached = parseCache.current.get(cacheKey); if (cached) return cached; - const parsed = getRenderablePatch(slice.patch, cacheKey); + const parsed = getRenderablePatch(slice.patch, cacheKey, { + compactPartialHunkOffsets: true, + }); if (parsed) parseCache.current.set(cacheKey, parsed); return parsed; }), @@ -940,8 +953,8 @@ export function PullRequestCodeTab({ {/* The viewer virtualizes against the element it is told is scrolling and places its rows absolutely, so it has to own that element — the thread diff panel hands it the same one. Scrolling from a parent instead leaves it painting over its neighbours. */} - - className="diff-render-surface min-h-0 flex-1 overflow-auto" + + className="min-h-0 flex-1 overflow-auto" items={items} selectedLines={selectedLines} onSelectedLinesChange={setSelectedLines} @@ -952,8 +965,7 @@ export function PullRequestCodeTab({ theme: resolveDiffThemeName(resolvedTheme), themeType: resolvedTheme, stickyHeaders: true, - itemMetrics: { diffHeaderHeight: 33 }, - layout: { paddingTop: 0, paddingBottom: 8, gap: 8 }, + loadDiffFiles, enableGutterUtility: canCommentOnLines && draft === null, enableLineSelection: canCommentOnLines && draft === null, // Two gestures reach the same place: dragging the line numbers selects a range, @@ -1024,17 +1036,26 @@ export function PullRequestCodeTab({ /> ))} {annotation.metadata.draft && draft ? ( - askAboutSelection(draft, question) } + ? { + secondaryAction: { + label: "Ask", + icon: , + allowEmpty: true, + onAction: (question: string) => askAboutSelection(draft, question), + }, + } : {})} onCancel={() => { setDraft(null); setSelectedLines(null); }} - onSubmit={(body) => { + onComment={(body) => { addComment(reviewKey, { id: nextPendingReviewCommentId(), path: draft.path, diff --git a/apps/web/src/components/pullRequest/PullRequestReviewAnnotation.tsx b/apps/web/src/components/pullRequest/PullRequestReviewAnnotation.tsx index 0b0cc8f3655..ddc2afaca6b 100644 --- a/apps/web/src/components/pullRequest/PullRequestReviewAnnotation.tsx +++ b/apps/web/src/components/pullRequest/PullRequestReviewAnnotation.tsx @@ -1,6 +1,6 @@ /** - * What sits under a line of the diff: a conversation already on the host, a comment queued for - * the review being written, or the box that writes one. + * Pull-request-specific annotations: conversations already on the host and comments queued for + * the review being written. New comment composition uses the shared diff annotation. */ import type { PullRequestReviewThread } from "@t3tools/contracts"; import { @@ -8,7 +8,6 @@ import { CircleIcon, HammerIcon, MessageSquareIcon, - SparklesIcon, Trash2Icon, } from "lucide-react"; import { useState } from "react"; @@ -25,7 +24,7 @@ import type { PendingReviewComment } from "./pullRequestReviewStore"; const CARD_CLASS = "mx-3 my-2 rounded-xl border border-border/70 bg-background p-3 text-sm shadow-sm"; -/** Sends on ⌘/Ctrl+Enter and abandons on Escape, which is what every other composer here does. */ +/** Sends a reply on ⌘/Ctrl+Enter and abandons it on Escape. */ function submitKeys(input: { readonly value: string; readonly onSubmit: () => void; @@ -43,69 +42,6 @@ function submitKeys(input: { }; } -/** The box that writes a new line comment into the review being drafted. */ -export function ReviewCommentComposer({ - lineLabel, - pending, - onAsk, - onCancel, - onSubmit, -}: { - lineLabel: string; - pending: boolean; - /** - * Hands the whole selection to an agent instead of the host. Absent where there is nothing to - * hand it to, since a button that cannot do its one thing is worse than no button. - */ - onAsk?: ((question: string) => void) | undefined; - onCancel: () => void; - onSubmit: (body: string) => void; -}) { - const [body, setBody] = useState(""); - const submit = () => { - const trimmed = body.trim(); - if (trimmed.length > 0) onSubmit(trimmed); - }; - return ( -
event.stopPropagation()} - > -
- - Comment on {lineLabel} -
-