Enhance spoken alarm feedback and controls - #334
Conversation
Deploying mouseterm with
|
| Latest commit: |
2d35049
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://69c69c07.mouseterm.pages.dev |
| Branch Preview URL: | https://speak-alarms.mouseterm.pages.dev |
Review findings on the speak-alarms branch, all of them in the two feature commits rather than the merge. Speech lifecycle (`alert-speech.ts`): - `onEnd`/`onError` were guarded by `if (utterance)`, but `utterance` was only assigned after `speak()` returned. An engine that dispatches `start` then `end`/`error` synchronously inside `synth.speak()` — Chrome reports `not-allowed` that way when speech runs without a user gesture, which is this exact call site — had its settle silently dropped, pinning the Session at SPEAKING for the rest of the ring. Handlers now close over the utterance itself and registration happens before dispatch; a refused dispatch settles too. - The disposer detached handlers but never called `speechSynthesis.cancel()`, so a webview unmounting mid-alarm kept reading Pane names aloud with no visible source and no UI left to stop it. - The tracking Set was only drained by `settle()`, which a WebKit-dropped utterance never reaches — so a wedged synthesizer accumulated handler closures for the life of the app. Bounded by evicting the oldest. - `clearResolvedSpeech` ran on every activity notification and rebuilt the activity Map that notification had just invalidated; it now bails when no delivery state exists. Its seed call was provably a no-op and is gone. Chrome: - Door: `spoken` persists until the ring is attended, which is unbounded, so it no longer evicts the bell and TODO pill — it adds a speaker icon beside them. `speaking` keeps the full takeover; it lasts one utterance. - AlertSpeechIndicator: `.lath-leaf-header` is a stacking context at z-20, which traps its own `z-[1000]` popovers there, so the single z-25 overlay covered and tinted every header menu. Split into a wash+label layer at z-19 (below the header, above terminal content) and a perimeter ring at z-25 (still outlines the whole Pane, covers nothing). Geometry now derives from the design constants instead of a hardcoded 34px and `rounded-lg`. - Baseboard: measured door widths fed a ref while the fitting budget is computed during render, so the frame that first showed a wide SPEAKING/SPOKEN door fitted against the old widths with no re-render to correct it. Now state. - The pulse honors `cfg.alert.ringingPaused` like the bell, so the new Speaking stories stop producing non-deterministic Chromatic snapshots. Its dead `motion-reduce:animate-none` (unreachable beside `motion-safe:`) is gone. - LathHost gained a generic per-leaf overlay slot resolved through the same registry as Body/Tab, replacing the hardcoded `component === 'terminal'` branch in the content-agnostic engine adapter. Specs updated to match, including an alert.md bullet that said asterisks both "become spaces" and "are removed". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…h chrome
Cleanup pass over the previous commit. One correction and several trims.
The correction: the previous commit split the pane overlay into two layers on
the grounds that `.lath-leaf-header`'s stacking context traps its own popovers,
so a single z-25 overlay would cover them. That premise is wrong — every header
popover (pane context menu, title candidates, notification preview, rename
warning) portals to `document.body` with `position: fixed`, so none of them was
ever inside the leaf. The split is still worth keeping for the reasons it
actually delivers — the wash no longer tints the header band, where
`--color-alarm-vs-terminal` is a contrast pick computed against the terminal
body, nor the z-20 pane-corner banners — so the layers stay and the rationale is
rewritten in the component, the spec, and the test name.
Following the Door's own rule to its conclusion: SPOKEN no longer washes the
terminal body either. It persists until the ring is attended, which is
unbounded, and degrading terminal-text contrast for that whole window is the
same mistake the Door's badge cluster was just fixed to avoid. The ring carries
it; SPEAKING keeps the wash, lasting one utterance.
Also:
- `alertSpeakingAnimationClass` moves to `bell-icon-class.ts`, beside the other
Chromatic-frozen alert animation, so `cfg` stops leaking into design.tsx.
- Class strings in Door and AlertSpeechIndicator use `clsx` (already a direct
dependency, and design.tsx's own idiom) instead of `[...].join(' ')` with
empty-string placeholders, which emitted stray double spaces.
- `track()`'s while-loop becomes an `if`: it is the only insertion point and
adds one entry per call, so the size can never exceed the cap.
- Comments trimmed where they overclaimed: the LathHost overlay registry does
not decouple anything (LathHost still imports the indicator), the Baseboard
equality guard prevents an extra render rather than a loop, and the
`clearResolvedSpeech` early-out saves nothing measurable because
`getActivitySnapshot` memoizes and Baseboard rebuilds it in the same
notification.
- Tests: the utterance-bounding test asserted a fact that held regardless of the
cap; it now pins the tracked tail exactly. Dropped two assertions that could
not fail and one duplicate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ring origin/main replaced the restored-pane resume offer with the new recovery flow and deleted ResumeBanner, so the spoken-alarm overlay's z-order note named a banner that no longer exists. MouseOverrideBanner is now the only z-20 pane-corner banner the wash has to stay under. Textual merge was clean; this is the semantic half. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
Reviewing as a draft — flagging anything that looks worth a quick fix. Mark ready for a full review.
Read through the speech lifecycle, the store, the overlay layering, and the spec updates; the synchronous-settle handling, the generation token, and the speechSynthesis.cancel() on teardown all hold up, and the test coverage tracks them closely. One thing stood out.
The two new baseboard status buttons use the standard mute-toggle icon pair (SpeakerHigh/SpeakerSlash, BellRinging/BellSlash), so a slashed speaker labelled "Spoken alarms disabled" reads as "click to enable". The aria-labels do say ; open Alarm settings, but title is the only affordance hint a sighted mouse user gets and it omits the action entirely — so exactly the users the icon misleads are the ones not told what the click does. Suggestions below bring title in line with aria-label; if the icons are meant to look like toggles, the alternative is making them actually toggle speakEnabled/pushEnabled and leaving the sliders button as the way into the dialog.
| <button | ||
| className={`${BASEBOARD_BUTTON_CLASS} ${settings.speakEnabled ? 'text-app-fg' : ''}`} | ||
| aria-label={`Spoken alarms ${settings.speakEnabled ? 'enabled' : 'disabled'}; open Alarm settings`} | ||
| title={`Spoken alarms ${settings.speakEnabled ? 'enabled' : 'disabled'}`} |
There was a problem hiding this comment.
| title={`Spoken alarms ${settings.speakEnabled ? 'enabled' : 'disabled'}`} | |
| title={`Spoken alarms ${settings.speakEnabled ? 'enabled' : 'disabled'} — open Alarm settings`} |
| <button | ||
| className={`${BASEBOARD_BUTTON_CLASS} ${settings.pushEnabled ? 'text-app-fg' : ''}`} | ||
| aria-label={`Push notifications ${settings.pushEnabled ? 'enabled' : 'disabled'}; open Alarm settings`} | ||
| title={`Push notifications ${settings.pushEnabled ? 'enabled' : 'disabled'}`} |
There was a problem hiding this comment.
| title={`Push notifications ${settings.pushEnabled ? 'enabled' : 'disabled'}`} | |
| title={`Push notifications ${settings.pushEnabled ? 'enabled' : 'disabled'} — open Alarm settings`} |
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
Full review now that this is out of draft. The speech lifecycle holds up under a close read — the synchronous-settle path, the generation token, the speechSynthesis.cancel() on teardown, and the doorWidthsRef → state change (the fitting budget runs during render, so a ref write really would have stranded a stale frame) are all right, and the tests pin the tricky parts rather than the easy ones. One new thing, plus the two baseboard title threads from the draft pass, which are still open.
The SPOKEN invariant this PR states in three places — docs/specs/layout.md ("A Door that hid both persistent signals for that whole window would be indistinguishable from a quiet one"), DESIGN.md, and the comment on Door.test.tsx's third case — holds visually but not in the accessible name. aria-label replaces the whole computed name rather than adding to it, so a Door rendering title + the TODO pill announces as build-server, spoken: the TODO is dropped from the accessible name for exactly the unbounded window the rest of the design goes out of its way to protect. SPEAKING is fine — the badge cluster genuinely is replaced there, and it lasts one utterance — so the fix only needs to cover the persistent state. Suggestion inline. (The bell contributes nothing to the name either way; the Phosphor <svg> carries no accessible text, so TODO is the only signal actually lost.)
| onPointerDown={onPointerDown} | ||
| title={title} | ||
| title={speechState ? `${title} — ${speechState}` : title} | ||
| aria-label={speechState ? `${title}, ${speechState}` : undefined} |
There was a problem hiding this comment.
| aria-label={speechState ? `${title}, ${speechState}` : undefined} | |
| aria-label={speechState ? `${title}${todo ? ', TODO' : ''}, ${speechState}` : undefined} |
aria-label overrides the computed name outright, so while spoken is set the TODO pill stops being announced — for the whole unbounded window layout.md and the test comment below argue it must not go dark. Using todo rather than todoPill.visible keeps the flourish (a TODO that just cleared) out of the name.
2 participants
Summary
SPEAKING/SPOKENstate until the alarm is deliberately attendedWhy
Spoken alarms were difficult to associate with a terminal, and labels containing
*produced confusing announcements such as “eight asterisk.” The settings affordance also hid whether speech and push delivery were enabled. This makes delivery state unmistakable while keeping resolution tied to deliberate attention rather than mere visibility.Validation
pnpm testpnpm --filter dormouse-lib buildpnpm --filter dormouse-lib build-storybooknode scripts/spec-lint.mjsgit diff --check