Fix visible helper setup targeting - #6494
Closed
lawrencecchen wants to merge 3 commits into
Closed
lawrencecchen wants to merge 3 commits into
lawrencecchen wants to merge 3 commits into
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:
📝 WalkthroughWalkthroughAdds a Changeshelper.visible feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 errors, 1 warning)
✅ Passed checks (19 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR adds
Confidence Score: 4/5Safe to merge for most cases; the window-event wait carries a small spurious-timeout risk for fast-layout newly created surfaces. The event-driven SurfaceHostedWindowWait design is sound and correctly isolated to the main actor with no blocking primitives. The placement logic correctly filters candidate panes by requested type. The known concern — that surfaceHostedViewDidMoveToWindow can fire before controlSurfaceWaitForInWindow is reached from the socket worker, leaving the observer registered too late — can produce spurious not_visible timeouts for newly created surfaces when the host view enters the window faster than the socket-worker path completes its pane-creation round-trip. In practice the 1.5 s timeout and the initial visibleInUI fast-path check mitigate most real-world cases, but the edge case remains. Sources/TerminalController+ControlSurfaceContext.swift — specifically the ordering of the initial visibility check vs. observer registration in controlSurfaceWaitForInWindow. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant CLI as cmux CLI
participant SW as Socket Worker
participant CC as ControlCommandCoordinator
participant MA as Main Actor (TerminalController)
participant NC as NotificationCenter
CLI->>SW: "helper.visible {target=focused, type, ...}"
SW->>SW: stamp mutation deadline
SW->>CC: handleAsync(request)
CC->>MA: controlSystemIdentify → focusedWorkspaceID
CC->>MA: controlPaneList(routing)
CC->>MA: controlSurfaceHealth(routing)
alt reuse visible right-side pane
CC->>MA: "helperVisibleSurfaceVisibility(waitForWindowEvent=false)"
MA-->>CC: visibleInUI snapshot
else create new pane
CC->>MA: "paneCreate(direction=right, focus=false)"
MA-->>CC: pane_id / surface_id
CC->>MA: controlSurfaceWaitForInWindow(surfaceID)
MA->>NC: addObserver(surfaceHostedViewDidMoveToWindow)
NC-->>MA: notification fires
MA->>MA: isVisible() check
MA-->>CC: "observed=true"
CC->>MA: controlSurfaceHealth final verify
end
CC->>MA: surfaceSendText(command)
CC-->>SW: ControlCallResult
SW-->>CLI: JSON response
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant CLI as cmux CLI
participant SW as Socket Worker
participant CC as ControlCommandCoordinator
participant MA as Main Actor (TerminalController)
participant NC as NotificationCenter
CLI->>SW: "helper.visible {target=focused, type, ...}"
SW->>SW: stamp mutation deadline
SW->>CC: handleAsync(request)
CC->>MA: controlSystemIdentify → focusedWorkspaceID
CC->>MA: controlPaneList(routing)
CC->>MA: controlSurfaceHealth(routing)
alt reuse visible right-side pane
CC->>MA: "helperVisibleSurfaceVisibility(waitForWindowEvent=false)"
MA-->>CC: visibleInUI snapshot
else create new pane
CC->>MA: "paneCreate(direction=right, focus=false)"
MA-->>CC: pane_id / surface_id
CC->>MA: controlSurfaceWaitForInWindow(surfaceID)
MA->>NC: addObserver(surfaceHostedViewDidMoveToWindow)
NC-->>MA: notification fires
MA->>MA: isVisible() check
MA-->>CC: "observed=true"
CC->>MA: controlSurfaceHealth final verify
end
CC->>MA: surfaceSendText(command)
CC-->>SW: ControlCallResult
SW-->>CLI: JSON response
Reviews (4): Last reviewed commit: "Bound visible helper setup failure paths" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+209
to
+215
| let hasAnyVisibleCandidate = orderedCandidates.contains { pane in | ||
| pane.surfaceIDs.contains { visibleEntriesByID[$0] != nil } | ||
| } | ||
| if hasAnyVisibleCandidate { | ||
| return .create | ||
| } | ||
| return .blockedInvisible(orderedCandidates.first ?? candidates[0]) |
Contributor
There was a problem hiding this comment.
blockedInvisible does not filter by requested type, blocking creation of a different-type helper
hasAnyVisibleCandidate checks all surface IDs across all candidate panes with no type guard. If the only non-focused pane is an invisible browser helper and the caller requests type=terminal, hasAnyVisibleCandidate will be false and the function returns .blockedInvisible(browser pane). The caller then gets a not_visible error and cannot create a terminal helper, even though no same-type helper pane exists to duplicate.
The reuse loop at line 201–207 correctly filters by requestedType, but the fallback guard at line 209–215 does not, making the type check asymmetric. An invisible same-type candidate should block creation; an invisible wrong-type candidate should allow it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Line 34323: The compact usage line for the visible-helper command is missing
the --working-directory flag that is actually parsed and documented elsewhere in
the codebase. Update the visible-helper usage line to include
--working-directory (or its short form if one exists) between the existing flags
to ensure consistency with the actual parser implementation and documentation,
so users can discover this option from the compact command list.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 92f3edec-6ccd-4f34-b9ae-7626175299eb
📒 Files selected for processing (5)
CLI/cmux.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Helper/ControlCommandCoordinator+Helper.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorHelperTests.swiftSources/TerminalController.swift
lawrencecchen
force-pushed
the
issue-6491-visible-helper-setup
branch
from
June 20, 2026 08:43
539cbc6 to
6f5d333
Compare
Contributor
Author
|
Updated divergence dogfood after adding the bounded surface-health retry. Proof:
|
lawrencecchen
force-pushed
the
issue-6491-visible-helper-setup
branch
from
June 20, 2026 09:10
6f5d333 to
fdd4f33
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorHelperTests.swift`:
- Around line 162-171: The controlSurfaceWaitForInWindow method in the test
double always returns true, which prevents testing the branch where this method
returns false. Add a configurable property (such as surfaceWindowWaitResult) to
the test double class that defaults to true, then modify the
controlSurfaceWaitForInWindow method to return this configurable property
instead of the hardcoded true value. This will allow tests to set the property
to false and verify the code path where no in-window event is observed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0bd354ea-dc9a-4cd8-bca9-a8432b770507
📒 Files selected for processing (12)
CLI/cmux.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Helper/ControlCommandCoordinator+Helper.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlSurfaceContext.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Wire/ControlCommandExecutionPolicy.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandContextTestStubs.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorHelperTests.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandExecutionPolicyTests.swiftSources/GhosttyTerminalView.swiftSources/Panels/BrowserPanelView.swiftSources/TerminalController+ControlSurfaceContext.swiftSources/TerminalController.swift
Comment on lines
+162
to
+171
| func controlSurfaceWaitForInWindow( | ||
| routing: ControlRoutingSelectors, | ||
| surfaceID: UUID | ||
| ) async -> Bool { | ||
| surfaceWindowWaits.append((routing, surfaceID)) | ||
| if let visibleAfterWindowEvent = createdSurfaceVisibleAfterWindowEvent { | ||
| createdSurfaceVisible = visibleAfterWindowEvent | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Make wait outcome configurable in the test double.
controlSurfaceWaitForInWindow always returns true, so you can’t cover the branch where no in-window event is observed (surface_window_event_observed == false). Add a configurable return value (e.g., surfaceWindowWaitResult) and a test for that path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorHelperTests.swift`
around lines 162 - 171, The controlSurfaceWaitForInWindow method in the test
double always returns true, which prevents testing the branch where this method
returns false. Add a configurable property (such as surfaceWindowWaitResult) to
the test double class that defaults to true, then modify the
controlSurfaceWaitForInWindow method to return this configurable property
instead of the hardcoded true value. This will allow tests to set the property
to false and verify the code path where no in-window event is observed.
lawrencecchen
force-pushed
the
issue-6491-visible-helper-setup
branch
from
June 20, 2026 09:24
fdd4f33 to
6838a0b
Compare
| if !rightSide.isEmpty { | ||
| return rightSide | ||
| } | ||
| return [] |
There was a problem hiding this comment.
Missing frames skip helper reuse
Medium Severity
When the focused pane has a pixelFrame but no non-focused pane passes the right-side geometry filter (including when candidate panes lack pixelFrame), helperVisibleOrderedCandidatePanes returns an empty list. Placement then skips reuse and blocked-invisible checks and always chooses create, which can add an extra right split beside an existing structural helper pane.
Reviewed by Cursor Bugbot for commit 6838a0b. Configure here.
Contributor
Author
|
Latest proof after commit 6838a0b:
This is the regression path from #6491: structural pane/surface existence is not accepted unless |
lawrencecchen
force-pushed
the
issue-6491-visible-helper-setup
branch
from
June 20, 2026 09:33
6838a0b to
4d4ef1f
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4d4ef1f. Configure here.
| reason: "didMoveToWindow" | ||
| ) else { return } | ||
| guard host.window != nil else { return } | ||
| browserPanel.postSurfaceHostedViewDidMoveToWindow() |
There was a problem hiding this comment.
Browser visibility check races bind
High Severity
For browser helpers, helper.visible waits on surfaceHostedViewDidMoveToWindow, then requires surface.health in_window=true via webView.window. The browser posts that notification before portal bind, so the health read can run while webView.window is still nil and the command fails with not_visible despite a valid helper surface.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 4d4ef1f. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Helper/ControlCommandCoordinator`+HelperPlacement.swift:
- Around line 102-127: The helperVisibleOrderedCandidatePanes function returns
Array(candidates.reversed()) as a fallback when focusedPane or focusedFrame is
unavailable, but this behavior lacks explanation. Add a clear comment above the
return statement on line 126 explaining why the candidate order is reversed in
this fallback case, what it represents in the context of helper pane placement,
and whether it reflects a deliberate ordering strategy or serves as a neutral
default when focused pane information is not available.
In `@Sources/TerminalController`+ControlSurfaceContext.swift:
- Around line 175-185: There is a race condition between the health check at the
start of the function and the notification subscription. If the surface moves to
window between the one-time health check (line 175) and when the notification
observer actually starts listening (line 182), the event will be missed and the
code may wait indefinitely. Reorder the logic by setting up the notification
subscription with NotificationCenter.default.notifications before performing the
health check, so that any surface visibility changes are captured either by the
health check or by the notification observer, closing the window where events
can be missed between these two steps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ab4e2a01-5f8d-4675-9b98-40019012d30f
📒 Files selected for processing (21)
CLI/CMUXCLI+V2Output.swiftCLI/CMUXCLI+VisibleHelper.swiftCLI/cmux.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Helper/ControlCommandCoordinator+Helper.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Helper/ControlCommandCoordinator+HelperPlacement.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Helper/ControlCommandCoordinator+HelperVisibility.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlSurfaceContext.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Wire/ControlCommandExecutionPolicy.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandContextTestStubs.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorHelperTestSupport.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorHelperTests.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandExecutionPolicyTests.swiftSources/GhosttyTerminalView.swiftSources/NotificationName+ControlSurfaceWindow.swiftSources/Panels/BrowserPanel+SurfaceHostedWindowNotification.swiftSources/Panels/BrowserPanelView.swiftSources/TerminalController+ControlSurfaceContext.swiftSources/TerminalController+SocketWorkerHelperVisible.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxproj
Comment on lines
+102
to
+127
| private func helperVisibleOrderedCandidatePanes( | ||
| _ candidates: [ControlPaneSummary], | ||
| in snapshot: ControlPaneListSnapshot, | ||
| focusedPaneID: UUID? | ||
| ) -> [ControlPaneSummary] { | ||
| if let focusedPane = focusedPaneID.flatMap({ id in snapshot.panes.first { $0.paneID == id } }), | ||
| let focusedFrame = focusedPane.pixelFrame { | ||
| let rightSide = candidates.filter { pane in | ||
| guard let frame = pane.pixelFrame else { return false } | ||
| return frame.x >= focusedFrame.x + (focusedFrame.width * 0.5) | ||
| }.sorted { lhs, rhs in | ||
| let lhsFrame = lhs.pixelFrame! | ||
| let rhsFrame = rhs.pixelFrame! | ||
| if lhsFrame.x == rhsFrame.x { | ||
| return lhsFrame.y < rhsFrame.y | ||
| } | ||
| return lhsFrame.x < rhsFrame.x | ||
| } | ||
| if !rightSide.isEmpty { | ||
| return rightSide | ||
| } | ||
| return [] | ||
| } | ||
|
|
||
| return Array(candidates.reversed()) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for comments or related logic explaining the reversed() fallback.
# Check for related comments or documentation
rg -nC5 'helperVisibleOrderedCandidatePanes|reversed.*candidate' --type=swiftRepository: manaflow-ai/cmux
Length of output: 3714
🏁 Script executed:
# Read the full file to understand the function context and purpose
wc -l "Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Helper/ControlCommandCoordinator+HelperPlacement.swift"Repository: manaflow-ai/cmux
Length of output: 190
🏁 Script executed:
# Get full file with context around the function
head -n 140 "Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Helper/ControlCommandCoordinator+HelperPlacement.swift" | tail -n +30Repository: manaflow-ai/cmux
Length of output: 4393
🏁 Script executed:
# Search for any design comments or related logic about ordering/placement strategy
rg -n 'placement|order|visible|reversed' "Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Helper/ControlCommandCoordinator+HelperPlacement.swift" -B2 -A2Repository: manaflow-ai/cmux
Length of output: 2119
Add a comment explaining the reversed() fallback behavior.
When focusedPane or focusedFrame is unavailable (line 126), the fallback returns Array(candidates.reversed()) without describing why reversing the candidate order is the intended behavior. The right-side spatial logic (lines 109–123) is clear, but the fallback strategy needs a comment explaining what "reversed" represents in the context of helper pane placement and whether it reflects a deliberate ordering strategy or a neutral default.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Helper/ControlCommandCoordinator`+HelperPlacement.swift
around lines 102 - 127, The helperVisibleOrderedCandidatePanes function returns
Array(candidates.reversed()) as a fallback when focusedPane or focusedFrame is
unavailable, but this behavior lacks explanation. Add a clear comment above the
return statement on line 126 explaining why the candidate order is reversed in
this fallback case, what it represents in the context of helper pane placement,
and whether it reflects a deliberate ordering strategy or serves as a neutral
default when focused pane information is not available.
Comment on lines
+175
to
+185
| if controlSurfaceHealth(routing: routing)? | ||
| .surfaces | ||
| .first(where: { $0.surfaceID == surfaceID })? | ||
| .inWindow == true { | ||
| return true | ||
| } | ||
|
|
||
| let notifications = NotificationCenter.default.notifications( | ||
| named: .surfaceHostedViewDidMoveToWindow, | ||
| object: nil | ||
| ) |
There was a problem hiding this comment.
Close the visibility wait race between health read and notification subscription.
Line 175 does a one-time health check before Line 182 starts observing notifications. If the surface becomes in-window between those two steps, the event can be missed and this loop may wait indefinitely for a later move event. That can stall helper.visible even though the surface is already visible.
Suggested fix
func controlSurfaceWaitForInWindow(
routing: ControlRoutingSelectors,
surfaceID: UUID
) async -> Bool {
- if controlSurfaceHealth(routing: routing)?
- .surfaces
- .first(where: { $0.surfaceID == surfaceID })?
- .inWindow == true {
- return true
- }
-
let notifications = NotificationCenter.default.notifications(
named: .surfaceHostedViewDidMoveToWindow,
object: nil
)
+ // Re-check after subscription starts to avoid missing a just-fired event.
+ if controlSurfaceHealth(routing: routing)?
+ .surfaces
+ .first(where: { $0.surfaceID == surfaceID })?
+ .inWindow == true {
+ return true
+ }
for await notification in notifications {
if Task.isCancelled {
return false
}
guard let hostedSurfaceID = notification.userInfo?["surfaceId"] as? UUID,
hostedSurfaceID == surfaceID else {
continue
}
return true
}
return false
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/TerminalController`+ControlSurfaceContext.swift around lines 175 -
185, There is a race condition between the health check at the start of the
function and the notification subscription. If the surface moves to window
between the one-time health check (line 175) and when the notification observer
actually starts listening (line 182), the event will be missed and the code may
wait indefinitely. Reorder the logic by setting up the notification subscription
with NotificationCenter.default.notifications before performing the health
check, so that any surface visibility changes are captured either by the health
check or by the notification observer, closing the window where events can be
missed between these two steps.
Contributor
Author
|
Final proof after commit 4d4ef1f5c6ffbb5845c0d6ad890226362f81b67b:
This tests the exact failure mode from #6491: structural pane/surface existence is insufficient unless |
Comment on lines
+171
to
+210
| func controlSurfaceWaitForInWindow( | ||
| routing: ControlRoutingSelectors, | ||
| surfaceID: UUID | ||
| ) async -> Bool { | ||
| if controlSurfaceHealth(routing: routing)? | ||
| .surfaces | ||
| .first(where: { $0.surfaceID == surfaceID })? | ||
| .inWindow == true { | ||
| return true | ||
| } | ||
|
|
||
| let (surfaceIDs, surfaceIDContinuation) = AsyncStream<UUID>.makeStream( | ||
| bufferingPolicy: .bufferingNewest(1) | ||
| ) | ||
| let observer = NotificationCenter.default.addObserver( | ||
| forName: .surfaceHostedViewDidMoveToWindow, | ||
| object: nil, | ||
| queue: nil | ||
| ) { notification in | ||
| guard let hostedSurfaceID = notification.userInfo?["surfaceId"] as? UUID else { | ||
| return | ||
| } | ||
| surfaceIDContinuation.yield(hostedSurfaceID) | ||
| } | ||
| defer { | ||
| surfaceIDContinuation.finish() | ||
| NotificationCenter.default.removeObserver(observer) | ||
| } | ||
|
|
||
| for await hostedSurfaceID in surfaceIDs { | ||
| if Task.isCancelled { | ||
| return false | ||
| } | ||
| guard hostedSurfaceID == surfaceID else { | ||
| continue | ||
| } | ||
| return true | ||
| } | ||
| return false | ||
| } |
Contributor
There was a problem hiding this comment.
The initial health check (line 175) fires before the NotificationCenter observer is installed (line 185). If surfaceHostedViewDidMoveToWindow posts between those two points — which is plausible because paneCreate dispatches UI work to the main actor and that work can complete before the socket worker resumes here — the notification is never yielded to the AsyncStream and the wait hangs until the 2-second outer timeout cancels the task, producing a spurious not_visible or timeout error even though the pane was created successfully.
The correct pattern is: create the stream and register the observer first, then do the initial health check. Any notification that fires after observer registration but before the for await begins is buffered (the policy already uses .bufferingNewest(1)), so the fast path still works without data races.
Contributor
Author
|
Additional tagged dogfood with real Claude/Codex sessions on
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/CMUXCLI`+VisibleHelper.swift:
- Around line 112-123: Update callerContextFromEnvironment() to return [String:
Any] instead of an optional dictionary, returning caller directly so empty
results produce an empty collection. Adjust its corresponding call site to
remove optional binding and handle the non-optional dictionary while preserving
existing caller-context behavior.
In
`@Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Helper/ControlCommandCoordinator`+HelperVisibility.swift:
- Around line 84-93: Update the failed-send handling in
helperVisibleCommandError and the related path around the additional referenced
branch so API responses contain only safe status metadata: remove the command
and any raw downstream sendResult/error payload from the returned response.
Preserve sent_command as false and keep detailed diagnostics only in sanitized
internal logs.
- Around line 101-103: Update helperVisibleCommandText to retrieve the raw
“command” or “initial_command” value without trimming and return it unchanged.
Perform any required non-empty validation using a trimmed copy, while preserving
all leading, trailing, and multiline whitespace in the returned command text.
In `@Sources/TerminalController`+ControlSurfaceContext.swift:
- Around line 183-199: Replace the SurfaceHostedWindowWait-based logic in
controlSurfaceWaitForInWindow with an async readiness completion owned by the
terminal/browser hosted-view lifecycle. Thread that completion through
hosted-view creation, await it after the initial visibility check, then confirm
readiness using controlSurfaceHealth. Remove the process-wide notification and
fixed 1.5-second timeout from this synchronization path, keeping lifecycle state
under the explicit owner.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 31d0c73a-762c-45e6-906f-00ea88b6154f
📒 Files selected for processing (29)
CLI/CMUXCLI+V2Output.swiftCLI/CMUXCLI+VisibleHelper.swiftCLI/cmux.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/ControlCommandCoordinator.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Helper/ControlCommandCoordinator+Helper.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Helper/ControlCommandCoordinator+HelperPlacement.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Helper/ControlCommandCoordinator+HelperVisibility.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Helper/ControlCommandCoordinator+HelperVisibleIdentify.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Helper/ControlCommandCoordinator+HelperVisiblePlacement.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Pane/ControlPaneContext.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Pane/ControlPaneListSnapshot.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlSurfaceContext.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlSurfaceHealthEntry.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlSurfaceHealthSnapshot.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Wire/ControlCommandExecutionPolicy.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandContextTestStubs.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorHelperTestSupport.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorHelperTests.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandExecutionPolicyTests.swiftSources/GhosttyTerminalView.swiftSources/NotificationName+ControlSurfaceWindow.swiftSources/Panels/BrowserPanel+SurfaceHostedWindowNotification.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/TerminalController+ControlPaneContext.swiftSources/TerminalController+ControlSurfaceContext.swiftSources/TerminalController+SocketWorkerHelperVisible.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxproj
Comment on lines
+112
to
+123
| private func callerContextFromEnvironment() -> [String: Any]? { | ||
| let environment = ProcessInfo.processInfo.environment | ||
| var caller: [String: Any] = [:] | ||
| if let workspace = environment["CMUX_WORKSPACE_ID"]?.nilIfEmpty { | ||
| caller["workspace_id"] = workspace | ||
| } | ||
| if let surface = (environment["CMUX_SURFACE_ID"] ?? environment["CMUX_TAB_ID"])?.nilIfEmpty { | ||
| caller["surface_id"] = surface | ||
| caller["tab_id"] = surface | ||
| } | ||
| return caller.isEmpty ? nil : caller | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Prefer empty collection over optional collection.
As highlighted by SwiftLint, it is idiomatic in Swift to return an empty collection rather than an optional collection. This simplifies the return type and avoids the need for optional binding at the call site.
♻️ Proposed refactor
- private func callerContextFromEnvironment() -> [String: Any]? {
+ private func callerContextFromEnvironment() -> [String: Any] {
let environment = ProcessInfo.processInfo.environment
var caller: [String: Any] = [:]
if let workspace = environment["CMUX_WORKSPACE_ID"]?.nilIfEmpty {
caller["workspace_id"] = workspace
}
if let surface = (environment["CMUX_SURFACE_ID"] ?? environment["CMUX_TAB_ID"])?.nilIfEmpty {
caller["surface_id"] = surface
caller["tab_id"] = surface
}
- return caller.isEmpty ? nil : caller
+ return caller
}Update the corresponding call site (lines 31-34) to match:
var params: [String: Any] = ["target": "focused"]
- if let caller = callerContextFromEnvironment() {
+ let caller = callerContextFromEnvironment()
+ if !caller.isEmpty {
params["caller"] = caller
}🧰 Tools
🪛 SwiftLint (0.65.0)
[Warning] 112-112: Prefer empty collection over optional collection
(discouraged_optional_collection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLI/CMUXCLI`+VisibleHelper.swift around lines 112 - 123, Update
callerContextFromEnvironment() to return [String: Any] instead of an optional
dictionary, returning caller directly so empty results produce an empty
collection. Adjust its corresponding call site to remove optional binding and
handle the non-optional dictionary while preserving existing caller-context
behavior.
Source: Linters/SAST tools
Comment on lines
+84
to
+93
| guard case .ok(.object(let sendPayload)) = sendResult else { | ||
| payload["sent_command"] = .bool(false) | ||
| payload["command"] = .string(command) | ||
| return helperVisibleCommandError( | ||
| sendResult, | ||
| identify: identify, | ||
| focusedWorkspaceID: focusedWorkspaceID, | ||
| focusedWindowID: focusedWindowID, | ||
| payload: payload | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not echo commands or raw downstream errors in the API response.
A failed send currently returns the complete command plus unsanitized nested error message/data. Commands may contain credentials or private content; expose only safe status metadata and keep detailed diagnostics in sanitized logs.
As per coding guidelines, user-facing API errors must not expose raw payloads or upstream messages.
Proposed sanitization
- payload["command"] = .string(command)
+ payload["command_present"] = .bool(true)
+ payload["command_bytes"] = .int(Int64(command.utf8.count))
...
- if case .err(let code, let message, let data) = sendResult {
+ if case .err(let code, _, _) = sendResult {
extra["send_error"] = .object([
"code": .string(code),
- "message": .string(message),
- "data": data ?? .null,
])
}Also applies to: 126-147
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Helper/ControlCommandCoordinator`+HelperVisibility.swift
around lines 84 - 93, Update the failed-send handling in
helperVisibleCommandError and the related path around the additional referenced
branch so API responses contain only safe status metadata: remove the command
and any raw downstream sendResult/error payload from the returned response.
Preserve sent_command as false and keep detailed diagnostics only in sanitized
internal logs.
Source: Coding guidelines
Comment on lines
+101
to
+103
| private func helperVisibleCommandText(_ params: [String: JSONValue]) -> String? { | ||
| string(params, "command") ?? string(params, "initial_command") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the command bytes instead of trimming them.
string(...) strips leading and trailing whitespace, changing shell semantics such as history-suppression prefixes and multiline commands. Validate with a trimmed copy but return the original value.
Proposed fix
private func helperVisibleCommandText(_ params: [String: JSONValue]) -> String? {
- string(params, "command") ?? string(params, "initial_command")
+ for key in ["command", "initial_command"] {
+ guard case .string(let raw)? = params[key] else { continue }
+ if !raw.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty {
+ return raw
+ }
+ }
+ return nil
}📝 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.
Suggested change
| private func helperVisibleCommandText(_ params: [String: JSONValue]) -> String? { | |
| string(params, "command") ?? string(params, "initial_command") | |
| } | |
| private func helperVisibleCommandText(_ params: [String: JSONValue]) -> String? { | |
| for key in ["command", "initial_command"] { | |
| guard case .string(let raw)? = params[key] else { continue } | |
| if !raw.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { | |
| return raw | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Helper/ControlCommandCoordinator`+HelperVisibility.swift
around lines 101 - 103, Update helperVisibleCommandText to retrieve the raw
“command” or “initial_command” value without trimming and return it unchanged.
Perform any required non-empty validation using a trimmed copy, while preserving
all leading, trailing, and multiline whitespace in the returned command text.
Comment on lines
+183
to
+199
| func controlSurfaceWaitForInWindow( | ||
| routing: ControlRoutingSelectors, | ||
| surfaceID: UUID | ||
| ) async -> Bool { | ||
| if controlSurfaceIsVisibleInTargetUI(routing: routing, surfaceID: surfaceID) { | ||
| return true | ||
| } | ||
|
|
||
| return await SurfaceHostedWindowWait( | ||
| surfaceID: surfaceID, | ||
| isVisible: { [weak self] in | ||
| self?.controlSurfaceIsVisibleInTargetUI(routing: routing, surfaceID: surfaceID) == true | ||
| } | ||
| ).wait( | ||
| timeout: .milliseconds(1_500) | ||
| ) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Replace the notification-and-timeout readiness repair with an owner-held completion signal.
This waits on a process-wide notification and a fixed 1.5-second clock. If the move event arrives before layout makes isVisible true, it is discarded and readiness depends on the timeout, causing latency or false failures. Thread an async readiness completion from the terminal/browser hosted-view owner through creation, then confirm with controlSurfaceHealth.
As per coding guidelines, lifecycle/rendering synchronization must not use notification waits or fixed delays to repair races, and lifecycle state must remain under one explicit owner.
Also applies to: 244-301
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/TerminalController`+ControlSurfaceContext.swift around lines 183 -
199, Replace the SurfaceHostedWindowWait-based logic in
controlSurfaceWaitForInWindow with an async readiness completion owned by the
terminal/browser hosted-view lifecycle. Thread that completion through
hosted-view creation, await it after the initial visibility check, then confirm
readiness using controlSurfaceHealth. Remove the process-wide notification and
fixed 1.5-second timeout from this synchronization path, keeping lifecycle state
under the explicit owner.
Source: Coding guidelines
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
helper.visibleandcmux visible-helperas the focused-workspace helper setup path.target_workspace_source=focusedandcaller_focused_diverged.surface.health in_window=trueas visible, waiting on a hosted-view window event for newly created surfaces and failing loudly when structural helper state is not actually visible.Testing
swift test --package-path Packages/macOS/CmuxControlSocket --filter ControlCommandCoordinatorHelperTestspassed, 12 tests.swift test --package-path Packages/macOS/CmuxControlSocket --filter ControlCommandExecutionPolicyTestspassed, 5 tests../scripts/reload-cloud.sh --tag vh6491passed, run https://github.com/manaflow-ai/cmux/actions/runs/27866417817./tmp/cmux-debug-vh6491.sock: fake callerworkspace:8, focusedworkspace:9;visible-helperreturnedcaller_focused_diverged=true,target_workspace_source=focused,workspace_ref=workspace:9,surface_ref=surface:13,surface_health_in_window=true, andsurface_window_event_observed=true.visible-helperreusedpane:13/surface:13,created_pane=false,created_surface=false; focused pane count stayed 2.surface-health --workspace workspace:9showedsurface:13 type=terminal in_window=true;read-screen --workspace workspace:9 --surface surface:13showed6491-event-visible-helper; callerworkspace:8still had only its original pane./var/folders/rr/vmfx6xh12dz2tlvgtmyvjmf80000gn/T/cmux-screenshots/vh6491-visible-helper-proof_2026-06-20T09-10-17Z_1DF84E3B.png.Issues
Summary by CodeRabbit
Release Notes
New Features
cmux visible-helperCLI subcommand to create or reuse helper panes/surfaces and to send an optional initial command only after the target becomes visible/in-window (supportsterminalandbrowser).helper.visibleto the socket v2 capability set to coordinate helper placement and visibility before sending commands.Documentation
visible-helper.Tests
Note
Medium Risk
Changes pane/split layout and automation routing (focused vs caller workspace) with async visibility gates; risk is mitigated by reusing existing pane/surface paths and broad unit tests, but wrong targeting could still confuse agents or duplicate panes in edge layouts.
Overview
Adds
cmux visible-helperand thehelper.visiblesocket v2 method so automation can create or reuse a right-side helper pane in the visually focused workspace (not the caller’sCMUX_*context), with responses that spell outtarget_workspace_source=focusedandcaller_focused_diverged.The coordinator reuses a visible right-side pane/surface of the requested type when possible, otherwise
pane.createwithdirection=rightandfocus=false. Success requiressurface.healthin_window=true; new surfaces await asurfaceHostedViewDidMoveToWindownotification (terminal + browser) before verifying health and optionallysurface.send_textfor--command/initial_command.helper.visibleruns on the socket worker viahandleAsyncso waits do not block the main actor.CLI help, command lists, and
printV2Outputextraction are included; 12 coordinator helper tests cover reuse, create, visibility failures, and command send errors.Reviewed by Cursor Bugbot for commit 4d4ef1f. Bugbot is set up for automated code reviews on this repo. Configure here.