Track fixed and ignored review findings

This commit is contained in:
Ian Gulliver
2026-10-16 10:44:27 -07:00
parent 0f2415570c
commit 8200ffd17e
20 changed files with 569 additions and 243 deletions
+1 -1
View File
@@ -115,7 +115,7 @@ export function taskPane() {
if (t === "reviews" && reviewing) {
return h("span", { class: "spinner", title: "review in progress" });
}
return count(t) > 0 && h("span", { class: "badge", title: t === "inbox" ? "inbox items" : "findings in the latest review" }, count(t));
return count(t) > 0 && h("span", { class: "badge", title: t === "inbox" ? "inbox items" : "open findings in the latest review" }, count(t));
};
const layout = () => {
for (const { button } of buttons) {
+1 -1
View File
File diff suppressed because one or more lines are too long
+45 -14
View File
@@ -1,7 +1,7 @@
import { get, post } from "./api.js";
import { get, post, put } from "./api.js";
import { ago, errorText, h, replace, showAlert } from "./dom.js";
import { markdown } from "./markdown.js";
import { focusTask, onEach, pickReview, state } from "./state.js";
import { onEach, pickReview, state } from "./state.js";
const severities = ["critical", "high", "medium", "low", "info"];
export function isReviewEvent(ev) {
return ev.kind.startsWith("review.");
@@ -11,14 +11,19 @@ function findingsOf(r) {
return Array.isArray(res["findings"]) ? res["findings"] : [];
}
export function findingCount(r) {
return r.state === "done" ? findingsOf(r).length : 0;
return r.state === "done" ? findingsOf(r).filter((_, i) => !r.finding_states[String(i)]).length : 0;
}
export function unignoredFindings(r) {
return r.state === "done" ? findingsOf(r).filter((_, i) => r.finding_states[String(i)] !== "ignored") : [];
}
export function latestReview(events) {
const all = events.filter(isReviewEvent).map((ev) => ev.payload);
return all.reduce((best, r) => (!best || r.snapshot_n >= best.snapshot_n ? r : best), undefined);
}
export function reviewCounts(r) {
const findings = findingsOf(r);
return severityCounts(findingsOf(r));
}
export function severityCounts(findings) {
const parts = severities.map((s) => [s, findings.filter((f) => f["severity"] === s).length]).filter(([, n]) => n > 0);
return parts.length > 0 ? parts.map(([s, n]) => `${n} ${s}`).join(", ") : "no findings";
}
@@ -42,13 +47,13 @@ function reviewResult(r, taskId) {
const findings = findingsOf(r);
const md = { taskId };
return h("div", { class: "review-result" }, markdown(String(res["summary"] ?? ""), "md review-summary", md), reviewImages(res["images"]), findings.length > 0
? h("ol", { class: "findings" }, ...findings.map((f) => {
? h("ol", { class: "findings" }, ...findings.map((f, i) => {
const severity = String(f["severity"] ?? "");
const path = typeof f["path"] === "string" ? f["path"] : "";
const line = typeof f["line"] === "number" && f["line"] > 0 ? f["line"] : 0;
const end = typeof f["end_line"] === "number" && f["end_line"] > line ? f["end_line"] : line;
const where = path ? `${path}${line ? `:${line}${end > line ? `–${end}` : ""}` : ""}` : "";
return h("li", { class: `finding ${severity}` }, h("div", { class: "finding-head" }, h("span", { class: `severity ${severity}` }, severity), typeof f["category"] === "string" && f["category"] ? h("span", { class: "finding-category" }, f["category"]) : null, h("span", { class: "finding-title" }, String(f["title"] ?? "")), where ? h("code", { class: "finding-where" }, where) : null, taskId ? fixButton(taskId, r, f, where) : null), taskId && path && line ? findingDiff(taskId, r.snapshot_n, path, line, end) : null, markdown(String(f["detail"] ?? ""), "md finding-detail", md), reviewImages(f["images"]));
return h("li", { class: `finding ${severity}` }, h("div", { class: "finding-head" }, h("span", { class: `severity ${severity}` }, severity), typeof f["category"] === "string" && f["category"] ? h("span", { class: "finding-category" }, f["category"]) : null, h("span", { class: "finding-title" }, String(f["title"] ?? "")), where ? h("code", { class: "finding-where" }, where) : null, taskId ? findingActions(taskId, r, i, f, where) : null), taskId && path && line ? findingDiff(taskId, r.snapshot_n, path, line, end) : null, markdown(String(f["detail"] ?? ""), "md finding-detail", md), reviewImages(f["images"]));
}))
: null);
}
@@ -139,11 +144,21 @@ function findingDiff(taskId, n, path, line, end) {
}, (err) => replace(box, h("div", { class: "hint" }, `diff not available: ${errorText(err)}`)));
return box;
}
function fixButton(taskId, r, f, where) {
const button = h("button", { type: "button", class: "finding-fix", title: "Send this finding to the task's agent to fix" }, "fix");
button.onclick = async () => {
function findingActions(taskId, r, index, f, where) {
const box = h("span", { class: "finding-actions" });
const done = (s) => replace(box, h("span", { class: "finding-done" }, s === "fixed" ? "fix sent" : "ignored"));
const current = r.finding_states[String(index)];
if (current) {
done(current);
return box;
}
const mark = (s) => put("/api/reviews/{reviewId}/findings/{index}", { path: { reviewId: r.id, index }, body: { state: s } });
const fix = h("button", { type: "button", class: "finding-fix", title: "Send this finding to the task's agent to fix" }, "fix");
const ignore = h("button", { type: "button", class: "finding-ignore", title: "Mark this finding done without fixing it; Merge won't warn about it" }, "ignore");
fix.onclick = async () => {
const text = `Fix this finding from the review of snapshot #${r.snapshot_n}:\n\n**[${String(f["severity"] ?? "")}] ${String(f["title"] ?? "")}**${where ? ` (\`${where}\`)` : ""}\n\n${String(f["detail"] ?? "")}`;
button.disabled = true;
fix.disabled = true;
ignore.disabled = true;
try {
const sessions = await get("/api/tasks/{taskId}/sessions", { path: { taskId } });
const live = sessions.findLast((s) => s.state !== "completed");
@@ -153,15 +168,31 @@ function fixButton(taskId, r, f, where) {
else {
await post("/api/tasks/{taskId}/sessions", { path: { taskId }, body: { prompt: text } });
}
replace(button, "sent");
focusTask(taskId);
}
catch (err) {
button.disabled = false;
fix.disabled = false;
ignore.disabled = false;
void showAlert(errorText(err));
return;
}
done("fixed");
await mark("fixed").catch((err) => showAlert(`fix sent, but not recorded: ${errorText(err)}`));
};
ignore.onclick = async () => {
fix.disabled = true;
ignore.disabled = true;
try {
await mark("ignored");
done("ignored");
}
catch (err) {
fix.disabled = false;
ignore.disabled = false;
void showAlert(errorText(err));
}
};
return button;
replace(box, fix, ignore);
return box;
}
export function mountReviews(root) {
const head = h("header", { class: "reviews-header" });
+1 -1
View File
File diff suppressed because one or more lines are too long
+5
View File
@@ -292,6 +292,11 @@ export function handleEvent(ev) {
void loadInbox();
}
break;
case "review.finding":
if (state.inbox.some((i) => i.task_id === ev.task_id && i.kind === "question")) {
void loadInbox();
}
break;
case "focus.updated":
if (ev.actor !== state.me?.login) {
applyFocus(ev.payload);
+1 -1
View File
File diff suppressed because one or more lines are too long
+6 -6
View File
@@ -2,7 +2,7 @@ import { get, post } from "./api.js";
import { ago, copyable, errorText, h, replace, showAlert, showConfirm } from "./dom.js";
import { markdown } from "./markdown.js";
import { previewURL } from "./preview.js";
import { findingCount, reviewCounts } from "./reviews.js";
import { findingCount, reviewCounts, severityCounts, unignoredFindings } from "./reviews.js";
import { composerText, endSend, flushDrafts, forgetLostImages, lostImageCount, setComposerImages, setComposerText, startSend } from "./drafts.js";
import { currentSession, findTask, focusTab, focusTask, on, onEach, pickReview, pinSnapshot, setFocus, showReview, state } from "./state.js";
import { taskStyle } from "./taskcolor.js";
@@ -377,7 +377,7 @@ export function mountChat(root, src, makeHead) {
put(snapRow(p, ctx));
continue;
}
if (ev.kind === "review.started" || ev.kind === "review.finished") {
if (ev.kind === "review.started" || ev.kind === "review.finished" || ev.kind === "review.finding") {
const r = p;
const button = ctx.reviewButtons.get(r.snapshot_n);
if (button) {
@@ -797,10 +797,10 @@ function markReview(button, r) {
return;
}
if (n === 0) {
replace(button, "review", h("span", { class: "review-mark clean", title: "no findings" }, "✓"));
replace(button, "review", h("span", { class: "review-mark clean", title: Object.keys(r.finding_states).length > 0 ? "every finding addressed" : "no findings" }, "✓"));
return;
}
replace(button, "review", h("span", { class: "badge", title: `${n} findings` }, n));
replace(button, "review", h("span", { class: "badge", title: `${n} open findings` }, n));
}
function snapRow(snap, ctx) {
const taskId = ctx.taskId;
@@ -1142,8 +1142,8 @@ async function reviewWarnings(taskId, leave) {
else if (r.state === "failed") {
items.push(mergeWarning("Review of the latest snapshot failed", open));
}
else if (findingCount(r) > 0) {
items.push(mergeWarning(`Review of the latest snapshot found ${reviewCounts(r)}`, open));
else if (unignoredFindings(r).length > 0) {
items.push(mergeWarning(`Review of the latest snapshot found ${severityCounts(unignoredFindings(r))}`, open));
}
const changes = await get("/api/tasks/{taskId}/changes", { path: { taskId }, query: { since: latest.commit_sha } });
if (changes.files.length > 0) {
+1 -1
View File
File diff suppressed because one or more lines are too long