Repository navigation
Conversation
… banners A blocking agent decision (PermissionRequest / ExitPlanMode / AskUserQuestion) fires two independent hooks: the agent's Notification hook posts a banner through TerminalNotificationStore while the feed decision bridge posts its own actionable banner directly through UNUserNotificationCenter (FeedCoordinator.postNotificationIfStillAwaiting). Both banners request a sound, so a single prompt dings twice when the app is in the background (manaflow-ai#2322). The banners carry different content and inline actions, so they cannot be deduplicated by identity. Instead, TerminalNotificationSoundGate grants the sound to the first banner per surface within a 2s window and silences follow-ups: banner visibility is unchanged, denied attempts do not extend the window, and unresolvable targets are never silenced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@danmirman is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds TerminalNotificationDeliveryGate to coordinate per-workspace/surface blocking-decision claims and per-key sound deduplication; integrates it into TerminalNotificationStore scheduling and FeedCoordinator notification delivery paths; and adds tests covering sound suppression, blocking claims, and withdrawal selection. ChangesTerminal notification delivery gate and integrations
Sequence Diagram(s)sequenceDiagram
participant FeedCoordinator
participant TerminalNotificationDeliveryGate
participant TerminalNotificationStore
FeedCoordinator->>TerminalNotificationDeliveryGate: beginBlockingDecision(forKey:)
FeedCoordinator->>FeedCoordinator: compute deliveryGateKey, call postNotificationIfStillAwaiting(deliveryGateKey)
FeedCoordinator->>TerminalNotificationDeliveryGate: endBlockingDecision(forKey:) when waiter removed
TerminalNotificationStore->>TerminalNotificationDeliveryGate: claimSound(forKey:) during deliver path
TerminalNotificationDeliveryGate-->>TerminalNotificationStore: grant/deny (Bool)
TerminalNotificationStore->>TerminalNotificationStore: clear effects.sound if denied
TerminalNotificationStore->>TerminalNotificationStore: withdrawRecentDeliveredDesktopNotifications(forGateKey:since:) when delivering new banner
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 1 warning)
✅ Passed checks (16 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR introduces
Confidence Score: 4/5The sound deduplication works in the common path, but the async authorization callback in scheduleUserNotification is not re-guarded, leaving a narrow race where both banners can appear simultaneously. The ensureAuthorization async path does not re-check the gate inside its callback, so if authorization resolves on a later main-actor turn after the feed path has already withdrawn and posted, the store banner is added with no withdrawal — both banners visible, one sound. Sources/TerminalNotificationStore.swift — the ensureAuthorization callback in scheduleUserNotification and the claimSound compaction path. Important Files Changed
Sequence DiagramsequenceDiagram
participant BG as Background Thread
participant Gate as DeliveryGate
participant Store as TerminalNotificationStore
participant Feed as FeedCoordinator Task
participant UN as UNUserNotificationCenter
Note over BG: Blocking decision arrives
BG->>Gate: beginBlockingDecision(forKey:)
BG->>Feed: spawn Task at MainActor
Note over Store: Agent notification hook fires
Store->>Gate: hasActiveBlockingDecision? true
Note over Store: Store returns early - sidebar kept, no banner
Note over Feed: Task runs on main actor
Feed->>Gate: claimSound(forKey:) returns true
Feed->>Store: withdrawRecentDeliveredDesktopNotifications
Store->>UN: removePending + removeDelivered for store banner IDs
Feed->>UN: add feed banner with sound and action buttons
BG->>BG: semaphore.wait
Note over BG: User responds or times out
BG->>Gate: endBlockingDecision(forKey:)
Note over Store: Race - store hook fires before beginBlockingDecision
Store->>Gate: hasActiveBlockingDecision? false
Store->>Gate: claimSound(forKey:) true - sound claimed
Store->>UN: center.add store banner as pending
Note over Feed: withdrawal removes pending store banner
Feed->>UN: add feed banner without sound
Reviews (2): Last reviewed commit: "Make the feed banner the canonical alert..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| final class TerminalNotificationSoundGate: @unchecked Sendable { | ||
| static let shared = TerminalNotificationSoundGate() | ||
|
|
||
| let suppressionWindow: TimeInterval | ||
|
|
||
| private let lock = NSLock() | ||
| private var lastGrantDateByKey: [String: Date] = [:] | ||
| private let maxTrackedKeys = 128 | ||
|
|
||
| init(suppressionWindow: TimeInterval = 2.0) { | ||
| self.suppressionWindow = suppressionWindow | ||
| } | ||
|
|
||
| /// The dedupe key for a notification target. Keyed by surface when known | ||
| /// so prompts on different panes of one workspace stay independent. | ||
| static func key(workspaceId: UUID, surfaceId: UUID?) -> String { | ||
| (surfaceId ?? workspaceId).uuidString | ||
| } | ||
|
|
||
| /// Returns whether a sound may play for the given key, recording a grant | ||
| /// when it does. `nil` keys are never deduplicated. | ||
| func shouldPlaySound(forKey key: String?, now: Date = Date()) -> Bool { | ||
| guard let key, !key.isEmpty else { return true } | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| if let lastGrant = lastGrantDateByKey[key], | ||
| abs(now.timeIntervalSince(lastGrant)) < suppressionWindow { | ||
| return false | ||
| } | ||
| if lastGrantDateByKey.count >= maxTrackedKeys { | ||
| lastGrantDateByKey = lastGrantDateByKey.filter { | ||
| abs(now.timeIntervalSince($0.value)) < suppressionWindow | ||
| } | ||
| } | ||
| lastGrantDateByKey[key] = now | ||
| return true | ||
| } |
There was a problem hiding this comment.
@MainActor call sites
TerminalNotificationSoundGate uses NSLock — a blocking primitive — to guard lastGrantDateByKey, but both call sites that invoke shouldPlaySound are already on @MainActor: TerminalNotificationStore.effectsAfterSoundGate is a method of an @MainActor final class, and FeedCoordinator.deliverFeedNotificationIfStillAwaiting is explicitly @MainActor. The main actor serialises all access, so the lock provides no safety benefit while adding a blocking acquisition on the main thread. If a background caller is ever introduced, it would be the @MainActor path that blocks waiting for the background thread to release — the opposite of the intended protection. The type should be an actor (or isolated to @MainActor) so Swift's concurrency checker owns the invariant and the lock can be removed entirely.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
There was a problem hiding this comment.
This was reviewed against the first commit — 48bd85f renamed the type to TerminalNotificationDeliveryGate and expanded it. On @MainActor: beginBlockingDecision/endBlockingDecision are called from FeedCoordinator.ingestBlocking, which runs on the CLI socket thread (before the DispatchQueue.main.sync hop and after the semaphore wait), and the claim must be registered synchronously before postNotificationIfStillAwaiting spawns its task. Making the gate @MainActor would either force an async hop (losing that ordering guarantee) or push main.sync deeper into the socket path. The NSLock-guarded micro-critical-section mirrors the existing patterns in these exact files: FeedCoordinator.waiterLock and NotificationSoundSettings.activePlaybackSoundsLock.
There was a problem hiding this comment.
Fair point — my comment was against the first commit and missed the beginBlockingDecision/endBlockingDecision call sites running on the socket thread. With a synchronous ordering requirement before the postNotificationIfStillAwaiting task spawn, @MainActor isn't viable without breaking that guarantee, and the NSLock micro-critical-section pattern is consistent with waiterLock and activePlaybackSoundsLock in the same files. The lock is appropriate here.
One small suggestion: a brief inline comment on the lock explaining why it's not @MainActor (the socket-thread call sites) would help the next reviewer avoid the same confusion:
| final class TerminalNotificationSoundGate: @unchecked Sendable { | |
| static let shared = TerminalNotificationSoundGate() | |
| let suppressionWindow: TimeInterval | |
| private let lock = NSLock() | |
| private var lastGrantDateByKey: [String: Date] = [:] | |
| private let maxTrackedKeys = 128 | |
| init(suppressionWindow: TimeInterval = 2.0) { | |
| self.suppressionWindow = suppressionWindow | |
| } | |
| /// The dedupe key for a notification target. Keyed by surface when known | |
| /// so prompts on different panes of one workspace stay independent. | |
| static func key(workspaceId: UUID, surfaceId: UUID?) -> String { | |
| (surfaceId ?? workspaceId).uuidString | |
| } | |
| /// Returns whether a sound may play for the given key, recording a grant | |
| /// when it does. `nil` keys are never deduplicated. | |
| func shouldPlaySound(forKey key: String?, now: Date = Date()) -> Bool { | |
| guard let key, !key.isEmpty else { return true } | |
| lock.lock() | |
| defer { lock.unlock() } | |
| if let lastGrant = lastGrantDateByKey[key], | |
| abs(now.timeIntervalSince(lastGrant)) < suppressionWindow { | |
| return false | |
| } | |
| if lastGrantDateByKey.count >= maxTrackedKeys { | |
| lastGrantDateByKey = lastGrantDateByKey.filter { | |
| abs(now.timeIntervalSince($0.value)) < suppressionWindow | |
| } | |
| } | |
| lastGrantDateByKey[key] = now | |
| return true | |
| } | |
| // NSLock rather than @MainActor: beginBlockingDecision/endBlockingDecision | |
| // are called from the CLI socket thread before the main-thread hop, so | |
| // @MainActor isolation would require an async hop that breaks the | |
| // synchronous ordering guarantee. Pattern mirrors FeedCoordinator.waiterLock. | |
| private let lock = NSLock() |
Otherwise the design is sound — withdrawing this concern.
| final class TerminalNotificationSoundGate: @unchecked Sendable { | ||
| static let shared = TerminalNotificationSoundGate() | ||
|
|
||
| let suppressionWindow: TimeInterval | ||
|
|
||
| private let lock = NSLock() | ||
| private var lastGrantDateByKey: [String: Date] = [:] | ||
| private let maxTrackedKeys = 128 | ||
|
|
||
| init(suppressionWindow: TimeInterval = 2.0) { | ||
| self.suppressionWindow = suppressionWindow | ||
| } | ||
|
|
||
| /// The dedupe key for a notification target. Keyed by surface when known | ||
| /// so prompts on different panes of one workspace stay independent. | ||
| static func key(workspaceId: UUID, surfaceId: UUID?) -> String { | ||
| (surfaceId ?? workspaceId).uuidString | ||
| } | ||
|
|
||
| /// Returns whether a sound may play for the given key, recording a grant | ||
| /// when it does. `nil` keys are never deduplicated. | ||
| func shouldPlaySound(forKey key: String?, now: Date = Date()) -> Bool { | ||
| guard let key, !key.isEmpty else { return true } | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| if let lastGrant = lastGrantDateByKey[key], | ||
| abs(now.timeIntervalSince(lastGrant)) < suppressionWindow { | ||
| return false | ||
| } | ||
| if lastGrantDateByKey.count >= maxTrackedKeys { | ||
| lastGrantDateByKey = lastGrantDateByKey.filter { | ||
| abs(now.timeIntervalSince($0.value)) < suppressionWindow | ||
| } | ||
| } | ||
| lastGrantDateByKey[key] = now | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
@unchecked Sendable papers over the actor isolation issue
TerminalNotificationSoundGate is a shared mutable reference type marked @unchecked Sendable to satisfy the concurrency checker without actually providing isolation. The cmux convention (and Swift 6 intent) for a shared mutable type whose callers are all @MainActor is to make the type itself @MainActor-isolated or an actor, letting the compiler verify the invariant. @unchecked Sendable turns a compile-time guarantee into a run-time assumption that the manual lock is sufficient — an assumption that won't hold once the lock is removed.
Rule Used: Flag new or materially worsened Swift 6 actor isol... (source)
There was a problem hiding this comment.
Same rationale as the sibling thread: the type is accessed from both the main actor (store + feed delivery) and the CLI socket thread (ingestBlocking claim lifecycle), so it is genuinely cross-isolation shared state — the same shape as FeedCoordinator itself and PendingWaiter, both @unchecked Sendable with lock/queue discipline in this file. All mutable state is touched only inside the single NSLock.
There was a problem hiding this comment.
Fair point — if ingestBlocking runs off the main actor on the CLI socket thread, @MainActor isolation would be wrong and actor would require await at every call site in latency-sensitive paths. Matching the FeedCoordinator/PendingWaiter pattern is the right call here.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
| final class TerminalNotificationSoundGate: @unchecked Sendable { | ||
| static let shared = TerminalNotificationSoundGate() | ||
|
|
||
| let suppressionWindow: TimeInterval | ||
|
|
||
| private let lock = NSLock() | ||
| private var lastGrantDateByKey: [String: Date] = [:] | ||
| private let maxTrackedKeys = 128 | ||
|
|
||
| init(suppressionWindow: TimeInterval = 2.0) { | ||
| self.suppressionWindow = suppressionWindow | ||
| } | ||
|
|
||
| /// The dedupe key for a notification target. Keyed by surface when known | ||
| /// so prompts on different panes of one workspace stay independent. | ||
| static func key(workspaceId: UUID, surfaceId: UUID?) -> String { | ||
| (surfaceId ?? workspaceId).uuidString | ||
| } | ||
|
|
||
| /// Returns whether a sound may play for the given key, recording a grant | ||
| /// when it does. `nil` keys are never deduplicated. | ||
| func shouldPlaySound(forKey key: String?, now: Date = Date()) -> Bool { | ||
| guard let key, !key.isEmpty else { return true } | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| if let lastGrant = lastGrantDateByKey[key], | ||
| abs(now.timeIntervalSince(lastGrant)) < suppressionWindow { | ||
| return false | ||
| } | ||
| if lastGrantDateByKey.count >= maxTrackedKeys { | ||
| lastGrantDateByKey = lastGrantDateByKey.filter { | ||
| abs(now.timeIntervalSince($0.value)) < suppressionWindow | ||
| } | ||
| } | ||
| lastGrantDateByKey[key] = now | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
The fix introduces TerminalNotificationSoundGate.shared — a global mutable singleton — to wire a shared side-channel between TerminalNotificationStore and FeedCoordinator. Both paths already post to UNUserNotificationCenter independently; the gate adds a third coordinating artefact rather than naming which path owns notification delivery for a blocking prompt and routing through it exclusively. A follow-on caller that posts a third banner (or any path that bypasses the gate) will double-ding again. The "Known behavior change" note in the PR also acknowledges that two legitimate distinct notifications on the same surface within 2 s will have one sound silenced — a user-visible regression on surfaces with high event cadence.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
There was a problem hiding this comment.
Agreed that the long-term fix is consolidating to a single delivery funnel — that's what #2322 calls for ("consolidating to a single notification path"). This PR is deliberately scoped to a deterministic, minimal contract that can land safely now: feed banner canonical for blocking decisions, store stands down while claimed, raced banners withdrawn, single sound guaranteed. Happy to follow up with the funnel consolidation if maintainers want to go that way.
Replaces the race-dependent sound-only dedupe with a deterministic contract: while a blocking decision claims a surface, the store keeps its sidebar record but stands down from desktop banners, sounds, and notification commands; if the agent-hook banner raced ahead of the claim, the feed withdraws it from Notification Center when posting its own actionable banner. Claims are refcounted for overlapping decisions and time-boxed so a decision answered in the terminal (which stays pending app-side until the hook timeout) cannot swallow legitimate later alerts like the completion banner. The shared per-surface sound window remains as the last-resort guarantee of a single ding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
danmirman
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Feed/FeedCoordinator.swift`:
- Around line 159-170: The code is claiming the TerminalNotificationDeliveryGate
unconditionally
(TerminalNotificationDeliveryGate.shared.beginBlockingDecision(forKey:
deliveryGateKey)) before we know feed banner will actually replace the store
banner; only claim the gate when the same predicate used by
postNotificationIfStillAwaiting allows posting (i.e. mirror the eligibility
checks around resolvedAttentionTarget/NSApp.isActive and the
TerminalNotificationStore key-window terminal check), and ensure you call
beginBlockingDecision only when that predicate is true and release it when the
predicate fails or you bail out; update the logic around resolvedAttentionTarget
and the NSApp.isActive / TerminalNotificationStore key-window check so they are
identical, and add a regression test covering the case “cmux is active but the
key window is not a terminal window” to verify no gate is claimed and both
banner/sound/notification behavior matches expected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b48536c2-fead-48dd-8bea-3e6bdcb3fcfe
📒 Files selected for processing (3)
Sources/Feed/FeedCoordinator.swiftSources/TerminalNotificationStore.swiftcmuxTests/NotificationSoundSettingsTests.swift
| // While this decision is pending, this banner is the canonical | ||
| // desktop alert for the surface: the same prompt also fires the | ||
| // agent's notification hook, which would otherwise post a second | ||
| // banner (and second sound) through TerminalNotificationStore | ||
| // (manaflow-ai/cmux#2322). Claim the surface for the decision's | ||
| // lifetime so the store keeps its sidebar record but stands down | ||
| // from desktop alerts. | ||
| let deliveryGateKey = resolvedAttentionTarget.map { | ||
| TerminalNotificationDeliveryGate.key(workspaceId: $0.workspaceId, surfaceId: $0.surfaceId) | ||
| } | ||
| TerminalNotificationDeliveryGate.shared.beginBlockingDecision(forKey: deliveryGateKey) | ||
|
|
There was a problem hiding this comment.
Only claim the delivery gate when the feed banner can actually replace the store banner.
Line 169 starts suppressing the store path before we know whether postNotificationIfStillAwaiting will post any desktop alert. Later, Lines 744-747 bail out whenever NSApp.isActive is true, but Sources/TerminalNotificationStore.swift explicitly treats non-terminal key windows as not focused so the store path remains the only banner/sound path in that state. With the new guard on Lines 2108-2110 there, a blocking prompt opened while Settings/About/debug UI is frontmost now produces no banner, no sound, and no notification command at all.
Please align the claim lifecycle with the same predicate that makes the feed banner eligible to post, and add a regression for the “cmux is active but the key window is not a terminal window” case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/Feed/FeedCoordinator.swift` around lines 159 - 170, The code is
claiming the TerminalNotificationDeliveryGate unconditionally
(TerminalNotificationDeliveryGate.shared.beginBlockingDecision(forKey:
deliveryGateKey)) before we know feed banner will actually replace the store
banner; only claim the gate when the same predicate used by
postNotificationIfStillAwaiting allows posting (i.e. mirror the eligibility
checks around resolvedAttentionTarget/NSApp.isActive and the
TerminalNotificationStore key-window terminal check), and ensure you call
beginBlockingDecision only when that predicate is true and release it when the
predicate fails or you bail out; update the logic around resolvedAttentionTarget
and the NSApp.isActive / TerminalNotificationStore key-window check so they are
identical, and add a regression test covering the case “cmux is active but the
key window is not a terminal window” to verify no gate is claimed and both
banner/sound/notification behavior matches expected.
teamleaderleo
added
area: notifications
This branch has not been deployed
2 participants
Problem
When a Claude Code session hits a blocking decision (tool permission request,
ExitPlanMode,AskUserQuestion) while cmux is in the background, the user gets two desktop banners and two sounds for one prompt. This is one concrete, reproducible slice of the notification flakiness tracked in #2322.Root cause
A single blocking prompt fires two independent hooks, and each posts its own desktop banner with its own sound:
Notificationhook (cmux hooks claude notification) classifies the permission message as.needsInput(classifyAgentHookNotification,CLI/cmux.swift) and posts throughTerminalNotificationStore.scheduleUserNotification.PermissionRequesthook (cmux hooks feed --source claude) routes throughFeedCoordinator.ingestBlocking → postNotificationIfStillAwaiting → deliverFeedNotificationIfStillAwaiting, which posts its own actionable banner (feed.<requestId>) directly toUNUserNotificationCenter.There is no shared state between the paths: the store's
cooldownKeyis per-call-site opt-in and the CLI'snotificationFingerprintdedup is per-hook-handler only. The feed path also runs regardless of whether the feed sidebar beta is enabled, so every user with Claude hooks gets the duplicate on blocking prompts.Fix — one event, one banner, one ding (deterministically)
TerminalNotificationDeliveryGatemakes the feed's actionable banner the canonical desktop alert for blocking decisions — not "whichever banner wins the race":FeedCoordinator.ingestBlockingclaims the surface for the decision's lifetime (beginBlockingDecision/endBlockingDecision, keyed bysurfaceId ?? workspaceId). While claimed, the store keeps recording sidebar notifications (history/unread untouched) but stands down from desktop banners, sounds, and notification commands for that surface.withdrawRecentDeliveredDesktopNotifications, 5s lookback, sidebar record untouched) — exactly one banner remains visible.claimSoundguarantees at most one sound per surface within 2s regardless of arrival order, covering the withdraw case where the raced banner already dinged.Net behavior: blocking prompt while backgrounded → one actionable banner + one ding. Blocking prompt while focused → in-app attention only (existing #5313 path). Completions and idle "waiting for input" notifications are unchanged: one banner, one ding, as before.
Degenerate case: if the decision's target can't be resolved from the hook-session store (
nilkey), nothing is claimed and behavior falls back to today's.Tests
TerminalNotificationDeliveryGateTests(Swift Testing, appended to the already-wiredNotificationSoundSettingsTests.swiftto avoidproject.pbxprojedits per the CLAUDE.md test-wiring pitfall): sound-window grant/silence/expiry, denied attempts not extending the window, per-key independence, nil-key passthrough, claim begin/end/refcount/time-box/extension, unbalanced-end no-ops, and the withdrawal id-selection logic (recent + matching surface only, workspace-key fallback).Note on the two-commit red/green policy: these are unit tests of a new type, so a failing-first commit cannot compile; an end-to-end repro would need a
UNUserNotificationCenterseam that doesn't exist today.Verification
swiftc -parseclean on all three touched files.reload.sh/xcodebuild testlocally — relying on CI for build + test. Happy to iterate on any failures.Fixes one source of #2322 (related: #942, #5286).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests