Add inline review comments to git diff pane (#530)

* Add inline git diff review comments

Adds a review draft store keyed per diff target, inline gutter + thread
rendering in the diff pane, and a generated review attachment that flows
through the composer and agent providers.

* Strip generated review attachments before saving to draft store

Drafts persist only UserComposerAttachment (no review kind), so filter the
auto-generated review attachment out when seeding the draft or restoring
after a failed auto-submit.

* Fix inline review typecheck and test harness drift

* Unslop inline review feature

- Fix stale-closure bug in composer queueMessage: clear drafts for the
  actually-queued attachments, not the latest props
- Drop dead useEffect mirroring initialBody in inline review editor
  (parent already remounts via key)
- Restore typed AgentAttachment signature on renderPromptAttachmentAsText;
  delete schema redispatch helper and drop null-checks across 5 providers
- Inline single-use review prompt formatters and replace marker switch
  with const map
- Drop normalizeComment field-copy redundancy after type guard
- Remove __reviewDraftStoreTestUtils export; test migration via
  rehydrate() public path
- Refactor findTarget to return the target line directly (no non-null
  assertion at call site)
- Drop redundant isGeneratedReviewAttachment helper; inline kind check

* Update review pill to withUnistyles after rebase

Main's unistyles refactor stripped useUnistyles in favor of withUnistyles +
StyleSheet. Convert ReviewAttachmentPill to use ThemedCircleDot + ICON_SIZE so
the icon picks up the foreground-muted mapping like the other pills.

* Fix lint and typecheck after rebase onto main

Resolve drift after main's withUnistyles refactor:
- Composer complexity: dedup voice?.isVoiceSwitching to drop to 20
- Move per-comment Pressable handlers in git-diff-inline-review into
  CommentRow subcomponent so callbacks are stable
- Hoist style/onPress factories to module level (iconButton,
  iconButtonDestructive, ghostButton)
- Memoize style arrays in git-diff-pane review wrappers
- Replace inline {} placeholders with useMemo styles
- review-draft-store: extract applyCommentUpdates to avoid map-spread,
  include input.key in useMemo deps
- Test fixtures: hoist EMPTY_COMMENTS, add buildReviewActions helper

* Drop GitDiffPane default param to keep complexity at 20

After the rebase, main's `enabled = true` default param combined with the
inline review additions pushed cyclomatic complexity to 21. Drop the
default and resolve `enabled !== false` at the call site (comparison
doesn't count toward complexity).

* Reshape inline review behind @/review module

Drop the `generated: true` attachment flag and consolidate inline-review
logic behind a deep `@/review` module so composer and git-diff-pane stop
hand-composing review primitives. Workspace-scoped review snapshots are
now derived at submit/queue time rather than persisted in the draft
store. Composer review references drop from 51 to 4; git-diff-pane drops
~108 lines and now consumes a controller hook + slot components.

* Fix optimistic review attachments
This commit is contained in:
Mohamed Boudra
2026-04-30 21:35:58 +08:00
committed by GitHub
parent 4bcf98a9e3
commit 92afb826d9
39 changed files with 4244 additions and 317 deletions
+175 -4
View File
@@ -1,8 +1,18 @@
import { describe, expect, it } from "vitest";
import { buildSplitDiffRows, buildUnifiedDiffLines } from "./diff-layout";
import {
buildReviewableDiffTargetKey,
buildSplitDiffRows,
buildUnifiedDiffLines,
} from "./diff-layout";
import type { ParsedDiffFile } from "@/hooks/use-checkout-diff-query";
import type { ReviewableDiffTarget } from "./diff-layout";
function makeFile(lines: ParsedDiffFile["hunks"][number]["lines"]): ParsedDiffFile {
function makeFile(
lines: ParsedDiffFile["hunks"][number]["lines"],
options: { oldStart?: number; newStart?: number } = {},
): ParsedDiffFile {
const oldStart = options.oldStart ?? 10;
const newStart = options.newStart ?? 10;
return {
path: "example.ts",
isNew: false,
@@ -12,9 +22,9 @@ function makeFile(lines: ParsedDiffFile["hunks"][number]["lines"]): ParsedDiffFi
status: "ok",
hunks: [
{
oldStart: 10,
oldStart,
oldCount: 4,
newStart: 10,
newStart,
newCount: 5,
lines,
},
@@ -22,7 +32,39 @@ function makeFile(lines: ParsedDiffFile["hunks"][number]["lines"]): ParsedDiffFi
};
}
function expectReviewTarget(
target: ReviewableDiffTarget | null | undefined,
expected: Omit<ReviewableDiffTarget, "key" | "filePath">,
) {
expect(target).toMatchObject({
filePath: "example.ts",
...expected,
});
expect(target?.key).toBe(
buildReviewableDiffTargetKey({
filePath: "example.ts",
side: expected.side,
lineNumber: expected.lineNumber,
}),
);
}
describe("buildSplitDiffRows", () => {
it("uses one canonical persisted key for rendered review targets", () => {
const rows = buildSplitDiffRows(
makeFile([
{ type: "header", content: "@@ -10,1 +20,1 @@" },
{ type: "context", content: "same line" },
]),
);
if (rows[1]?.kind !== "pair") {
throw new Error("Expected split pair row");
}
expect(rows[1].left?.reviewTarget?.key).toBe("example.ts:old:10");
expect(rows[1].right?.reviewTarget?.key).toBe("example.ts:new:10");
});
it("pairs replacement runs by index", () => {
const rows = buildSplitDiffRows(
makeFile([
@@ -78,6 +120,86 @@ describe("buildSplitDiffRows", () => {
right: { type: "context", content: "same line", lineNumber: 10 },
});
});
it("emits old and new review targets for split context cells", () => {
const rows = buildSplitDiffRows(
makeFile(
[
{ type: "header", content: "@@ -10,1 +20,1 @@" },
{ type: "context", content: "same line" },
],
{ newStart: 20 },
),
);
expect(rows[0]).toEqual({
kind: "header",
content: "@@ -10,1 +20,1 @@",
});
expect(rows[1]).toMatchObject({
kind: "pair",
left: { type: "context", content: "same line", lineNumber: 10 },
right: { type: "context", content: "same line", lineNumber: 20 },
});
if (rows[1]?.kind !== "pair") {
throw new Error("Expected split pair row");
}
expectReviewTarget(rows[1].left?.reviewTarget, {
hunkHeader: "@@ -10,1 +20,1 @@",
hunkIndex: 0,
lineIndex: 1,
oldLineNumber: 10,
newLineNumber: 20,
side: "old",
lineNumber: 10,
lineType: "context",
content: "same line",
});
expectReviewTarget(rows[1].right?.reviewTarget, {
hunkHeader: "@@ -10,1 +20,1 @@",
hunkIndex: 0,
lineIndex: 1,
oldLineNumber: 10,
newLineNumber: 20,
side: "new",
lineNumber: 20,
lineType: "context",
content: "same line",
});
});
it("does not emit review targets for split headers or empty cells", () => {
const rows = buildSplitDiffRows(
makeFile([
{ type: "header", content: "@@ -10,1 +10,2 @@" },
{ type: "add", content: "after" },
]),
);
expect(rows[0]).toEqual({
kind: "header",
content: "@@ -10,1 +10,2 @@",
});
expect(rows[1]).toMatchObject({
kind: "pair",
left: null,
right: { type: "add", content: "after", lineNumber: 10 },
});
if (rows[1]?.kind !== "pair") {
throw new Error("Expected split pair row");
}
expectReviewTarget(rows[1].right?.reviewTarget, {
hunkHeader: "@@ -10,1 +10,2 @@",
hunkIndex: 0,
lineIndex: 1,
oldLineNumber: null,
newLineNumber: 10,
side: "new",
lineNumber: 10,
lineType: "add",
content: "after",
});
});
});
describe("buildUnifiedDiffLines", () => {
@@ -151,4 +273,53 @@ describe("buildUnifiedDiffLines", () => {
expect(lines[5]?.lineNumber).toBe(166);
expect(lines[6]?.lineNumber).toBe(167);
});
it("emits canonical review targets for unified add, remove, and context lines", () => {
const lines = buildUnifiedDiffLines(
makeFile(
[
{ type: "header", content: "@@ -10,2 +20,2 @@" },
{ type: "context", content: "before" },
{ type: "remove", content: "removed" },
{ type: "add", content: "inserted" },
],
{ newStart: 20 },
),
);
expect(lines[0]?.reviewTarget).toBeNull();
expectReviewTarget(lines[1]?.reviewTarget, {
hunkHeader: "@@ -10,2 +20,2 @@",
hunkIndex: 0,
lineIndex: 1,
oldLineNumber: 10,
newLineNumber: 20,
side: "new",
lineNumber: 20,
lineType: "context",
content: "before",
});
expectReviewTarget(lines[2]?.reviewTarget, {
hunkHeader: "@@ -10,2 +20,2 @@",
hunkIndex: 0,
lineIndex: 2,
oldLineNumber: 11,
newLineNumber: null,
side: "old",
lineNumber: 11,
lineType: "remove",
content: "removed",
});
expectReviewTarget(lines[3]?.reviewTarget, {
hunkHeader: "@@ -10,2 +20,2 @@",
hunkIndex: 0,
lineIndex: 3,
oldLineNumber: null,
newLineNumber: 21,
side: "new",
lineNumber: 21,
lineType: "add",
content: "inserted",
});
});
});
+201 -88
View File
@@ -1,16 +1,69 @@
import type { DiffLine, ParsedDiffFile } from "@/hooks/use-checkout-diff-query";
type ReviewSide = "old" | "new";
type ReviewableLineType = "add" | "remove" | "context";
export interface ReviewableDiffTargetKeyInput {
filePath: string;
side: ReviewSide;
lineNumber: number;
}
export interface ReviewableDiffTarget {
key: string;
filePath: string;
hunkHeader: string;
hunkIndex: number;
lineIndex: number;
oldLineNumber: number | null;
newLineNumber: number | null;
side: ReviewSide;
lineNumber: number;
lineType: ReviewableLineType;
content: string;
}
export function buildReviewableDiffTargetKey(input: ReviewableDiffTargetKeyInput): string {
return `${input.filePath}:${input.side}:${input.lineNumber}`;
}
export interface NumberedDiffCell extends ReviewableDiffTarget {
line: DiffLine;
}
export interface NumberedDiffLine {
key: string;
filePath: string;
hunkHeader: string;
hunkIndex: number;
lineIndex: number;
line: DiffLine;
oldLineNumber: number | null;
newLineNumber: number | null;
unifiedCell: NumberedDiffCell | null;
oldCell: NumberedDiffCell | null;
newCell: NumberedDiffCell | null;
}
export interface NumberedDiffHunk {
hunkIndex: number;
hunkHeader: string;
lines: NumberedDiffLine[];
}
export interface SplitDiffDisplayLine {
type: DiffLine["type"];
content: string;
tokens?: DiffLine["tokens"];
lineNumber: number | null;
reviewTarget: ReviewableDiffTarget | null;
}
export interface UnifiedDiffDisplayLine {
key: string;
line: DiffLine;
lineNumber: number | null;
reviewTarget: ReviewableDiffTarget | null;
}
export type SplitDiffRow =
@@ -24,95 +77,175 @@ export type SplitDiffRow =
right: SplitDiffDisplayLine | null;
};
function toDisplayLine(input: {
line: DiffLine;
oldLineNumber: number | null;
newLineNumber: number | null;
side: "left" | "right";
}): SplitDiffDisplayLine | null {
const { line, oldLineNumber, newLineNumber, side } = input;
if (line.type === "header") {
function toSplitDisplayLine(cell: NumberedDiffCell | null): SplitDiffDisplayLine | null {
if (!cell) {
return null;
}
if (line.type === "remove") {
if (side !== "left") {
return null;
}
return {
type: "remove",
content: line.content,
tokens: line.tokens,
lineNumber: oldLineNumber,
};
}
if (line.type === "add") {
if (side !== "right") {
return null;
}
return {
type: "add",
content: line.content,
tokens: line.tokens,
lineNumber: newLineNumber,
};
}
return {
type: "context",
content: line.content,
tokens: line.tokens,
lineNumber: side === "left" ? oldLineNumber : newLineNumber,
type: cell.lineType,
content: cell.content,
...(cell.line.tokens ? { tokens: cell.line.tokens } : {}),
lineNumber: cell.lineNumber,
reviewTarget: toReviewTarget(cell),
};
}
export function buildUnifiedDiffLines(file: ParsedDiffFile): UnifiedDiffDisplayLine[] {
const lines: UnifiedDiffDisplayLine[] = [];
function toReviewTarget(cell: NumberedDiffCell): ReviewableDiffTarget {
return {
key: cell.key,
filePath: cell.filePath,
hunkHeader: cell.hunkHeader,
hunkIndex: cell.hunkIndex,
lineIndex: cell.lineIndex,
oldLineNumber: cell.oldLineNumber,
newLineNumber: cell.newLineNumber,
side: cell.side,
lineNumber: cell.lineNumber,
lineType: cell.lineType,
content: cell.content,
};
}
function getHunkHeader(hunk: ParsedDiffFile["hunks"][number]): string {
const headerLine = hunk.lines.find((line) => line.type === "header");
return headerLine?.content ?? "@@";
}
export function buildNumberedDiffHunks(file: ParsedDiffFile): NumberedDiffHunk[] {
const numberedHunks: NumberedDiffHunk[] = [];
for (const [hunkIndex, hunk] of file.hunks.entries()) {
let oldLineNo = hunk.oldStart;
let newLineNo = hunk.newStart;
const hunkHeader = getHunkHeader(hunk);
const lines: NumberedDiffLine[] = [];
for (const [lineIndex, line] of hunk.lines.entries()) {
let lineNumber: number | null = null;
let oldLineNumber: number | null = null;
let newLineNumber: number | null = null;
if (line.type === "remove") {
lineNumber = oldLineNo;
oldLineNumber = oldLineNo;
oldLineNo += 1;
} else if (line.type === "add") {
lineNumber = newLineNo;
newLineNumber = newLineNo;
newLineNo += 1;
} else if (line.type === "context") {
lineNumber = newLineNo;
oldLineNumber = oldLineNo;
newLineNumber = newLineNo;
oldLineNo += 1;
newLineNo += 1;
}
const oldCell = buildNumberedCell({
filePath: file.path,
hunkHeader,
hunkIndex,
lineIndex,
line,
oldLineNumber,
newLineNumber,
side: "old",
});
const newCell = buildNumberedCell({
filePath: file.path,
hunkHeader,
hunkIndex,
lineIndex,
line,
oldLineNumber,
newLineNumber,
side: "new",
});
lines.push({
key: `${hunkIndex}-${lineIndex}`,
filePath: file.path,
hunkHeader,
hunkIndex,
lineIndex,
line,
lineNumber,
oldLineNumber,
newLineNumber,
unifiedCell: line.type === "remove" ? oldCell : newCell,
oldCell,
newCell,
});
}
numberedHunks.push({ hunkIndex, hunkHeader, lines });
}
return lines;
return numberedHunks;
}
function buildNumberedCell(input: {
filePath: string;
hunkHeader: string;
hunkIndex: number;
lineIndex: number;
line: DiffLine;
oldLineNumber: number | null;
newLineNumber: number | null;
side: ReviewSide;
}): NumberedDiffCell | null {
if (input.line.type === "header") {
return null;
}
if (input.line.type === "remove" && input.side !== "old") {
return null;
}
if (input.line.type === "add" && input.side !== "new") {
return null;
}
const lineNumber = input.side === "old" ? input.oldLineNumber : input.newLineNumber;
if (lineNumber === null) {
return null;
}
return {
key: buildReviewableDiffTargetKey({
filePath: input.filePath,
side: input.side,
lineNumber,
}),
filePath: input.filePath,
hunkHeader: input.hunkHeader,
hunkIndex: input.hunkIndex,
lineIndex: input.lineIndex,
oldLineNumber: input.oldLineNumber,
newLineNumber: input.newLineNumber,
side: input.side,
lineNumber,
lineType: input.line.type,
content: input.line.content,
line: input.line,
};
}
export function buildUnifiedDiffLines(file: ParsedDiffFile): UnifiedDiffDisplayLine[] {
return buildNumberedDiffHunks(file).flatMap((hunk) =>
hunk.lines.map((numberedLine) => ({
key: numberedLine.key,
line: numberedLine.line,
lineNumber: numberedLine.unifiedCell?.lineNumber ?? null,
reviewTarget: numberedLine.unifiedCell ? toReviewTarget(numberedLine.unifiedCell) : null,
})),
);
}
export function buildSplitDiffRows(file: ParsedDiffFile): SplitDiffRow[] {
const rows: SplitDiffRow[] = [];
for (const hunk of file.hunks) {
let oldLineNo = hunk.oldStart;
let newLineNo = hunk.newStart;
for (const hunk of buildNumberedDiffHunks(file)) {
rows.push({
kind: "header",
content: hunk.lines[0]?.type === "header" ? hunk.lines[0].content : "@@",
content: hunk.hunkHeader,
});
let pendingRemovals: Array<{ line: DiffLine; oldLineNumber: number }> = [];
let pendingAdditions: Array<{ line: DiffLine; newLineNumber: number }> = [];
let pendingRemovals: NumberedDiffCell[] = [];
let pendingAdditions: NumberedDiffCell[] = [];
const flushPendingRows = () => {
const pairCount = Math.max(pendingRemovals.length, pendingAdditions.length);
@@ -121,61 +254,41 @@ export function buildSplitDiffRows(file: ParsedDiffFile): SplitDiffRow[] {
const addition = pendingAdditions[index] ?? null;
rows.push({
kind: "pair",
left: removal
? toDisplayLine({
line: removal.line,
oldLineNumber: removal.oldLineNumber,
newLineNumber: null,
side: "left",
})
: null,
right: addition
? toDisplayLine({
line: addition.line,
oldLineNumber: null,
newLineNumber: addition.newLineNumber,
side: "right",
})
: null,
left: toSplitDisplayLine(removal),
right: toSplitDisplayLine(addition),
});
}
pendingRemovals = [];
pendingAdditions = [];
};
for (const line of hunk.lines.slice(1)) {
if (line.type === "remove") {
pendingRemovals.push({ line, oldLineNumber: oldLineNo });
oldLineNo += 1;
for (const numberedLine of hunk.lines) {
if (numberedLine.line.type === "header") {
continue;
}
if (line.type === "add") {
pendingAdditions.push({ line, newLineNumber: newLineNo });
newLineNo += 1;
if (numberedLine.line.type === "remove") {
if (numberedLine.oldCell) {
pendingRemovals.push(numberedLine.oldCell);
}
continue;
}
if (numberedLine.line.type === "add") {
if (numberedLine.newCell) {
pendingAdditions.push(numberedLine.newCell);
}
continue;
}
flushPendingRows();
if (line.type === "context") {
if (numberedLine.line.type === "context") {
rows.push({
kind: "pair",
left: toDisplayLine({
line,
oldLineNumber: oldLineNo,
newLineNumber: newLineNo,
side: "left",
}),
right: toDisplayLine({
line,
oldLineNumber: oldLineNo,
newLineNumber: newLineNo,
side: "right",
}),
left: toSplitDisplayLine(numberedLine.oldCell),
right: toSplitDisplayLine(numberedLine.newCell),
});
oldLineNo += 1;
newLineNo += 1;
}
}
@@ -1,6 +1,7 @@
import { describe, expect, it } from "vitest";
import type { StreamItem } from "@/types/stream";
import { mergePendingCreateImages } from "./pending-create-images";
import type { AgentAttachment } from "@server/shared/messages";
function userMessage(params: {
id: string;
@@ -12,6 +13,7 @@ function userMessage(params: {
mimeType: string;
createdAt: number;
}>;
attachments?: AgentAttachment[];
}): StreamItem {
return {
kind: "user_message",
@@ -19,6 +21,7 @@ function userMessage(params: {
text: params.text,
timestamp: new Date("2026-01-01T00:00:00Z"),
...(params.images ? { images: params.images } : {}),
...(params.attachments ? { attachments: params.attachments } : {}),
};
}
@@ -34,6 +37,16 @@ function buildImage(id: string) {
];
}
function buildReviewAttachment(): AgentAttachment {
return {
type: "review",
mimeType: "application/paseo-review",
cwd: "/repo",
mode: "base",
comments: [],
};
}
describe("mergePendingCreateImages", () => {
it("returns same reference when pending images are absent", () => {
const streamItems = [userMessage({ id: "msg-1", text: "hello" })];
@@ -63,6 +76,24 @@ describe("mergePendingCreateImages", () => {
expect(updated.images).toEqual(images);
});
it("merges attachments by clientMessageId when matched message has none", () => {
const streamItems = [userMessage({ id: "msg-1", text: "hello" })];
const attachments = [buildReviewAttachment()];
const result = mergePendingCreateImages({
streamItems,
clientMessageId: "msg-1",
attachments,
});
expect(result).not.toBe(streamItems);
const updated = result[0];
expect(updated?.kind).toBe("user_message");
if (updated?.kind !== "user_message") {
throw new Error("Expected user_message item");
}
expect(updated.attachments).toEqual(attachments);
});
it("does not merge when clientMessageId does not match", () => {
const streamItems = [userMessage({ id: "msg-1", text: "same text" })];
const images = buildImage("image-2");
@@ -92,4 +123,32 @@ describe("mergePendingCreateImages", () => {
}
expect(unchanged.images).toEqual(existingImages);
});
it("does not overwrite existing user message attachments", () => {
const existingAttachments = [buildReviewAttachment()];
const streamItems = [
userMessage({ id: "msg-1", text: "hello", attachments: existingAttachments }),
];
const result = mergePendingCreateImages({
streamItems,
clientMessageId: "msg-1",
attachments: [
{
type: "github_issue",
mimeType: "application/github-issue",
number: 7,
title: "Issue",
url: "https://github.com/getpaseo/paseo/issues/7",
},
],
});
expect(result).toBe(streamItems);
const unchanged = result[0];
expect(unchanged?.kind).toBe("user_message");
if (unchanged?.kind !== "user_message") {
throw new Error("Expected user_message item");
}
expect(unchanged.attachments).toEqual(existingAttachments);
});
});
@@ -1,17 +1,22 @@
import type { StreamItem, UserMessageImageAttachment } from "@/types/stream";
import type { AgentAttachment } from "@server/shared/messages";
interface MergePendingCreateImagesParams {
streamItems: StreamItem[];
clientMessageId: string;
images?: UserMessageImageAttachment[];
attachments?: AgentAttachment[];
}
export function mergePendingCreateImages({
streamItems,
clientMessageId,
images,
attachments,
}: MergePendingCreateImagesParams): StreamItem[] {
if (!images || images.length === 0) {
const hasPendingImages = Boolean(images && images.length > 0);
const hasPendingAttachments = Boolean(attachments && attachments.length > 0);
if (!hasPendingImages && !hasPendingAttachments) {
return streamItems;
}
@@ -26,11 +31,18 @@ export function mergePendingCreateImages({
if (target.kind !== "user_message") {
return streamItems;
}
if (target.images && target.images.length > 0) {
const shouldMergeImages = hasPendingImages && (!target.images || target.images.length === 0);
const shouldMergeAttachments =
hasPendingAttachments && (!target.attachments || target.attachments.length === 0);
if (!shouldMergeImages && !shouldMergeAttachments) {
return streamItems;
}
const next = [...streamItems];
next[targetIndex] = { ...target, images };
next[targetIndex] = {
...target,
...(shouldMergeImages ? { images } : {}),
...(shouldMergeAttachments ? { attachments } : {}),
};
return next;
}