From e9262f6c984a5a842db546fb258f900030feb56c Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Mon, 12 Oct 2026 10:57:37 +1100 Subject: [PATCH] fix(canvas): address PR board review feedback Fence review results with required queue tokens, lock concurrent GitHub actions, recover persistence after errors, and reopen feedbacked PRs on new commits. Guard detail request races and add modal focus management and keyboard tabs with regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8045e7e0-dfd7-425a-9131-21ea5ca1553e --- .github/extensions/open-pr-board/README.md | 13 + .../extensions/open-pr-board/extension.mjs | 49 +-- .../open-pr-board/lib/decisions.mjs | 54 ++++ .../extensions/open-pr-board/lib/github.mjs | 2 + .../open-pr-board/lib/review-prompt.mjs | 7 +- .../extensions/open-pr-board/lib/state.mjs | 22 +- .../extensions/open-pr-board/public/app.js | 76 ++++- .../open-pr-board/public/index.html | 14 +- .../open-pr-board/review-guidance.md | 6 +- eng/open-pr-board.test.mjs | 306 ++++++++++++++++++ 10 files changed, 481 insertions(+), 68 deletions(-) create mode 100644 .github/extensions/open-pr-board/lib/decisions.mjs create mode 100644 eng/open-pr-board.test.mjs diff --git a/.github/extensions/open-pr-board/README.md b/.github/extensions/open-pr-board/README.md index 3091a4d02..4a930d12a 100644 --- a/.github/extensions/open-pr-board/README.md +++ b/.github/extensions/open-pr-board/README.md @@ -18,13 +18,18 @@ authenticated `gh` CLI. - **State tracking** — each PR remembers whether it has been reviewed, when, what the agent concluded, any manual move, and any decision you recorded. A PR updated after its review is flagged `PR changed` and is picked up by the next review pass. + PRs with submitted feedback or requested changes return to Unreviewed after new + commits; the previous action remains in history. - **Re-review** — ask for a fresh pass on one PR with optional focus guidance. - **Labels** — GitHub labels render on every card in their own colours, blended toward the app theme so they stay readable in both light and dark mode. - **Detail drawer** — AI review, description, conversation, changed files, and checks. + Keyboard focus stays inside the modal drawer and returns to its card on close. + Use Left/Right arrows or Home/End to switch detail tabs. - **GitHub actions** — submit feedback, request changes, close a PR, or approve it and enable auto-merge with a merge commit. Each action needs two clicks to confirm and is also recorded in local board history. + Only one GitHub action can run per PR at a time. - **Reset board** — after a second click to confirm, clear every local AI review, queued review, manual move, recorded action, and history entry, then reload open PRs. Reset never undoes or changes anything on GitHub. @@ -63,3 +68,11 @@ procedure on top of `.github/skills/code-review/SKILL.md` and `.github/copilot-instructions.md`. AI evidence gathering is read-only: the agent never edits files or runs write commands against the PR. Only an explicitly confirmed drawer action writes to GitHub. + +Review results require the exact queue token from the current review request. +Results arriving after a manual move, reset, or replacement review are rejected. + +## Regression tests + +Run `node --test eng/open-pr-board.test.mjs` from the repository root. GitHub writes +are tested with injected in-process fakes, never the authenticated CLI. diff --git a/.github/extensions/open-pr-board/extension.mjs b/.github/extensions/open-pr-board/extension.mjs index 5aa472a38..cce764687 100644 --- a/.github/extensions/open-pr-board/extension.mjs +++ b/.github/extensions/open-pr-board/extension.mjs @@ -10,6 +10,7 @@ import { fileURLToPath } from "node:url"; import { joinSession, createCanvas, CanvasError } from "@github/copilot-sdk/extension"; import { BoardStore, RECOMMENDATIONS } from "./lib/state.mjs"; import * as gh from "./lib/github.mjs"; +import { applyDecisionOnGitHub } from "./lib/decisions.mjs"; import { buildReviewPrompt, buildRereviewPrompt, REVIEW_FIELDS_SCHEMA } from "./lib/review-prompt.mjs"; import { startBoardServer, expectedError } from "./lib/server.mjs"; @@ -22,8 +23,6 @@ const PUBLIC_DIR = path.join(EXT_DIR, "public"); const GUIDANCE_PATH = path.join(EXT_DIR, "review-guidance.md"); const DETAIL_TTL_MS = 2 * 60 * 1000; const MAX_GUIDANCE_LENGTH = 4000; -const MAX_COMMENT_LENGTH = 60000; -const DECISION_KINDS = ["approve", "request-changes", "comment", "close"]; const stores = new Map(); const servers = new Map(); @@ -174,50 +173,6 @@ async function startRereview({ store, number, guidance, instanceId, send }) { return { queued: [item.number], message: `Asked the agent to re-review #${item.number}.` }; } -async function applyDecisionOnGitHub({ store, number, kind, comment }) { - if (!DECISION_KINDS.includes(kind)) throw new Error(`Unknown decision "${kind}".`); - const item = requireItem(store, number); - if (item.decision) throw new Error(`#${number} was already actioned (${item.decision.kind}).`); - - const body = String(comment ?? "") - .trim() - .slice(0, MAX_COMMENT_LENGTH); - if ((kind === "request-changes" || kind === "comment") && !body) { - throw new Error(`A ${kind} decision needs a non-empty comment.`); - } - - let result; - if (kind === "approve") { - result = await gh.approveAndEnableAutoMerge(store.repo, number, { body }); - } else if (kind === "close") { - result = await gh.closePullRequest(store.repo, number, { body }); - } else { - result = await gh.submitPullRequestReview(store.repo, number, { kind, body }); - } - - const warnings = result.warning ? [result.warning] : []; - try { - await store.recordDecision(number, { kind, comment: body, status: result.status }); - } catch (error) { - warnings.push(`GitHub was updated, but the board could not record the action: ${error.message}`); - } - try { - await refresh(store); - } catch (error) { - warnings.push(`GitHub was updated, but the board could not refresh: ${error.message}`); - } - - return { - number, - kind, - comment: body, - posted: true, - status: result.status, - warning: warnings.length ? warnings.join(" ") : null, - url: item.url, - }; -} - function boardSummary(store) { const snapshot = store.snapshot(); return { @@ -259,7 +214,7 @@ function routesFor(instanceId, store) { "GET /api/pr/:number": ({ number }) => getPullRequest(store, number), "POST /api/pr/:number": ({ number }) => getPullRequest(store, number, { force: true }), "POST /api/decision": ({ body }) => - applyDecisionOnGitHub({ store, number: Number(body.number), kind: body.kind, comment: body.comment }), + applyDecisionOnGitHub({ store, number: Number(body.number), kind: body.kind, comment: body.comment, refresh }), }; // Board errors are user-facing messages (gh failures, "already actioned", ...); surface only the // message text, never the stack, and log the full error for debugging. diff --git a/.github/extensions/open-pr-board/lib/decisions.mjs b/.github/extensions/open-pr-board/lib/decisions.mjs new file mode 100644 index 000000000..dab2202d1 --- /dev/null +++ b/.github/extensions/open-pr-board/lib/decisions.mjs @@ -0,0 +1,54 @@ +import * as gh from "./github.mjs"; + +const DECISION_KINDS = ["approve", "request-changes", "comment", "close"]; +const MAX_COMMENT_LENGTH = 60000; +const decisionsInFlight = new Set(); + +export async function applyDecisionOnGitHub({ store, number, kind, comment, refresh, github = gh }) { + if (!DECISION_KINDS.includes(kind)) throw new Error(`Unknown decision "${kind}".`); + const key = `${store.repo}#${number}`; + if (decisionsInFlight.has(key)) throw new Error(`#${number} already has a GitHub action in progress.`); + decisionsInFlight.add(key); + try { + const item = store.getItem(number); + if (!item) throw new Error(`Pull request #${number} is not on the board. Refresh first.`); + if (item.decision) throw new Error(`#${number} was already actioned (${item.decision.kind}).`); + + const body = String(comment ?? "").trim().slice(0, MAX_COMMENT_LENGTH); + if ((kind === "request-changes" || kind === "comment") && !body) { + throw new Error(`A ${kind} decision needs a non-empty comment.`); + } + + let result; + if (kind === "approve") { + result = await github.approveAndEnableAutoMerge(store.repo, number, { body }); + } else if (kind === "close") { + result = await github.closePullRequest(store.repo, number, { body }); + } else { + result = await github.submitPullRequestReview(store.repo, number, { kind, body }); + } + + const warnings = result.warning ? [result.warning] : []; + try { + await store.recordDecision(number, { kind, comment: body, status: result.status }); + } catch (error) { + warnings.push(`GitHub was updated, but the board could not record the action: ${error.message}`); + } + try { + await refresh(store); + } catch (error) { + warnings.push(`GitHub was updated, but the board could not refresh: ${error.message}`); + } + return { + number, + kind, + comment: body, + posted: true, + status: result.status, + warning: warnings.length ? warnings.join(" ") : null, + url: item.url, + }; + } finally { + decisionsInFlight.delete(key); + } +} diff --git a/.github/extensions/open-pr-board/lib/github.mjs b/.github/extensions/open-pr-board/lib/github.mjs index de445256c..72f955b45 100644 --- a/.github/extensions/open-pr-board/lib/github.mjs +++ b/.github/extensions/open-pr-board/lib/github.mjs @@ -147,6 +147,7 @@ function normalizePullRequest(raw) { isDraft: Boolean(raw.isDraft), baseRefName: raw.baseRefName, headRefName: raw.headRefName, + headRefOid: raw.headRefOid, additions: raw.additions ?? 0, deletions: raw.deletions ?? 0, changedFiles: raw.changedFiles ?? 0, @@ -175,6 +176,7 @@ const LIST_FIELDS = [ "labels", "baseRefName", "headRefName", + "headRefOid", "additions", "deletions", "changedFiles", diff --git a/.github/extensions/open-pr-board/lib/review-prompt.mjs b/.github/extensions/open-pr-board/lib/review-prompt.mjs index e36621238..095d0a981 100644 --- a/.github/extensions/open-pr-board/lib/review-prompt.mjs +++ b/.github/extensions/open-pr-board/lib/review-prompt.mjs @@ -8,19 +8,20 @@ export const RECOMMENDATION_GUIDE = [ /** Shared schema for a single recorded review. Reused directly as the `record_review` input schema. */ export const REVIEW_FIELDS_SCHEMA = { type: "object", - required: ["number", "recommendation", "rationale"], + required: ["number", "queueId", "recommendation", "rationale"], properties: { number: { type: "integer", description: "Pull request number." }, queueId: { type: "string", - description: "The queueId handed out when the review was requested. Omitting it skips the staleness check.", + minLength: 1, + description: "The current queueId handed out when the review was requested; must match exactly.", }, recommendation: { type: "string", enum: ["close", "request-changes", "needs-insight", "approve"], description: "Which bucket the pull request belongs in.", }, - rationale: { type: "string", description: "Two or three sentences explaining the recommendation." }, + rationale: { type: "string", description: "One sentence explaining the deciding factor, shown on the card." }, summary: { type: "string", description: "What the pull request actually changes." }, repoFit: { type: "string", description: "Repository fit and meaningful value for Copilot users." }, compliance: { type: "string", description: "Deterministic checks: frontmatter, naming, README regeneration, manifests." }, diff --git a/.github/extensions/open-pr-board/lib/state.mjs b/.github/extensions/open-pr-board/lib/state.mjs index 9543876ec..7a0f3f498 100644 --- a/.github/extensions/open-pr-board/lib/state.mjs +++ b/.github/extensions/open-pr-board/lib/state.mjs @@ -42,6 +42,7 @@ const SYNCED_FIELDS = [ "isDraft", "baseRefName", "headRefName", + "headRefOid", "additions", "deletions", "changedFiles", @@ -85,7 +86,7 @@ export class BoardStore { /** Serialised atomic write so concurrent mutations cannot interleave or truncate the file. */ persist() { - this.writeChain = this.writeChain.then(async () => { + this.writeChain = this.writeChain.catch(() => {}).then(async () => { await mkdir(path.dirname(this.file), { recursive: true }); const tmp = `${this.file}.${randomBytes(6).toString("hex")}.tmp`; await writeFile(tmp, JSON.stringify(this.state, null, 2), "utf8"); @@ -152,6 +153,22 @@ export class BoardStore { manualColumn: null, decision: null, }; + const decision = existing.decision; + if ((decision?.kind === "comment" || decision?.kind === "request-changes") && !decision.headRefOid) { + decision.headRefOid = existing.headRefOid ?? pr.headRefOid ?? null; + } + if ( + (decision?.kind === "comment" || decision?.kind === "request-changes") && + decision.headRefOid && + pr.headRefOid && + decision.headRefOid !== pr.headRefOid + ) { + existing.decision = null; + existing.review = null; + existing.reviewStatus = "none"; + existing.queueId = null; + existing.manualColumn = null; + } for (const field of SYNCED_FIELDS) existing[field] = pr[field]; this.state.items[key] = existing; } @@ -194,7 +211,7 @@ export class BoardStore { recordReviewInternal(payload) { const item = this.getItem(payload.number); if (!item) return { number: payload.number, status: "skipped", reason: "not on the board" }; - if (item.queueId && payload.queueId && payload.queueId !== item.queueId) { + if (!item.queueId || !payload.queueId || payload.queueId !== item.queueId) { return { number: payload.number, status: "skipped", reason: "stale review result" }; } if (!RECOMMENDATIONS.includes(payload.recommendation)) { @@ -248,6 +265,7 @@ export class BoardStore { comment: decision.comment ?? "", at: new Date().toISOString(), status: decision.status ?? null, + headRefOid: item.headRefOid ?? null, }; this.state.history.unshift({ number: item.number, diff --git a/.github/extensions/open-pr-board/public/app.js b/.github/extensions/open-pr-board/public/app.js index a20281e73..b28e2b910 100644 --- a/.github/extensions/open-pr-board/public/app.js +++ b/.github/extensions/open-pr-board/public/app.js @@ -39,6 +39,9 @@ let detailError = null; let activeTab = "review"; let filterText = ""; let pendingConfirm = null; +let detailGeneration = 0; +let drawerOriginNumber = null; +const decisionsInFlight = new Set(); const el = { title: document.getElementById("board-title"), @@ -48,6 +51,7 @@ const el = { reset: document.getElementById("reset-button"), review: document.getElementById("review-button"), board: document.getElementById("board"), + toolbar: document.querySelector(".toolbar"), drawer: document.getElementById("drawer"), kicker: document.getElementById("drawer-kicker"), drawerTitle: document.getElementById("drawer-title"), @@ -483,8 +487,11 @@ function renderDrawerBody() { if (!item) return; for (const tab of el.drawer.querySelectorAll(".tab")) { - tab.setAttribute("aria-selected", String(tab.dataset.tab === activeTab)); + const selected = tab.dataset.tab === activeTab; + tab.setAttribute("aria-selected", String(selected)); + tab.tabIndex = selected ? 0 : -1; } + el.drawerBody.setAttribute("aria-labelledby", `tab-${activeTab}`); if (activeTab === "review") { el.drawerBody.replaceChildren(reviewTab(item)); @@ -534,20 +541,26 @@ function renderDrawerHeader() { el.rereviewToggle.disabled = Boolean(item.decision) || item.reviewStatus === "queued"; for (const button of el.drawer.querySelectorAll("[data-decision]")) { - button.disabled = Boolean(item.decision); + button.disabled = Boolean(item.decision) || decisionsInFlight.has(item.number); button.classList.remove("confirming"); button.textContent = DECISION_BUTTONS[button.dataset.decision]; } + clearTimeout(pendingConfirm?.timer); pendingConfirm = null; } async function loadDetail(number, { force = false } = {}) { + if (selectedNumber !== number) return; + const generation = ++detailGeneration; detail = null; detailError = null; renderDrawerBody(); try { - detail = await api(`/api/pr/${number}`, force ? { method: "POST" } : {}); + const response = await api(`/api/pr/${number}`, force ? { method: "POST" } : {}); + if (generation !== detailGeneration || selectedNumber !== number) return; + detail = response; } catch (error) { + if (generation !== detailGeneration || selectedNumber !== number) return; detailError = error.message; } if (selectedNumber === number) renderDrawerBody(); @@ -559,6 +572,10 @@ function openDrawer(number) { const item = currentItem(); if (!item) return; + if (!el.drawer.classList.contains("open")) drawerOriginNumber = number; + el.drawer.inert = false; + el.board.inert = true; + el.toolbar.inert = true; el.drawer.classList.add("open"); el.drawer.setAttribute("aria-hidden", "false"); el.rereviewPanel.hidden = true; @@ -570,17 +587,26 @@ function openDrawer(number) { renderDrawerHeader(); renderDrawerBody(); renderBoard(); + el.drawerClose.focus(); if (changed) loadDetail(number); } function closeDrawer() { + ++detailGeneration; selectedNumber = null; detail = null; detailError = null; + clearTimeout(pendingConfirm?.timer); pendingConfirm = null; el.drawer.classList.remove("open"); el.drawer.setAttribute("aria-hidden", "true"); + el.drawer.inert = true; + el.board.inert = false; + el.toolbar.inert = false; renderBoard(); + const origin = el.board.querySelector(`[data-number="${drawerOriginNumber}"]`); + (origin ?? el.filter).focus(); + drawerOriginNumber = null; } /* --------------------------------------------------------------- decisions */ @@ -608,15 +634,20 @@ function armConfirm(button, kind) { async function submitDecision(button, kind) { const item = currentItem(); - if (!item) return; + if (!item || decisionsInFlight.has(item.number)) return; if (!armConfirm(button, kind)) return; clearTimeout(pendingConfirm.timer); pendingConfirm = null; button.classList.remove("confirming"); + const comment = el.comment.value; + decisionsInFlight.add(item.number); + renderDrawerHeader(); const result = await run(button, () => - api("/api/decision", { method: "POST", body: { number: item.number, kind, comment: el.comment.value } }), + api("/api/decision", { method: "POST", body: { number: item.number, kind, comment } }), ); + decisionsInFlight.delete(item.number); + renderDrawerHeader(); button.textContent = DECISION_BUTTONS[kind]; if (result) { if (result.warning) toast(result.warning, { error: true, url: result.url }); @@ -667,7 +698,24 @@ el.review.addEventListener("click", (event) => el.drawerClose.addEventListener("click", closeDrawer); document.addEventListener("keydown", (event) => { - if (event.key === "Escape" && el.drawer.classList.contains("open")) closeDrawer(); + if (!el.drawer.classList.contains("open")) return; + if (event.key === "Escape") { + event.preventDefault(); + closeDrawer(); + } else if (event.key === "Tab") { + const controls = [...el.drawer.querySelectorAll( + 'button:not(:disabled), a[href], input:not(:disabled), select:not(:disabled), textarea:not(:disabled), [tabindex="0"]', + )].filter((node) => node.getClientRects().length); + const first = controls[0] ?? el.drawer; + const last = controls.at(-1) ?? el.drawer; + if (event.shiftKey && (document.activeElement === first || document.activeElement === el.drawer)) { + event.preventDefault(); + last.focus(); + } else if (!event.shiftKey && document.activeElement === last) { + event.preventDefault(); + first.focus(); + } + } }); el.drawerColumn.addEventListener("change", async (event) => { @@ -683,12 +731,26 @@ el.drawerColumn.addEventListener("change", async (event) => { } }); -for (const tab of el.drawer.querySelectorAll(".tab")) { +const tabs = [...el.drawer.querySelectorAll(".tab")]; +for (const tab of tabs) { tab.addEventListener("click", () => { activeTab = tab.dataset.tab; renderDrawerBody(); if (!detail && !detailError && selectedNumber !== null) loadDetail(selectedNumber); }); + tab.addEventListener("keydown", (event) => { + const index = tabs.indexOf(tab); + const next = { + ArrowRight: (index + 1) % tabs.length, + ArrowLeft: (index - 1 + tabs.length) % tabs.length, + Home: 0, + End: tabs.length - 1, + }[event.key]; + if (next === undefined) return; + event.preventDefault(); + tabs[next].focus(); + tabs[next].click(); + }); } el.rereviewToggle.addEventListener("click", () => { diff --git a/.github/extensions/open-pr-board/public/index.html b/.github/extensions/open-pr-board/public/index.html index f12fabe87..775ff39fb 100644 --- a/.github/extensions/open-pr-board/public/index.html +++ b/.github/extensions/open-pr-board/public/index.html @@ -26,7 +26,7 @@
-