diff --git a/.github/extensions/external-plugin-review-board/README.md b/.github/extensions/external-plugin-review-board/README.md index 728c1fd287..bf4aab5503 100644 --- a/.github/extensions/external-plugin-review-board/README.md +++ b/.github/extensions/external-plugin-review-board/README.md @@ -22,6 +22,11 @@ Open it by asking Copilot to "open the plugin review board" (canvas id `external `requires-submitter-fixes` states. Pending intake cards stay on the board so their AI reviews, manual buckets, queued re-reviews, and fast-track dismissal state are preserved. Approval and first-party fast-track are unavailable until intake is ready. + The same read-only refresh runs automatically every 15 minutes while the canvas is open; + concurrent fetches are shared and closing the canvas stops its timer. Selection, staged feedback, + reviews, queue tokens, and local decisions remain intact for submissions still on the board. + Polling never starts AI reviews or writes to GitHub. Failures show an error without clearing + the board; the next scheduled or manual refresh can retry. - **Blocked intake status** keeps submissions visible in their existing buckets with a *Blocked by intake* badge and a reason (submitter fixes required, automated intake pending, or missing `ready-for-review`). The toolbar shows how many submissions can actually be queued and @@ -37,6 +42,10 @@ Open it by asking Copilot to "open the plugin review board" (canvas id `external Fast-track never overrides an existing review or a bucket you chose; drag a card to *Unreviewed* to opt it out and have it reviewed normally. Cards show *First party* / *Third party* pills from the intake labels. - **Details panel** shows the AI review, the rendered issue body, and all comments. + Successful refreshes invalidate cached issue details and reload the selected body/comments + tab without changing staged feedback. + When detail requests overlap, only the latest request for each issue can update its + cache, so an older poll cannot overwrite comments loaded after posting feedback. Clicking outside the panel or successfully submitting an item action closes it; 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 @@ -78,6 +87,8 @@ not imported; boards start fresh. ## Regression tests +Run `node --experimental-vm-modules --test eng/board-periodic-refresh.test.mjs` to check +automatic refresh cadence, concurrent/manual refreshes, state preservation, errors, and disposal. Run `node --test eng/board-column-order.test.mjs` to check oldest-first column ordering. Run `node --experimental-vm-modules --test eng/board-state-storage.test.mjs` from the repository root to check both canvases' session-scoped persistence without accessing GitHub. diff --git a/.github/extensions/external-plugin-review-board/extension.mjs b/.github/extensions/external-plugin-review-board/extension.mjs index 2e3d092472..b276ae678a 100644 --- a/.github/extensions/external-plugin-review-board/extension.mjs +++ b/.github/extensions/external-plugin-review-board/extension.mjs @@ -81,10 +81,16 @@ async function refresh() { async function getIssue(number, { force = false } = {}) { const cached = detailCache.get(number); - if (!force && cached && Date.now() - cached.at < DETAIL_TTL_MS) return cached.data; - const data = await gh.getIssueDetail(store.state.repo, number); - detailCache.set(number, { at: Date.now(), data }); - return data; + if (!force && cached?.data && Date.now() - cached.at < DETAIL_TTL_MS) return cached.data; + const request = {}; + detailCache.set(number, request); + try { + const data = await gh.getIssueDetail(store.state.repo, number); + if (detailCache.get(number) === request) detailCache.set(number, { at: Date.now(), data }); + return data; + } finally { + if (detailCache.get(number) === request) detailCache.delete(number); + } } async function startReview({ numbers, instanceId, send }) { diff --git a/.github/extensions/external-plugin-review-board/lib/server.mjs b/.github/extensions/external-plugin-review-board/lib/server.mjs index bfe3675548..4e3f56e1f8 100644 --- a/.github/extensions/external-plugin-review-board/lib/server.mjs +++ b/.github/extensions/external-plugin-review-board/lib/server.mjs @@ -24,6 +24,7 @@ const CSP = [ ].join("; "); const MAX_BODY = 64 * 1024; +const REFRESH_INTERVAL_MS = 15 * 60 * 1000; export function expectedError(message, status) { return Object.assign(new Error(message), { publicMessage: message, status }); @@ -168,9 +169,26 @@ export async function startBoardServer({ publicDir, api }) { await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); port = server.address().port; + let refreshing = false; + const refreshTimer = setInterval(async () => { + if (refreshing) return; + refreshing = true; + try { + await api.routes["POST /api/refresh"]({}); + } catch (error) { + if (!Number.isInteger(error?.status) || error.status >= 500) reportError(api, error); + const frame = `event: refresh-error\ndata: ${JSON.stringify({ error: clientErrorMessage(error) })}\n\n`; + for (const client of sseClients) client.write(frame); + } finally { + refreshing = false; + } + }, REFRESH_INTERVAL_MS); + refreshTimer.unref(); + return { url: `http://127.0.0.1:${port}/`, async close() { + clearInterval(refreshTimer); unsubscribe(); for (const client of sseClients) client.end(); sseClients.clear(); diff --git a/.github/extensions/external-plugin-review-board/public/app.js b/.github/extensions/external-plugin-review-board/public/app.js index e8d4e9c670..6339e0edfb 100644 --- a/.github/extensions/external-plugin-review-board/public/app.js +++ b/.github/extensions/external-plugin-review-board/public/app.js @@ -44,6 +44,8 @@ let filter = ""; let selected = null; let activeTab = "review"; const details = new Map(); +let detailGeneration = 0; +const detailRequests = new Map(); const pendingConfirm = { kind: null, timer: null }; const stagedComments = new Map(); let bulkBusy = false; @@ -435,7 +437,7 @@ function renderDrawer() { for (const tab of ui.tabs) { tab.setAttribute("aria-selected", String(tab.dataset.tab === activeTab)); if (tab.dataset.tab === "comments") { - const count = details.get(item.number)?.comments.length; + const count = details.get(item.number)?.comments?.length; tab.textContent = count != null ? `Comments (${count})` : "Comments"; } } @@ -571,13 +573,20 @@ function renderComment(comment) { async function loadDetail(number, { force = false } = {}) { if (!force && details.has(number) && !details.get(number).error) return; + const generation = detailGeneration; + const request = {}; + detailRequests.set(number, request); details.delete(number); if (selected === number) renderDrawer(); try { const data = await api(force ? "POST" : "GET", `/api/issue/${number}`); + if (generation !== detailGeneration || detailRequests.get(number) !== request) return; details.set(number, data); } catch (error) { + if (generation !== detailGeneration || detailRequests.get(number) !== request) return; details.set(number, { error: error.message }); + } finally { + if (detailRequests.get(number) === request) detailRequests.delete(number); } if (selected === number) renderDrawer(); } @@ -773,6 +782,7 @@ ui.refresh.addEventListener("click", () => withBusy(ui.refresh, "Refreshing…", async () => { try { const result = await api("POST", "/api/refresh"); + ++detailGeneration; details.clear(); const parts = [`${result.total} open`]; if (result.added.length) parts.push(`${result.added.length} new`); @@ -883,10 +893,18 @@ document.addEventListener("keydown", (event) => { function connect() { const events = new EventSource(`/events?token=${encodeURIComponent(token)}`); events.addEventListener("board", (event) => { - board = JSON.parse(event.data); + const snapshot = JSON.parse(event.data); + const refreshed = board?.lastRefreshedAt !== snapshot.lastRefreshedAt; + board = snapshot; + if (refreshed) { + ++detailGeneration; + details.clear(); + } renderBoard(); if (selected != null) renderDrawer(); + if (refreshed && selected != null && activeTab !== "review") loadDetail(selected, { force: true }); }); + events.addEventListener("refresh-error", (event) => toast(JSON.parse(event.data).error, { error: true })); events.addEventListener("error", () => { ui.meta.textContent = "Reconnecting…"; }); diff --git a/.github/extensions/open-pr-board/README.md b/.github/extensions/open-pr-board/README.md index 8fc740b6a6..3d7a4fc0be 100644 --- a/.github/extensions/open-pr-board/README.md +++ b/.github/extensions/open-pr-board/README.md @@ -8,6 +8,12 @@ authenticated `gh` CLI. ## What it does +- **Automatic refresh** — fetches open PRs every 15 minutes while the canvas is open, + using the same read-only path as manual Refresh. Concurrent fetches are shared; + closing the canvas stops its timer. Existing reviews, queue tokens, manual buckets, + decisions, selection, and staged feedback are retained for PRs still on the board. + Refresh does not start AI reviews or write to GitHub. Failures show an error without + clearing the board, and the next scheduled or manual refresh can retry. - **Buckets** — `Unreviewed`, `Reviewing`, `Close`, `Request changes`, `Needs insight`, `Approve`, `Actioned`. The AI recommendation picks the bucket; dragging a card overrides it. Cards in each bucket are sorted oldest to newest @@ -40,8 +46,21 @@ authenticated `gh` CLI. - **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. Approval is pinned to the board's commit - SHA, and auto-merge requires that same head commit. If the head changes, refresh - and review the new commit before approving again. + SHA displayed by the client, and auto-merge requires that same head commit. + A changed head automatically reloads an open preview; approval is unavailable until + that preview has loaded. Bulk approval skips stale AI reviews. If the head changes, + review the new commit before approving again. + Queued reviews and manual approval moves retain their original commit SHA, so a + refresh cannot carry those approval choices onto newer commits. Legacy reviews or + approval moves without a recorded SHA must be reviewed or explicitly moved again. + Moves carry the client's displayed SHA even if polling updates the board before the + request arrives. Delayed feedback actions retain their original SHA so new commits + still return to Unreviewed. A successful refresh retries a failed open preview. + Feedback completed for an older head is recorded only in history, preserving any + review queue, completed review, or manual bucket already created for the newer head. + Any retained review, queue, or manual bucket belonging to the older head is cleared. + Feedback requests also carry the displayed SHA and are rejected if polling has + already discovered a newer head before dispatch. 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. @@ -72,7 +91,7 @@ touching GitHub. | Action | Purpose | | ---------------- | ----------------------------------------------------------------------- | -| `get_board` | Current buckets, counts, and per-PR state. | +| `get_board` | Current buckets, counts, per-PR commit SHA, and approval staleness. | | `refresh` | Re-fetch open pull requests from GitHub. | | `start_review` | Queue a review pass and return the review instructions. | | `start_rereview` | Queue a re-review of one PR with optional guidance. | @@ -94,6 +113,8 @@ Results arriving after a manual move, reset, or replacement review are rejected. ## Regression tests +Run `node --experimental-vm-modules --test eng/board-periodic-refresh.test.mjs` to check +automatic refresh cadence, concurrent/manual refreshes, state preservation, errors, and disposal. Run `node --test eng/board-column-order.test.mjs` to check oldest-first column ordering. 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 fd717c7e3a..5a939fe659 100644 --- a/.github/extensions/open-pr-board/extension.mjs +++ b/.github/extensions/open-pr-board/extension.mjs @@ -97,10 +97,16 @@ async function resetBoard(store) { async function getPullRequest(store, number, { force = false } = {}) { const key = cacheKey(store.repo, number); const cached = detailCache.get(key); - if (!force && cached && Date.now() - cached.at < DETAIL_TTL_MS) return cached.data; - const data = await gh.getPullRequestDetail(store.repo, number); - detailCache.set(key, { at: Date.now(), data }); - return data; + if (!force && cached?.data && Date.now() - cached.at < DETAIL_TTL_MS) return cached.data; + const request = {}; + detailCache.set(key, request); + try { + const data = await gh.getPullRequestDetail(store.repo, number); + if (detailCache.get(key) === request) detailCache.set(key, { at: Date.now(), data }); + return data; + } finally { + if (detailCache.get(key) === request) detailCache.delete(key); + } } function requireItem(store, number) { @@ -182,6 +188,7 @@ function boardSummary(store) { counts: snapshot.counts, items: snapshot.items.map((item) => ({ number: item.number, + headRefOid: item.headRefOid, title: item.title, author: item.author, url: item.url, @@ -193,6 +200,7 @@ function boardSummary(store) { aiRecommendation: item.review?.recommendation ?? null, aiRationale: item.review?.rationale ?? null, reviewStale: item.reviewStale, + approvalStale: item.approvalStale, reviewStatus: item.reviewStatus, manualOverride: item.manualColumn ?? null, suggestedComment: item.review?.suggestedComment ?? null, @@ -211,11 +219,14 @@ function routesFor(instanceId, store) { "POST /api/review": ({ body }) => startReview({ store, numbers: body.numbers, instanceId, send: true }), "POST /api/rereview": ({ body }) => startRereview({ store, number: body.number, guidance: body.guidance, instanceId, send: true }), - "POST /api/move": ({ body }) => store.moveItem(Number(body.number), String(body.column)), + "POST /api/move": ({ body }) => store.moveItem(Number(body.number), String(body.column), body.headRefOid), "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, refresh }), + applyDecisionOnGitHub({ + store, number: Number(body.number), kind: body.kind, comment: body.comment, + headRefOid: body.headRefOid, 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. @@ -343,10 +354,11 @@ session = await joinSession({ repo: repoProperty, number: { type: "integer" }, column: { type: "string", enum: ["unreviewed", ...RECOMMENDATIONS] }, + headRefOid: { type: "string", description: "Commit SHA displayed when choosing the bucket; required for a current manual approval choice." }, }, }, handler: wrap(async (ctx) => - (await storeForContext(ctx)).moveItem(ctx.input.number, ctx.input.column), + (await storeForContext(ctx)).moveItem(ctx.input.number, ctx.input.column, ctx.input.headRefOid), ), }, { diff --git a/.github/extensions/open-pr-board/lib/decisions.mjs b/.github/extensions/open-pr-board/lib/decisions.mjs index 6bbae16fc5..2d610a62a3 100644 --- a/.github/extensions/open-pr-board/lib/decisions.mjs +++ b/.github/extensions/open-pr-board/lib/decisions.mjs @@ -4,7 +4,7 @@ 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 }) { +export async function applyDecisionOnGitHub({ store, number, kind, comment, headRefOid, 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.`); @@ -13,6 +13,11 @@ export async function applyDecisionOnGitHub({ store, number, kind, comment, refr 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 actionHeadRefOid = item.headRefOid ?? null; + if ((kind === "comment" || kind === "request-changes") && + (!headRefOid || headRefOid !== actionHeadRefOid)) { + throw new Error("PR changed since it was displayed; review the current commit before submitting feedback."); + } const body = String(comment ?? "").trim().slice(0, MAX_COMMENT_LENGTH); if ((kind === "request-changes" || kind === "comment") && !body) { @@ -21,7 +26,10 @@ export async function applyDecisionOnGitHub({ store, number, kind, comment, refr let result; if (kind === "approve") { - result = await github.approveAndEnableAutoMerge(store.repo, number, { body, headRefOid: item.headRefOid }); + if (!headRefOid || headRefOid !== item.headRefOid) { + throw new Error("PR changed since it was displayed; refresh and review it again before approving."); + } + result = await github.approveAndEnableAutoMerge(store.repo, number, { body, headRefOid }); } else if (kind === "close") { result = await github.closePullRequest(store.repo, number, { body }); } else { @@ -30,7 +38,7 @@ export async function applyDecisionOnGitHub({ store, number, kind, comment, refr const warnings = result.warning ? [result.warning] : []; try { - await store.recordDecision(number, { kind, comment: body, status: result.status }); + await store.recordDecision(number, { kind, comment: body, status: result.status, headRefOid: actionHeadRefOid }); } catch (error) { warnings.push(`GitHub was updated, but the board could not record the action: ${error.message}`); } diff --git a/.github/extensions/open-pr-board/lib/github.mjs b/.github/extensions/open-pr-board/lib/github.mjs index 6f1f9d5644..241b0493d7 100644 --- a/.github/extensions/open-pr-board/lib/github.mjs +++ b/.github/extensions/open-pr-board/lib/github.mjs @@ -242,6 +242,7 @@ export async function getPullRequestDetail(repo, number) { number: pr.number, title: pr.title, state: pr.state, + headRefOid: pr.head?.sha, merged: Boolean(pr.merged), mergeableState: pr.mergeable_state ?? null, url: pr.html_url, diff --git a/.github/extensions/open-pr-board/lib/server.mjs b/.github/extensions/open-pr-board/lib/server.mjs index b90b697fa0..c471d61395 100644 --- a/.github/extensions/open-pr-board/lib/server.mjs +++ b/.github/extensions/open-pr-board/lib/server.mjs @@ -24,6 +24,7 @@ const CSP = [ ].join("; "); const MAX_BODY = 64 * 1024; +const REFRESH_INTERVAL_MS = 15 * 60 * 1000; export function expectedError(message, status) { return Object.assign(new Error(message), { publicMessage: message, status }); @@ -168,9 +169,26 @@ export async function startBoardServer({ publicDir, api }) { await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); port = server.address().port; + let refreshing = false; + const refreshTimer = setInterval(async () => { + if (refreshing || typeof api.routes["POST /api/refresh"] !== "function") return; + refreshing = true; + try { + await api.routes["POST /api/refresh"]({}); + } catch (error) { + if (!Number.isInteger(error?.status) || error.status >= 500) reportError(api, error); + const frame = `event: refresh-error\ndata: ${JSON.stringify({ error: clientErrorMessage(error) })}\n\n`; + for (const client of sseClients) client.write(frame); + } finally { + refreshing = false; + } + }, REFRESH_INTERVAL_MS); + refreshTimer.unref(); + return { url: `http://127.0.0.1:${port}/`, async close() { + clearInterval(refreshTimer); unsubscribe(); for (const client of sseClients) client.end(); sseClients.clear(); diff --git a/.github/extensions/open-pr-board/lib/state.mjs b/.github/extensions/open-pr-board/lib/state.mjs index 7a0f3f4983..a79f3c5260 100644 --- a/.github/extensions/open-pr-board/lib/state.mjs +++ b/.github/extensions/open-pr-board/lib/state.mjs @@ -29,6 +29,7 @@ export function effectiveColumn(item) { /** A review is stale when the pull request changed after the review was recorded. */ function reviewIsStale(item) { + if (item.review && (!item.review.headRefOid || item.review.headRefOid !== item.headRefOid)) return true; if (!item.review?.reviewedAt || !item.updatedAt) return false; return new Date(item.updatedAt).getTime() > new Date(item.review.reviewedAt).getTime() + 1000; } @@ -118,6 +119,8 @@ export class BoardStore { ...item, column: effectiveColumn(item), reviewStale: reviewIsStale(item), + approvalStale: item.manualColumn === "approve" && + (!item.manualHeadRefOid || item.manualHeadRefOid !== item.headRefOid), })) .sort((a, b) => b.number - a.number); const counts = {}; @@ -189,9 +192,13 @@ export class BoardStore { for (const number of numbers) { const item = this.getItem(number); if (!item) continue; - previous.push({ number: item.number, reviewStatus: item.reviewStatus, queueId: item.queueId }); + previous.push({ + number: item.number, reviewStatus: item.reviewStatus, queueId: item.queueId, + queueHeadRefOid: item.queueHeadRefOid, + }); item.reviewStatus = "queued"; item.queueId = randomBytes(8).toString("hex"); + item.queueHeadRefOid = item.headRefOid ?? null; queued.push({ number: item.number, queueId: item.queueId }); } await this.commit(); @@ -204,6 +211,7 @@ export class BoardStore { if (!item) continue; item.reviewStatus = entry.reviewStatus; item.queueId = entry.queueId; + item.queueHeadRefOid = entry.queueHeadRefOid; } return this.commit(); } @@ -221,6 +229,7 @@ export class BoardStore { item.review = { recommendation, ...fields, + headRefOid: item.queueHeadRefOid ?? null, reviewedAt: new Date().toISOString(), }; item.reviewStatus = "reviewed"; @@ -236,7 +245,7 @@ export class BoardStore { return results; } - async moveItem(number, column) { + async moveItem(number, column, headRefOid = null) { const item = this.getItem(number); if (!item) throw new Error(`Pull request #${number} is not on the board.`); if (!DROPPABLE.has(column)) throw new Error(`Column "${column}" cannot be set directly.`); @@ -249,6 +258,7 @@ export class BoardStore { item.manualColumn = null; } else { item.manualColumn = column; + item.manualHeadRefOid = headRefOid; if (item.reviewStatus === "queued") { item.reviewStatus = item.review ? "reviewed" : "none"; item.queueId = null; @@ -260,12 +270,12 @@ export class BoardStore { async recordDecision(number, decision) { const item = this.getItem(number); if (!item) throw new Error(`Pull request #${number} is not on the board.`); - item.decision = { + const recorded = { kind: decision.kind, comment: decision.comment ?? "", at: new Date().toISOString(), status: decision.status ?? null, - headRefOid: item.headRefOid ?? null, + headRefOid: decision.headRefOid === undefined ? item.headRefOid ?? null : decision.headRefOid, }; this.state.history.unshift({ number: item.number, @@ -273,9 +283,29 @@ export class BoardStore { url: item.url, kind: decision.kind, comment: decision.comment ?? "", - at: item.decision.at, + at: recorded.at, status: decision.status ?? null, }); + const obsoleteFeedback = (recorded.kind === "comment" || recorded.kind === "request-changes") && + recorded.headRefOid && item.headRefOid && recorded.headRefOid !== item.headRefOid; + if (!obsoleteFeedback) { + item.decision = recorded; + } else { + if (item.review && item.review.headRefOid !== item.headRefOid) item.review = null; + if (item.manualColumn && item.manualHeadRefOid !== item.headRefOid) { + item.manualColumn = null; + item.manualHeadRefOid = null; + } + if (item.queueId && item.queueHeadRefOid !== item.headRefOid) { + item.queueId = null; + item.queueHeadRefOid = null; + } + if (item.reviewStatus === "queued" && !item.queueId) { + item.reviewStatus = item.review ? "reviewed" : "none"; + } else if (item.reviewStatus === "reviewed" && !item.review) { + item.reviewStatus = "none"; + } + } this.state.history = this.state.history.slice(0, HISTORY_LIMIT); return this.commit(); } diff --git a/.github/extensions/open-pr-board/public/app.js b/.github/extensions/open-pr-board/public/app.js index 1ae8cd60ca..bf068d031b 100644 --- a/.github/extensions/open-pr-board/public/app.js +++ b/.github/extensions/open-pr-board/public/app.js @@ -36,6 +36,7 @@ let board = null; let selectedNumber = null; let detail = null; let detailError = null; +let previewHeadRefOid = null; let activeTab = "review"; let filterText = ""; let pendingConfirm = null; @@ -255,6 +256,7 @@ function renderCard(item) { }, ondragstart: (event) => { event.dataTransfer.setData("text/plain", String(item.number)); + event.dataTransfer.setData("application/x-pr-head", item.headRefOid ?? ""); event.dataTransfer.effectAllowed = "move"; card.classList.add("dragging"); }, @@ -302,7 +304,9 @@ function renderBoard() { const number = Number(event.dataTransfer.getData("text/plain")); if (!Number.isInteger(number)) return; try { - await api("/api/move", { method: "POST", body: { number, column: column.id } }); + await api("/api/move", { method: "POST", body: { + number, column: column.id, headRefOid: event.dataTransfer.getData("application/x-pr-head") || null, + } }); } catch (error) { toast(error.message, { error: true }); } @@ -399,9 +403,15 @@ async function bulkAction(columnId, kind) { if (!current || current.headRefOid !== item.headRefOid) { throw new Error("PR moved, changed, or was already actioned; review it again."); } + if (kind === "approve" && (current.manualColumn === "approve" ? current.approvalStale : current.reviewStale)) { + throw new Error("PR changed since its AI review or manual approval choice; review the current commit before approving."); + } decisionsInFlight.add(item.number); const result = await api("/api/decision", { - method: "POST", body: { number: item.number, kind, comment: item.comment }, + method: "POST", body: { + number: item.number, kind, comment: item.comment, + ...(kind !== "close" ? { headRefOid: item.headRefOid } : {}), + }, }); completed++; if (selectedNumber === item.number) closeDrawer(); @@ -638,13 +648,19 @@ function renderDrawerHeader() { async function loadDetail(number, { force = false } = {}) { if (selectedNumber !== number) return; const generation = ++detailGeneration; + const headRefOid = currentItem()?.headRefOid; + previewHeadRefOid = null; detail = null; detailError = null; renderDrawerBody(); try { const response = await api(`/api/pr/${number}`, force ? { method: "POST" } : {}); if (generation !== detailGeneration || selectedNumber !== number) return; + if (response.headRefOid && response.headRefOid !== headRefOid) { + throw new Error("PR changed while loading its preview; refresh and review the current commit."); + } detail = response; + previewHeadRefOid = response.headRefOid ?? null; } catch (error) { if (generation !== detailGeneration || selectedNumber !== number) return; detailError = error.message; @@ -669,6 +685,7 @@ function openDrawer(number) { el.comment.value = stagedComment(item); detail = null; detailError = null; + previewHeadRefOid = null; } renderDrawerHeader(); renderDrawerBody(); @@ -682,6 +699,7 @@ function closeDrawer() { selectedNumber = null; detail = null; detailError = null; + previewHeadRefOid = null; clearTimeout(pendingConfirm?.timer); pendingConfirm = null; el.drawer.classList.remove("open"); @@ -721,6 +739,10 @@ function armConfirm(button, kind) { async function submitDecision(button, kind) { const item = currentItem(); if (!item || bulkBusy || decisionsInFlight.has(item.number)) return; + if (kind === "approve" && (!previewHeadRefOid || previewHeadRefOid !== item.headRefOid)) { + toast("PR preview is loading or changed; review the current commit before approving.", { error: true }); + return; + } if (!armConfirm(button, kind)) return; clearTimeout(pendingConfirm.timer); pendingConfirm = null; @@ -730,7 +752,10 @@ async function submitDecision(button, kind) { decisionsInFlight.add(item.number); renderDrawerHeader(); const result = await run(button, () => - api("/api/decision", { method: "POST", body: { number: item.number, kind, comment } }), + api("/api/decision", { method: "POST", body: { + number: item.number, kind, comment, + ...(kind !== "close" ? { headRefOid: kind === "approve" ? previewHeadRefOid : item.headRefOid } : {}), + } }), ); decisionsInFlight.delete(item.number); if (selectedNumber === item.number) renderDrawerHeader(); @@ -814,7 +839,7 @@ el.drawerColumn.addEventListener("change", async (event) => { const column = event.target.value; if (column === item.column) return; try { - await api("/api/move", { method: "POST", body: { number: item.number, column } }); + await api("/api/move", { method: "POST", body: { number: item.number, column, headRefOid: item.headRefOid } }); if (selectedNumber === item.number) closeDrawer(); } catch (error) { toast(error.message, { error: true }); @@ -884,19 +909,25 @@ el.comment.addEventListener("input", () => { }); function applySnapshot(snapshot) { + const previousHead = currentItem()?.headRefOid; + const refreshed = board?.updatedAt !== snapshot.updatedAt; board = snapshot; renderBoard(); if (selectedNumber !== null) { if (!currentItem()) closeDrawer(); else { renderDrawerHeader(); - renderDrawerBody(); + if (previousHead !== currentItem().headRefOid || (refreshed && detailError)) { + loadDetail(selectedNumber, { force: true }); + } + else renderDrawerBody(); } } } const events = new EventSource(`/events?token=${encodeURIComponent(TOKEN)}`); events.addEventListener("board", (event) => applySnapshot(JSON.parse(event.data))); +events.addEventListener("refresh-error", (event) => toast(JSON.parse(event.data).error, { error: true })); events.addEventListener("error", () => { el.summary.textContent = "Connection lost — reopen the canvas to reconnect."; }); diff --git a/eng/board-bulk-actions.test.mjs b/eng/board-bulk-actions.test.mjs index 195b911da5..5afff10d0e 100644 --- a/eng/board-bulk-actions.test.mjs +++ b/eng/board-bulk-actions.test.mjs @@ -60,8 +60,8 @@ for (const boardName of ["open-pr-board", "external-plugin-review-board"]) { vm.runInContext('stagedComments.set(1, "Edited feedback"); filterText = filter = "no matches";', ui.context); await ui.context.bulkAction("test", reject); assert.deepEqual(ui.calls.map(args => ({ ...payload(args) })), [ - { number: 1, kind: reject, comment: "Edited feedback" }, - { number: 2, kind: reject, comment: "Feedback 2" }, + { number: 1, kind: reject, comment: "Edited feedback", ...(isPR ? { headRefOid: "first" } : {}) }, + { number: 2, kind: reject, comment: "Feedback 2", ...(isPR ? { headRefOid: "first" } : {}) }, ]); assert.match(ui.confirmations[0], /Edited feedback/); assert.match(ui.confirmations[0], /hidden search results/); @@ -171,7 +171,20 @@ test("PR changed during a batch is skipped, not approved at a new commit", async const ui = await createUI("open-pr-board", items, { request: async () => { items[1].headRefOid = "second"; return {}; }, }); + await ui.context.bulkAction("approve", "approve"); assert.equal(ui.calls.length, 1); assert.match(ui.messages.at(-1).text, /#2: PR moved, changed/); }); + +test("bulk approvals send the displayed commit and refuse stale AI reviews", async () => { + const ui = await createUI("open-pr-board", [ + { number: 1, column: "approve", headRefOid: "displayed" }, + { number: 2, column: "approve", headRefOid: "new", reviewStale: true }, + { number: 3, column: "approve", headRefOid: "new", manualColumn: "approve", approvalStale: true }, + ]); + await ui.context.bulkAction("approve", "approve"); + assert.equal(ui.calls.length, 1); + assert.equal(ui.calls[0][1].body.headRefOid, "displayed"); + assert.match(ui.messages.at(-1).text, /changed since its AI review/); +}); diff --git a/eng/board-periodic-refresh.test.mjs b/eng/board-periodic-refresh.test.mjs new file mode 100644 index 0000000000..fd4d662704 --- /dev/null +++ b/eng/board-periodic-refresh.test.mjs @@ -0,0 +1,269 @@ +import assert from "node:assert/strict"; +import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { setImmediate } from "node:timers/promises"; +import vm from "node:vm"; +import { test } from "node:test"; +import { startBoardServer } from "../.github/extensions/open-pr-board/lib/server.mjs"; + +const INTERVAL = 15 * 60 * 1000; +const repo = "github/awesome-copilot"; + +test("shared server consumers without a refresh route never poll or report refresh errors", async (t) => { + t.mock.timers.enable({ apis: ["setInterval"] }); + const errors = []; + let unsubscribed = false; + const server = await startBoardServer({ + api: { + snapshot: () => ({}), routes: {}, + subscribe: () => () => { unsubscribed = true; }, + logger: { error: (...args) => errors.push(args) }, + }, + }); + t.after(() => server.close()); + t.mock.timers.tick(INTERVAL * 3); + await setImmediate(); + assert.deepEqual(errors, []); + assert.equal(unsubscribed, false); +}); + +function item(number) { + return { + number, title: `Example ${number}`, author: "contributor", + url: `https://github.com/${repo}/issues/${number}`, + labels: ["external-plugin", "ready-for-review"], + headRefOid: "first", updatedAt: "2026-10-01T00:00:00Z", + }; +} + +async function loadBoard(t, id, { getIssueDetail, getPullRequestDetail } = {}) { + const workspacePath = await mkdtemp(path.join(os.tmpdir(), "board-polling-")); + t.after(() => rm(workspacePath, { recursive: true, force: true })); + const isPR = id === "open-pr-board"; + const column = isPR ? "needs-insight" : "needs-review"; + const review = { recommendation: isPR ? "approve" : "accept", rationale: "Ready." }; + const items = { + 1: { ...item(1), review, reviewStatus: "reviewed" }, + 2: { ...item(2), manualColumn: column }, + 3: { ...item(3), reviewStatus: "queued", queueId: "current-token", rereviewGuidance: "Check docs" }, + 4: { ...item(4), decision: { kind: isPR ? "close" : "reject", status: "confirmed" } }, + }; + const stateFile = path.join(workspacePath, "extensions", id, "state", + isPR ? "github__awesome-copilot.json" : "board.json"); + const initial = { + repo, items, updatedAt: "2026-10-01", lastRefreshedAt: "2026-10-01", + history: [{ number: 4, comment: "Previous action" }], + }; + await mkdir(path.dirname(stateFile), { recursive: true }); + await writeFile(stateFile, JSON.stringify(initial)); + + let canvas; + let calls = 0; + let sends = 0; + let fetchItems = async () => [1, 2, 3, 4, 5].map(item); + const logs = []; + const context = vm.createContext({ setTimeout }); + const sdk = { + createCanvas: (definition) => definition, + joinSession: async ({ canvases }) => { + [canvas] = canvases; + return { workspacePath, log: (message) => logs.push(message), send: () => { sends++; } }; + }, + CanvasError: class CanvasError extends Error { + constructor(code, message) { super(message); this.code = code; } + }, + }; + const url = new URL(`../.github/extensions/${id}/extension.mjs`, import.meta.url); + const module = new vm.SourceTextModule(await readFile(url, "utf8"), { + context, initializeImportMeta: (meta) => { meta.url = url.href; }, + }); + await module.link(async (specifier) => { + let exports; + if (specifier === "@github/copilot-sdk/extension") exports = sdk; + else if (specifier === "./lib/github.mjs") { + const list = () => { calls++; return fetchItems(); }; + // Any unexpected GitHub operation (especially a write) fails the test. + exports = { listOpenPullRequests: list, listBoardIssues: list }; + if (getIssueDetail) exports.getIssueDetail = getIssueDetail; + if (getPullRequestDetail) exports.getPullRequestDetail = getPullRequestDetail; + } else { + exports = await import(specifier.startsWith(".") ? new URL(specifier, url).href : specifier); + } + return new vm.SyntheticModule(Object.keys(exports), function () { + for (const [name, value] of Object.entries(exports)) this.setExport(name, value); + }, { context }); + }); + await module.evaluate(); + const ctx = { instanceId: "polling-board", input: {} }; + t.after(() => canvas.onClose(ctx)); + const action = (name, input = {}) => canvas.actions.find((entry) => entry.name === name) + .handler({ ...ctx, input }); + return { + action, open: () => canvas.open(ctx), close: () => canvas.onClose(ctx), + calls: () => calls, sends: () => sends, logs, initial, stateFile, + setFetch: (fn) => { fetchItems = fn; }, + }; +} + +async function readFrame(reader, eventName) { + let text = ""; + while (!text.includes(`event: ${eventName}\n`)) { + const { value, done } = await reader.read(); + assert.equal(done, false, `Expected ${eventName} event`); + text += new TextDecoder().decode(value); + } + return text; +} + +test("PR agent board summaries provide provenance for manual approval choices", async (t) => { + const board = await loadBoard(t, "open-pr-board"); + const before = (await board.action("get_board")).items.find((entry) => entry.number === 2); + assert.equal(before.headRefOid, "first"); + await board.action("move_item", { number: 2, column: "approve", headRefOid: before.headRefOid }); + const current = (await board.action("get_board")).items.find((entry) => entry.number === 2); + assert.equal(current.approvalStale, false); + board.setFetch(async () => [1, 2, 3, 4].map((number) => ({ ...item(number), headRefOid: "second" }))); + await board.action("refresh"); + const updated = (await board.action("get_board")).items.find((entry) => entry.number === 2); + assert.equal(updated.headRefOid, "second"); + assert.equal(updated.approvalStale, true); +}); + +for (const id of ["open-pr-board", "external-plugin-review-board"]) { +test(`${id}: server cache cannot be repopulated by a detail request invalidated by refresh`, async (t) => { + let finish; + let calls = 0; + const isPR = id === "open-pr-board"; + const board = await loadBoard(t, id, { + [isPR ? "getPullRequestDetail" : "getIssueDetail"]: () => { + calls++; + if (calls === 1) return new Promise((resolve) => { finish = resolve; }); + return { bodyHtml: "new", comments: [] }; + }, + }); + const action = isPR ? "get_pr" : "get_issue"; + const field = isPR ? "pullRequest" : "issue"; + const old = board.action(action, { number: 1 }); + await board.action("refresh"); + const latest = await board.action(action, { number: 1 }); + assert.equal(latest[field].bodyHtml, "new"); + finish({ bodyHtml: "old", comments: [] }); + await old; + const reopened = await board.action(action, { number: 1 }); + assert.equal(reopened[field].bodyHtml, "new"); + assert.equal(calls, 2, "Reopening an unselected issue fetches fresh details"); + await board.action(action, { number: 1 }); + assert.equal(calls, 2, "The fresh result remains cached"); +}); +} + +for (const id of ["open-pr-board", "external-plugin-review-board"]) { + test(`${id}: scheduled refresh preserves state, shares manual fetches, recovers, and stops on close`, + { timeout: 15000 }, async (t) => { + t.mock.timers.enable({ apis: ["setInterval"] }); + const board = await loadBoard(t, id); + const opened = await board.open(); + await board.open(); // Focusing an existing canvas must not create another timer. + const html = await (await fetch(opened.url)).text(); + const token = html.match(/ controller.abort()); + await readFrame(reader, "board"); + + t.mock.timers.tick(INTERVAL - 1); + assert.equal(board.calls(), 0, "No fetch before fifteen minutes"); + t.mock.timers.tick(1); + // Joining via the existing action waits for the scheduled fetch, not another fetch. + await board.action("refresh"); + assert.equal(board.calls(), 1); + const saved = JSON.parse(await readFile(board.stateFile, "utf8")); + assert.ok(saved.items["5"], "New matching item is discovered"); + assert.deepEqual(saved.history, board.initial.history); + for (const [number, before] of Object.entries(board.initial.items)) { + for (const field of ["review", "reviewStatus", "queueId", "manualColumn", "decision", "rereviewGuidance"]) { + assert.deepEqual(saved.items[number][field], before[field], `${number}: ${field}`); + } + } + assert.match(await readFrame(reader, "board"), /Example 5/); + await setImmediate(); + + let finish; + board.setFetch(() => new Promise((resolve) => { finish = resolve; })); + const manual = board.action("refresh"); + t.mock.timers.tick(INTERVAL); + await setImmediate(); + t.mock.timers.tick(INTERVAL); + assert.equal(board.calls(), 2, "Manual and scheduled refreshes share one fetch"); + await board.action("move_item", { number: 2, column: id === "open-pr-board" ? "close" : "probably-reject" }); + await board.action("record_review", { reviews: [{ + number: 3, queueId: "current-token", + recommendation: id === "open-pr-board" ? "approve" : "accept", + rationale: "Finished while fetching.", + }] }); + finish([1, 2, 3, 4, 5, 6].map(item)); + await manual; + await setImmediate(); + const after = JSON.parse(await readFile(board.stateFile, "utf8")); + assert.ok(after.items["6"]); + assert.equal(after.items["2"].manualColumn, id === "open-pr-board" ? "close" : "probably-reject", + "A user action during the fetch is not overwritten"); + assert.equal(after.items["3"].review.rationale, "Finished while fetching.", + "An in-flight review can finish during refresh without losing its result"); + assert.equal(after.items["3"].reviewStatus, "reviewed"); + + board.setFetch(async () => { throw new Error("GitHub unavailable"); }); + t.mock.timers.tick(INTERVAL); + assert.match(await readFrame(reader, "refresh-error"), /GitHub unavailable/); + assert.deepEqual(JSON.parse(await readFile(board.stateFile, "utf8")), after, + "A failed refresh leaves good data intact"); + assert.ok(board.logs.some((message) => message.includes("GitHub unavailable"))); + await setImmediate(); + + board.setFetch(async () => [1, 2, 3, 4, 5, 6, 7].map(item)); + t.mock.timers.tick(INTERVAL); + await board.action("refresh"); + assert.ok(JSON.parse(await readFile(board.stateFile, "utf8")).items["7"]); + assert.equal(board.calls(), 4, "Polling continues after an error"); + assert.equal(board.sends(), 0, "Polling never starts AI reviews"); + + controller.abort(); + await board.close(); + t.mock.timers.tick(INTERVAL * 2); + assert.equal(board.calls(), 4, "Closing the canvas cancels its timer"); + await board.open(); + t.mock.timers.tick(INTERVAL); + await board.action("refresh"); + assert.equal(board.calls(), 5, "Reopening starts exactly one new timer"); + await setImmediate(); + board.setFetch(() => new Promise((resolve) => { finish = resolve; })); + t.mock.timers.tick(INTERVAL); + const closingRefresh = board.action("refresh"); + await setImmediate(); + await board.close(); + t.mock.timers.tick(INTERVAL * 2); + assert.equal(board.calls(), 6, "Closing during a fetch prevents later scheduled fetches"); + finish([1, 2, 3, 4, 5, 6, 7].map(item)); + await closingRefresh; + }); + + test(`${id}: polling errors use the existing toast without changing the displayed board`, async () => { + const source = await readFile(new URL(`../.github/extensions/${id}/public/app.js`, import.meta.url), "utf8"); + const handler = source.split("\n").find((line) => line.includes('events.addEventListener("refresh-error"')); + const handlers = new Map(); + const messages = []; + const context = vm.createContext({ + events: { addEventListener: (name, fn) => handlers.set(name, fn) }, + toast: (message, options) => messages.push({ message, error: options.error }), + board: { items: [1, 2] }, selected: 1, selectedNumber: 1, + }); + vm.runInContext(handler, context); + handlers.get("refresh-error")({ data: JSON.stringify({ error: "GitHub unavailable" }) }); + assert.deepEqual(messages, [{ message: "GitHub unavailable", error: true }]); + assert.deepEqual(context.board.items, [1, 2]); + assert.equal(context.selected, 1); + assert.equal(context.selectedNumber, 1); + }); +} diff --git a/eng/board-preview.test.mjs b/eng/board-preview.test.mjs index 77b92a8103..9ceb89c42c 100644 --- a/eng/board-preview.test.mjs +++ b/eng/board-preview.test.mjs @@ -62,7 +62,7 @@ async function createUI(boardName, request = async () => ({ nodes.get("board").inert = isPR; if (isPR) { vm.runInContext(`drawerOriginNumber = ${number}`, context); - vm.runInContext("el.toolbar.inert = true", context); + vm.runInContext('el.toolbar.inert = true; previewHeadRefOid = "first"', context); } nodes.get("decision-comment").value = "Feedback"; } @@ -77,12 +77,78 @@ async function createUI(boardName, request = async () => ({ return context.decide(kind); } return { - context, nodes, drawer, listeners, messages, select, decide, + context, nodes, drawer, listeners, messages, select, decide, source, selected: () => vm.runInContext(isPR ? "selectedNumber" : "selected", context), }; } +for (const outcome of ["success", "error"]) { + test(`plugin detail reload ignores an older polling ${outcome} after the post-comment reload`, async () => { + const requests = []; + const ui = await createUI("external-plugin-review-board", () => new Promise((resolve, reject) => { + requests.push({ resolve, reject }); + })); + const polling = ui.context.loadDetail(1, { force: true }); + const posted = ui.context.loadDetail(1, { force: true }); + requests[1].resolve({ bodyHtml: "new", comments: [{ bodyHtml: "posted comment" }] }); + await posted; + if (outcome === "success") requests[0].resolve({ bodyHtml: "old", comments: [] }); + else requests[0].reject(new Error("Old failure")); + await polling; + assert.equal(vm.runInContext("details.get(1).bodyHtml", ui.context), "new"); + assert.equal(vm.runInContext("details.get(1).comments.length", ui.context), 1); + assert.equal(vm.runInContext("detailRequests.size", ui.context), 0); + }); +} + +for (const kind of ["comment", "request-changes"]) { + test(`PR drawer ${kind} includes the displayed SHA`, async () => { + const calls = []; + const ui = await createUI("open-pr-board", async (...args) => { + calls.push(args); + return { status: "submitted" }; + }); + await ui.decide(kind); + assert.equal(calls[0][1].body.headRefOid, "first"); + }); +} + for (const boardName of ["open-pr-board", "external-plugin-review-board"]) { + test(`${boardName}: live refresh retains selection, drafts, tabs, and in-flight action guards`, async () => { + const ui = await createUI(boardName); + const isPR = boardName === "open-pr-board"; + vm.runInContext(` + stagedComments.set(1, "Draft feedback"); + activeTab = "comments"; + bulkBusy = true; + ${isPR ? "decisionsInFlight.add(1)" : "decisionBusy = true"}; + `, ui.context); + ui.nodes.get("decision-comment").value = "Draft feedback"; + const snapshot = { + items: [1, 2, 3].map((number) => ({ number, reviewEligible: true })), + columns: [], + }; + if (isPR) ui.context.applySnapshot(snapshot); + else { + const handlers = new Map(); + ui.context.EventSource = class { + addEventListener(name, handler) { handlers.set(name, handler); } + }; + const start = ui.source.indexOf("function connect()"); + vm.runInContext(ui.source.slice(start, ui.source.indexOf("\nconnect();", start)), ui.context); + ui.context.connect(); + handlers.get("board")({ data: JSON.stringify(snapshot) }); + } + assert.equal(ui.selected(), 1); + assert.equal(ui.drawer.classList.contains("open"), true); + assert.equal(ui.nodes.get("decision-comment").value, "Draft feedback"); + assert.equal(vm.runInContext("stagedComments.get(1)", ui.context), "Draft feedback"); + assert.equal(vm.runInContext("activeTab", ui.context), "comments"); + assert.equal(vm.runInContext("bulkBusy", ui.context), true); + assert.equal(vm.runInContext(isPR ? "decisionsInFlight.has(1)" : "decisionBusy", ui.context), true); + assert.equal(vm.runInContext("board.items.length", ui.context), 3); + }); + test(`${boardName}: clicking inside retains the preview; clicking outside closes it`, async () => { const ui = await createUI(boardName); const { handler, capture } = ui.listeners.get("pointerdown"); @@ -115,6 +181,48 @@ for (const boardName of ["open-pr-board", "external-plugin-review-board"]) { }); } + test("plugin polling invalidates cached details, reloads the selected tab, and rejects pre-refresh responses", async () => { + const requests = []; + const pending = new Map(); + const ui = await createUI("external-plugin-review-board", (...args) => { + requests.push(args); + return new Promise((resolve) => pending.set(args[1], resolve)); + }); + vm.runInContext(` + board.lastRefreshedAt = "before"; + activeTab = "comments"; + details.set(1, { bodyHtml: "old", comments: [] }); + details.set(3, { bodyHtml: "old unselected", comments: [] }); + stagedComments.set(1, "Keep my feedback"); + `, ui.context); + ui.nodes.get("decision-comment").value = "Keep my feedback"; + const stale = ui.context.loadDetail(2, { force: true }); + const handlers = new Map(); + ui.context.EventSource = class { + addEventListener(name, handler) { handlers.set(name, handler); } + }; + const start = ui.source.indexOf("function connect()"); + vm.runInContext(ui.source.slice(start, ui.source.indexOf("\nconnect();", start)), ui.context); + ui.context.connect(); + const snapshot = { lastRefreshedAt: "after", items: [{ number: 1 }], columns: [] }; + handlers.get("board")({ data: JSON.stringify(snapshot) }); + assert.equal(vm.runInContext("details.size", ui.context), 0); + assert.deepEqual(requests.map((args) => args.slice(0, 2)), [ + ["POST", "/api/issue/2"], ["POST", "/api/issue/1"], + ]); + pending.get("/api/issue/2")({ bodyHtml: "stale response", comments: [] }); + pending.get("/api/issue/1")({ bodyHtml: "new body", comments: [{ bodyHtml: "intake completed" }] }); + await stale; + await new Promise((resolve) => setImmediate(resolve)); + assert.equal(vm.runInContext("details.has(2)", ui.context), false); + assert.equal(vm.runInContext("details.get(1).bodyHtml", ui.context), "new body"); + assert.equal(ui.selected(), 1); + assert.equal(ui.nodes.get("decision-comment").value, "Keep my feedback"); + assert.equal(vm.runInContext("stagedComments.get(1)", ui.context), "Keep my feedback"); + handlers.get("board")({ data: JSON.stringify(snapshot) }); + assert.equal(requests.length, 2, "Unchanged refresh timestamp does not reload details on local edits"); + }); + test(`${boardName}: a failed action leaves the preview open`, async () => { const ui = await createUI(boardName, async () => { throw new Error("Offline"); }); await ui.decide(); diff --git a/eng/external-plugin-review-board-ui.test.mjs b/eng/external-plugin-review-board-ui.test.mjs index 2faa0be048..4344e2b62b 100644 --- a/eng/external-plugin-review-board-ui.test.mjs +++ b/eng/external-plugin-review-board-ui.test.mjs @@ -93,6 +93,22 @@ test("Approve is disabled for submissions that are not review-eligible", () => { assert.equal(ui.reject.disabled, false); }); +test("an automatic detail failure renders its error and allows the drawer to be rendered again", () => { + const item = { number: 42, author: "contributor", labels: [], reviewEligible: true }; + const { context, ui } = createContext(item); + ui.tabs = [{ dataset: { tab: "comments" }, setAttribute() {} }]; + ui.drawerBody = { replaceChildren(...children) { this.children = children; } }; + context.activeTab = "comments"; + context.h = element; + context.details.set(42, { error: "GitHub unavailable" }); + evaluateFunction(context, appSource, "function renderDrawer()", "function renderTab("); + evaluateFunction(context, appSource, "function renderTab(", "function renderReview("); + context.renderDrawer(); + assert.equal(ui.tabs[0].textContent, "Comments"); + assert.match(text(ui.drawerBody.children[0]), /GitHub unavailable/); + assert.doesNotThrow(() => context.renderDrawer()); +}); + test("Approve retains its existing eligible, pending-decision, and bulk-busy states", () => { const eligible = { number: 42, author: "contributor", labels: [], reviewEligible: true }; diff --git a/eng/open-pr-board-drag.test.mjs b/eng/open-pr-board-drag.test.mjs index 769b8b0085..029868a4a0 100644 --- a/eng/open-pr-board-drag.test.mjs +++ b/eng/open-pr-board-drag.test.mjs @@ -55,11 +55,12 @@ test("PR cards explicitly enable native dragging; actioned cards explicitly disa test("dragging transfers the PR number for moving and clears its visual state on completion", () => { const context = createContext(); - const card = context.renderCard({ number: 42, title: "Example", author: "contributor" }); + const card = context.renderCard({ number: 42, title: "Example", author: "contributor", headRefOid: "displayed" }); const values = new Map(); const dataTransfer = { setData: (type, value) => values.set(type, value) }; card.listeners.dragstart({ dataTransfer }); assert.equal(values.get("text/plain"), "42"); + assert.equal(values.get("application/x-pr-head"), "displayed"); assert.equal(dataTransfer.effectAllowed, "move"); assert.equal(card.classList.contains("dragging"), true); card.listeners.dragend(); diff --git a/eng/open-pr-board.test.mjs b/eng/open-pr-board.test.mjs index bf914b4e09..f9a4678b50 100644 --- a/eng/open-pr-board.test.mjs +++ b/eng/open-pr-board.test.mjs @@ -32,7 +32,7 @@ test("approval passes the board's commit SHA to the GitHub helper", async (t) => const store = await createStore(t); let options; await applyDecisionOnGitHub({ - store, number: 1, kind: "approve", comment: "Reviewed.", refresh: async () => {}, + store, number: 1, kind: "approve", comment: "Reviewed.", headRefOid: "first", refresh: async () => {}, github: { approveAndEnableAutoMerge: async (repo, number, input) => { options = input; @@ -43,6 +43,170 @@ test("approval passes the board's commit SHA to the GitHub helper", async (t) => assert.deepEqual(options, { body: "Reviewed.", headRefOid: "first" }); }); +test("approval rejects a missing or previously displayed SHA after a scheduled sync without writing", async (t) => { + const store = await createStore(t); + await store.syncPullRequests([pr("second")]); + let writes = 0; + for (const headRefOid of [undefined, "", "first"]) { + await assert.rejects(applyDecisionOnGitHub({ + store, number: 1, kind: "approve", headRefOid, refresh: async () => {}, + github: { approveAndEnableAutoMerge: async () => { writes++; } }, + }), /changed since it was displayed/); + } + assert.equal(writes, 0); + assert.equal(store.getItem(1).decision, null); +}); + +test("a review finishing after a head sync retains its queued commit and remains stale", async (t) => { + const store = await createStore(t); + const { queued } = await store.markQueued([1]); + await store.syncPullRequests([pr("second")]); + await store.recordReviews([{ + number: 1, queueId: queued[0].queueId, recommendation: "approve", + rationale: "Reviewed the first commit.", headRefOid: "second", + }]); + assert.equal(store.getItem(1).review.headRefOid, "first", "Payload cannot overwrite queue provenance"); + assert.equal(store.snapshot().items[0].reviewStale, true); + const next = await store.markQueued([1]); + await store.recordReviews([{ number: 1, queueId: next.queued[0].queueId, recommendation: "approve" }]); + assert.equal(store.getItem(1).review.headRefOid, "second"); + assert.equal(store.snapshot().items[0].reviewStale, false); +}); + +test("manual approval provenance survives refresh and is renewed only by an explicit move", async (t) => { + const store = await createStore(t); + await store.moveItem(1, "approve", "first"); + assert.equal(store.snapshot().items[0].approvalStale, false); + await store.syncPullRequests([pr("second")]); + assert.equal(store.getItem(1).manualColumn, "approve"); + assert.equal(store.getItem(1).manualHeadRefOid, "first"); + assert.equal(store.snapshot().items[0].approvalStale, true); + await store.moveItem(1, "approve", "second"); + assert.equal(store.snapshot().items[0].approvalStale, false); + const { previous } = await store.markQueued([1]); + await store.restoreQueueState(previous); + assert.equal(store.getItem(1).queueHeadRefOid, previous[0].queueHeadRefOid); +}); + +test("legacy approval choices without commit provenance cannot silently authorize bulk approval", async (t) => { + const store = await createStore(t); + store.getItem(1).review = { recommendation: "approve", reviewedAt: "2099-01-01" }; + assert.equal(store.snapshot().items[0].reviewStale, true); + store.getItem(1).manualColumn = "approve"; + assert.equal(store.snapshot().items[0].approvalStale, true); +}); + +test("sync before a delayed manual move retains the client's original commit", async (t) => { + const store = await createStore(t); + await store.syncPullRequests([pr("second")]); + await store.moveItem(1, "approve", "first"); + assert.equal(store.getItem(1).manualHeadRefOid, "first"); + assert.equal(store.snapshot().items[0].approvalStale, true); + await store.moveItem(1, "approve"); + assert.equal(store.snapshot().items[0].approvalStale, true, "Missing client provenance fails closed"); +}); + +for (const kind of ["comment", "request-changes"]) { + test(`${kind} rejects feedback dispatched after the displayed head has changed`, async (t) => { + const store = await createStore(t); + await store.syncPullRequests([pr("second")]); + let writes = 0; + for (const headRefOid of ["first", undefined]) { + await assert.rejects(applyDecisionOnGitHub({ + store, number: 1, kind, comment: "Feedback on first", headRefOid, + github: { submitPullRequestReview: async () => { writes++; } }, + refresh: async () => {}, + }), /changed since it was displayed/); + } + assert.equal(writes, 0); + assert.equal(store.getItem(1).decision, null); + }); + + test(`a delayed ${kind} response retains the action head when polling discovers new commits`, async (t) => { + const store = await createStore(t); + const pending = deferred(); + const action = applyDecisionOnGitHub({ + store, number: 1, kind, comment: "Feedback on first", headRefOid: "first", + github: { submitPullRequestReview: async () => { await pending.promise; return { status: "submitted" }; } }, + refresh: () => store.syncPullRequests([pr("second")]), + }); + await store.syncPullRequests([pr("second")]); + pending.resolve(); + await action; + assert.equal(store.getItem(1).decision, null); + assert.equal(store.snapshot().items[0].column, "unreviewed"); + assert.equal(store.state.history[0].kind, kind); + }); + + for (const currentState of ["queued", "reviewed", "manual"]) { + test(`obsolete ${kind} feedback preserves the newer head's ${currentState} state`, async (t) => { + const store = await createStore(t); + const pending = deferred(); + const action = applyDecisionOnGitHub({ + store, number: 1, kind, comment: "Feedback on first", headRefOid: "first", + github: { submitPullRequestReview: async () => { await pending.promise; return { status: "submitted" }; } }, + refresh: () => store.syncPullRequests([pr("second")]), + }); + await store.syncPullRequests([pr("second")]); + const { queued } = await store.markQueued([1]); + if (currentState === "reviewed") { + await store.recordReviews([{ number: 1, queueId: queued[0].queueId, recommendation: "approve" }]); + } else if (currentState === "manual") { + await store.moveItem(1, "needs-insight", "second"); + } + const before = structuredClone(store.getItem(1)); + pending.resolve(); + await action; + assert.deepEqual(store.getItem(1), before, "Obsolete feedback must not replace current review or move state"); + assert.equal(store.state.history[0].kind, kind); + if (currentState === "queued") { + const [result] = await store.recordReviews([{ + number: 1, queueId: queued[0].queueId, recommendation: "approve", + }]); + assert.equal(result.status, "recorded", "The newer head's queue token remains valid"); + } + }); + } + + for (const oldState of ["queued", "reviewed", "manual"]) { + test(`obsolete ${kind} feedback clears the old head's ${oldState} state`, async (t) => { + const store = await createStore(t); + const { queued } = await store.markQueued([1]); + if (oldState === "reviewed") { + await store.recordReviews([{ number: 1, queueId: queued[0].queueId, recommendation: "request-changes" }]); + } else if (oldState === "manual") { + await store.moveItem(1, "close", "first"); + } + const pending = deferred(); + const action = applyDecisionOnGitHub({ + store, number: 1, kind, comment: "Feedback on first", headRefOid: "first", + github: { submitPullRequestReview: async () => { await pending.promise; return { status: "submitted" }; } }, + refresh: () => store.syncPullRequests([pr("second")]), + }); + await store.syncPullRequests([pr("second")]); + pending.resolve(); + await action; + assert.equal(store.snapshot().items[0].column, "unreviewed"); + assert.equal(store.getItem(1).review, null); + assert.equal(store.getItem(1).manualColumn, null); + assert.equal(store.getItem(1).queueId, null); + assert.equal(store.state.history[0].kind, kind); + }); + } + + test(`obsolete ${kind} feedback clears an old review while preserving the new head's queue`, async (t) => { + const store = await createStore(t); + const old = await store.markQueued([1]); + await store.recordReviews([{ number: 1, queueId: old.queued[0].queueId, recommendation: "request-changes" }]); + await store.syncPullRequests([pr("second")]); + const current = await store.markQueued([1]); + await store.recordDecision(1, { kind, headRefOid: "first" }); + assert.equal(store.getItem(1).review, null); + assert.equal(store.getItem(1).queueId, current.queued[0].queueId); + assert.equal(store.snapshot().items[0].column, "reviewing"); + }); +} + test("approval and auto-merge are pinned to the reviewed commit", async () => { const headRefOid = "a".repeat(40); const calls = []; @@ -231,7 +395,7 @@ test("concurrent GitHub decisions for the same repo/PR cannot both write", async submitPullRequestReview: async () => { calls++; await waiting.promise; return { status: "comment-submitted" }; }, closePullRequest: async () => { calls++; throw new Error("must not be reached"); }, }; - const args = { store, number: 1, comment: "Feedback", github, refresh: async () => {} }; + const args = { store, number: 1, comment: "Feedback", headRefOid: "first", github, refresh: async () => {} }; const first = applyDecisionOnGitHub({ ...args, kind: "comment" }); await assert.rejects(applyDecisionOnGitHub({ ...args, kind: "close" }), /in progress/); assert.equal(calls, 1); @@ -242,7 +406,7 @@ test("concurrent GitHub decisions for the same repo/PR cannot both write", async test("decision lock is released on failure and independent repositories can proceed", async (t) => { const store = await createStore(t); - const args = { store, number: 1, kind: "comment", comment: "Feedback", refresh: async () => {} }; + const args = { store, number: 1, kind: "comment", comment: "Feedback", headRefOid: "first", refresh: async () => {} }; await assert.rejects(applyDecisionOnGitHub({ ...args, github: { submitPullRequestReview: async () => { throw new Error("offline"); } }, }), /offline/); @@ -322,16 +486,84 @@ async function createUI() { select(number) { selectedNumber = number; board = { columns: [], items: [1, 2].map(number => ({ - number, title: "Example", labels: [], createdAt: "2026-10-01", column: "unreviewed", + number, title: "Example", labels: [], createdAt: "2026-10-01", column: "unreviewed", headRefOid: "first", })) }; }, - state: () => ({ detail, detailError, activeTab }), + state: () => ({ detail, detailError, activeTab, previewHeadRefOid }), render: renderDrawerBody, }; `, context); return { context, ui: context.ui, nodes, tabs, document }; } +test("a refreshed PR head reloads the preview and blocks approval until the new detail arrives", async () => { + const { context, ui, nodes } = await createUI(); + const requests = []; + const messages = []; + context.notify = (message) => messages.push(message); + vm.runInContext("toast = notify", context); + ui.select(1); + context.nextRequest = deferred(); + const initial = ui.loadDetail(1); + context.nextRequest.resolve({ headRefOid: "first", files: [{ filename: "old.js" }] }); + await initial; + assert.equal(ui.state().previewHeadRefOid, "first"); + nodes.get("decision-comment").value = "Keep this feedback"; + context.nextRequest = deferred(); + const pending = context.nextRequest; + context.applySnapshot({ + columns: [], items: [{ number: 1, title: "New head", labels: [], headRefOid: "second" }], + }); + assert.equal(ui.state().detail, null); + assert.equal(ui.state().previewHeadRefOid, null); + await ui.submitDecision(nodes.get("decision-approve"), "approve"); + assert.match(messages[0], /preview is loading or changed/); + assert.equal(nodes.get("decision-comment").value, "Keep this feedback"); + pending.resolve({ headRefOid: "second", files: [{ filename: "new.js" }] }); + await new Promise((resolve) => setImmediate(resolve)); + assert.equal(ui.state().previewHeadRefOid, "second"); + assert.equal(ui.state().detail.files[0].filename, "new.js"); + context.request = async (...args) => { requests.push(args); throw new Error("stop after request"); }; + vm.runInContext("api = request", context); + await ui.submitDecision(nodes.get("decision-approve"), "approve"); + await ui.submitDecision(nodes.get("decision-approve"), "approve"); + assert.equal(requests.length, 1); + assert.equal(requests[0][1].body.headRefOid, "second"); +}); + +test("a cached preview from another commit cannot authorize approval", async () => { + const { context, ui } = await createUI(); + ui.select(1); + context.nextRequest = deferred(); + const pending = ui.loadDetail(1); + context.nextRequest.resolve({ headRefOid: "older", files: [] }); + await pending; + assert.equal(ui.state().previewHeadRefOid, null); + assert.equal(ui.state().detail, null); + assert.match(ui.state().detailError, /changed while loading/); +}); + +test("a successful same-head refresh retries a failed preview without disturbing the draft", async () => { + const { context, ui, nodes } = await createUI(); + ui.select(1); + context.nextRequest = deferred(); + const failed = ui.loadDetail(1); + context.nextRequest.reject(new Error("Transient GitHub error")); + await failed; + assert.match(ui.state().detailError, /Transient/); + nodes.get("decision-comment").value = "Keep my feedback"; + context.nextRequest = deferred(); + context.applySnapshot({ + updatedAt: "new-refresh", columns: [], + items: [{ number: 1, title: "Example", labels: [], headRefOid: "first" }], + }); + assert.equal(ui.state().detailError, null); + context.nextRequest.resolve({ headRefOid: "first", files: [] }); + await new Promise((resolve) => setImmediate(resolve)); + assert.equal(ui.state().previewHeadRefOid, "first"); + assert.equal(nodes.get("decision-comment").value, "Keep my feedback"); +}); + test("older PR detail responses and errors cannot overwrite a newer selected PR", async () => { const { context, ui } = await createUI(); for (const fail of [false, true]) {