Repository navigation
Improve tmux notification attention routing - #1898
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors attention/flash coordination and panel APIs, adds focused-read indicator and tmux-pane overlay rendering, centralizes flash decisioning, refines CLI Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as cmux CLI
participant Resolver as CLI Resolver
participant Env as Environment
participant Debug as debug.terminals
participant Server as JSON-RPC Server
CLI->>Resolver: parse command (+ --workspace/--surface)
Resolver->>Env: check explicit args & CMUX_* env
alt explicit --workspace or --surface present
Resolver-->>Server: send command with explicit IDs
else
Env-->>Resolver: CMUX_WORKSPACE_ID / CMUX_SURFACE_ID (if set)
alt env IDs present
Resolver-->>Server: send command with env IDs
else
Resolver->>Debug: resolve caller TTY → candidate workspace/surface
Debug-->>Resolver: candidate IDs (validated exist)
Resolver-->>Server: send command with caller-TTY-derived IDs
end
end
Server-->>CLI: "OK\n"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 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 improves tmux notification attention routing across three areas: (1) CLI Key issues found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as cmux CLI
participant Socket as SocketClient
participant Store as TerminalNotificationStore
participant TM as TabManager
participant WS as Workspace
participant Panel as TerminalPanel
participant Overlay as TmuxWorkspacePaneOverlay
Note over CLI: notify / trigger-flash with stale env IDs
CLI->>Socket: surface.list(stale_workspace_id) → not_found
CLI->>Socket: debug.terminals() → match by TTY
CLI->>Socket: surface.list(resolved_workspace_id) → validate
CLI->>Socket: notify_target / surface.trigger_flash (current IDs)
Note over Store,TM: Notification arrives while panel is focused
Store->>Store: addNotification()<br/>(shouldSuppressExternalDelivery=true)
Store->>Store: setFocusedReadIndicator(tabId, surfaceId)
Note over TM: Surface gains focus
TM->>Store: setFocusedReadIndicator(tabId, panelId)
TM->>Store: markRead(tabId, panelId)
Store-->>Store: ⚠ clearFocusedReadIndicator(tabId, panelId)<br/>(undoes indicator)
Note over TM: User dismisses notification explicitly
TM->>WS: triggerNotificationDismissFlash(panelId)
WS->>Panel: triggerFlash(reason: .notificationDismiss)
Panel->>WS: onRequestWorkspacePaneFlash(.notificationDismiss)
WS->>WS: triggerWorkspacePaneFlash(panelId, reason)
WS-->>Overlay: tmuxWorkspaceFlashToken++, flashReason set
TM->>Store: clearFocusedReadIndicator(tabId, panelId)
Note over Overlay: ContentView render loop
Overlay->>Overlay: TmuxWorkspacePaneOverlayModel.apply(state)
Overlay->>Overlay: FocusFlashPattern.opacity(at: elapsed)
Overlay->>Overlay: drawFlashRing / drawUnreadRing
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| guard !idsToClear.isEmpty else { return } | ||
| notifications = updated | ||
| clearFocusedReadIndicator(forTabId: tabId, surfaceId: surfaceId) | ||
| center.removeDeliveredNotificationsOffMain(withIdentifiers: idsToClear) |
There was a problem hiding this comment.
markRead clears the focused-read indicator, defeating its purpose
markRead(forTabId:surfaceId:) now calls clearFocusedReadIndicator(forTabId: tabId, surfaceId: surfaceId) whenever it finds unread notifications to clear. This directly contradicts the design intent: the focusedReadIndicator is supposed to keep the ring visible after a notification is read (e.g. when the user was already looking at the pane when the notification arrived).
The sequence in TabManager.handleSurfaceFocus exposes the problem clearly:
notificationStore.setFocusedReadIndicator(forTabId: tabId, surfaceId: panelId) // sets indicator
notificationStore.markRead(forTabId: tabId, surfaceId: panelId) // immediately clears itWhen the pane has unread notifications, markRead finds idsToClear non-empty and calls clearFocusedReadIndicator with the same surfaceId, removing the entry that was just set. The net result is that the indicator never persists.
This will also cause testFocusedNotificationIndicatorRemainsVisibleAfterFocusedNotificationIsRead to fail: that test adds a suppressed notification (which calls setFocusedReadIndicator inside addNotification), asserts hasUnreadNotification is true, then calls store.markRead and asserts hasVisibleNotificationIndicator is still true — but markRead will have cleared it.
The clearFocusedReadIndicator call should be removed from markRead. The indicator should only be cleared through the explicit dismissal paths (dismissNotificationOnDirectInteraction, remove(id:), clearAll, and the competing-surface guard in addNotification).
| if let tab = tabs.first(where: { $0.id == tabId }) { | ||
| tab.triggerNotificationFocusFlash(panelId: panelId, requiresSplit: false, shouldFocus: false) | ||
| } | ||
| notificationStore.setFocusedReadIndicator(forTabId: tabId, surfaceId: panelId) |
There was a problem hiding this comment.
setFocusedReadIndicator on every focus event
setFocusedReadIndicator is called unconditionally on every surface-focus event, even when the pane has no notification at all. When there are no unread notifications, markRead hits its guard !idsToClear.isEmpty else { return } early-exit without calling clearFocusedReadIndicator, so the indicator is left set. Any recently focused pane — even one that never had a notification — would then have hasVisibleNotificationIndicator return true and would display an unread ring.
This should be guarded so the indicator is only set when the pane actually has a visible notification:
if notificationStore.hasVisibleNotificationIndicator(forTabId: tabId, surfaceId: panelId) {
notificationStore.setFocusedReadIndicator(forTabId: tabId, surfaceId: panelId)
}
notificationStore.markRead(forTabId: tabId, surfaceId: panelId)![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: 656786fb71
ℹ️ 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".
| if let callerWorkspaceId = resolveCallerWorkspaceIdByTTY(client: client), | ||
| (try? client.sendV2(method: "surface.list", params: ["workspace_id": callerWorkspaceId])) != nil { | ||
| return callerWorkspaceId |
There was a problem hiding this comment.
resolveWorkspaceIdAllowingFallback now prefers resolveCallerWorkspaceIdByTTY whenever the workspace arg is omitted, but notify/trigger-flash intentionally omit env fallback when --window is used and rely on window.focus + current workspace routing. With this change, cmux --window <other> notify (or trigger-flash) can still resolve to the caller terminal’s workspace instead of the targeted window’s workspace, so attention events are sent to the wrong pane/window. The TTY fallback should be skipped when a window override is active.
Useful? React with 👍 / 👎.
![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 17 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="Sources/TerminalNotificationStore.swift">
<violation number="1" location="Sources/TerminalNotificationStore.swift:882">
P1: Optional equality here can return a false positive when `surfaceId` is `nil` (`nil == nil`), causing tabs to appear to have a visible indicator even when none exists.</violation>
</file>
<file name="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:10546">
P1: Caller-TTY workspace fallback can override `--window` targeting and route notifications/flashes to the wrong workspace.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| func hasVisibleNotificationIndicator(forTabId tabId: UUID, surfaceId: UUID?) -> Bool { | ||
| hasUnreadNotification(forTabId: tabId, surfaceId: surfaceId) || | ||
| focusedReadIndicatorByTabId[tabId] == surfaceId |
There was a problem hiding this comment.
P1: Optional equality here can return a false positive when surfaceId is nil (nil == nil), causing tabs to appear to have a visible indicator even when none exists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/TerminalNotificationStore.swift, line 882:
<comment>Optional equality here can return a false positive when `surfaceId` is `nil` (`nil == nil`), causing tabs to appear to have a visible indicator even when none exists.</comment>
<file context>
@@ -876,10 +877,19 @@ final class TerminalNotificationStore: ObservableObject {
+ func hasVisibleNotificationIndicator(forTabId tabId: UUID, surfaceId: UUID?) -> Bool {
+ hasUnreadNotification(forTabId: tabId, surfaceId: surfaceId) ||
+ focusedReadIndicatorByTabId[tabId] == surfaceId
+ }
+
</file context>
| focusedReadIndicatorByTabId[tabId] == surfaceId | |
| (surfaceId != nil && focusedReadIndicatorByTabId[tabId] == surfaceId) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Workspace.swift (1)
8232-8250:⚠️ Potential issue | 🟡 MinorAvoid double-firing the navigation flash for manually unread panes.
When focus lands on a manually unread pane,
applyTabSelectionNow(...)already callstriggerFocusFlash(panelId:)before clearing unread state. The new block at Line 8248 fires the same flash again, so one navigation step bumpstmuxWorkspaceFlashTokentwice and restarts the animation.Suggested fix
- if let focusedPanelId, focusedPanelId != previousFocusedPanelId { + if let focusedPanelId, + focusedPanelId != previousFocusedPanelId, + !manualUnreadPanelIds.contains(focusedPanelId) { triggerFocusFlash(panelId: focusedPanelId) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 8232 - 8250, The navigation code double-triggers the focus flash because applyTabSelectionNow(...) may call triggerFocusFlash(panelId:) for manually-unread panes and the new post-navigation block also always calls triggerFocusFlash when focusedPanelId changed; modify applyTabSelection(tabId:inPane:) / applyTabSelectionNow(...) to indicate (return Bool or accept a suppressFlash flag) whether it already triggered the flash (or to suppress its own flash), then update this caller to only call triggerFocusFlash(panelId:) when that indicator is false (i.e., when applyTabSelection did not already fire the flash), using symbols bonsplitController.focusedPaneId, applyTabSelection(tabId:inPane:), applyTabSelectionNow(...), and triggerFocusFlash(panelId:) to locate the changes.
🧹 Nitpick comments (4)
Sources/GhosttyTerminalView.swift (2)
8931-8931:updateOverlayRingPathno longer uses itsinsetparameter.Line 8931 hardcodes
PanelOverlayRingMetrics.pathRect(in:), so theinsetargument in the helper signature is now misleading. Either remove the unused parameter or restore parameter-driven path construction.♻️ Proposed cleanup (remove unused parameter)
- private func updateOverlayRingPath( - layer: CAShapeLayer, - bounds: CGRect, - inset: CGFloat, - radius: CGFloat - ) { + private func updateOverlayRingPath( + layer: CAShapeLayer, + bounds: CGRect, + radius: CGFloat + ) { layer.frame = bounds - guard bounds.width > inset * 2, bounds.height > inset * 2 else { + let inset = PanelOverlayRingMetrics.inset + guard bounds.width > inset * 2, bounds.height > inset * 2 else { layer.path = nil return } let rect = PanelOverlayRingMetrics.pathRect(in: bounds) layer.path = CGPath(roundedRect: rect, cornerWidth: radius, cornerHeight: radius, transform: nil) }updateOverlayRingPath( layer: notificationRingLayer, bounds: notificationRingOverlayView.bounds, - inset: NotificationRingMetrics.inset, radius: NotificationRingMetrics.cornerRadius )updateOverlayRingPath( layer: flashLayer, bounds: flashOverlayView.bounds, - inset: inset, radius: radius )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` at line 8931, The helper updateOverlayRingPath currently ignores its inset parameter and always calls PanelOverlayRingMetrics.pathRect(in: bounds); remove the unused inset parameter from updateOverlayRingPath's signature and all callers (or, if you prefer to keep inset semantics, modify updateOverlayRingPath to compute the rect using the inset and pass it into PanelOverlayRingMetrics.pathRect), and update any references to updateOverlayRingPath accordingly so there are no unused parameters; specifically edit the updateOverlayRingPath declaration and its invocations and ensure PanelOverlayRingMetrics.pathRect(in:) is used consistently with the chosen approach.
6782-6789: Cache the navigation presentation once in init.
WorkspaceAttentionCoordinator.flashStyle(for: .navigation)is called three times back-to-back. Caching it once keeps setup tighter and avoids duplicate lookups.♻️ Proposed refactor
- flashLayer.strokeColor = WorkspaceAttentionCoordinator.flashStyle(for: .navigation).accent.strokeColor.cgColor + let navigationPresentation = WorkspaceAttentionCoordinator.flashStyle(for: .navigation) + flashLayer.strokeColor = navigationPresentation.accent.strokeColor.cgColor flashLayer.lineWidth = NotificationRingMetrics.lineWidth flashLayer.lineJoin = .round flashLayer.lineCap = .round - flashLayer.shadowColor = WorkspaceAttentionCoordinator.flashStyle(for: .navigation).accent.strokeColor.cgColor - flashLayer.shadowOpacity = Float(WorkspaceAttentionCoordinator.flashStyle(for: .navigation).glowOpacity) - flashLayer.shadowRadius = WorkspaceAttentionCoordinator.flashStyle(for: .navigation).glowRadius + flashLayer.shadowColor = navigationPresentation.accent.strokeColor.cgColor + flashLayer.shadowOpacity = Float(navigationPresentation.glowOpacity) + flashLayer.shadowRadius = navigationPresentation.glowRadius🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 6782 - 6789, Cache the result of WorkspaceAttentionCoordinator.flashStyle(for: .navigation) once (e.g. in the initializer or a stored property like navigationFlashStyle) and reuse it when configuring flashLayer instead of calling WorkspaceAttentionCoordinator.flashStyle(for: .navigation) three times; update the flashLayer configuration in the block that sets strokeColor, shadowColor, shadowOpacity, and shadowRadius to read from the cached navigationFlashStyle.accent.strokeColor and navigationFlashStyle.glowOpacity/glowRadius to avoid duplicate lookups.Sources/Panels/Panel.swift (2)
81-97: Avoid a second reason→style source of truth.
WorkspaceAttentionCoordinator.flashStyle(for:)now encodes the same reason-to-style policy that terminal surfaces already derive throughGhosttySurfaceScrollView.flashStyle(for:)viaSources/Panels/TerminalPanel.swift:1-20. Keeping those mappings separate makes tmux whole-pane overlays and surface flashes easy to desync on the next style tweak. I’d pull the mapping behind a shared helper and have both paths consume it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/Panel.swift` around lines 81 - 97, The reason→style mapping is duplicated between WorkspaceAttentionCoordinator.flashStyle(for:) and GhosttySurfaceScrollView.flashStyle(for:); extract a single shared helper (for example a static function or computed property like WorkspaceAttentionFlashStyle.mapping(for:) or a shared function flashPresentation(for:)) that returns WorkspaceAttentionFlashPresentation for a WorkspaceAttentionFlashReason, replace both WorkspaceAttentionCoordinator.flashStyle(for:) and GhosttySurfaceScrollView.flashStyle(for:) to call that helper, and remove the hardcoded switch in each location so both consumers use the single source of truth.
99-109: Consider removing the zero-argtriggerFlash()shim once it is confirmed unused.The migration appears complete—all existing call sites (Workspace.swift, TerminalPanel.swift, MarkdownPanel.swift, BrowserPanel.swift) already use the parametrized
triggerFlash(reason:)form with explicit reasons. The zero-arg compatibility layer at lines 307–309 is currently unused. Removing it would prevent accidental future call sites from silently defaulting to.navigationand would enforce compile-time migration discipline. However, this can be deferred if the shim is intentionally kept for backward-compatibility with plugin or extension code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/Panel.swift` around lines 99 - 109, The code contains an unused zero-argument compatibility shim triggerFlash() that silently defaults to .navigation; remove the zero-arg triggerFlash() function (and its declaration at lines ~307–309) so callers must use triggerFlash(reason:), search for any remaining call sites (e.g., Workspace.swift, TerminalPanel.swift, MarkdownPanel.swift, BrowserPanel.swift) and update them if found, run the build and tests to ensure no external plugins/extensions rely on the removed shim, and if backward-compatibility is required instead add a deprecation comment or keep the shim but mark it deprecated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 10528-10534: A call site is still invoking resolveSurfaceId(...)
directly for session-end events, bypassing the new TTY fallback; update that
handler to call resolveSurfaceIdForClaudeHook(_ raw: String?, workspaceId:
String, client: SocketClient) instead of resolveSurfaceId so the session-end
path goes through resolveSurfaceIdAllowingFallback. Locate the code that
resolves surfaces for "session-end" events and replace the direct
resolveSurfaceId(...) invocation with resolveSurfaceIdForClaudeHook(...),
ensuring you pass the same raw, workspaceId and client arguments and propagate
any throws or errors as the original call did.
- Around line 1866-1892: The code currently lets
resolveWorkspaceIdAllowingFallback/resolveSurfaceIdAllowingFallback fall back to
the caller TTY even when an explicit window (windowId) was provided; change the
calls so that when windowId != nil (or explicitWorkspaceArg/explicitSurfaceArg
is non-nil) you call the non-fallback behavior (or pass a flag disabling TTY
fallback) instead of resolve*AllowingFallback; specifically, in the wsId and
sfId closure blocks replace the fallback-allowing helper with the variant that
does not use the CMUX_WORKSPACE_ID/CMUX_SURFACE_ID environment (or add and pass
a disableTTYFallback parameter to
resolveWorkspaceIdAllowingFallback/resolveSurfaceIdAllowingFallback), and ensure
normalizeWorkspaceHandle/normalizeSurfaceHandle remain used when
explicitWorkspaceArg/explicitSurfaceArg are present so explicit values still
normalize correctly.
In `@cmuxTests/WorkspaceRemoteConnectionTests.swift`:
- Around line 556-569: On timeout in the run-and-wait block (where
exitSignal.wait, process.terminate() and the 1s follow-up wait occur) add a
SIGKILL fallback: after calling process.terminate() and waiting the extra
second, check if the Process is still running (e.g., process.isRunning or
checking process.processIdentifier) and, if so, send a SIGKILL (kill(pid,
SIGKILL)) and wait again for termination before proceeding to
readDataToEndOfFile() from stdoutPipe/stderrPipe; update the logic around
exitSignal, process.terminate(), and the subsequent wait so the code guarantees
the child has exited (or been killed) before calling readDataToEndOfFile().
- Around line 492-496: bindUnixSocket silently truncates long UNIX socket paths
via strncpy, causing mismatches with callers using the full path; add an upfront
length check in bindUnixSocket (the same validation used by
TerminalController.unixSocketAddress()) to verify the provided path fits within
sun_path (104 bytes) and fail fast (throw or return nil) when it does not.
Locate bindUnixSocket and before any strncpy/use of sun_path, compute the byte
length of the path (UTF-8) and compare to the sun_path limit (104); if it
exceeds the limit, return an error/optional nil so callers like makeSocketPath
or tests can handle the failure rather than relying on truncated bind behavior.
Ensure the error path is documented/propagated to tests and remove reliance on
silent truncation by strncpy.
In `@Sources/ContentView.swift`:
- Around line 1778-1784: The current containment test (exactFitsWithinPane)
treats any rect fully inside paneRect as a match; replace it with a
near-equality edge check so only rects whose edges are within tolerance are
considered equal. Update the boolean to compare edges (e.g., abs(exactRect.minX
- paneRect.minX) <= tolerance, abs(exactRect.maxX - paneRect.maxX) <= tolerance,
abs(exactRect.minY - paneRect.minY) <= tolerance, abs(exactRect.maxY -
paneRect.maxY) <= tolerance) and use that new check in place of
exactFitsWithinPane in the return decision (look for the variables tolerance,
exactFitsWithinPane, exactRect, paneRect in ContentView.swift).
In `@Sources/TerminalNotificationStore.swift`:
- Around line 880-883: The helper
hasVisibleNotificationIndicator(forTabId:surfaceId:) erroneously returns true
when both the stored focusedReadIndicatorByTabId[tabId] and the passed surfaceId
are nil (nil == nil). Fix it by only comparing the stored value to surfaceId if
a stored value exists; e.g. replace the current boolean expression with a check
that focusedReadIndicatorByTabId[tabId] is non-nil and equals surfaceId (or use
if let focused = focusedReadIndicatorByTabId[tabId] { return focused ==
surfaceId } else { return false }), combined with the existing
hasUnreadNotification(...) OR.
- Around line 1006-1010: The current
clearFocusedReadIndicator(forTabId:surfaceId:) treats surfaceId == nil as “clear
any indicator in the tab”, which is too broad; change it so the function only
removes the indicator when the provided surfaceId exactly matches the stored
surface id (i.e., if surfaceId is nil, do not clear anything unless
existingSurfaceId is also nil). Add a new helper
clearFocusedReadIndicatorForTab(tabId:) (or a boolean parameter like
forceClearTab) that unconditionally removes the entry for a tab and call that
helper only from clearNotifications(forTabId:), leaving remove(id:) and
clearNotifications(forTabId:surfaceId:) to call the surface-specific
clearFocusedReadIndicator(forTabId:surfaceId:).
In `@Sources/Workspace.swift`:
- Around line 5616-5620: The onRequestWorkspacePaneFlash callback is only set in
configureTerminalPanel(_ terminalPanel: TerminalPanel) when a panel is first
created, so reusing a TerminalPanel in attachDetachedSurface(...) leaves the old
callback bound to the previous workspace; rebind the callback whenever a panel
is reattached by either calling configureTerminalPanel(_:) from
attachDetachedSurface(...) after updating the panel's workspace ID or explicitly
resetting terminalPanel.onRequestWorkspacePaneFlash there to the closure that
calls triggerWorkspacePaneFlash(panelId:reason:). Ensure you reference
TerminalPanel, onRequestWorkspacePaneFlash, attachDetachedSurface(...),
configureTerminalPanel(_ terminalPanel: TerminalPanel), and
triggerWorkspacePaneFlash(panelId:reason:) so the callback is always bound to
the current workspace.
In `@Sources/WorkspaceContentView.swift`:
- Around line 390-393: shouldShow can become stale because it depends on
notificationStore.hasVisibleNotificationIndicator(…) but the view only syncs to
notificationStore.notifications and workspace.manualUnreadPanelIds; add a sync
hook so the view also observes the focused-read state that the focused-read
mutators change (e.g. subscribe to notificationStore.focusedReadState or
workspace.focusedReadPanelIds or whatever property the mutators update) and
trigger a view refresh whenever that focused-read property changes so shouldShow
(and tab.showsNotificationBadge) are recomputed immediately.
- Around line 124-129: The code currently advances self.lastFlashToken before a
flashRect exists, causing a token published during layout recovery to be treated
as already handled; modify the logic so you only consume (assign)
state.flashToken into lastFlashToken when there is a valid rect or when you
actually start the flash: i.e. move the self.lastFlashToken = state.flashToken
assignment inside the branch that checks state.flashRect != nil (or inside the
same branch where you set flashStartedAt) so that tokens published while
state.flashRect is nil are not marked as handled.
---
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 8232-8250: The navigation code double-triggers the focus flash
because applyTabSelectionNow(...) may call triggerFocusFlash(panelId:) for
manually-unread panes and the new post-navigation block also always calls
triggerFocusFlash when focusedPanelId changed; modify
applyTabSelection(tabId:inPane:) / applyTabSelectionNow(...) to indicate (return
Bool or accept a suppressFlash flag) whether it already triggered the flash (or
to suppress its own flash), then update this caller to only call
triggerFocusFlash(panelId:) when that indicator is false (i.e., when
applyTabSelection did not already fire the flash), using symbols
bonsplitController.focusedPaneId, applyTabSelection(tabId:inPane:),
applyTabSelectionNow(...), and triggerFocusFlash(panelId:) to locate the
changes.
---
Nitpick comments:
In `@Sources/GhosttyTerminalView.swift`:
- Line 8931: The helper updateOverlayRingPath currently ignores its inset
parameter and always calls PanelOverlayRingMetrics.pathRect(in: bounds); remove
the unused inset parameter from updateOverlayRingPath's signature and all
callers (or, if you prefer to keep inset semantics, modify updateOverlayRingPath
to compute the rect using the inset and pass it into
PanelOverlayRingMetrics.pathRect), and update any references to
updateOverlayRingPath accordingly so there are no unused parameters;
specifically edit the updateOverlayRingPath declaration and its invocations and
ensure PanelOverlayRingMetrics.pathRect(in:) is used consistently with the
chosen approach.
- Around line 6782-6789: Cache the result of
WorkspaceAttentionCoordinator.flashStyle(for: .navigation) once (e.g. in the
initializer or a stored property like navigationFlashStyle) and reuse it when
configuring flashLayer instead of calling
WorkspaceAttentionCoordinator.flashStyle(for: .navigation) three times; update
the flashLayer configuration in the block that sets strokeColor, shadowColor,
shadowOpacity, and shadowRadius to read from the cached
navigationFlashStyle.accent.strokeColor and
navigationFlashStyle.glowOpacity/glowRadius to avoid duplicate lookups.
In `@Sources/Panels/Panel.swift`:
- Around line 81-97: The reason→style mapping is duplicated between
WorkspaceAttentionCoordinator.flashStyle(for:) and
GhosttySurfaceScrollView.flashStyle(for:); extract a single shared helper (for
example a static function or computed property like
WorkspaceAttentionFlashStyle.mapping(for:) or a shared function
flashPresentation(for:)) that returns WorkspaceAttentionFlashPresentation for a
WorkspaceAttentionFlashReason, replace both
WorkspaceAttentionCoordinator.flashStyle(for:) and
GhosttySurfaceScrollView.flashStyle(for:) to call that helper, and remove the
hardcoded switch in each location so both consumers use the single source of
truth.
- Around line 99-109: The code contains an unused zero-argument compatibility
shim triggerFlash() that silently defaults to .navigation; remove the zero-arg
triggerFlash() function (and its declaration at lines ~307–309) so callers must
use triggerFlash(reason:), search for any remaining call sites (e.g.,
Workspace.swift, TerminalPanel.swift, MarkdownPanel.swift, BrowserPanel.swift)
and update them if found, run the build and tests to ensure no external
plugins/extensions rely on the removed shim, and if backward-compatibility is
required instead add a deprecation comment or keep the shim but mark it
deprecated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09627778-9a8c-4bb2-87a9-047ea1d4ae9c
📒 Files selected for processing (17)
CLI/cmux.swiftSources/ContentView.swiftSources/GhosttyTerminalView.swiftSources/Panels/BrowserPanel.swiftSources/Panels/MarkdownPanel.swiftSources/Panels/Panel.swiftSources/Panels/TerminalPanel.swiftSources/TabManager.swiftSources/TerminalNotificationStore.swiftSources/Workspace.swiftSources/WorkspaceContentView.swiftcmuxTests/NotificationAndMenuBarTests.swiftcmuxTests/TabManagerUnitTests.swiftcmuxTests/WindowAndDragTests.swiftcmuxTests/WorkspaceContentViewVisibilityTests.swiftcmuxTests/WorkspaceRemoteConnectionTests.swiftcmuxTests/WorkspaceUnitTests.swift
| let explicitWorkspaceArg = tfWsFlag | ||
| let callerWorkspaceArg = windowId == nil ? ProcessInfo.processInfo.environment["CMUX_WORKSPACE_ID"] : nil | ||
| let workspaceArg = explicitWorkspaceArg ?? callerWorkspaceArg | ||
| let explicitSurfaceArg = optionValue(commandArgs, name: "--surface") ?? optionValue(commandArgs, name: "--panel") | ||
| let callerSurfaceArg = explicitWorkspaceArg == nil && windowId == nil | ||
| ? ProcessInfo.processInfo.environment["CMUX_SURFACE_ID"] | ||
| : nil | ||
| let surfaceArg = explicitSurfaceArg ?? callerSurfaceArg | ||
| var params: [String: Any] = [:] | ||
| let wsId = try normalizeWorkspaceHandle(workspaceArg, client: client) | ||
| let wsId = try { | ||
| if explicitWorkspaceArg != nil { | ||
| return try normalizeWorkspaceHandle(workspaceArg, client: client) | ||
| } | ||
| return try resolveWorkspaceIdAllowingFallback(workspaceArg, client: client) | ||
| }() | ||
| if let wsId { params["workspace_id"] = wsId } | ||
| let sfId = try normalizeSurfaceHandle(surfaceArg, client: client, workspaceHandle: wsId) | ||
| let sfId = try { | ||
| if explicitSurfaceArg != nil { | ||
| return try normalizeSurfaceHandle(surfaceArg, client: client, workspaceHandle: wsId) | ||
| } | ||
| guard let wsId else { return nil } | ||
| return try resolveSurfaceIdAllowingFallback( | ||
| surfaceArg, | ||
| workspaceId: wsId, | ||
| client: client | ||
| ) | ||
| }() |
There was a problem hiding this comment.
Don't let --window fall back to the caller TTY.
These branches correctly clear env-derived caller IDs when windowId != nil, but then they call resolveWorkspaceIdAllowingFallback(nil, ...), which reintroduces caller-TTY routing. cmux --window ... notify / trigger-flash can therefore target the originating workspace instead of the selected workspace in the explicit window.
Suggested fix
Apply the same change in both call sites, and gate the helper's TTY fallback behind a flag.
- return try resolveWorkspaceIdAllowingFallback(workspaceArg, client: client)
+ return try resolveWorkspaceIdAllowingFallback(
+ workspaceArg,
+ client: client,
+ allowCallerTTYFallback: windowId == nil
+ )Also applies to: 2086-2110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 1866 - 1892, The code currently lets
resolveWorkspaceIdAllowingFallback/resolveSurfaceIdAllowingFallback fall back to
the caller TTY even when an explicit window (windowId) was provided; change the
calls so that when windowId != nil (or explicitWorkspaceArg/explicitSurfaceArg
is non-nil) you call the non-fallback behavior (or pass a flag disabling TTY
fallback) instead of resolve*AllowingFallback; specifically, in the wsId and
sfId closure blocks replace the fallback-allowing helper with the variant that
does not use the CMUX_WORKSPACE_ID/CMUX_SURFACE_ID environment (or add and pass
a disableTTYFallback parameter to
resolveWorkspaceIdAllowingFallback/resolveSurfaceIdAllowingFallback), and ensure
normalizeWorkspaceHandle/normalizeSurfaceHandle remain used when
explicitWorkspaceArg/explicitSurfaceArg are present so explicit values still
normalize correctly.
| private func resolveSurfaceIdForClaudeHook( | ||
| _ raw: String?, | ||
| workspaceId: String, | ||
| client: SocketClient | ||
| ) throws -> String { | ||
| if let raw, !raw.isEmpty, let candidate = try? resolveSurfaceId(raw, workspaceId: workspaceId, client: client) { | ||
| try resolveSurfaceIdAllowingFallback(raw, workspaceId: workspaceId, client: client) | ||
| } |
There was a problem hiding this comment.
Route the session-end fallback through resolveSurfaceIdForClaudeHook.
The new TTY fallback only helps call sites that use this helper. Line 10171 still calls resolveSurfaceId(...) directly, so a session-end without session_id can still consume the focused surface's session instead of the caller surface when multiple Claude panes share a workspace.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 10528 - 10534, A call site is still invoking
resolveSurfaceId(...) directly for session-end events, bypassing the new TTY
fallback; update that handler to call resolveSurfaceIdForClaudeHook(_ raw:
String?, workspaceId: String, client: SocketClient) instead of resolveSurfaceId
so the session-end path goes through resolveSurfaceIdAllowingFallback. Locate
the code that resolves surfaces for "session-end" events and replace the direct
resolveSurfaceId(...) invocation with resolveSurfaceIdForClaudeHook(...),
ensuring you pass the same raw, workspaceId and client arguments and propagate
any throws or errors as the original call did.
| private func makeSocketPath(_ name: String) -> String { | ||
| let shortID = UUID().uuidString.replacingOccurrences(of: "-", with: "").prefix(8) | ||
| return URL(fileURLWithPath: NSTemporaryDirectory()) | ||
| .appendingPathComponent("cli-\(name.prefix(6))-\(shortID).sock") | ||
| .path |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the test file and examine the relevant sections
fd -t f "WorkspaceRemoteConnectionTests.swift" --exec wc -l {} \;Repository: manaflow-ai/cmux
Length of output: 113
🏁 Script executed:
# Look at the specific lines mentioned in the review
fd -t f "WorkspaceRemoteConnectionTests.swift" --exec head -600 {} \; | tail -150Repository: manaflow-ai/cmux
Length of output: 5223
🏁 Script executed:
# Search for bindUnixSocket implementation
rg -n "bindUnixSocket|makeSocketPath" --type swiftRepository: manaflow-ai/cmux
Length of output: 2359
🏁 Script executed:
# Get the complete bindUnixSocket function from the test file
sed -n '572,630p' cmuxTests/WorkspaceRemoteConnectionTests.swiftRepository: manaflow-ai/cmux
Length of output: 2224
🏁 Script executed:
# Check what's at lines 584-592 (the "Also applies to" section)
sed -n '584,592p' cmuxTests/WorkspaceRemoteConnectionTests.swiftRepository: manaflow-ai/cmux
Length of output: 493
🏁 Script executed:
# Also check the TerminalController.swift implementation
sed -n '781,840p' Sources/TerminalController.swiftRepository: manaflow-ai/cmux
Length of output: 2491
🏁 Script executed:
# Get the actual size of sun_path on macOS
python3 << 'EOF'
import ctypes
# Define sockaddr_un structure for macOS
class SockaddrUn(ctypes.Structure):
pass
# sun_path field size
try:
import socket
# Check sun_path size (typically 104 bytes on macOS)
sock = socket.socket(socket.AF_UNIX)
# sun_path is typically 104 bytes
print(f"Typical sun_path max length: 104 bytes")
print(f"NSTemporaryDirectory() typically: /var/folders/... (around 50-60 chars)")
except Exception as e:
print(f"Error: {e}")
EOFRepository: manaflow-ai/cmux
Length of output: 170
🏁 Script executed:
# Find unixSocketAddress implementation
rg -n "unixSocketAddress" --type swift -B 2 -A 10Repository: manaflow-ai/cmux
Length of output: 2100
🏁 Script executed:
# Search for how the CLI connects to sockets to understand if it uses full vs truncated path
rg -n "connect|sockaddr_un" Sources/TerminalController.swift -A 3 -B 1Repository: manaflow-ai/cmux
Length of output: 7906
🏁 Script executed:
# Get the complete unixSocketAddress implementation
sed -n '759,780p' Sources/TerminalController.swiftRepository: manaflow-ai/cmux
Length of output: 928
🏁 Script executed:
# Check line 763 more carefully
sed -n '759,776p' Sources/TerminalController.swiftRepository: manaflow-ai/cmux
Length of output: 876
🏁 Script executed:
# Check if the CLI code receives socket path as argument or generates it differently
rg -n "makeSocketPath\|bindListenerSocket\|unixSocketAddress" Sources/ --type swift -B 1 -A 1 | head -50Repository: manaflow-ai/cmux
Length of output: 42
Add path length validation to bindUnixSocket to fail fast on overlong paths.
The test code truncates socket paths silently with strncpy without checking the sun_path limit (104 bytes). If makeSocketPath() generates a path ≥ 104 bytes, bindUnixSocket will truncate it during bind, but later connection attempts using the full path will fail or connect to the wrong socket. Add an upfront length check that throws or returns nil when the path doesn't fit, matching the validation in TerminalController.unixSocketAddress().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/WorkspaceRemoteConnectionTests.swift` around lines 492 - 496,
bindUnixSocket silently truncates long UNIX socket paths via strncpy, causing
mismatches with callers using the full path; add an upfront length check in
bindUnixSocket (the same validation used by
TerminalController.unixSocketAddress()) to verify the provided path fits within
sun_path (104 bytes) and fail fast (throw or return nil) when it does not.
Locate bindUnixSocket and before any strncpy/use of sun_path, compute the byte
length of the path (UTF-8) and compare to the sun_path limit (104); if it
exceeds the limit, return an error/optional nil so callers like makeSocketPath
or tests can handle the failure rather than relying on truncated bind behavior.
Ensure the error path is documented/propagated to tests and remove reliance on
silent truncation by strncpy.
| let timedOut = exitSignal.wait(timeout: .now() + timeout) == .timedOut | ||
| if timedOut { | ||
| process.terminate() | ||
| _ = exitSignal.wait(timeout: .now() + 1) | ||
| } | ||
|
|
||
| let stdout = String(data: stdoutPipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "" | ||
| let stderr = String(data: stderrPipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "" | ||
| return ProcessRunResult( | ||
| status: process.terminationStatus, | ||
| stdout: stdout, | ||
| stderr: stderr, | ||
| timedOut: timedOut | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the file
find . -name "WorkspaceRemoteConnectionTests.swift" -type fRepository: manaflow-ai/cmux
Length of output: 108
🏁 Script executed:
# Check the file size to see if we can read it entirely
wc -l cmuxTests/WorkspaceRemoteConnectionTests.swiftRepository: manaflow-ai/cmux
Length of output: 111
🏁 Script executed:
# Read the specific lines in question (556-569) and surrounding context
sed -n '540,580p' cmuxTests/WorkspaceRemoteConnectionTests.swiftRepository: manaflow-ai/cmux
Length of output: 1461
🏁 Script executed:
# Also get the function definition this is part of to understand the full context
sed -n '500,600p' cmuxTests/WorkspaceRemoteConnectionTests.swiftRepository: manaflow-ai/cmux
Length of output: 3609
🏁 Script executed:
# Check if there are any other uses of readDataToEndOfFile in the codebase to understand expected patterns
rg "readDataToEndOfFile" -A 2 -B 2Repository: manaflow-ai/cmux
Length of output: 10320
Add SIGKILL fallback if SIGTERM fails to terminate the child process.
On timeout, process.terminate() sends SIGTERM but does not wait for confirmation. If the process ignores SIGTERM, the 1-second semaphore timeout expires and the code proceeds to readDataToEndOfFile() calls, which block indefinitely waiting for pipe EOF. This breaks the 5-second test timeout bound.
Suggested fix
let timedOut = exitSignal.wait(timeout: .now() + timeout) == .timedOut
if timedOut {
process.terminate()
- _ = exitSignal.wait(timeout: .now() + 1)
+ if exitSignal.wait(timeout: .now() + 1) == .timedOut {
+ Darwin.kill(process.processIdentifier, SIGKILL)
+ _ = exitSignal.wait(timeout: .now() + 1)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let timedOut = exitSignal.wait(timeout: .now() + timeout) == .timedOut | |
| if timedOut { | |
| process.terminate() | |
| _ = exitSignal.wait(timeout: .now() + 1) | |
| } | |
| let stdout = String(data: stdoutPipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "" | |
| let stderr = String(data: stderrPipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "" | |
| return ProcessRunResult( | |
| status: process.terminationStatus, | |
| stdout: stdout, | |
| stderr: stderr, | |
| timedOut: timedOut | |
| ) | |
| let timedOut = exitSignal.wait(timeout: .now() + timeout) == .timedOut | |
| if timedOut { | |
| process.terminate() | |
| if exitSignal.wait(timeout: .now() + 1) == .timedOut { | |
| Darwin.kill(process.processIdentifier, SIGKILL) | |
| _ = exitSignal.wait(timeout: .now() + 1) | |
| } | |
| } | |
| let stdout = String(data: stdoutPipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "" | |
| let stderr = String(data: stderrPipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8) ?? "" | |
| return ProcessRunResult( | |
| status: process.terminationStatus, | |
| stdout: stdout, | |
| stderr: stderr, | |
| timedOut: timedOut | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/WorkspaceRemoteConnectionTests.swift` around lines 556 - 569, On
timeout in the run-and-wait block (where exitSignal.wait, process.terminate()
and the 1s follow-up wait occur) add a SIGKILL fallback: after calling
process.terminate() and waiting the extra second, check if the Process is still
running (e.g., process.isRunning or checking process.processIdentifier) and, if
so, send a SIGKILL (kill(pid, SIGKILL)) and wait again for termination before
proceeding to readDataToEndOfFile() from stdoutPipe/stderrPipe; update the logic
around exitSignal, process.terminate(), and the subsequent wait so the code
guarantees the child has exited (or been killed) before calling
readDataToEndOfFile().
| let tolerance: CGFloat = 0.5 | ||
| let exactFitsWithinPane = | ||
| exactRect.minX >= paneRect.minX - tolerance && | ||
| exactRect.maxX <= paneRect.maxX + tolerance && | ||
| exactRect.minY >= paneRect.minY - tolerance && | ||
| exactRect.maxY <= paneRect.maxY + tolerance | ||
| return exactFitsWithinPane ? exactRect : paneRect |
There was a problem hiding this comment.
Use a near-equality check here instead of a containment check.
exactFitsWithinPane is true for any exactRect fully inside paneRect, even when it is much smaller. For panes whose hosted view is inset from the pane bounds, this shrinks the unread/flash overlay to the inner content area instead of the full tmux pane.
Suggested fix
- let exactFitsWithinPane =
- exactRect.minX >= paneRect.minX - tolerance &&
- exactRect.maxX <= paneRect.maxX + tolerance &&
- exactRect.minY >= paneRect.minY - tolerance &&
- exactRect.maxY <= paneRect.maxY + tolerance
- return exactFitsWithinPane ? exactRect : paneRect
+ let exactMatchesPane =
+ abs(exactRect.minX - paneRect.minX) <= tolerance &&
+ abs(exactRect.maxX - paneRect.maxX) <= tolerance &&
+ abs(exactRect.minY - paneRect.minY) <= tolerance &&
+ abs(exactRect.maxY - paneRect.maxY) <= tolerance
+ return exactMatchesPane ? exactRect : paneRect📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let tolerance: CGFloat = 0.5 | |
| let exactFitsWithinPane = | |
| exactRect.minX >= paneRect.minX - tolerance && | |
| exactRect.maxX <= paneRect.maxX + tolerance && | |
| exactRect.minY >= paneRect.minY - tolerance && | |
| exactRect.maxY <= paneRect.maxY + tolerance | |
| return exactFitsWithinPane ? exactRect : paneRect | |
| let tolerance: CGFloat = 0.5 | |
| let exactMatchesPane = | |
| abs(exactRect.minX - paneRect.minX) <= tolerance && | |
| abs(exactRect.maxX - paneRect.maxX) <= tolerance && | |
| abs(exactRect.minY - paneRect.minY) <= tolerance && | |
| abs(exactRect.maxY - paneRect.maxY) <= tolerance | |
| return exactMatchesPane ? exactRect : paneRect |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ContentView.swift` around lines 1778 - 1784, The current containment
test (exactFitsWithinPane) treats any rect fully inside paneRect as a match;
replace it with a near-equality edge check so only rects whose edges are within
tolerance are considered equal. Update the boolean to compare edges (e.g.,
abs(exactRect.minX - paneRect.minX) <= tolerance, abs(exactRect.maxX -
paneRect.maxX) <= tolerance, abs(exactRect.minY - paneRect.minY) <= tolerance,
abs(exactRect.maxY - paneRect.maxY) <= tolerance) and use that new check in
place of exactFitsWithinPane in the return decision (look for the variables
tolerance, exactFitsWithinPane, exactRect, paneRect in ContentView.swift).
| func hasVisibleNotificationIndicator(forTabId tabId: UUID, surfaceId: UUID?) -> Bool { | ||
| hasUnreadNotification(forTabId: tabId, surfaceId: surfaceId) || | ||
| focusedReadIndicatorByTabId[tabId] == surfaceId | ||
| } |
There was a problem hiding this comment.
Avoid nil == nil turning every tab-level query into a hit.
On Line 882, focusedReadIndicatorByTabId[tabId] == surfaceId evaluates as nil == nil when the tab has no focused-read entry and the caller asks for surfaceId: nil. That makes this helper return true even though no indicator exists.
💡 Suggested fix
func hasVisibleNotificationIndicator(forTabId tabId: UUID, surfaceId: UUID?) -> Bool {
hasUnreadNotification(forTabId: tabId, surfaceId: surfaceId) ||
- focusedReadIndicatorByTabId[tabId] == surfaceId
+ (surfaceId.map { focusedReadIndicatorByTabId[tabId] == $0 } ?? false)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func hasVisibleNotificationIndicator(forTabId tabId: UUID, surfaceId: UUID?) -> Bool { | |
| hasUnreadNotification(forTabId: tabId, surfaceId: surfaceId) || | |
| focusedReadIndicatorByTabId[tabId] == surfaceId | |
| } | |
| func hasVisibleNotificationIndicator(forTabId tabId: UUID, surfaceId: UUID?) -> Bool { | |
| hasUnreadNotification(forTabId: tabId, surfaceId: surfaceId) || | |
| (surfaceId.map { focusedReadIndicatorByTabId[tabId] == $0 } ?? false) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalNotificationStore.swift` around lines 880 - 883, The helper
hasVisibleNotificationIndicator(forTabId:surfaceId:) erroneously returns true
when both the stored focusedReadIndicatorByTabId[tabId] and the passed surfaceId
are nil (nil == nil). Fix it by only comparing the stored value to surfaceId if
a stored value exists; e.g. replace the current boolean expression with a check
that focusedReadIndicatorByTabId[tabId] is non-nil and equals surfaceId (or use
if let focused = focusedReadIndicatorByTabId[tabId] { return focused ==
surfaceId } else { return false }), combined with the existing
hasUnreadNotification(...) OR.
| func clearFocusedReadIndicator(forTabId tabId: UUID, surfaceId: UUID? = nil) { | ||
| guard let existingSurfaceId = focusedReadIndicatorByTabId[tabId] else { return } | ||
| guard surfaceId == nil || existingSurfaceId == surfaceId else { return } | ||
| focusedReadIndicatorByTabId.removeValue(forKey: tabId) | ||
| } |
There was a problem hiding this comment.
Split wildcard clearing from surface-specific clearing.
Treating surfaceId == nil as “clear any focused-read indicator in this tab” is too broad for callers like remove(id:) and clearNotifications(forTabId:surfaceId:). Removing a tab-scoped notification will also clear an unrelated pane-scoped focused indicator on the same tab.
💡 Suggested fix
- func clearFocusedReadIndicator(forTabId tabId: UUID, surfaceId: UUID? = nil) {
- guard let existingSurfaceId = focusedReadIndicatorByTabId[tabId] else { return }
- guard surfaceId == nil || existingSurfaceId == surfaceId else { return }
- focusedReadIndicatorByTabId.removeValue(forKey: tabId)
- }
+ func clearFocusedReadIndicator(forTabId tabId: UUID, surfaceId: UUID) {
+ guard focusedReadIndicatorByTabId[tabId] == surfaceId else { return }
+ focusedReadIndicatorByTabId.removeValue(forKey: tabId)
+ }
+
+ func clearAllFocusedReadIndicators(forTabId tabId: UUID) {
+ focusedReadIndicatorByTabId.removeValue(forKey: tabId)
+ }Use the tab-wide helper only from clearNotifications(forTabId:); keep remove(id:) and clearNotifications(forTabId:surfaceId:) surface-specific.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func clearFocusedReadIndicator(forTabId tabId: UUID, surfaceId: UUID? = nil) { | |
| guard let existingSurfaceId = focusedReadIndicatorByTabId[tabId] else { return } | |
| guard surfaceId == nil || existingSurfaceId == surfaceId else { return } | |
| focusedReadIndicatorByTabId.removeValue(forKey: tabId) | |
| } | |
| func clearFocusedReadIndicator(forTabId tabId: UUID, surfaceId: UUID) { | |
| guard focusedReadIndicatorByTabId[tabId] == surfaceId else { return } | |
| focusedReadIndicatorByTabId.removeValue(forKey: tabId) | |
| } | |
| func clearAllFocusedReadIndicators(forTabId tabId: UUID) { | |
| focusedReadIndicatorByTabId.removeValue(forKey: tabId) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalNotificationStore.swift` around lines 1006 - 1010, The
current clearFocusedReadIndicator(forTabId:surfaceId:) treats surfaceId == nil
as “clear any indicator in the tab”, which is too broad; change it so the
function only removes the indicator when the provided surfaceId exactly matches
the stored surface id (i.e., if surfaceId is nil, do not clear anything unless
existingSurfaceId is also nil). Add a new helper
clearFocusedReadIndicatorForTab(tabId:) (or a boolean parameter like
forceClearTab) that unconditionally removes the entry for a tab and call that
helper only from clearNotifications(forTabId:), leaving remove(id:) and
clearNotifications(forTabId:surfaceId:) to call the surface-specific
clearFocusedReadIndicator(forTabId:surfaceId:).
| private func configureTerminalPanel(_ terminalPanel: TerminalPanel) { | ||
| terminalPanel.onRequestWorkspacePaneFlash = { [weak self, weak terminalPanel] reason in | ||
| guard let self, let terminalPanel else { return } | ||
| self.triggerWorkspacePaneFlash(panelId: terminalPanel.id, reason: reason) | ||
| } |
There was a problem hiding this comment.
Rebind the tmux flash callback when reattaching an existing terminal panel.
Line 5617 only wires onRequestWorkspacePaneFlash when a TerminalPanel is first created. attachDetachedSurface(...) reuses the same panel object and only updates its workspace ID, so a tab moved across workspaces/windows keeps sending pane-flash requests back to the old workspace. That breaks the new tmux attention routing after a drag/move.
Suggested fix
if let terminalPanel = detached.panel as? TerminalPanel {
terminalPanel.updateWorkspaceId(id)
+ configureTerminalPanel(terminalPanel)
} else if let browserPanel = detached.panel as? BrowserPanel {
browserPanel.reattachToWorkspace(
id,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 5616 - 5620, The
onRequestWorkspacePaneFlash callback is only set in configureTerminalPanel(_
terminalPanel: TerminalPanel) when a panel is first created, so reusing a
TerminalPanel in attachDetachedSurface(...) leaves the old callback bound to the
previous workspace; rebind the callback whenever a panel is reattached by either
calling configureTerminalPanel(_:) from attachDetachedSurface(...) after
updating the panel's workspace ID or explicitly resetting
terminalPanel.onRequestWorkspacePaneFlash there to the closure that calls
triggerWorkspacePaneFlash(panelId:reason:). Ensure you reference TerminalPanel,
onRequestWorkspacePaneFlash, attachDetachedSurface(...),
configureTerminalPanel(_ terminalPanel: TerminalPanel), and
triggerWorkspacePaneFlash(panelId:reason:) so the callback is always bound to
the current workspace.
| if let lastFlashToken, | ||
| state.flashToken != lastFlashToken, | ||
| state.flashRect != nil { | ||
| flashStartedAt = now() | ||
| } | ||
| self.lastFlashToken = state.flashToken |
There was a problem hiding this comment.
Don't consume the flash token before a rect exists.
Sources/ContentView.swift can publish a new flashToken while flashRect is still nil during layout recovery. This branch still advances lastFlashToken, so when the rect resolves on the next pass the same token is treated as already handled and the flash never starts.
💡 Suggested fix
- if let lastFlashToken,
- state.flashToken != lastFlashToken,
- state.flashRect != nil {
- flashStartedAt = now()
- }
- self.lastFlashToken = state.flashToken
+ if let lastFlashToken,
+ state.flashToken != lastFlashToken {
+ guard state.flashRect != nil else { return }
+ flashStartedAt = now()
+ }
+ self.lastFlashToken = state.flashToken🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/WorkspaceContentView.swift` around lines 124 - 129, The code
currently advances self.lastFlashToken before a flashRect exists, causing a
token published during layout recovery to be treated as already handled; modify
the logic so you only consume (assign) state.flashToken into lastFlashToken when
there is a valid rect or when you actually start the flash: i.e. move the
self.lastFlashToken = state.flashToken assignment inside the branch that checks
state.flashRect != nil (or inside the same branch where you set flashStartedAt)
so that tokens published while state.flashRect is nil are not marked as handled.
| let shouldShow = panelId.map { | ||
| notificationStore.hasVisibleNotificationIndicator(forTabId: workspace.id, surfaceId: $0) || | ||
| manualUnread.contains($0) | ||
| } ?? false |
There was a problem hiding this comment.
Resync Bonsplit badges when focused-read state changes.
shouldShow now depends on hasVisibleNotificationIndicator(...), but the only sync hooks in this view still watch notificationStore.notifications and workspace.manualUnreadPanelIds. This PR also adds focused-read mutators that can change the visible-indicator source without touching either collection, so tab.showsNotificationBadge can stay stale until a later notification mutation.
💡 Suggested fix
.onChange(of: notificationStore.notifications) { _, _ in
syncBonsplitNotificationBadges()
}
+ .onChange(of: notificationStore.focusedReadIndicatorByTabId) { _, _ in
+ syncBonsplitNotificationBadges()
+ }
.onChange(of: workspace.manualUnreadPanelIds) { _, _ in
syncBonsplitNotificationBadges()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/WorkspaceContentView.swift` around lines 390 - 393, shouldShow can
become stale because it depends on
notificationStore.hasVisibleNotificationIndicator(…) but the view only syncs to
notificationStore.notifications and workspace.manualUnreadPanelIds; add a sync
hook so the view also observes the focused-read state that the focused-read
mutators change (e.g. subscribe to notificationStore.focusedReadState or
workspace.focusedReadPanelIds or whatever property the mutators update) and
trigger a view refresh whenever that focused-read property changes so shouldShow
(and tab.showsNotificationBadge) are recomputed immediately.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a41bc97410
ℹ️ 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".
| hasUnreadNotification(forTabId: tabId, surfaceId: surfaceId) || | ||
| focusedReadIndicatorByTabId[tabId] == surfaceId |
There was a problem hiding this comment.
hasVisibleNotificationIndicator currently compares focusedReadIndicatorByTabId[tabId] == surfaceId; when surfaceId is nil and no focused indicator is stored, that comparison is true (nil == nil). This makes callers think a visible indicator exists even when none does, so nil-surface paths (for example, direct-interaction dismissal when no focused surface is resolved) can take notification-dismiss logic unexpectedly and clear unrelated state.
Useful? React with 👍 / 👎.
| if hasUnreadNotification { | ||
| notificationStore.markRead(forTabId: tabId, surfaceId: surfaceId) | ||
| } | ||
| notificationStore.clearFocusedReadIndicator(forTabId: tabId, surfaceId: surfaceId) | ||
| if let panelId = surfaceId, |
There was a problem hiding this comment.
In the indicator-only path (hasUnreadNotification == false), this code clears the focused-read indicator but does not trigger any tab badge recomputation. Because no notification is marked read in this branch, the notifications array is unchanged and the Bonsplit badge sync path is not driven, so the unread dot can remain visible even after direct interaction dismissed the focused-read indicator.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/Workspace.swift (1)
5616-5621:⚠️ Potential issue | 🟠 MajorRebind the tmux flash callback when reattaching existing
TerminalPanelinstances.
configureTerminalPanel(_:)is introduced here, butattachDetachedSurface(...)(Line 7909–7912) still only updates workspace ID and does not rewireonRequestWorkspacePaneFlash. That can keep flash routing bound to the previous workspace after cross-workspace/window moves.Suggested fix
if let terminalPanel = detached.panel as? TerminalPanel { terminalPanel.updateWorkspaceId(id) + configureTerminalPanel(terminalPanel) } else if let browserPanel = detached.panel as? BrowserPanel {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 5616 - 5621, The attachDetachedSurface(...) path fails to rebind TerminalPanel.onRequestWorkspacePaneFlash when an existing TerminalPanel is reattached, leaving its flash callback pointing at the old workspace; update attachDetachedSurface(...) to call configureTerminalPanel(_:) for the reattached TerminalPanel (or explicitly reset onRequestWorkspacePaneFlash to call triggerWorkspacePaneFlash(panelId:reason:)) so the callback is rebound to the current workspace/window context.
🧹 Nitpick comments (1)
Sources/Workspace.swift (1)
5789-5791: RenamehasUnreadNotificationto match its new semantics.It now checks
hasVisibleNotificationIndicator(...), not unread-only state. Renaming would reduce future logic mistakes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 5789 - 5791, Rename the method hasUnreadNotification to reflect that it queries visible indicators rather than unread state—e.g. hasVisibleNotificationIndicator(forPanel:) or hasVisibleIndicator(forPanelId:). Update the method declaration (currently calling AppDelegate.shared?.notificationStore?.hasVisibleNotificationIndicator(forTabId: id, surfaceId: panelId)) and all call sites, tests, and any comments to use the new name so usages match the actual semantics; ensure method signature still takes panelId: UUID (or rename parameter to panelId/forPanelId for clarity) and run tests to catch missed references.
🤖 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/Workspace.swift`:
- Around line 8376-8380: triggerDebugFlash must not change app focus for
socket/CLI-triggered flashes: remove the call to focusPanel(panelId) from
triggerDebugFlash and invoke only requestAttentionFlash(panelId: panelId,
reason: .debug). If other code relies on the old behavior, add a separate
explicit-focus API (e.g. focusAndTriggerDebugFlash) or a boolean parameter to
request callers that actually intend to change focus; keep focusPanel usage
confined to explicit focus-intent handlers only.
---
Duplicate comments:
In `@Sources/Workspace.swift`:
- Around line 5616-5621: The attachDetachedSurface(...) path fails to rebind
TerminalPanel.onRequestWorkspacePaneFlash when an existing TerminalPanel is
reattached, leaving its flash callback pointing at the old workspace; update
attachDetachedSurface(...) to call configureTerminalPanel(_:) for the reattached
TerminalPanel (or explicitly reset onRequestWorkspacePaneFlash to call
triggerWorkspacePaneFlash(panelId:reason:)) so the callback is rebound to the
current workspace/window context.
---
Nitpick comments:
In `@Sources/Workspace.swift`:
- Around line 5789-5791: Rename the method hasUnreadNotification to reflect that
it queries visible indicators rather than unread state—e.g.
hasVisibleNotificationIndicator(forPanel:) or hasVisibleIndicator(forPanelId:).
Update the method declaration (currently calling
AppDelegate.shared?.notificationStore?.hasVisibleNotificationIndicator(forTabId:
id, surfaceId: panelId)) and all call sites, tests, and any comments to use the
new name so usages match the actual semantics; ensure method signature still
takes panelId: UUID (or rename parameter to panelId/forPanelId for clarity) and
run tests to catch missed references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 624a506f-2425-4b2e-b4c3-2b9811c690eb
📒 Files selected for processing (2)
Sources/Workspace.swiftcmuxTests/WorkspaceUnitTests.swift
| func triggerDebugFlash(panelId: UUID) { | ||
| triggerNotificationFocusFlash(panelId: panelId, requiresSplit: false, shouldFocus: true) | ||
| guard panels[panelId] != nil else { return } | ||
| focusPanel(panelId) | ||
| requestAttentionFlash(panelId: panelId, reason: .debug) | ||
| } |
There was a problem hiding this comment.
Avoid focus mutation in triggerDebugFlash for socket/CLI-triggered flash paths.
Line 8378 calls focusPanel(panelId) before flashing. For trigger-flash/socket-driven diagnostics this introduces in-app focus changes in a non-focus-intent command path.
Suggested fix
func triggerDebugFlash(panelId: UUID) {
guard panels[panelId] != nil else { return }
- focusPanel(panelId)
requestAttentionFlash(panelId: panelId, reason: .debug)
}As per coding guidelines: “Socket/CLI commands must not steal macOS app focus ... Only explicit focus-intent commands may mutate in-app focus/selection.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 8376 - 8380, triggerDebugFlash must not
change app focus for socket/CLI-triggered flashes: remove the call to
focusPanel(panelId) from triggerDebugFlash and invoke only
requestAttentionFlash(panelId: panelId, reason: .debug). If other code relies on
the old behavior, add a separate explicit-focus API (e.g.
focusAndTriggerDebugFlash) or a boolean parameter to request callers that
actually intend to change focus; keep focusPanel usage confined to explicit
focus-intent handlers only.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmuxTests/TabManagerUnitTests.swift (1)
581-585: Replace fixed sleep-style wait with deterministic synchronizationThe 100ms delay can be flaky on slower CI runs. Prefer
XCTNSPredicateExpectation(or a queue-drain helper loop) ontmuxWorkspaceFlashToken/ notification-store state instead of time-based waiting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/TabManagerUnitTests.swift` around lines 581 - 585, Replace the brittle time-based wait (DispatchQueue.main.asyncAfter + XCTestExpectation) with a deterministic XCTest expectation that observes the actual state change: use XCTNSPredicateExpectation or a loop that polls until tmuxWorkspaceFlashToken (or the notification store's relevant property) reflects the flash/notification state; for example, create an NSPredicate that checks tmuxWorkspaceFlashToken != nil (or the specific notification count/state) and wait(for: [predicateExpectation], timeout: 1) so the test waits for the real condition instead of a fixed 0.1s delay.
🤖 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/TabManager.swift`:
- Around line 2909-2916: The dismiss call can no-op if the app isn't active
(AppFocusState.isAppActive() == false) and the selected-tab async hook runs
before activation; after calling focusTab (suppressFocusFlash / focusTab), if
dismissNotificationOnDirectInteraction(tabId:..., surfaceId:...) returns early
due to inactivity, schedule or register a retry to run once the app becomes
active (e.g., add a one-shot observer or enqueue a closure on the app-activation
handler that calls dismissNotificationOnDirectInteraction with the same
tabId/surfaceId), ensuring dismissal is attempted again after activation rather
than relying on the initial call to succeed.
---
Nitpick comments:
In `@cmuxTests/TabManagerUnitTests.swift`:
- Around line 581-585: Replace the brittle time-based wait
(DispatchQueue.main.asyncAfter + XCTestExpectation) with a deterministic XCTest
expectation that observes the actual state change: use XCTNSPredicateExpectation
or a loop that polls until tmuxWorkspaceFlashToken (or the notification store's
relevant property) reflects the flash/notification state; for example, create an
NSPredicate that checks tmuxWorkspaceFlashToken != nil (or the specific
notification count/state) and wait(for: [predicateExpectation], timeout: 1) so
the test waits for the real condition instead of a fixed 0.1s delay.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b284e0c-60fc-4b1a-84b5-ca02687d1817
📒 Files selected for processing (2)
Sources/TabManager.swiftcmuxTests/TabManagerUnitTests.swift
| suppressFocusFlash = true | ||
| focusTab(tabId, surfaceId: desiredPanelId, suppressFlash: true) | ||
| if wasSelected { | ||
| suppressFocusFlash = false | ||
| } | ||
|
|
||
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.05) { [weak self] in | ||
| guard let self, | ||
| let tab = self.tabs.first(where: { $0.id == tabId }) else { return } | ||
| let targetPanelId = desiredPanelId ?? tab.focusedPanelId | ||
| guard let targetPanelId, | ||
| tab.panels[targetPanelId] != nil else { return } | ||
| guard let notificationStore = AppDelegate.shared?.notificationStore else { return } | ||
| guard notificationStore.hasUnreadNotification(forTabId: tabId, surfaceId: targetPanelId) else { return } | ||
| tab.triggerNotificationFocusFlash(panelId: targetPanelId, requiresSplit: false, shouldFocus: true) | ||
| notificationStore.markRead(forTabId: tabId, surfaceId: targetPanelId) | ||
| suppressFocusFlash = false | ||
|
|
||
| if let targetPanelId = desiredPanelId ?? tab.focusedPanelId, | ||
| tab.panels[targetPanelId] != nil { | ||
| _ = dismissNotificationOnDirectInteraction(tabId: tabId, surfaceId: targetPanelId) | ||
| } |
There was a problem hiding this comment.
Dismiss can be skipped when notification focus happens from an inactive app
Line 2915 can run before the app becomes active, and dismissNotificationOnDirectInteraction(...) returns early on AppFocusState.isAppActive() == false. Because the selected-tab async hook is also queued before the activation block, both dismissal attempts may no-op, leaving unread/indicator state uncleared.
Proposed fix
suppressFocusFlash = true
focusTab(tabId, surfaceId: desiredPanelId, suppressFlash: true)
- suppressFocusFlash = false
-
- if let targetPanelId = desiredPanelId ?? tab.focusedPanelId,
- tab.panels[targetPanelId] != nil {
- _ = dismissNotificationOnDirectInteraction(tabId: tabId, surfaceId: targetPanelId)
- }
+ DispatchQueue.main.async { [weak self, weak tab] in
+ guard let self, let tab else { return }
+ self.suppressFocusFlash = false
+ guard let targetPanelId = desiredPanelId ?? tab.focusedPanelId,
+ tab.panels[targetPanelId] != nil else { return }
+ _ = self.dismissNotificationOnDirectInteraction(tabId: tabId, surfaceId: targetPanelId)
+ }
return true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TabManager.swift` around lines 2909 - 2916, The dismiss call can
no-op if the app isn't active (AppFocusState.isAppActive() == false) and the
selected-tab async hook runs before activation; after calling focusTab
(suppressFocusFlash / focusTab), if
dismissNotificationOnDirectInteraction(tabId:..., surfaceId:...) returns early
due to inactivity, schedule or register a retry to run once the app becomes
active (e.g., add a one-shot observer or enqueue a closure on the app-activation
handler that calls dismissNotificationOnDirectInteraction with the same
tabId/surfaceId), ensuring dismissal is attempted again after activation rather
than relying on the initial call to succeed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a04ac378c
ℹ️ 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".
| if let targetPanelId = desiredPanelId ?? tab.focusedPanelId, | ||
| tab.panels[targetPanelId] != nil { | ||
| _ = dismissNotificationOnDirectInteraction(tabId: tabId, surfaceId: targetPanelId) | ||
| } |
There was a problem hiding this comment.
focusTabFromNotification now calls dismissNotificationOnDirectInteraction immediately after focusTab, but focusTab only activates the app on a queued DispatchQueue.main.async block. When this path is triggered while cmux is inactive (for example from a system notification click), dismissNotificationOnDirectInteraction hits AppFocusState.isAppActive() == false and returns without clearing unread state, and there is no later retry, so the jumped-to notification can remain unread incorrectly.
Useful? React with 👍 / 👎.
| func triggerFocusFlash(panelId: UUID) { | ||
| panels[panelId]?.triggerFlash() | ||
| requestAttentionFlash(panelId: panelId, reason: .navigation) | ||
| } |
There was a problem hiding this comment.
triggerFocusFlash now routes through requestAttentionFlash(..., reason: .navigation), which suppresses flashes when another panel has any persistent indicator. Because surface.trigger_flash calls this method, cmux trigger-flash can become a no-op whenever a different pane has unread/manual/focused-read attention, which is a behavior regression for an explicit flash command that should not be blocked by unrelated pane state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
2 issues 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="cmuxTests/TabManagerUnitTests.swift">
<violation number="1" location="cmuxTests/TabManagerUnitTests.swift:581">
P2: Avoid fixed-delay waiting in this test; wait for the flash state condition instead to reduce flakiness and test runtime variance.</violation>
</file>
<file name="Sources/TabManager.swift">
<violation number="1" location="Sources/TabManager.swift:2915">
P2: Dismissal is executed before async app activation, so notification jump can fail to clear unread/indicator state when the app starts inactive.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| let expectation = XCTestExpectation(description: "notification focus flash") | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { | ||
| expectation.fulfill() | ||
| } | ||
| wait(for: [expectation], timeout: 1) |
There was a problem hiding this comment.
P2: Avoid fixed-delay waiting in this test; wait for the flash state condition instead to reduce flakiness and test runtime variance.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/TabManagerUnitTests.swift, line 581:
<comment>Avoid fixed-delay waiting in this test; wait for the flash state condition instead to reduce flakiness and test runtime variance.</comment>
<file context>
@@ -517,6 +517,80 @@ final class TabManagerNotificationFocusTests: XCTestCase {
+
+ XCTAssertTrue(manager.focusTabFromNotification(workspace.id, surfaceId: rightPanel.id))
+
+ let expectation = XCTestExpectation(description: "notification focus flash")
+ DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) {
+ expectation.fulfill()
</file context>
| let expectation = XCTestExpectation(description: "notification focus flash") | |
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { | |
| expectation.fulfill() | |
| } | |
| wait(for: [expectation], timeout: 1) | |
| let flashExpectation = XCTNSPredicateExpectation( | |
| predicate: NSPredicate { _, _ in workspace.tmuxWorkspaceFlashToken == 1 }, | |
| object: nil | |
| ) | |
| XCTAssertEqual(XCTWaiter.wait(for: [flashExpectation], timeout: 1), .completed) |
|
|
||
| if let targetPanelId = desiredPanelId ?? tab.focusedPanelId, | ||
| tab.panels[targetPanelId] != nil { | ||
| _ = dismissNotificationOnDirectInteraction(tabId: tabId, surfaceId: targetPanelId) |
There was a problem hiding this comment.
P2: Dismissal is executed before async app activation, so notification jump can fail to clear unread/indicator state when the app starts inactive.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/TabManager.swift, line 2915:
<comment>Dismissal is executed before async app activation, so notification jump can fail to clear unread/indicator state when the app starts inactive.</comment>
<file context>
@@ -2915,20 +2908,11 @@ class TabManager: ObservableObject {
+
+ if let targetPanelId = desiredPanelId ?? tab.focusedPanelId,
+ tab.panels[targetPanelId] != nil {
+ _ = dismissNotificationOnDirectInteraction(tabId: tabId, surfaceId: targetPanelId)
}
return true
</file context>
| _ = dismissNotificationOnDirectInteraction(tabId: tabId, surfaceId: targetPanelId) | |
| DispatchQueue.main.async { [weak self] in | |
| guard let self else { return } | |
| _ = self.dismissNotificationOnDirectInteraction(tabId: tabId, surfaceId: targetPanelId) | |
| } |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Resources/shell-integration/cmux-bash-integration.bash (1)
134-137: Simplify export to avoid shellcheck warning SC2163.The two-step
printf -vthenexportpattern works but triggers a shellcheck warning. Use the single-statement form that both sets and exports the variable, matching the zsh integration style.Suggested fix
if [[ "${!key}" != "$value" ]]; then - printf -v "$key" '%s' "$value" - export "$key" + export "$key=$value" did_change=1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/shell-integration/cmux-bash-integration.bash` around lines 134 - 137, Replace the two-step assignment and export (printf -v "$key" '%s' "$value" followed by export "$key") with a single export assignment to avoid SC2163; assign and export in one statement using the dynamic name/value pair (i.e., export "$key=$value") and keep updating did_change=1 as before so the code still records modifications to the environment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Resources/shell-integration/cmux-bash-integration.bash`:
- Around line 134-137: Replace the two-step assignment and export (printf -v
"$key" '%s' "$value" followed by export "$key") with a single export assignment
to avoid SC2163; assign and export in one statement using the dynamic name/value
pair (i.e., export "$key=$value") and keep updating did_change=1 as before so
the code still records modifications to the environment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6edbeb50-1ed7-4628-a429-b9ead84011a0
📒 Files selected for processing (3)
Resources/shell-integration/cmux-bash-integration.bashResources/shell-integration/cmux-zsh-integration.zshcmuxTests/GhosttyConfigTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e9a58d1d0
ℹ️ 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".
| if let raw, | ||
| !raw.isEmpty, | ||
| let candidate = try? resolveWorkspaceId(raw, client: client), | ||
| (try? client.sendV2(method: "surface.list", params: ["workspace_id": candidate])) != nil { | ||
| return candidate |
There was a problem hiding this comment.
When --workspace is omitted, this branch accepts any resolvable workspace from inherited env (CMUX_WORKSPACE_ID) before consulting debug.terminals. If that env value is stale-but-still-valid (for example, from another tmux pane/workspace that still exists), notify/trigger-flash will be routed to the wrong workspace even though a caller TTY mapping is available. The TTY-derived binding should be tried before trusting inherited workspace IDs in this fallback path.
Useful? React with 👍 / 👎.
| if items.contains(where: { | ||
| ($0["id"] as? String) == candidate || ($0["ref"] as? String) == candidate | ||
| }) { | ||
| return candidate | ||
| } |
There was a problem hiding this comment.
This candidate-return path runs before the TTY fallback, so a stale-but-existing CMUX_SURFACE_ID is treated as authoritative as long as it appears in surface.list. In multi-pane tmux sessions, that can direct notify/surface.trigger_flash to a different pane than the caller terminal, defeating the new caller-routing logic. The resolver should prioritize the TTY-derived surface binding when no explicit --surface/--panel was provided.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
2 issues 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="Resources/shell-integration/cmux-zsh-integration.zsh">
<violation number="1" location="Resources/shell-integration/cmux-zsh-integration.zsh:127">
P2: `_cmux_tmux_refresh_cmux_environment` filters only `CMUX_*`, so `CMUXD_UNIX_PATH` and `CMUXTERM_REPO_ROOT` from `_CMUX_TMUX_SYNC_KEYS` never get refreshed inside tmux shells.</violation>
</file>
<file name="Resources/shell-integration/cmux-bash-integration.bash">
<violation number="1" location="Resources/shell-integration/cmux-bash-integration.bash:123">
P1: Tmux environment sync ignores removed CMUX variables, so stale `CMUX_*` values can persist and misroute notifications.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| local line filtered="" did_change=0 | ||
| while IFS= read -r line; do | ||
| [[ "$line" == CMUX_* ]] || continue |
There was a problem hiding this comment.
P2: _cmux_tmux_refresh_cmux_environment filters only CMUX_*, so CMUXD_UNIX_PATH and CMUXTERM_REPO_ROOT from _CMUX_TMUX_SYNC_KEYS never get refreshed inside tmux shells.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Resources/shell-integration/cmux-zsh-integration.zsh, line 127:
<comment>`_cmux_tmux_refresh_cmux_environment` filters only `CMUX_*`, so `CMUXD_UNIX_PATH` and `CMUXTERM_REPO_ROOT` from `_CMUX_TMUX_SYNC_KEYS` never get refreshed inside tmux shells.</comment>
<file context>
@@ -60,6 +60,111 @@ typeset -g _CMUX_TTY_NAME=""
+
+ local line filtered="" did_change=0
+ while IFS= read -r line; do
+ [[ "$line" == CMUX_* ]] || continue
+ filtered+="${line}"$'\n'
+ done <<< "$output"
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cef77e456
ℹ️ 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".
| _cmux_tmux_sync_cmux_environment | ||
|
|
||
| [[ -S "$CMUX_SOCKET_PATH" ]] || return 0 | ||
| [[ -n "$CMUX_TAB_ID" ]] || return 0 | ||
| [[ -n "$CMUX_PANEL_ID" ]] || return 0 |
There was a problem hiding this comment.
_cmux_preexec_command now syncs tmux state but still returns early when CMUX_PANEL_ID is empty, which prevents _cmux_report_tty_once from running at all. In tmux panes that only have workspace-scoped CMUX variables, this means the pane’s tty is never reported and the new caller-tty routing for notify/trigger-flash cannot resolve to the originating surface.
Useful? React with 👍 / 👎.
| [[ -n "$filtered" ]] || return 0 | ||
| [[ "$filtered" == "$_CMUX_TMUX_PULL_SIGNATURE" ]] && return 0 |
There was a problem hiding this comment.
The refresh path only applies keys that are still present in filtered and returns immediately when filtered is empty, so managed vars that disappear from tmux global environment are never cleared locally. After a workspace/tab key is removed upstream, the shell can keep stale CMUX_* values and continue sending commands to the wrong workspace context.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
3 issues found across 5 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="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:1867">
P2: The new `TMUX` gate clears `CMUX_WORKSPACE_ID`/`CMUX_SURFACE_ID` defaults, so `notify`/`trigger-flash` can misroute in tmux when tty fallback is unavailable.</violation>
</file>
<file name="Resources/shell-integration/cmux-zsh-integration.zsh">
<violation number="1" location="Resources/shell-integration/cmux-zsh-integration.zsh:126">
P2: Surface-scoped tmux env cleanup is bypassed by the existing signature short-circuit, so leaked `CMUX_PANEL_ID`/`CMUX_SURFACE_ID` can persist until some other workspace-scoped key changes.</violation>
<violation number="2" location="Resources/shell-integration/cmux-zsh-integration.zsh:321">
P2: Always omitting `--panel` in tmux mode makes `report_tty` fall back to the focused panel, which can misattribute TTY ownership in multi-surface workspaces.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| let workspaceArg = tfWsFlag ?? (windowId == nil ? ProcessInfo.processInfo.environment["CMUX_WORKSPACE_ID"] : nil) | ||
| let surfaceArg = optionValue(commandArgs, name: "--surface") ?? optionValue(commandArgs, name: "--panel") ?? (tfWsFlag == nil && windowId == nil ? ProcessInfo.processInfo.environment["CMUX_SURFACE_ID"] : nil) | ||
| let explicitWorkspaceArg = tfWsFlag | ||
| let preferTTYFallback = windowId == nil && ProcessInfo.processInfo.environment["TMUX"] != nil |
There was a problem hiding this comment.
P2: The new TMUX gate clears CMUX_WORKSPACE_ID/CMUX_SURFACE_ID defaults, so notify/trigger-flash can misroute in tmux when tty fallback is unavailable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CLI/cmux.swift, line 1867:
<comment>The new `TMUX` gate clears `CMUX_WORKSPACE_ID`/`CMUX_SURFACE_ID` defaults, so `notify`/`trigger-flash` can misroute in tmux when tty fallback is unavailable.</comment>
<file context>
@@ -1864,10 +1864,13 @@ struct CMUXCLI {
let tfWsFlag = optionValue(commandArgs, name: "--workspace")
let explicitWorkspaceArg = tfWsFlag
- let callerWorkspaceArg = windowId == nil ? ProcessInfo.processInfo.environment["CMUX_WORKSPACE_ID"] : nil
+ let preferTTYFallback = windowId == nil && ProcessInfo.processInfo.environment["TMUX"] != nil
+ let callerWorkspaceArg = preferTTYFallback
+ ? nil
</file context>
| tmux set-environment -g "$key" "$value" >/dev/null 2>&1 || return 0 | ||
| done | ||
|
|
||
| for key in "${_CMUX_TMUX_SURFACE_SCOPED_KEYS[@]}"; do |
There was a problem hiding this comment.
P2: Surface-scoped tmux env cleanup is bypassed by the existing signature short-circuit, so leaked CMUX_PANEL_ID/CMUX_SURFACE_ID can persist until some other workspace-scoped key changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Resources/shell-integration/cmux-zsh-integration.zsh, line 126:
<comment>Surface-scoped tmux env cleanup is bypassed by the existing signature short-circuit, so leaked `CMUX_PANEL_ID`/`CMUX_SURFACE_ID` can persist until some other workspace-scoped key changes.</comment>
<file context>
@@ -119,6 +123,10 @@ _cmux_tmux_publish_cmux_environment() {
tmux set-environment -g "$key" "$value" >/dev/null 2>&1 || return 0
done
+ for key in "${_CMUX_TMUX_SURFACE_SCOPED_KEYS[@]}"; do
+ tmux set-environment -gu "$key" >/dev/null 2>&1 || return 0
+ done
</file context>
| if [[ -z "$TMUX" ]]; then | ||
| [[ -n "$CMUX_PANEL_ID" ]] || return 0 | ||
| payload+=" --panel=$CMUX_PANEL_ID" | ||
| fi |
There was a problem hiding this comment.
P2: Always omitting --panel in tmux mode makes report_tty fall back to the focused panel, which can misattribute TTY ownership in multi-surface workspaces.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Resources/shell-integration/cmux-zsh-integration.zsh, line 321:
<comment>Always omitting `--panel` in tmux mode makes `report_tty` fall back to the focused panel, which can misattribute TTY ownership in multi-surface workspaces.</comment>
<file context>
@@ -305,17 +313,32 @@ _cmux_git_head_signature() {
+ [[ -n "$_CMUX_TTY_NAME" ]] || return 0
+
+ local payload="report_tty $_CMUX_TTY_NAME --tab=$CMUX_TAB_ID"
+ if [[ -z "$TMUX" ]]; then
+ [[ -n "$CMUX_PANEL_ID" ]] || return 0
+ payload+=" --panel=$CMUX_PANEL_ID"
</file context>
| if [[ -z "$TMUX" ]]; then | |
| [[ -n "$CMUX_PANEL_ID" ]] || return 0 | |
| payload+=" --panel=$CMUX_PANEL_ID" | |
| fi | |
| if [[ -n "$CMUX_PANEL_ID" ]]; then | |
| payload+=" --panel=$CMUX_PANEL_ID" | |
| fi |
…cation-attention-state Improve tmux notification attention routing
This branch was successfully deployed
1 participant
Summary
Testing
Summary by cubic
Routes tmux notifications to the right pane and aligns whole‑pane overlays so unread rings and flashes share the same bounds. Adds self‑healing routing for stale surfaces, dismisses attention on focus, and suppresses navigation flashes; keeps CLI routing accurate by syncing workspace‑scoped
CMUX_*through tmux.New Features
surface, Bonsplit pane, or tmux active pane.CMUX_*to the tmux server frombash/zshfor accurate routing across panes and sessions.Bug Fixes
notify/trigger-flashto the originating workspace/pane, honoring--workspaceand--surface/--panel; self‑heal stale surface IDs and resolve via tmux when invoked from a TTY.Written for commit 5cef77e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests