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 @@
-