Skip to content

feat(tui): allow custom completion sound files#2512

Draft
cyq1017 wants to merge 2 commits into
Hmbown:mainfrom
cyq1017:codex/2484-notification-sound
Draft

feat(tui): allow custom completion sound files#2512
cyq1017 wants to merge 2 commits into
Hmbown:mainfrom
cyq1017:codex/2484-notification-sound

Conversation

@cyq1017
Copy link
Copy Markdown
Contributor

@cyq1017 cyq1017 commented Jun 1, 2026

Refs #2484

Problem

completion_sound supports the built-in modes, but Windows users cannot point CodeWhale at a custom notification sound file from config.

Change

  • add completion_sound = "file" and [notifications].sound_file
  • play the configured file asynchronously through the native Windows audio API
  • warn and no-op when the file mode is missing a path or running on an unsupported platform
  • document the new option in the example config and configuration guide

Verification

  • cargo test -p codewhale-tui custom_completion_sound --all-features --locked -- --nocapture
  • cargo test -p codewhale-tui tui::notifications::tests --all-features --locked -- --nocapture
  • cargo fmt --all -- --check
  • cargo check -p codewhale-tui --all-features --locked
  • cargo clippy -p codewhale-tui --all-features --locked -- -D warnings
  • git diff --check origin/main..HEAD

Greptile Summary

This PR adds completion_sound = "file" as a new option that lets Windows users specify a custom WAV file path ([notifications].sound_file) that is played asynchronously via PlaySoundW at turn completion.

  • Adds the CompletionSound::File variant, the sound_file: Option<PathBuf> config field, and Windows-only play_sound_file backed by PlaySoundW(SND_FILENAME | SND_ASYNC | SND_NODEFAULT).
  • Adds "warn once" AtomicBool flags to limit log noise when the feature is misconfigured (missing path, or used on a non-Windows platform), along with corresponding unit tests and documentation updates.

Confidence Score: 5/5

Safe to merge; the change is additive and self-contained, with no impact on existing sound modes or notification delivery paths.

The Windows PlaySoundW path is correctly wired with SND_FILENAME | SND_ASYNC | SND_NODEFAULT, the once-fire warning guards prevent log spam, and the new config field degrades gracefully on unsupported platforms. The only notable concern is that the warn-once flags are never reset after the user repairs and then re-breaks the configuration, which could silently suppress a useful diagnostic but does not affect correctness or stability.

crates/tui/src/tui/notifications.rs — specifically the interaction between set_completion_sound and the COMPLETION_SOUND_FILE_MISSING_WARNED flag across config reloads.

Important Files Changed

Filename Overview
crates/tui/src/tui/notifications.rs Adds File sound mode with OnceLock+Mutex for the path, SND_ASYNC PlaySoundW on Windows, and once-fire AtomicBool warning guards; the MISSING/UNSUPPORTED_WARNED flags are never reset when settings change, which can silently suppress subsequent misconfiguration warnings.
crates/tui/src/config.rs Adds CompletionSound::File variant and sound_file: Option to NotificationsConfig; clean serde deserialization with a new round-trip parse test.
crates/tui/Cargo.toml Adds Win32_Media_Audio feature to the windows crate dependency to unlock PlaySoundW.
crates/tui/src/tui/ui/tests.rs Adds sound_file: None to two existing NotificationsConfig struct literals to keep them compiling after the new field was added.
config.example.toml Documents the new file mode and sound_file option; also updates the Windows BEL fallback description to reflect the current code behaviour.
docs/CONFIGURATION.md Adds documentation for completion_sound = "file" and sound_file; corrects the Windows auto-fallback description from "off" to "bel via MessageBeep".

Fix All in Codex Fix All in Claude Code Fix All in Cursor

Reviews (2): Last reviewed commit: "fix(tui): clarify custom completion soun..." | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces support for playing a custom completion sound file on Windows when a turn completes in the TUI, adding a new completion_sound = "file" option and a sound_file configuration path. The reviewer feedback focuses on preventing potential log spam by ensuring warnings for unsupported platforms or missing sound files are only logged once per session. Additionally, the reviewer recommends explicitly documenting that only WAV files are supported by the underlying Windows PlaySoundW API in both the codebase doc comments and the configuration documentation.

Comment on lines +445 to +448
#[cfg(not(target_os = "windows"))]
fn play_sound_file(_path: &Path) {
tracing::warn!("completion_sound = \"file\" is currently supported on Windows only");
}
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.

medium

On non-Windows platforms, if completion_sound = "file" is configured, this warning will be logged on every single turn completion, leading to unnecessary log spam. Consider using a static AtomicBool to log this warning only once per session.

#[cfg(not(target_os = "windows"))]
fn play_sound_file(_path: &Path) {
    static WARNED: AtomicBool = AtomicBool::new(false);
    if !WARNED.swap(true, Ordering::Relaxed) {
        tracing::warn!("completion_sound = \"file\" is currently supported on Windows only");
    }
}

Comment on lines +450 to +456
fn file_sound() {
if let Some(path) = configured_sound_file() {
play_sound_file(&path);
} else {
tracing::warn!("completion_sound = \"file\" requires [notifications].sound_file");
}
}
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.

medium

If completion_sound = "file" is configured but sound_file is missing, this warning will be logged on every single turn completion. Consider using a static AtomicBool to log this warning only once per session to prevent log spam.

fn file_sound() {
    if let Some(path) = configured_sound_file() {
        play_sound_file(&path);
    } else {
        static WARNED: AtomicBool = AtomicBool::new(false);
        if !WARNED.swap(true, Ordering::Relaxed) {
            tracing::warn!("completion_sound = \"file\" requires [notifications].sound_file");
        }
    }
}

Comment thread crates/tui/src/config.rs Outdated
Comment on lines +771 to +773
/// Path to the sound file used when `completion_sound = "file"`.
#[serde(default)]
pub sound_file: Option<PathBuf>,
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.

medium

The Windows PlaySoundW API only supports Waveform Audio (WAV) files. It is highly recommended to specify this format restriction in the doc comments so users do not attempt to configure unsupported formats like MP3 or OGG.

    /// Path to the WAV sound file used when completion_sound = "file".
    #[serde(default)]
    pub sound_file: Option<PathBuf>,

Comment thread docs/CONFIGURATION.md Outdated
Comment on lines +702 to +703
- `[notifications].sound_file` (path, optional): path to a custom sound file
used when `completion_sound = "file"`.
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.

medium

The Windows PlaySoundW API only supports Waveform Audio (WAV) files. Explicitly documenting this format restriction helps prevent users from trying to use unsupported formats like MP3 or OGG.

Suggested change
- `[notifications].sound_file` (path, optional): path to a custom sound file
used when `completion_sound = "file"`.
- [notifications].sound_file (path, optional): path to a custom WAV sound file
used when completion_sound = "file".

Comment thread crates/tui/src/tui/notifications.rs
Comment on lines +1255 to +1274
#[test]
fn settings_installs_custom_completion_sound_file() {
let config: crate::config::Config = toml::from_str(
r#"
[notifications]
completion_sound = "file"
sound_file = "E:\\google\\downloads\\xm4114.wav"
"#,
)
.expect("custom completion sound config should parse");

let _ = settings(&config);

let (mode, file) = completion_sound_state_for_tests();
assert_eq!(mode, crate::config::CompletionSound::File);
assert_eq!(
file.as_deref(),
Some(std::path::Path::new("E:\\google\\downloads\\xm4114.wav"))
);
}
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.

P2 Test reads shared global state without serialization

settings_installs_custom_completion_sound_file writes COMPLETION_SOUND_MODE and COMPLETION_SOUND_FILE (both process-wide statics) via settings(), then immediately reads them back via completion_sound_state_for_tests(). Tests in crates/tui/src/tui/ui/tests.rs (e.g. notification_settings_tui_always_keeps_configured_method_no_threshold) also call crate::tui::notifications::settings() concurrently, which resets the same statics to Beep/None. Under the default parallel test runner this can produce a spurious assertion failure on the File or path check. The pre-existing env-sensitive tests already use env_lock() for similar reasons; adding the same mutex here (or annotating with #[serial]) would prevent this race.

Fix in Codex Fix in Claude Code Fix in Cursor

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.

1 participant