Skip to content

feat(chat): surface background streams as per-tab status dots - #409

Open
sneumannb5 wants to merge 3 commits into
byte5ai:mainfrom
sneumannb5:feat/lume-stream-toasts-conflict
Open

feat(chat): surface background streams as per-tab status dots#409
sneumannb5 wants to merge 3 commits into
byte5ai:mainfrom
sneumannb5:feat/lume-stream-toasts-conflict

Conversation

@sneumannb5

Copy link
Copy Markdown
Contributor

What

Replace the floating StreamToasts overlay with per-tab status dots on each chat tab, so background streams surface
in-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 test in web-ui (0 errors, 173 tests pass)

Risk / blast radius

UI-only. No schema, API, or env-var changes. i18n de/en updated.

@sneumannb5
sneumannb5 force-pushed the feat/lume-stream-toasts-conflict branch from fea9770 to 9665ed4 Compare July 3, 2026 06:28

@ConnysCode ConnysCode left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 StreamToasts or its streamToasts i18n namespace anywhere (grep across the head clone — only docs/** mention it now). No orphaned useTranslations('streamToasts').
  • dismiss('done') on select is safe. Messages render from activeSession.messages and the liveness pill from message.liveness — both independent of the stream record. The only per-session store read on the active tab is agentUnavailableSlug (chat/page.tsx:547), which a done record never carries. Deleting the record on select breaks nothing inline; error / aborted / running are correctly kept so the recovery banner, stop button, and composer lock stay correct.
  • i18n parity holds. streamToasts.* removed from both de.json and en.json; three chatTabs.* keys added to both with matching {title} placeholders, so i18n-parity.test.ts (key-set + placeholder equality) stays green.
  • tabStreamState matrix is correct — active → no dot, aborted → no dot, running / done / error as 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's aria-label / title repeat 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.)

Comment thread web-ui/app/_components/ChatTabs.tsx
Comment thread web-ui/app/_components/ChatTabs.tsx
@Weegy
Weegy enabled auto-merge (squash) July 28, 2026 15:00

@ConnysCode ConnysCode left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.md10 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

  1. Restore docs/CHANGELOG.md from main and re-apply only the new [Unreleased] block — docs/CHANGELOG.md:1256.
  2. Give done and error a visible non-colour differentiator, and stop filling with --dangerChatTabs.tsx:106.
  3. Add the tabStreamState / dismiss test — ChatTabs.tsx:83.
  4. Fix the foreground-done re-flag — chat/page.tsx:456.
  5. Add a polite live region for background-stream terminal transitions — layout.tsx:141.
Nits (non-blocking)
  • ChatTabs.tsx:94streamAriaKey and streamDotClass both end in an unguarded fall-through returning the success mapping, and both return string, so TS can't catch a missing arm. Add a new TabStreamState and it silently renders a green "response ready" dot, typecheck clean. The component being deleted did this right: phaseLabelFor in StreamToasts.tsx was an exhaustive switch with no default. Consider restoring that shape.
  • ChatTabs.tsx:215aria-label and title are 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 a done dot the tab announces as "Deploy notes Deploy notes: response ready Close tab: Deploy notes" — the {title} interpolation in the three new chatTabs.* 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 /chat entirely. StreamToasts was mounted in the root layout and rendered on every route; ChatTabs renders only from chat/page.tsx:465. Sitting on /admin or /store while 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 StreamRecord fields write-only — previewTail, toolName, tokensIn, tokensOut, cacheTokens (_lib/streamStore.tsx:43-54) now have writers but no readers. derivePhasePatch (_components/StreamRunner.tsx:286) still calls tailOf() on every text_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 on previewTail being 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,272 and _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-133 has 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 StreamToasts removal itself is clean. No dangling import or reference in any .ts/.tsx; the layout.tsx import and mount are both gone and the comment was rewritten rather than orphaned. No orphaned CSS (lume-busy-dots, --danger-edge, --state-loading all 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-312 and is untouched. No leak.
  • The phase matrix is complete. All seven StreamPhase values (pending | thinking | streaming | tool_running | done | error | aborted) are covered, and aborted → no dot is deliberate and matches ADR-0006:55-60.
  • prefers-reduced-motion is handledglobals.css:813-822 is a universal reset that does cover animate-pulse. Not a separate finding (its consequence is folded into blocker 2).
  • The dot's size and pulse are house stylesize-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 matches DevJobPhaseRail.tsx:175. Hook usage is legaluseStreamRecord is called unconditionally at ChatTabs.tsx:139. No re-render regression — the strip already re-rendered per token via useStreamStore() in ChatPage.
  • docs/adr/README.md indexes 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.)

Comment thread docs/CHANGELOG.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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.md

then 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)]`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; abortedno dot (the intentional gap, currently protected by nothing)
  • editing suppresses the dot (:212)
  • the correct aria key per state
  • both dismiss transitions: background-done clears on select, and a foreground-done must 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.

Comment thread web-ui/app/chat/page.tsx
// 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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread web-ui/app/layout.tsx
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:589aria-live="polite" on the message scroller, but it wraps activeSession.messages only (: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 static role="img" with an aria-label. Not a live region; it announces only if the user happens to navigate onto it — and the tab strip has no tabIndex or 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lume: stream toasts conflict with the no-toasts rule (visual-spec §7.6)

3 participants