fix(notifications): default Windows Auto fallback to Off, not BEL (#583)#591
Merged
Conversation
On Windows, the audio stack maps BEL (`\x07`) to the `SystemAsterisk` / `MB_OK` chime — the same sound applications use for error popups. So with the previous `Method::Auto` fallback to `Bel`, every successful turn-completion notification ended up sounding identical to a software error. Reported by a community user who described it as "the popup-error sound from a CAD program I used to use" (#583). resolve_method() now returns `Off` instead of `Bel` on Windows for unknown TERM_PROGRAM values. Known OSC-9-capable terminals (`iTerm.app`, `Ghostty`, `WezTerm`) still resolve to `Osc9` on every platform, so users running WezTerm on Windows keep getting real notifications. macOS and Linux behaviour is unchanged. Windows users who actively want an audible cue can opt back in by setting `[notifications].method = "bel"` in `~/.deepseek/config.toml`. Also: - Documents `[notifications]` in `docs/CONFIGURATION.md` with an explicit Windows note (the schema was previously undocumented). - Updates the inline comment in `config.example.toml` so users reading the seed config see the platform-specific behaviour. - Splits the existing `auto_detect_picks_bel_for_unknown` test into a Unix variant (`#[cfg(not(target_os = "windows"))]`) and adds a new Windows-gated test that asserts the `Off` fallback, so CI's Windows runner exercises the platform-specific path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the TUI notification logic to handle Windows fallbacks more gracefully. When the notification method is set to 'auto', it now defaults to 'off' on Windows if an OSC-9 compatible terminal is not detected, preventing the 'BEL' character from triggering the system error chime. The PR includes documentation updates in 'config.example.toml' and 'docs/CONFIGURATION.md', as well as logic changes and new platform-specific tests in the TUI crate. I have no feedback to provide.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR narrows a Windows-specific TUI UX fix: it changes notification auto-detection so unknown Windows terminals default to off instead of emitting BEL, which currently sounds like the Windows error chime. It also updates config/docs so the new platform-specific behavior is documented.
Changes:
- Change
Method::Autoon Windows to fall back toOfffor unrecognizedTERM_PROGRAMvalues. - Add/update Windows-specific notification tests around auto-detection.
- Document the notifications config in
docs/CONFIGURATION.md,config.example.toml, and config rustdocs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
docs/CONFIGURATION.md |
Adds a new notifications configuration section and explains platform-specific auto behavior. |
crates/tui/src/tui/notifications.rs |
Changes Windows fallback logic for Method::Auto, updates rustdocs, and adjusts tests. |
crates/tui/src/config.rs |
Syncs config enum/field documentation with the new runtime behavior. |
config.example.toml |
Updates inline notification comments to describe the Windows fallback. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+405
to
+411
| The TUI can emit a desktop notification (OSC 9 escape or plain BEL) when a turn takes longer than a threshold, so you can tab away while a long task runs. Configuration lives under `[notifications]`: | ||
|
|
||
| ```toml | ||
| [notifications] | ||
| method = "auto" # auto | osc9 | bel | off | ||
| threshold_secs = 30 # only notify when the turn took >= this many seconds | ||
| include_summary = false # include elapsed time + cost in the notification body |
Comment on lines
284
to
285
| @@ -285,7 +285,12 @@ default_text_model = "deepseek-ai/deepseek-v4-pro" | |||
| # than `threshold_secs`. Useful when you tab away from the TUI and want an alert. | |||
Comment on lines
+311
to
+318
| /// #583: on Windows, an unknown TERM_PROGRAM resolves to `Off` | ||
| /// (not `Bel`) so the post-turn notification doesn't ring the | ||
| /// `SystemAsterisk` / `MB_OK` chime. Known OSC-9 terminals like | ||
| /// WezTerm still resolve to `Osc9` — see the iTerm test, which | ||
| /// also exercises the OSC-9 branch on Windows. | ||
| #[test] | ||
| #[cfg(target_os = "windows")] | ||
| fn auto_detect_picks_off_for_unknown_on_windows() { |
Comment on lines
45
to
+58
| /// Resolve `Auto` to a concrete method by inspecting `$TERM_PROGRAM`. | ||
| /// | ||
| /// Known OSC-9 capable programs: `iTerm.app`, `Ghostty`, `WezTerm`. | ||
| /// Everything else falls back to `Bel`. | ||
| /// Known OSC-9 capable programs: `iTerm.app`, `Ghostty`, `WezTerm` | ||
| /// (these resolve to `Osc9` on every platform, including Windows | ||
| /// when running inside WezTerm). | ||
| /// | ||
| /// Otherwise the fallback is platform-dependent: | ||
| /// - **macOS / Linux / other Unix:** `Bel` (a single `\x07` byte). | ||
| /// - **Windows:** `Off`. BEL is mapped by the Windows audio stack | ||
| /// to `SystemAsterisk` / `MB_OK`, the same chime used by | ||
| /// application error popups, so it sounds like an error | ||
| /// notification even though the turn completed successfully (#583). | ||
| /// Users can opt back in with `[notifications].method = "bel"` or | ||
| /// pick a known OSC-9 terminal. |
Comment on lines
+405
to
+406
| The TUI can emit a desktop notification (OSC 9 escape or plain BEL) when a turn takes longer than a threshold, so you can tab away while a long task runs. Configuration lives under `[notifications]`: | ||
|
|
… WezTerm-on-Windows test Post-merge review feedback on #583 surfaced four small accuracy gaps: 1. The narrative docs in `docs/CONFIGURATION.md` and the inline comment in `config.example.toml` said the notification fires "when a turn takes longer than a threshold" — but the call site in `tui/ui.rs:928` is gated on `TurnOutcomeStatus::Completed`. Failed and cancelled turns are silent on purpose. Spell that out so users don't expect alerts on long failures. 2. The `notify_done` rustdoc still summarised `Auto` as "Osc9 for known terminals, Bel otherwise" — internally inconsistent with the new Windows-aware fallback documented one screen earlier on the `Method::Auto` enum and on `resolve_method`. Update the public rustdoc to point at the canonical resolution table on `resolve_method` and call out the `Off`-on-Windows branch. 3. The `## Key Reference` list in `docs/CONFIGURATION.md` had no entries for `[notifications].method`, `[notifications].threshold_secs`, or `[notifications].include_summary`. Other features with a dedicated subsection (e.g. `[memory].enabled`) are listed there too, so readers scanning the canonical key list could not discover the notification knobs. Added the three keys with cross-references to the Notifications subsection. 4. The Windows-only test only covered the unknown-`TERM_PROGRAM` → `Off` fallback. The positive path (known OSC-9 terminal still resolves to `Osc9`) was only tested via `iTerm.app`, which is a macOS-only program — Windows CI would still pass if the `WezTerm` arm of the match disappeared. Added `auto_detect_picks_osc9_for_wezterm_on_windows` so the WezTerm-on-Windows compatibility guarantee is exercised on the Windows runner. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
MMMarcinho
pushed a commit
to MMMarcinho/DeepSeek-TUI
that referenced
this pull request
May 6, 2026
…-off fix(notifications): default Windows Auto fallback to Off, not BEL (Hmbown#583)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Method::Autonow resolves toOffinstead ofBelfor unknown terminals. BEL maps to the WindowsSystemAsterisk/MB_OKchime — the same sound used by application error popups, so a successful turn-completion notification ended up sounding identical to a software error.Bel.iTerm.app,Ghostty,WezTerm) still resolve toOsc9on every platform, so users running WezTerm on Windows keep getting real notifications.[notifications].method = "bel"in~/.deepseek/config.toml.Reported by
A community user (Simplified Chinese, via Telegram) — "every time ds tui completes a task, the notification sounds identical to the error-popup sound from a CAD program I used to use" (#583).
Changes
crates/tui/src/tui/notifications.rsresolve_method()returnsMethod::Offon Windows for unknown TERM_PROGRAM. Doc comment onMethod::Autoupdated.crates/tui/src/config.rsNotificationMethod::AutoandNotificationsConfig.methodupdated to match runtime behaviour.docs/CONFIGURATION.md### Notificationssubsection. The schema was previously undocumented — closes the discoverability half of the issue.config.example.tomlmethod = "auto"now describes the platform-specific fallback.Tests
auto_detect_picks_bel_for_unknowntest renamed toauto_detect_picks_bel_for_unknown_on_unixand gated behind#[cfg(not(target_os = "windows"))]. Same TERM_PROGRAM value, same assertion, just no longer false-failing on the Windows runner.auto_detect_picks_off_for_unknown_on_windowstest gated behind#[cfg(target_os = "windows")]exercises the new path on the CI Windows runner.Notes for the reviewer
/configruntime toggle deferred. The issue's acceptance criteria include "User can toggle the setting from/configwithout editing TOML." Wiringnotifications.methodintoset_config_value+show_single_settingplus a persist path (similar topersist_status_items) and a live-App propagation hook is materially bigger than this default-behaviour fix. I opted to ship the user-pain fix tight and clean here, and split the/configexposure into a follow-up PR. Happy to land it next if you'd rather have both in v0.8.11.Offand not a newVisualvariant. The handoff suggested a possible stretch goal of aMethod::Visual(terminal-title flash) as the Windows default. Adding a new variant touches theNotificationMethodparse path, theMethodenum,build_escape, the example config, and the test surface — meaningful scope. For v0.8.11 the priority is "stop ringing the error chime";Offsolves that with a five-line change. If users start asking for a non-audible visible cue we can addVisualin a separate PR.Test plan
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features --locked -- -D warningscargo test --workspace --all-features --locked(all crates pass)cargo test -p deepseek-tui --bin deepseek-tui --locked notifications— 12 tests pass on macOS; the_on_windowstest is correctly gated out.deepseek-tui-core --test snapshot,deepseek-protocol --test parity_protocol,deepseek-state --test parity_stateall pass.Cargo.lockclean.[notifications].method = "bel"opts back in.Closes #583.
🤖 Generated with Claude Code