From a99071037b20a22a30136727f389870324eb2e5a Mon Sep 17 00:00:00 2001 From: Nickk'z Date: Sun, 25 Oct 2026 10:20:21 +0530 Subject: [PATCH 1/2] Unify the approval cards onto one shell The thread drew human approvals two different ways (#351): the gateway tool approval wrapped a radio-select card with an always-visible denial reason and rounded Approve/Deny, while the provider-native approval was a flat card with four buttons in another order. The same decision looked and worked differently each time, and neither was reachable by keyboard. Add one presentational shell, components/chat/approval-shell.tsx, that owns the header, body, footer button order, focus-on-appear, and the Escape (reject) / Cmd-Enter (approve) shortcuts. Both lanes now render through it: - GatewayApprovalCard keeps its optimistic resolve/rollback and status, and now captures the optional denial reason only after the user chooses to deny (a two-step deny), instead of a dangling input. - NativeApprovalCard keeps its four decisions (Approve once, Always allow this session, Decline, Cancel turn) in one consistent order. The old divergent layouts are gone; the keyboard contract is a pure, unit-tested function. Render tests cover pending, submitting and submission-error for both kinds, the gateway resolved states, and the native card's removal after resolution. A /lab section shows both lanes for visual review. --- frontend/AGENTS.md | 9 +- frontend/app/lab/component-lab.tsx | 59 +++++- .../components/chat/approval-shell.test.tsx | 84 ++++++++ frontend/components/chat/approval-shell.tsx | 179 ++++++++++++++++++ .../chat/gateway-approval-card.test.tsx | 4 +- .../components/chat/gateway-approval-card.tsx | 160 ++++++++-------- .../chat/native-approval-card.test.tsx | 93 +++++++++ .../components/chat/native-approval-card.tsx | 115 +++++------ 8 files changed, 551 insertions(+), 152 deletions(-) create mode 100644 frontend/components/chat/approval-shell.test.tsx create mode 100644 frontend/components/chat/approval-shell.tsx create mode 100644 frontend/components/chat/native-approval-card.test.tsx diff --git a/frontend/AGENTS.md b/frontend/AGENTS.md index a6a1b5b20..40a08b7ef 100644 --- a/frontend/AGENTS.md +++ b/frontend/AGENTS.md @@ -127,8 +127,13 @@ Search the code and `/lab` first. The kit and its homes: - Native approvals, questions, child turns, cancel and reconnect render through the shared session components for every engine; never a per-engine copy. Engine pickers and engine-specific UI read `GET /api/config` engines and - capability flags, never `engine === "..."`. Gateway approval cards: - `components/chat/gateway-approval-card.tsx` over `components/ai/approval-card.tsx`. + capability flags, never `engine === "..."`. Every human approval (gateway and + provider-native) renders through one shell, + `components/chat/approval-shell.tsx`, which owns the presentation, focus and + Escape / Cmd-Enter shortcuts; the lane wrappers + `components/chat/gateway-approval-card.tsx` and + `components/chat/native-approval-card.tsx` own their decision semantics, never + a second layout. - A thread has one stream: the thread-scoped events endpoint through `use-thread-stream.ts` into the shared store and timeline builder. Never a second reducer, stream controller or poll. diff --git a/frontend/app/lab/component-lab.tsx b/frontend/app/lab/component-lab.tsx index be65177ce..b03c91f01 100644 --- a/frontend/app/lab/component-lab.tsx +++ b/frontend/app/lab/component-lab.tsx @@ -33,6 +33,7 @@ import { StatusDot } from "@/components/base/badges/status-dot"; import { Button } from "@/components/base/buttons/button"; import { IconButton } from "@/components/base/buttons/icon-button"; import { Input } from "@/components/base/input/input"; +import * as Modal from "@/components/base/modal/modal"; import { SegmentedControl, SegmentedControlItem, @@ -49,8 +50,9 @@ import { } from "@/components/base/table/table"; import { Tab, TabList, TabPanel, Tabs } from "@/components/base/tabs/tabs"; import { type OrbState, ThinkingOrb } from "@/components/base/thinking-orb"; +import { GatewayApprovalCard } from "@/components/chat/gateway-approval-card"; +import { NativeApprovalCard } from "@/components/chat/native-approval-card"; import { AsteriskMark } from "@/components/foundations/brand/asterisk-mark"; -import * as Modal from "@/components/base/modal/modal"; import { ARTIFACT_CAPABILITY_ROWS } from "./artifact-capability-matrix"; import { BeautifulUiExtras } from "./beautiful-ui-extras"; import { BeuiAgentShowcase } from "./beui-agent-showcase"; @@ -96,7 +98,9 @@ export function ComponentLab() {
Component lab - UseAgent · Base kit parts bin + + UseAgent · Base kit parts bin +
@@ -192,12 +196,7 @@ export function ComponentLab() { leadingIcon={RiSearch2Line} placeholder="Search agents, runs, skills…" /> - + @@ -332,7 +331,9 @@ export function ComponentLab() { {/* Motion primitives */}
-

Thinking through the plan…

+

+ Thinking through the plan… +

Streaming output

+
+
+ {}} + /> + + +
+
+
{ + const noMods = { ctrl: false, meta: false }; + + test("Escape rejects", () => { + expect(approvalShortcut("Escape", noMods)).toBe("reject"); + }); + + test("Cmd+Enter and Ctrl+Enter approve", () => { + expect(approvalShortcut("Enter", { ctrl: false, meta: true })).toBe("approve"); + expect(approvalShortcut("Enter", { ctrl: true, meta: false })).toBe("approve"); + }); + + test("a bare Enter is not a shortcut (plain typing stays safe)", () => { + expect(approvalShortcut("Enter", noMods)).toBeNull(); + }); + + test("other keys are not shortcuts", () => { + expect(approvalShortcut("a", noMods)).toBeNull(); + expect(approvalShortcut(" ", { ctrl: true, meta: false })).toBeNull(); + }); +}); + +const ACTIONS: ApprovalAction[] = [ + { id: "deny", label: "Deny", tone: "secondary", onSelect: () => {} }, + { id: "approve", label: "Approve", tone: "primary", onSelect: () => {} }, +]; + +const render = (props: Partial[0]> = {}) => + renderToStaticMarkup( + , + ); + +describe("ApprovalShell", () => { + test("pending renders the title, actions, and a focusable group", () => { + const html = render(); + expect(html).toContain('data-testid="shell-under-test"'); + expect(html).toContain('role="group"'); + expect(html).toContain('aria-label="Approval"'); + expect(html).toContain(">Deny<"); + expect(html).toContain(">Approve<"); + }); + + test("busy disables every action (the submitting state)", () => { + const html = render({ busy: true }); + // Both footer buttons carry the native disabled attribute while in flight. + expect(html.match(/disabled/g)?.length ?? 0).toBeGreaterThanOrEqual(2); + }); + + test("error renders the actionable error line (the submission-error state)", () => { + const html = render({ error: "Could not send your decision - try again" }); + expect(html).toContain("Could not send your decision - try again"); + }); + + test("note and chip render when supplied; reason slot renders its content", () => { + const html = render({ + note: "Approved by dana", + chip: { color: "lime", label: "Approved" }, + reason: reason, + }); + expect(html).toContain("Approved by dana"); + expect(html).toContain(">Approved<"); + expect(html).toContain('data-testid="reason-slot"'); + }); + + test("no actions renders no footer buttons (a resolved card)", () => { + const html = render({ actions: [] }); + expect(html).not.toContain(">Deny<"); + expect(html).not.toContain(">Approve<"); + }); +}); diff --git a/frontend/components/chat/approval-shell.tsx b/frontend/components/chat/approval-shell.tsx new file mode 100644 index 000000000..4952a667a --- /dev/null +++ b/frontend/components/chat/approval-shell.tsx @@ -0,0 +1,179 @@ +"use client"; + +// Shared approval surface (#351): one presentation, focus and keyboard model +// for every human approval decision, so the gateway lane +// (gateway-approval-card.tsx) and the provider-native lane +// (native-approval-card.tsx) look and behave identically instead of drawing +// two different cards for the same choice. The lane wrappers own their decision +// semantics and lifecycle; this component owns the shell - header, body, +// footer button order, the focus-on-appear and Escape / Cmd-Enter shortcuts - +// and renders exactly the actions it is handed, in the order given. + +import { RiShieldCheckLine } from "@remixicon/react"; +import { type ReactNode, useEffect, useRef } from "react"; +import { Chip } from "@/components/base/badges/chip"; +import { Button } from "@/components/base/buttons/button"; + +export type ApprovalActionTone = "primary" | "secondary" | "ghost"; + +export interface ApprovalAction { + /** Stable id; also the handle Cmd/Ctrl+Enter or Escape can target. */ + readonly id: string; + readonly label: string; + readonly tone: ApprovalActionTone; + readonly onSelect: () => void; +} + +export interface ApprovalChip { + readonly color: "yellow" | "lime" | "rose" | "neutral"; + readonly label: string; +} + +/** + * The keyboard contract, pure so it is unit-tested without a DOM: Escape + * rejects and Cmd/Ctrl+Enter approves. Returns which role the key press + * invokes, or null when the press is not one of the two shortcuts. + */ +export function approvalShortcut( + key: string, + modifiers: { readonly ctrl: boolean; readonly meta: boolean }, +): "approve" | "reject" | null { + if (key === "Escape") return "reject"; + if (key === "Enter" && (modifiers.ctrl || modifiers.meta)) return "approve"; + return null; +} + +const TONE_VARIANT: Record = { + primary: "primary", + secondary: "secondary", + ghost: "ghost", +}; + +export interface ApprovalShellProps { + readonly testId: string; + /** Lead line, e.g. "Tool approval: deploy" or "Approval required to run a command". */ + readonly title: ReactNode; + readonly subtitle?: ReactNode; + /** Status pill (the gateway lane; native approvals carry no status). */ + readonly chip?: ApprovalChip; + /** Structured detail: the gateway argument list or the native detail block. */ + readonly body?: ReactNode; + /** A resolved or in-flight status line (gateway lane). */ + readonly note?: ReactNode; + /** Actionable error, shown only while the card is still actionable. */ + readonly error?: string | null; + /** Reason-capture row, rendered above the footer only while present (the + * gateway deny flow supplies it after the user chooses to deny). */ + readonly reason?: ReactNode; + readonly actions: readonly ApprovalAction[]; + /** The action Cmd/Ctrl+Enter triggers (an approve-role button). */ + readonly approveActionId?: string; + /** The action Escape triggers (a reject-role button). */ + readonly rejectActionId?: string; + /** Move focus to the card when it first appears (the newest pending request). */ + readonly autoFocus?: boolean; + /** Disables the footer and the shortcuts while a decision is in flight. */ + readonly busy?: boolean; + /** Accessible name for the focusable card group. */ + readonly ariaLabel: string; +} + +export function ApprovalShell({ + testId, + title, + subtitle, + chip, + body, + note, + error, + reason, + actions, + approveActionId, + rejectActionId, + autoFocus = false, + busy = false, + ariaLabel, +}: ApprovalShellProps) { + const ref = useRef(null); + + // Focus the card once, when it appears, so a keyboard-only user can act + // without tabbing to it - but never steal focus from wherever the user is + // already working (e.g. a second stacked approval already grabbed it). + useEffect(() => { + if (!autoFocus) return; + const el = ref.current; + if (!el) return; + const active = document.activeElement; + if (active && active !== document.body) return; + el.focus(); + }, [autoFocus]); + + function handleKeyDown(event: React.KeyboardEvent) { + if (busy) return; + const role = approvalShortcut(event.key, { + ctrl: event.ctrlKey, + meta: event.metaKey, + }); + if (!role) return; + const targetId = role === "approve" ? approveActionId : rejectActionId; + if (!targetId) return; + const action = actions.find((candidate) => candidate.id === targetId); + if (!action) return; + event.preventDefault(); + action.onSelect(); + } + + return ( +
+
+
+ + + +
+

{title}

+ {subtitle &&

{subtitle}

} +
+
+ {chip && ( + + {chip.label} + + )} +
+ + {body} + + {note &&

{note}

} + {error &&

{error}

} + + {reason} + + {actions.length > 0 && ( +
+ {actions.map((action) => ( + + ))} +
+ )} +
+ ); +} diff --git a/frontend/components/chat/gateway-approval-card.test.tsx b/frontend/components/chat/gateway-approval-card.test.tsx index 5e8ab624e..a36480b5c 100644 --- a/frontend/components/chat/gateway-approval-card.test.tsx +++ b/frontend/components/chat/gateway-approval-card.test.tsx @@ -30,14 +30,14 @@ describe("GatewayApprovalCard", () => { test("pending renders the tool name, argument summary, and Approve/Deny actions", () => { const html = render(approval()); expect(html).toContain('data-testid="gateway-approval-card"'); + expect(html).toContain("Tool approval:"); expect(html).toContain("deploy_service"); - expect(html).toContain("Allow deploy_service?"); // One-level argument summary: key + value (long values truncated). expect(html).toContain("service"); expect(html).toContain("billing"); expect(html).toContain(`${"x".repeat(79)}…`); expect(html).not.toContain("x".repeat(81)); - // The composed vendored approval card provides the actions. + // The shared shell renders the Approve/Deny footer. expect(html).toContain(">Approve<"); expect(html).toContain(">Deny<"); expect(html).toContain(">Pending<"); diff --git a/frontend/components/chat/gateway-approval-card.tsx b/frontend/components/chat/gateway-approval-card.tsx index 8b8990d34..42676dd92 100644 --- a/frontend/components/chat/gateway-approval-card.tsx +++ b/frontend/components/chat/gateway-approval-card.tsx @@ -1,15 +1,14 @@ "use client"; -// Gateway approval container (#77): the human side of the approval lane. Wraps -// (never forks) the vendored components/ai/approval-card.tsx with the run -// context it needs - tool name, one-level argument summary, status - and wires -// Approve/Deny to lib/gateway-approvals.ts with an optimistic pending -> -// resolved transition that rolls back on API failure. +// Gateway approval container (#77, unified in #351): the human side of the +// approval lane. Supplies the run context (tool name, one-level argument +// summary, status) to the shared ApprovalShell and wires Approve / Deny to +// lib/gateway-approvals.ts with an optimistic pending -> resolved transition +// that rolls back on API failure. The denial reason is captured only after the +// user chooses to deny, then carried to the agent when the decision continues +// the thread. -import { RiShieldCheckLine } from "@remixicon/react"; import { useState } from "react"; -import { ApprovalCard } from "@/components/ai/approval-card"; -import { Chip } from "@/components/base/badges/chip"; import { Input } from "@/components/base/input/input"; import { type GatewayApproval, @@ -18,6 +17,7 @@ import { type GatewayApprovalStatus, resolveGatewayApproval, } from "@/lib/gateway-approvals"; +import { type ApprovalAction, ApprovalShell } from "./approval-shell"; import { type ApprovalResolution, beginResolution, @@ -57,6 +57,10 @@ function resolvedLine(status: GatewayApprovalStatus, approval: GatewayApproval): return at ? `${verb}${by} · ${at}` : `${verb}${by}`; } +function decisionInFlightLine(decision: GatewayApprovalDecision): string { + return decision === "approve" ? "Approving…" : "Denying…"; +} + export function GatewayApprovalCard({ approval, onResolved, @@ -66,16 +70,18 @@ export function GatewayApprovalCard({ onResolved?: () => void; }) { const [resolution, setResolution] = useState(idleResolution); - // Why a denial: carried to the agent when the decision continues the thread. + // The reason is only entered after the user chooses to deny. + const [denying, setDenying] = useState(false); const [reason, setReason] = useState(""); const status = effectiveStatus(approval.status, resolution); const error = resolution.phase === "idle" ? resolution.error : null; const entries = summarizeApprovalArguments(approval.arguments); const badge = STATUS_BADGE[status]; const requestedAt = formatTimestamp(approval.requestedAt); + const submitting = resolution.phase === "submitting"; async function submit(decision: GatewayApprovalDecision) { - if (resolution.phase === "submitting") return; + if (submitting) return; setResolution(beginResolution(resolution, decision)); try { const settledStatus = await resolveGatewayApproval( @@ -86,40 +92,64 @@ export function GatewayApprovalCard({ setResolution(resolutionSucceeded(settledStatus)); onResolved?.(); } catch (cause) { - const httpStatus = - cause instanceof GatewayApprovalRequestError ? cause.httpStatus : null; + const httpStatus = cause instanceof GatewayApprovalRequestError ? cause.httpStatus : null; setResolution(resolutionFailed(httpStatus)); // Lost the race: someone else already resolved it; refetch shows the truth. if (httpStatus === 409) onResolved?.(); } } - return ( -
-
-
-
- - - -
-

- Tool approval:{" "} - {approval.toolName} -

- {requestedAt && ( -

- Requested {requestedAt} -

- )} -
-
- - {badge.label} - -
+ const pending = status === "pending"; + const actions: ApprovalAction[] = !pending + ? [] + : denying + ? [ + { + id: "cancel-deny", + label: "Keep reviewing", + tone: "ghost", + onSelect: () => setDenying(false), + }, + { + id: "confirm-deny", + label: "Deny", + tone: "primary", + onSelect: () => void submit("deny"), + }, + ] + : [ + { + id: "deny", + label: "Deny", + tone: "secondary", + onSelect: () => setDenying(true), + }, + { + id: "approve", + label: "Approve", + tone: "primary", + onSelect: () => void submit("approve"), + }, + ]; - {entries.length > 0 && ( + return ( + + Tool approval: {approval.toolName} + + } + subtitle={requestedAt ? `Requested ${requestedAt}` : undefined} + chip={badge} + autoFocus + busy={submitting} + approveActionId={denying ? "confirm-deny" : "approve"} + rejectActionId={denying ? "cancel-deny" : "deny"} + actions={actions} + body={ + entries.length > 0 ? (
{entries.map((entry) => (
@@ -128,50 +158,28 @@ export function GatewayApprovalCard({
))}
- )} - - {status !== "pending" && ( -

- {resolution.phase === "submitting" - ? decisionInFlightLine(resolution.decision) - : resolvedLine(status, approval)} -

- )} - {/* Action errors only matter while the card is still actionable - once - the record resolves (e.g. the 409 refetch landed) the resolved line - is the whole truth. */} - {status === "pending" && error && ( -

{error}

- )} -
- - {status === "pending" && ( - <> + ) : undefined + } + note={ + submitting + ? decisionInFlightLine(resolution.decision) + : pending + ? undefined + : resolvedLine(status, approval) + } + error={pending ? error : null} + reason={ + pending && denying ? ( - void submit("approve")} - onDeny={() => void submit("deny")} - /> - - )} -
+ ) : undefined + } + /> ); } - -function decisionInFlightLine(decision: GatewayApprovalDecision): string { - return decision === "approve" ? "Approving…" : "Denying…"; -} diff --git a/frontend/components/chat/native-approval-card.test.tsx b/frontend/components/chat/native-approval-card.test.tsx new file mode 100644 index 000000000..e44a3382e --- /dev/null +++ b/frontend/components/chat/native-approval-card.test.tsx @@ -0,0 +1,93 @@ +import { describe, expect, test } from "bun:test"; +import { renderToStaticMarkup } from "react-dom/server"; +import { selectPendingApproval } from "./approval-state"; +import { NativeApprovalCard } from "./native-approval-card"; +import type { NativeFrame } from "./native-events"; + +function frame(seq: number, eventType: string, payload: unknown): NativeFrame { + return { + schemaVersion: 1, + eventId: `event-${seq}`, + seq, + provider: "t3", + eventType, + native: { + sessionId: "runtime-thread-1", + parentSessionId: null, + messageId: null, + partId: null, + callId: null, + }, + payload, + }; +} + +const requested = frame(1, "approval.requested", { + id: "approval-1", + sessionID: "runtime-thread-1", + requestKind: "command", + detail: "git status", +}); + +const render = (props: { submitting?: boolean; error?: string | null }) => { + const request = selectPendingApproval([requested]); + if (!request) return ""; + return renderToStaticMarkup( + {}} + />, + ); +}; + +describe("NativeApprovalCard", () => { + test("pending renders the request kind, detail, and all four decisions in order", () => { + const html = render({}); + expect(html).toContain('data-testid="native-approval-card"'); + expect(html).toContain("Approval required to run a command"); + expect(html).toContain("git status"); + expect(html).toContain(">Cancel turn<"); + expect(html).toContain(">Decline<"); + expect(html).toContain(">Always allow this session<"); + expect(html).toContain(">Approve once<"); + // Order: cancel, decline, session, approve (primary last). + expect(html.indexOf("Cancel turn")).toBeLessThan(html.indexOf("Decline")); + expect(html.indexOf("Decline")).toBeLessThan(html.indexOf("Always allow this session")); + expect(html.indexOf("Always allow this session")).toBeLessThan(html.indexOf("Approve once")); + }); + + test("submitting disables the actions and shows progress on the primary", () => { + const html = render({ submitting: true }); + expect(html).toContain(">Responding…<"); + expect(html).not.toContain(">Approve once<"); + expect(html.match(/disabled/g)?.length ?? 0).toBeGreaterThanOrEqual(4); + }); + + test("submission error renders the error line", () => { + const html = render({ error: "Could not send your decision - try again" }); + expect(html).toContain("Could not send your decision - try again"); + }); + + test("the card is removed once the approval resolves", () => { + // Removal is driven by the selection: a responded frame clears the pending + // request, so the lane renders nothing. + const resolved = selectPendingApproval([ + requested, + frame(2, "approval.responded", { requestId: "approval-1", decision: "accept" }), + ]); + expect(resolved).toBeNull(); + const html = resolved + ? renderToStaticMarkup( + {}} + />, + ) + : ""; + expect(html).not.toContain('data-testid="native-approval-card"'); + }); +}); diff --git a/frontend/components/chat/native-approval-card.tsx b/frontend/components/chat/native-approval-card.tsx index 13ccd2ce1..137e7a17c 100644 --- a/frontend/components/chat/native-approval-card.tsx +++ b/frontend/components/chat/native-approval-card.tsx @@ -1,9 +1,22 @@ "use client"; -import { RiShieldCheckLine } from "@remixicon/react"; -import { Button } from "@/components/base/buttons/button"; +// Provider-native approval container (unified in #351): the human side of the +// engine-native approval lane (OpenCode/Codex/Pi `approval.*` frames). It +// supplies the request context to the shared ApprovalShell and wires the four +// native decisions - Approve once, Always allow this session, Decline, Cancel +// turn - to the session's onRespond. These cards carry no status: the lane +// removes them on resolution (see approval-state.selectPendingApproval). + +import { type ApprovalAction, ApprovalShell } from "./approval-shell"; import type { ApprovalDecision, PendingApproval } from "./approval-state"; +const ACTION_PHRASE: Record = { + command: "run a command", + "file-read": "read a file", + "file-change": "change files", + other: "use a tool", +}; + export function NativeApprovalCard({ request, submitting, @@ -15,67 +28,43 @@ export function NativeApprovalCard({ error: string | null; onRespond: (decision: ApprovalDecision) => void | Promise; }) { - const action = - request.requestKind === "command" - ? "run a command" - : request.requestKind === "file-read" - ? "read a file" - : request.requestKind === "file-change" - ? "change files" - : "use a tool"; + const respond = (decision: ApprovalDecision) => () => void onRespond(decision); + + const actions: ApprovalAction[] = [ + { id: "cancel", label: "Cancel turn", tone: "ghost", onSelect: respond("cancel") }, + { id: "decline", label: "Decline", tone: "secondary", onSelect: respond("decline") }, + { + id: "acceptForSession", + label: "Always allow this session", + tone: "secondary", + onSelect: respond("acceptForSession"), + }, + { + id: "accept", + label: submitting ? "Responding…" : "Approve once", + tone: "primary", + onSelect: respond("accept"), + }, + ]; + return ( -
-
- - - -
-

Approval required to {action}

- {request.detail && ( -
-              {request.detail}
-            
- )} -
-
- {error &&

{error}

} -
- - - - -
-
+ + {request.detail} + + ) : undefined + } + /> ); } From d7f817378cc436080a8729ea820c866cf480a79c Mon Sep 17 00:00:00 2001 From: Nickk'z Date: Mon, 26 Oct 2026 13:13:34 +0530 Subject: [PATCH 2/2] Address review: one card, keyboard and focus Fold the shared shell into the single card and fix the focus and test gaps from review of #351: - The shell now lives in components/ai/approval-card.tsx, replacing the old radio-select version, so there is one approval card and no second component. Both lanes render through it; the AGENTS.md seam note points at it. - Only the pending card takes focus. Decided cards no longer auto-focus (so Cmd+Enter lands on the pending one and a reload does not scroll to an old card). The claim only happens when nothing else holds focus, so stacked cards do not fight. - Opening the deny flow moves focus to the reason box, so the shortcuts keep working after the Deny button unmounts; backing out returns focus to the card. - The focused card shows a visible ring. - Drop the self-mapping TONE_VARIANT; pass the tone straight to Button. - Keep the lab change to the section only, with no unrelated reformat. - Add interaction tests that press keys, assert focus (pending vs decided), and exercise the two-step deny, alongside the static render and pure key-map tests. --- frontend/AGENTS.md | 7 +- frontend/app/lab/component-lab.tsx | 36 +-- .../ai/approval-card.interaction.test.tsx | 173 +++++++++++ .../approval-card.test.tsx} | 16 +- frontend/components/ai/approval-card.tsx | 285 ++++++++++-------- frontend/components/chat/approval-shell.tsx | 179 ----------- .../components/chat/gateway-approval-card.tsx | 18 +- .../components/chat/native-approval-card.tsx | 6 +- 8 files changed, 367 insertions(+), 353 deletions(-) create mode 100644 frontend/components/ai/approval-card.interaction.test.tsx rename frontend/components/{chat/approval-shell.test.tsx => ai/approval-card.test.tsx} (84%) delete mode 100644 frontend/components/chat/approval-shell.tsx diff --git a/frontend/AGENTS.md b/frontend/AGENTS.md index 40a08b7ef..60ccbe848 100644 --- a/frontend/AGENTS.md +++ b/frontend/AGENTS.md @@ -128,10 +128,9 @@ Search the code and `/lab` first. The kit and its homes: the shared session components for every engine; never a per-engine copy. Engine pickers and engine-specific UI read `GET /api/config` engines and capability flags, never `engine === "..."`. Every human approval (gateway and - provider-native) renders through one shell, - `components/chat/approval-shell.tsx`, which owns the presentation, focus and - Escape / Cmd-Enter shortcuts; the lane wrappers - `components/chat/gateway-approval-card.tsx` and + provider-native) renders through one card, `components/ai/approval-card.tsx`, + which owns the presentation, focus and Escape / Cmd-Enter shortcuts; the lane + wrappers `components/chat/gateway-approval-card.tsx` and `components/chat/native-approval-card.tsx` own their decision semantics, never a second layout. - A thread has one stream: the thread-scoped events endpoint through diff --git a/frontend/app/lab/component-lab.tsx b/frontend/app/lab/component-lab.tsx index b03c91f01..c18cb5896 100644 --- a/frontend/app/lab/component-lab.tsx +++ b/frontend/app/lab/component-lab.tsx @@ -15,7 +15,6 @@ import { } from "@remixicon/react"; import Link from "next/link"; import * as React from "react"; -import { ApprovalCard } from "@/components/ai/approval-card"; import { ContextCardStack } from "@/components/ai/context-card"; import { DiffTable } from "@/components/ai/diff-table"; import { FilterTable } from "@/components/ai/filter-table"; @@ -33,7 +32,6 @@ import { StatusDot } from "@/components/base/badges/status-dot"; import { Button } from "@/components/base/buttons/button"; import { IconButton } from "@/components/base/buttons/icon-button"; import { Input } from "@/components/base/input/input"; -import * as Modal from "@/components/base/modal/modal"; import { SegmentedControl, SegmentedControlItem, @@ -53,6 +51,7 @@ import { type OrbState, ThinkingOrb } from "@/components/base/thinking-orb"; import { GatewayApprovalCard } from "@/components/chat/gateway-approval-card"; import { NativeApprovalCard } from "@/components/chat/native-approval-card"; import { AsteriskMark } from "@/components/foundations/brand/asterisk-mark"; +import * as Modal from "@/components/base/modal/modal"; import { ARTIFACT_CAPABILITY_ROWS } from "./artifact-capability-matrix"; import { BeautifulUiExtras } from "./beautiful-ui-extras"; import { BeuiAgentShowcase } from "./beui-agent-showcase"; @@ -98,9 +97,7 @@ export function ComponentLab() {
Component lab - - UseAgent · Base kit parts bin - + UseAgent · Base kit parts bin
@@ -196,7 +193,12 @@ export function ComponentLab() { leadingIcon={RiSearch2Line} placeholder="Search agents, runs, skills…" /> - +
@@ -331,9 +333,7 @@ export function ComponentLab() { {/* Motion primitives */}
-

- Thinking through the plan… -

+

Thinking through the plan…

Streaming output

-
- {}} - onDeny={() => {}} - /> -
- -
+
{}, + configurable: true, + writable: true, + }); +} +if (typeof window !== "undefined" && typeof window.matchMedia !== "function") { + window.matchMedia = (media: string) => + ({ + matches: false, + media, + onchange: null, + addEventListener: () => {}, + removeEventListener: () => {}, + addListener: () => {}, + removeListener: () => {}, + dispatchEvent: () => false, + }) as unknown as MediaQueryList; +} +if (typeof globalThis.ResizeObserver === "undefined") { + globalThis.ResizeObserver = class { + observe() {} + unobserve() {} + disconnect() {} + } as unknown as typeof ResizeObserver; +} + +const { act } = await import("react"); +const { createRoot } = await import("react-dom/client"); +const { NativeApprovalCard } = await import("../chat/native-approval-card"); +const { GatewayApprovalCard } = await import("../chat/gateway-approval-card"); + +import type { ReactElement } from "react"; +import type { GatewayApproval } from "@/lib/gateway-approvals"; +import type { PendingApproval } from "../chat/approval-state"; + +let roots: Array<{ root: ReturnType; container: HTMLElement }> = []; + +function mount(ui: ReactElement): HTMLElement { + const container = document.createElement("div"); + document.body.appendChild(container); + const root = createRoot(container); + act(() => root.render(ui)); + roots.push({ root, container }); + return container; +} + +afterEach(() => { + for (const { root, container } of roots) { + act(() => root.unmount()); + container.remove(); + } + roots = []; + (document.activeElement as HTMLElement | null)?.blur?.(); +}); + +function press(el: Element, key: string, mods: { ctrl?: boolean; meta?: boolean } = {}) { + act(() => { + el.dispatchEvent( + new KeyboardEvent("keydown", { + key, + bubbles: true, + cancelable: true, + ctrlKey: !!mods.ctrl, + metaKey: !!mods.meta, + }), + ); + }); +} + +function button(container: HTMLElement, text: string): HTMLButtonElement | undefined { + return [...container.querySelectorAll("button")].find((b) => b.textContent?.trim() === text) as + | HTMLButtonElement + | undefined; +} + +const request: PendingApproval = { + id: "n1", + sessionId: "s", + requestKind: "command", + detail: "git status", +}; + +function approval(overrides: Partial = {}): GatewayApproval { + return { + id: "g1", + runId: "r1", + toolName: "deploy_service", + arguments: { service: "billing" }, + status: "pending", + requestedAt: "2026-01-01T00:00:00Z", + resolvedAt: null, + resolvedBy: null, + ...overrides, + }; +} + +describe("native approval card interaction", () => { + test("takes focus on appear, and the keyboard approves and declines", () => { + const decisions: string[] = []; + const container = mount( + { + decisions.push(d); + }} + />, + ); + const card = container.querySelector('[data-testid="native-approval-card"]'); + if (!card) throw new Error("native card did not render"); + expect(document.activeElement).toBe(card); + + press(card, "Enter", { meta: true }); + expect(decisions).toContain("accept"); + + press(card, "Escape"); + expect(decisions).toContain("decline"); + }); +}); + +describe("gateway approval card interaction", () => { + test("a decided card never takes focus", () => { + mount( + , + ); + expect(document.activeElement).toBe(document.body); + }); + + test("two-step deny: Escape opens the reason box and focuses it; Keep reviewing returns focus", () => { + const container = mount(); + const card = container.querySelector('[data-testid="gateway-approval-card"]') as HTMLElement; + expect(document.activeElement).toBe(card); + // No reason box until the user chooses to deny. + expect(card.querySelector('[aria-label="Reason for denying"]')).toBeNull(); + + // Escape enters the deny flow; focus moves to the reason box. + press(card, "Escape"); + const reason = card.querySelector('[aria-label="Reason for denying"]'); + expect(reason).not.toBeNull(); + expect(document.activeElement).toBe(reason); + expect(button(container, "Keep reviewing")).toBeTruthy(); + + // Backing out removes the reason box and returns focus to the card, so the + // shortcuts keep working. + act(() => button(container, "Keep reviewing")?.click()); + expect(card.querySelector('[aria-label="Reason for denying"]')).toBeNull(); + expect(document.activeElement).toBe(card); + }); +}); diff --git a/frontend/components/chat/approval-shell.test.tsx b/frontend/components/ai/approval-card.test.tsx similarity index 84% rename from frontend/components/chat/approval-shell.test.tsx rename to frontend/components/ai/approval-card.test.tsx index 44c7c39e2..ea27f8893 100644 --- a/frontend/components/chat/approval-shell.test.tsx +++ b/frontend/components/ai/approval-card.test.tsx @@ -1,9 +1,11 @@ import { describe, expect, test } from "bun:test"; import { renderToStaticMarkup } from "react-dom/server"; -import { type ApprovalAction, ApprovalShell, approvalShortcut } from "./approval-shell"; +import { type ApprovalAction, ApprovalCard, approvalShortcut } from "./approval-card"; // The keyboard contract is pure, so it is locked here without a DOM: every -// shortcut the shell binds, and every press that is NOT a shortcut. +// shortcut the card binds, and every press that is NOT a shortcut. The +// interaction half (real key presses, focus, the two-step deny) lives in +// approval-card.interaction.test.tsx. describe("approvalShortcut", () => { const noMods = { ctrl: false, meta: false }; @@ -31,10 +33,10 @@ const ACTIONS: ApprovalAction[] = [ { id: "approve", label: "Approve", tone: "primary", onSelect: () => {} }, ]; -const render = (props: Partial[0]> = {}) => +const render = (props: Partial[0]> = {}) => renderToStaticMarkup( - [0]> = {}) => />, ); -describe("ApprovalShell", () => { +describe("ApprovalCard", () => { test("pending renders the title, actions, and a focusable group", () => { const html = render(); - expect(html).toContain('data-testid="shell-under-test"'); + expect(html).toContain('data-testid="card-under-test"'); expect(html).toContain('role="group"'); expect(html).toContain('aria-label="Approval"'); expect(html).toContain(">Deny<"); diff --git a/frontend/components/ai/approval-card.tsx b/frontend/components/ai/approval-card.tsx index 2b7a73e15..a25a9fc59 100644 --- a/frontend/components/ai/approval-card.tsx +++ b/frontend/components/ai/approval-card.tsx @@ -1,153 +1,180 @@ "use client"; -import { useState } from "react"; -import { RiCloseLine } from "@remixicon/react"; -import { cx } from "@/utils/cx"; +// The approval card (#351): one presentation, focus and keyboard model for +// every human approval decision, so the gateway tool-approval lane +// (components/chat/gateway-approval-card.tsx) and the provider-native lane +// (components/chat/native-approval-card.tsx) look and behave identically +// instead of drawing two different cards for the same choice. The lane +// wrappers own their decision semantics and lifecycle; this card owns the +// shell - header, body, footer button order, the focus-on-appear and the +// Escape (reject) / Cmd-Enter (approve) shortcuts - and renders exactly the +// actions it is handed, in the order given. It replaces the earlier +// radio-select prompt that only one lane ever used. + +import { RiShieldCheckLine } from "@remixicon/react"; +import { type ReactNode, useEffect, useMemo, useRef } from "react"; +import { Chip } from "@/components/base/badges/chip"; import { Button } from "@/components/base/buttons/button"; -export interface ApprovalOption { - label: string; - detail?: string; +export type ApprovalActionTone = "primary" | "secondary" | "ghost"; + +export interface ApprovalAction { + /** Stable id; also the handle Cmd/Ctrl+Enter or Escape can target. */ + readonly id: string; + readonly label: string; + readonly tone: ApprovalActionTone; + readonly onSelect: () => void; } -export interface ApprovalCardProps { - question: string; - options: ApprovalOption[]; - onApprove: (option: ApprovalOption) => void; - onDeny: () => void; - /** Adds a free-text "Type something…" row that overrides the radio choice. */ - allowCustom?: boolean; - className?: string; +export interface ApprovalChip { + readonly color: "yellow" | "lime" | "rose" | "neutral"; + readonly label: string; } /** - * Richer approval prompt: radio-select option rows, an optional custom-answer - * field, and a deny / approve footer. Approve stays disabled until a choice is - * made. Ported from the beautiful-ui ApprovalCard demo onto our tokens + - * the Button. + * The keyboard contract, pure so it is unit-tested without a DOM: Escape + * rejects and Cmd/Ctrl+Enter approves. Returns which role the key press + * invokes, or null when the press is not one of the two shortcuts. */ +export function approvalShortcut( + key: string, + modifiers: { readonly ctrl: boolean; readonly meta: boolean }, +): "approve" | "reject" | null { + if (key === "Escape") return "reject"; + if (key === "Enter" && (modifiers.ctrl || modifiers.meta)) return "approve"; + return null; +} + +export interface ApprovalCardProps { + readonly testId: string; + /** Lead line, e.g. "Tool approval: deploy" or "Approval required to run a command". */ + readonly title: ReactNode; + readonly subtitle?: ReactNode; + /** Status pill (the gateway lane; native approvals carry no status). */ + readonly chip?: ApprovalChip; + /** Structured detail: the gateway argument list or the native detail block. */ + readonly body?: ReactNode; + /** A resolved or in-flight status line (gateway lane). */ + readonly note?: ReactNode; + /** Actionable error, shown only while the card is still actionable. */ + readonly error?: string | null; + /** Reason-capture row, rendered above the footer only while present (the + * gateway deny flow supplies it after the user chooses to deny). */ + readonly reason?: ReactNode; + readonly actions: readonly ApprovalAction[]; + /** The action Cmd/Ctrl+Enter triggers (an approve-role button). */ + readonly approveActionId?: string; + /** The action Escape triggers (a reject-role button). */ + readonly rejectActionId?: string; + /** Hold keyboard focus: true only for the single pending card, so decided + * cards never grab focus (and a reload never scrolls to one). */ + readonly autoFocus?: boolean; + /** Disables the footer and the shortcuts while a decision is in flight. */ + readonly busy?: boolean; + /** Accessible name for the focusable card group. */ + readonly ariaLabel: string; +} + export function ApprovalCard({ - question, - options, - onApprove, - onDeny, - allowCustom = false, - className, + testId, + title, + subtitle, + chip, + body, + note, + error, + reason, + actions, + approveActionId, + rejectActionId, + autoFocus = false, + busy = false, + ariaLabel, }: ApprovalCardProps) { - const [selected, setSelected] = useState(null); - const [custom, setCustom] = useState(""); + const ref = useRef(null); + // Changes whenever the footer swaps (e.g. entering the deny flow), so focus + // can be reclaimed after the previously focused button unmounts. + const actionKey = useMemo(() => actions.map((a) => a.id).join("|"), [actions]); - const customActive = allowCustom && custom.trim().length > 0; - const canApprove = selected !== null || customActive; + // Move focus into the card when it appears and whenever its buttons change - + // but never steal it from wherever the user is already working (a focused + // reason box inside this card, or another card above it). Claiming it only + // when nothing holds it means just the one pending card ever takes focus. + useEffect(() => { + if (!autoFocus) return; + const el = ref.current; + if (!el) return; + const active = document.activeElement; + if (active && active !== document.body) return; + el.focus(); + }, [autoFocus, actionKey]); - function approve() { - if (customActive) { - onApprove({ label: custom.trim() }); - return; - } - if (selected !== null) onApprove(options[selected]); + function handleKeyDown(event: React.KeyboardEvent) { + if (busy) return; + const role = approvalShortcut(event.key, { + ctrl: event.ctrlKey, + meta: event.metaKey, + }); + if (!role) return; + const targetId = role === "approve" ? approveActionId : rejectActionId; + if (!targetId) return; + const action = actions.find((candidate) => candidate.id === targetId); + if (!action) return; + event.preventDefault(); + action.onSelect(); } return ( -
-
-
- {question} - +
+
+ + + +
+

{title}

+ {subtitle &&

{subtitle}

} +
+ {chip && ( + + {chip.label} + + )} +
-
- {options.map((option, index) => { - const active = selected === index && !customActive; - return ( - - ); - })} + {body} - {allowCustom && ( - - )} -
-
+ {note &&

{note}

} + {error &&

{error}

} -
- - -
-
+ {reason} + + {actions.length > 0 && ( +
+ {actions.map((action) => ( + + ))} +
+ )} +
); } diff --git a/frontend/components/chat/approval-shell.tsx b/frontend/components/chat/approval-shell.tsx deleted file mode 100644 index 4952a667a..000000000 --- a/frontend/components/chat/approval-shell.tsx +++ /dev/null @@ -1,179 +0,0 @@ -"use client"; - -// Shared approval surface (#351): one presentation, focus and keyboard model -// for every human approval decision, so the gateway lane -// (gateway-approval-card.tsx) and the provider-native lane -// (native-approval-card.tsx) look and behave identically instead of drawing -// two different cards for the same choice. The lane wrappers own their decision -// semantics and lifecycle; this component owns the shell - header, body, -// footer button order, the focus-on-appear and Escape / Cmd-Enter shortcuts - -// and renders exactly the actions it is handed, in the order given. - -import { RiShieldCheckLine } from "@remixicon/react"; -import { type ReactNode, useEffect, useRef } from "react"; -import { Chip } from "@/components/base/badges/chip"; -import { Button } from "@/components/base/buttons/button"; - -export type ApprovalActionTone = "primary" | "secondary" | "ghost"; - -export interface ApprovalAction { - /** Stable id; also the handle Cmd/Ctrl+Enter or Escape can target. */ - readonly id: string; - readonly label: string; - readonly tone: ApprovalActionTone; - readonly onSelect: () => void; -} - -export interface ApprovalChip { - readonly color: "yellow" | "lime" | "rose" | "neutral"; - readonly label: string; -} - -/** - * The keyboard contract, pure so it is unit-tested without a DOM: Escape - * rejects and Cmd/Ctrl+Enter approves. Returns which role the key press - * invokes, or null when the press is not one of the two shortcuts. - */ -export function approvalShortcut( - key: string, - modifiers: { readonly ctrl: boolean; readonly meta: boolean }, -): "approve" | "reject" | null { - if (key === "Escape") return "reject"; - if (key === "Enter" && (modifiers.ctrl || modifiers.meta)) return "approve"; - return null; -} - -const TONE_VARIANT: Record = { - primary: "primary", - secondary: "secondary", - ghost: "ghost", -}; - -export interface ApprovalShellProps { - readonly testId: string; - /** Lead line, e.g. "Tool approval: deploy" or "Approval required to run a command". */ - readonly title: ReactNode; - readonly subtitle?: ReactNode; - /** Status pill (the gateway lane; native approvals carry no status). */ - readonly chip?: ApprovalChip; - /** Structured detail: the gateway argument list or the native detail block. */ - readonly body?: ReactNode; - /** A resolved or in-flight status line (gateway lane). */ - readonly note?: ReactNode; - /** Actionable error, shown only while the card is still actionable. */ - readonly error?: string | null; - /** Reason-capture row, rendered above the footer only while present (the - * gateway deny flow supplies it after the user chooses to deny). */ - readonly reason?: ReactNode; - readonly actions: readonly ApprovalAction[]; - /** The action Cmd/Ctrl+Enter triggers (an approve-role button). */ - readonly approveActionId?: string; - /** The action Escape triggers (a reject-role button). */ - readonly rejectActionId?: string; - /** Move focus to the card when it first appears (the newest pending request). */ - readonly autoFocus?: boolean; - /** Disables the footer and the shortcuts while a decision is in flight. */ - readonly busy?: boolean; - /** Accessible name for the focusable card group. */ - readonly ariaLabel: string; -} - -export function ApprovalShell({ - testId, - title, - subtitle, - chip, - body, - note, - error, - reason, - actions, - approveActionId, - rejectActionId, - autoFocus = false, - busy = false, - ariaLabel, -}: ApprovalShellProps) { - const ref = useRef(null); - - // Focus the card once, when it appears, so a keyboard-only user can act - // without tabbing to it - but never steal focus from wherever the user is - // already working (e.g. a second stacked approval already grabbed it). - useEffect(() => { - if (!autoFocus) return; - const el = ref.current; - if (!el) return; - const active = document.activeElement; - if (active && active !== document.body) return; - el.focus(); - }, [autoFocus]); - - function handleKeyDown(event: React.KeyboardEvent) { - if (busy) return; - const role = approvalShortcut(event.key, { - ctrl: event.ctrlKey, - meta: event.metaKey, - }); - if (!role) return; - const targetId = role === "approve" ? approveActionId : rejectActionId; - if (!targetId) return; - const action = actions.find((candidate) => candidate.id === targetId); - if (!action) return; - event.preventDefault(); - action.onSelect(); - } - - return ( -
-
-
- - - -
-

{title}

- {subtitle &&

{subtitle}

} -
-
- {chip && ( - - {chip.label} - - )} -
- - {body} - - {note &&

{note}

} - {error &&

{error}

} - - {reason} - - {actions.length > 0 && ( -
- {actions.map((action) => ( - - ))} -
- )} -
- ); -} diff --git a/frontend/components/chat/gateway-approval-card.tsx b/frontend/components/chat/gateway-approval-card.tsx index 42676dd92..b6bb47c3a 100644 --- a/frontend/components/chat/gateway-approval-card.tsx +++ b/frontend/components/chat/gateway-approval-card.tsx @@ -2,13 +2,14 @@ // Gateway approval container (#77, unified in #351): the human side of the // approval lane. Supplies the run context (tool name, one-level argument -// summary, status) to the shared ApprovalShell and wires Approve / Deny to +// summary, status) to the shared ApprovalCard and wires Approve / Deny to // lib/gateway-approvals.ts with an optimistic pending -> resolved transition // that rolls back on API failure. The denial reason is captured only after the // user chooses to deny, then carried to the agent when the decision continues // the thread. -import { useState } from "react"; +import { useEffect, useRef, useState } from "react"; +import { type ApprovalAction, ApprovalCard } from "@/components/ai/approval-card"; import { Input } from "@/components/base/input/input"; import { type GatewayApproval, @@ -17,7 +18,6 @@ import { type GatewayApprovalStatus, resolveGatewayApproval, } from "@/lib/gateway-approvals"; -import { type ApprovalAction, ApprovalShell } from "./approval-shell"; import { type ApprovalResolution, beginResolution, @@ -73,6 +73,13 @@ export function GatewayApprovalCard({ // The reason is only entered after the user chooses to deny. const [denying, setDenying] = useState(false); const [reason, setReason] = useState(""); + const reasonRef = useRef(null); + + // Opening the deny flow unmounts the Deny button that had focus; move focus + // to the reason box so the shortcuts keep working and the user can type. + useEffect(() => { + if (denying) reasonRef.current?.focus(); + }, [denying]); const status = effectiveStatus(approval.status, resolution); const error = resolution.phase === "idle" ? resolution.error : null; const entries = summarizeApprovalArguments(approval.arguments); @@ -133,7 +140,7 @@ export function GatewayApprovalCard({ ]; return ( - = { @@ -48,7 +48,7 @@ export function NativeApprovalCard({ ]; return ( -