From f1287f612cdb8ef6616e224aee7baeccf0daf970 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sat, 19 Sep 2026 23:34:45 -0700 Subject: [PATCH] Defer terminal-notification alerts until animation stops by default Coding agents emit their "done" notification while their TUI is still redrawing, so the undeferred ring summoned the user to a pane that was still animating. The switch stays as an escape hatch for the protocols' literal timing, and the AlertManager field now follows DEFAULT_ALERT_SETTINGS so a manager that never receives a settings blob behaves like one that does. Co-Authored-By: Claude Fable 5.1 --- docs/specs/alert.md | 2 +- docs/specs/alert.rationale.md | 2 +- lib/src/lib/alert-manager.test.ts | 16 ++++++++++++++++ lib/src/lib/alert-manager.ts | 6 ++++-- lib/src/lib/alert-settings-host.test.ts | 10 +++++++--- lib/src/lib/alert-settings-model.ts | 2 +- lib/src/lib/alert-settings.test.ts | 7 ++++++- lib/src/stories/SettingsDialog.stories.tsx | 8 ++++---- 8 files changed, 40 insertions(+), 13 deletions(-) diff --git a/docs/specs/alert.md b/docs/specs/alert.md index 7c262f1ed..3a5176df7 100644 --- a/docs/specs/alert.md +++ b/docs/specs/alert.md @@ -261,7 +261,7 @@ Application alarm defaults live beside the WATCHING rule set, edited in **Settin | Field | Meaning | |---|---| | `inactivityTimeoutMs` | `T_USER_ATTENTION` — the walk-away window defined under Attention. | -| `deferAlertsUntilQuiet` | Defer eligible terminal-notification rings while the animation watcher is fully armed. Default off. (rationale) | +| `deferAlertsUntilQuiet` | Defer eligible terminal-notification rings while the animation watcher is fully armed. Default on. (rationale) | | `speakEnabled` / `speakDelayMs` | Spoken alarms, below. | | `pushEnabled` / `pushDelayMs` | Push notifications, below. | diff --git a/docs/specs/alert.rationale.md b/docs/specs/alert.rationale.md index 0fcf750fd..c10711877 100644 --- a/docs/specs/alert.rationale.md +++ b/docs/specs/alert.rationale.md @@ -62,7 +62,7 @@ ## Alarm settings -**Why animation deferral defaults off.** BEL and notification OSCs explicitly ask to alert now, while continuously changing output may never become quiet. Opt-in preserves their established timing and makes indefinite deferral a deliberate choice. +**Why animation deferral defaults on.** Coding agents (`claude`, `codex`) send their notification OSC while their TUI is still redrawing its spinner, so an undeferred ring summons the user to a pane that is still animating (2026-09). The gate engages only while the private detector is fully armed, so a BEL from an otherwise quiet shell still rings at once. Turning the switch off restores the protocols' literal timing, including a ring that continuous output can never quiet. **Why the settings ride the WATCHING rule set's seed/broadcast shape.** Each VS Code webview has its own origin and therefore its own `localStorage`, while the `AlertManager` is shared; without a host-authoritative copy, two webviews would each believe their own blob. The one difference is the whole-blob relay: an alarm setting is not a set of independent keys the way a rule list is. diff --git a/lib/src/lib/alert-manager.test.ts b/lib/src/lib/alert-manager.test.ts index 393a4657e..8367e873d 100644 --- a/lib/src/lib/alert-manager.test.ts +++ b/lib/src/lib/alert-manager.test.ts @@ -102,6 +102,9 @@ describe('AlertManager in isolation', () => { it('ALERT_RINGING latches when user has no attention (view hidden)', () => { const id = 'latch-test'; + // Deferral ships on and withdraws a WATCHING ring once output resumes + // confirmed BUSY; latching through output is the switched-off timing. + manager.setDeferAlertsUntilQuiet(false); runWatchedCommand(id); manager.clearAttention(id); @@ -934,6 +937,17 @@ describe('AlertManager in isolation', () => { }); }); + it('defers a protocol alert with no settings call, because deferral ships on', () => { + const id = 'defer-shipped-default'; + driveToBusy(id); + + manager.notifyFromProtocol(id, { source: 'OSC 9', title: null, body: 'Done' }); + expect(manager.getState(id)).toMatchObject({ todo: false, notification: null }); + + vi.advanceTimersByTime(5_000); + expect(manager.getState(id)).toMatchObject({ status: 'ALERT_RINGING', todo: true }); + }); + describe('defer terminal notifications until quiet', () => { beforeEach(() => { manager.setDeferAlertsUntilQuiet(true); @@ -1489,6 +1503,8 @@ describe('AlertManager in isolation', () => { ['after the detector has noticed the output', 800], ] as const)('leaves a stale WATCHING ring alone once output has resumed, %s', async (_label, gapMs) => { const id = `await-stale-watching-ring-${gapMs}`; + // Keep the latched ring across resumed output: deferral would withdraw it. + manager.setDeferAlertsUntilQuiet(false); driveToRinging(id); // The peer was sent another turn and is talking again. Nothing clears the diff --git a/lib/src/lib/alert-manager.ts b/lib/src/lib/alert-manager.ts index 4c9769aa9..c41a9dacf 100644 --- a/lib/src/lib/alert-manager.ts +++ b/lib/src/lib/alert-manager.ts @@ -1,7 +1,7 @@ import { createAlertEpisode, type AlertEpisode } from './alert-episode'; import { QuiesceDetector, type QuiesceStatus, type QuiesceSnapshot } from './quiesce-detector'; import { applyTerminalProtocolEvents, collectTerminalSemanticEvents, type TerminalProtocolParseResult } from './terminal-protocol'; -import type { AlertSettings } from './alert-settings'; +import { DEFAULT_ALERT_SETTINGS, type AlertSettings } from './alert-settings-model'; import { cfg } from '../cfg'; import { commandArgv0, @@ -239,7 +239,9 @@ export class AlertManager { * drops them here, so a host marks the id once instead of guarding each call. */ private helpers = new Set(); private inactivityTimeoutMs = cfg.alert.userAttention; - private deferAlertsUntilQuiet = false; + /** The shipped default (platform-free module: this runs in both hosts), so a + * manager that never receives a settings blob behaves like one that does. */ + private deferAlertsUntilQuiet = DEFAULT_ALERT_SETTINGS.deferAlertsUntilQuiet; // --- Settings --- diff --git a/lib/src/lib/alert-settings-host.test.ts b/lib/src/lib/alert-settings-host.test.ts index ab8b7b0f3..125247366 100644 --- a/lib/src/lib/alert-settings-host.test.ts +++ b/lib/src/lib/alert-settings-host.test.ts @@ -28,14 +28,18 @@ describe('AlertSettingsHost', () => { it('keeps the first startup seed but always applies an explicit update', () => { const { host, target } = createHost(); - host.initialize({ deferAlertsUntilQuiet: true }); + // The seeded value is the non-default one, so a second seed winning would show. host.initialize({ deferAlertsUntilQuiet: false }); + host.initialize({ deferAlertsUntilQuiet: true }); expect(target.applySettings).toHaveBeenCalledTimes(1); + expect(target.applySettings).toHaveBeenCalledWith( + expect.objectContaining({ deferAlertsUntilQuiet: false }), + ); - host.update({ deferAlertsUntilQuiet: false }); + host.update({ deferAlertsUntilQuiet: true }); expect(target.applySettings).toHaveBeenNthCalledWith( 2, - expect.objectContaining({ deferAlertsUntilQuiet: false }), + expect.objectContaining({ deferAlertsUntilQuiet: true }), ); }); }); diff --git a/lib/src/lib/alert-settings-model.ts b/lib/src/lib/alert-settings-model.ts index 310349efb..31fdd587a 100644 --- a/lib/src/lib/alert-settings-model.ts +++ b/lib/src/lib/alert-settings-model.ts @@ -32,7 +32,7 @@ export const MAX_DELAY_MS = 600_000; export const DEFAULT_ALERT_SETTINGS: AlertSettings = { // cfg.ts stays the single source of the shipped default. inactivityTimeoutMs: cfg.alert.userAttention, - deferAlertsUntilQuiet: false, + deferAlertsUntilQuiet: true, speakEnabled: false, speakDelayMs: 10_000, pushEnabled: false, diff --git a/lib/src/lib/alert-settings.test.ts b/lib/src/lib/alert-settings.test.ts index 72a6d4fad..930b95fd4 100644 --- a/lib/src/lib/alert-settings.test.ts +++ b/lib/src/lib/alert-settings.test.ts @@ -50,6 +50,10 @@ describe('normalizeAlertSettings', () => { expect(DEFAULT_ALERT_SETTINGS.inactivityTimeoutMs).toBe(cfg.alert.userAttention); }); + it('ships animation deferral on', () => { + expect(normalizeAlertSettings({}).deferAlertsUntilQuiet).toBe(true); + }); + it('fills in missing keys and drops unknown ones', () => { const result = normalizeAlertSettings({ speakEnabled: true, bogus: 'x' }); expect(result).toEqual({ ...DEFAULT_ALERT_SETTINGS, speakEnabled: true }); @@ -76,7 +80,8 @@ describe('normalizeAlertSettings', () => { it('rejects non-boolean flags', () => { expect(normalizeAlertSettings({ speakEnabled: 'yes' }).speakEnabled).toBe(false); expect(normalizeAlertSettings({ speakEnabled: 1 }).speakEnabled).toBe(false); - expect(normalizeAlertSettings({ deferAlertsUntilQuiet: 'yes' }).deferAlertsUntilQuiet).toBe(false); + // Falsy non-booleans must not switch the on-by-default flag off either. + expect(normalizeAlertSettings({ deferAlertsUntilQuiet: 0 }).deferAlertsUntilQuiet).toBe(true); }); }); diff --git a/lib/src/stories/SettingsDialog.stories.tsx b/lib/src/stories/SettingsDialog.stories.tsx index 136261d3f..26aff9089 100644 --- a/lib/src/stories/SettingsDialog.stories.tsx +++ b/lib/src/stories/SettingsDialog.stories.tsx @@ -52,15 +52,15 @@ export const WithRules: Story = { }, }; -/** The animation watcher gates terminal-notification alerts. */ -export const DeferralEnabled: Story = { +/** The escape hatch: deferral off, so terminal notifications ring during animation. */ +export const DeferralDisabled: Story = { parameters: { primedWatchedCommands: ['claude', 'codex'], - primedAlertSettings: { deferAlertsUntilQuiet: true }, + primedAlertSettings: { deferAlertsUntilQuiet: false }, }, play: async ({ canvasElement }) => { await dialog(canvasElement).findByRole('switch', { - name: 'Defer alerts until animation stops on', + name: 'Defer alerts until animation stops off', }); }, };