fix(chat): surface collapsed approval failures

[leonlaiyc]

Problem / Motivation

When a decision in a collapsed tool group fails, the optimistic resolved state rolls back to live buttons without telling the user that the request failed. The button that owned keyboard focus was also unmounted during the optimistic state and returned unfocused, so a keyboard user could not safely retry the same decision.

Why it matters

The silent rollback makes an unrecorded approval look like a completed action. Losing the attempted control's focus adds ambiguity: a retry can start from a different decision than the one the user just chose.

What changed (motivation → approach → change)

• Reuse the established ApprovalCard rollback pattern and existing translated failure copy.
• Show an inline ErrorNotice after onApprove rejects.
• Remember the attempted decision and restore focus to its exact button after the controls remount.
• Clear stale failure state when a retry begins or a new permission request arrives.
• Add regression coverage for both Approve and Reject focus restoration.

This does not change approval transport semantics or the expanded approval card.

Tests

• Red-before: 2 focused assertions failed (missing alert and missing Approve/Reject focus restoration); 15 existing tests passed.
• vitest run src/test/CollapsibleToolGroupCov80.test.tsx src/test/CollapsibleToolGroup.purposePreview.test.tsx --pool=forks --maxWorkers=1 --fileParallelism=false — 22 passed after the final rebase.
• eslint src/pages/chat/CollapsibleToolGroup.tsx src/test/CollapsibleToolGroupCov80.test.tsx — passed.
• tsc -b — passed.
• node scripts/i18n-check.mjs — all 18 checks passed.

Manual verification

Rendered the real CollapsibleToolGroup, rejected the decision promise, and verified that the localized alert appears while the attempted Approve button regains focus (visible focus ring and active DOM state).

Screenshots / video

Related Issues

Fixes #5554

Checklist

• Single commit with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing relevant tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• User-visible copy reuses an existing translated key
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed f69cccb3aa26cf72bf5d737a79d311ccf53bb324 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] f69cccb

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of f69cccb3aa26cf72bf5d737a79d311ccf53bb324 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The base ApprovalCard contains this exact machinery — the PR copies its failure state machine wholesale. I have what I need for the verdict.

Design-Verdict: CONCERNS

Real fix for a real gap, but it clones ApprovalCard's entire failure state machine instead of sharing it — two copies of one gateway contract.

Watch

• The ~40 lines added (gone classification with the exact 'no pending approval' string match, the {terminal, message, attempted} state shape, the positional focus-restore effect, the three-way ErrorNotice message ternary) are byte-for-byte duplicates of ApprovalCard.tsx:43-107. Cause: reuse-by-copy → mechanism: the gateway refusal contract and focus heuristic now live in two components → consequence: a gateway copy change or a button-order edit must be made twice, and the missed copy fails silently (drift degrades to the retryable path, so nothing goes red). An extracted useApprovalDecision-style hook shared by both surfaces is the durable shape; this repo even gates copy-paste via jscpd, so a third approval surface will force the extraction anyway.

Suggestions

• On a terminal failure the buttons unmount but needsAttention stays true, so the amber "approval needed" ping and command preview keep advertising an approval the notice says is gone; gate those on !failure?.terminal too.

[DESIGN-REVIEWED] f69cccb

[github-actions]

UX Review (Fable 5, fork) — 🟡 CONCERNS

UX-level review of f69cccb3aa26cf72bf5d737a79d311ccf53bb324 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

UX-Verdict: CONCERNS

Terminal failure hides the buttons but leaves the pulsing amber "Approval needed" header directly contradicting the "no longer pending" notice below it.

Watch

• Contradictory terminal state. The diff gates only the button row on !failure?.terminal, but the catch resets localResolved to null, so needsAttention stays true — the row keeps the amber animate-ping dot, the "Approval needed" label, and the command preview while the new notice says "This approval has expired or was already decided — the agent is no longer waiting on it." A user sees a pulsing attention magnet on a dead approval with nothing to do, until new permission props arrive (an expired approval may never push one). Rare path × confusion-not-failure × persists indefinitely → CONCERNS. Sibling ApprovalCard avoids this (no pending badge; border flips to danger on terminal). Smallest fix: fold !failure?.terminal into the needsAttention derivation so header, preview, and buttons all agree with the notice.

Retryable path is solid: rollback + role="alert" notice reuses ApprovalCard's exact copy and focus-restore behavior, and the hedged "may not have been recorded" renders only when no server response is in hand.

[UX-REVIEWED] f69cccb

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed f69cccb3aa26cf72bf5d737a79d311ccf53bb324 via the fork AI-review pipeline; updated in place on each push.

Review details

No blocking issues; one advisory finding.

FINDING — website/src/pages/chat/CollapsibleToolGroup.tsx:248 — a terminal refusal (404, or 400 no pending approval) sets localResolved=null + failure.terminal=true, so needsAttention stays true and only the button row is gated on !failure?.terminal, while the pulsing amber approval_needed header dot/label and the command-preview block (both gated on needsAttention alone) keep advertising a required action next to the approval_no_longer_pending notice with no buttons — a contradictory dead-end → Fix: treat failure?.terminal as not-needs-attention for the header indicator/label and the preview block too (or clear the pending affordances), mirroring ApprovalCard's terminal repaint.

[OPUS-REVIEWED] f69cccb

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of f69cccb3aa26cf72bf5d737a79d311ccf53bb324 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I have everything needed. The base ApprovalCard.tsx:28-107 contains the identical mechanism (failure state shape, terminal predicate, focus-restoration effect, ErrorNotice render) that this diff copies into CollapsibleToolGroup; ChatInput.tsx:953-975 is a third, pre-existing variant that already surfaces its failures. All three i18n keys exist in all locales. No unfixed silent-rollback sibling remains after this PR.

First-Principles-Verdict: CONCERNS

The fix is real and lands at mechanism level, but it ships as a ~40-line verbatim copy of ApprovalCard's failure machinery — a second spelling that will diverge.

What this change ships

Intent: make a failed approval decision in a collapsed tool group visible and safely retryable by keyboard (#5554). This is a FIX.

1. Failed decision in a collapsed group now shows a localized inline error, not just a console line — justified (the reported defect).
2. Focus returns to the exact button whose decision failed, so a keyboard retry can't flip the verdict — justified, declared.
3. A 404 / "no pending approval" refusal now removes the Approve/Reject buttons permanently with "no longer pending" copy — rides along; declared only as "the established pattern", but derived (a gone approval has no action retry can succeed at).
4. Stale failure notice clears on retry or when a new permission request arrives — justified.

Watch

• Second spelling: the terminal predicate status === 404 || (status === 400 && message === 'no pending approval'), the {terminal, message, attempted} state, the positional focus effect, and the three-way ErrorNotice render are byte-identical copies of website/src/components/ApprovalCard.tsx:43-72,101-107 (grep for the predicate: 2 copies after this PR; ChatInput.tsx:953-975 is a third hand-rolled variant of "surface a failed decision"). Both copies must now track any gateway change to that 400 message. Sharing one mechanism is genuinely larger than this fix — accepted and deferred, but a human should record it.
• Item 3 rides along in a fix; kept because its zero option leaves a dead approval presented as retryable.

[FIRST-PRINCIPLES-REVIEWED] f69cccb

[chenmingwei23]

Tier 1 auto-approve: fix (3 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean (CodeQL + Semgrep, 0 alerts), security checklist all-NO, AI reviewers green. Category: fix - surfaces collapsed approval-POST failures and restores focus to the attempted decision (#5524); presentation-layer rollback only, approval gate logic unchanged.

[bolichen97]

Approving per triage sweep: readiness passed, no blocking reviews, fix-type change. Auto-merge will be enabled; branch protection still gates.

[bolichen97]

Re-approved after conflict resolution against the latest main; focused validation passed.

[bolichen97]

Full-diff overlap audit (current head edf7248db60b320c1d426f94325769b7e14b889e): #5562 and #6065 are alternative implementations of the same CollapsibleToolGroup.tsx failure path. Both add ErrorNotice, rollback/error state, buttonsRef, and focus restoration to the attempted action after approval fails. #6065 is the more complete variant because it distinguishes terminal 404-style ApiError outcomes from retryable/API failures; #6072 also carries #6065 byte-for-byte as part of its cumulative chain.

Please choose one owner for this UI contract and consolidate the focused tests instead of merging both implementations. My recommendation is to preserve #6065's terminal-vs-retryable behavior after #6065 is cleaned of its unrelated carried commits, then close/supersede the other alternative. This is not covered by #5536: that PR reaches the rollback promise path but does not render this inline error or restore focus.

[bolichen97]

Tier 1 auto-approve: fix (3 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: surface collapsed approval-dispatch failures with an inline notice and focus restore, classifying terminal vs retryable refusals — display only, server gate untouched. CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.