Merge pull request #5064 from github/aaronpowell-canvas-tweaks

Fix review board preview dismissal and PR card dragging

.github/extensions/external-plugin-review-board/README.md

@@ -10,6 +10,7 @@ Open it by asking Copilot to "open the plugin review board" (canvas id `external
 - **Kanban board** with buckets: Unreviewed → Reviewing → Straight reject / Probably reject / Needs review /
   Accept → Actioned. Drag cards between buckets (or use the *Bucket* dropdown in the details panel).
   Dragging a card back to *Unreviewed* clears its AI review so it's picked up by the next review.
+  Cards in each bucket are sorted oldest to newest by creation date.
 - **Perform review** asks the agent to review every item without an AI review, using
   [`review-guidance.md`](./review-guidance.md) and the decisions you've made from the board. Results are
   recorded with the `record_review` canvas action and move cards into their recommended bucket.
@@ -26,6 +27,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 <reason>` 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
@@ -56,7 +59,9 @@ not imported; boards start fresh.
 
 ## Regression tests
 
+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.
 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.

.github/extensions/external-plugin-review-board/public/app.js

@@ -179,7 +179,9 @@ function renderBoard() {
     );
 
     const columns = board.columns.map((column) => {
-        const items = board.items.filter((item) => item.column === column.id && matchesFilter(item));
+        const items = board.items
+            .filter((item) => item.column === column.id && matchesFilter(item))
+            .sort((a, b) => new Date(a.createdAt) - new Date(b.createdAt) || a.number - b.number);
         const cards = h(
             "div",
             { class: "column-cards" },
@@ -261,6 +263,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 +346,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 +620,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 +677,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 +719,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 {

.github/extensions/open-pr-board/README.md

@@ -10,7 +10,8 @@ authenticated `gh` CLI.
 
 - **Buckets** — `Unreviewed`, `Reviewing`, `Close`, `Request changes`,
   `Needs insight`, `Approve`, `Actioned`. The AI recommendation picks the bucket;
-  dragging a card overrides it.
+  dragging a card overrides it. Cards in each bucket are sorted oldest to newest
+  by creation date.
 - **AI reviews** — "Review pending" asks the agent to review every PR that has never
   been reviewed or whose review is stale. The agent records structured results
   (rationale, repo fit, compliance, differentiation, risks, validation, confidence,
@@ -32,6 +33,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
@@ -88,9 +91,12 @@ Results arriving after a manual move, reset, or replacement review are rejected.
 
 ## Regression tests
 
+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.
 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.

.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",
@@ -280,7 +280,9 @@ function renderBoard() {
     for (const list of el.board.querySelectorAll(".column-cards")) scroll.set(list.dataset.column, list.scrollTop);
 
     const columns = board.columns.map((column) => {
-        const items = board.items.filter((item) => item.column === column.id && matchesFilter(item));
+        const items = board.items
+            .filter((item) => item.column === column.id && matchesFilter(item))
+            .sort((a, b) => new Date(a.createdAt) - new Date(b.createdAt) || a.number - b.number);
         const list = h(
             "div",
             {
@@ -402,6 +404,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 +439,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 +733,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 +783,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 +815,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 +865,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();
     }
 });
 

eng/board-column-order.test.mjs

@@ -0,0 +1,45 @@
+import assert from "node:assert/strict";
+import { readFile } from "node:fs/promises";
+import vm from "node:vm";
+import { test } from "node:test";
+
+for (const boardName of ["open-pr-board", "external-plugin-review-board"]) {
+    test(`${boardName}: columns render oldest to newest without changing board state`, async () => {
+        const source = await readFile(
+            new URL(`../.github/extensions/${boardName}/public/app.js`, import.meta.url), "utf8",
+        );
+        const items = [
+            { number: 50, column: "approve", createdAt: "2026-10-14T00:00:00Z", updatedAt: "2026-10-14T00:00:00Z" },
+            { number: 30, column: "approve", createdAt: "2026-10-01T00:00:00Z", updatedAt: "2026-10-14T01:00:00Z" },
+            { number: 10, column: "close", createdAt: "2026-09-01T00:00:00Z" },
+            { number: 20, column: "approve", createdAt: "2026-10-01T00:00:00Z" },
+            { number: 5, column: "approve", createdAt: "2026-08-01T00:00:00Z", hidden: true },
+            { number: 40, column: "close", createdAt: "2026-10-10T00:00:00Z" },
+        ];
+        const original = structuredClone(items);
+        const rendered = [];
+        const node = { querySelectorAll: () => [], replaceChildren() {} };
+        const ui = {
+            board: node, title: {}, summary: {}, meta: {}, review: {},
+        };
+        const context = vm.createContext({
+            board: { items, columns: [{ id: "approve" }, { id: "close" }, { id: "empty" }] },
+            el: ui, ui, filter: "",
+            matchesFilter: (item) => !item.hidden,
+            renderCard: (item) => { rendered.push(item.number); return {}; },
+            h: () => ({}),
+            relativeTime: () => "now",
+            pendingReviewCount: () => 0,
+            COLUMN_HINTS: {},
+        });
+        vm.runInContext(source.slice(source.indexOf("function renderBoard()"), source.indexOf("function columnItems(")), context);
+        context.renderBoard();
+        assert.deepEqual(rendered, [20, 30, 50, 10, 40]);
+        assert.deepEqual(items, original);
+
+        items[0].column = "close";
+        rendered.length = 0;
+        context.renderBoard();
+        assert.deepEqual(rendered, [20, 30, 10, 40, 50]);
+    });
+}

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");
+});

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);
+});