fix(notifications): honor disabled sound settings

[kyleseaman]

Problem / Motivation

Notification sounds can still play after the user disables them. Native OS notifications do not request silent delivery, sound settings are cached per window, failed storage writes can leave the UI showing unapplied state, and the global All = None setting does not override the built-in approval pulse.

Why it matters

A sound-off control must be authoritative. Unexpected notification audio is disruptive, and settings that differ between windows or between the UI and runtime behavior undermine trust in the notification controls.

What changed

• Mark both native notification paths silent: true, keeping WebAudio as the single sound source.
• Synchronize sound settings across dashboard windows through filtered storage events.
• Make persistence failure explicit and keep the settings UI aligned with persisted state.
• Make global All = None silence built-in category defaults unless a category has an explicit audible override.
• Resolve settings previews through the same preset logic used at runtime.
• Document the client notification-sound contract.

Coordination note: #7821 rewrites useNativeNotification for service-worker delivery. It does not address these sound-setting bugs; if it lands first, this PR must preserve silent: true in its shared NotificationOptions object during conflict resolution.

Tests

• npx vitest run src/test/useNotificationSound.test.ts src/test/notificationSilentOptions.test.ts src/test/NotificationsPanel.test.tsx — 53 passed
• npx tsc -b
• ESLint on all changed TypeScript files
• ./scripts/docs-lint.sh
• git diff origin/main...HEAD --check

[github-actions]

UX Review (Fable 5, fork) — 🟡 CONCERNS

UX-level review of 3839ddfef843daf460255d685fee8e7335b9e9ec via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I have everything I need. The change is sound (silent native toasts, cross-tab sync, all=none fix, truthful previews), but the new failure branch in NotificationsPanel.update deliberately keeps the UI on the persisted value when a save fails — with no user-visible signal at all.

UX-Verdict: CONCERNS

When a settings save fails, every control in the sound panel silently refuses to move — the panel looks frozen, with zero explanation.

Watch

• Dead controls on failed persist, no feedback. if (saveSoundSettings(next)) setSettings(next) means a quota-dropped write makes the toggle, slider, and every category select visibly snap back (or never move) with nothing telling the user why. Rare trigger (storage quota) × high impact (the whole panel reads as broken and the task fails) × persists on every retry while quota is full. Smallest fix: on false, render one inline error near the Sound card ("Couldn't save settings — browser storage is full") via a catalog key.
• Contradictory preview on that same failure. The category select's onChange still calls playPreset(v, …) after setCategoryPreset fails, so the user hears the sound they picked while the select reverts to the old value — audio says "applied," pixels say "refused." Gate the preview on the save result (same root cause; fix together).

[UX-REVIEWED] 3839ddf

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of 3839ddfef843daf460255d685fee8e7335b9e9ec via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Cross-window sync stops at the playback hook; the settings panel stays stale and can silently clobber another window's saved settings.

Watch

• "Synchronize sound settings across dashboard windows" is only half-backed: the storage listener lands in useNotificationSound, but NotificationsPanel still snapshots via useState(() => loadSoundSettings()) and never re-reads. A panel open in window B shows stale values after window A saves, and its next update writes the whole stale object back ({ ...settings, ...partial }), last-write-wins reverting window A's change — the per-window divergence the description says this fixes survives at the surface users actually touch. Either sync the panel too or scope the claim to playback.

Suggestions

• On a quota-dropped save the control now just doesn't move — correct state, zero feedback, reads as a dead toggle. Surface the false return to the user (toast/inline notice) in a follow-up.

[DESIGN-REVIEWED] 3839ddf

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of 3839ddfef843daf460255d685fee8e7335b9e9ec via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All verification is done. Every claimed fix maps to a real defect in the base code, safeSetItem already returned the boolean the base was discarding, both production new Notification( sites are covered, and the one undeclared edit (dropping prev.current >= 0) is provably behavior-neutral since prev starts at 0 and is only ever assigned a .filter().length. Final review:

First-Principles-Verdict: PASS

Every item traces to a named, reproducible defect in the base code, fixed in the single owning mechanism; nothing rides along except a verified dead-guard deletion.

What this change ships

Intent: make the user's sound-off settings actually stop notification audio — a FIX.

1. OS feed toast no longer adds a system chime over the WebAudio tone — justified, cause-level (removes the second sound source)
2. OS approval toast likewise silent — justified; both production new Notification( sites covered (grepped: 2, both in this diff)
3. Sound-setting changes now propagate to other open dashboard tabs — justified; matches the repo's inline storage-listener idiom (17 existing sites, no shared helper to reuse)
4. A quota-dropped settings save no longer shows an unapplied toggle — justified; propagates safeSetItem's existing boolean (1 consumer: NotificationsPanel.update), no new surface
5. All = None now silences approval's built-in pulse unless approval is explicitly overridden — justified, fixed in presetForKind, the single resolver
6. Settings-row previews now show what runtime plays — justified; deletes a second spelling of preset resolution (naive perCategory[cat] ?? fallback) in favor of the existing presetForKind
7. Spec section documenting the sound contract — justified; AGENTS.md mandates same-commit spec updates, appended to the existing app-notifications.md
8. prev.current >= 0 guard removed in useNativeNotification — undeclared, rides along; verified behavior-neutral (prev is useRef(0), only assigned a non-negative .length), a pure dead-code deletion

Electron's updater toasts (ipc-registrar.js:284, auto-update.js) construct OS notifications without silent, but they never coincide with a WebAudio tone and sit outside the feed-sound system — not siblings of this root cause.

[FIRST-PRINCIPLES-REVIEWED] 3839ddf

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 3839ddfef843daf460255d685fee8e7335b9e9ec via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 3839ddf

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 3839ddfef843daf460255d685fee8e7335b9e9ec via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 3839ddf