diff --git a/web/src/connections.test.ts b/web/src/connections.test.ts index 9464504..20d03de 100644 --- a/web/src/connections.test.ts +++ b/web/src/connections.test.ts @@ -2,28 +2,78 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { ConnectionPool } from "./connections.js"; +import { CatalogStore } from "./events.js"; +import type { Session } from "./types.js"; // The pane's own bytes-out path, stubbed: the escape hatch only needs to know // the pool reaches the focused connection with them. const sent: { id: string; data: string }[] = []; -vi.mock("./attach.js", () => ({ - createAttachConnection: (id: string) => ({ +const disposed: string[] = []; +function fakeConn(kind: string, id: string) { + const el = document.createElement("div"); + el.dataset.kind = kind; + el.dataset.session = id; + return { sessionId: id, - el: document.createElement("div"), + kind, + el, onMount() {}, onUnmount() {}, focusTerminal() {}, sendInput(data: string) { sent.push({ id, data }); }, - dispose() {}, - }), + dispose() { + disposed.push(`${kind}:${id}`); + }, + }; +} +vi.mock("./attach.js", () => ({ + createAttachConnection: (id: string) => fakeConn("attach", id), +})); +vi.mock("./structured.js", () => ({ + createStructuredConnection: (id: string) => fakeConn("structured", id), })); afterEach(() => { vi.unstubAllGlobals(); + sent.length = 0; + disposed.length = 0; }); +/** Minimal fixture for the `CatalogState` a `CatalogStore` broadcasts. */ +function session(id: string, view?: "Terminal" | "Structured"): Session { + return { + id, + cwd: "/repo", + command: "claude", + env: {}, + metadata: {}, + status: "Running", + created_at: "2026-01-01T00:00:00Z", + view, + }; +} + +/** A `CatalogStore` an id can be pushed into on demand, mirroring + * events.test.ts's own injectable-socket pattern. */ +class FakeCatalogSocket { + onmessage: ((ev: { data: string }) => void) | null = null; + onopen: (() => void) | null = null; + onclose: (() => void) | null = null; + onerror: (() => void) | null = null; + close(): void {} + push(sessions: Session[]): void { + this.onmessage?.({ data: JSON.stringify({ type: "Snapshot", sessions }) }); + } +} +function fakeStore(): { store: CatalogStore; socket: FakeCatalogSocket } { + const socket = new FakeCatalogSocket(); + const store = new CatalogStore({ socket: () => socket as unknown as WebSocket }); + store.start(); + return { store, socket }; +} + const A = "aaaaaaaa-0000-0000-0000-0000000000ff"; const B = "bbbbbbbb-0000-0000-0000-0000000000ff"; @@ -125,3 +175,85 @@ describe("ConnectionPool.sendToFocused", () => { expect(sent).toEqual([]); }); }); + +// A deep link or restored layout can `mount()` a session before the catalog +// snapshot names its view — the daemon defaults an absent `view` to +// `"Terminal"`, but so does an *unknown* session, and only one of those is +// actually resolved (#140 review). +describe("ConnectionPool: view resolution races", () => { + it("replaces a wrongly-typed connection once the catalog resolves it, without waiting for another mount()", () => { + mockFetch(); + const { store, socket } = fakeStore(); + const pool = new ConnectionPool({ store }); + const pane = document.createElement("div"); + + pool.mount(A, pane); // no info: deep-link-before-snapshot race + expect(disposed).toEqual([]); + + socket.push([session(A, "Structured")]); + + expect(disposed).toEqual(["attach:" + A]); + // The fresh connection landed in the same pane nothing re-mounted it into. + expect(pane.children).toHaveLength(1); + }); + + it("also repairs a wrongly-typed connection that is parked, not mounted, without evicting its other parked siblings", () => { + mockFetch(); + const { store, socket } = fakeStore(); + const pool = new ConnectionPool({ store }); + const hidden = document.body.lastElementChild as HTMLElement; // this pool's own cache + pool.mount(A, document.createElement("div")); + pool.mount(B, document.createElement("div")); + pool.unmount(A); + pool.unmount(B); + disposed.length = 0; + + socket.push([session(A, "Structured")]); + + expect(disposed).toEqual(["attach:" + A]); + // B's own element is still parked — A's repair didn't wipe its siblings + // out of the shared hidden cache. + expect(hidden.querySelector(`[data-session="${B}"]`)).not.toBeNull(); + expect((hidden.querySelector(`[data-session="${A}"]`) as HTMLElement | null)?.dataset.kind).toBe("structured"); + }); + + it("does not replace a connection whose resolved view already matches", () => { + mockFetch(); + const { store, socket } = fakeStore(); + const pool = new ConnectionPool({ store }); + const pane = document.createElement("div"); + pool.mount(A, pane, { name: "a", command: "claude", view: "Structured" }); + disposed.length = 0; + + socket.push([session(A, "Structured")]); + + expect(disposed).toEqual([]); + }); + + it("does not downgrade an already-resolved connection when a later mount() call has no info", () => { + mockFetch(); + const pool = new ConnectionPool(); + const pane = document.createElement("div"); + pool.mount(A, pane, { name: "a", command: "claude", view: "Structured" }); + disposed.length = 0; + + pool.mount(A, pane); // e.g. a re-render before the catalog answers again + + expect(disposed).toEqual([]); + }); + + it("does not treat a session missing from the catalog as Terminal and churn a live connection", () => { + mockFetch(); + const { store, socket } = fakeStore(); + const pool = new ConnectionPool({ store }); + const pane = document.createElement("div"); + pool.mount(A, pane, { name: "a", command: "claude", view: "Structured" }); + disposed.length = 0; + + // `A` is absent from this snapshot — main.ts's own subscription handles + // closing the pane; the pool must not race it by "fixing" the view first. + socket.push([session(B, "Terminal")]); + + expect(disposed).toEqual([]); + }); +}); diff --git a/web/src/connections.ts b/web/src/connections.ts index 1c37b2d..bcb6809 100644 --- a/web/src/connections.ts +++ b/web/src/connections.ts @@ -3,8 +3,8 @@ import type { AttachInfo } from "./paneTitle.js"; import type { PaneConnection } from "./pane.js"; import { createStructuredConnection } from "./structured.js"; import { setSessionFocus } from "./api.js"; -import type { CatalogStore } from "./events.js"; -import type { Uuid } from "./types.js"; +import type { CatalogState, CatalogStore } from "./events.js"; +import { sessionName, type SessionView, type Uuid } from "./types.js"; /** * How many *parked* (off-screen but still-connected) sessions to keep warm @@ -16,6 +16,15 @@ import type { Uuid } from "./types.js"; */ const MAX_PARKED = 6; +/** A session's view defaults to `"Terminal"` wherever it isn't carried — + * matching the wire default (`SessionView`'s `#[default]`, types.ts). Only + * meaningful once the session itself is known; an altogether-unknown + * session (no `AttachInfo`/`Session` at all) is a distinct, un-resolved + * state callers must check for separately, not "Terminal". */ +function resolveView(view: SessionView | undefined): SessionView { + return view ?? "Terminal"; +} + export interface PoolOptions { /** Fired when the focused session changes (drives sidebar highlight + HUD). */ onFocusChange?: (id: Uuid | null) => void; @@ -44,11 +53,18 @@ export interface PoolOptions { export class ConnectionPool { /** All live connections. Insertion order doubles as LRU: oldest first. */ private readonly cache = new Map(); + /** The view each cached connection was actually created for — a deep link + * or a restored layout can `mount()` a session before the catalog + * snapshot names its view, defaulting it to `"Terminal"`; this is what + * lets a later reconciliation notice the resolved view disagrees (#140 + * review). */ + private readonly connViews = new Map(); /** Sessions currently mounted into a visible pane (never evicted). */ private readonly mounted = new Set(); private readonly hidden: HTMLElement; private focusedId: Uuid | null = null; private readonly opts: PoolOptions; + private readonly unsubscribeStore?: () => void; constructor(opts: PoolOptions = {}) { this.opts = opts; @@ -57,6 +73,11 @@ export class ConnectionPool { this.hidden.className = "conn-cache"; this.hidden.setAttribute("aria-hidden", "true"); document.body.appendChild(this.hidden); + // A session `mount()`ed before its catalog entry exists gets a + // `"Terminal"` connection by default; once the snapshot actually names + // the view, fix any connection that guessed wrong — nothing else + // re-`mount()`s an already-open pane just because the catalog updated. + this.unsubscribeStore = opts.store?.subscribe((state) => this.reconcileViews(state)); } /** The focused session, or null when nothing is focused (welcome pane). */ @@ -80,18 +101,18 @@ export class ConnectionPool { mount(id: Uuid, pane: HTMLElement, info?: AttachInfo, opts?: { focus?: boolean }): void { let conn = this.cache.get(id); if (conn) { - this.cache.delete(id); - this.cache.set(id, conn); + // `info === undefined` means "session not known yet", not "Terminal" — + // never downgrade a resolved connection just because a later caller + // couldn't resolve the session. Only a *resolved* view that disagrees + // with what this connection was built for forces a replace. + if (info !== undefined && resolveView(info.view) !== this.connViews.get(id)) { + conn = this.replace(id, info); + } else { + this.cache.delete(id); + this.cache.set(id, conn); // LRU refresh + } } else { - const connOpts = { - onRequestClose: (sid: Uuid) => this.opts.onRequestClose?.(sid), - onClosed: (sid: Uuid) => this.opts.onClosed?.(sid), - shouldReconnect: (sid: Uuid, code: number) => this.opts.shouldReconnect?.(sid, code) ?? false, - }; - conn = - info?.view === "Structured" - ? createStructuredConnection(id, connOpts, info, this.opts.store) - : createAttachConnection(id, connOpts, info); + conn = this.create(id, info); this.cache.set(id, conn); } if (conn.el.parentElement !== pane) { @@ -104,6 +125,57 @@ export class ConnectionPool { this.evict(); } + private create(id: Uuid, info?: AttachInfo): PaneConnection { + const view = resolveView(info?.view); + this.connViews.set(id, view); + const connOpts = { + onRequestClose: (sid: Uuid) => this.opts.onRequestClose?.(sid), + onClosed: (sid: Uuid) => this.opts.onClosed?.(sid), + shouldReconnect: (sid: Uuid, code: number) => this.opts.shouldReconnect?.(sid, code) ?? false, + }; + return view === "Structured" + ? createStructuredConnection(id, connOpts, info, this.opts.store) + : createAttachConnection(id, connOpts, info); + } + + /** Dispose whatever is cached for `id` and cache a freshly created + * connection in its place. Does not touch the DOM or `mounted` — callers + * place the new element themselves. */ + private replace(id: Uuid, info?: AttachInfo): PaneConnection { + this.cache.get(id)?.dispose(); + const fresh = this.create(id, info); + this.cache.set(id, fresh); + return fresh; + } + + /** Proactively fix any cached connection whose view no longer matches the + * catalog — the only path that repairs a pane nothing will `mount()` + * again on its own (parked, or mounted but not currently being + * re-rendered by anything else). A session absent from `state` (not yet + * known, or removed) is left alone — that's `main.ts`'s + * `closePane`/eviction concern, not a view mismatch. */ + private reconcileViews(state: CatalogState): void { + for (const id of [...this.cache.keys()]) { + const session = state.sessions.get(id); + if (session === undefined || resolveView(session.view) === this.connViews.get(id)) continue; + const existing = this.cache.get(id)!; + const parent = existing.el.parentElement; + const wasMounted = this.mounted.has(id); + const info: AttachInfo = { name: sessionName(session), command: session.command, view: session.view }; + const fresh = this.replace(id, info); + existing.el.remove(); // don't rely on dispose() to clear the DOM + if (parent && parent !== this.hidden) { + parent.replaceChildren(fresh.el); + } else { + // Parked: `this.hidden` holds every parked connection's element as + // siblings — append, never replaceChildren, or they'd all vanish. + this.hidden.appendChild(fresh.el); + } + if (wasMounted) fresh.onMount({ focus: false }); + else fresh.onUnmount(); + } + } + /** Park `id` back into the hidden cache (still connected) and stop showing it. */ unmount(id: Uuid): void { if (!this.mounted.delete(id)) return; @@ -180,13 +252,16 @@ export class ConnectionPool { if (!conn) return; conn.dispose(); this.cache.delete(id); + this.connViews.delete(id); this.mounted.delete(id); if (this.focusedId === id) this.setFocus(null); } dispose(): void { + this.unsubscribeStore?.(); for (const conn of this.cache.values()) conn.dispose(); this.cache.clear(); + this.connViews.clear(); this.mounted.clear(); this.hidden.remove(); } diff --git a/web/src/structured.test.ts b/web/src/structured.test.ts index b0c88d2..ba74be4 100644 --- a/web/src/structured.test.ts +++ b/web/src/structured.test.ts @@ -45,6 +45,12 @@ function row(id: string, kind: TranscriptRow["kind"], data: unknown): Transcript const idleState: ReducedState = { turn: { responding: false, thinking: false, active_tool_calls: [] } }; +const allApprovalOptions = [ + { optionId: "a1", name: "Allow once", kind: "allow_once" as const }, + { optionId: "a2", name: "Allow always", kind: "allow_always" as const }, + { optionId: "r1", name: "Reject once", kind: "reject_once" as const }, +]; + function mount(opts: { sockets: FakeSocket[]; shouldReconnect?: (id: string, code: number) => boolean; @@ -175,9 +181,9 @@ describe("structured connection: approvals", () => { type: "projection_update", transcript: { type: "append", - row: row("approval:1", "approval", { toolCall: { title: "Write x" }, options: [] }), + row: row("approval:1", "approval", { toolCall: { title: "Write x" }, options: allApprovalOptions }), }, - state: { ...idleState, pending_approval: { nonce: "n1", request: { sessionId: "s1", toolCall: {}, options: [] } } }, + state: { ...idleState, pending_approval: { nonce: "n1", request: { sessionId: "s1", toolCall: {}, options: allApprovalOptions } } }, }); const buttons = conn.el.querySelectorAll(".st-approval-acts button"); expect(buttons).toHaveLength(3); @@ -192,11 +198,11 @@ describe("structured connection: approvals", () => { ws.open(); ws.emit({ type: "transcript_snapshot", - snapshot: { cursor: 0, rows: [row("approval:1", "approval", { toolCall: { title: "Write x" }, options: [] })] }, + snapshot: { cursor: 0, rows: [row("approval:1", "approval", { toolCall: { title: "Write x" }, options: allApprovalOptions })] }, }); ws.emit({ type: "reduced_state", - state: { ...idleState, pending_approval: { nonce: "n1", request: { sessionId: "s1", toolCall: {}, options: [] } } }, + state: { ...idleState, pending_approval: { nonce: "n1", request: { sessionId: "s1", toolCall: {}, options: allApprovalOptions } } }, }); expect(conn.el.querySelectorAll(".st-approval-acts button")).toHaveLength(3); // Resolved on another client: state clears with no transcript delta at all. @@ -212,11 +218,11 @@ describe("structured connection: approvals", () => { ws.open(); ws.emit({ type: "transcript_snapshot", - snapshot: { cursor: 0, rows: [row("approval:1", "approval", { toolCall: {}, options: [] })] }, + snapshot: { cursor: 0, rows: [row("approval:1", "approval", { toolCall: {}, options: allApprovalOptions })] }, }); ws.emit({ type: "reduced_state", - state: { ...idleState, pending_approval: { nonce: "n1", request: { sessionId: "s1", toolCall: {}, options: [] } } }, + state: { ...idleState, pending_approval: { nonce: "n1", request: { sessionId: "s1", toolCall: {}, options: allApprovalOptions } } }, }); const buttons = conn.el.querySelectorAll(".st-approval-acts button"); // Approval is superseded before the click is handled (buttons already @@ -227,6 +233,65 @@ describe("structured connection: approvals", () => { expect(resolveStructuredApproval).not.toHaveBeenCalled(); conn.dispose(); }); + + it("restores the buttons and surfaces an error when the POST rejects while still pending", async () => { + vi.mocked(resolveStructuredApproval).mockRejectedValueOnce(new Error("network down")); + const ws = new FakeSocket(); + const conn = mount({ sockets: [ws] }); + ws.open(); + ws.emit({ + type: "transcript_snapshot", + snapshot: { cursor: 0, rows: [row("approval:1", "approval", { toolCall: {}, options: allApprovalOptions })] }, + }); + ws.emit({ + type: "reduced_state", + state: { ...idleState, pending_approval: { nonce: "n1", request: { sessionId: "s1", toolCall: {}, options: allApprovalOptions } } }, + }); + const buttons = conn.el.querySelectorAll(".st-approval-acts button"); + buttons[0].click(); + expect(buttons[0].disabled).toBe(true); + await Promise.resolve().then(() => Promise.resolve()).then(() => Promise.resolve()); + expect(buttons[0].disabled).toBe(false); + expect(buttons[1].disabled).toBe(false); + expect(conn.el.textContent).toContain("network down"); + conn.dispose(); + }); + + it("does not resurrect buttons or show an error for a decision the approval already moved past", async () => { + let reject!: (e: unknown) => void; + vi.mocked(resolveStructuredApproval).mockImplementationOnce( + () => new Promise((_, r) => { reject = r; }), + ); + const ws = new FakeSocket(); + const conn = mount({ sockets: [ws] }); + ws.open(); + ws.emit({ + type: "transcript_snapshot", + snapshot: { cursor: 0, rows: [row("approval:1", "approval", { toolCall: {}, options: allApprovalOptions })] }, + }); + ws.emit({ + type: "reduced_state", + state: { ...idleState, pending_approval: { nonce: "n1", request: { sessionId: "s1", toolCall: {}, options: allApprovalOptions } } }, + }); + const buttons = conn.el.querySelectorAll(".st-approval-acts button"); + buttons[0].click(); + expect(resolveStructuredApproval).toHaveBeenCalledWith("s1", "n1", "allow_once"); + + // Superseded by a new approval before the in-flight POST settles. + ws.emit({ + type: "projection_update", + transcript: { type: "append", row: row("approval:2", "approval", { toolCall: {}, options: allApprovalOptions }) }, + state: { ...idleState, pending_approval: { nonce: "n2", request: { sessionId: "s1", toolCall: {}, options: allApprovalOptions } } }, + }); + reject(new Error("too late")); + await Promise.resolve().then(() => Promise.resolve()).then(() => Promise.resolve()); + + expect(conn.el.textContent).not.toContain("too late"); + const newButtons = conn.el.querySelectorAll(".st-approval-acts button"); + expect(newButtons).toHaveLength(3); + expect(newButtons[0].disabled).toBe(false); + conn.dispose(); + }); }); describe("structured connection: reconnection/error/loading", () => { diff --git a/web/src/structured.ts b/web/src/structured.ts index d9d5d0e..eee7f7b 100644 --- a/web/src/structured.ts +++ b/web/src/structured.ts @@ -115,13 +115,26 @@ export function createStructuredConnection( }; } - function decide(rowId: string, decision: StructuredApprovalDecision): void { + async function decide(rowId: string, decision: StructuredApprovalDecision): Promise { const nonce = state.pending_approval?.nonce; // Already resolved elsewhere between the click and here — the row's // buttons should already be gone, but a stale nonce is a no-op, not an - // error (docs/STRUCTURED-VIEW.md: any nonce answers 204). + // error (docs/STRUCTURED-VIEW.md: any nonce answers 204). Resolve + // quietly: there is nothing for the row to restore. if (!nonce || rowId !== pendingApprovalRowId) return; - resolveStructuredApproval(sessionId, nonce, decision).catch((e: unknown) => showError(String(e))); + try { + await resolveStructuredApproval(sessionId, nonce, decision); + } catch (e: unknown) { + // Only propagate the failure — so the row re-enables its buttons — if + // this is still the pending approval. If it moved on while the + // request was in flight (superseded, cancelled, answered elsewhere), + // the state-driven re-render already replaced the row; reviving + // buttons on it would be reviving a decision that no longer applies. + if (rowId === pendingApprovalRowId) { + showError(String(e)); + throw e; + } + } } function mountRow(row: TranscriptRow): void { diff --git a/web/src/structuredRender.test.ts b/web/src/structuredRender.test.ts index 22da887..1aa406b 100644 --- a/web/src/structuredRender.test.ts +++ b/web/src/structuredRender.test.ts @@ -160,29 +160,41 @@ describe("buildTranscriptRow: tool diffs", () => { }); describe("buildTranscriptRow: approval", () => { - const approvalRow = () => + const approvalRow = (options: Array<{ optionId: string; name: string; kind: string }>) => row("approval", { sessionId: "s1", toolCall: { toolCallId: "t1", title: "Write outside worktree", locations: [{ path: "/etc/x" }] }, - options: [], + options, }); - it("shows the three decision buttons only when it is the pending approval", () => { - const el = buildTranscriptRow(approvalRow(), { pending: true, onDecide: () => {} }); + const allThreeOptions = [ + { optionId: "a1", name: "Allow once", kind: "allow_once" }, + { optionId: "a2", name: "Allow always", kind: "allow_always" }, + { optionId: "r1", name: "Reject once", kind: "reject_once" }, + ]; + + it("shows the three decision buttons only when the request offers all three option kinds", () => { + const el = buildTranscriptRow(approvalRow(allThreeOptions), { + pending: true, + onDecide: () => Promise.resolve(), + }); const buttons = el.querySelectorAll(".st-approval-acts button"); expect(buttons).toHaveLength(3); expect(el.querySelector(".st-approval-path")!.textContent).toBe("/etc/x"); }); it("hides the buttons and shows an answered note once resolved", () => { - const el = buildTranscriptRow(approvalRow(), { pending: false, onDecide: () => {} }); + const el = buildTranscriptRow(approvalRow(allThreeOptions), { + pending: false, + onDecide: () => Promise.resolve(), + }); expect(el.querySelector(".st-approval-acts")).toBeNull(); expect(el.querySelector(".st-approval-resolved")!.textContent).toBe("answered"); }); it("invokes onDecide with the clicked decision and disables the buttons", () => { - const onDecide = vi.fn(); - const el = buildTranscriptRow(approvalRow(), { pending: true, onDecide }); + const onDecide = vi.fn().mockResolvedValue(undefined); + const el = buildTranscriptRow(approvalRow(allThreeOptions), { pending: true, onDecide }); const buttons = el.querySelectorAll(".st-approval-acts button"); buttons[2].click(); expect(onDecide).toHaveBeenCalledWith("deny"); @@ -190,9 +202,65 @@ describe("buildTranscriptRow: approval", () => { }); it("treats a row with no approval state as not pending, defensively", () => { - const el = buildTranscriptRow(approvalRow()); + const el = buildTranscriptRow(approvalRow(allThreeOptions)); expect(el.querySelector(".st-approval-acts")).toBeNull(); }); + + it("omits Allow for session when the request offers no allow_always option", () => { + const el = buildTranscriptRow( + approvalRow([ + { optionId: "a1", name: "Allow once", kind: "allow_once" }, + { optionId: "r1", name: "Reject once", kind: "reject_once" }, + ]), + { pending: true, onDecide: () => Promise.resolve() }, + ); + const labels = [...el.querySelectorAll(".st-approval-acts button")].map((b) => b.textContent); + expect(labels).toEqual(["Allow once", "Deny"]); + }); + + it("offers Deny for either reject_once or reject_always, never for neither", () => { + const rejectAlwaysOnly = buildTranscriptRow( + approvalRow([{ optionId: "r2", name: "Reject always", kind: "reject_always" }]), + { pending: true, onDecide: () => Promise.resolve() }, + ); + expect([...rejectAlwaysOnly.querySelectorAll(".st-approval-acts button")].map((b) => b.textContent)).toEqual([ + "Deny", + ]); + }); + + it("shows a 'no decision offered' note instead of an empty action row when nothing matches", () => { + const el = buildTranscriptRow(approvalRow([{ optionId: "x", name: "Something else", kind: "allow_once" }]), { + pending: true, + onDecide: () => Promise.resolve(), + }); + // allow_once is offered, so this specific case still shows a button — + // use a kind the client doesn't map to any decision at all. + expect(el.querySelectorAll(".st-approval-acts button")).toHaveLength(1); + + const none = buildTranscriptRow(approvalRow([]), { pending: true, onDecide: () => Promise.resolve() }); + expect(none.querySelector(".st-approval-acts")).toBeNull(); + expect(none.querySelector(".st-approval-resolved")!.textContent).toBe("no decision offered"); + }); + + it("re-enables the buttons when onDecide rejects", async () => { + const onDecide = vi.fn().mockRejectedValue(new Error("network down")); + const el = buildTranscriptRow(approvalRow(allThreeOptions), { pending: true, onDecide }); + const buttons = el.querySelectorAll(".st-approval-acts button"); + buttons[0].click(); + expect(buttons[0].disabled).toBe(true); + await Promise.resolve().then(() => Promise.resolve()); + expect(buttons[0].disabled).toBe(false); + expect(buttons[1].disabled).toBe(false); + }); + + it("leaves the buttons disabled when onDecide resolves", async () => { + const onDecide = vi.fn().mockResolvedValue(undefined); + const el = buildTranscriptRow(approvalRow(allThreeOptions), { pending: true, onDecide }); + const buttons = el.querySelectorAll(".st-approval-acts button"); + buttons[0].click(); + await Promise.resolve().then(() => Promise.resolve()); + expect(buttons[0].disabled).toBe(true); + }); }); describe("buildTranscriptRow: notice / unknown kinds", () => { diff --git a/web/src/structuredRender.ts b/web/src/structuredRender.ts index 9756711..6faae2f 100644 --- a/web/src/structuredRender.ts +++ b/web/src/structuredRender.ts @@ -7,6 +7,7 @@ import { escapeHtml } from "./html.js"; import { computeDiffLines, diffStats } from "./structuredDiff.js"; import type { AcpContentBlock, + AcpPermissionOption, AcpPlan, AcpToolCallContent, AcpToolCallLike, @@ -271,11 +272,29 @@ function buildPlainRow(row: TranscriptRow): HTMLElement { export interface ApprovalRowState { pending: boolean; - onDecide: (decision: StructuredApprovalDecision) => void; + /** Resolves once the decision has been submitted; rejects when the POST + * failed and this approval is still the one awaiting a decision, so the + * caller knows to restore its controls. Never rejects for a stale click + * (the approval already moved on) — there is nothing to restore then. */ + onDecide: (decision: StructuredApprovalDecision) => Promise; +} + +/** Which decisions a permission request actually backs, by the + * `PermissionOptionKind` values it offers (docs/STRUCTURED-VIEW.md's + * "Decision mapping"). A decision with no matching option resolves + * `Cancelled` server-side regardless of what the client sent — offering its + * button would let the operator "allow" something that silently no-ops. */ +function offeredDecisions(options: AcpPermissionOption[]): StructuredApprovalDecision[] { + const kinds = new Set(options.map((o) => o.kind)); + const decisions: StructuredApprovalDecision[] = []; + if (kinds.has("allow_once")) decisions.push("allow_once"); + if (kinds.has("allow_always")) decisions.push("allow_for_session"); + if (kinds.has("reject_once") || kinds.has("reject_always")) decisions.push("deny"); + return decisions; } function buildApprovalRow(row: TranscriptRow, approval: ApprovalRowState): HTMLElement { - const request = (row.data ?? {}) as { toolCall?: AcpToolCallLike }; + const request = (row.data ?? {}) as { toolCall?: AcpToolCallLike; options?: AcpPermissionOption[] }; const toolCall = request.toolCall ?? {}; const wrap = el("div", "st-row st-row-approval"); const card = el("div", "st-approval"); @@ -304,17 +323,33 @@ function buildApprovalRow(row: TranscriptRow, approval: ApprovalRowState): HTMLE btn.className = cls ? `act ${cls}` : "act"; btn.textContent = label; btn.addEventListener("click", () => { - acts.querySelectorAll("button").forEach((b) => (b.disabled = true)); - approval.onDecide(decision); + const buttons = [...acts.querySelectorAll("button")]; + buttons.forEach((b) => (b.disabled = true)); + approval.onDecide(decision).catch(() => { + // Rejects only when this approval is still pending (see + // ApprovalRowState) — restore the controls so the operator can + // retry; the error itself is surfaced by the connection. + buttons.forEach((b) => (b.disabled = false)); + }); }); return btn; }; - acts.append( - mk("Allow once", "allow_once", "primary"), - mk("Allow for session", "allow_for_session", ""), - mk("Deny", "deny", "deny"), - ); - card.appendChild(acts); + const offered = offeredDecisions(request.options ?? []); + const specs: [StructuredApprovalDecision, string, string][] = [ + ["allow_once", "Allow once", "primary"], + ["allow_for_session", "Allow for session", ""], + ["deny", "Deny", "deny"], + ]; + for (const [decision, label, cls] of specs) { + if (offered.includes(decision)) acts.appendChild(mk(label, decision, cls)); + } + if (acts.children.length > 0) { + card.appendChild(acts); + } else { + const none = el("div", "st-approval-resolved"); + none.textContent = "no decision offered"; + card.appendChild(none); + } } else { const resolved = el("div", "st-approval-resolved"); resolved.textContent = "answered"; @@ -343,7 +378,7 @@ export function buildTranscriptRow(row: TranscriptRow, approval?: ApprovalRowSta built = buildToolRow(row); break; case "approval": - built = buildApprovalRow(row, approval ?? { pending: false, onDecide: () => {} }); + built = buildApprovalRow(row, approval ?? { pending: false, onDecide: () => Promise.resolve() }); break; default: built = buildPlainRow(row);