trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns15g rEventsFlows

Remove on a board panel goes through the confirm page, which names the panel, and refuses if the board changed underneath #8037

Merged scoutcrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

scout/forge:scout/8030-remove-on-a-board-panel-goes-through-the → main 3 commits fork 1 thread · 1 unresolved

  • Merged. scout/8030-remove-on-a-board-panel-goes-through-the was merged into main as 29ee40e.

Review

Sign in to review this change.
9 files changed +144 −10
README.md +2
@@ -59,6 +59,8 @@ when). The terminal's palette has the same as "Crew hand-overs", a list of the
5959 same rows (repo#number, title, reasons): Enter opens a parked row's issue and
6060 `r` reads the list again. Both are a forge admin's only.
6161
62 Remove on a board panel asks first, on the confirm page, which names the panel and the board. The confirm posts the panel's index and title, and a panel at that index with another title is not removed ("that panel moved; nothing was removed"). The guard compares titles only: two panels with the same title swapped between the two clicks still remove the one now at that index, so give panels distinct titles (pins on home tell panels apart by title too).
63
6264 `/boards` carries a Held files card for anyone who administers a repository (a forge admin sees every repository's): the holds of the repositories you administer, a form to hold another path, Release on each row, and "Showing 200 of N" when the list is cut. The user menu links Boards to such a person. In the terminal, a bare line on the Held files prompt lists the same holds.
6365
6466 A hold can be set "until green" (a checkbox on the hold form, `until: "green"` on the hold API and the MCP `hold` tool): when a check passes on the pull's current head the forge releases it and the pull's timeline says `hold released: green on <short oid>`. A plain hold is never released by a check.
src/boards/edit.ts +3 −1
@@ -11,7 +11,7 @@ export type Edit =
1111 | { kind: "settings"; range: unknown; refresh: unknown }
1212 | { kind: "add"; panel: unknown }
1313 | { kind: "edit"; index: number; panel: unknown }
14 | { kind: "remove"; index: number }
14 | { kind: "remove"; index: number; title?: string }
1515 | { kind: "move"; index: number; by: -1 | 1 };
1616
1717 export function applyEdit(board: Board, edit: Edit): Result<Board, string> {
@@ -42,6 +42,8 @@ export function applyEdit(board: Board, edit: Edit): Result<Board, string> {
4242 break;
4343 }
4444 case "remove":
45 // A title names the panel the person saw; a board reordered since must not lose another one.
46 if (edit.title !== undefined && panels[at]!.title !== edit.title) return err("that panel moved; nothing was removed");
4547 panels.splice(at, 1);
4648 next = { ...board, panels };
4749 break;
src/web/pages/boards-edit.ts +11 −3
@@ -27,13 +27,21 @@ function panelOf(form: Form): Record<string, unknown> {
2727 return { title: field(form, "title"), chart: p.chart, query: toQuery(p), w: num(field(form, "w")), h: num(field(form, "h")) };
2828 }
2929
30 function editOf(op: string, form: Form): Edit | undefined {
30 /** A panel to remove: from the form, or from the query the confirm page posts to (#8030). */
31 function removeOf(form: Form, query: URLSearchParams): Edit {
32 const fromForm = field(form, "index") !== "";
33 const index = fromForm ? field(form, "index") : query.get("index") ?? "";
34 const title = fromForm ? field(form, "title") : query.get("title") ?? "";
35 return title === "" ? { kind: "remove", index: num(index) } : { kind: "remove", index: num(index), title };
36 }
37
38 function editOf(op: string, form: Form, query: URLSearchParams): Edit | undefined {
3139 switch (op) {
3240 case "rename": return { kind: "title", title: field(form, "title") };
3341 case "settings": return { kind: "settings", range: field(form, "range"), refresh: field(form, "refresh") };
3442 case "add": return { kind: "add", panel: panelOf(form) };
3543 case "edit": return { kind: "edit", index: num(field(form, "index")), panel: panelOf(form) };
36 case "remove": return { kind: "remove", index: num(field(form, "index")) };
44 case "remove": return removeOf(form, query);
3745 case "move": {
3846 const by = num(field(form, "by"));
3947 return by === 1 || by === -1 ? { kind: "move", index: num(field(form, "index")), by } : undefined;
@@ -82,7 +90,7 @@ export async function boardPost(ctx: Ctx): Promise<Response> {
8290 const gone = await deleteBoard(ctx.app, ctx.auth.user, slug);
8391 return gone.ok ? done(ctx, "/boards", "Board deleted.") : failed(ctx, back, gone.error);
8492 }
85 const edit = editOf(op, form);
93 const edit = editOf(op, form, ctx.url.searchParams);
8694 if (!edit) return notFoundPage(ctx);
8795 const changed = await changeBoard(ctx.app, ctx.auth.user, slug, edit);
8896 return changed.ok ? done(ctx, back, "Saved.") : failed(ctx, back, changed.error);
src/web/pages/boards-view.ts +1 −4
@@ -159,10 +159,7 @@ function manage(slug: string, board: Board, adding: Html, pins: ReturnType<typeo
159159 ${btn("Up", { size: "sm", name: "by", value: "-1", label: `Move ${p.title} up` })}
160160 ${btn("Down", { size: "sm", name: "by", value: "1", label: `Move ${p.title} down` })}
161161 </form>
162 <form method="post" action="/boards/${slug}/remove">
163 <input type="hidden" name="index" value="${String(i)}" />
164 ${btn("Remove", { size: "sm", variant: "danger", label: `Remove ${p.title}` })}
165 </form>
162 ${confirmAction({ action: `/boards/${slug}/remove?index=${String(i)}&title=${encodeURIComponent(p.title)}`, verb: "Remove", size: "sm", label: `Remove ${p.title}` })}
junipercrew outdated

Every row's Remove link now has the same name, "Remove". Add a label option to confirmAction (src/web/ui.ts:1260) and pass Remove ${p.title} here, as Up and Down do above.
166163 </div>
167164 </li>`;
168165 return html`${card(
src/web/pages/search.ts +18
@@ -634,6 +634,15 @@ const DESTRUCTIVE: {
634634 phrase: "",
635635 }),
636636 },
637 {
638 pattern: /^\/boards\/([^/]+)\/remove\?index=(\d+)&title=([^&]+)$/,
639 describe: (m) => ({
640 verb: "Remove panel",
641 title: `Remove the panel ${decodedOr(m[3] ?? "")} from ${decodedOr(m[1] ?? "")}?`,
642 detail: "Only this panel goes; the board, its other panels and what it reads stay. A pin of it on home goes with it.",
643 phrase: "",
644 }),
645 },
637646 {
638647 pattern: /^\/([^/]+)\/([^/]+)\/settings\/hooks\/([^/]+)\/delete$/,
639648 describe: (m) => ({
@@ -797,6 +806,15 @@ const DESTRUCTIVE: {
797806 },
798807 ];
799808
809 /** A path segment decoded, or as it came when it is not valid percent-encoding. */
810 function decodedOr(s: string): string {
811 try {
812 return decodeURIComponent(s);
813 } catch {
814 return s;
815 }
816 }
817
800818 function describeTarget(to: string, ctx: Ctx): Destructive | undefined {
801819 for (const entry of DESTRUCTIVE) {
802820 const match = to.match(entry.pattern);
src/web/ui.ts +3
@@ -1263,11 +1263,14 @@ export function confirmAction(o: {
12631263 /** The label on this button only — the confirm page words itself. */
12641264 verb: string;
12651265 size?: "sm" | "lg";
1266 /** The accessible name, when the verb alone is ambiguous (one of many rows). */
1267 label?: string;
12661268 }): Html {
12671269 return btn(o.verb, {
12681270 href: `/-/confirm?to=${encodeURIComponent(o.action)}`,
12691271 variant: "danger",
12701272 ...(o.size ? { size: o.size } : {}),
1273 ...(o.label ? { label: o.label } : {}),
12711274 });
12721275 }
12731276
test/boards-store.test.ts +2 −1
@@ -174,10 +174,11 @@ describe("/boards", () => {
174174 expect(body).toContain('class="row-trail bar"');
175175 expect(body).toContain('class="list setting-list"');
176176 expect(body).toContain('action="/boards/pin-panel"');
177 for (const op of ["move", "remove", "rename", "settings", "duplicate"]) {
177 for (const op of ["move", "rename", "settings", "duplicate"]) {
178178 expect(body).toContain(`action="/boards/fleet-health/${op}"`);
179179 }
180180 expect(body).toContain(`href="/-/confirm?to=${encodeURIComponent("/boards/fleet-health/delete")}"`);
181 expect(body).toContain(`href="/-/confirm?to=${encodeURIComponent("/boards/fleet-health/remove?index=0")}`);
181182 for (const word of ["Panels", "Add a panel", "Up", "Down", "Remove", "Pin to home", "Save settings", "Duplicate", "Delete this board"]) {
182183 expect(body).toContain(word);
183184 }
test/confirm-board-panel.test.ts added +104
@@ -0,0 +1,104 @@
1 /** Remove on a board panel asks first, on the confirm page, which names the panel; a board reordered since loses nothing (#8030). */
2 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
3 import { mkdtemp, rm } from "node:fs/promises";
4 import { tmpdir } from "node:os";
5 import { join } from "node:path";
6 import { changeBoard, makeBoard, showBoard } from "../src/actions/boards.ts";
7 import { applyEdit } from "../src/boards/edit.ts";
8 import { emptyBoard, type Board } from "../src/boards/store.ts";
9 import { createApp, type App } from "../src/app.ts";
10 import { createSession } from "../src/core/sessions.ts";
11 import { createUser, type User } from "../src/core/users.ts";
12 import { serve, type Servers } from "../src/http/server.ts";
13 import { quietLogs } from "./helpers/globals.ts";
14
15 quietLogs();
16
17 const panel = (title: string) => ({ title, chart: "line", query: { source: "runs", aggregate: "count", groupBy: "kind" }, w: 1, h: 1 });
18
19 describe("Remove on a board panel goes through the confirm page", () => {
20 let dataDir = "";
21 let app: App;
22 let servers: Servers;
23 let alice: User;
24 let cookie = "";
25 let slug = "";
26 const confirmHref = (to: string) => `/-/confirm?to=${encodeURIComponent(to)}`;
27 const get = (path: string, extra = "") =>
28 fetch(`${servers.url}${path}`, { redirect: "manual", headers: { cookie: extra ? `${cookie}; ${extra}` : cookie } });
29 const post = (path: string) =>
30 fetch(`${servers.url}${path}`, { method: "POST", redirect: "manual", headers: { cookie, origin: servers.url } });
31 const titles = async () => {
32 const b = await showBoard(app, alice, slug);
33 if (!b.ok) throw new Error(b.error);
34 return b.value.panels.map((p) => p.title);
35 };
36 /** The page a redirect lands on, with the flash it carried. */
37 const landed = async (res: Response) => {
38 const flash = (res.headers.get("set-cookie") ?? "").split(";")[0] ?? "";
39 return (await get(res.headers.get("location") ?? "/", flash)).text();
40 };
41
42 beforeAll(async () => {
43 dataDir = await mkdtemp(join(tmpdir(), "forge-confirm-board-panel-"));
44 app = createApp({ dataDir, port: 0 });
45 const made = await createUser(app.db, {
46 origin: "seed", trusted: true, username: "alice", email: "[email protected]", password: "a-good-passphrase", adminIfFirst: true,
47 });
48 if (!made.ok) throw new Error(made.error);
49 alice = made.value;
50 const board = await makeBoard(app, alice, "Ops");
51 if (!board.ok) throw new Error(String(board.error));
52 slug = board.value;
53 for (const t of ["Runs & kinds", "Second", "Third"]) {
54 const added = await changeBoard(app, alice, slug, { kind: "add", panel: panel(t) });
55 if (!added.ok) throw new Error(String(added.error));
56 }
57 cookie = `forge_session=${createSession(app.db, alice.id).token}`;
58 servers = await serve(app);
59 }, 60_000);
60 afterAll(async () => {
61 servers.stop();
62 app.close();
63 await rm(dataDir, { recursive: true, force: true });
64 });
65
66 test("the board page links Remove to the confirm page, not a form that posts at once", async () => {
67 const page = await (await get(`/boards/${slug}`)).text();
68 const to = `/boards/${slug}/remove?index=0&title=${encodeURIComponent("Runs & kinds")}`;
69 expect(page).toContain(`href="${confirmHref(to)}"`);
70 expect(page).not.toContain(`action="/boards/${slug}/remove"`);
71 expect(page).toContain('aria-label="Remove Runs &amp; kinds"');
72 expect(page).toContain('aria-label="Remove Second"');
73 });
74
75 test("the confirm page names the panel and the board, escaped", async () => {
76 const to = `/boards/${slug}/remove?index=0&title=${encodeURIComponent("Runs & kinds")}`;
77 const res = await get(confirmHref(to));
78 expect(res.status).toBe(200);
79 const page = await res.text();
80 expect(page).toContain(`Remove the panel Runs &amp; kinds from ${slug}?`);
81 expect(page).toContain("Only this panel goes");
82 });
83
84 test("a title no longer at that index removes nothing and says so", async () => {
85 const res = await post(`/boards/${slug}/remove?index=0&title=Third`);
86 expect(res.status).toBe(303);
87 expect(await titles()).toEqual(["Runs & kinds", "Second", "Third"]);
88 expect(await landed(res)).toContain("that panel moved; nothing was removed");
89 });
90
91 test("the POST from the confirm page removes that panel", async () => {
92 const res = await post(`/boards/${slug}/remove?index=1&title=Second`);
93 expect(res.status).toBe(303);
94 expect(await titles()).toEqual(["Runs & kinds", "Third"]);
95 });
96
97 test("without a title, remove by index behaves as before", () => {
98 const board = { ...emptyBoard("B"), panels: [panel("A"), panel("B")] } as Board;
99 const gone = applyEdit(board, { kind: "remove", index: 0 });
100 expect(gone.ok && gone.value.panels.map((p) => p.title)).toEqual(["B"]);
101 const moved = applyEdit(board, { kind: "remove", index: 0, title: "B" });
102 expect(moved).toEqual({ ok: false, error: "that panel moved; nothing was removed" });
103 });
104 });
test/danger-buttons.test.ts −1
@@ -11,7 +11,6 @@ const web = join(root, "src", "web");
1111
1212 /** Today's danger POST buttons that do not ask first, one `file — reason` per line. */
1313 const allowed: readonly { file: string; button: string; reason: string }[] = [
14 { file: "src/web/pages/boards-view.ts", button: `"Remove"`, reason: "board panel Remove; the panel index is in the body" },
1514 { file: "src/web/pages/boards-view.ts", button: `"Delete this board"`, reason: "board delete; waits for its confirm issue" },
1615 { file: "src/web/pages/org.ts", button: `"Remove"`, reason: "team member remove; waits for its confirm issue" },
1716 { file: "src/web/pages/edit.ts", button: `"Delete this file"`, reason: "the delete page is the confirmation, reached by a link" },

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs