From ffda463c6c9f26d775334964ff38d129e1fe2906 Mon Sep 17 00:00:00 2001 From: Ed Bordin Date: Thu, 14 May 2026 06:44:02 +1000 Subject: [PATCH 1/3] feat: add content-anchored resolvable comments --- apps/cli/app/api/comments/route.test.ts | 134 ++++++- apps/cli/app/api/comments/route.ts | 59 ++- apps/cli/components/DiffApp.test.tsx | 38 +- apps/cli/components/DiffApp.tsx | 70 +++- apps/cli/components/DiffViewer.test.tsx | 11 +- apps/cli/components/DiffViewer.tsx | 318 ++++++++++++--- apps/cli/components/DiffsWorkerProvider.tsx | 5 +- apps/cli/components/StatusBar.tsx | 2 +- apps/cli/components/theme-provider.tsx | 101 ++++- apps/cli/lib/comment-types.ts | 24 ++ apps/cli/lib/comments.test.ts | 245 +++++++++++- apps/cli/lib/comments.ts | 416 +++++++++++++++++++- apps/cli/test/comments-anchor.test.ts | 134 +++++++ apps/cli/test/comments-resolve.test.ts | 116 ++++++ 14 files changed, 1571 insertions(+), 102 deletions(-) create mode 100644 apps/cli/test/comments-anchor.test.ts create mode 100644 apps/cli/test/comments-resolve.test.ts diff --git a/apps/cli/app/api/comments/route.test.ts b/apps/cli/app/api/comments/route.test.ts index 2a393fa..e307164 100644 --- a/apps/cli/app/api/comments/route.test.ts +++ b/apps/cli/app/api/comments/route.test.ts @@ -2,7 +2,7 @@ import { mkdirSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import { DELETE, GET, POST } from "./route"; +import { DELETE, GET, PATCH, POST } from "./route"; const tempPaths: string[] = []; @@ -19,6 +19,7 @@ describe("/api/comments", () => { }); afterEach(() => { + delete process.env.DIFFHUB_USER; delete process.env.DIFFHUB_REPO; for (const tempPath of tempPaths.splice(0)) { @@ -88,6 +89,29 @@ describe("/api/comments", () => { }); }); + it("rejects non-positive and non-integer line numbers", async () => { + for (const lineNumber of [0, -1, 1.5]) { + const response = await POST( + new Request("http://localhost/api/comments", { + body: JSON.stringify({ + body: "Review this hunk", + file: "src/a.ts", + lineNumber, + side: "right", + tag: "", + }), + headers: { "Content-Type": "application/json" }, + method: "POST", + }), + ); + + expect(response.status).toBe(400); + await expect(response.json()).resolves.toStrictEqual({ + error: "lineNumber must be a positive integer", + }); + } + }); + it("requires an id to delete a comment", async () => { const response = await DELETE( new Request("http://localhost/api/comments", { method: "DELETE" }), @@ -125,4 +149,112 @@ describe("/api/comments", () => { await expect(response.json()).resolves.toStrictEqual({ ok: true }); await expect(GET().json()).resolves.toStrictEqual([]); }); + + it("resolves, replies, and unresolves comments through PATCH", async () => { + process.env.DIFFHUB_USER = "Route Tester"; + const createResponse = await POST( + new Request("http://localhost/api/comments", { + body: JSON.stringify({ + body: "Review this hunk", + file: "src/a.ts", + lineNumber: 12, + side: "right", + tag: "", + }), + headers: { "Content-Type": "application/json" }, + method: "POST", + }), + ); + const created = (await createResponse.json()) as { id: string }; + + const resolveResponse = await PATCH( + new Request(`http://localhost/api/comments?id=${created.id}`, { + body: JSON.stringify({ action: "resolve" }), + headers: { "Content-Type": "application/json" }, + method: "PATCH", + }), + ); + expect(resolveResponse.status).toBe(200); + await expect(resolveResponse.json()).resolves.toMatchObject({ + resolved: true, + resolvedAt: expect.any(String), + resolvedBy: "Route Tester", + }); + + const replyResponse = await PATCH( + new Request(`http://localhost/api/comments?id=${created.id}`, { + body: JSON.stringify({ action: "reply", body: "Fixed now" }), + headers: { "Content-Type": "application/json" }, + method: "PATCH", + }), + ); + expect(replyResponse.status).toBe(200); + await expect(replyResponse.json()).resolves.toMatchObject({ + replies: [{ at: expect.any(String), body: "Fixed now", by: "Route Tester" }], + resolved: true, + }); + + const unresolveResponse = await PATCH( + new Request(`http://localhost/api/comments?id=${created.id}`, { + body: JSON.stringify({ action: "unresolve" }), + headers: { "Content-Type": "application/json" }, + method: "PATCH", + }), + ); + expect(unresolveResponse.status).toBe(200); + const unresolved = (await unresolveResponse.json()) as { + resolved: boolean; + resolvedAt?: string; + resolvedBy?: string; + }; + expect(unresolved.resolved).toBeFalsy(); + expect(unresolved.resolvedAt).toBeUndefined(); + expect(unresolved.resolvedBy).toBeUndefined(); + }); + + it("validates PATCH requests", async () => { + const missingIdResponse = await PATCH( + new Request("http://localhost/api/comments", { + body: JSON.stringify({ action: "resolve" }), + headers: { "Content-Type": "application/json" }, + method: "PATCH", + }), + ); + expect(missingIdResponse.status).toBe(400); + await expect(missingIdResponse.json()).resolves.toStrictEqual({ error: "id required" }); + + const unknownCommentResponse = await PATCH( + new Request("http://localhost/api/comments?id=missing-id", { + body: JSON.stringify({ action: "resolve" }), + headers: { "Content-Type": "application/json" }, + method: "PATCH", + }), + ); + expect(unknownCommentResponse.status).toBe(404); + await expect(unknownCommentResponse.json()).resolves.toStrictEqual({ + error: "comment not found", + }); + + const invalidActionResponse = await PATCH( + new Request("http://localhost/api/comments?id=missing-id", { + body: JSON.stringify({ action: "invalid" }), + headers: { "Content-Type": "application/json" }, + method: "PATCH", + }), + ); + expect(invalidActionResponse.status).toBe(400); + await expect(invalidActionResponse.json()).resolves.toStrictEqual({ + error: "invalid action", + }); + + const emptyReplyResponse = await PATCH( + new Request("http://localhost/api/comments?id=missing-id", { + body: JSON.stringify({ action: "reply", body: " " }), + headers: { "Content-Type": "application/json" }, + method: "PATCH", + }), + ); + expect(emptyReplyResponse.status).toBe(400); + await expect(emptyReplyResponse.json()).resolves.toStrictEqual({ error: "body is required" }); + }); }); diff --git a/apps/cli/app/api/comments/route.ts b/apps/cli/app/api/comments/route.ts index 2175e48..4cad408 100644 --- a/apps/cli/app/api/comments/route.ts +++ b/apps/cli/app/api/comments/route.ts @@ -1,6 +1,13 @@ import { NextResponse } from "next/server"; import { parseCommentSide } from "@/lib/comment-sides"; -import { addComment, clearComments, deleteComment, readComments } from "@/lib/comments"; +import { + addComment, + addReply, + clearComments, + deleteComment, + readComments, + setCommentResolved, +} from "@/lib/comments"; import type { CommentTag } from "@/lib/comments"; const VALID_TAGS = new Set(["[must-fix]", "[suggestion]", "[nit]", "[question]", ""]); @@ -22,8 +29,13 @@ export const POST = async (request: Request) => { if (typeof data.file !== "string" || !data.file) { return NextResponse.json({ error: "file is required" }, { status: 400 }); } - if (typeof data.lineNumber !== "number" || !Number.isFinite(data.lineNumber)) { - return NextResponse.json({ error: "lineNumber must be a number" }, { status: 400 }); + if ( + typeof data.lineNumber !== "number" || + !Number.isFinite(data.lineNumber) || + !Number.isInteger(data.lineNumber) || + data.lineNumber < 1 + ) { + return NextResponse.json({ error: "lineNumber must be a positive integer" }, { status: 400 }); } if (typeof data.body !== "string" || !data.body) { return NextResponse.json({ error: "body is required" }, { status: 400 }); @@ -45,6 +57,8 @@ export const POST = async (request: Request) => { try { const comment = await addComment({ body: data.body, + createdBy: typeof data.createdBy === "string" ? data.createdBy : undefined, + diffHunkHeader: typeof data.diffHunkHeader === "string" ? data.diffHunkHeader : undefined, file: data.file, lineNumber: data.lineNumber, side, @@ -59,6 +73,45 @@ export const POST = async (request: Request) => { } }; +export const PATCH = async (request: Request) => { + const { searchParams } = new URL(request.url); + const id = searchParams.get("id"); + if (!id) { + return NextResponse.json({ error: "id required" }, { status: 400 }); + } + + const data = (await request.json()) as Record; + const { action } = data; + + try { + if (action === "resolve" || action === "unresolve") { + const comment = await setCommentResolved(id, action === "resolve"); + if (!comment) { + return NextResponse.json({ error: "comment not found" }, { status: 404 }); + } + return NextResponse.json(comment); + } + + if (action === "reply") { + if (typeof data.body !== "string" || !data.body.trim()) { + return NextResponse.json({ error: "body is required" }, { status: 400 }); + } + const comment = await addReply(id, data.body.trim()); + if (!comment) { + return NextResponse.json({ error: "comment not found" }, { status: 404 }); + } + return NextResponse.json(comment); + } + + return NextResponse.json({ error: "invalid action" }, { status: 400 }); + } catch (error) { + return NextResponse.json( + { error: error instanceof Error ? error.message : "Internal server error" }, + { status: 500 }, + ); + } +}; + export const DELETE = async (request: Request) => { const { searchParams } = new URL(request.url); const clearAll = searchParams.get("all") === "1"; diff --git a/apps/cli/components/DiffApp.test.tsx b/apps/cli/components/DiffApp.test.tsx index 2a8760a..1382ef0 100644 --- a/apps/cli/components/DiffApp.test.tsx +++ b/apps/cli/components/DiffApp.test.tsx @@ -5,6 +5,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { DiffApp } from "./DiffApp"; import type { Comment } from "@/lib/comment-types"; import { WATCH_STREAM_EVENTS } from "@/lib/watch-stream"; +import type * as ThemeProviderModule from "./theme-provider"; interface MockAnnotation { lineNumber: number; @@ -45,8 +46,8 @@ const { MockDynamicPatch } = vi.hoisted(() => ({ ), })); -vi.mock(import("next-themes"), async (importOriginal) => { - const actual = await importOriginal(); +vi.mock(import("./theme-provider"), async (importOriginal) => { + const actual = await importOriginal(); return { ...actual, useTheme: () => ({ @@ -101,6 +102,21 @@ const filesPayload = { insertions: 2, }; +const withCommentDefaults = ( + comment: Omit, +): Comment => ({ + ...comment, + anchor: { + afterContext: [], + beforeContext: [], + fileSha: "", + lineContent: "", + }, + replies: [], + resolved: false, + staleness: "fresh", +}); + const diffPayload = { baseBranch: "origin/main", branch: "feature/diff-review", @@ -205,12 +221,15 @@ describe("DiffApp review flow", () => { } if (url === "/api/comments" && method === "POST") { - const body = JSON.parse(String(init?.body ?? "{}")) as Omit; - const createdComment: Comment = { + const body = JSON.parse(String(init?.body ?? "{}")) as Omit< + Comment, + "anchor" | "createdAt" | "id" | "replies" | "resolved" | "staleness" + >; + const createdComment = withCommentDefaults({ ...body, createdAt: "2026-04-15T00:00:00.000Z", id: `comment-${comments.length + 1}`, - }; + }); comments = [...comments, createdComment]; return Promise.resolve(jsonResponse(createdComment)); } @@ -658,12 +677,15 @@ describe("DiffApp review flow", () => { } if (url === "/api/comments" && method === "POST") { - const body = JSON.parse(String(init?.body ?? "{}")) as Omit; - const createdComment: Comment = { + const body = JSON.parse(String(init?.body ?? "{}")) as Omit< + Comment, + "anchor" | "createdAt" | "id" | "replies" | "resolved" | "staleness" + >; + const createdComment = withCommentDefaults({ ...body, createdAt: "2026-04-15T00:00:00.000Z", id: "comment-1", - }; + }); comments = [createdComment]; return Promise.resolve(jsonResponse(createdComment)); } diff --git a/apps/cli/components/DiffApp.tsx b/apps/cli/components/DiffApp.tsx index 468892a..2fc7621 100644 --- a/apps/cli/components/DiffApp.tsx +++ b/apps/cli/components/DiffApp.tsx @@ -1,12 +1,12 @@ "use client"; import type { AnnotationSide } from "@pierre/diffs"; -import { useTheme } from "next-themes"; import { useCallback, useEffect, useRef, useState, useTransition, startTransition } from "react"; import { StatusBar } from "./StatusBar"; import type { DiffMode, WatchStatus } from "./StatusBar"; import { FileList } from "./FileList"; import { DiffViewer, getDiffSectionId } from "./DiffViewer"; +import { useTheme } from "./theme-provider"; import { SidebarInset, SidebarProvider } from "@/components/ui/sidebar"; import { Button } from "@/components/ui/button"; import { toCommentSide } from "@/lib/comment-sides"; @@ -88,6 +88,8 @@ interface MainPanelProps { tag: CommentTag, ) => Promise; onDeleteComment: (id: string) => Promise; + onResolveComment: (id: string, resolved: boolean) => Promise; + onReplyToComment: (id: string, body: string) => Promise; selectedFile: string | null; collapsedFiles: Set; forceRenderFiles: ReadonlySet; @@ -177,8 +179,14 @@ const areCommentsEqual = (previous: readonly Comment[], next: readonly Comment[] comment.file === nextComment.file && comment.id === nextComment.id && comment.lineNumber === nextComment.lineNumber && + comment.rebasedFromLine === nextComment.rebasedFromLine && + comment.resolved === nextComment.resolved && + comment.resolvedAt === nextComment.resolvedAt && + comment.resolvedBy === nextComment.resolvedBy && comment.side === nextComment.side && - comment.tag === nextComment.tag + comment.staleness === nextComment.staleness && + comment.tag === nextComment.tag && + JSON.stringify(comment.replies) === JSON.stringify(nextComment.replies) ); }); }; @@ -282,6 +290,8 @@ const MainPanel = ({ comments, onAddComment, onDeleteComment, + onResolveComment, + onReplyToComment, selectedFile, collapsedFiles, forceRenderFiles, @@ -345,6 +355,8 @@ const MainPanel = ({ comments={comments} onAddComment={onAddComment} onDeleteComment={onDeleteComment} + onResolveComment={onResolveComment} + onReplyToComment={onReplyToComment} activeFileId={selectedFile} fileStats={filesData.files} collapsedFiles={collapsedFiles} @@ -1165,6 +1177,58 @@ export const DiffApp = ({ return true; }, []); + const replaceComment = useCallback((nextComment: Comment) => { + setComments((previous) => + previous.map((comment) => (comment.id === nextComment.id ? nextComment : comment)), + ); + }, []); + + const handleResolveComment = useCallback( + async (id: string, resolved: boolean) => { + const response = await fetch(`/api/comments?id=${id}`, { + body: JSON.stringify({ action: resolved ? "resolve" : "unresolve" }), + headers: { "Content-Type": "application/json" }, + method: "PATCH", + }); + if (!response.ok) { + const errorBody = (await response + .json() + .catch(() => ({ error: "Failed to update comment" }))) as DiffErrorResponse; + console.error("[diffhub] resolve comment failed", { + error: errorBody.error ?? `Failed to update comment (${response.status})`, + }); + return false; + } + + replaceComment((await response.json()) as Comment); + return true; + }, + [replaceComment], + ); + + const handleReplyToComment = useCallback( + async (id: string, body: string) => { + const response = await fetch(`/api/comments?id=${id}`, { + body: JSON.stringify({ action: "reply", body }), + headers: { "Content-Type": "application/json" }, + method: "PATCH", + }); + if (!response.ok) { + const errorBody = (await response + .json() + .catch(() => ({ error: "Failed to save reply" }))) as DiffErrorResponse; + console.error("[diffhub] reply failed", { + error: errorBody.error ?? `Failed to save reply (${response.status})`, + }); + return false; + } + + replaceComment((await response.json()) as Comment); + return true; + }, + [replaceComment], + ); + const syncNotice = getSyncNotice(loadError, filesData, deferredDiffData, diffError); if (loadError && filesData === null) { @@ -1230,6 +1294,8 @@ export const DiffApp = ({ comments={comments} onAddComment={handleAddComment} onDeleteComment={handleDeleteComment} + onResolveComment={handleResolveComment} + onReplyToComment={handleReplyToComment} selectedFile={selectedFile} collapsedFiles={collapsedFiles} forceRenderFiles={forceRenderFiles} diff --git a/apps/cli/components/DiffViewer.test.tsx b/apps/cli/components/DiffViewer.test.tsx index 13d722a..1d71eb8 100644 --- a/apps/cli/components/DiffViewer.test.tsx +++ b/apps/cli/components/DiffViewer.test.tsx @@ -2,13 +2,14 @@ import React from "react"; import { cleanup, fireEvent, render, screen } from "@testing-library/react"; import { afterEach, describe, expect, it, vi } from "vitest"; import { DiffViewer } from "./DiffViewer"; +import type * as ThemeProviderModule from "./theme-provider"; const { MockPatchDiff } = vi.hoisted(() => ({ MockPatchDiff: ({ patch }: { patch: string }) =>
{patch}
, })); -vi.mock(import("next-themes"), async (importOriginal) => { - const actual = await importOriginal(); +vi.mock(import("./theme-provider"), async (importOriginal) => { + const actual = await importOriginal(); return { ...actual, useTheme: () => ({ @@ -66,6 +67,12 @@ const makeProps = (fileCount: number) => { >() .mockResolvedValue(true), onDeleteComment: vi.fn<(id: string) => Promise>().mockResolvedValue(true), + onReplyToComment: vi + .fn<(id: string, body: string) => Promise>() + .mockResolvedValue(true), + onResolveComment: vi + .fn<(id: string, resolved: boolean) => Promise>() + .mockResolvedValue(true), onToggleCollapse: vi.fn<(file: string) => void>(), patchesByFile: Object.fromEntries( files.map((file, index) => [ diff --git a/apps/cli/components/DiffViewer.tsx b/apps/cli/components/DiffViewer.tsx index 153a543..48c1a80 100644 --- a/apps/cli/components/DiffViewer.tsx +++ b/apps/cli/components/DiffViewer.tsx @@ -3,7 +3,6 @@ import dynamic from "next/dynamic"; import { Component, memo, useCallback, useEffect, useMemo, useRef, useState } from "react"; import type { CSSProperties, ErrorInfo, ReactNode } from "react"; -import { useTheme } from "next-themes"; import type { DiffLineAnnotation, AnnotationSide } from "@pierre/diffs"; import { toAnnotationSide } from "@/lib/comment-sides"; import type { Comment, CommentTag } from "@/lib/comment-types"; @@ -13,6 +12,7 @@ import type { PrerenderedDiffHtml } from "@/lib/diff-prerender"; import type { DiffTheme } from "@/lib/diff-colors"; import { getDiffUnsafeCSS } from "@/lib/diff-colors"; import { FileDiffHeader } from "./FileDiffHeader"; +import { useTheme } from "./theme-provider"; import { cn } from "@/lib/utils"; import { BranchIcon, CopySimpleIcon, TrashIcon, CheckIcon } from "blode-icons-react"; import { Button } from "@/components/ui/button"; @@ -91,7 +91,7 @@ const formatRelativeTime = (iso: string): string => { return `${diffDay}d ago`; }; -type AnnotationData = { type: "comment"; comment: Comment } | { type: "input"; file: string }; +type AnnotationData = { type: "comment"; commentId: string } | { type: "input"; file: string }; const DiffSkeleton = () => (
@@ -278,16 +278,25 @@ const InlineCommentInput = ({ onSubmit, onCancel }: InlineCommentInputProps) => ); }; +// oxlint-disable-next-line complexity const CommentDisplay = ({ comment, onDelete, + onResolve, + onReply, }: { comment: Comment; onDelete: () => Promise; + onResolve: (resolved: boolean) => Promise; + onReply: (body: string) => Promise; }) => { const [copied, setCopied] = useState(false); const [deleteError, setDeleteError] = useState(null); const [isDeleting, setIsDeleting] = useState(false); + const [isUpdating, setIsUpdating] = useState(false); + const [isExpanded, setIsExpanded] = useState(!comment.resolved); + const [replyBody, setReplyBody] = useState(""); + const [replyError, setReplyError] = useState(null); const copiedTimerRef = useRef | null>(null); useEffect( @@ -334,9 +343,67 @@ const CommentDisplay = ({ setIsDeleting(false); }, [isDeleting, onDelete]); + const handleResolve = useCallback( + async (resolved: boolean): Promise => { + if (isUpdating) { + return; + } + + setIsUpdating(true); + const updated = await onResolve(resolved).catch(() => false); + if (updated) { + setIsExpanded(!resolved); + } else { + setDeleteError("Failed to update comment."); + } + setIsUpdating(false); + }, + [isUpdating, onResolve], + ); + + const handleExpandToggle = useCallback(() => { + setIsExpanded((value) => !value); + }, []); + + const handleResolveClick = useCallback(() => { + void handleResolve(true); + }, [handleResolve]); + + const handleUnresolveClick = useCallback(() => { + void handleResolve(false); + }, [handleResolve]); + + const handleReplyChange = useCallback((event: React.ChangeEvent) => { + setReplyBody(event.target.value); + }, []); + + const handleReply = useCallback(async (): Promise => { + const trimmed = replyBody.trim(); + if (!trimmed || isUpdating) { + return; + } + + setReplyError(null); + setIsUpdating(true); + const saved = await onReply(trimmed).catch(() => false); + if (saved) { + setReplyBody(""); + setIsExpanded(true); + } else { + setReplyError("Failed to save reply."); + } + setIsUpdating(false); + }, [isUpdating, onReply, replyBody]); + + const handleReplyClick = useCallback(() => { + void handleReply(); + }, [handleReply]); + const borderAccent = comment.tag ? (TAG_META[comment.tag]?.border ?? "border-l-ring/40") : "border-l-ring/40"; + const firstLine = comment.body.split(/\s+/).join(" ").slice(0, 60); + const canResolve = comment.staleness !== "stale"; return (
- {/* Body row */} -
- {comment.tag && ( - - {comment.tag} + {comment.staleness === "stale" && ( +
+ Stale — content changed +
+ )} + {comment.staleness === "moved" && comment.rebasedFromLine !== undefined && ( +
+ ↳ moved from line {comment.rebasedFromLine} +
+ )} + {comment.resolved && ( + + )} + {/* Body row */} + {isExpanded && ( +
+ {comment.tag && ( + + {comment.tag} + + )} +

{comment.body}

+ {/* Action buttons — hover-revealed */} + +
+ {canResolve && !comment.resolved && ( + + )} + {comment.resolved && ( + + )} + + + } + > + {copied ? : } + + {copied ? "Copied!" : "Copy comment"} + + + + } + > + + + Delete comment + +
+
+
+ )} + {isExpanded && comment.replies.length > 0 && ( +
+
+ {comment.replies.map((reply) => ( +
+
+ {reply.by ?? "unknown"} · {formatRelativeTime(reply.at)} +
+

{reply.body}

+
+ ))}
- -
+
+ )} + {isExpanded && ( +
+