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