Fix notification unread persistence when workspaces regain focus - #971
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughNotification handling was changed to preserve unread notifications for focused panels while providing a flash visual cue instead of auto-marking them read; native delivery is suppressed when focused but notifications are stored; tests and helpers updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Source as Notification Source
participant Store as TerminalNotificationStore
participant App as AppDelegate/FocusState
participant UI as TabManager/PanelView
participant System as macOS Notification Center
Source->>Store: deliver(notification)
Store->>App: query isAppFocused && isFocusedPanel
alt app+panel focused (suppress)
Store->>Store: store(notification) rgba(0,128,255,0.5)
Store-->>System: (skip scheduling)
Store->>UI: trigger flash for focused panel rgba(255,215,0,0.5)
else not focused
Store->>Store: store(notification) rgba(0,128,255,0.5)
Store->>System: scheduleUserNotification(notification)
end
UI->>UI: flash panel (visual cue)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxTests/CmuxWebViewKeyEquivalentTests.swift">
<violation number="1" location="cmuxTests/CmuxWebViewKeyEquivalentTests.swift:7307">
P2: Avoid fixed-time async delays in this test; the hardcoded `0.1s` wait can make CI runs flaky and slower. Use a deterministic queue-drain or event-based expectation instead.
(Based on your team's feedback about avoiding fixed sleeps in tests to reduce flakiness.) [FEEDBACK_USED]</violation>
</file>
<file name="tests/test_focus_notification_dismiss.py">
<violation number="1" location="tests/test_focus_notification_dismiss.py:77">
P2: The post-focus unread assertion is too weak and can pass even if the notification becomes read shortly after focus, causing a false-negative regression test.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Greptile SummaryThis PR changes the notification lifecycle so that unread state persists when a workspace regains focus — previously, focusing a panel, switching workspaces, or activating the app would silently mark notifications as read. Now the internal Key changes:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[addNotification called] --> B{isAppFocused AND isFocusedPanel?}
B -- Yes --> C[suppressNativeDelivery = true]
B -- No --> D[suppressNativeDelivery = false]
C --> E[Remove old notifications for tab/surface]
D --> E
E --> F{suppressNativeDelivery?}
F -- No --> G[moveTabToTop via WorkspaceAutoReorderSettings]
F -- Yes --> H
G --> H[Create TerminalNotification with isRead=false]
H --> I[Insert into internal store]
I --> J[Remove old IDs from OS notification center]
J --> K{suppressNativeDelivery?}
K -- No --> L[scheduleUserNotification]
K -- Yes --> M[Skip OS notification\nNotification stored as unread internally]
subgraph FocusEvents [Focus / Activation Events - REMOVED markRead calls]
N[applicationDidBecomeActive] --> O[triggerNotificationFocusFlash only]
P[selectWorkspace / tab switch] --> Q[flashFocusedPanelIfUnreadAndActive only]
R[focusTabFromNotification] --> S[triggerNotificationFocusFlash only]
end
subgraph MarkReadPaths [Remaining markRead Paths]
T[User dismisses OS notification\nUNNotificationDismissActionIdentifier] --> U[markRead by ID]
V[Workspace.markPanelRead via UI] --> W[markRead forTabId+surfaceId]
X[TerminalController workspace close] --> Y[markRead forTabId]
end
Last reviewed commit: 82f8a13 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (2)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
7306-7308: Remove fixed-delay synchronization in test.Line 7307 uses a 0.1-second explicit delay before draining the queue. Replace with deterministic
DispatchQueue.main.asyncto reduce test latency and eliminate timing sensitivity.♻️ Proposed change
- let drained = expectation(description: "notification focus drained") - DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { drained.fulfill() } - wait(for: [drained], timeout: 1.0) + let drained = expectation(description: "notification focus drained") + DispatchQueue.main.async { drained.fulfill() } + wait(for: [drained], timeout: 1.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 7306 - 7308, Replace the fixed 0.1s delay used to drain the main queue: find the expectation named `drained` and the `DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { drained.fulfill() }` call and change it to use `DispatchQueue.main.async { drained.fulfill() }` so the test deterministically waits for the next runloop turn without an arbitrary sleep; keep the existing `wait(for: [drained], timeout: 1.0)` assertion unchanged.tests/test_notifications.py (1)
218-307: Prefer polling over fixed 100ms sleeps in unread-preservation tests.Line 232, Line 236, Line 261, Line 269, Line 290, Line 297 use fixed
time.sleep(0.1)before assertions. These transitions are async and can intermittently race in CI. Please switch these checkpoints to existing polling helpers (wait_for_notifications/wait_for_flash_count) to reduce flakes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_notifications.py` around lines 218 - 307, Replace the fixed 0.1s time.sleep calls in the unread-preservation tests with the existing polling helpers to avoid races: in test_preserve_unread_on_focus_change, replace the sleeps after client.notify_surface(other[0], ...), client.focus_surface(other[0]) with wait_for_flash_count(client, other[1], minimum=1, timeout=...) (or wait_for_notifications to detect the new notification) and use that result before asserting; in test_preserve_unread_on_app_active, after client.notify("activate") and after client.simulate_app_active() poll with wait_for_notifications(...) to ensure the notification appears and remains unread before checking items; in test_preserve_unread_on_tab_switch, replace sleeps after client.notify("tabswitch"), client.new_workspace(), and client.select_workspace(tab1) with wait_for_notifications(...) (and/or wait_for_flash_count where appropriate) to wait for the notification to be present for tab1 before asserting. Ensure time.sleep removals are paired with sensible timeouts so assertions only run after the async events complete.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/TerminalNotificationStore.swift`:
- Around line 858-860: When suppressNativeDelivery is true the call to
scheduleUserNotification(notification) is skipped which prevents
NotificationSoundSettings.runCustomCommand(...) from executing; ensure custom
command hooks still run even when native delivery is suppressed by invoking
NotificationSoundSettings.runCustomCommand(...) (or a helper that triggers the
same hook) in the branch where scheduleUserNotification would be skipped. Update
the logic around suppressNativeDelivery and scheduleUserNotification so that
after deciding not to call scheduleUserNotification(notification) you still call
NotificationSoundSettings.runCustomCommand(for: notification) (or refactor to a
shared method invoked in both paths) and preserve any existing early-return
behavior.
---
Nitpick comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 7306-7308: Replace the fixed 0.1s delay used to drain the main
queue: find the expectation named `drained` and the
`DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { drained.fulfill() }`
call and change it to use `DispatchQueue.main.async { drained.fulfill() }` so
the test deterministically waits for the next runloop turn without an arbitrary
sleep; keep the existing `wait(for: [drained], timeout: 1.0)` assertion
unchanged.
In `@tests/test_notifications.py`:
- Around line 218-307: Replace the fixed 0.1s time.sleep calls in the
unread-preservation tests with the existing polling helpers to avoid races: in
test_preserve_unread_on_focus_change, replace the sleeps after
client.notify_surface(other[0], ...), client.focus_surface(other[0]) with
wait_for_flash_count(client, other[1], minimum=1, timeout=...) (or
wait_for_notifications to detect the new notification) and use that result
before asserting; in test_preserve_unread_on_app_active, after
client.notify("activate") and after client.simulate_app_active() poll with
wait_for_notifications(...) to ensure the notification appears and remains
unread before checking items; in test_preserve_unread_on_tab_switch, replace
sleeps after client.notify("tabswitch"), client.new_workspace(), and
client.select_workspace(tab1) with wait_for_notifications(...) (and/or
wait_for_flash_count where appropriate) to wait for the notification to be
present for tab1 before asserting. Ensure time.sleep removals are paired with
sensible timeouts so assertions only run after the async events complete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 472384eb-c8c9-4344-8ddb-f3b085f306c3
📒 Files selected for processing (8)
Sources/AppDelegate.swiftSources/TabManager.swiftSources/TerminalNotificationStore.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swifttests/test_focus_notification_dismiss.pytests/test_notifications.pytests_v2/test_focus_notification_dismiss.pytests_v2/test_notifications.py
💤 Files with no reviewable changes (1)
- Sources/AppDelegate.swift
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxTests/CmuxWebViewKeyEquivalentTests.swift">
<violation number="1" location="cmuxTests/CmuxWebViewKeyEquivalentTests.swift:7308">
P2: This async drain can finish before notification-focus delayed side effects run, so the test may assert too early and miss regressions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| tabManager.focusTabFromNotification(tabId, surfaceId: surfaceId) | ||
|
|
||
| let drained = expectation(description: "notification focus drained") | ||
| DispatchQueue.main.async { drained.fulfill() } |
There was a problem hiding this comment.
P2: This async drain can finish before notification-focus delayed side effects run, so the test may assert too early and miss regressions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/CmuxWebViewKeyEquivalentTests.swift, line 7308:
<comment>This async drain can finish before notification-focus delayed side effects run, so the test may assert too early and miss regressions.</comment>
<file context>
@@ -7304,7 +7305,7 @@ final class NotificationDockBadgeTests: XCTestCase {
let drained = expectation(description: "notification focus drained")
- DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { drained.fulfill() }
+ DispatchQueue.main.async { drained.fulfill() }
wait(for: [drained], timeout: 1.0)
</file context>
This was referenced Mar 7, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…aflow-ai#971) * Fix notification unread persistence on focus * Address review feedback on notification unread fix
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…cus (manaflow-ai#971)" (manaflow-ai#992) This reverts commit 5e0869c.
israeligal
pushed a commit
to israeligal/cmux
that referenced
this pull request
May 2, 2026
…cus (manaflow-ai#971)" This reverts commit 5e0869c.
This was referenced May 6, 2026
rodchristiansen
pushed a commit
to rodchristiansen/cmux
that referenced
this pull request
Sep 2, 2026
…cus (manaflow-ai#971)" This reverts commit 7204cf2.
This branch was successfully deployed
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
1 participant
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
Verification
Closes #963
Summary by cubic
Fixes unread notification persistence when the app, workspace, or panel regains focus. Notifications stay unread while focused panels still flash for visibility; native delivery is suppressed when focus is present.
Written for commit 82f8a13. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests