Skip to content

Fix notification ring dismissal on direct terminal clicks #1126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
austinywang merged 7 commits into from
Mar 10, 2026
Merged

Fix notification ring dismissal on direct terminal clicks #1126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2af34b4
Add regression test for terminal notification click dismissal
austinywang Mar 9, 2026
e5e6ba7
Dismiss terminal notifications on direct clicks
austinywang Mar 9, 2026
650bafa
Add regression for focused terminal notification ring
austinywang Mar 10, 2026
989645d
Keep focused terminal notifications unread until click
austinywang Mar 10, 2026
28a1b9b
Verify direct notification dismiss triggers flash
austinywang Mar 10, 2026
bb4ab6f
Use focus-flash path for direct notification dismiss
austinywang Mar 10, 2026
e29e7c8
Align notification dismiss flash with ring geometry
austinywang Mar 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Sources/ContentView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9816,6 +9816,7 @@ private struct TabItemView: View {
let modifiers = NSEvent.modifierFlags
let isCommand = modifiers.contains(.command)
let isShift = modifiers.contains(.shift)
let wasSelected = tabManager.selectedTabId == tab.id

if isShift, let lastIndex = lastSidebarSelectionIndex {
let lower = min(lastIndex, index)
Expand All @@ -9838,6 +9839,12 @@ private struct TabItemView: View {

lastSidebarSelectionIndex = index
tabManager.selectTab(tab)
if wasSelected, !isCommand, !isShift {
tabManager.dismissNotificationOnDirectInteraction(
tabId: tab.id,
surfaceId: tabManager.focusedSurfaceId(for: tab.id)
)
Comment on lines +9842 to +9846

@coderabbitai coderabbitai Bot Mar 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

}
selection = .tabs
}

Expand Down
42 changes: 34 additions & 8 deletions Sources/GhosttyTerminalView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4547,6 +4547,12 @@ class GhosttyNSView: NSView, NSUserInterfaceValidations {
dlog("terminal.mouseDown surface=\(terminalSurface?.id.uuidString.prefix(5) ?? "nil") mods=[\(debugModifierString(event.modifierFlags))] clickCount=\(event.clickCount) point=(\(String(format: "%.0f", debugPoint.x)),\(String(format: "%.0f", debugPoint.y)))")
#endif
window?.makeFirstResponder(self)
if let terminalSurface {
AppDelegate.shared?.tabManager?.dismissNotificationOnDirectInteraction(
tabId: terminalSurface.tabId,
surfaceId: terminalSurface.id
)
}
guard let surface = surface else { return }
let point = convert(event.locationInWindow, from: nil)
ghostty_surface_mouse_pos(surface, point.x, bounds.height - point.y, modsFromEvent(event))
Expand Down Expand Up @@ -5017,6 +5023,16 @@ private final class GhosttyPassthroughVisualEffectView: NSVisualEffectView {
}

final class GhosttySurfaceScrollView: NSView {
enum FlashStyle {
case standardFocus
case notificationDismiss
}

private enum NotificationRingMetrics {
static let inset: CGFloat = 2
static let cornerRadius: CGFloat = 6
}

private let backgroundView: NSView
private let scrollView: GhosttyScrollView
private let documentView: NSView
Expand Down Expand Up @@ -5457,7 +5473,7 @@ final class GhosttySurfaceScrollView: NSView {
_ = setFrameIfNeeded(notificationRingOverlayView, to: bounds)
_ = setFrameIfNeeded(flashOverlayView, to: bounds)
updateNotificationRingPath()
updateFlashPath()
updateFlashPath(style: .standardFocus)

@cubic-dev-ai cubic-dev-ai Bot Mar 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Fix with Cubic

synchronizeScrollView()
synchronizeSurfaceView()
let didCoreSurfaceChange = synchronizeCoreSurface()
Expand Down Expand Up @@ -5892,15 +5908,15 @@ final class GhosttySurfaceScrollView: NSView {
}
#endif

func triggerFlash() {
func triggerFlash(style: FlashStyle = .standardFocus) {
DispatchQueue.main.async { [weak self] in
guard let self else { return }
#if DEBUG
if let surfaceId = self.surfaceView.terminalSurface?.id {
Self.recordFlash(for: surfaceId)
}
#endif
self.updateFlashPath()
self.updateFlashPath(style: style)
self.flashLayer.removeAllAnimations()
self.flashLayer.opacity = 0
let animation = CAKeyframeAnimation(keyPath: "opacity")
Expand Down Expand Up @@ -6642,17 +6658,27 @@ final class GhosttySurfaceScrollView: NSView {
updateOverlayRingPath(
layer: notificationRingLayer,
bounds: notificationRingOverlayView.bounds,
inset: 2,
radius: 6
inset: NotificationRingMetrics.inset,
radius: NotificationRingMetrics.cornerRadius
)
}

private func updateFlashPath() {
private func updateFlashPath(style: FlashStyle) {
let inset: CGFloat
let radius: CGFloat
switch style {
case .standardFocus:
inset = CGFloat(FocusFlashPattern.ringInset)
radius = CGFloat(FocusFlashPattern.ringCornerRadius)
case .notificationDismiss:
inset = NotificationRingMetrics.inset
radius = NotificationRingMetrics.cornerRadius
}
updateOverlayRingPath(
layer: flashLayer,
bounds: flashOverlayView.bounds,
inset: CGFloat(FocusFlashPattern.ringInset),
radius: CGFloat(FocusFlashPattern.ringCornerRadius)
inset: inset,
radius: radius
)
}

Expand Down
4 changes: 4 additions & 0 deletions Sources/Panels/TerminalPanel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,10 @@ final class TerminalPanel: Panel, ObservableObject {
hostedView.triggerFlash()
}

func triggerNotificationDismissFlash() {
hostedView.triggerFlash(style: .notificationDismiss)
}

func applyWindowBackgroundIfActive() {
surface.applyWindowBackgroundIfActive()
}
Expand Down
30 changes: 24 additions & 6 deletions Sources/TabManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1819,19 +1819,37 @@ class TabManager: ObservableObject {
guard !shouldSuppressFlash else { return }
guard AppFocusState.isAppActive() else { return }
guard let panelId = focusedPanelId(for: tabId) else { return }
markPanelReadOnFocusIfActive(tabId: tabId, panelId: panelId)
_ = dismissNotificationIfActive(tabId: tabId, surfaceId: panelId, triggerFlash: true)
}

private func markPanelReadOnFocusIfActive(tabId: UUID, panelId: UUID) {
guard selectedTabId == tabId else { return }
guard !suppressFocusFlash else { return }
guard AppFocusState.isAppActive() else { return }
guard let notificationStore = AppDelegate.shared?.notificationStore else { return }
guard notificationStore.hasUnreadNotification(forTabId: tabId, surfaceId: panelId) else { return }
if let tab = tabs.first(where: { $0.id == tabId }) {
_ = dismissNotificationIfActive(tabId: tabId, surfaceId: panelId, triggerFlash: true)
}

@discardableResult
func dismissNotificationOnDirectInteraction(tabId: UUID, surfaceId: UUID?) -> Bool {
dismissNotificationIfActive(tabId: tabId, surfaceId: surfaceId, triggerFlash: true)
}

@discardableResult
private func dismissNotificationIfActive(
tabId: UUID,
surfaceId: UUID?,
triggerFlash: Bool
) -> Bool {
guard selectedTabId == tabId else { return false }
guard AppFocusState.isAppActive() else { return false }
guard let notificationStore = AppDelegate.shared?.notificationStore else { return false }
guard notificationStore.hasUnreadNotification(forTabId: tabId, surfaceId: surfaceId) else { return false }
if triggerFlash,
let panelId = surfaceId,
let tab = tabs.first(where: { $0.id == tabId }) {
tab.triggerNotificationFocusFlash(panelId: panelId, requiresSplit: false, shouldFocus: false)
}
notificationStore.markRead(forTabId: tabId, surfaceId: panelId)
notificationStore.markRead(forTabId: tabId, surfaceId: surfaceId)
return true
}

private func enqueuePanelTitleUpdate(tabId: UUID, panelId: UUID, title: String) {
Expand Down
13 changes: 4 additions & 9 deletions Sources/TerminalNotificationStore.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,14 +838,7 @@ final class TerminalNotificationStore: ObservableObject {
let isFocusedSurface = surfaceId == nil || focusedSurfaceId == surfaceId
let isFocusedPanel = isActiveTab && isFocusedSurface
let isAppFocused = AppFocusState.isAppFocused()
if isAppFocused && isFocusedPanel {
if !idsToClear.isEmpty {
notifications = updated
center.removeDeliveredNotificationsOffMain(withIdentifiers: idsToClear)
center.removePendingNotificationRequestsOffMain(withIdentifiers: idsToClear)
}
return
}
let shouldSuppressExternalDelivery = isAppFocused && isFocusedPanel

if WorkspaceAutoReorderSettings.isEnabled() {
AppDelegate.shared?.tabManager?.moveTabToTopForNotification(tabId)
Expand All @@ -867,7 +860,9 @@ final class TerminalNotificationStore: ObservableObject {
center.removeDeliveredNotificationsOffMain(withIdentifiers: idsToClear)
center.removePendingNotificationRequestsOffMain(withIdentifiers: idsToClear)
}
notificationDeliveryHandler(self, notification)
if !shouldSuppressExternalDelivery {
notificationDeliveryHandler(self, notification)
}
}

func markRead(id: UUID) {
Expand Down
2 changes: 1 addition & 1 deletion Sources/Workspace.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3260,7 +3260,7 @@ final class Workspace: Identifiable, ObservableObject {
if requiresSplit && !isSplit {
return
}
terminalPanel.triggerFlash()
terminalPanel.triggerNotificationDismissFlash()
}

func triggerDebugFlash(panelId: UUID) {
Expand Down
112 changes: 112 additions & 0 deletions cmuxTests/CmuxWebViewKeyEquivalentTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7446,6 +7446,118 @@ final class NotificationDockBadgeTests: XCTestCase {
}
}

@MainActor
final class TerminalNotificationDirectInteractionTests: XCTestCase {
private func makeWindow() -> NSWindow {
let window = NSWindow(
contentRect: NSRect(x: 0, y: 0, width: 480, height: 320),
styleMask: [.titled, .closable],
backing: .buffered,
defer: false
)
window.contentView = NSView(frame: window.contentRect(forFrameRect: window.frame))
return window
}

private func makeMouseEvent(type: NSEvent.EventType, location: NSPoint, window: NSWindow) -> NSEvent {
guard let event = NSEvent.mouseEvent(
with: type,
location: location,
modifierFlags: [],
timestamp: ProcessInfo.processInfo.systemUptime,
windowNumber: window.windowNumber,
context: nil,
eventNumber: 0,
clickCount: 1,
pressure: 1.0
) else {
fatalError("Failed to create \(type) mouse event")
}
return event
}

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
}

GhosttySurfaceScrollView.resetFlashCounts()
AppFocusState.overrideIsFocused = true
XCTAssertTrue(window.makeFirstResponder(surfaceView))

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)
let drained = expectation(description: "flash drained")
DispatchQueue.main.async { drained.fulfill() }
wait(for: [drained], timeout: 1.0)

XCTAssertFalse(store.hasUnreadNotification(forTabId: workspace.id, surfaceId: terminalPanel.id))
XCTAssertEqual(GhosttySurfaceScrollView.flashCount(for: terminalPanel.id), 1)
}
Comment on lines +7479 to +7558

@coderabbitai coderabbitai Bot Mar 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 +7488 to +7558

@greptile-apps greptile-apps Bot Mar 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}


final class MenuBarBadgeLabelFormatterTests: XCTestCase {
func testBadgeLabelFormatting() {
Expand Down