feat: add per-category notification alert sounds with app-owned playback - #356
Conversation
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord automated source review
Submitted through Wes’s account (wesbillman), as a non-blocking COMMENT review.
Reviewed head e4dd7521bb9de928f108f4cb58229255420265f0 against base bebb54ec5bd9547b9905cd9d669a48936ec40ac2 (merge base 96f074343d580c0e27c0cb11da1153b5ff867c20). Two actionable P2 findings, detailed inline: a known native failure does not cancel pending audio, and Settings previews outlive their playback controls/policy.
Scope: full 41-file diff, preference migration, notification producers/policy, browser/native adapters, Settings controls, changed tests, and relevant repository/product/design guidance. All 24 new assets match the pinned reference-client assets byte-for-byte. Source came from pinned Git blobs, not dirty working files.
Validation limits: source-only; I ran no tests, builds, installs, PR code, or app instances. One hosted CI snapshot showed JavaScript, Rust/tool integration, browser measurements, all three Chromium shards, WebKit shard 2/3, security checks and DCO passing; WebKit shards 1/3 and 3/3 were still running, and Windows validation was skipped. No CI polling. Actual audio, packaged OS banner/click behavior, and human acceptance remain unverified. This is neither an approval nor merge authorization.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: fix the two audio-lifecycle defects below and add focused regression coverage. The app-owned sound policy and acceptance-not-display boundary are preserved; no new notification receipt system or preference migration is requested.
Reviewed head e4dd7521bb9de928f108f4cb58229255420265f0, target base bebb54ec5bd9547b9905cd9d669a48936ec40ac2, diff merge-base 96f074343d580c0e27c0cb11da1153b5ff867c20. Existing JavaScript CI passed 405 files / 4,772 tests. Focused service and mounted-component reproductions exposed cases outside that coverage. Full diff/PR-text privacy and artifact review found no issue.
Acceptance caveat: renderer audio is independent of OS notification sound/Focus controls. Packaged macOS/Windows/Linux acceptance, including Linux MP3 decoding, is not established by these tests. This is a consequence of the stated app-owned policy, not an additional blocker here.
00d1a9d to
039679d
Compare
Port notification sound parity from the reference client: bundle the twelve reference sound assets, add per-category sound selection (direct, mention, thread) persisted in notification preferences with backward-compatible parsing, play the selected sound app-side after a banner is accepted, always submit platform banners silent, and surface the Sound switch plus per-event sound rows with waveform and preview in Settings on all platforms. Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
…banners Address review findings on the sound-parity change: - Re-fence sound playback after platform.show resolves: play only when the captured account generation is live, the alert is still allowed and eligible, and Sound was on both at submission and acceptance. Add a deferred-submission regression test covering account switch, mid-flight mute, off-to-on resurrection, category disable and lost eligibility. - Build the Windows toast with sound(None) so the OS toast stays silent (tauri-winrt-notification emits <audio silent="true"/>). - Send the freedesktop suppress-sound hint on Linux Notify and assert it in the D-Bus integration test. - Let the settings sound rows wrap and shrink so narrow/high-text layouts stay within the pane, and assert the sound controls in the two-engine settings acceptance test. - Match the reference rec. copy exactly (no added parentheses). Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
An audio decision now registers in a sounding set when its banner is submitted and stays there until the platform resolves it. Revalidation cancels the decision stickily on any interval of Sound off, alerts or category disabled, or revoked authorization/eligibility; restoring the setting before the submission resolves no longer resurrects the sound. Cancellation never affects the banner, decisions retire on success and failure, and the generation/current-policy fences remain. Regression test now covers on-off-on for Sound, master alerts and category, plus revoke-revalidate-restore eligibility. Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
…bels The design-system pre-push guard rejects raw font-weight values; --type-weight-medium resolves to the same 500. Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
The page subtitle, Desktop alerts description (including the enabled/disabled variants), and Notify while viewing description now use the exact reference copy from block/buzz desktop/src/features/settings/ui/NotificationSettingsCard.tsx. Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
DM events p-tag their recipient, so the mention-first category order routed every direct message to the mention category: wrong sound, wrong banner copy, and the Direct messages toggle never gated DMs. Direct now wins inside DM channels, matching the reference slotting. The explicit-mention mute bypass is preserved by exposing the p-tag evidence as MessageAttention.mentioned and keying the bypass on it instead of on the category. Also: rename the desktop settings copy test to match its assertions, add a regression render for the disabled Desktop alerts description, and drop the stale documentation claim about permission wording removed from the settings subtitle. Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
cf4c294 to
28274ab
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord automated source review
Submitted through Wes’s account (wesbillman) as a non-blocking COMMENT review.
Reviewed head 28274abf91759e8e24378ecfa1a894c2553bbc71 against base a2bfc8120233453b376c2382ad8e990b987bdbda. No new actionable findings in this audio-lifecycle follow-up. Both previously reported defects are addressed in the current source:
src/features/notifications/service.ts:425–435cancels the outstanding audio decision when native failure arrives before submission resolves, independently of the account fence for error reporting, and retires it infinally.src/app/NotificationSettings.tsx:128–190owns one preview separately from notification playback, fences stale callbacks by audio identity, resets on playback rejection/error, and stops on replacement, disablement, inactive section, selection change, and unmount. The mounted Settings caller supplies the section’s active state.
I inspected the regression-test source and current integration, including preferences, message classification, silent platform submissions, and Settings success/cancel/error/retry paths. Independent source review of the delivery/policy lane found no additional defect. Inspection of the public PR description, changed material, and commit messages found no additional disclosure issue; the description contains no attached images.
Validation limits: source-only; no tests, builds, installs, PR code, or app instances were run. The single hosted-check snapshot showed JavaScript, Rust/tool integration, browser measurements, five browser shards, security checks, and DCO passing; Chromium shard 1/3 was still running and Windows validation was skipped. No CI polling. Actual audio/autoplay, Linux MP3 decoding, packaged macOS/Windows/Linux banner/sound/click behavior, browser geometry/focus, and human acceptance remain unverified. This is not an approval or merge authorization.
…sh-followup * origin/main: Add message-level read and unread controls (#352) feat: add per-category notification alert sounds with app-owned playback (#356) test(relay): stabilize per-channel replay boundary coverage (#378) fix(design-system): keep button labels single-line and corners capsule-shaped (#357) Signed-off-by: Codex <noreply@openai.com>
Behavior
public/sounds/, matching the reference Buzz desktop client sound set, defaults, and copy.flutterdefault. Previously saved preferences load with the defaults and stay valid; plugin-contributed categories use the default sound.silent: true, Windows toasts are built with silent audio, Linux Notify sends the standardsuppress-soundhint, and macOS leaves the sound name unset.Validation