From 511fed408c3afbf6a5d40db6ba45e8421fc050c1 Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Thu, 3 Sep 2026 22:08:06 +0100 Subject: [PATCH] feat(maestro): confirm every kill in the web client The TUI has always raised `KillConfirm` before `x` kills anything; the web sent `DELETE /api/sessions/:id` on the first press and only prompted when the daemon refused. The command bar puts both behind the same three keystrokes, so the difference now reads as a bug (#160). The web follows the TUI: a kill is destructive and unattended agents live in these sessions. `killDialog.ts` grows a confirmation that shares the force prompt's shell, names the blockers the card already knows about, and keeps the focus on Cancel. The guarded refusal still opens its own dialog behind it. --- docs/COMMAND-BAR.md | 12 ++--- docs/SIDEBAR.md | 17 +++++- web/src/barActions.test.ts | 10 +++- web/src/catalog.test.ts | 41 ++++++++++++++- web/src/catalog.ts | 55 ++++++++++--------- web/src/killDialog.ts | 105 +++++++++++++++++++++++++++++-------- web/src/sidebarNav.test.ts | 4 +- 7 files changed, 188 insertions(+), 56 deletions(-) diff --git a/docs/COMMAND-BAR.md b/docs/COMMAND-BAR.md index 1a38c9d..0fdae05 100644 --- a/docs/COMMAND-BAR.md +++ b/docs/COMMAND-BAR.md @@ -158,12 +158,12 @@ searchable — the query line keeps working, now over those actions. ambiguous. - Escape, or backspace at an empty query, returns to the result list **with the previous query intact**. Drilling in and backing out is free. -- Each action raises the flow it already has: kill goes through the existing - confirm-and-blockers dialog (`docs/WORKTREES.md`), rename through the - existing prompt pre-filled with the session's own name - (`docs/SIDEBAR.md`), spawn similar through the spawn form seeded from the - session's directory and command. The bar picks the action; it never - reimplements one. +- Each action raises the flow it already has: kill through the confirmation + both clients ask for (`docs/SIDEBAR.md`) and the blockers dialog behind it + (`docs/WORKTREES.md`), rename through the existing prompt pre-filled with the + session's own name (`docs/SIDEBAR.md`), spawn similar through the spawn form + seeded from the session's directory and command. The bar picks the action; it + never reimplements one. No actions beyond those three, and no action across more than one session. diff --git a/docs/SIDEBAR.md b/docs/SIDEBAR.md index ca339bf..8cc4fb2 100644 --- a/docs/SIDEBAR.md +++ b/docs/SIDEBAR.md @@ -316,7 +316,7 @@ Everything else is the same key on both clients: | `Tab` | hand the keyboard to the pane | | `c` | collapse or expand the highlighted folder | | `r` | rename the highlighted session | -| `x` | kill the highlighted session | +| `x` | kill the highlighted session (confirms) | | `n` | open the spawn form | | `N` | spawn with every default resolved | | `?` | show the shortcut overlay | @@ -324,3 +324,18 @@ Everything else is the same key on both clients: The TUI binds these through its keymap, so a user can rebind any of them; the web client's are fixed. A binding added to one client belongs in this table and in both help overlays. + +## Killing a session + +**Every kill confirms first, in both clients** (#160). A kill is destructive +and unattended agents live in these sessions, so neither client sends +`DELETE /api/sessions/:id` off a single press. The TUI raises +`OverlayKind::KillConfirm` (`[y]`/`[n]`); the web raises the same dialog its +`✕` button and the command bar's action page both use. Cancel holds the focus +in the web's dialog, so nothing kills on a stray Enter. + +The confirmation names what the card last saw in the way, when anything was — +the daemon will refuse an unforced kill over such a worktree, and saying so +before the attempt is cheaper than a refusal. What comes back from that refusal +is a second, separate prompt offering `force=true`; `docs/WORKTREES.md` owns +what a forced kill leaves behind. diff --git a/web/src/barActions.test.ts b/web/src/barActions.test.ts index 0720de6..6a19097 100644 --- a/web/src/barActions.test.ts +++ b/web/src/barActions.test.ts @@ -65,7 +65,7 @@ async function flush(): Promise { } describe("kill from the action page", () => { - it("goes through the daemon, without a first confirmation", async () => { + it("goes through the same confirmation the sidebar raises (#160)", async () => { const calls: string[] = []; window.fetch = ((url: string, init?: RequestInit) => { calls.push(`${init?.method ?? "GET"} ${url}`); @@ -75,6 +75,12 @@ describe("kill from the action page", () => { mount(s); raise(KILL_SESSION_EVENT, s); await flush(); + expect(document.querySelector(".kill-dialog")?.getAttribute("aria-label")).toBe( + "Kill amber-falcon", + ); + expect(calls.filter((c) => c.startsWith("DELETE"))).toHaveLength(0); + document.querySelector(".kill-dialog-kill")?.click(); + await flush(); expect(calls).toContain(`DELETE /api/sessions/${s.id}`); expect(document.querySelector(".kill-dialog")).toBeNull(); }); @@ -101,6 +107,8 @@ describe("kill from the action page", () => { mount(s); raise(KILL_SESSION_EVENT, s); await flush(); + document.querySelector(".kill-dialog-kill")?.click(); + await flush(); const dialog = document.querySelector(".kill-dialog"); expect(dialog?.getAttribute("aria-label")).toBe("Kill amber-falcon"); expect(dialog?.textContent).toContain("1 untracked file"); diff --git a/web/src/catalog.test.ts b/web/src/catalog.test.ts index b81e5f7..9447f5c 100644 --- a/web/src/catalog.test.ts +++ b/web/src/catalog.test.ts @@ -98,7 +98,7 @@ describe("blocked-work indicator (#83)", () => { expect(rowFor(el, unprobed.id)?.querySelector(".leaf-blocked")).toBeNull(); }); - it("kills straight away, without a first confirmation", async () => { + it("confirms before an unguarded kill (#160)", async () => { const calls: string[] = []; window.fetch = ((url: string, init?: RequestInit) => { calls.push(`${init?.method ?? "GET"} ${url}`); @@ -108,10 +108,45 @@ describe("blocked-work indicator (#83)", () => { const el = renderSidebar(storeWith([plain])).el; rowFor(el, plain.id)?.querySelector(".leaf-kill")?.click(); await flush(); + const dialog = document.querySelector(".kill-dialog"); + expect(dialog?.getAttribute("aria-label")).toBe("Kill claude"); + expect(calls.filter((c) => c.startsWith("DELETE"))).toHaveLength(0); + dialog?.querySelector(".kill-dialog-kill")?.click(); + await flush(); expect(calls).toContain(`DELETE /api/sessions/${plain.id}`); expect(document.querySelector(".kill-dialog")).toBeNull(); }); + it("kills nothing when the confirmation is cancelled (#160)", async () => { + const calls: string[] = []; + window.fetch = ((url: string, init?: RequestInit) => { + calls.push(`${init?.method ?? "GET"} ${url}`); + return Promise.resolve(new Response(null, { status: 204 })); + }) as typeof fetch; + const plain = session({ id: "aaaaaaaa-0000-0000-0000-000000000007" }); + const el = renderSidebar(storeWith([plain])).el; + rowFor(el, plain.id)?.querySelector(".leaf-kill")?.click(); + await flush(); + document.querySelector(".kill-dialog-cancel")?.click(); + await flush(); + expect(document.querySelector(".kill-dialog")).toBeNull(); + expect(calls.filter((c) => c.startsWith("DELETE"))).toHaveLength(0); + }); + + it("names the blockers in the confirmation, before the daemon refuses (#160)", async () => { + window.fetch = (() => Promise.reject(new Error("never called"))) as typeof fetch; + const blocked = session({ + id: "aaaaaaaa-0000-0000-0000-000000000008", + blocked_work: { untracked_files: 1 }, + }); + const el = renderSidebar(storeWith([blocked])).el; + rowFor(el, blocked.id)?.querySelector(".leaf-kill")?.click(); + await flush(); + const dialog = document.querySelector(".kill-dialog"); + expect(dialog?.textContent).toContain("1 untracked file"); + expect(dialog?.textContent).toContain("refused unless it is forced"); + }); + it("offers the override when the daemon refuses a guarded kill", async () => { const calls: string[] = []; window.fetch = ((url: string, init?: RequestInit) => { @@ -138,6 +173,8 @@ describe("blocked-work indicator (#83)", () => { const el = renderSidebar(storeWith([blocked])).el; rowFor(el, blocked.id)?.querySelector(".leaf-kill")?.click(); await flush(); + document.querySelector(".kill-dialog-kill")?.click(); + await flush(); const dialog = document.querySelector(".kill-dialog"); expect(dialog?.textContent).toContain("1 untracked file"); dialog?.querySelector(".kill-dialog-force")?.click(); @@ -166,6 +203,8 @@ describe("blocked-work indicator (#83)", () => { const el = renderSidebar(storeWith([blocked])).el; rowFor(el, blocked.id)?.querySelector(".leaf-kill")?.click(); await flush(); + document.querySelector(".kill-dialog-kill")?.click(); + await flush(); document.querySelector(".kill-dialog-cancel")?.click(); expect(document.querySelector(".kill-dialog")).toBeNull(); expect(calls.filter((c) => c.includes("force=true"))).toHaveLength(0); diff --git a/web/src/catalog.ts b/web/src/catalog.ts index 6461401..ab8364c 100644 --- a/web/src/catalog.ts +++ b/web/src/catalog.ts @@ -1,6 +1,6 @@ import { escapeHtml } from "./html.js"; import { ApiRequestError, getDaemonInfo, killSession, renameSession } from "./api.js"; -import { openForceKillDialog } from "./killDialog.js"; +import { openForceKillDialog, openKillConfirmDialog } from "./killDialog.js"; import { openContextMenu } from "./contextMenu.js"; import { toggleHelp } from "./help.js"; import { CommandSet, defineCommands } from "./commands.js"; @@ -230,7 +230,7 @@ function renderNode(node: TreeNode, depth: number): HTMLElement { killBtn.addEventListener("click", (ev) => { ev.preventDefault(); ev.stopPropagation(); - void requestKill(cur, sessionName(cur)); + requestKill(cur, sessionName(cur)); }); row.addEventListener("click", () => { document.dispatchEvent(new CustomEvent(SESSION_SELECT_EVENT, { detail: cur.id })); @@ -248,7 +248,7 @@ function renderNode(node: TreeNode, depth: number): HTMLElement { { label: "Kill session", danger: true, - onSelect: () => void requestKill(cur, sessionName(cur)), + onSelect: () => requestKill(cur, sessionName(cur)), }, ]); }); @@ -293,26 +293,33 @@ function treeSignature(node: TreeNode): string { } /** - * Kill on the first click (#81): the daemon is the one that decides whether a - * session may go, and it refuses over a worktree holding unsaved work. Only - * that refusal is worth a prompt, and it needs one `confirm()` cannot give — - * a forced kill. Anything else surfaces as an alert. + * Kill behind a confirmation (#160): a kill is destructive and unattended + * agents live in these sessions, so the web asks first, the way the TUI always + * has. The daemon decides the rest — it refuses a kill over a worktree holding + * unsaved work, and that refusal needs a prompt the confirmation cannot give, + * a forced kill. Anything else surfaces in the dialog that asked. */ -async function requestKill(s: Session, name: string): Promise { - try { - await killSession(s.id); - } catch (e) { - if (e instanceof ApiRequestError && e.code === "kill_guarded") { - openForceKillDialog({ - name, - blockers: e.unsavedWork ?? sessionBlockers(s), - reason: e.detail, - onForce: () => killSession(s.id, true), - }); - return; - } - alert(`Failed to kill: ${(e as Error).message}`); - } +function requestKill(s: Session, name: string): void { + openKillConfirmDialog({ + name, + blockers: sessionBlockers(s), + onKill: async () => { + try { + await killSession(s.id); + } catch (e) { + if (e instanceof ApiRequestError && e.code === "kill_guarded") { + openForceKillDialog({ + name, + blockers: e.unsavedWork ?? sessionBlockers(s), + reason: e.detail, + onForce: () => killSession(s.id, true), + }); + return; + } + throw e; + } + }, + }); } /** @@ -719,7 +726,7 @@ export function renderSidebar(store: CatalogStore, opts: SidebarOptions = {}): S }, "list.kill": () => { const hit = highlightedSession(); - if (hit) void requestKill(hit.session, sessionName(hit.session)); + if (hit) requestKill(hit.session, sessionName(hit.session)); }, "list.focusPane": () => document.dispatchEvent(new CustomEvent(FOCUS_PANE_EVENT)), }), @@ -782,7 +789,7 @@ export function renderSidebar(store: CatalogStore, opts: SidebarOptions = {}): S // than by row. const killHandler = (ev: Event) => { const s = (ev as CustomEvent).detail; - void requestKill(s, sessionName(s)); + requestKill(s, sessionName(s)); }; const renameHandler = (ev: Event) => { const s = (ev as CustomEvent).detail; diff --git a/web/src/killDialog.ts b/web/src/killDialog.ts index 3597f82..1962be6 100644 --- a/web/src/killDialog.ts +++ b/web/src/killDialog.ts @@ -1,14 +1,44 @@ /** - * Forced-kill prompt (#81). + * The two kill prompts (#81, #160). * - * The daemon refuses to kill a session whose worktree still holds uncommitted - * or unpushed work. `confirm()` can only ask the same question twice, so the - * refusal gets its own dialog: it names what is in the way and offers the - * override the daemon's `force=true` already implements. A kill the daemon - * accepts never reaches here — there is nothing to warn about. + * A kill is destructive and unattended agents live in these sessions, so both + * clients ask before one goes through — the TUI's `KillConfirm`, and + * `openKillConfirmDialog` here (#160). Where the session's worktree still + * holds uncommitted or unpushed work, the confirmation says so up front, the + * same way the TUI's does. + * + * The daemon decides the rest: it refuses a kill over such a worktree, and + * that refusal needs an answer the confirmation cannot give — a forced kill. + * `openForceKillDialog` is that second prompt. A kill the daemon accepts never + * reaches it. */ import { formatInTheWay, type InTheWay } from "./types.js"; +/** What both prompts need: a target to name and a button that acts on it. */ +interface KillDialogSpec { + /** Session name, as the sidebar shows it. */ + name: string; + /** Sentence under the title. */ + body: string; + /** Label on the acting button. */ + confirmLabel: string; + /** Extra class on the acting button, so tests and styles can find it. */ + confirmClass: string; + /** Run it. A rejection is rendered in the dialog, which stays open so the + * press can be retried. */ + onConfirm: () => Promise; +} + +export interface KillConfirmPrompt { + name: string; + /** What the card last saw in the way, when anything was. Shown before the + * attempt, so "this will be refused" is not a surprise. */ + blockers: InTheWay | null; + /** Run the plain kill. Opening a further dialog from here is allowed: the + * confirmation only closes itself if it is still the one on screen. */ + onKill: () => Promise; +} + export interface ForceKillPrompt { /** Session name, as the sidebar shows it. */ name: string; @@ -30,9 +60,9 @@ export function closeForceKillDialog(): void { open = null; } -/** Mount the prompt. Opening while one is up replaces it, so a second refusal - * can never stack two dialogs. */ -export function openForceKillDialog(p: ForceKillPrompt): HTMLElement { +/** Mount one of the prompts. Opening while one is up replaces it, so the + * confirmation and a refusal that follows it can never stack. */ +function mount(p: KillDialogSpec): HTMLElement { closeForceKillDialog(); const el = document.createElement("div"); @@ -50,9 +80,7 @@ export function openForceKillDialog(p: ForceKillPrompt): HTMLElement { const body = document.createElement("p"); body.className = "kill-dialog-body"; - body.textContent = p.blockers - ? `Its worktree still has ${formatInTheWay(p.blockers)}. Forcing the kill removes the session and leaves the worktree on disk.` - : `${p.reason} Forcing the kill removes the session and leaves the worktree on disk.`; + body.textContent = p.body; const error = document.createElement("p"); error.className = "kill-dialog-error"; @@ -65,22 +93,27 @@ export function openForceKillDialog(p: ForceKillPrompt): HTMLElement { cancel.className = "btn btn-ghost kill-dialog-cancel"; cancel.textContent = "Cancel"; cancel.addEventListener("click", closeForceKillDialog); - const force = document.createElement("button"); - force.type = "button"; - force.className = "btn btn-danger kill-dialog-force"; - force.textContent = "Force kill"; - force.addEventListener("click", () => { - force.disabled = true; + const confirm = document.createElement("button"); + confirm.type = "button"; + confirm.className = `btn btn-danger ${p.confirmClass}`; + confirm.textContent = p.confirmLabel; + confirm.addEventListener("click", () => { + confirm.disabled = true; void p - .onForce() - .then(closeForceKillDialog) + .onConfirm() + .then(() => { + // A handler that opened the refusal prompt has already replaced this + // dialog; closing here would take that one down with it. + if (open?.el === el) closeForceKillDialog(); + }) .catch((e: unknown) => { - force.disabled = false; + if (open?.el !== el) return; + confirm.disabled = false; error.hidden = false; error.textContent = `Failed to kill: ${(e as Error).message}`; }); }); - actions.append(cancel, force); + actions.append(cancel, confirm); panel.append(title, body, error, actions); @@ -99,6 +132,34 @@ export function openForceKillDialog(p: ForceKillPrompt): HTMLElement { }; window.addEventListener("keydown", onKeydown, true); open = { el, onKeydown }; + // Cancel takes the focus in both prompts: a kill is never one keystroke + // away from whatever asked for the dialog. cancel.focus(); return el; } + +/** Ask before a kill the daemon has not seen yet (#160). */ +export function openKillConfirmDialog(p: KillConfirmPrompt): HTMLElement { + return mount({ + name: p.name, + body: p.blockers + ? `Its worktree still has ${formatInTheWay(p.blockers)}. The kill will be refused unless it is forced.` + : "The session and everything running in it go away. Its worktree stays on disk.", + confirmLabel: "Kill", + confirmClass: "kill-dialog-kill", + onConfirm: p.onKill, + }); +} + +/** Offer the override after the daemon refuses a guarded kill (#81). */ +export function openForceKillDialog(p: ForceKillPrompt): HTMLElement { + return mount({ + name: p.name, + body: p.blockers + ? `Its worktree still has ${formatInTheWay(p.blockers)}. Forcing the kill removes the session and leaves the worktree on disk.` + : `${p.reason} Forcing the kill removes the session and leaves the worktree on disk.`, + confirmLabel: "Force kill", + confirmClass: "kill-dialog-force", + onConfirm: p.onForce, + }); +} diff --git a/web/src/sidebarNav.test.ts b/web/src/sidebarNav.test.ts index 46cb1d9..4e836ac 100644 --- a/web/src/sidebarNav.test.ts +++ b/web/src/sidebarNav.test.ts @@ -143,7 +143,7 @@ describe("sidebar keyboard navigation", () => { field!.dispatchEvent(new KeyboardEvent("keydown", { key: "Escape", bubbles: true })); }); - it("x kills the highlighted session", async () => { + it("x kills the highlighted session, once confirmed (#160)", async () => { const { el } = twoFolders(); const killed: string[] = []; window.fetch = ((input: RequestInfo | URL, init?: RequestInit) => { @@ -153,6 +153,8 @@ describe("sidebar keyboard navigation", () => { press(tree(el), "j"); press(tree(el), "x"); + expect(killed.some((r) => r.startsWith("DELETE"))).toBe(false); + document.querySelector(".kill-dialog-kill")?.click(); await Promise.resolve(); expect(killed.some((r) => r.startsWith("DELETE") && r.includes(A))).toBe(true); });