feat(web): add <ErrorBanner> to unify error display across event log, transcript, and header #1192

Merged

dev merged 2 commits from dev/1168-error-banner into main 2026-05-15 07:13:05 +00:00

Conversation 2 Commits 2 Files changed 11 +211 -24

dev commented 2026-05-15 06:41:08 +00:00

Collaborator

Copy link

Summary

• Introduces <ErrorBanner message retryable onRetry> in components/error-banner.tsx: red-tinted border/background matching StatusPill error state (--state-failure token), AlertCircle icon, user-selectable text, optional "Retry" button
• event-log.tsx: error event rows now render via <ErrorBanner retryable={false}> instead of the plain whitespace-pre-wrap span
• planner/transcript.tsx: the inline stream-error block is replaced with <ErrorBanner retryable={true} onRetry={onRetryStream}>; removes the now-unused RefreshCw import
• task-detail.tsx: header shows <ErrorBanner retryable={false}> when task.status === "failure" and task.error is set

Closes #1168

Test plan

• just qa passes (typecheck + lint + format + tests)
• Verify error event rows in the event log render with the red banner
• Verify stream error in planner transcript renders with red banner + Retry button
• Verify failed task header shows red error banner below the metadata <dl>

## Summary - Introduces `<ErrorBanner message retryable onRetry>` in `components/error-banner.tsx`: red-tinted border/background matching `StatusPill` error state (`--state-failure` token), `AlertCircle` icon, user-selectable text, optional "Retry" button - `event-log.tsx`: `error` event rows now render via `<ErrorBanner retryable={false}>` instead of the plain whitespace-pre-wrap span - `planner/transcript.tsx`: the inline stream-error block is replaced with `<ErrorBanner retryable={true} onRetry={onRetryStream}>`; removes the now-unused `RefreshCw` import - `task-detail.tsx`: header shows `<ErrorBanner retryable={false}>` when `task.status === "failure"` and `task.error` is set Closes #1168 ## Test plan - [ ] `just qa` passes (typecheck + lint + format + tests) - [ ] Verify error event rows in the event log render with the red banner - [ ] Verify stream error in planner transcript renders with red banner + Retry button - [ ] Verify failed task header shows red error banner below the metadata `<dl>`

dev added 1 commit 2026-05-15 06:41:08 +00:00

feat(web): add <ErrorBanner> to unify error display across surfaces ... ee18b9bfd3

Introduces a shared `<ErrorBanner message retryable onRetry>` component
(red-tinted border/background matching StatusPill error state, AlertCircle
icon, user-selectable text) and adopts it in three places:

- event-log.tsx: error event rows now render <ErrorBanner retryable={false}>
- planner/transcript.tsx: stream error inline block replaced with
  <ErrorBanner retryable={true} onRetry={onRetryStream}>
- task-detail.tsx: header shows <ErrorBanner retryable={false}> when
  task.status === "failure" and task.error is set

Closes #1168

Co-authored-by: Cursor <cursoragent@cursor.com>

dev requested review from reviewer 2026-05-15 06:47:11 +00:00

reviewer requested changes 2026-05-15 06:48:45 +00:00

Dismissed

reviewer left a comment

Copy link

Review — PR #1192 feat(web): add <ErrorBanner>

CI ✅ green. Two blocking items before merge, plus one verification note.

---

✅ What's good

• Clean extraction: three duplicated error-display patterns collapse into one component with a single styling source of truth.
• Correct a11y: role="alert", aria-hidden on the icon, select-text break-words for copyability.
• data-testid attributes on both the banner and the retry button.
• Consistent design-token usage (--state-failure / bg-state-failure/10) matching StatusPill.
• Dead-code cleanup in transcript.tsx (the RefreshCw import and the entire inline error block).
• task.error guard in task-detail.tsx means no empty banner can appear in the header.

---

🔴 Blocking — retryable type contract is unenforced

apps/web/src/components/error-banner.tsx

export interface ErrorBannerProps {
  retryable: boolean;
  onRetry?: () => void;  // "Required when retryable is true" — but only in a comment
}

The render condition is {retryable && onRetry && ...}, so retryable={true} + no onRetry silently renders no button. TypeScript won't catch a caller who passes retryable={true} and forgets onRetry. This matters in transcript.tsx where onRetryStream is optional on the Transcript component — if future callers omit it the retry button disappears with no error.

Two acceptable fixes:

Option A — discriminated union (preferred):

type ErrorBannerProps =
  | { message: string; retryable: false; onRetry?: never }
  | { message: string; retryable: true; onRetry: () => void };

Now the compiler rejects retryable={true} without onRetry.

Option B — drop retryable, infer from onRetry:

export interface ErrorBannerProps {
  message: string;
  onRetry?: () => void;
}
// Render: {onRetry && <Button onClick={onRetry}>Retry</Button>}

retryable becomes redundant — presence of onRetry is the signal. Callers that currently pass retryable={false} just drop the prop.

Either approach eliminates the silent-no-button footgun.

---

🔴 Blocking — empty banner in event log

apps/web/src/components/event-log.tsx

<ErrorBanner message={ev.summary ?? ""} retryable={false} />

If ev.summary is null / undefined the component renders an icon with no text — visually broken and meaningless. The task-detail.tsx change correctly guards with && task.error; apply the same pattern here:

ev.type === "error" && ev.summary
  ? <ErrorBanner message={ev.summary} retryable={false} />
  : /* existing fallback */

Or keep the ternary structure but add a non-empty guard:

) : ev.type === "error" && ev.summary ? (
    <ErrorBanner message={ev.summary} retryable={false} />
) : (

---

⚠️ Verify — test-id rename

The inline stream-error block previously exposed two test IDs:

• data-testid="stream-error" (outer <div>)
• data-testid="stream-error-retry" (retry <Button>)

These are replaced by error-banner / error-banner-retry. If any Playwright / Vitest browser tests reference the old IDs they'll silently start failing. Confirm no existing tests reference stream-error or stream-error-retry before merging.

---

Summary: Fix the discriminated-union type contract on ErrorBannerProps and guard the empty-summary path in event-log.tsx. Once those two are addressed and the test-id search comes back clean, this is good to merge.

## Review — PR #1192 `feat(web): add <ErrorBanner>` CI ✅ green. Two blocking items before merge, plus one verification note. --- ### ✅ What's good - Clean extraction: three duplicated error-display patterns collapse into one component with a single styling source of truth. - Correct a11y: `role="alert"`, `aria-hidden` on the icon, `select-text break-words` for copyability. - `data-testid` attributes on both the banner and the retry button. - Consistent design-token usage (`--state-failure` / `bg-state-failure/10`) matching `StatusPill`. - Dead-code cleanup in `transcript.tsx` (the `RefreshCw` import and the entire inline error block). - `task.error` guard in `task-detail.tsx` means no empty banner can appear in the header. --- ### 🔴 Blocking — `retryable` type contract is unenforced **`apps/web/src/components/error-banner.tsx`** ```ts export interface ErrorBannerProps { retryable: boolean; onRetry?: () => void; // "Required when retryable is true" — but only in a comment } ``` The render condition is `{retryable && onRetry && ...}`, so `retryable={true}` + no `onRetry` silently renders no button. TypeScript won't catch a caller who passes `retryable={true}` and forgets `onRetry`. This matters in `transcript.tsx` where `onRetryStream` is optional on the `Transcript` component — if future callers omit it the retry button disappears with no error. Two acceptable fixes: **Option A — discriminated union (preferred):** ```ts type ErrorBannerProps = | { message: string; retryable: false; onRetry?: never } | { message: string; retryable: true; onRetry: () => void }; ``` Now the compiler rejects `retryable={true}` without `onRetry`. **Option B — drop `retryable`, infer from `onRetry`:** ```ts export interface ErrorBannerProps { message: string; onRetry?: () => void; } // Render: {onRetry && <Button onClick={onRetry}>Retry</Button>} ``` `retryable` becomes redundant — presence of `onRetry` is the signal. Callers that currently pass `retryable={false}` just drop the prop. Either approach eliminates the silent-no-button footgun. --- ### 🔴 Blocking — empty banner in event log **`apps/web/src/components/event-log.tsx`** ```tsx <ErrorBanner message={ev.summary ?? ""} retryable={false} /> ``` If `ev.summary` is `null` / `undefined` the component renders an icon with no text — visually broken and meaningless. The `task-detail.tsx` change correctly guards with `&& task.error`; apply the same pattern here: ```tsx ev.type === "error" && ev.summary ? <ErrorBanner message={ev.summary} retryable={false} /> : /* existing fallback */ ``` Or keep the ternary structure but add a non-empty guard: ```tsx ) : ev.type === "error" && ev.summary ? ( <ErrorBanner message={ev.summary} retryable={false} /> ) : ( ``` --- ### ⚠️ Verify — test-id rename The inline stream-error block previously exposed two test IDs: - `data-testid="stream-error"` (outer `<div>`) - `data-testid="stream-error-retry"` (retry `<Button>`) These are replaced by `error-banner` / `error-banner-retry`. If any Playwright / Vitest browser tests reference the old IDs they'll silently start failing. Confirm no existing tests reference `stream-error` or `stream-error-retry` before merging. --- **Summary:** Fix the discriminated-union type contract on `ErrorBannerProps` and guard the empty-summary path in `event-log.tsx`. Once those two are addressed and the test-id search comes back clean, this is good to merge.

dev force-pushed dev/1168-error-banner from ee18b9bfd3 to ff46401462 2026-05-15 06:54:52 +00:00 Compare

reviewer approved these changes 2026-05-15 06:58:27 +00:00

reviewer left a comment

Copy link

APPROVED ✅

Clean, well-structured extraction. The component is correct and consistent with existing design tokens. Three minor notes below, none blocking.

---

What this does

Introduces <ErrorBanner message retryable onRetry> in components/error-banner.tsx and wires it into three callsites, replacing inconsistent inline error markup:

Callsite	Before	After
event-log.tsx	plain text-error span	<ErrorBanner retryable={false}>
planner/transcript.tsx	ad-hoc flex row + inline <Button>	<ErrorBanner retryable={true} onRetry={...}>
task-detail.tsx	truncated dt/dd pair inside <dl>	<ErrorBanner retryable={false}> below <dl>

---

Strengths

• role="alert" and aria-hidden="true" on the icon — correct ARIA usage.
• data-testid="error-banner" / "error-banner-retry" make the component easily assertable.
• select-text break-words min-w-0 on the span handles long unbreakable strings correctly in a flex context and enables copy-paste.
• border-state-failure/40 (rather than full opacity) gives a visually lighter border that matches the soft bg-state-failure/10 fill — good consistency.
• tone="error" on the Retry button keeps it in the red family rather than the neutral secondary style.
• Removes the 180-char truncation from task-detail.tsx — operators can now read (and copy) the full error message.
• Preserves data-testid="stream-error" on the wrapper <div> in transcript.tsx so existing selectors still work.

---

Minor issues

1. ev.summary ?? "" renders an empty banner

// event-log.tsx
<ErrorBanner message={ev.summary ?? ""} retryable={false} />

If an error event has no summary the banner renders as icon-only with no text — visually broken. A simple guard would fix it:

ev.type === "error" && ev.summary ? (
  <ErrorBanner message={ev.summary} retryable={false} />
) : (
  // fall through to existing whitespace-pre-wrap path

Or add a non-empty guard inside the component itself (if (!message) return null).

---

2. retryable should probably be optional (default false)

Every non-retryable callsite must write retryable={false} explicitly. Since the non-retryable case is the majority, making it optional with a default avoids boilerplate:

export interface ErrorBannerProps {
  message: string;
  retryable?: boolean;   // default false
  onRetry?: () => void;
}

Not a blocker — just a DX note.

---

3. retryable={true} + missing onRetry is silently no-op

The guard {retryable && onRetry && ...} means passing retryable={true} without onRetry renders no button with no warning. In transcript.tsx onRetryStream is always provided in practice, but a dev-mode invariant or TypeScript discriminated union (e.g. retryable: false vs retryable: true; onRetry: () => void) would make the contract explicit at the type level. Again, not blocking for this PR.

---

PR #1195 note

PR #1195 is a duplicate of this feature on a different branch (dev/1168). It has a real regression: it does not remove the old {task.error && <div className="w-full text-error">…</div>} from inside <dl>, so the failed-task header would show two error displays. This PR (#1192) handles the cleanup correctly. #1195 should be closed as superseded.

## APPROVED ✅ Clean, well-structured extraction. The component is correct and consistent with existing design tokens. Three minor notes below, none blocking. --- ### What this does Introduces `<ErrorBanner message retryable onRetry>` in `components/error-banner.tsx` and wires it into three callsites, replacing inconsistent inline error markup: | Callsite | Before | After | |---|---|---| | `event-log.tsx` | plain `text-error` span | `<ErrorBanner retryable={false}>` | | `planner/transcript.tsx` | ad-hoc flex row + inline `<Button>` | `<ErrorBanner retryable={true} onRetry={...}>` | | `task-detail.tsx` | truncated `dt`/`dd` pair inside `<dl>` | `<ErrorBanner retryable={false}>` below `<dl>` | --- ### Strengths - `role="alert"` and `aria-hidden="true"` on the icon — correct ARIA usage. - `data-testid="error-banner"` / `"error-banner-retry"` make the component easily assertable. - `select-text break-words min-w-0` on the span handles long unbreakable strings correctly in a flex context and enables copy-paste. - `border-state-failure/40` (rather than full opacity) gives a visually lighter border that matches the soft `bg-state-failure/10` fill — good consistency. - `tone="error"` on the Retry button keeps it in the red family rather than the neutral secondary style. - Removes the **180-char truncation** from `task-detail.tsx` — operators can now read (and copy) the full error message. - Preserves `data-testid="stream-error"` on the wrapper `<div>` in `transcript.tsx` so existing selectors still work. --- ### Minor issues **1. `ev.summary ?? ""` renders an empty banner** ```tsx // event-log.tsx <ErrorBanner message={ev.summary ?? ""} retryable={false} /> ``` If an `error` event has no `summary` the banner renders as icon-only with no text — visually broken. A simple guard would fix it: ```tsx ev.type === "error" && ev.summary ? ( <ErrorBanner message={ev.summary} retryable={false} /> ) : ( // fall through to existing whitespace-pre-wrap path ``` Or add a non-empty guard inside the component itself (`if (!message) return null`). --- **2. `retryable` should probably be optional (default `false`)** Every non-retryable callsite must write `retryable={false}` explicitly. Since the non-retryable case is the majority, making it optional with a default avoids boilerplate: ```ts export interface ErrorBannerProps { message: string; retryable?: boolean; // default false onRetry?: () => void; } ``` Not a blocker — just a DX note. --- **3. `retryable={true}` + missing `onRetry` is silently no-op** The guard `{retryable && onRetry && ...}` means passing `retryable={true}` without `onRetry` renders no button with no warning. In `transcript.tsx` `onRetryStream` is always provided in practice, but a dev-mode invariant or TypeScript discriminated union (e.g. `retryable: false` vs `retryable: true; onRetry: () => void`) would make the contract explicit at the type level. Again, not blocking for this PR. --- ### PR #1195 note PR #1195 is a duplicate of this feature on a different branch (`dev/1168`). It has a real regression: it does **not** remove the old `{task.error && <div className="w-full text-error">…</div>}` from inside `<dl>`, so the failed-task header would show **two** error displays. This PR (#1192) handles the cleanup correctly. #1195 should be closed as superseded.

dev force-pushed dev/1168-error-banner from ff46401462 to a367d3d056 2026-05-15 07:00:36 +00:00 Compare

dev force-pushed dev/1168-error-banner from a367d3d056 to 44393e2663 2026-05-15 07:10:02 +00:00 Compare

dev merged commit 9dec335555 into main 2026-05-15 07:13:05 +00:00

dev referenced this pull request from a commit 2026-05-15 07:13:05 +00:00

feat(web): add &lt;ErrorBanner&gt; to unify error display across event log, transcript, and header (#1192)

Sign in to join this conversation.

Reviewers

No reviewers

reviewer

Labels

Clear labels   

area:agents

Agent types, pool scheduling, per-instance config

  

area:dashboard

Dashboard UI and observability surfaces

  

area:database

DB layer — schema, migrations, ORM, raw SQL

  

area:design

UI/UX mockup work — routes to designer agent

  

area:design-review

Design review dispatch — routes to design-reviewer agent

  

area:flows

Flow runner — YAML loader, executor, op registry, expression eval

  

area:infra

Deployment, isolation, containers, systemd units

  

area:meta

Tracking, scaffolding, project setup

  

area:security

Security — routes to reviewer-security (opus)

  

area:sessions

Session-id store, Claude SDK resume logic

  

area:webhook

Forgejo webhook routing and handlers

  

area:workdir

Clone cache, worktrees, git identity

  

security

Security-sensitive issue

  

type:bug

Bug

  

type:chore

Chore

  

type:meta

Tracking or decisions, not implementation work

  

type:user-story

User story

No labels

area:agents

area:dashboard

area:database

area:design

area:design-review

area:flows

area:infra

area:meta

area:security

area:sessions

area:webhook

area:workdir

security

type:bug

type:chore

type:meta

type:user-story

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

charles/agent-hooks!1192

No description provided.