Dismiss notifications on click/keystroke in focused terminal - #1011
lawrencecchen wants to merge 6 commits into
Conversation
When a terminal tab has a notification ring and the user clicks or types in that terminal, the notification is now automatically dismissed with a flash animation. This handles the edge case where a notification arrives for the currently focused terminal. The implementation adds a dismissNotificationIfPresent() method to GhosttyTerminalView that: - Checks if the current tab/surface has unread notifications - Triggers the notification flash animation - Marks the notifications as read This is called at the start of both mouseDown() and keyDown() event handlers. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughNotification auto-dismiss on focus was removed. A new private method, dismissNotificationIfPresent(), was added to GhosttyNSView and is invoked at the start of keyDown(with:) and mouseDown(with:) to mark unread notifications as read and trigger a workspace panel focus flash on user interaction. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Terminal as GhosttyNSView
participant NotifStore as TerminalNotificationStore
participant TabMgr as TabManager
participant WorkspacePanel as Workspace Panel
User->>Terminal: Keyboard or Mouse input
activate Terminal
Terminal->>Terminal: dismissNotificationIfPresent()
alt Unread notification exists for current tab/surface
Terminal->>NotifStore: query unread notification for tab/surface
activate NotifStore
NotifStore-->>Terminal: unread notification
deactivate NotifStore
Terminal->>NotifStore: mark notification as read
activate NotifStore
NotifStore-->>Terminal: ack update
deactivate NotifStore
Terminal->>TabMgr: get workspace panel for tab
activate TabMgr
TabMgr-->>Terminal: workspace panel reference
deactivate TabMgr
Terminal->>WorkspacePanel: trigger focus flash
activate WorkspacePanel
WorkspacePanel-->>Terminal: flash completed
deactivate WorkspacePanel
end
deactivate Terminal
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds automatic notification dismissal (with a flash animation) when the user interacts with a focused terminal that already has an active notification ring — handling the edge case where the notification arrives for the terminal the user is currently in. Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
actor User
participant GhosttyNSView
participant NotificationStore
participant TabManager
participant Workspace
User->>GhosttyNSView: keyDown / mouseDown
GhosttyNSView->>GhosttyNSView: dismissNotificationIfPresent()
GhosttyNSView->>NotificationStore: hasUnreadNotification(forTabId:surfaceId:)
NotificationStore-->>GhosttyNSView: true / false
alt notification present
GhosttyNSView->>TabManager: tabs.first(where: id == tabId)
TabManager-->>GhosttyNSView: workspace (optional)
alt workspace found
GhosttyNSView->>Workspace: triggerNotificationFocusFlash(panelId:requiresSplit:shouldFocus:)
Workspace->>Workspace: triggerFlash() (visual ring animation)
end
GhosttyNSView->>NotificationStore: markRead(forTabId:surfaceId:)
end
GhosttyNSView->>GhosttyNSView: continue normal event handling
Last reviewed commit: c6360c8 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let workspace = tabManager.tabs.first(where: { $0.id == tabId }) { | ||
| workspace.triggerNotificationFocusFlash(panelId: surfaceId, requiresSplit: false, shouldFocus: false) | ||
| } | ||
| notificationStore.markRead(forTabId: tabId, surfaceId: surfaceId) |
There was a problem hiding this comment.
Silent dismissal when workspace lookup fails
If tabManager.tabs.first(where: { $0.id == tabId }) returns nil (workspace not found for this tab ID), notificationStore.markRead is still called unconditionally, dismissing the notification silently without the flash animation. Per the PR description, the flash is meant to be the visual confirmation that dismissal occurred. In the unlikely edge case where the workspace is missing, the ring would disappear with no feedback, which is inconsistent with the intended UX.
Consider guarding markRead on the same optional, or at minimum triggering the read first and the flash second (reversed order doesn't apply here, but the guard could be restructured):
guard let workspace = tabManager.tabs.first(where: { $0.id == tabId }) else {
return
}
workspace.triggerNotificationFocusFlash(panelId: surfaceId, requiresSplit: false, shouldFocus: false)
notificationStore.markRead(forTabId: tabId, surfaceId: surfaceId)This ensures the flash and the read are always paired, or neither happens (leaving the notification in its unread state for the next interaction attempt).
![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 1 file
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:3393">
P2: `markRead` is executed even when no matching workspace is found, which can clear the notification without the expected flash feedback. Guard on the workspace lookup so the flash and read stay paired.</violation>
<violation number="2" location="Sources/GhosttyTerminalView.swift:3974">
P2: Notification dismissal is only wired to left-click; right/middle clicks in the terminal do not clear unread notifications.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Previously, notifications were silently dropped when sent to an already-focused terminal. Now they are added to the store and shown with a blue ring, then dismissed on the next user interaction (click/keystroke) with the flash animation.
There was a problem hiding this comment.
1 issue found across 2 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:3384">
P1: Debug scaffolding (`logPath`, `timestamp`, `writeLog`) is defined outside `#if DEBUG`, so it compiles into Release builds. The `writeLog` function also duplicates the project's unified `dlog` system by writing to a separate hardcoded path (`/tmp/cmux-notif-dismiss-debug.log`). Remove these — the existing `dlog` calls inside `#if DEBUG` are sufficient.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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 `@Sources/GhosttyTerminalView.swift`:
- Around line 3383-3429: The dismissNotificationIfPresent function introduces a
bespoke /tmp debug file logger (logPath, timestamp, writeLog(_:), and calls to
writeLog) which duplicates debug output and violates the guideline; remove the
logPath/timestamp variables, delete the nested writeLog(_:) helper, and remove
all writeLog(...) invocations so that only dlog(...) under `#if` DEBUG remains for
debug events; keep the guard/hasUnread logic and ensure no other references to
writeLog or logPath remain in dismissNotificationIfPresent.
- Around line 3405-3434: The code currently uses AppDelegate.shared?.tabManager
(the active manager) to find the workspace to call
triggerNotificationFocusFlash, which fails for secondary windows; change the
lookup to find the tab's owning TabManager (the manager whose tabs contain
tabId) and use that manager to find the workspace and call
triggerNotificationFocusFlash before calling notificationStore.markRead.
Concretely: replace the workspace lookup that uses
AppDelegate.shared?.tabManager with a lookup that searches all TabManagers (or
an AppDelegate.shared?.tabManagers collection) for the manager where
manager.tabs contains the tabId, then call manager.tabs.first(where: { $0.id ==
tabId }).triggerNotificationFocusFlash(panelId: surfaceId, requiresSplit:false,
shouldFocus:false); keep using notificationStore.markRead(forTabId:surfaceId)
afterwards.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c3914d8-b958-463c-b3c8-7c43fff4158b
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftSources/TerminalNotificationStore.swift
Addresses Greptile feedback - previously markRead was called unconditionally even when workspace lookup failed, which would silently dismiss without showing the flash animation.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Sources/GhosttyTerminalView.swift (2)
3384-3398:⚠️ Potential issue | 🟡 MinorRemove the bespoke
/tmplogger from this hot path.Lines 3384-3398 add a second debug sink on every key/mouse-triggered dismissal path. That duplicates
dlog()and keeps synchronous file I/O in a latency-sensitive handler.Suggested cleanup
- let logPath = "/tmp/cmux-notif-dismiss-debug.log" - let timestamp = Date().timeIntervalSince1970 - - func writeLog(_ message: String) { - let logMessage = "[\(timestamp)] \(message)\n" - if let data = logMessage.data(using: .utf8) { - if let fileHandle = FileHandle(forWritingAtPath: logPath) { - fileHandle.seekToEndOfFile() - fileHandle.write(data) - fileHandle.closeFile() - } else { - try? data.write(to: URL(fileURLWithPath: logPath), options: .atomic) - } - } - } - `#if` DEBUG dlog("dismissNotificationIfPresent: checking tabId=\(tabId?.uuidString.prefix(8) ?? "nil") surfaceId=\(terminalSurface?.id.uuidString.prefix(8) ?? "nil")") - writeLog("dismissNotificationIfPresent: checking tabId=\(tabId?.uuidString.prefix(8) ?? "nil") surfaceId=\(terminalSurface?.id.uuidString.prefix(8) ?? "nil")") `#endif` @@ `#if` DEBUG dlog("dismissNotificationIfPresent: early return - missing required objects") - writeLog("dismissNotificationIfPresent: early return - missing required objects tabId=\(tabId == nil ? "nil" : "ok") surfaceId=\(terminalSurface?.id == nil ? "nil" : "ok") store=\(AppDelegate.shared?.notificationStore == nil ? "nil" : "ok") manager=\(AppDelegate.shared?.tabManager == nil ? "nil" : "ok")") `#endif` @@ `#if` DEBUG dlog("dismissNotificationIfPresent: hasUnread=\(hasUnread) for tab=\(tabId.uuidString.prefix(8)) surface=\(surfaceId.uuidString.prefix(8))") - writeLog("dismissNotificationIfPresent: hasUnread=\(hasUnread) for tab=\(tabId.uuidString.prefix(8)) surface=\(surfaceId.uuidString.prefix(8))") `#endif` @@ `#if` DEBUG dlog("dismissNotificationIfPresent: dismissing notification and triggering flash") - writeLog("dismissNotificationIfPresent: dismissing notification and triggering flash") `#endif` @@ `#if` DEBUG dlog("dismissNotificationIfPresent: workspace not found, skipping dismissal") - writeLog("dismissNotificationIfPresent: workspace not found for tab=\(tabId.uuidString.prefix(8))") `#endif`As per coding guidelines,
**/*.swift: "All debug events (keys, mouse, focus, splits, tabs) must be logged to the unified debug event log in DEBUG builds using thedlog()free function".Also applies to: 3400-3429
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 3384 - 3398, The new synchronous /tmp logger (logPath, timestamp and writeLog(_:) using FileHandle/data write) introduces duplicate logging and blocking I/O on the key/mouse dismissal hot path; remove the writeLog function and any calls to it (also in the nearby block around lines referenced) and replace them with calls to the unified DEBUG-only dlog(...) free function so events are only logged via the standardized debug sink and avoid synchronous file I/O.
3405-3440:⚠️ Potential issue | 🟠 MajorUse the owning tab manager for the workspace lookup.
Line 3408 still pulls
AppDelegate.shared?.tabManager, i.e. the active manager. In a secondary window,tabs.first(where: { $0.id == tabId })can miss the current workspace, so the new guard returns early and the notification never dismisses for that terminal.Suggested fix
- guard let tabId, - let surfaceId = terminalSurface?.id, - let notificationStore = AppDelegate.shared?.notificationStore, - let tabManager = AppDelegate.shared?.tabManager else { + guard let tabId, + let surfaceId = terminalSurface?.id, + let app = AppDelegate.shared, + let notificationStore = app.notificationStore else { `#if` DEBUG dlog("dismissNotificationIfPresent: early return - missing required objects") `#endif` return } @@ - guard let workspace = tabManager.tabs.first(where: { $0.id == tabId }) else { + guard let tabManager = app.tabManagerFor(tabId: tabId) ?? app.tabManager, + let workspace = tabManager.tabs.first(where: { $0.id == tabId }) else { `#if` DEBUG dlog("dismissNotificationIfPresent: workspace not found, skipping dismissal") `#endif` return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 3405 - 3440, The workspace lookup currently uses AppDelegate.shared?.tabManager (captured as tabManager) which can point to the active window's manager; instead, obtain the owning tab manager from terminalSurface and use that for the lookup. Replace the workspace guard with something like: guard let owningTabManager = terminalSurface?.owningTabManager, let workspace = owningTabManager.tabs.first(where: { $0.id == tabId }) else { ... } and keep calling workspace.triggerNotificationFocusFlash(...) and notificationStore.markRead(...); reference symbols: terminalSurface, owningTabManager, tabManager (old), workspace, triggerNotificationFocusFlash, markRead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 3384-3398: The new synchronous /tmp logger (logPath, timestamp and
writeLog(_:) using FileHandle/data write) introduces duplicate logging and
blocking I/O on the key/mouse dismissal hot path; remove the writeLog function
and any calls to it (also in the nearby block around lines referenced) and
replace them with calls to the unified DEBUG-only dlog(...) free function so
events are only logged via the standardized debug sink and avoid synchronous
file I/O.
- Around line 3405-3440: The workspace lookup currently uses
AppDelegate.shared?.tabManager (captured as tabManager) which can point to the
active window's manager; instead, obtain the owning tab manager from
terminalSurface and use that for the lookup. Replace the workspace guard with
something like: guard let owningTabManager = terminalSurface?.owningTabManager,
let workspace = owningTabManager.tabs.first(where: { $0.id == tabId }) else {
... } and keep calling workspace.triggerNotificationFocusFlash(...) and
notificationStore.markRead(...); reference symbols: terminalSurface,
owningTabManager, tabManager (old), workspace, triggerNotificationFocusFlash,
markRead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d06c452-8e36-4755-8ec0-0ff503c27cda
📒 Files selected for processing (1)
Sources/GhosttyTerminalView.swift
…support, add right/middle click - Removed bespoke /tmp file logger that duplicated dlog and added sync I/O - Fixed workspace lookup to use tabManagerFor(tabId:) instead of active window's tabManager - Added dismissNotificationIfPresent() to rightMouseDown and otherMouseDown for complete mouse button coverage Addresses feedback from cubic and CodeRabbit reviews.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b51f3d89e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard let tabId, | ||
| let surfaceId = terminalSurface?.id, | ||
| let app = AppDelegate.shared, |
There was a problem hiding this comment.
dismissNotificationIfPresent() only checks unread state for the current concrete surfaceId, because it guards on terminalSurface?.id and then calls hasUnreadNotification(forTabId:surfaceId:) with that value. After this commit removed the focused auto-dismiss branch in addNotification (which previously treated surfaceId == nil as focused), notifications created at tab scope (surfaceId == nil) can remain unread even after clicks/keystrokes in the focused terminal, so the unread badge can stick until a manual mark-read action.
Useful? React with 👍 / 👎.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Implementation
Previously, notifications sent to an already-focused terminal were silently dropped (TerminalNotificationStore.swift:836-843). This prevented the blue ring from appearing in the edge case where a notification arrives for the currently focused terminal.
Now notifications always appear with the blue ring and are dismissed on the next user interaction (click/keystroke), with the flash animation for visual feedback.
Changes:
dismissNotificationIfPresent()method to GhosttyTerminalView that checks for unread notifications and triggers dismissal with flashmouseDown()andkeyDown()event handlersTerminalNotificationStore.addNotification()Testing
/tmp/cmux-notif-dismiss-debug.logduring developmentRelated
Summary by CodeRabbit