fix(notifications): only alert for background session completions - #726
Open
beruro wants to merge 2 commits into
Open
fix(notifications): only alert for background session completions#726beruro wants to merge 2 commits into
beruro wants to merge 2 commits into
Conversation
Pre-commit hook ran. Total eslint: 0, total circular: 0
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.
Problem
A foreground Session completion followed the same task-completion delivery path as background work. The notification policy used
background: falseto suppress only the native system alert, while still playing the completion sound. The CLI monitor also relied on the persisted background flag without checking whether another Session was currently active.Solution
Suppress
taskCompletiondelivery when its context explicitly shows that the Session already has user attention. CLI completion events now combine the persisted background flag with the active Session ID before applying the existing document visibility and focus check. Background completions continue to alert, while approvals, errors, Team Inbox notifications, quiet hours, and per-Session muting keep their existing behavior.The change adds a typed
foreground-sessionsuppression reason and regression coverage at both the policy and sound-delivery boundaries. It does not change settings, persistence, IPC, or wire formats.Potential risks
During a transient state where no Session is active, a completion is conservatively treated as needing attention and may alert. Completion calls without a Session attention context retain their previous behavior; the Session terminal delivery boundary supplies that context. Native desktop audio was not manually exercised, but the sound invocation boundary is covered by tests. Rollback is a normal revert of this commit; no data migration or recovery is required.
Verification
npx eslint src/api/services/notificationPolicy.ts src/types/ui/notification.ts src/hooks/cliSession/useBackgroundSessionMonitor.ts src/api/services/notificationPolicy.test.ts src/api/services/notification.test.ts— passednpx vitest run src/api/services/notificationPolicy.test.ts src/api/services/notification.test.ts src/hooks/session/sessionTerminalNotifications.test.ts— 3 files, 28 tests passednpm run typecheck— passedgit diff --check origin/develop...HEAD— passedorigin/developis an ancestor; branch is 0 behind / 1 commit aheadUI evidence
No screenshot is included because this changes notification/audio behavior without changing rendered UI or layout.