diff --git a/.github/extensions/external-plugin-review-board/extension.mjs b/.github/extensions/external-plugin-review-board/extension.mjs index b28cc4da4..68ce27a69 100644 --- a/.github/extensions/external-plugin-review-board/extension.mjs +++ b/.github/extensions/external-plugin-review-board/extension.mjs @@ -70,7 +70,7 @@ async function getIssue(number, { force = false } = {}) { async function startReview({ numbers, instanceId, send }) { const items = store.pendingReview(numbers); - if (!items.length) return { queued: [], message: "Nothing to review — all items already have an AI review." }; + if (!items.length) return { queued: [], message: "Nothing to review — no eligible unreviewed items remain." }; const queued = items.map((item) => item.number); const snapshots = await store.markQueued(queued); const prompt = buildReviewPrompt({ diff --git a/.github/extensions/external-plugin-review-board/lib/state.mjs b/.github/extensions/external-plugin-review-board/lib/state.mjs index f42a99521..d75de61d1 100644 --- a/.github/extensions/external-plugin-review-board/lib/state.mjs +++ b/.github/extensions/external-plugin-review-board/lib/state.mjs @@ -136,7 +136,8 @@ export class BoardStore { } pendingReview(numbers) { - const all = Object.values(this.state.items).filter((item) => !item.decision && item.reviewStatus !== "queued"); + const all = Object.values(this.state.items).filter((item) => + !item.decision && !item.pendingDecision && item.reviewStatus !== "queued"); if (numbers?.length) { const wanted = new Set(numbers.map(Number)); return all.filter((item) => wanted.has(item.number)); diff --git a/.github/extensions/external-plugin-review-board/public/app.js b/.github/extensions/external-plugin-review-board/public/app.js index 797c5fe96..d06bfc8fa 100644 --- a/.github/extensions/external-plugin-review-board/public/app.js +++ b/.github/extensions/external-plugin-review-board/public/app.js @@ -561,6 +561,11 @@ function resetConfirm() { ui.approve.disabled = false; ui.reject.disabled = false; updatePreview(); + const item = findItem(selected); + if (bulkBusy || item?.pendingDecision) { + ui.approve.disabled = true; + ui.reject.disabled = true; + } } async function decide(kind) { diff --git a/eng/external-plugin-review-board.test.mjs b/eng/external-plugin-review-board.test.mjs new file mode 100644 index 000000000..e0f5a8222 --- /dev/null +++ b/eng/external-plugin-review-board.test.mjs @@ -0,0 +1,50 @@ +import assert from "node:assert/strict"; +import { readFile } from "node:fs/promises"; +import vm from "node:vm"; +import { test } from "node:test"; +import { BoardStore } from "../.github/extensions/external-plugin-review-board/lib/state.mjs"; + +test("external-plugin-review-board excludes items with pending decisions from AI review", () => { + const store = new BoardStore("unused", "github/awesome-copilot"); + store.state.items = { + "1": { number: 1, reviewStatus: "unreviewed" }, + "2": { number: 2, reviewStatus: "unreviewed", pendingDecision: { kind: "approve" } }, + "3": { number: 3, reviewStatus: "queued" }, + "4": { number: 4, reviewStatus: "unreviewed", decision: { kind: "reject" } }, + }; + + assert.deepEqual(store.pendingReview().map(({ number }) => number), [1]); + assert.deepEqual(store.pendingReview([1, 2, 3, 4]).map(({ number }) => number), [1]); +}); + +test("resetConfirm keeps decision buttons disabled while a decision is pending", async () => { + const source = await readFile( + new URL("../.github/extensions/external-plugin-review-board/public/app.js", import.meta.url), + "utf8", + ); + const start = source.indexOf("function resetConfirm()"); + const end = source.indexOf("async function decide(", start); + assert.notEqual(start, -1); + assert.notEqual(end, -1); + + const button = () => ({ + disabled: false, + textContent: "", + classList: { remove() {} }, + }); + const context = vm.createContext({ + clearTimeout() {}, + pendingConfirm: { timer: null, kind: "approve" }, + ui: { approve: button(), reject: button() }, + bulkBusy: false, + selected: 42, + findItem: () => ({ pendingDecision: { kind: "approve" } }), + updatePreview() {}, + }); + vm.runInContext(source.slice(start, end), context); + + context.resetConfirm(); + + assert.equal(context.ui.approve.disabled, true); + assert.equal(context.ui.reject.disabled, true); +}); diff --git a/eng/open-pr-board.test.mjs b/eng/open-pr-board.test.mjs index b6d5e4f8e..151b3ec50 100644 --- a/eng/open-pr-board.test.mjs +++ b/eng/open-pr-board.test.mjs @@ -6,7 +6,7 @@ import vm from "node:vm"; import { test } from "node:test"; import { BoardStore } from "../.github/extensions/open-pr-board/lib/state.mjs"; import { applyDecisionOnGitHub } from "../.github/extensions/open-pr-board/lib/decisions.mjs"; -import { approveAndEnableAutoMerge } from "../.github/extensions/open-pr-board/lib/github.mjs"; +import { approveAndEnableAutoMerge, listOpenPullRequests } from "../.github/extensions/open-pr-board/lib/github.mjs"; import { REVIEW_FIELDS_SCHEMA } from "../.github/extensions/open-pr-board/lib/review-prompt.mjs"; const pr = (headRefOid = "first") => ({ @@ -59,6 +59,50 @@ test("approval and auto-merge are pinned to the reviewed commit", async () => { assert.deepEqual(calls[2].args, ["pr", "merge", "1", "-R", "example/repo", "--auto", "--merge", "--match-head-commit", headRefOid]); }); +test("open pull requests fetch checks with bounded concurrency, preserve order, and tolerate a failed check request", async () => { + const numbers = [41, 7, 23, 4, 99, 12, 68, 51]; + let active = 0; + let peak = 0; + const requested = []; + const runner = async (args) => { + if (args[1] === "list") { + return JSON.stringify(numbers.map((number) => ({ number, title: `PR ${number}`, labels: [] }))); + } + if (args[1] === "view") { + const number = Number(args[2]); + requested.push(number); + active++; + peak = Math.max(peak, active); + try { + await new Promise((resolve) => setTimeout(resolve, 10)); + if (number === 23) throw new Error("check request failed"); + return JSON.stringify({ + statusCheckRollup: [{ name: `check-${number}`, status: "COMPLETED", conclusion: "SUCCESS" }], + }); + } finally { + active--; + } + } + throw new Error(`Unexpected gh command: ${args.join(" ")}`); + }; + + const pullRequests = await listOpenPullRequests("example/repo", runner); + + assert.ok(peak <= 6, "check requests should not exceed the six-request limit"); + assert.ok(peak > 1, "check requests should run concurrently"); + assert.deepEqual(pullRequests.map(({ number }) => number), numbers, "results should preserve list order"); + assert.deepEqual(requested.sort((a, b) => a - b), [...numbers].sort((a, b) => a - b), "every PR should request checks"); + for (const pullRequest of pullRequests) { + if (pullRequest.number === 23) { + assert.deepEqual(pullRequest.checks, []); + assert.equal(pullRequest.checksState, "none"); + } else { + assert.deepEqual(pullRequest.checks.map(({ name }) => name), [`check-${pullRequest.number}`]); + assert.equal(pullRequest.checksState, "success"); + } + } +}); + test("approval refuses a missing SHA, changed head, or closed pull request without writing", async () => { let calls = 0; const runner = async () => { calls++; throw new Error("must not run"); };