From a10206b7f6d3a0e3dbf810290e6cf35153fa537a Mon Sep 17 00:00:00 2001 From: Aaron Powell Date: Wed, 14 Oct 2026 11:25:57 +1100 Subject: [PATCH] fix(canvases): dismiss previews and enable PR card dragging Close review board previews after successful item actions and outside clicks without dismissing a different item during in-flight requests. Explicitly enable native dragging on editable PR cards and add regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd29e26b-0e56-48bf-a046-ac749aef86ce --- .../external-plugin-review-board/README.md | 3 + .../public/app.js | 10 +- .github/extensions/open-pr-board/README.md | 4 + .../extensions/open-pr-board/public/app.js | 23 ++- eng/board-preview.test.mjs | 185 ++++++++++++++++++ eng/open-pr-board-drag.test.mjs | 67 +++++++ 6 files changed, 283 insertions(+), 9 deletions(-) create mode 100644 eng/board-preview.test.mjs create mode 100644 eng/open-pr-board-drag.test.mjs diff --git a/.github/extensions/external-plugin-review-board/README.md b/.github/extensions/external-plugin-review-board/README.md index 941fef6a2..82aa3d9fe 100644 --- a/.github/extensions/external-plugin-review-board/README.md +++ b/.github/extensions/external-plugin-review-board/README.md @@ -26,6 +26,8 @@ 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. + 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 after a confirm click. The reject reason is prefilled from the AI's suggested comment. - **Column actions** approve, reject, or re-review every eligible item in a column, including @@ -60,3 +62,4 @@ Run `node --experimental-vm-modules --test eng/board-state-storage.test.mjs` fro to check both canvases' session-scoped persistence without accessing GitHub. Run `node --test eng/board-bulk-actions.test.mjs` to check column confirmation, staged feedback, queue exclusions, and partial failures using in-process fakes. +Run `node --test eng/board-preview.test.mjs` to check preview dismissal and in-flight actions. diff --git a/.github/extensions/external-plugin-review-board/public/app.js b/.github/extensions/external-plugin-review-board/public/app.js index b28f9d8c0..192accb43 100644 --- a/.github/extensions/external-plugin-review-board/public/app.js +++ b/.github/extensions/external-plugin-review-board/public/app.js @@ -261,6 +261,7 @@ async function bulkAction(columnId, kind) { } await api("POST", "/api/decision", { ...item, kind }); completed++; + if (selected === item.number) closeDrawer(); } catch (error) { failures.push(`#${item.number}: ${error.message}`); } @@ -343,6 +344,7 @@ async function moveItem(number, column) { const hadReview = column === "unreviewed" && Boolean(findItem(number)?.review); try { await api("POST", "/api/move", { number, column }); + if (selected === number) closeDrawer(); if (hadReview) toast(`Cleared the AI review for #${number}; it will be included in the next review.`); } catch (error) { toast(error.message, { error: true }); @@ -616,10 +618,11 @@ async function decide(kind) { : `Posted ${result.body.split("\n")[0]} on #${item.number}`, { link: result.commentUrl }, ); + if (selected === item.number) closeDrawer(); } catch (error) { toast(error.message, { error: true }); } finally { - resetConfirm(); + if (selected === item.number) resetConfirm(); } } @@ -672,6 +675,9 @@ ui.search.addEventListener("input", () => { }); ui.drawerClose.addEventListener("click", closeDrawer); +document.addEventListener("pointerdown", (event) => { + if (selected !== null && !ui.drawer.contains(event.target)) closeDrawer(); +}, true); ui.drawerMove.addEventListener("change", () => moveItem(selected, ui.drawerMove.value)); function hideRereview() { ui.rereviewPanel.hidden = true; @@ -711,7 +717,7 @@ ui.rereviewPanel.addEventListener("submit", async (event) => { try { const result = await api("POST", "/api/rereview", { number, guidance: ui.rereviewGuidance.value }); toast(result.message); - hideRereview(); + if (selected === number) closeDrawer(); } catch (error) { toast(error.message, { error: true }); } finally { diff --git a/.github/extensions/open-pr-board/README.md b/.github/extensions/open-pr-board/README.md index c2648c8bf..098f12fbb 100644 --- a/.github/extensions/open-pr-board/README.md +++ b/.github/extensions/open-pr-board/README.md @@ -32,6 +32,8 @@ authenticated `gh` CLI. - **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. + Clicking outside the drawer or successfully submitting an item action closes it; + failed actions leave it open so you can correct or retry them. - **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 @@ -92,5 +94,7 @@ Run `node --test eng/open-pr-board.test.mjs` from the repository root. GitHub wr are tested with injected in-process fakes, never the authenticated CLI. Run `node --test eng/board-bulk-actions.test.mjs` to check column confirmation, staged feedback, queue exclusions, and partial failures using in-process fakes. +Run `node --test eng/board-preview.test.mjs` to check preview dismissal and in-flight actions. +Run `node --test eng/open-pr-board-drag.test.mjs` to check native card dragging and transfer data. Run `node --experimental-vm-modules --test eng/board-state-storage.test.mjs` to check both canvases' session-scoped persistence without accessing GitHub. diff --git a/.github/extensions/open-pr-board/public/app.js b/.github/extensions/open-pr-board/public/app.js index a3004d39c..5f16c4b30 100644 --- a/.github/extensions/open-pr-board/public/app.js +++ b/.github/extensions/open-pr-board/public/app.js @@ -241,7 +241,7 @@ function renderCard(item) { "article", { class: item.number === selectedNumber ? "card selected" : "card", - draggable: !item.decision, + draggable: item.decision ? "false" : "true", dataset: { number: String(item.number) }, tabindex: "0", role: "button", @@ -402,6 +402,7 @@ async function bulkAction(columnId, kind) { method: "POST", body: { number: item.number, kind, comment: item.comment }, }); completed++; + if (selectedNumber === item.number) closeDrawer(); if (result.warning) failures.push(`#${item.number}: ${result.warning}`); } catch (error) { failures.push(`#${item.number}: ${error.message}`); @@ -436,12 +437,14 @@ function reviewTab(item) { { class: "btn btn-small btn-primary", type: "button", - onclick: (event) => - run( + onclick: async (event) => { + const result = await run( event.currentTarget, () => api("/api/review", { method: "POST", body: { numbers: [item.number] } }), (result) => result.message ?? "Review requested.", - ), + ); + if (result && selectedNumber === item.number) closeDrawer(); + }, }, "Review this PR", ), @@ -728,12 +731,12 @@ async function submitDecision(button, kind) { api("/api/decision", { method: "POST", body: { number: item.number, kind, comment } }), ); decisionsInFlight.delete(item.number); - renderDrawerHeader(); + if (selectedNumber === item.number) renderDrawerHeader(); button.textContent = DECISION_BUTTONS[kind]; if (result) { if (result.warning) toast(result.warning, { error: true, url: result.url }); else toast(`#${item.number}: ${result.status}.`, { url: result.url }); - if (currentItem()) loadDetail(item.number, { force: true }); + if (selectedNumber === item.number) closeDrawer(); } } @@ -778,6 +781,10 @@ el.review.addEventListener("click", (event) => el.drawerClose.addEventListener("click", closeDrawer); +document.addEventListener("pointerdown", (event) => { + if (selectedNumber !== null && !el.drawer.contains(event.target)) closeDrawer(); +}, true); + document.addEventListener("keydown", (event) => { if (!el.drawer.classList.contains("open")) return; if (event.key === "Escape") { @@ -806,6 +813,7 @@ el.drawerColumn.addEventListener("change", async (event) => { if (column === item.column) return; try { await api("/api/move", { method: "POST", body: { number: item.number, column } }); + if (selectedNumber === item.number) closeDrawer(); } catch (error) { toast(error.message, { error: true }); event.target.value = item.column; @@ -855,9 +863,10 @@ el.rereviewStart.addEventListener("click", async (event) => { }), (value) => value.message ?? "Re-review requested.", ); - if (result) { + if (result && selectedNumber === item.number) { el.rereviewGuidance.value = ""; el.rereviewPanel.hidden = true; + closeDrawer(); } }); diff --git a/eng/board-preview.test.mjs b/eng/board-preview.test.mjs new file mode 100644 index 000000000..3809e295f --- /dev/null +++ b/eng/board-preview.test.mjs @@ -0,0 +1,185 @@ +import assert from "node:assert/strict"; +import { readFile } from "node:fs/promises"; +import vm from "node:vm"; +import { test } from "node:test"; + +async function createUI(boardName, request = async () => ({ + status: "submitted", body: "/approve", pending: true, message: "Queued", +})) { + const nodes = new Map(); + function node() { + const classes = new Set(); + return { + content: "", value: "", dataset: {}, listeners: {}, attributes: {}, + classList: { + add: (name) => classes.add(name), + remove: (name) => classes.delete(name), + contains: (name) => classes.has(name), + }, + addEventListener(name, handler) { this.listeners[name] = handler; }, + setAttribute(name, value) { this.attributes[name] = value; }, + querySelectorAll: () => [], + querySelector: () => null, + contains(target) { return target === this || target.insideDrawer === true; }, + focus() { this.focused = true; }, + }; + } + const listeners = new Map(); + const document = { + querySelector: node, + querySelectorAll: () => [], + getElementById(id) { + if (!nodes.has(id)) nodes.set(id, node()); + return nodes.get(id); + }, + addEventListener(name, handler, capture) { listeners.set(name, { handler, capture }); }, + }; + const messages = []; + const context = vm.createContext({ + document, setTimeout: () => 1, clearTimeout() {}, confirm: () => true, + }); + const isPR = boardName === "open-pr-board"; + const source = await readFile(new URL(`../.github/extensions/${boardName}/public/app.js`, import.meta.url), "utf8"); + const end = isPR ? "const events = new EventSource" : "function connect()"; + vm.runInContext(source.slice(0, source.indexOf(end)), context); + context.request = request; + context.notify = (message) => messages.push(message); + vm.runInContext(` + board = { items: [1, 2].map(number => ({ + number, column: "approve", headRefOid: "first", reviewEligible: true, + review: { suggestedComment: "Feedback" }, + })), columns: [] }; + renderBoard = renderDrawer = renderDrawerHeader = renderDrawerBody = () => {}; + api = request; + toast = notify; + `, context); + const drawer = nodes.get("drawer"); + function select(number) { + vm.runInContext(`${isPR ? "selectedNumber" : "selected"} = ${number}`, context); + drawer.classList.add("open"); + drawer.setAttribute("aria-hidden", "false"); + drawer.inert = false; + nodes.get("board").inert = isPR; + if (isPR) { + vm.runInContext(`drawerOriginNumber = ${number}`, context); + vm.runInContext("el.toolbar.inert = true", context); + } + nodes.get("decision-comment").value = "Feedback"; + } + select(1); + async function decide(kind = "approve") { + if (isPR) { + const button = nodes.get("decision-approve") ?? node(); + await context.submitDecision(button, kind); + return context.submitDecision(button, kind); + } + await context.decide(kind); + return context.decide(kind); + } + return { + context, nodes, drawer, listeners, messages, select, decide, + selected: () => vm.runInContext(isPR ? "selectedNumber" : "selected", context), + }; +} + +for (const boardName of ["open-pr-board", "external-plugin-review-board"]) { + test(`${boardName}: clicking inside retains the preview; clicking outside closes it`, async () => { + const ui = await createUI(boardName); + const { handler, capture } = ui.listeners.get("pointerdown"); + assert.equal(capture, true); + handler({ target: { insideDrawer: true } }); + assert.equal(ui.selected(), 1); + handler({ target: {} }); + assert.equal(ui.selected(), null); + assert.equal(ui.drawer.classList.contains("open"), false); + assert.equal(ui.drawer.attributes["aria-hidden"], "true"); + assert.equal(ui.nodes.get("board").inert, false); + if (boardName === "open-pr-board") { + assert.equal(ui.drawer.inert, true); + assert.equal(ui.nodes.get("filter").focused, true); + assert.equal(vm.runInContext("el.toolbar.inert", ui.context), false); + } + handler({ target: {} }); + assert.equal(ui.selected(), null); + }); + + const kinds = boardName === "open-pr-board" + ? ["approve", "close", "request-changes", "comment"] : ["approve", "reject"]; + for (const kind of kinds) { + test(`${boardName}: successful ${kind} closes the preview`, async () => { + const ui = await createUI(boardName); + await ui.decide(kind); + assert.equal(ui.selected(), null); + assert.equal(ui.drawer.classList.contains("open"), false); + assert.equal(ui.messages.length, 1); + }); + } + + test(`${boardName}: a failed action leaves the preview open`, async () => { + const ui = await createUI(boardName, async () => { throw new Error("Offline"); }); + await ui.decide(); + assert.equal(ui.selected(), 1); + assert.equal(ui.drawer.classList.contains("open"), true); + assert.equal(ui.messages[0], "Offline"); + }); + + test(`${boardName}: completion does not close a different preview opened in flight`, async () => { + let release; + const ui = await createUI(boardName, () => new Promise(resolve => { release = resolve; })); + const pending = ui.decide(); + await new Promise(resolve => setImmediate(resolve)); + ui.listeners.get("pointerdown").handler({ target: {} }); + ui.select(2); + release({ status: "submitted", body: "/approve" }); + await pending; + assert.equal(ui.selected(), 2); + assert.equal(ui.drawer.classList.contains("open"), true); + }); + + test(`${boardName}: successful bucket moves close the preview`, async () => { + const ui = await createUI(boardName); + if (boardName === "open-pr-board") { + await ui.nodes.get("drawer-column").listeners.change({ target: { value: "close" } }); + } else { + await ui.context.moveItem(1, "straight-reject"); + } + assert.equal(ui.selected(), null); + }); + + test(`${boardName}: successful re-review closes the preview`, async () => { + const ui = await createUI(boardName); + if (boardName === "open-pr-board") { + await ui.nodes.get("rereview-start").listeners.click({ currentTarget: ui.nodes.get("rereview-start") }); + } else { + await ui.nodes.get("rereview-panel").listeners.submit({ preventDefault() {} }); + } + assert.equal(ui.selected(), null); + }); + + test(`${boardName}: failed bucket moves and re-reviews leave the preview open`, async () => { + const ui = await createUI(boardName, async () => { throw new Error("Offline"); }); + if (boardName === "open-pr-board") { + await ui.nodes.get("drawer-column").listeners.change({ target: { value: "close" } }); + await ui.nodes.get("rereview-start").listeners.click({ currentTarget: ui.nodes.get("rereview-start") }); + } else { + await ui.context.moveItem(1, "straight-reject"); + await ui.nodes.get("rereview-panel").listeners.submit({ preventDefault() {} }); + } + assert.equal(ui.selected(), 1); + assert.equal(ui.drawer.classList.contains("open"), true); + assert.deepEqual(ui.messages, ["Offline", "Offline"]); + }); + + test(`${boardName}: bulk decisions close a preview of a successfully actioned item`, async () => { + const ui = await createUI(boardName); + await ui.context.bulkAction("approve", "approve"); + assert.equal(ui.selected(), null); + }); +} + +test("open-pr-board: partial approval success closes the preview and keeps the warning visible", async () => { + const ui = await createUI("open-pr-board", async () => ({ warning: "Auto-merge could not be enabled" })); + await ui.decide(); + assert.equal(ui.selected(), null); + assert.equal(ui.messages[0], "Auto-merge could not be enabled"); +}); diff --git a/eng/open-pr-board-drag.test.mjs b/eng/open-pr-board-drag.test.mjs new file mode 100644 index 000000000..769b8b008 --- /dev/null +++ b/eng/open-pr-board-drag.test.mjs @@ -0,0 +1,67 @@ +import assert from "node:assert/strict"; +import { readFile } from "node:fs/promises"; +import vm from "node:vm"; +import { test } from "node:test"; + +const source = await readFile( + new URL("../.github/extensions/open-pr-board/public/app.js", import.meta.url), "utf8", +); + +class Node { + constructor() { + this.attributes = {}; + this.dataset = {}; + this.listeners = {}; + const classes = new Set(); + this.classList = { + add: (name) => classes.add(name), + remove: (name) => classes.delete(name), + contains: (name) => classes.has(name), + }; + } + setAttribute(name, value) { this.attributes[name] = value; } + addEventListener(name, handler) { this.listeners[name] = handler; } + append() {} + get draggable() { return this.attributes.draggable === "true"; } +} + +function createContext() { + const context = vm.createContext({ + Node, + document: { createElement: () => new Node(), createTextNode: () => new Node() }, + selectedNumber: null, + relativeTime: () => "now", + statusPills: () => [], + }); + for (const [start, end] of [ + ["function h(", "/**"], + ["function renderCard(", "function renderBoard("], + ]) { + vm.runInContext(source.slice(source.indexOf(start), source.indexOf(end, source.indexOf(start))), context); + } + return context; +} + +test("PR cards explicitly enable native dragging; actioned cards explicitly disable it", () => { + const context = createContext(); + const item = { number: 42, title: "Example", author: "contributor" }; + const card = context.renderCard(item); + assert.equal(card.attributes.draggable, "true"); + assert.equal(card.draggable, true); + const actioned = context.renderCard({ ...item, decision: { kind: "approve" } }); + assert.equal(actioned.attributes.draggable, "false"); + assert.equal(actioned.draggable, false); +}); + +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 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(dataTransfer.effectAllowed, "move"); + assert.equal(card.classList.contains("dragging"), true); + card.listeners.dragend(); + assert.equal(card.classList.contains("dragging"), false); +});