Fix notification ring dismissal on direct terminal clicks - #1126
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds dismissal of unread terminal notifications when the user clicks a terminal surface (even if already focused) and when reselecting a tab without modifiers; introduces a new TerminalNotificationStore API to perform "dismiss if active" checks and updates tests to verify the direct-interaction behavior. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as "ContentView / Terminal View"
participant NotifStore as "TerminalNotificationStore"
participant AppState as "App Focus State"
User->>UI: Click terminal surface (mouseDown) or reselect tab (no modifiers)
UI->>UI: becomeFirstResponder / focus handling
UI->>NotifStore: dismissUnreadNotificationIfActive(tabId, surfaceId)
NotifStore->>AppState: isAppActive?
alt App active and unread exists
NotifStore->>NotifStore: mark notification read
NotifStore-->>UI: return true
else App inactive or no unread
NotifStore-->>UI: return false
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 7479-7554: The test
testTerminalMouseDownDismissesUnreadWhenSurfaceIsAlreadyFirstResponder is
bypassing the real dismissal path by calling surfaceView.mouseDown(with:) on the
innermost view; instead invoke the GhosttySurfaceScrollView mouse handling so
GhosttySurfaceScrollView.mouseDown triggers the production dismissal hook.
Locate the hosted view via surfaceView(in:) and dispatch the click through the
scroll view (e.g., call mouseDown on terminalPanel.hostedView or send the event
to the window so GhosttySurfaceScrollView.mouseDown runs) and then assert the
workspace/tab-level unread state (check the Tab/Workspace unread API, e.g.,
verify store.hasUnreadNotification(forTabId: workspace.id) or the
manager/workspace unread count) rather than only the surface-scoped unread flag.
In `@Sources/ContentView.swift`:
- Around line 9842-9846: The reselect handling only dismisses notifications for
the exact focused surface (notificationStore.dismissUnreadNotificationIfActive
with surfaceId: tabManager.focusedSurfaceId(for:)), so workspace-scoped unreads
with surfaceId == nil are not cleared; update the wasSelected && !isCommand &&
!isShift branch to also call
notificationStore.dismissUnreadNotificationIfActive(tabId: tab.id, surfaceId:
nil) (in addition to the existing focusedSurfaceId call) so workspace-level
unread notifications are dismissed when a workspace row is reselected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3214160f-f807-447b-b76a-f92892700ef4
📒 Files selected for processing (4)
Sources/ContentView.swiftSources/GhosttyTerminalView.swiftSources/TerminalNotificationStore.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
Comment on lines
+7479
to
+7554
| private func surfaceView(in hostedView: GhosttySurfaceScrollView) -> NSView? { | ||
| hostedView.subviews | ||
| .compactMap { $0 as? NSScrollView } | ||
| .first? | ||
| .documentView? | ||
| .subviews | ||
| .first | ||
| } | ||
|
|
||
| func testTerminalMouseDownDismissesUnreadWhenSurfaceIsAlreadyFirstResponder() { | ||
| let appDelegate = AppDelegate.shared ?? AppDelegate() | ||
| let manager = TabManager() | ||
| let store = TerminalNotificationStore.shared | ||
| let window = makeWindow() | ||
|
|
||
| let originalTabManager = appDelegate.tabManager | ||
| let originalNotificationStore = appDelegate.notificationStore | ||
| let originalAppFocusOverride = AppFocusState.overrideIsFocused | ||
|
|
||
| store.replaceNotificationsForTesting([]) | ||
| store.configureNotificationDeliveryHandlerForTesting { _, _ in } | ||
| appDelegate.tabManager = manager | ||
| appDelegate.notificationStore = store | ||
|
|
||
| defer { | ||
| store.replaceNotificationsForTesting([]) | ||
| store.resetNotificationDeliveryHandlerForTesting() | ||
| appDelegate.tabManager = originalTabManager | ||
| appDelegate.notificationStore = originalNotificationStore | ||
| AppFocusState.overrideIsFocused = originalAppFocusOverride | ||
| window.orderOut(nil) | ||
| } | ||
|
|
||
| guard let workspace = manager.selectedWorkspace, | ||
| let terminalPanel = workspace.focusedTerminalPanel else { | ||
| XCTFail("Expected an initial focused terminal panel") | ||
| return | ||
| } | ||
|
|
||
| guard let contentView = window.contentView else { | ||
| XCTFail("Expected content view") | ||
| return | ||
| } | ||
|
|
||
| let hostedView = terminalPanel.hostedView | ||
| hostedView.frame = contentView.bounds | ||
| hostedView.autoresizingMask = [.width, .height] | ||
| contentView.addSubview(hostedView) | ||
| contentView.layoutSubtreeIfNeeded() | ||
| hostedView.layoutSubtreeIfNeeded() | ||
|
|
||
| guard let surfaceView = surfaceView(in: hostedView) else { | ||
| XCTFail("Expected terminal surface view") | ||
| return | ||
| } | ||
|
|
||
| AppFocusState.overrideIsFocused = true | ||
| XCTAssertTrue(window.makeFirstResponder(surfaceView)) | ||
|
|
||
| AppFocusState.overrideIsFocused = false | ||
| store.addNotification( | ||
| tabId: workspace.id, | ||
| surfaceId: terminalPanel.id, | ||
| title: "Unread", | ||
| subtitle: "", | ||
| body: "" | ||
| ) | ||
| XCTAssertTrue(store.hasUnreadNotification(forTabId: workspace.id, surfaceId: terminalPanel.id)) | ||
|
|
||
| AppFocusState.overrideIsFocused = true | ||
| let pointInWindow = surfaceView.convert(NSPoint(x: 20, y: 20), to: nil) | ||
| let event = makeMouseEvent(type: .leftMouseDown, location: pointInWindow, window: window) | ||
| surfaceView.mouseDown(with: event) | ||
|
|
||
| XCTAssertFalse(store.hasUnreadNotification(forTabId: workspace.id, surfaceId: terminalPanel.id)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ghostty_file=$(fd 'GhosttyTerminalView.swift$' | head -n1)
test_file=$(fd 'CmuxWebViewKeyEquivalentTests.swift$' | head -n1)
echo "== Production mouseDown wiring =="
rg -n -C4 'class GhosttySurfaceScrollView|func mouseDown|dismissUnreadNotificationIfActive' "$ghostty_file"
echo
echo "== Test click target + assertions =="
rg -n -C3 'surfaceView\(in: hostedView\)|surfaceView\.mouseDown|hostedView\.mouseDown|unreadCount\(forTabId:' "$test_file"Repository: manaflow-ai/cmux
Length of output: 3284
Exercise the real terminal click path and assert the tab-level unread state.
The test at line 7551 calls surfaceView.mouseDown(with:) directly on the innermost view, bypassing GhosttySurfaceScrollView.mouseDown where the production dismissal hook is wired (line 4544 in GhosttyTerminalView.swift). The test also asserts only the surface-scoped unread state, not the workspace/tab unread count that drives the visible ring. This can allow the test to pass without exercising the actual code being fixed.
Suggested test adjustment
- let pointInWindow = surfaceView.convert(NSPoint(x: 20, y: 20), to: nil)
+ let pointInWindow = hostedView.convert(NSPoint(x: 20, y: 20), to: nil)
let event = makeMouseEvent(type: .leftMouseDown, location: pointInWindow, window: window)
- surfaceView.mouseDown(with: event)
+ hostedView.mouseDown(with: event)
+ XCTAssertEqual(store.unreadCount(forTabId: workspace.id), 0)
XCTAssertFalse(store.hasUnreadNotification(forTabId: workspace.id, surfaceId: terminalPanel.id))Per coding guidelines (**/*Tests.swift): tests must verify observable runtime behavior through executable paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 7479 - 7554, The
test testTerminalMouseDownDismissesUnreadWhenSurfaceIsAlreadyFirstResponder is
bypassing the real dismissal path by calling surfaceView.mouseDown(with:) on the
innermost view; instead invoke the GhosttySurfaceScrollView mouse handling so
GhosttySurfaceScrollView.mouseDown triggers the production dismissal hook.
Locate the hosted view via surfaceView(in:) and dispatch the click through the
scroll view (e.g., call mouseDown on terminalPanel.hostedView or send the event
to the window so GhosttySurfaceScrollView.mouseDown runs) and then assert the
workspace/tab-level unread state (check the Tab/Workspace unread API, e.g.,
verify store.hasUnreadNotification(forTabId: workspace.id) or the
manager/workspace unread count) rather than only the surface-scoped unread flag.
Comment on lines
+9842
to
+9846
| if wasSelected, !isCommand, !isShift { | ||
| notificationStore.dismissUnreadNotificationIfActive( | ||
| tabId: tab.id, | ||
| surfaceId: tabManager.focusedSurfaceId(for: tab.id) | ||
| ) |
There was a problem hiding this comment.
Also dismiss workspace-scoped unread on row reselect.
dismissUnreadNotificationIfActive matches the exact (tabId, surfaceId) pair. Here we always pass focusedSurfaceId(for:), so a workspace-level unread stored with surfaceId == nil will survive clicking the already-selected workspace row—the case this PR is supposed to fix.
Suggested fix
tabManager.selectTab(tab)
if wasSelected, !isCommand, !isShift {
- notificationStore.dismissUnreadNotificationIfActive(
+ let focusedSurfaceId = tabManager.focusedSurfaceId(for: tab.id)
+ let dismissedFocusedSurface = notificationStore.dismissUnreadNotificationIfActive(
tabId: tab.id,
- surfaceId: tabManager.focusedSurfaceId(for: tab.id)
+ surfaceId: focusedSurfaceId
+ )
+ if !dismissedFocusedSurface {
+ notificationStore.dismissUnreadNotificationIfActive(
+ tabId: tab.id,
+ surfaceId: nil
+ )
)
}
selection = .tabs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ContentView.swift` around lines 9842 - 9846, The reselect handling
only dismisses notifications for the exact focused surface
(notificationStore.dismissUnreadNotificationIfActive with surfaceId:
tabManager.focusedSurfaceId(for:)), so workspace-scoped unreads with surfaceId
== nil are not cleared; update the wasSelected && !isCommand && !isShift branch
to also call notificationStore.dismissUnreadNotificationIfActive(tabId: tab.id,
surfaceId: nil) (in addition to the existing focusedSurfaceId call) so
workspace-level unread notifications are dismissed when a workspace row is
reselected.
Contributor
Greptile SummaryThis PR fixes a regression where clicking an already-focused terminal (or re-selecting an already-selected workspace row) left the notification ring visible, because no focus-transition event fired to trigger the existing
Confidence Score: 4/5
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User clicks terminal or workspace row] --> B{Already focused / selected?}
B -- No --> C[Normal focus transition fires\nExisting markRead on focus handles dismissal]
B -- Yes --> D[No focus transition fires\nNotification ring was stuck]
D --> E{GhosttyNSView.mouseDown\nor TabItemView tap handler}
E --> F[dismissUnreadNotificationIfActive]
F --> G{AppFocusState.isAppActive?}
G -- No --> H[Return false\nNo dismissal]
G -- Yes --> I{hasUnreadNotification?}
I -- No --> J[Return false\nNothing to dismiss]
I -- Yes --> K[markRead for tabId + surfaceId\nClears notification ring]
K --> L[Return true]
Last reviewed commit: e5e6ba7 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+7488
to
+7554
| func testTerminalMouseDownDismissesUnreadWhenSurfaceIsAlreadyFirstResponder() { | ||
| let appDelegate = AppDelegate.shared ?? AppDelegate() | ||
| let manager = TabManager() | ||
| let store = TerminalNotificationStore.shared | ||
| let window = makeWindow() | ||
|
|
||
| let originalTabManager = appDelegate.tabManager | ||
| let originalNotificationStore = appDelegate.notificationStore | ||
| let originalAppFocusOverride = AppFocusState.overrideIsFocused | ||
|
|
||
| store.replaceNotificationsForTesting([]) | ||
| store.configureNotificationDeliveryHandlerForTesting { _, _ in } | ||
| appDelegate.tabManager = manager | ||
| appDelegate.notificationStore = store | ||
|
|
||
| defer { | ||
| store.replaceNotificationsForTesting([]) | ||
| store.resetNotificationDeliveryHandlerForTesting() | ||
| appDelegate.tabManager = originalTabManager | ||
| appDelegate.notificationStore = originalNotificationStore | ||
| AppFocusState.overrideIsFocused = originalAppFocusOverride | ||
| window.orderOut(nil) | ||
| } | ||
|
|
||
| guard let workspace = manager.selectedWorkspace, | ||
| let terminalPanel = workspace.focusedTerminalPanel else { | ||
| XCTFail("Expected an initial focused terminal panel") | ||
| return | ||
| } | ||
|
|
||
| guard let contentView = window.contentView else { | ||
| XCTFail("Expected content view") | ||
| return | ||
| } | ||
|
|
||
| let hostedView = terminalPanel.hostedView | ||
| hostedView.frame = contentView.bounds | ||
| hostedView.autoresizingMask = [.width, .height] | ||
| contentView.addSubview(hostedView) | ||
| contentView.layoutSubtreeIfNeeded() | ||
| hostedView.layoutSubtreeIfNeeded() | ||
|
|
||
| guard let surfaceView = surfaceView(in: hostedView) else { | ||
| XCTFail("Expected terminal surface view") | ||
| return | ||
| } | ||
|
|
||
| AppFocusState.overrideIsFocused = true | ||
| XCTAssertTrue(window.makeFirstResponder(surfaceView)) | ||
|
|
||
| AppFocusState.overrideIsFocused = false | ||
| store.addNotification( | ||
| tabId: workspace.id, | ||
| surfaceId: terminalPanel.id, | ||
| title: "Unread", | ||
| subtitle: "", | ||
| body: "" | ||
| ) | ||
| XCTAssertTrue(store.hasUnreadNotification(forTabId: workspace.id, surfaceId: terminalPanel.id)) | ||
|
|
||
| AppFocusState.overrideIsFocused = true | ||
| let pointInWindow = surfaceView.convert(NSPoint(x: 20, y: 20), to: nil) | ||
| let event = makeMouseEvent(type: .leftMouseDown, location: pointInWindow, window: window) | ||
| surfaceView.mouseDown(with: event) | ||
|
|
||
| XCTAssertFalse(store.hasUnreadNotification(forTabId: workspace.id, surfaceId: terminalPanel.id)) | ||
| } |
Contributor
There was a problem hiding this comment.
Missing test for the workspace-row click dismissal path
The new test covers the GhosttyNSView.mouseDown dismissal logic, but the companion fix in ContentView.swift — dismissing the notification ring when clicking an already-selected workspace row — has no corresponding test. The TabItemView click handler is testable (similar action handler patterns exist elsewhere in the test file), and adding a test would close the coverage gap and protect against future regressions in the sidebar dismissal logic.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
7547-7552:⚠️ Potential issue | 🟠 MajorDrive the click through
GhosttySurfaceScrollViewand assert the tab-level unread clears.This still calls
surfaceView.mouseDown(with:), which bypassesGhosttySurfaceScrollView.mouseDown(with:)inSources/GhosttyTerminalView.swift:4543-4559where the unread-dismissal hook actually lives. That means the regression can pass without exercising the fixed path, and it only verifies the surface-scoped flag instead of the workspace/tab unread state that keeps the ring visible.Suggested test adjustment
- let pointInWindow = surfaceView.convert(NSPoint(x: 20, y: 20), to: nil) + let pointInWindow = hostedView.convert(NSPoint(x: 20, y: 20), to: nil) let event = makeMouseEvent(type: .leftMouseDown, location: pointInWindow, window: window) - surfaceView.mouseDown(with: event) + hostedView.mouseDown(with: event) + XCTAssertEqual(store.unreadCount(forTabId: workspace.id), 0) XCTAssertFalse(store.hasUnreadNotification(forTabId: workspace.id, surfaceId: terminalPanel.id))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 7547 - 7552, The test currently calls surfaceView.mouseDown(with:) which bypasses the unread-dismissal logic in GhosttySurfaceScrollView.mouseDown(with:) (see GhosttyTerminalView.mouseDown area), so change the test to dispatch the mouse event through the GhosttySurfaceScrollView instance instead of the raw surface view and then assert the tab/workspace unread clears via store.hasUnreadNotification(forTabId: workspace.id, surfaceId: terminalPanel.id) (or the tab-level check used elsewhere); specifically locate where the test constructs pointInWindow and event and call ghosttySurfaceScrollView.mouseDown(with: event) (or send the event to the scroll view that contains surfaceView) so the unread-dismissal hook in GhosttySurfaceScrollView.mouseDown is exercised and then verify the workspace/tab unread state is cleared.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 7547-7552: The test currently calls surfaceView.mouseDown(with:)
which bypasses the unread-dismissal logic in
GhosttySurfaceScrollView.mouseDown(with:) (see GhosttyTerminalView.mouseDown
area), so change the test to dispatch the mouse event through the
GhosttySurfaceScrollView instance instead of the raw surface view and then
assert the tab/workspace unread clears via store.hasUnreadNotification(forTabId:
workspace.id, surfaceId: terminalPanel.id) (or the tab-level check used
elsewhere); specifically locate where the test constructs pointInWindow and
event and call ghosttySurfaceScrollView.mouseDown(with: event) (or send the
event to the scroll view that contains surfaceView) so the unread-dismissal hook
in GhosttySurfaceScrollView.mouseDown is exercised and then verify the
workspace/tab unread state is cleared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3be5309-440c-462e-9319-bb9b9f154c13
📒 Files selected for processing (2)
Sources/TerminalNotificationStore.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 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="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:5476">
P2: The flash path is always reset to `.standardFocus` during layout synchronization, which will visibly change the flash ring shape mid-animation if a `.notificationDismiss` flash is playing. Store the active flash style in a property and use it here instead of hardcoding `.standardFocus`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| _ = setFrameIfNeeded(flashOverlayView, to: bounds) | ||
| updateNotificationRingPath() | ||
| updateFlashPath() | ||
| updateFlashPath(style: .standardFocus) |
There was a problem hiding this comment.
P2: The flash path is always reset to .standardFocus during layout synchronization, which will visibly change the flash ring shape mid-animation if a .notificationDismiss flash is playing. Store the active flash style in a property and use it here instead of hardcoding .standardFocus.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/GhosttyTerminalView.swift, line 5476:
<comment>The flash path is always reset to `.standardFocus` during layout synchronization, which will visibly change the flash ring shape mid-animation if a `.notificationDismiss` flash is playing. Store the active flash style in a property and use it here instead of hardcoding `.standardFocus`.</comment>
<file context>
@@ -5463,7 +5473,7 @@ final class GhosttySurfaceScrollView: NSView {
_ = setFrameIfNeeded(flashOverlayView, to: bounds)
updateNotificationRingPath()
- updateFlashPath()
+ updateFlashPath(style: .standardFocus)
synchronizeScrollView()
synchronizeSurfaceView()
</file context>
This was referenced Mar 10, 2026
This was referenced Mar 20, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…i#1126) * Add regression test for terminal notification click dismissal * Dismiss terminal notifications on direct clicks * Add regression for focused terminal notification ring * Keep focused terminal notifications unread until click * Verify direct notification dismiss triggers flash * Use focus-flash path for direct notification dismiss * Align notification dismiss flash with ring geometry
This was referenced May 5, 2026
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
Closes #1123
Notes
** BUILD SUCCEEDED **
Full build log: /tmp/cmux-xcodebuild-issue-1123-notification-ring-dismiss.log
Tag cleanup status:
current tag: issue-1123-notification-ring-dismiss (keep this running until you verify)
stale tags: none
stale cleanup: not needed
After you verify current tag, cleanup command:
pkill -f "cmux DEV issue-1123-notification-ring-dismiss.app/Contents/MacOS/cmux DEV"
rm -rf "/tmp/cmux-issue-1123-notification-ring-dismiss" "/tmp/cmux-debug-issue-1123-notification-ring-dismiss.sock"
rm -f "/tmp/cmux-debug-issue-1123-notification-ring-dismiss.log"
rm -f "/Users/austinwang/Library/Application Support/cmux/cmuxd-dev-issue-1123-notification-ring-dismiss.sock"
Summary by cubic
Fixes notification rings not dismissing on direct clicks of an already-focused terminal or the selected workspace (addresses #1123). When the app is active, focused-terminal notifications stay unread and aren’t delivered externally until you click; on mouse down they dismiss and show a flash that matches the notification ring.
Written for commit e29e7c8. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests