From 40fbdedd9f8a5a81e0f4f99880e56cacec6c6999 Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Fri, 23 Oct 2026 14:52:24 +1100 Subject: [PATCH 1/4] feat(review-board): post free-form comments from the details panel Adds a Comment panel to the External Plugin Review Board drawer with a two-click confirm. Bodies must be non-empty and cannot start with '/' so they can't trigger the command router. Posts via the existing gh.postComment helper and per-issue in-flight guard, then reloads the Comments tab. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../external-plugin-review-board/README.md | 10 +- .../extension.mjs | 15 +++ .../lib/github.mjs | 18 ++++ .../lib/github.test.mjs | 24 ++++- .../public/app.js | 94 +++++++++++++++++++ .../public/index.html | 16 ++++ .../public/styles.css | 4 + eng/board-preview.test.mjs | 53 +++++++++++ 8 files changed, 232 insertions(+), 2 deletions(-) diff --git a/.github/extensions/external-plugin-review-board/README.md b/.github/extensions/external-plugin-review-board/README.md index ecad4e8832..a44116ca7f 100644 --- a/.github/extensions/external-plugin-review-board/README.md +++ b/.github/extensions/external-plugin-review-board/README.md @@ -41,6 +41,11 @@ Open it by asking Copilot to "open the plugin review board" (canvas id `external failed actions leave it open so you can correct or retry them. - **Quick actions** post `/approve` (with an optional note) or `/reject ` straight to the issue after a confirm click. The reject reason is prefilled from the AI's suggested comment. +- **Comment** (in the details panel) posts a free-form comment, for example a question for the + submitter, without changing the board state. The comment must be non-empty and needs a second + *Confirm post* click. Comments starting with `/` are rejected so they can't trigger the command + router; use the dedicated actions for commands. After posting, the panel stays open on the + *Comments* tab and reloads it from GitHub. - **Column actions** approve, reject, or re-review every eligible item in a column, including cards hidden by search. Confirmation previews each staged comment; rejection requires a reason for every item. Drawer edits remain staged when switching cards in the current browser session. @@ -74,6 +79,9 @@ Run `node --experimental-vm-modules --test eng/board-state-storage.test.mjs` fro to check both canvases' session-scoped persistence without accessing GitHub. Run `node --test eng/board-bulk-actions.test.mjs` to check column confirmation, staged feedback, queue exclusions, and partial failures using in-process fakes. -Run `node --test eng/board-preview.test.mjs` to check preview dismissal and in-flight actions. +Run `node --test eng/board-preview.test.mjs` to check preview dismissal, in-flight actions, and the +free-form comment confirm/post flow. +Run `node --test .github/extensions/external-plugin-review-board/lib/github.test.mjs` to check board +feed filtering and free-form comment validation. Run `node --test eng/external-plugin-review-board-state.test.mjs eng/external-plugin-review-board-ui.test.mjs` to check intake-blocked reasons, eligibility transitions, visible status, and unavailable controls. diff --git a/.github/extensions/external-plugin-review-board/extension.mjs b/.github/extensions/external-plugin-review-board/extension.mjs index 7608033194..3e5a76c453 100644 --- a/.github/extensions/external-plugin-review-board/extension.mjs +++ b/.github/extensions/external-plugin-review-board/extension.mjs @@ -182,6 +182,20 @@ async function postDecisionCommand({ number, kind, comment }) { return { number, ...pendingDecision, pending: true }; } +async function postFreeformComment({ number, body }) { + const text = gh.normalizeFreeformComment(body); + store.getItem(number); + if (decisionsInFlight.has(number)) throw new Error(`#${number} already has a GitHub action in progress.`); + decisionsInFlight.add(number); + try { + const commentUrl = await gh.postComment(store.state.repo, number, text); + detailCache.delete(number); + return { number, commentUrl }; + } finally { + decisionsInFlight.delete(number); + } +} + function boardSummary() { const snapshot = store.snapshot(); return { @@ -221,6 +235,7 @@ function routesFor(instanceId) { "POST /api/issue/:number": ({ number }) => getIssue(number, { force: true }), "POST /api/decision": ({ body }) => postDecision({ number: Number(body.number), kind: body.kind, comment: body.comment }), + "POST /api/comment": ({ body }) => postFreeformComment({ number: Number(body.number), body: body.body }), }; // Board errors are user-facing messages (e.g. "already actioned", gh failures); surface only the // message text, never the stack, and log the full error for debugging. diff --git a/.github/extensions/external-plugin-review-board/lib/github.mjs b/.github/extensions/external-plugin-review-board/lib/github.mjs index bcca92e958..d328431d44 100644 --- a/.github/extensions/external-plugin-review-board/lib/github.mjs +++ b/.github/extensions/external-plugin-review-board/lib/github.mjs @@ -98,6 +98,24 @@ export async function getIssueStatus(repo, number) { return { state: data.state, labels: (data.labels ?? []).map((label) => label.name) }; } +export const MAX_COMMENT_LENGTH = 60000; + +// Free-form comments must never be parsed as slash commands by the external plugin command router, +// which matches on the comment body starting with `/`. +export function normalizeFreeformComment(body) { + const text = String(body ?? "").trim(); + if (!text) throw new Error("Comment cannot be empty."); + if (text.length > MAX_COMMENT_LENGTH) { + throw new Error(`Comment is too long (${text.length} characters; maximum ${MAX_COMMENT_LENGTH}).`); + } + if (text.startsWith("/")) { + throw new Error( + "Comments starting with \"/\" could trigger issue automation. Use the Approve, Reject, or Re-review actions instead, or reword the comment.", + ); + } + return text; +} + export async function postComment(repo, number, body) { const out = await runGh(["issue", "comment", String(number), "-R", repo, "--body-file", "-"], { input: body }); return out.trim(); diff --git a/.github/extensions/external-plugin-review-board/lib/github.test.mjs b/.github/extensions/external-plugin-review-board/lib/github.test.mjs index 5ac6dd2c24..089a7552f7 100644 --- a/.github/extensions/external-plugin-review-board/lib/github.test.mjs +++ b/.github/extensions/external-plugin-review-board/lib/github.test.mjs @@ -1,7 +1,29 @@ import assert from "node:assert/strict"; import { test } from "node:test"; -import { mapBoardIssues } from "./github.mjs"; +import { MAX_COMMENT_LENGTH, mapBoardIssues, normalizeFreeformComment } from "./github.mjs"; + +test("free-form comments are trimmed and posted as written", () => { + assert.equal(normalizeFreeformComment(" Could you add a README?\n/approve is not needed "), + "Could you add a README?\n/approve is not needed"); +}); + +test("free-form comments reject empty bodies", () => { + for (const body of ["", " \n\t", null, undefined]) { + assert.throws(() => normalizeFreeformComment(body), /cannot be empty/); + } +}); + +test("free-form comments cannot start with a slash command", () => { + for (const body of ["/approve", " /reject nope", "/rerun-intake", "/mark-ready-for-review", "/anything"]) { + assert.throws(() => normalizeFreeformComment(body), /could trigger issue automation/); + } +}); + +test("free-form comments enforce a maximum length", () => { + assert.equal(normalizeFreeformComment("a".repeat(MAX_COMMENT_LENGTH)).length, MAX_COMMENT_LENGTH); + assert.throws(() => normalizeFreeformComment("a".repeat(MAX_COMMENT_LENGTH + 1)), /too long/); +}); test("board feed includes submissions in each active intake state", () => { const issues = ["ready-for-review", "awaiting-review", "requires-submitter-fixes", "rejected"].map( diff --git a/.github/extensions/external-plugin-review-board/public/app.js b/.github/extensions/external-plugin-review-board/public/app.js index 8764ad49a2..31a4e270ff 100644 --- a/.github/extensions/external-plugin-review-board/public/app.js +++ b/.github/extensions/external-plugin-review-board/public/app.js @@ -26,6 +26,11 @@ const ui = { rereviewGuidance: document.getElementById("rereview-guidance"), rereviewCancel: document.getElementById("rereview-cancel"), rereviewStart: document.getElementById("rereview-start"), + drawerComment: document.getElementById("drawer-comment"), + commentPanel: document.getElementById("comment-panel"), + freeformComment: document.getElementById("freeform-comment"), + commentCancel: document.getElementById("comment-cancel"), + commentPost: document.getElementById("comment-post"), drawerLink: document.getElementById("drawer-link"), drawerBody: document.getElementById("drawer-body"), drawerFooter: document.getElementById("drawer-footer"), @@ -43,6 +48,7 @@ let selected = null; let activeTab = "review"; const details = new Map(); const pendingConfirm = { kind: null, timer: null }; +const commentConfirm = { armed: false, timer: null }; const stagedComments = new Map(); let bulkBusy = false; @@ -379,6 +385,7 @@ function openDrawer(number) { selected = number; resetConfirm(); hideRereview(); + hideComment(); const item = findItem(number); ui.comment.value = stagedComment(item); ui.drawer.classList.add("open"); @@ -394,6 +401,7 @@ function closeDrawer() { selected = null; resetConfirm(); hideRereview(); + hideComment(); ui.drawer.classList.remove("open"); ui.drawer.setAttribute("aria-hidden", "true"); renderBoard(); @@ -718,6 +726,7 @@ ui.drawerRereview.addEventListener("click", () => { hideRereview(); return; } + hideComment(); ui.rereviewPanel.hidden = false; ui.drawerRereview.setAttribute("aria-expanded", "true"); ui.rereviewGuidance.focus(); @@ -752,6 +761,91 @@ ui.rereviewPanel.addEventListener("submit", async (event) => { } }); +// ---------- free-form comments ---------- + +function freeformCommentError(text) { + if (!text) return "Write a comment before posting."; + if (text.startsWith("/")) { + return "Comments starting with “/” could trigger issue automation. Use Approve, Reject, or Re-review instead, or reword the comment."; + } + return null; +} + +function resetCommentConfirm() { + clearTimeout(commentConfirm.timer); + commentConfirm.armed = false; + ui.commentPost.textContent = "Post comment"; + ui.commentPost.classList.remove("confirming"); +} + +function hideComment() { + resetCommentConfirm(); + ui.commentPanel.hidden = true; + ui.freeformComment.value = ""; + ui.drawerComment.setAttribute("aria-expanded", "false"); +} + +ui.drawerComment.addEventListener("click", () => { + if (!ui.commentPanel.hidden) { + hideComment(); + return; + } + hideRereview(); + ui.commentPanel.hidden = false; + ui.drawerComment.setAttribute("aria-expanded", "true"); + ui.freeformComment.focus(); +}); +ui.commentCancel.addEventListener("click", () => { + hideComment(); + ui.drawerComment.focus(); +}); +ui.freeformComment.addEventListener("input", resetCommentConfirm); +ui.freeformComment.addEventListener("keydown", (event) => { + if (event.key === "Enter" && (event.ctrlKey || event.metaKey)) { + event.preventDefault(); + ui.commentPanel.requestSubmit(); + } else if (event.key === "Escape") { + event.stopPropagation(); + hideComment(); + ui.drawerComment.focus(); + } +}); +ui.commentPanel.addEventListener("submit", async (event) => { + event.preventDefault(); + const number = selected; + if (number == null) return; + const text = ui.freeformComment.value.trim(); + const problem = freeformCommentError(text); + if (problem) { + resetCommentConfirm(); + toast(problem, { error: true }); + return; + } + if (!commentConfirm.armed) { + commentConfirm.armed = true; + ui.commentPost.textContent = `Confirm post to #${number}`; + ui.commentPost.classList.add("confirming"); + commentConfirm.timer = setTimeout(resetCommentConfirm, 6000); + return; + } + resetCommentConfirm(); + ui.commentPost.disabled = true; + try { + const result = await api("POST", "/api/comment", { number, body: text }); + toast(`Posted comment on #${number}`, { link: result.commentUrl }); + if (selected === number) { + hideComment(); + activeTab = "comments"; + renderDrawer(); + } + await loadDetail(number, { force: true }); + } catch (error) { + toast(error.message, { error: true }); + } finally { + ui.commentPost.disabled = false; + } +}); + for (const tab of ui.tabs) { tab.addEventListener("click", () => { activeTab = tab.dataset.tab; diff --git a/.github/extensions/external-plugin-review-board/public/index.html b/.github/extensions/external-plugin-review-board/public/index.html index 6528f9b037..f564765a2e 100644 --- a/.github/extensions/external-plugin-review-board/public/index.html +++ b/.github/extensions/external-plugin-review-board/public/index.html @@ -41,9 +41,25 @@

+ Open on GitHub ↗ +