Repository navigation
Unify the approval cards onto one shell - #381
Conversation
The thread drew human approvals two different ways (useagenthq#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.
Fold the shared shell into the single card and fix the focus and test gaps from review of useagenthq#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.
|
Thanks for the review — all points addressed, pushed to this branch. Must fix — one card: folded the shared presentation into Deny dropped focus: opening the deny flow now moves focus to the reason box (so the shortcuts keep working after the Deny button unmounts); backing out returns focus to the card. Every card grabbed focus: only the single pending card auto-focuses, and only when nothing else holds focus — decided cards never take it, so Cmd+Enter lands on the pending one and a reload no longer scrolls to an old card. No focus ring: the focused card now shows a visible ring. Tests: added Small: dropped the self-mapping Screenshots (before / after / two-step deny) are on the description. |
|
Merged, thanks @coolestnick! One card for every approval now, and the keyboard and focus work is a real upgrade. |
Closes #351
What this changes
The thread drew human approvals two different ways, so the same decision looked and worked differently each time:
Neither was reachable by keyboard.
This PR makes one approval card. The shared presentation now lives in
components/ai/approval-card.tsx(replacing the old radio-select version, which only one lane ever used), and both lanes render through it. The two old layouts are gone.Approach
components/ai/approval-card.tsxis the single card. It renders a focusablerole="group", the shield header + optional subtitle, an optional status chip, a body slot (the gateway argument list or the native detail block), a note/error line, an optional reason slot, and the footer actions in the order given (primary last).approvalShortcut(key, modifiers)→"approve" | "reject" | null:Escaperejects,Cmd/Ctrl+Enterapproves. It is unit-tested without a DOM.gateway-approval-card.tsxkeeps its optimistic resolve/rollback and status, and captures the optional denial reason only after the user chooses to deny (a two-step deny: Deny → reason box appears and is focused → confirm). Backing out returns focus to the card.native-approval-card.tsxkeeps its four decisions — Approve once, Always allow this session, Decline, Cancel turn — in one consistent order. Its public props are unchanged, soconversation.tsxandagent-ui/approval-request.tsxare untouched.frontend/AGENTS.mdseam note to point at the single card.Keyboard mapping
Cmd/Ctrl+EnterEscapeEscapebacks out)Done when
Tests
components/ai/approval-card.test.tsx— the pure key-map and static render states.components/ai/approval-card.interaction.test.tsx— real key presses, focus (pending vs decided), and the two-step deny, mounted under happy-dom.Checks
bun run typecheck(repo root) — passcd frontend && bun run test— 1761 pass, 0 failcd frontend && bun run lint— cleancd frontend && bun run build— passbun test scripts/agent-docs.test.ts— passHow to see it
A
/labsection ("Chat · Approval cards (one card, two lanes)") renders both lanes — native command, gateway pending, gateway resolved — for visual review in every theme.Screenshots
Before — two divergent cards (native's four flat buttons vs. the gateway's always-on reason input above a separate radio sub-card):
After — one card, consistent header/body/footer; only the pending card holds focus (visible ring), the resolved card never does:
Two-step deny — Deny reveals the reason box already focused; footer swaps to Keep reviewing / confirm Deny: