charles/agent-hooks

Fork 0

feat(web): add global approval banner for blocked tasks (#1167) #1197

Merged

charles merged 1 commit from code-lead/1167 into main

2026-05-15 11:20:03 +00:00

code-lead commented

2026-05-15 11:10:02 +00:00

Collaborator

Summary

Closes #1167.

The inline <ApprovalCard> only shows inside the open task's event log, so a gate on a different task can sit invisible for the full 5-minute window before auto-deny fires. This PR adds a sticky banner that surfaces every pending approval gate above main content on every operational route, fed live by SSE.

Changes

useApprovalsStore (new, zustand) — pending-gate registry keyed by callId, fed by SSE approval_pending / approval_decision rows plus terminal task events (result, cancelled) so the banner clears the instant the operator decides.
<GlobalApprovalBanner> (new) — mounts inside AppShell with overlay positioning (no content push), role="alert", warning amber colours (--ch-color-warning, not error red — approval is not failure), and a prev/next cycler that lets the operator step through every open gate without leaving the current route. The "Open task" link uses <Link to="/agents/live" search={{ detail: taskId }}> for a uniform deep-link.
Route gating — banner is hidden on /settings/*, /login, /onboarding; visible everywhere else AppShell renders (/board, /workspace, /agents/live, /agents/roster, …).
Pure parsers — parseApprovalPending, parseApprovalDecisionCallId, isTerminalTaskEvent are exported from the store module so unit tests can exercise the SSE → store shape without mounting a component or opening an EventSource.

Acceptance criteria

<GlobalApprovalBanner> rendered in the AppShell when ≥ 1 task has a pending approval_pending event
Banner text: "N task(s) awaiting your approval" (Paraglide _one / _other keys via Intl.PluralRules)
Multiple blocked tasks: arrow buttons cycle through every gate without leaving the current route (internal currentIndex state)
Dismisses automatically when all approval gates resolve (SSE approval_decision → clearByCallId; terminal task event → clearByTaskId)
Warning amber colour palette (bg-warning/10 + border-warning), not error red
Positioned above main content, below nav bar — absolute inset-x-0 top-0 inside <main> so the page never gets pushed
role="alert" on the banner root
Active on /board, /workspace, /agents/live, /agents/roster (verified per-route in tests)
Hidden on settings routes (/settings/*)
No new HTTP endpoint required — pure SSE-derived
just qa passes locally (typecheck, lint, biome format, unit tests, paraglide-check, i18n-string-check)

Test plan

bun x vitest run src/stores/approvals-store.test.ts — 15 pure-logic tests cover record/clear/parse paths
bun x vitest run src/components/global-approval-banner.test.tsx — 18 component tests cover visibility gating, count rendering, cycler, and link target
bun x vitest run src/components/app-shell.test.tsx — existing AppShell suite stays green (banner mock added)
just qa runs end-to-end (typecheck + lint + format + all unit tests + sql-layer + paraglide + i18n + flow-schema)
Manual smoke against a live approval_pending SSE event in dev mode (will validate post-merge against the dev fleet)

🤖 Generated with Claude Code

## Summary Closes #1167. The inline `<ApprovalCard>` only shows inside the open task's event log, so a gate on a different task can sit invisible for the full 5-minute window before auto-deny fires. This PR adds a sticky banner that surfaces every pending approval gate above main content on every operational route, fed live by SSE. ## Changes - **`useApprovalsStore` (new, zustand)** — pending-gate registry keyed by `callId`, fed by SSE `approval_pending` / `approval_decision` rows plus terminal task events (`result`, `cancelled`) so the banner clears the instant the operator decides. - **`<GlobalApprovalBanner>` (new)** — mounts inside `AppShell` with overlay positioning (no content push), `role="alert"`, warning amber colours (`--ch-color-warning`, not error red — approval is not failure), and a prev/next cycler that lets the operator step through every open gate without leaving the current route. The "Open task" link uses `<Link to="/agents/live" search={{ detail: taskId }}>` for a uniform deep-link. - **Route gating** — banner is hidden on `/settings/*`, `/login`, `/onboarding`; visible everywhere else `AppShell` renders (`/board`, `/workspace`, `/agents/live`, `/agents/roster`, …). - **Pure parsers** — `parseApprovalPending`, `parseApprovalDecisionCallId`, `isTerminalTaskEvent` are exported from the store module so unit tests can exercise the SSE → store shape without mounting a component or opening an `EventSource`. ## Acceptance criteria - [x] `<GlobalApprovalBanner>` rendered in the AppShell when ≥ 1 task has a pending `approval_pending` event - [x] Banner text: "N task(s) awaiting your approval" (Paraglide `_one` / `_other` keys via `Intl.PluralRules`) - [x] Multiple blocked tasks: arrow buttons cycle through every gate without leaving the current route (internal `currentIndex` state) - [x] Dismisses automatically when all approval gates resolve (SSE `approval_decision` → `clearByCallId`; terminal task event → `clearByTaskId`) - [x] Warning amber colour palette (`bg-warning/10` + `border-warning`), not error red - [x] Positioned above main content, below nav bar — `absolute inset-x-0 top-0` inside `<main>` so the page never gets pushed - [x] `role="alert"` on the banner root - [x] Active on `/board`, `/workspace`, `/agents/live`, `/agents/roster` (verified per-route in tests) - [x] Hidden on settings routes (`/settings/*`) - [x] No new HTTP endpoint required — pure SSE-derived - [x] `just qa` passes locally (typecheck, lint, biome format, unit tests, paraglide-check, i18n-string-check) ## Test plan - [x] `bun x vitest run src/stores/approvals-store.test.ts` — 15 pure-logic tests cover record/clear/parse paths - [x] `bun x vitest run src/components/global-approval-banner.test.tsx` — 18 component tests cover visibility gating, count rendering, cycler, and link target - [x] `bun x vitest run src/components/app-shell.test.tsx` — existing AppShell suite stays green (banner mock added) - [x] `just qa` runs end-to-end (typecheck + lint + format + all unit tests + sql-layer + paraglide + i18n + flow-schema) - [ ] Manual smoke against a live `approval_pending` SSE event in dev mode (will validate post-merge against the dev fleet) 🤖 Generated with [Claude Code](https://claude.com/claude-code)

code-lead added 1 commit

2026-05-15 11:10:02 +00:00

feat(web): add global approval banner for blocked tasks (#1167 )

qa / sql-layer-check (pull_request) Successful in 9s

Details

qa / i18n-string-check (pull_request) Successful in 11s

Details

qa / dockerfile (pull_request) Successful in 18s

Details

qa / db-schema (pull_request) Successful in 20s

Details

qa / qa-1 (pull_request) Successful in 3m18s

Details

qa / qa (pull_request) Successful in 0s

Details

8456680d2a

The inline `<ApprovalCard>` only shows inside the open task's event log,
so a gate on a different task can sit invisible for the full 5-minute
window before auto-deny fires. Add a sticky banner that surfaces every
pending gate above main content on every operational route.

- New `useApprovalsStore` (zustand) — pending-gate registry keyed by
  callId, fed by SSE `approval_pending` / `approval_decision` rows plus
  terminal task events so the banner clears the instant the operator
  decides.
- `<GlobalApprovalBanner>` mounts inside `AppShell` with overlay
  positioning (no content push), `role="alert"`, warning amber colours,
  and a prev/next cycler that lets the operator step through every open
  gate without leaving the current route.
- Self-gates by route: hidden on `/settings/*`, `/login`, `/onboarding`.
- Pure parsers (`parseApprovalPending`, `parseApprovalDecisionCallId`)
  are exported separately so unit tests can exercise the SSE → store
  shape without mounting a component.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

code-lead requested review from reviewer

2026-05-15 11:16:05 +00:00

reviewer approved these changes

2026-05-15 11:19:38 +00:00

Dismissed

reviewer left a comment

Code Review — `feat(web): add global approval banner for blocked tasks`

CI ✅ green. No prior reviews. APPROVED — merging below.

Overview

Fixes a real operator UX gap: the existing <ApprovalCard> is only visible inside the event log of the currently-open task, so a blocked gate on any other task sits invisible for up to 5 minutes before auto-deny fires. This PR adds a sticky global banner that surfaces every pending gate on every operational route, with a prev/next cycler so the operator can step through multiple open gates without leaving the current page.

Architecture is clean: Zustand store → SSE bridge hook → pure presentational component, with parsers exported separately for unit-testability. Exactly the right layering for this kind of ambient UI concern.

What's good

Store design: keying by callId (not taskId) correctly handles the "multiple gates per task" edge case while still letting clearByTaskId sweep them all on terminal events. The idempotent recordPending + reset() on SSE reconnect is a solid double-defence against replay double-counting.
A11y: role="alert", aria-label on the container, aria-live="polite" on the cycler indicator, aria-hidden on decorative icons — complete coverage.
Test breadth: pure store tests (no component mount), browser component tests for every visibility-gate permutation, and the app-shell.test.tsx mock so existing tests keep passing. Missing a direct test for useApprovalsSSE itself, but it's a thin glue layer and the integration is exercised via the component tests.
i18n: both EN and FR shipped together. Using Intl.PluralRules in pluralLabel is forward-compatible with additional locales.
currentIndex clamp: the useEffect + Math.min guard in render correctly handles the intermediate render frame where pending.length has shrunk but state hasn't updated yet. Subtle edge case, handled correctly.

Nits (non-blocking)

1. Stale doc comment: says sticky, JSX uses absolute (global-approval-banner.tsx, Layout section of the file-level JSDoc):

Positioned sticky top-0 z-40 inside the AppShell content area

The actual class string is absolute inset-x-0 top-0 z-40. The absolute approach (relative to the <main> which gained position: relative) is the right call for the "no content push" AC — the comment is just stale from an earlier draft.

2. Duplicated sort between selector and component — selectPendingApprovals (store) and the component's useMemo both do [...pending.values()].sort((a, b) => a.openedAt - b.openedAt). The component could use the selector directly (useApprovalsStore(selectPendingApprovals)) and drop the intermediate pendingMap subscription. Current approach is correct; just noting the duplication.

3. clearByTaskId deletes from Map while iterating — safe per spec (Map iterators skip entries removed after creation), but non-obvious. A filter-based alternative reads more clearly:

const next = new Map([...s.pending].filter(([, a]) => a.taskId !== taskId));

Not a bug — readability note only.

4. deadlineAt not surfaced in banner UI — the field is stored but not shown. A countdown for the 5-minute window would reinforce urgency. Fine as a follow-up; not in this ticket's AC.

Verdict

No blockers. All three nits are polish items that do not affect correctness or the acceptance criteria. Merging.

## Code Review — `feat(web): add global approval banner for blocked tasks` CI ✅ green. No prior reviews. **APPROVED** — merging below. --- ### Overview Fixes a real operator UX gap: the existing `<ApprovalCard>` is only visible inside the event log of the currently-open task, so a blocked gate on any other task sits invisible for up to 5 minutes before auto-deny fires. This PR adds a sticky global banner that surfaces every pending gate on every operational route, with a prev/next cycler so the operator can step through multiple open gates without leaving the current page. Architecture is clean: Zustand store → SSE bridge hook → pure presentational component, with parsers exported separately for unit-testability. Exactly the right layering for this kind of ambient UI concern. --- ### What's good - **Store design**: keying by `callId` (not `taskId`) correctly handles the "multiple gates per task" edge case while still letting `clearByTaskId` sweep them all on terminal events. The idempotent `recordPending` + `reset()` on SSE reconnect is a solid double-defence against replay double-counting. - **A11y**: `role="alert"`, `aria-label` on the container, `aria-live="polite"` on the cycler indicator, `aria-hidden` on decorative icons — complete coverage. - **Test breadth**: pure store tests (no component mount), browser component tests for every visibility-gate permutation, and the `app-shell.test.tsx` mock so existing tests keep passing. Missing a direct test for `useApprovalsSSE` itself, but it's a thin glue layer and the integration is exercised via the component tests. - **i18n**: both EN and FR shipped together. Using `Intl.PluralRules` in `pluralLabel` is forward-compatible with additional locales. - **`currentIndex` clamp**: the `useEffect` + `Math.min` guard in render correctly handles the intermediate render frame where `pending.length` has shrunk but state hasn't updated yet. Subtle edge case, handled correctly. --- ### Nits (non-blocking) **1. Stale doc comment: says `sticky`, JSX uses `absolute`** (`global-approval-banner.tsx`, Layout section of the file-level JSDoc): > Positioned `sticky top-0 z-40` inside the AppShell content area The actual class string is `absolute inset-x-0 top-0 z-40`. The `absolute` approach (relative to the `<main>` which gained `position: relative`) is the right call for the "no content push" AC — the comment is just stale from an earlier draft. **2. Duplicated sort between selector and component** — `selectPendingApprovals` (store) and the component's `useMemo` both do `[...pending.values()].sort((a, b) => a.openedAt - b.openedAt)`. The component could use the selector directly (`useApprovalsStore(selectPendingApprovals)`) and drop the intermediate `pendingMap` subscription. Current approach is correct; just noting the duplication. **3. `clearByTaskId` deletes from Map while iterating** — safe per spec (Map iterators skip entries removed after creation), but non-obvious. A filter-based alternative reads more clearly: ```ts const next = new Map([...s.pending].filter(([, a]) => a.taskId !== taskId)); ``` Not a bug — readability note only. **4. `deadlineAt` not surfaced in banner UI** — the field is stored but not shown. A countdown for the 5-minute window would reinforce urgency. Fine as a follow-up; not in this ticket's AC. --- ### Verdict No blockers. All three nits are polish items that do not affect correctness or the acceptance criteria. Merging.

charles merged commit c6abdeeb10 into main

2026-05-15 11:20:03 +00:00

charles deleted branch code-lead/1167

2026-05-15 11:20:03 +00:00

reviewer approved these changes

2026-05-15 11:20:04 +00:00

reviewer left a comment

Code Review — `feat(web): add global approval banner for blocked tasks`

CI ✅ green. No prior reviews. APPROVED — merging below.

Overview

What's good

Store design: keying by callId (not taskId) correctly handles the "multiple gates per task" edge case while still letting clearByTaskId sweep them all on terminal events. The idempotent recordPending + reset() on SSE reconnect is a solid double-defence against replay double-counting.
A11y: role="alert", aria-label on the container, aria-live="polite" on the cycler indicator, aria-hidden on decorative icons — complete coverage.
Test breadth: pure store tests (no component mount), browser component tests for every visibility-gate permutation, and the app-shell.test.tsx mock so existing tests keep passing. No direct test for useApprovalsSSE itself, but it's a thin glue layer and covered indirectly by the component tests.
i18n: both EN and FR shipped together. Using Intl.PluralRules in pluralLabel is forward-compatible with additional locales.
currentIndex clamp: the useEffect + Math.min guard in render correctly handles the intermediate render frame where pending.length has shrunk but state hasn't updated yet — subtle edge case, handled correctly.

Nits (non-blocking)

1. Stale doc comment: says sticky, JSX uses absolute (global-approval-banner.tsx, Layout section of the file-level JSDoc):

Positioned sticky top-0 z-40 inside the AppShell content area

2. Duplicated sort between selector and component — selectPendingApprovals in the store and the component's useMemo both do [...pending.values()].sort((a, b) => a.openedAt - b.openedAt). The component could use the selector directly (useApprovalsStore(selectPendingApprovals)) and drop the intermediate pendingMap subscription. Current approach is correct; just noting the duplication.

3. clearByTaskId deletes from Map while iterating — safe per spec (Map iterators skip entries removed after creation), but non-obvious. A filter-based form reads more clearly:

const next = new Map([...s.pending].filter(([, a]) => a.taskId !== taskId));

Not a bug — readability note only.

4. deadlineAt not surfaced in banner UI — the field is stored but not displayed. A countdown for the 5-minute window would reinforce urgency for the operator. Fine as a follow-up; not in this ticket's AC.

Verdict

No blockers. All four nits are polish items that do not affect correctness or the acceptance criteria. Merging.

## Code Review — `feat(web): add global approval banner for blocked tasks` CI ✅ green. No prior reviews. **APPROVED** — merging below. --- ### Overview Fixes a real operator UX gap: the existing `<ApprovalCard>` is only visible inside the event log of the currently-open task, so a blocked gate on any other task sits invisible for up to 5 minutes before auto-deny fires. This PR adds a sticky global banner that surfaces every pending gate on every operational route, with a prev/next cycler so the operator can step through multiple open gates without leaving the current page. Architecture is clean: Zustand store → SSE bridge hook → pure presentational component, with parsers exported separately for unit-testability. Exactly the right layering for this kind of ambient UI concern. --- ### What's good - **Store design**: keying by `callId` (not `taskId`) correctly handles the "multiple gates per task" edge case while still letting `clearByTaskId` sweep them all on terminal events. The idempotent `recordPending` + `reset()` on SSE reconnect is a solid double-defence against replay double-counting. - **A11y**: `role="alert"`, `aria-label` on the container, `aria-live="polite"` on the cycler indicator, `aria-hidden` on decorative icons — complete coverage. - **Test breadth**: pure store tests (no component mount), browser component tests for every visibility-gate permutation, and the `app-shell.test.tsx` mock so existing tests keep passing. No direct test for `useApprovalsSSE` itself, but it's a thin glue layer and covered indirectly by the component tests. - **i18n**: both EN and FR shipped together. Using `Intl.PluralRules` in `pluralLabel` is forward-compatible with additional locales. - **`currentIndex` clamp**: the `useEffect` + `Math.min` guard in render correctly handles the intermediate render frame where `pending.length` has shrunk but state hasn't updated yet — subtle edge case, handled correctly. --- ### Nits (non-blocking) **1. Stale doc comment: says `sticky`, JSX uses `absolute`** (`global-approval-banner.tsx`, Layout section of the file-level JSDoc): > Positioned `sticky top-0 z-40` inside the AppShell content area The actual class string is `absolute inset-x-0 top-0 z-40`. The `absolute` approach (relative to the `<main>` which gained `position: relative`) is the right call for the "no content push" AC — the comment is just stale from an earlier draft. **2. Duplicated sort between selector and component** — `selectPendingApprovals` in the store and the component's `useMemo` both do `[...pending.values()].sort((a, b) => a.openedAt - b.openedAt)`. The component could use the selector directly (`useApprovalsStore(selectPendingApprovals)`) and drop the intermediate `pendingMap` subscription. Current approach is correct; just noting the duplication. **3. `clearByTaskId` deletes from Map while iterating** — safe per spec (Map iterators skip entries removed after creation), but non-obvious. A filter-based form reads more clearly: ```ts const next = new Map([...s.pending].filter(([, a]) => a.taskId !== taskId)); ``` Not a bug — readability note only. **4. `deadlineAt` not surfaced in banner UI** — the field is stored but not displayed. A countdown for the 5-minute window would reinforce urgency for the operator. Fine as a follow-up; not in this ticket's AC. --- ### Verdict No blockers. All four nits are polish items that do not affect correctness or the acceptance criteria. Merging.

charles referenced this pull request from a commit

2026-05-15 11:20:05 +00:00

feat(web): add global approval banner for blocked tasks (#1167) (#1197)