diff --git a/desktop/src/features/projects/lib/projectReviewChecks.d.mts b/desktop/src/features/projects/lib/projectReviewChecks.d.mts index dc951439d8..aa9d0b82b6 100644 --- a/desktop/src/features/projects/lib/projectReviewChecks.d.mts +++ b/desktop/src/features/projects/lib/projectReviewChecks.d.mts @@ -7,10 +7,17 @@ export type ProjectReviewCheckDefinition = { export type ProjectReviewCheckConclusion = "approved" | "fix-recommended"; +export type ProjectReviewCheckFinding = { + title: string; + detail: string | null; + file: string | null; + line: number | null; +}; + export type ProjectReviewCheckResult = { conclusion: ProjectReviewCheckConclusion; summary: string; - findings: string[]; + findings: ProjectReviewCheckFinding[]; }; export const PROJECT_REVIEW_CHECK_RESULT_MARKER: string; diff --git a/desktop/src/features/projects/lib/projectReviewChecks.mjs b/desktop/src/features/projects/lib/projectReviewChecks.mjs index 2dd78ec2c3..65c80f0e3f 100644 --- a/desktop/src/features/projects/lib/projectReviewChecks.mjs +++ b/desktop/src/features/projects/lib/projectReviewChecks.mjs @@ -144,6 +144,26 @@ function normalizedConclusion(value) { return null; } +function normalizedFinding(value) { + if (typeof value === "string") { + const title = normalizedText(value, 240); + return title ? { title, detail: null, file: null, line: null } : null; + } + if (!value || typeof value !== "object" || Array.isArray(value)) return null; + const title = normalizedText(value.title, 240); + if (!title) return null; + const line = + Number.isInteger(value.line) && value.line > 0 && value.line <= 10_000_000 + ? value.line + : null; + return { + title, + detail: normalizedText(value.detail, 1_000), + file: normalizedText(value.file, 500), + line, + }; +} + function jsonObjectsAfterMarker(content) { const markerIndex = content.lastIndexOf(PROJECT_REVIEW_CHECK_RESULT_MARKER); if (markerIndex < 0) return []; @@ -177,10 +197,7 @@ export function parseProjectReviewCheckResult(content) { const summary = normalizedText(parsed.summary, 4_000); if (!conclusion || !summary) continue; const findings = Array.isArray(parsed.findings) - ? parsed.findings - .map((finding) => normalizedText(finding, 1_000)) - .filter(Boolean) - .slice(0, 20) + ? parsed.findings.map(normalizedFinding).filter(Boolean).slice(0, 20) : []; return { conclusion, summary, findings }; } catch { @@ -222,7 +239,9 @@ export function buildProjectReviewCheckPrompt({ "", "End your response with exactly this marker and one JSON object:", PROJECT_REVIEW_CHECK_RESULT_MARKER, - '{"conclusion":"approved|fix-recommended","summary":"short explanation","findings":["actionable finding"]}', + '{"conclusion":"approved|fix-recommended","summary":"short explanation","findings":[{"title":"concise actionable fix","detail":"why this matters and what to change","file":"relative/path.ext","line":123}]}', + "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.", ].join("\n"); } diff --git a/desktop/src/features/projects/lib/projectReviewChecks.test.mjs b/desktop/src/features/projects/lib/projectReviewChecks.test.mjs index c5399b82d2..1f51066998 100644 --- a/desktop/src/features/projects/lib/projectReviewChecks.test.mjs +++ b/desktop/src/features/projects/lib/projectReviewChecks.test.mjs @@ -89,7 +89,62 @@ test("normalizes a changes-requested result to fix-recommended", () => { assert.deepEqual(result, { conclusion: "fix-recommended", summary: "The empty state has no accessible name.", - findings: ["Add an accessible label.", "Cover it with a UI test."], + findings: [ + { + title: "Add an accessible label.", + detail: null, + file: null, + line: null, + }, + { + title: "Cover it with a UI test.", + detail: null, + file: null, + line: null, + }, + ], + }); +}); + +test("parses structured findings with bounded source locations", () => { + const result = parseProjectReviewCheckResult( + `${PROJECT_REVIEW_CHECK_RESULT_MARKER}\n${JSON.stringify({ + conclusion: "fix-recommended", + summary: "Two interface fixes are needed.", + findings: [ + { + title: "Use the shared button primitive", + detail: "The custom button misses the standard focus treatment.", + file: "desktop/src/features/projects/ui/ProjectReviewChecks.tsx", + line: 742, + }, + { + title: "Add an accessible name", + detail: "Icon-only controls need an explicit label.", + file: null, + line: -1, + }, + ], + })}`, + ); + + assert.deepEqual(result, { + conclusion: "fix-recommended", + summary: "Two interface fixes are needed.", + findings: [ + { + title: "Use the shared button primitive", + detail: "The custom button misses the standard focus treatment.", + file: "desktop/src/features/projects/ui/ProjectReviewChecks.tsx", + line: 742, + }, + { + title: "Add an accessible name", + detail: "Icon-only controls need an explicit label.", + file: null, + line: null, + }, + ], }); }); diff --git a/desktop/src/features/projects/ui/ProjectReviewChecks.tsx b/desktop/src/features/projects/ui/ProjectReviewChecks.tsx index 77b6b175db..254e543810 100644 --- a/desktop/src/features/projects/ui/ProjectReviewChecks.tsx +++ b/desktop/src/features/projects/ui/ProjectReviewChecks.tsx @@ -353,6 +353,74 @@ function CheckStatus({ return null; } +function ReviewCheckResultCard({ + result, +}: { + result: ProjectReviewCheckResult; +}) { + const isApproved = result.conclusion === "approved"; + const findingCount = result.findings.length; + const title = isApproved + ? "Approved" + : findingCount === 0 + ? "Fix recommended" + : `${findingCount} ${findingCount === 1 ? "fix" : "fixes"} recommended`; + + return ( +
+
+
+ {isApproved ? ( + + ) : ( + + )} +
+
+
{title}
+

+ {result.summary} +

+
+
+ {findingCount > 0 ? ( +
+ {result.findings.map((finding) => ( +
+
+

+ {finding.title} +

+ {finding.file ? ( + + {finding.file} + {finding.line ? `:${finding.line}` : ""} + + ) : null} +
+ {finding.detail ? ( +

+ {finding.detail} +

+ ) : null} +
+ ))} +
+ ) : null} +
+ ); +} + function AgentPicker({ candidates, disabled, @@ -763,18 +831,7 @@ export function ProjectReviewChecks({

{run.error}

) : null} {run.result ? ( -
-

- {run.result.summary} -

- {run.result.findings.length > 0 ? ( - - ) : null} -
+ ) : null} {run.status === "running" && run.opener && run.agentPubkey ? ( { + window.__BUZZ_E2E_EMIT_MOCK_MESSAGE__?.({ + channelName: "DM", + content: `BUZZ_CHECK_RESULT_V1\n${JSON.stringify({ + conclusion: "fix-recommended", + summary: "Two interface fixes are needed before approval.", + findings: [ + { + title: "Use the shared button primitive", + detail: "The custom control misses the standard focus treatment.", + file: "desktop/src/features/projects/ui/ProjectReviewChecks.tsx", + line: 742, + }, + { + title: "Cover the disabled interaction state", + detail: "Add a UI assertion for the pending check state.", + file: "desktop/tests/e2e/project-pr-review.spec.ts", + line: 118, + }, + ], + })}`, + createdAt: Math.floor(Date.now() / 1_000) + 1, + kind: 9, + pubkey: agentPubkey, + }); + }, RELAY_REVIEW_AGENT_PUBKEY); + + const result = interfaceCheck.getByTestId("project-review-check-result"); + await expect(result.getByRole("heading")).toHaveText("2 fixes recommended"); + await expect(result).toContainText( + "Two interface fixes are needed before approval.", + ); + await expect(result.getByTestId("project-review-check-finding")).toHaveCount( + 2, + ); + await expect(result).toContainText("ProjectReviewChecks.tsx:742"); + await waitForAnimations(page); + await result.screenshot({ path: `${SHOTS}/review-check-result.png` }); }); test("review checks dispatch to a running local managed agent", async ({