refactor(web): all banners through the shared Banner component — success variant, non-dismissible, action slot, extra classes #289

Merged

dominik.polakovics merged 1 commit from afk/283 into main

2026-08-04 21:34:27 +02:00

dominik.polakovics commented

2026-08-04 21:23:55 +02:00

Owner

Closes #283

ErrorBanner already rendered a dismissible banner with error and notice variants — yet 33 inline <div class="banner …"> copies survived across the app shell, most routes and both settings trees. They existed because the component's API didn't cover what those sites needed: non-dismissible rendering, a success variant, and extra-class passthrough. So this extends the API to the needs actually found in the codebase, then migrates every copy.

The API, extended (not forked)

Prop	What it does
`variant`	gains `'success'` — `role="status"`, like `'notice'`
`onDismiss`	now optional; its absence is the non-dismissible case — no button rendered, rather than a disabled one
`action`	slot for the three banners carrying a control (AFK `Reset`, paused-schedule `Re-enable`, clone `Retry`)
`class`	appends site modifiers after the variant (`afk-strip-paused`, `clone-error`)

message deliberately stays a string prop and never becomes a JSX slot: text-only, never HTML, is the v0 sticky-banner property this component exists to guarantee. Banner bodies carry strings the app didn't write — git remote errors, agent output, server replies.

Why `ErrorBanner` → `Banner`

The issue allowed either extending ErrorBanner or generalising to a Banner it delegates to. I renamed it (git mv, same file extended) rather than leaving a delegating alias: it renders the success variant now, and a component named for one of its three variants is what invited the 33 copies in the first place. Two names for one thing would leave every future author guessing which to reach for. All former call sites pass identical props under the new name.

Migration

All 33 sites, grouped: app shell + top-level routes (6 files), repo/issue routes (7), repo-settings (10), global settings (4).

Markup is preserved byte-for-byte at every site — same classes in the same order, same role, same text — so there is no visual change. Specifics worth reviewing:

No onDismiss was invented anywhere. Every migrated site had no dismiss button and still has none; the ~24 pre-existing dismissible sites keep theirs.
Every <Match> wrapper (a <Switch> branch) is kept. An enclosing <Show> was collapsed only where its condition was exactly the message — <Show when={form.note()}> becomes <Banner message={form.note()} variant="success" />, since Banner does that truthiness Show internally. Where the condition was something else (paused(), clone_status === 'error', repo.error !== undefined), the <Show> stays and the <Banner> goes inside it.
Tokens' reveal banner had its text JSX-interpolated across lines; it becomes an equivalent template literal producing the identical single-line text.

Tests

New colocated Banner.test.tsx (15 cases, SectionCard/FormCard style) pinning: variant and role asserted as one pair per variant (split assertions would pass while drifting apart); the text-not-HTML property (asserted via querySelector('b') === null, since textContent can't tell an escaped <b> from a live one); absent-vs-disabled dismiss; exact class strings so order is the contract; and the action slot's position between text and dismiss.

Verification

Full gate, locally, all green:

npm run lint — clean
npm run format:check — clean
npm test — 72 files, 1030 tests passing (was 71 / 1015 on main; +15 from the new file, no existing test changed to accommodate the refactor)
npm run build — succeeds
grep -rn 'class="banner' web/src --include=*.tsx outside tests matches only Banner.tsx
No ErrorBanner identifier remains in the tree

One judgement call to flag

docs/adr/0019 names ErrorBanner.tsx in its Status section, listing the files that ADR's diff touched at the time. I left it: an ADR is a historical record of a past decision, and rewriting it to say Banner.tsx would falsify what was true then. Happy to update it if this repo treats ADR file lists as a live index instead.

Closes #283 `ErrorBanner` already rendered a dismissible banner with `error` and `notice` variants — yet **33 inline `<div class="banner …">` copies** survived across the app shell, most routes and both settings trees. They existed because the component's API didn't cover what those sites needed: non-dismissible rendering, a success variant, and extra-class passthrough. So this extends the API to the needs actually found in the codebase, then migrates every copy. ## The API, extended (not forked) | Prop | What it does | | --- | --- | | `variant` | gains `'success'` — `role="status"`, like `'notice'` | | `onDismiss` | now **optional**; its absence *is* the non-dismissible case — no button rendered, rather than a disabled one | | `action` | slot for the three banners carrying a control (AFK `Reset`, paused-schedule `Re-enable`, clone `Retry`) | | `class` | appends site modifiers after the variant (`afk-strip-paused`, `clone-error`) | `message` deliberately stays a `string` prop and never becomes a JSX slot: text-only, never HTML, is the v0 sticky-banner property this component exists to guarantee. Banner bodies carry strings the app didn't write — git remote errors, agent output, server replies. ## Why `ErrorBanner` → `Banner` The issue allowed either extending `ErrorBanner` or generalising to a `Banner` it delegates to. I renamed it (`git mv`, same file extended) rather than leaving a delegating alias: it renders the success variant now, and **a component named for one of its three variants is what invited the 33 copies in the first place**. Two names for one thing would leave every future author guessing which to reach for. All former call sites pass identical props under the new name. ## Migration All 33 sites, grouped: app shell + top-level routes (6 files), repo/issue routes (7), repo-settings (10), global settings (4). Markup is preserved **byte-for-byte** at every site — same classes in the same order, same `role`, same text — so there is no visual change. Specifics worth reviewing: - **No `onDismiss` was invented anywhere.** Every migrated site had no dismiss button and still has none; the ~24 pre-existing dismissible sites keep theirs. - Every `<Match>` wrapper (a `<Switch>` branch) is kept. An enclosing `<Show>` was collapsed **only** where its condition was exactly the message — `<Show when={form.note()}>` becomes `<Banner message={form.note()} variant="success" />`, since `Banner` does that truthiness `Show` internally. Where the condition was something else (`paused()`, `clone_status === 'error'`, `repo.error !== undefined`), the `<Show>` stays and the `<Banner>` goes inside it. - Tokens' reveal banner had its text JSX-interpolated across lines; it becomes an equivalent template literal producing the identical single-line text. ## Tests New colocated `Banner.test.tsx` (15 cases, `SectionCard`/`FormCard` style) pinning: variant and role asserted **as one pair** per variant (split assertions would pass while drifting apart); the text-not-HTML property (asserted via `querySelector('b') === null`, since `textContent` can't tell an escaped `<b>` from a live one); absent-vs-disabled dismiss; exact class strings so *order* is the contract; and the action slot's position between text and dismiss. ## Verification Full gate, locally, all green: - `npm run lint` — clean - `npm run format:check` — clean - `npm test` — **72 files, 1030 tests passing** (was 71 / 1015 on `main`; +15 from the new file, no existing test changed to accommodate the refactor) - `npm run build` — succeeds - `grep -rn 'class="banner' web/src --include=*.tsx` outside tests matches **only `Banner.tsx`** - No `ErrorBanner` identifier remains in the tree ## One judgement call to flag `docs/adr/0019` names `ErrorBanner.tsx` in its **Status** section, listing the files that ADR's diff touched at the time. I left it: an ADR is a historical record of a past decision, and rewriting it to say `Banner.tsx` would falsify what was true then. Happy to update it if this repo treats ADR file lists as a live index instead.

dominik.polakovics added 1 commit

2026-08-04 21:23:55 +02:00

refactor(web): all banners through the shared Banner component

ci / native (pull_request) Successful in 8m21s

Details

ca48fc91d8

`ErrorBanner` rendered a dismissible error/notice banner, yet 33 inline
`<div class="banner …">` copies survived across the app shell, the routes
and both settings trees — because the component's API did not cover what
those sites needed: non-dismissible rendering, a success variant, and
extra-class passthrough.

Extend the API to the needs actually found in the codebase and migrate
every copy:

- `variant` gains 'success' (role="status", like 'notice')
- `onDismiss` becomes optional — its absence IS the non-dismissible case,
  so no dismiss button is rendered rather than a disabled one
- `action` slot for the three banners carrying a control (AFK Reset,
  paused-schedule Re-enable, clone Retry)
- `class` appends site modifiers after the variant (`afk-strip-paused`,
  `clone-error`)

`ErrorBanner` is renamed to `Banner` (git mv, same file extended — not a
fork): it renders the success variant too, and a component named for one
of its three variants is what invited the copies in the first place. All
former call sites pass identical props under the new name.

Markup is preserved byte-for-byte at every migrated site — same classes in
the same order, same role, same text — so this is no visual change. The
message stays a `string` prop and is never a slot: text-only, never HTML,
is the v0 sticky-banner property the component exists to guarantee.

Adds a colocated `Banner.test.tsx` (15 cases) pinning variant/role as one
decision, the text-not-HTML property, absent-vs-disabled dismiss, class
order, and the action slot's position between text and dismiss.

Closes #283

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

dominik.polakovics commented

2026-08-04 21:34:23 +02:00

Author

Owner

[autoland] verdict: pass

PASS — verified against the green ci / native (pull_request) aggregate (npm run lint, format:check, npm test, npm run build, plus the Go build/test/lint legs); no local re-run needed. ci-nix correctly did not run — it is path-gated on **/*.nix, flake.lock, go.mod, go.sum, none of which this diff touches.

Conventions: Conventional Commits title, Closes #283 present, head branch afk/283, branch is a fast-forward of main (d4ea730) — no conflict.

Diff scope matches issue #283's acceptance criteria, each verified:

Banner.tsx gains 'success' variant, optional onDismiss (absence = no button, not a disabled one), action slot and class passthrough — extended in place via git mv, not forked.
grep 'class="banner' web/src --include=*.tsx returns only Banner.tsx (component internals) and Banner.test.tsx fixtures; no ErrorBanner identifier remains outside Banner.tsx's explanatory comments.
Markup preserved byte-for-byte at the sites I spot-checked (AFKStrip.tsx:99, Repos.tsx:180, Schedules.tsx:307, Tokens.tsx:141, App.tsx:31): same class order, same role, same text. Tokens' multi-line JSX text collapses to exactly the new template literal.
collapse is sound: it happens only where the condition was the message itself; paused()/clone_status/repo.error guards are kept.
No behavioral regression at the sites now nesting Banner's internal : errorMessage (web/src/api/core.ts:23) always returns a non-empty string.
Banner.test.tsx (15 cases) pins variant+role as pairs, text-not-HTML via querySelector('b'), absent-vs-disabled dismiss, exact class strings, and action-slot position.

The author's call to leave docs/adr/0019's Status file list naming ErrorBanner.tsx is right — that section records the diff that ADR shipped at the time, not a live index.

[autoland] verdict: pass PASS — verified against the green `ci / native (pull_request)` aggregate (npm run lint, format:check, npm test, npm run build, plus the Go build/test/lint legs); no local re-run needed. ci-nix correctly did not run — it is path-gated on **/*.nix, flake.lock, go.mod, go.sum, none of which this diff touches. Conventions: Conventional Commits title, `Closes #283` present, head branch afk/283, branch is a fast-forward of main (d4ea730) — no conflict. Diff scope matches issue #283's acceptance criteria, each verified: - Banner.tsx gains 'success' variant, optional onDismiss (absence = no button, not a disabled one), `action` slot and `class` passthrough — extended in place via git mv, not forked. - grep 'class="banner' web/src --include=*.tsx returns only Banner.tsx (component internals) and Banner.test.tsx fixtures; no ErrorBanner identifier remains outside Banner.tsx's explanatory comments. - Markup preserved byte-for-byte at the sites I spot-checked (AFKStrip.tsx:99, Repos.tsx:180, Schedules.tsx:307, Tokens.tsx:141, App.tsx:31): same class order, same role, same text. Tokens' multi-line JSX text collapses to exactly the new template literal. - <Show> collapse is sound: it happens only where the condition was the message itself; paused()/clone_status/repo.error guards are kept. - No behavioral regression at the <Match> sites now nesting Banner's internal <Show>: errorMessage (web/src/api/core.ts:23) always returns a non-empty string. - Banner.test.tsx (15 cases) pins variant+role as pairs, text-not-HTML via querySelector('b'), absent-vs-disabled dismiss, exact class strings, and action-slot position. The author's call to leave docs/adr/0019's Status file list naming ErrorBanner.tsx is right — that section records the diff that ADR shipped at the time, not a live index.

dominik.polakovics merged commit dc62f8c278 into main

2026-08-04 21:34:27 +02:00

dominik.polakovics referenced this pull request from a commit

2026-08-04 21:34:29 +02:00

Merge pull request 'refactor(web): all banners through the shared Banner component — success variant, non-dismissible, action slot, extra classes' (#289) from afk/283 into main

dominik.polakovics referenced this pull request

2026-08-04 21:56:11 +02:00

refactor(web): one shared head row per kind — extract SectionHead, fold the rest into SectionCard #286

dominik.polakovics referenced this pull request

2026-08-04 21:56:24 +02:00

refactor(web): one shared head row per kind — extract SectionHead, fold the rest into SectionCard #286

dominik.polakovics referenced this pull request