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 3 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.)
ConnysCode
left a comment
There was a problem hiding this comment.
Review: Request changes
One new blocker that did not exist at my last review, plus the two items from 2026-07-03 that are still unfixed. Verified green at head 17818ed: tsc --noEmit exit 0, eslint on the three changed TS files 0 errors, i18n-validate: OK — 3117 keys, en/de, vitest run 46 files / 319 tests pass.
Must change before merge
🔴 1. This PR now deletes 89 released sections from docs/CHANGELOG.md. A merge resolution kept the branch's stale copy of the file instead of main's. Measured at head vs the merge base b55b360:
| merge base | PR head | |
|---|---|---|
docs/CHANGELOG.md lines |
2276 | 1530 |
## [x.y.z] sections |
92 | 3 (Unreleased, 0.2.0, 0.1.0) |
git diff --numstat b55b360 HEAD -- docs/CHANGELOG.md → 10 756. Everything from ## [0.3.0] through ## [0.54.0] is gone, plus the hand-written ### Fixed block under [Unreleased] for #403 (provider errors humanized across chat surfaces) — which has nothing to do with toasts. CONTRIBUTING.md:130-131 says "Do not hand-edit past version sections". Detail inline on docs/CHANGELOG.md:1256.
Please re-resolve that file taking main's version, then re-apply only the new [Unreleased] block. This is the one thing that makes the PR unsafe to merge as-is.
🔴 2. §8 "colour is never the sole signal" is still not satisfied — my 2026-07-03 ask, unchanged. I've now read the actual spec (byte5ai/omadia-ui is public; docs/visual-spec.md:1589-1590) and it is stricter than the code comment assumes: a text label or an icon, not an aria-label. Two independent collapses, both provable from this repo. Detail inline on ChatTabs.tsx:106.
🟠 3. The new state logic still ships with no test — my other 2026-07-03 ask, unchanged. Detail inline on ChatTabs.tsx:83.
🟠 4. A turn you watched finish re-flags its own tab the moment you switch away. handleSelect only ever inspects the tab being entered, never the one being left — so the exact case its own comment says it prevents is the case it misses. Detail inline on chat/page.tsx:456.
🟠 5. No screen-reader announcement of background streams survives the removal. The deleted overlay's container carried aria-live="polite"; nothing replaced it. Detail inline on layout.tsx:141.
Since my last review
Nothing in the feature changed. The three commits on this branch are the original feature commit (9665ed4, 2026-07-02 — before my review) and two Merge branch 'main' commits. Verified: git diff 9665ed4 HEAD -- web-ui/app/_components/ChatTabs.tsx is empty, and the handleSelect block in chat/page.tsx is byte-identical too.
Both of my inline threads are currently marked resolved on GitHub, and the code each one points at is unchanged. I'm re-raising them on the same two lines rather than reopening the old threads. If they were resolved because you disagree with the finding, say so on the thread and I'll engage with the argument — the resolve on its own leaves no record of the reasoning.
PR description — factcheck
| Claim | Reality |
|---|---|
"npm run lint && npm run typecheck && npm run test in web-ui (0 errors, 173 tests pass)" |
✅ Still green, count is just stale after the merges: 319 tests / 46 files, tsc exit 0, lint 0 errors (4 pre-existing _drop warnings in chat/page.tsx, not from this PR). |
| "i18n de/en updated" | ✅ Accurate. All 17 streamToasts.* keys removed from both locales with no surviving references; 3 new chatTabs.* keys present and genuinely translated in both. i18n-validate exits 0. |
| "UI-only. No schema, API, or env-var changes." | ✅ True of the code. |
| "Risk / blast radius: UI-only" | ❌ Not true as the branch stands. The diff removes 756 lines of tracked release documentation (blocker 1). That is the largest single change in the PR and isn't mentioned. |
Minimum to merge
- Restore
docs/CHANGELOG.mdfrommainand re-apply only the new[Unreleased]block —docs/CHANGELOG.md:1256. - Give
doneanderrora visible non-colour differentiator, and stop filling with--danger—ChatTabs.tsx:106. - Add the
tabStreamState/ dismiss test —ChatTabs.tsx:83. - Fix the foreground-
donere-flag —chat/page.tsx:456. - Add a polite live region for background-stream terminal transitions —
layout.tsx:141.
Nits (non-blocking)
ChatTabs.tsx:94—streamAriaKeyandstreamDotClassboth end in an unguarded fall-through returning the success mapping, and both returnstring, so TS can't catch a missing arm. Add a newTabStreamStateand it silently renders a green "response ready" dot, typecheck clean. The component being deleted did this right:phaseLabelForinStreamToasts.tsxwas an exhaustiveswitchwith nodefault. Consider restoring that shape.ChatTabs.tsx:215—aria-labelandtitleare set to the identical string, so several screen readers speak the sentence twice; contrast:226-227, where the close button's two attributes deliberately differ. Separately,role="tab"takes its name from content, so with adonedot the tab announces as "Deploy notes Deploy notes: response ready Close tab: Deploy notes" — the{title}interpolation in the three newchatTabs.*messages is redundant because the dot is never focusable on its own. Consider dropping{title}from those keys.docs/adr/0006-…md:53— Consequences don't mention that background streams are now invisible off/chatentirely.StreamToastswas mounted in the root layout and rendered on every route;ChatTabsrenders only fromchat/page.tsx:465. Sitting on/adminor/storewhile a turn finishes now surfaces nothing at all. Consider adding it as an accepted 🔴 so it's a decision rather than an omission.- Deleting the overlay left five
StreamRecordfields write-only —previewTail,toolName,tokensIn,tokensOut,cacheTokens(_lib/streamStore.tsx:43-54) now have writers but no readers.derivePhasePatch(_components/StreamRunner.tsx:286) still callstailOf()on everytext_delta, running a regex over the whole growing answer buffer per token to build a 160-char string nobody reads. ADR-0006:28-29 partly justifies the chosen option onpreviewTailbeing available, but the dot never uses it. Consider dropping them or recording the intent. - Eight comments in surviving code still describe the deleted toasts —
_lib/streamStore.tsx:24,105,272and_components/StreamRunner.tsx:90,97,129,331. The one that matters most is_components/__tests__/StreamRunner.test.tsx:18, the doc-comment of a live test, which states its invariant in terms of a component that no longer exists. docs/CHANGELOG.md:1256—### Changed — background chat streams….CONTRIBUTING.md:121-133has the generator emitting plain### Added/### Changed/### Fixed, so a suffixed heading will sit alongside a second, differently-named Changed heading at release time.
Checked and deliberately not raised
- The
StreamToastsremoval itself is clean. No dangling import or reference in any.ts/.tsx; thelayout.tsximport and mount are both gone and the comment was rewritten rather than orphaned. No orphaned CSS (lume-busy-dots,--danger-edge,--state-loadingall still have other consumers). - No provider or side effect was lost with the mount. I read the deleted file at the merge base in full: it rendered no provider, had no module-level side effect, and did no GC of finished records — the 30 s sweep / 5 min expiry lives in
_lib/streamStore.tsx:296-312and is untouched. No leak. - The phase matrix is complete. All seven
StreamPhasevalues (pending | thinking | streaming | tool_running | done | error | aborted) are covered, andaborted→ no dot is deliberate and matches ADR-0006:55-60. prefers-reduced-motionis handled —globals.css:813-822is a universal reset that does coveranimate-pulse. Not a separate finding (its consequence is folded into blocker 2).- The dot's size and pulse are house style —
size-1.5+animate-pulse+ accent is the repo's established busy-dot (chat/page.tsx:1730,PlanProgressCard.tsx:64). 6px isn't the problem; the missing second channel is. role="img"on the span is valid and matchesDevJobPhaseRail.tsx:175. Hook usage is legal —useStreamRecordis called unconditionally atChatTabs.tsx:139. No re-render regression — the strip already re-rendered per token viauseStreamStore()inChatPage.docs/adr/README.mdindexes 0006 correctly (number, link, status, date all match the ADR).- The ADR's §7.6 premise is accurate — spec:1561 does list toasts as an anti-pattern and spec:1631-1632 does make them ship-blocking.
- The tab strip's missing roving tabindex / keyboard nav (
ChatTabs.tsx:177-193) is pre-existing, not introduced here, so I'm not blocking on it — though it does undercut the "the aria-label makes it accessible" argument, since a keyboard user can't reach that label.
(Reviewed against head 17818ed; spec quotes from byte5ai/omadia-ui@main:docs/visual-spec.md.)
|
|
||
| - **ci**: set git identity before annotated release tag (#218) | ||
| - **builder**: ctx.memory in preview runtime, accessor permission lint, and setup_fields rename (#207) | ||
| ### Changed — background chat streams surface in-context, not as toasts |
There was a problem hiding this comment.
This hunk deletes 89 released changelog sections along with adding the new entry. The hunk header is @@ -1253,762 +1253,16 @@ — 756 lines removed, 10 added.
Measured at head vs the merge base b55b360:
merge base docs/CHANGELOG.md: 2276 lines, 92 "## [x.y.z]" sections
PR head docs/CHANGELOG.md: 1530 lines, 3 "## [x.y.z]" sections
git diff --numstat b55b360 HEAD -- docs/CHANGELOG.md -> 10 756
The only sections left are [Unreleased], [0.2.0] and [0.1.0]. Everything from ## [0.3.0] through ## [0.54.0] is gone. Also gone: the hand-written ### Fixed block that main has under [Unreleased] for #403 (provider errors surfaced as the provider's human-readable sentence across chat surfaces) — unrelated to this change, and not recoverable from the generator.
Cause: the branch was cut when the changelog was still short, and both Merge branch 'main' commits resolved this file in favour of the branch copy. All three commits on the branch carry the truncated file, so re-merging main again will not fix it on its own. Note the file's own preamble at :11-12 still claims it mirrors "every section from [0.2.1] onward", which this commit makes false. CONTRIBUTING.md:130-131 is explicit: "Do not hand-edit past version sections".
Please restore the file from main and re-apply only the new block:
git checkout origin/main -- docs/CHANGELOG.mdthen re-add the ### Changed — background chat streams… bullets under ## [Unreleased], keeping main's existing ### Fixed block. Per CONTRIBUTING.md:126-130 the generated region can also be rebuilt with node .github/scripts/generate-changelog.mjs backfill, but the hand-written #403 entry is not generated — restore that one by hand.
| * 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.
Still open from my 2026-07-03 review — and the spec is stricter than this code assumes. byte5ai/omadia-ui is public, so I read the real text rather than the summary. §8 (docs/visual-spec.md:1589-1590):
Colour as sole signal: forbidden. Every state communicated through colour also carries a text label, an icon, or both.
An aria-label is neither — it's invisible — and title needs a hover, so it never fires on touch or keyboard. §8's sibling bullets are contrast, focus rings, hit targets, reduced motion: a perceivable-by-sight floor. This is also WCAG 2.2 SC 1.4.1 (Level A), which §8:1580 claims conformance to. What actually differs here:
| state | classes |
|---|---|
running |
size-1.5 rounded-full bg-[var(--accent)] animate-pulse |
done |
size-1.5 rounded-full bg-[var(--accent)] |
error |
size-1.5 rounded-full bg-[var(--danger)] |
Same size, same shape, no border, no glyph. Two independent collapses, both provable from this repo:
(a) running vs done are pixel-identical under reduced motion. app/globals.css:813-822 is a universal reset (animation-duration: 0.01ms !important; animation-iteration-count: 1 !important). It correctly covers animate-pulse — which means the pulse finishes instantly back at opacity: 1, and the only thing separating "still running" from "finished" is gone.
(b) done vs error are hue-only, and on Atelier + dark the spec's own escape hatch doesn't apply. The spec anticipates the tight separation at §2.6:477-478 and argues "L+C separation takes over there (error at L 0.45 hue 25 vs Atelier at L 0.57 hue 50…)" — but that is stated for light mode. In dark mode, by the spec's own tables:
state.error.fg dark OKLCH 0.75 0.12 25 (visual-spec.md:464)
Atelier accent dark OKLCH 0.76 0.12 60 (visual-spec.md:350)
Identical chroma, lightness within 0.01 — hue is the only separator, and hue is exactly what a red-green CVD user cannot use. Atelier is user-selectable (app/_lib/uiPrefs.ts). A user with both reduced motion and deuteranopia sees all three states as the same dot.
(c) Separately, this line violates §2.6. --danger → --state-error-fg (theme.css:142,77), and the spec marks that token "Error text (never as pill bg)" (visual-spec.md:453) under "Intentionally text-only — never filled pills, badges or block fills" (:444-445). A filled 6px circle is a block fill. §10:1621-1622 makes §2 token semantics authoritative.
(d) The component being deleted was compliant. StreamToasts rendered a distinct glyph per phase — ✓ done, ✗ error, ⏹ aborted, … running — plus a visible text label via phaseLabelFor. This is a regression against it.
Please give each terminal state a visible non-colour cue and tint a glyph instead of filling. lucide-react is already a dependency, and §2.12:739-743 sanctions a monochrome single-stroke line glyph:
{state === 'running' && <span className="ml-1 inline-block size-1.5 shrink-0 animate-pulse rounded-full bg-[color:var(--accent)]" />}
{state === 'done' && <Check size={12} className="ml-1 shrink-0 text-[color:var(--success)]" />}
{state === 'error' && <AlertTriangle size={12} className="ml-1 shrink-0 text-[color:var(--danger)]" />}That resolves (a), (b) and (c) in one change. alert-triangle is also what §7.4:1539 prescribes for the error idiom, and DevJobPhaseRail.tsx:171-173 is the in-house precedent for a compact glyph+colour+title status rail.
| * (active tab, no record, or a user-aborted turn). */ | ||
| type TabStreamState = 'running' | 'done' | 'error'; | ||
|
|
||
| function tabStreamState( |
There was a problem hiding this comment.
Still open from my 2026-07-03 review. tabStreamState here, streamDotClass/streamAriaKey below, and the done-only dismiss in chat/page.tsx:454-458 are pure functions encoding the subtlest part of this change, and none of them has a test. Confirmed at head: grep -rn "ChatTabs\|tabStreamState\|streamDoneAria" web-ui matches only this file and the two message catalogues.
This isn't a hypothetical bar. The house convention covers exactly this: app/_components/__tests__/StreamRunner.test.tsx renders a real <StreamStoreProvider>, captures the live context value, drives it and asserts the terminal record — for the sibling component that produces the very records this file now consumes. Its header comment says it exists because #403 was a state-transition bug. The chat/page.tsx:456 defect I flagged in this review is the same class of bug, and it reproduces in about 40 lines using that harness plus renderWithIntl.
Please add a test covering the matrix before merge:
- active tab + any phase → no dot; no record → no dot
- each of
pending/thinking/streaming/tool_running→ running dot done→ done dot;error→ error dot;aborted→ no dot (the intentional gap, currently protected by nothing)editingsuppresses the dot (:212)- the correct aria key per state
- both dismiss transitions: background-
doneclears on select, and a foreground-donemust not re-flag on switch-away
tabStreamState / streamDotClass / streamAriaKey are module-private, so either export them for a pure-logic test (cheapest, matches app/_lib/__tests__/ style) or drive them through the component.
| // lock) stays correct once the tab is active. | ||
| const handleSelect = (id: string): void => { | ||
| const rec = streamStore.get(id); | ||
| if (rec?.phase === 'done') streamStore.dismiss(id); |
There was a problem hiding this comment.
This misses the exact case the comment above says it exists for. handleSelect reads the record of the tab being entered (id). The record of the tab being left is never touched. The comment at :446-449 says dismiss "exists purely to stop a finished answer re-flagging after the user switches away again" — that is the one flow it doesn't cover, and it's the most common one of all: a turn that runs and completes while you're watching it.
Trace: finish(A,'done') keeps the record with expiresAt = now + 5min (_lib/streamStore.tsx:234, GC_AFTER_MS at :120). While you're on A, tabStreamState short-circuits on active (ChatTabs.tsx:87) so no dot shows. Switch to B → handleSelect('B') inspects B's record, not A's → A goes non-active → tabStreamState(false, {phase:'done'}) → 'done' → A lights up "response ready" for an answer you already read. It stays until the 5-minute GC or until you click back onto A. dismiss has exactly one caller repo-wide (this line), so nothing else covers it.
Second door to the same bug: onCreate={createSession} (:469) sets the active id directly and never goes through handleSelect, so finishing a turn and hitting "+ New chat" flags the tab you just left.
Note this is distinct from the error re-flag that ADR-0006:55-60 documents as intended — that one is deliberate, this one contradicts the comment.
Please drive the dismiss off the active session rather than off the click, so every exit path is covered (tab click, "+ New chat", sidebar, route change):
useEffect(() => {
const rec = streamStore.get(activeId);
if (rec?.phase === 'done') streamStore.dismiss(activeId);
}, [activeId, streamStore]);The store's context value is re-memoized on every record change (_lib/streamStore.tsx:314-339), so this re-fires when the record flips to done and once more after dismiss as a no-op — no loop. If you'd rather keep handleSelect, it must also dismiss the outgoing activeId, and onCreate has to route through the same logic.
| another menu route doesn't kill an in-flight turn. */} | ||
| {/* Headless stream runner — owns the fetch + NDJSON-parse loop | ||
| so switching menu route doesn't kill an in-flight turn. | ||
| Background-stream state surfaces in-context on the chat tab |
There was a problem hiding this comment.
Removing the mount also removed the only live region announcing background streams. The deleted overlay's container carried aria-live="polite" (StreamToasts.tsx:61 at the merge base), so a screen-reader user was told when a background turn finished or failed, with the phase label and error text.
I grepped every aria-live / role="status" / role="alert" under web-ui/app at head. Nothing covers this:
chat/page.tsx:589—aria-live="polite"on the message scroller, but it wrapsactiveSession.messagesonly (:592-606). A background session's messages aren't in that subtree._components/SessionWatcher.tsx:201— session-expiry warning, unrelated.- The new dot (
ChatTabs.tsx:212-219) is a staticrole="img"with anaria-label. Not a live region; it announces only if the user happens to navigate onto it — and the tab strip has notabIndexor arrow-key handling, so keyboard focus can't reach it at all.
Net: a screen-reader user now gets no notification when a background turn completes or errors. Neither ADR-0006 nor the changelog entry acknowledges this — ADR-0006:53-54 lists the lost inline abort as the only 🔴 cost.
Please add a visually-hidden polite live region in the chat shell that announces terminal transitions for non-active sessions, e.g.
<span className="sr-only" role="status" aria-live="polite">{lastTerminalAnnouncement}</span>driven off streamStore. That stays §7.6-clean (no floating surface) and restores parity. Then record the trade-off in ADR-0006 § Consequences.
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.