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

Closed

dev wants to merge 1 commit from feat/error-banner-1168 into dev/0

dev commented

2026-05-15 07:05:50 +00:00

Collaborator

Closes #1168

Summary

New component components/error-banner.tsx: red-tinted banner (--state-failure tokens) with AlertCircle icon matching <StatusPill> error state. retryable prop controls whether a "Retry" button appears. Message text is select-text so operators can copy it.
event-log.tsx: error event rows now render <ErrorBanner retryable={false}> instead of the generic whitespace-pre-wrap path.
planner/transcript.tsx: stream error block replaced with <ErrorBanner retryable={true} onRetry={onRetryStream}> (removed the local inline error + RefreshCw import that duplicated the same pattern).
task-detail.tsx: header shows <ErrorBanner retryable={false}> below the metadata <dl> when task.status === "failure" and task.error is set. Also removed the semantically-invalid <div> inside <dl> that previously held the error text.

Test plan

just qa passes (typecheck + lint + format + tests)
Open a task in the failure state — <ErrorBanner> appears in the task detail header below the metadata row
Scroll to an error event in the event log — renders as a red banner (no retry button)
Trigger a stream error in the planner workspace — banner appears with Retry button that calls back to the parent

Closes #1168 ## Summary - **New component** `components/error-banner.tsx`: red-tinted banner (`--state-failure` tokens) with `AlertCircle` icon matching `<StatusPill>` error state. `retryable` prop controls whether a "Retry" button appears. Message text is `select-text` so operators can copy it. - **`event-log.tsx`**: `error` event rows now render `<ErrorBanner retryable={false}>` instead of the generic whitespace-pre-wrap path. - **`planner/transcript.tsx`**: stream error block replaced with `<ErrorBanner retryable={true} onRetry={onRetryStream}>` (removed the local inline error + `RefreshCw` import that duplicated the same pattern). - **`task-detail.tsx`**: header shows `<ErrorBanner retryable={false}>` below the metadata `<dl>` when `task.status === "failure"` and `task.error` is set. Also removed the semantically-invalid `<div>` inside `<dl>` that previously held the error text. ## Test plan - [ ] `just qa` passes (typecheck + lint + format + tests) - [ ] Open a task in the failure state — `<ErrorBanner>` appears in the task detail header below the metadata row - [ ] Scroll to an `error` event in the event log — renders as a red banner (no retry button) - [ ] Trigger a stream error in the planner workspace — banner appears with Retry button that calls back to the parent

dev added the

area:dashboard

label

2026-05-15 07:05:50 +00:00

dev added 1 commit

2026-05-15 07:05:51 +00:00

feat(web): add <ErrorBanner> to unify error display across surfaces 7b8dd9d7a3

Closes #1168

- New `components/error-banner.tsx`: red-tinted banner with AlertCircle
  icon matching StatusPill error state; `retryable` prop controls Retry
  button visibility; message text is user-selectable.
- `event-log.tsx`: error event rows now render `<ErrorBanner retryable={false}>`
  instead of the generic whitespace-pre-wrap path.
- `planner/transcript.tsx`: stream error 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; removed the
  semantically-invalid `<div>` inside `<dl>` that existed before.

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

dev requested review from reviewer

2026-05-15 07:08:53 +00:00

reviewer approved these changes

2026-05-15 07:11:53 +00:00

reviewer left a comment

Review — `feat(web): add <ErrorBanner>`

Clean extraction. The component is well-structured, the three call-sites are all improved, and the accessibility basics are in place (role="alert", aria-hidden on the icon, select-text for copy). A few notes below — none are blockers.

CI / QA

CI shows no runs against the head SHA and the just qa checkbox is unchecked in the test plan. Please push a CI trigger (or confirm just qa passes locally) before merge — this is a required gate.

Minor issues

1. Redundant ?? "" on ev.summary (event-log.tsx)

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

TaskEvent.summary is typed as string (required, non-nullable) in @claude-hooks/shared. The ?? "" branch is unreachable dead code. Safe to drop:

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

If you want a defensive guard anyway, a non-empty assertion is more informative than a silent empty string.

2. Silent retryable / onRetry mismatch

The button only renders when retryable && onRetry — so passing retryable={true} without onRetry silently shows no button. The current call-sites handle this correctly (retryable={!!onRetryStream} in transcript), but the API allows an inconsistent state with no dev-time feedback.

Consider a discriminated union to encode the contract statically:

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

This is a nice-to-have, not a blocker — the current runtime guard is safe.

3. Full error text in task header (intentional, but worth noting)

The old code truncated task.error at 180 characters. The new <ErrorBanner> shows the full string (only break-words wrapping it). This is strictly better UX given select-text, just confirming it's intentional.

4. PR description says <div> inside <dl> was "semantically invalid"

For the record: <div> as a grouping wrapper around <dt>/<dd> pairs is valid HTML5. Moving the error banner outside the <dl> is still the right call (it's not a term/description pair), but the motivation in the description is slightly off.

Positives

retryable && onRetry guard in the component body is defensively correct even if the prop type doesn't enforce it.
data-testid="error-banner" / data-testid="error-banner-retry" give test surfaces; confirmed no existing tests targeted the old stream-error-retry testid, so no breakage.
Removing the inline RefreshCw import from transcript.tsx and the 11-line manual button block is a solid simplification.
status === "failure" && task.error in task-detail.tsx is more correct than the previous bare task.error guard (prevents showing a stale error on a recovered task).

LGTM pending CI green.

## Review — `feat(web): add <ErrorBanner>` Clean extraction. The component is well-structured, the three call-sites are all improved, and the accessibility basics are in place (`role="alert"`, `aria-hidden` on the icon, `select-text` for copy). A few notes below — none are blockers. --- ### CI / QA CI shows **no runs** against the head SHA and the `just qa` checkbox is unchecked in the test plan. Please push a CI trigger (or confirm `just qa` passes locally) before merge — this is a required gate. --- ### Minor issues **1. Redundant `?? ""` on `ev.summary`** (`event-log.tsx`) ```tsx <ErrorBanner message={ev.summary ?? ""} retryable={false} /> ``` `TaskEvent.summary` is typed as `string` (required, non-nullable) in `@claude-hooks/shared`. The `?? ""` branch is unreachable dead code. Safe to drop: ```tsx <ErrorBanner message={ev.summary} retryable={false} /> ``` If you want a defensive guard anyway, a non-empty assertion is more informative than a silent empty string. --- **2. Silent `retryable` / `onRetry` mismatch** The button only renders when `retryable && onRetry` — so passing `retryable={true}` without `onRetry` silently shows no button. The current call-sites handle this correctly (`retryable={!!onRetryStream}` in transcript), but the API allows an inconsistent state with no dev-time feedback. Consider a discriminated union to encode the contract statically: ```ts export type ErrorBannerProps = | { message: string; retryable: false; onRetry?: never } | { message: string; retryable: true; onRetry: () => void }; ``` This is a nice-to-have, not a blocker — the current runtime guard is safe. --- **3. Full error text in task header (intentional, but worth noting)** The old code truncated `task.error` at 180 characters. The new `<ErrorBanner>` shows the full string (only `break-words` wrapping it). This is strictly better UX given `select-text`, just confirming it's intentional. --- **4. PR description says `<div>` inside `<dl>` was "semantically invalid"** For the record: `<div>` as a grouping wrapper around `<dt>`/`<dd>` pairs is [valid HTML5](https://html.spec.whatwg.org/multipage/grouping-content.html#the-dl-element). Moving the error banner outside the `<dl>` is still the right call (it's not a term/description pair), but the motivation in the description is slightly off. --- ### Positives - `retryable && onRetry` guard in the component body is defensively correct even if the prop type doesn't enforce it. - `data-testid="error-banner"` / `data-testid="error-banner-retry"` give test surfaces; confirmed no existing tests targeted the old `stream-error-retry` testid, so no breakage. - Removing the inline `RefreshCw` import from `transcript.tsx` and the 11-line manual button block is a solid simplification. - `status === "failure" && task.error` in `task-detail.tsx` is more correct than the previous bare `task.error` guard (prevents showing a stale error on a recovered task). LGTM pending CI green.