From fe2082543ca4a351885c6f69da5c8c48aec779c5 Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Thu, 27 Aug 2026 20:38:23 -0700 Subject: [PATCH] Redesign project review proposals Signed-off-by: Taylor Ho --- .../features/messages/lib/parseDiff.test.mjs | 32 ++ .../src/features/messages/lib/parseDiff.ts | 43 ++ .../src/features/messages/ui/DiffViewer.tsx | 16 +- .../projects/lib/projectReviewChecks.d.mts | 14 + .../projects/lib/projectReviewChecks.mjs | 76 ++- .../projects/lib/projectReviewChecks.test.mjs | 135 ++++- .../ui/ProjectReviewCheckResultCard.tsx | 472 +++++++++++++----- .../projects/ui/ProjectReviewChecks.tsx | 269 ++++++++-- desktop/tests/e2e/project-pr-review.spec.ts | 205 +++++++- 9 files changed, 1049 insertions(+), 213 deletions(-) create mode 100644 desktop/src/features/messages/lib/parseDiff.test.mjs diff --git a/desktop/src/features/messages/lib/parseDiff.test.mjs b/desktop/src/features/messages/lib/parseDiff.test.mjs new file mode 100644 index 0000000000..42e79eae04 --- /dev/null +++ b/desktop/src/features/messages/lib/parseDiff.test.mjs @@ -0,0 +1,32 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { getShortestUniquePathLabels } from "./parseDiff.ts"; + +test("uses filenames when diff paths have distinct basenames", () => { + assert.deepEqual( + getShortestUniquePathLabels([ + "desktop/src/features/channels/ui/ChannelPane.tsx", + "desktop/src/features/messages/ui/MessageTimeline.tsx", + ]), + ["ChannelPane.tsx", "MessageTimeline.tsx"], + ); +}); + +test("adds only enough parent directories to disambiguate duplicate names", () => { + assert.deepEqual( + getShortestUniquePathLabels([ + "desktop/src/features/channels/ui/index.ts", + "desktop/src/features/messages/ui/index.ts", + "desktop/src/features/messages/lib/parseDiff.ts", + ]), + ["channels/ui/index.ts", "messages/ui/index.ts", "parseDiff.ts"], + ); +}); + +test("expands the longer path when one path is another path's suffix", () => { + assert.deepEqual( + getShortestUniquePathLabels(["src/shared/index.ts", "shared/index.ts"]), + ["src/shared/index.ts", "shared/index.ts"], + ); +}); diff --git a/desktop/src/features/messages/lib/parseDiff.ts b/desktop/src/features/messages/lib/parseDiff.ts index c797dc8904..c7cb9b604c 100644 --- a/desktop/src/features/messages/lib/parseDiff.ts +++ b/desktop/src/features/messages/lib/parseDiff.ts @@ -55,6 +55,49 @@ export function getDiffFileLabel( return newPath || oldPath || fallbackFilePath || "diff"; } +/** Return the resulting file path, preferring the new side of a rename. */ +export function getDiffFilePath( + file: FileData, + fallbackFilePath?: string, +): string { + const oldPath = file.oldPath === "/dev/null" ? undefined : file.oldPath; + const newPath = file.newPath === "/dev/null" ? undefined : file.newPath; + return newPath || oldPath || fallbackFilePath || "diff"; +} + +/** Reduce paths to the shortest unique suffix, usually just the filename. */ +export function getShortestUniquePathLabels(paths: string[]): string[] { + const pathParts = paths.map((path) => { + const parts = path.split(/[\\/]/).filter(Boolean); + return parts.length > 0 ? parts : [path]; + }); + const depths = pathParts.map(() => 1); + + while (true) { + const labels = pathParts.map((parts, index) => + parts.slice(-depths[index]).join("/"), + ); + const collisions = new Set(); + const counts = new Map(); + for (const label of labels) { + counts.set(label, (counts.get(label) ?? 0) + 1); + } + for (const [label, count] of counts) { + if (count > 1) collisions.add(label); + } + if (collisions.size === 0) return labels; + + let expanded = false; + for (const [index, label] of labels.entries()) { + if (collisions.has(label) && depths[index] < pathParts[index].length) { + depths[index] += 1; + expanded = true; + } + } + if (!expanded) return labels; + } +} + export function shouldShowDiffFileHeader( label: string, fileCount: number, diff --git a/desktop/src/features/messages/ui/DiffViewer.tsx b/desktop/src/features/messages/ui/DiffViewer.tsx index ed6703f66c..cd99a3c0f7 100644 --- a/desktop/src/features/messages/ui/DiffViewer.tsx +++ b/desktop/src/features/messages/ui/DiffViewer.tsx @@ -21,6 +21,8 @@ type DiffViewerProps = { viewType?: ViewType; className?: string; searchQuery?: string; + fileIndex?: number; + hideFileHeader?: boolean; }; function FileChangeBadge({ @@ -50,11 +52,15 @@ export function DiffViewer({ viewType = "unified", className, searchQuery, + fileIndex, + hideFileHeader = false, }: DiffViewerProps) { const { files, parseError } = useMemo( () => parseUnifiedDiff(content), [content], ); + const visibleFiles = + fileIndex === undefined ? files : files.slice(fileIndex, fileIndex + 1); const searchPreview = searchQuery ? buildSearchResultPreview(content, searchQuery, 160) : null; @@ -89,15 +95,13 @@ export function DiffViewer({ ) : null}
- {files.map((file) => { + {visibleFiles.map((file) => { const label = getDiffFileLabel(file, fallbackFilePath); const { additions, deletions } = countDiffFileChanges(file); const diffType = normalizeDiffType(file.type); - const showFileHeader = shouldShowDiffFileHeader( - label, - files.length, - fallbackFilePath, - ); + const showFileHeader = + !hideFileHeader && + shouldShowDiffFileHeader(label, files.length, fallbackFilePath); const fileKey = [ file.oldPath || "", file.newPath || "", diff --git a/desktop/src/features/projects/lib/projectReviewChecks.d.mts b/desktop/src/features/projects/lib/projectReviewChecks.d.mts index 1d42925b18..eb68fcf97e 100644 --- a/desktop/src/features/projects/lib/projectReviewChecks.d.mts +++ b/desktop/src/features/projects/lib/projectReviewChecks.d.mts @@ -14,9 +14,17 @@ export type ProjectReviewCheckFinding = { line: number | null; }; +export type ProjectReviewCheckProposalReference = { + title: string; + summary: string; + diffEventId: string; +}; + export type ProjectReviewCheckResult = { requestId?: string; + /** Legacy single-proposal reference. New results use `proposals`. */ diffEventId?: string; + proposals?: ProjectReviewCheckProposalReference[]; conclusion: ProjectReviewCheckConclusion; summary: string; findings: ProjectReviewCheckFinding[]; @@ -32,6 +40,12 @@ export type ProjectReviewCheckDiffProposal = { truncated: boolean; }; +export type ProjectReviewCheckResolvedProposal = + ProjectReviewCheckDiffProposal & { + title: string; + summary: string; + }; + export const PROJECT_REVIEW_CHECK_RESULT_MARKER: string; export const DEFAULT_PROJECT_REVIEW_CHECKS: ProjectReviewCheckDefinition[]; diff --git a/desktop/src/features/projects/lib/projectReviewChecks.mjs b/desktop/src/features/projects/lib/projectReviewChecks.mjs index e844728f00..d92d637f59 100644 --- a/desktop/src/features/projects/lib/projectReviewChecks.mjs +++ b/desktop/src/features/projects/lib/projectReviewChecks.mjs @@ -75,6 +75,22 @@ function normalizedText(value, maxLength) { : null; } +function normalizedDecisionCopy(value) { + const text = normalizedText(value, 1_000)?.replace(/\s+/g, " "); + if (!text) return null; + + const sentenceBounded = text + .split(/(?<=[.!?])\s+/) + .slice(0, 2) + .join(" "); + if (sentenceBounded.length <= 240) return sentenceBounded; + + const visible = sentenceBounded.slice(0, 239); + const wordBoundary = visible.lastIndexOf(" "); + const cutoff = wordBoundary >= 160 ? wordBoundary : visible.length; + return `${visible.slice(0, cutoff).trimEnd()}…`; +} + function normalizedConclusion(value) { if (typeof value !== "string") return null; const normalized = value.trim().toLowerCase(); @@ -117,6 +133,18 @@ function normalizedFinding(value) { }; } +function normalizedProposalReference(value) { + if (!value || typeof value !== "object" || Array.isArray(value)) return null; + const title = normalizedText(value.title, 160); + const summary = normalizedDecisionCopy(value.summary); + const diffEventId = normalizedEventId( + value.diff_event_id ?? value.diffEventId, + ); + return title && summary && diffEventId + ? { title, summary, diffEventId } + : null; +} + function jsonObjectsAfterMarker(content) { const markerIndex = content.lastIndexOf(PROJECT_REVIEW_CHECK_RESULT_MARKER); if (markerIndex < 0) return []; @@ -147,7 +175,7 @@ export function parseProjectReviewCheckResult(content) { continue; } const conclusion = normalizedConclusion(parsed.conclusion); - const summary = normalizedText(parsed.summary, 4_000); + const summary = normalizedDecisionCopy(parsed.summary); if (!conclusion || !summary) continue; const requestId = normalizedText( parsed.request_id ?? parsed.requestId, @@ -155,11 +183,23 @@ export function parseProjectReviewCheckResult(content) { ); const rawDiffEventId = parsed.diff_event_id ?? parsed.diffEventId; const diffEventId = normalizedEventId(rawDiffEventId); + const rawProposals = parsed.proposals; + if (rawProposals !== undefined && !Array.isArray(rawProposals)) { + continue; + } + const proposalCandidates = Array.isArray(rawProposals) + ? rawProposals.slice(0, 4).map(normalizedProposalReference) + : []; + const proposals = proposalCandidates.filter(Boolean); if ( (rawDiffEventId !== undefined && rawDiffEventId !== null && !diffEventId) || - (conclusion === "approved" && diffEventId) + proposals.length !== proposalCandidates.length || + new Set(proposals.map((proposal) => proposal.diffEventId)).size !== + proposals.length || + (diffEventId && proposals.length > 0) || + (conclusion === "approved" && (diffEventId || proposals.length > 0)) ) { continue; } @@ -171,6 +211,9 @@ export function parseProjectReviewCheckResult(content) { ...(conclusion === "fix-recommended" && diffEventId ? { diffEventId } : {}), + ...(conclusion === "fix-recommended" && proposals.length > 0 + ? { proposals } + : {}), conclusion, summary, findings, @@ -306,8 +349,19 @@ export function buildProjectReviewCheckPrompt({ JSON.stringify({ request_id: requestId, conclusion: "fix-recommended", - summary: "short explanation", - diff_event_id: null, + summary: "one short sentence framing the decision", + proposals: [ + { + title: "first viable direction", + summary: "one plain-language sentence about the outcome", + diff_event_id: null, + }, + { + title: "countering viable direction", + summary: "one plain-language sentence about the tradeoff", + diff_event_id: null, + }, + ], findings: [ { title: "concise actionable fix", @@ -321,15 +375,19 @@ export function buildProjectReviewCheckPrompt({ "Return an empty findings array when approved. For every recommended fix, return one finding object.", "Use null for file or line when the finding is not tied to a precise source location.", "Use approved only when no material fix is recommended. Use fix-recommended otherwise.", + "Treat a fix-recommended result as a decision to negotiate, not a verdict. Frame the concern neutrally and look for countering viewpoints that could each be a defensible implementation.", + "When code can safely express alternatives, return two genuinely distinct proposal directions. Do not manufacture a second option when only one is safe.", + "DECISION COPY CONTRACT: The top-level summary and each proposal summary MUST be no more than two short sentences and 240 characters total; prefer one sentence. Proposal titles must be action-oriented and no more than eight words. State only the user-visible outcome or essential tradeoff in plain language. Put implementation details, identifiers, evidence, and reasoning in the finding or diff, never in decision copy.", + "A human may reply in this request's thread with their reasoning. Treat that as a continuation of this review negotiation: reconsider the concern, inspect any newly suggested direction, and publish a revised structured result in the same thread with the same request_id. Create new diff events for revised code proposals; never reuse an earlier diff event id.", ...(repoUrl && commit ? [ - "When recommending a fix and you can express it safely as code, create one valid unified diff against the exact commit without modifying or committing the review branch.", - `Pipe that patch into this command before the final result to publish one native Buzz diff message (kind 40008) in this request's thread: buzz messages send-diff --channel ${JSON.stringify(channelId)} --diff - --repo ${JSON.stringify(repoUrl)} --commit ${JSON.stringify(commit)} --description ${JSON.stringify(`Review check ${requestId}`)} --reply-to `, - "Only after send-diff succeeds, copy its exact 64-character event_id into diff_event_id in the final JSON. Never invent or reuse an event id. Do not paste the patch into the JSON.", - "Use diff_event_id: null when there is no safe code patch. Approved results must use diff_event_id: null.", + "For each proposed direction you can express safely as code, create a valid unified diff against the exact commit without modifying or committing the review branch.", + `Pipe each patch into this command before the final result to publish a native Buzz diff message (kind 40008) in this request's thread: buzz messages send-diff --channel ${JSON.stringify(channelId)} --diff - --repo ${JSON.stringify(repoUrl)} --commit ${JSON.stringify(commit)} --description --reply-to `, + "Only after each send-diff succeeds, copy its exact 64-character event_id into that proposal's diff_event_id in the final JSON. Never invent or reuse an event id. Do not paste patches into the JSON.", + "Omit any proposal that has no safe code patch. Return proposals: [] when no direction can be expressed safely as code. Approved results must return proposals: [].", ] : [ - "Use diff_event_id: null because this review does not expose the repository URL and exact commit required for a native diff proposal.", + "Return proposals: [] because this review does not expose the repository URL and exact commit required for native diff proposals.", ]), ].join("\n"); } diff --git a/desktop/src/features/projects/lib/projectReviewChecks.test.mjs b/desktop/src/features/projects/lib/projectReviewChecks.test.mjs index e979eff89f..f6a6aa89e0 100644 --- a/desktop/src/features/projects/lib/projectReviewChecks.test.mjs +++ b/desktop/src/features/projects/lib/projectReviewChecks.test.mjs @@ -150,6 +150,94 @@ test("parses structured findings with bounded source locations", () => { }); }); +test("parses competing proposal directions with exact diff references", () => { + const firstDiffEventId = "d".repeat(64); + const secondDiffEventId = "e".repeat(64); + const result = parseProjectReviewCheckResult( + `${PROJECT_REVIEW_CHECK_RESULT_MARKER}\n${JSON.stringify({ + request_id: "request-123", + conclusion: "fix-recommended", + summary: "Choose where the read boundary should live.", + proposals: [ + { + title: "Follow the rendered timeline", + summary: "Only mark messages read once they are visible.", + diff_event_id: firstDiffEventId, + }, + { + title: "Follow the loaded query", + summary: "Treat loaded messages as read at the physical bottom.", + diff_event_id: secondDiffEventId, + }, + ], + findings: [], + })}`, + ); + + assert.deepEqual(result, { + requestId: "request-123", + conclusion: "fix-recommended", + summary: "Choose where the read boundary should live.", + proposals: [ + { + title: "Follow the rendered timeline", + summary: "Only mark messages read once they are visible.", + diffEventId: firstDiffEventId, + }, + { + title: "Follow the loaded query", + summary: "Treat loaded messages as read at the physical bottom.", + diffEventId: secondDiffEventId, + }, + ], + findings: [], + }); +}); + +test("bounds proposal copy to two short sentences", () => { + const result = parseProjectReviewCheckResult(` +${PROJECT_REVIEW_CHECK_RESULT_MARKER} +${JSON.stringify({ + request_id: "request-123", + conclusion: "fix-recommended", + summary: "Choose a read boundary.", + proposals: [ + { + title: "Use rendered rows", + summary: + "Keep unread state tied to what the user can see. This preserves the navigation signal. This implementation detail should not appear in the decision copy.", + diff_event_id: "a".repeat(64), + }, + ], + findings: [], +})} +`); + + assert.equal( + result?.proposals?.[0]?.summary, + "Keep unread state tied to what the user can see. This preserves the navigation signal.", + ); +}); + +test("bounds the shared decision summary", () => { + const result = parseProjectReviewCheckResult(` +${PROJECT_REVIEW_CHECK_RESULT_MARKER} +${JSON.stringify({ + request_id: "request-123", + conclusion: "fix-recommended", + summary: + "Choose whether visibility or loaded state owns the read boundary. Both preserve valid behavior. This extra analysis belongs in the finding.", + proposals: [], + findings: [], +})} +`); + + assert.equal( + result?.summary, + "Choose whether visibility or loaded state owns the read boundary. Both preserve valid behavior.", + ); +}); + test("resolves only the exact agent diff for the reviewed repository and commit", () => { const eventId = "d".repeat(64); const agentPubkey = "a".repeat(64); @@ -204,6 +292,45 @@ test("resolves only the exact agent diff for the reviewed repository and commit" }), null, ); + assert.equal( + parseProjectReviewCheckResult( + `${PROJECT_REVIEW_CHECK_RESULT_MARKER}\n${JSON.stringify({ + conclusion: "fix-recommended", + summary: "Proposal references must be unique.", + proposals: [ + { + title: "First", + summary: "First direction.", + diff_event_id: "f".repeat(64), + }, + { + title: "Second", + summary: "Second direction.", + diff_event_id: "f".repeat(64), + }, + ], + findings: [], + })}`, + ), + null, + ); + assert.equal( + parseProjectReviewCheckResult( + `${PROJECT_REVIEW_CHECK_RESULT_MARKER}\n${JSON.stringify({ + conclusion: "fix-recommended", + summary: "Every proposal needs a verified patch reference.", + proposals: [ + { + title: "Missing reference", + summary: "This direction cannot be correlated to a patch.", + diff_event_id: null, + }, + ], + findings: [], + })}`, + ), + null, + ); }); test("does not treat ordinary agent prose as a completed check", () => { @@ -275,7 +402,13 @@ test("builds a review-only prompt pinned to the exact commit", () => { ); assert.match(prompt, /buzz messages send-diff/); assert.match(prompt, /send-diff[^\n]+--reply-to /); - assert.match(prompt, /"diff_event_id":null/); + assert.match(prompt, /"proposals":\[/); + assert.match(prompt, /countering viable direction/); + assert.match(prompt, /two genuinely distinct proposal directions/); + assert.match(prompt, /DECISION COPY CONTRACT/); + assert.match(prompt, /no more than two short sentences and 240 characters/); + assert.match(prompt, /implementation details, identifiers, evidence/); + assert.match(prompt, /continuation of this review negotiation/); assert.match(prompt, /copy its exact 64-character event_id/); assert.match(prompt, new RegExp(PROJECT_REVIEW_CHECK_RESULT_MARKER)); assert.doesNotMatch(prompt, /Superseded request id:/); diff --git a/desktop/src/features/projects/ui/ProjectReviewCheckResultCard.tsx b/desktop/src/features/projects/ui/ProjectReviewCheckResultCard.tsx index 9ff50b344e..f9b1f5630c 100644 --- a/desktop/src/features/projects/ui/ProjectReviewCheckResultCard.tsx +++ b/desktop/src/features/projects/ui/ProjectReviewCheckResultCard.tsx @@ -1,179 +1,411 @@ -import { Check, CheckCircle2, CircleAlert, FileDiff } from "lucide-react"; +import { Check, ChevronRight, FileDiff, LoaderCircle, X } from "lucide-react"; import * as React from "react"; +import { + countDiffFileChanges, + getDiffFilePath, + getShortestUniquePathLabels, + parseUnifiedDiff, +} from "@/features/messages/lib/parseDiff"; +import { DiffViewer } from "@/features/messages/ui/DiffViewer"; import type { - ProjectReviewCheckDiffProposal, + ProjectReviewCheckResolvedProposal, ProjectReviewCheckResult, } from "@/features/projects/lib/projectReviewChecks.mjs"; +import { cn } from "@/shared/lib/cn"; import { Button } from "@/shared/ui/button"; +import { Textarea } from "@/shared/ui/textarea"; -const DiffMessage = React.lazy( - () => import("@/features/messages/ui/DiffMessage"), -); -const DiffMessageExpanded = React.lazy( - () => import("@/features/messages/ui/DiffMessageExpanded"), -); +function ProposalDiffFiles({ + files, + parseError, + proposal, +}: { + files: ReturnType["files"]; + parseError: boolean; + proposal: ProjectReviewCheckResolvedProposal; +}) { + if (parseError || files.length === 0) { + const label = getShortestUniquePathLabels([ + proposal.filePath ?? "Proposed patch", + ])[0]; -function ReviewCheckDiffProposal({ + return ( +
+ + + + + {label} + + +
+ +
+
+ ); + } + + const labels = getShortestUniquePathLabels( + files.map((file) => getDiffFilePath(file, proposal.filePath ?? undefined)), + ); + + return ( +
+ {files.map((file, index) => { + const label = labels[index]; + const { additions, deletions } = countDiffFileChanges(file); + const key = `${file.oldPath ?? ""}:${file.newPath ?? ""}:${index}`; + + return ( +
+ + + + {label} + + + +{additions} + -{deletions} + + +
+ +
+
+ ); + })} +
+ ); +} + +function ProposalOption({ accepted, canAccept, + index, onAccept, proposal, }: { accepted: boolean; canAccept: boolean; + index: number; onAccept: () => void; - proposal: ProjectReviewCheckDiffProposal; + proposal: ProjectReviewCheckResolvedProposal; }) { - const [expanded, setExpanded] = React.useState(false); const acceptEnabled = canAccept && !proposal.truncated; + const parsedDiff = React.useMemo( + () => parseUnifiedDiff(proposal.content), + [proposal.content], + ); + const totals = parsedDiff.files.reduce( + (sum, file) => { + const changes = countDiffFileChanges(file); + return { + additions: sum.additions + changes.additions, + deletions: sum.deletions + changes.deletions, + }; + }, + { additions: 0, deletions: 0 }, + ); return ( -
-
-
- -
-
- Proposed code changes -
-

- Native Buzz diff · {proposal.commitSha.slice(0, 7)} +

+
+

+ Direction {index + 1} +

+
+ {proposal.title} +
+
+
+ {!parsedDiff.parseError && parsedDiff.files.length > 0 ? ( + + +{totals.additions} + -{totals.deletions} + + ) : null} + +
+
+ +
+ + + Read more + Hide details + +
+

+ {proposal.summary} +

+ {proposal.truncated || !canAccept ? ( +

+ {proposal.truncated + ? "The patch is incomplete and cannot be selected." + : "Run this check again to select a current proposal."}

-
+ ) : null} + +
+ + + ); +} + +function ReviewResponse({ + canRespond, + onRespond, +}: { + canRespond: boolean; + onRespond?: (response: string) => Promise; +}) { + const [response, setResponse] = React.useState(""); + const [expanded, setExpanded] = React.useState(false); + const [isSending, setIsSending] = React.useState(false); + const responseId = React.useId(); + const canSubmit = + canRespond && Boolean(onRespond) && Boolean(response.trim()); + + if (!expanded) { + return ( +
+
+

+ Propose another direction +

+

+ Suggest a different approach for the agent to consider. +

+
+ ); + } + + return ( +
{ + event.preventDefault(); + if (!canSubmit || !onRespond) return; + setIsSending(true); + void onRespond(response.trim()) + .then(() => { + setResponse(""); + setExpanded(false); + }) + .catch(() => undefined) + .finally(() => setIsSending(false)); + }} + > +
+ +

+ Suggest a different approach for the agent to consider. +

- - Loading proposed changes… -
- } - > - setExpanded(true)} - repoUrl={proposal.repoUrl} - truncated={proposal.truncated} +
+