feat(chat): surface background streams as per-tab status dots#409
feat(chat): surface background streams as per-tab status dots#409sneumannb5 wants to merge 2 commits into
Conversation
fea9770 to
9665ed4
Compare
ConnysCode
left a comment
There was a problem hiding this comment.
Review: Request changes
Two things to change before merge — both non-cosmetic. The implementation itself is sound: I verified the removal is clean, the done-dismiss is safe, and i18n parity holds (details below).
Must change before merge
🟠 a11y — error vs done dots are distinguished by colour alone. All three dots share shape and size; running carries a non-colour cue (animate-pulse), but done (accent) and error (danger) differ only in hue. The aria-label/title cover screen-reader and hover users, but a colour-blind operator glancing at the tab strip can't tell "ready" from "failed" without hovering — and ADR-0006 elevates §8 ("colour is never the sole signal") to ship-blocking. Please give the error dot a non-colour cue (e.g. a ring, or a hollow-vs-filled shape) so the distinction survives without hover. Detail inline on ChatTabs.tsx:106.
🟠 The new state logic ships untested. tabStreamState and the done-only dismiss in handleSelect (chat/page.tsx:441) encode the subtlest part of this change — no dot on the active tab, aborted → no marker, forget only done while keeping error/aborted/running. The repo already unit-tests pure logic (29 test files, e.g. i18n-parity.test.ts). Please add a test covering the tabStreamState matrix (active / no-record / running / done / error / aborted) and the handleSelect dismiss rule before merge, so the contract ADR-0006 describes can't silently regress. Detail inline on ChatTabs.tsx:83.
Verified against the head clone
- Clean removal. No dangling references to
StreamToastsor itsstreamToastsi18n namespace anywhere (grep across the head clone — onlydocs/**mention it now). No orphaneduseTranslations('streamToasts'). dismiss('done')on select is safe. Messages render fromactiveSession.messagesand the liveness pill frommessage.liveness— both independent of the stream record. The only per-session store read on the active tab isagentUnavailableSlug(chat/page.tsx:547), which adonerecord never carries. Deleting the record on select breaks nothing inline;error/aborted/runningare correctly kept so the recovery banner, stop button, and composer lock stay correct.- i18n parity holds.
streamToasts.*removed from bothde.jsonanden.json; threechatTabs.*keys added to both with matching{title}placeholders, soi18n-parity.test.ts(key-set + placeholder equality) stays green. tabStreamStatematrix is correct — active → no dot,aborted→ no dot,running/done/erroras documented in the ADR.
PR description — factcheck
| Claim | Reality |
|---|---|
| "UI-only. No schema, API, or env-var changes." | ✅ Only web-ui/** .tsx, messages/{de,en}.json, and docs/** touched. |
| "i18n de/en updated." | ✅ Both locales updated symmetrically; parity test unaffected. |
| "0 errors, 173 tests pass." | Not re-run here (heavy Next.js install skipped). Consistent on inspection — no dangling refs, parity test stays green, types align. |
| "Closes #286." | ✅ Option 2. The issue's illustrative "inline status row in the affected session" is already covered by the existing inline streaming UI + inline errors, per ADR-0006. |
Nits (non-blocking)
web-ui/app/_components/ChatTabs.tsx:215— the dot'saria-label/titlerepeat the session title ("{title}: response ready") while the dot sits inside a tab that already exposes that title, so a screen reader announces the title twice. Consider dropping{title}from the dot label (just"response ready"); the tab context already supplies it.
(Reviewed against head 9665ed4.)
| * each dot carries an aria-label + title (§8). */ | ||
| function streamDotClass(state: TabStreamState): string { | ||
| const base = 'ml-1 inline-block size-1.5 shrink-0 rounded-full'; | ||
| if (state === 'error') return `${base} bg-[color:var(--danger)]`; |
There was a problem hiding this comment.
error and done dots are distinguished by colour alone. All three dots share the same shape and size; running carries a non-colour cue (animate-pulse), but done (accent) and error (danger) differ only in hue. The aria-label / title cover screen-reader and hover users, but a colour-blind operator glancing at the tab strip can't tell "ready" from "failed" without hovering — and ADR-0006 itself elevates §8 ("colour is never the sole signal") to ship-blocking. Please give the error dot a non-colour cue too (e.g. a ring, or a hollow-vs-filled shape) so the distinction survives without hover — that is what makes the §8 claim in the ADR actually hold visually.
| * (active tab, no record, or a user-aborted turn). */ | ||
| type TabStreamState = 'running' | 'done' | 'error'; | ||
|
|
||
| function tabStreamState( |
There was a problem hiding this comment.
The new state logic ships without tests. tabStreamState here (and the done-only dismiss in handleSelect, chat/page.tsx:441) encode the subtlest part of this change: no dot on the active tab, aborted → no marker, and forget only done while keeping error / aborted / running. The repo already unit-tests pure logic (29 test files, e.g. i18n-parity.test.ts), and both of these are pure and trivially testable. Please add a test covering the tabStreamState matrix (active / no-record / running / done / error / aborted) and the handleSelect dismiss rule, so the contract ADR-0006 describes can't silently regress.
What
Replace the floating
StreamToastsoverlay with per-tab status dots on each chat tab, so background streams surfacein-context. Adds ADR 0006. Closes #286
Why
Floating toasts stacked up and detached the stream state from the tab it belonged to. Per-tab dots put liveness where
the user is already looking.
Test plan
npm run lint && npm run typecheck && npm run testin web-ui (0 errors, 173 tests pass)Risk / blast radius
UI-only. No schema, API, or env-var changes. i18n de/en updated.