Add sticky terminal overlay extension surface - #9449
Closed
lawrencecchen wants to merge 7 commits into
Closed
lawrencecchen wants to merge 7 commits into
lawrencecchen wants to merge 7 commits into
Conversation
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
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 retained terminal overlays with viewport and scrollback placement, socket commands, CLI management, localization, Codex prompt publication, rendering, and unit, coordinator, hook, and UI tests. ChangesTerminal overlay feature
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 21❌ Failed checks (1 warning, 20 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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`:
- Around line 31680-31693: Update the catch block surrounding
publishLatestCodexUserMessageOverlay to include String(describing: error) in the
telemetry.breadcrumb data, while preserving the existing breadcrumb name and
overlay failure handling.
In `@CLI/CMUXCLI`+TerminalOverlay.swift:
- Around line 78-93: After the stdin-or-joined-token resolution in the overlay
set command, validate the resulting text is non-empty before calling
surface.overlay.set. Reuse the existing setRequiresText CLIError for empty stdin
content, while preserving the current handling of non-empty stdin and regular
text tokens.
- Around line 4-36: Update the surfaceOverlayCommandHelp text to document the --
argument terminator supported by overlayArguments(splitAtTerminator:), including
that it allows overlay text beginning with -- to bypass unknown-flag parsing.
Add a concise usage example or instruction near the existing stdin/text guidance
without changing command behavior.
In `@cmuxUITests/AutomationSocketUITests.swift`:
- Around line 151-273: Extend
testTerminalOverlaySocketRendersUpdatesAndRemovesPassiveCard to exercise input
pass-through while the overlay card exists: send terminal keyboard input and
mouse input targeting surfaceID after the card renders, then assert the socket
responses or observable terminal state confirm both inputs reached that surface
before calling surface.overlay.remove. Keep the existing render, update, list,
and removal assertions unchanged.
In
`@Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlCommandCoordinator`+SurfaceOverlay.swift:
- Around line 140-153: The overlayStrings fallback currently produces an empty
tabManagerUnavailable message when context is nil. Update the surface overlay
dispatch around overlayStrings and surfaceOverlay to return the established
unavailable response before dispatch, or require context availability, while
preserving normal context-resolved strings. Add a test covering context absence
and ensure socket strings are resolved through the app context rather than
direct package-bundle localization.
In
`@Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface`+Overlay.swift:
- Around line 51-57: Update removeAllTerminalOverlays to pass
terminalOverlayStore.overlays to paneHost.setTerminalOverlays instead of the
literal empty array, matching the snapshot behavior used by the other overlay
mutators.
In
`@Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/Overlay/TerminalOverlay.swift`:
- Around line 125-136: Update normalizedText in
Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/Overlay/TerminalOverlay.swift#L125-L136
to consume each complete escape sequence following ESC and filter only Cc
controls, preserving Cf scalars such as ZWJ and bidi marks. Update the expected
escape-sequence output in
Packages/macOS/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/TerminalOverlayTests.swift#L5-L17
from "first\n[31msecond" to "first\nsecond", and add coverage confirming a ZWJ
emoji sequence remains intact.
In
`@Packages/macOS/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/TerminalOverlayTests.swift`:
- Around line 5-17: The TerminalOverlayRequest tests lack coverage for clamping
maximum dimensions. Extend validatesAndNormalizesProducerInput, or add a focused
test, with out-of-range maximumWidthColumns and maximumHeightRows values, then
assert they normalize to 16...200 and 1...50 respectively.
In `@Resources/Localizable.xcstrings`:
- Around line 35189-35358: The new cli.surfaceOverlay.* and
socket.surfaceOverlay.* localization entries currently contain only en and ja;
add translated stringUnit values for every remaining supported locale in
Resources/Localizable.xcstrings, preserving each key’s existing placeholders and
formatting.
In `@Sources/TerminalOverlayView.swift`:
- Around line 62-66: Promote OverlayMetrics from the private
GhosttySurfaceScrollView extension to file scope, then update the
maximumCardWidth calculation to subtract OverlayMetrics.edgeMargin * 2 instead
of the literal 16. Keep the width calculation and placement margins synchronized
by reusing this shared metric.
- Around line 250-259: Compute documentHeight() once before the scrollback
overlay loop, alongside scrollbackGeometry, then reuse that stored value in the
TerminalOverlayGeometry.scrollbackOverlayOriginY call instead of invoking
documentHeight() for each overlay.
- Around line 53-109: Add a measurement cache to TerminalOverlayView’s apply
method keyed by overlay.text, cellSize, and availableWidth, and reuse the cached
CGSize when all inputs match. Store the computed size after measurement and
reset the cache when the maximumCardWidth early return produces .zero. Keep
label content and frame updates applying on every call while avoiding repeated
text measurement during scrolling.
🪄 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 Plus
Run ID: 6637b7c3-30c3-4411-b407-26be88b5dc2d
📒 Files selected for processing (21)
CLI/CMUXCLI+TerminalOverlay.swiftCLI/cmux.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlCommandCoordinator+Surface.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlCommandCoordinator+SurfaceOverlay.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlSurfaceContext.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlSurfaceOverlay.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandContextTestStubs.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorSurfaceTests.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/FakeSurfaceControlCommandContext.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Hosting/TerminalSurfacePaneHosting.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+Overlay.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface.swiftPackages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/Overlay/TerminalOverlay.swiftPackages/macOS/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/TerminalOverlayTests.swiftResources/Localizable.xcstringsSources/GhosttyTerminalView.swiftSources/TerminalController+ControlSurfaceOverlay.swiftSources/TerminalController.swiftSources/TerminalOverlayView.swiftcmux.xcodeproj/project.pbxprojcmuxUITests/AutomationSocketUITests.swift
Comment on lines
+31680
to
+31693
| } | ||
| if def.name == "codex", | ||
| let prompt = feedPromptText(from: input.rawObject ?? input.object) { | ||
| do { | ||
| try publishLatestCodexUserMessageOverlay( | ||
| prompt, | ||
| workspaceId: workspaceId, | ||
| surfaceId: surfaceId, | ||
| client: client | ||
| ) | ||
| } catch { | ||
| telemetry.breadcrumb("codex-hook.prompt-submit.overlay-failed") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Capture the error when the overlay publish fails.
The catch block discards error and records only the static string "codex-hook.prompt-submit.overlay-failed". Other failure paths in this function preserve the error detail, for example telemetry.breadcrumb("claude-hook.stop.ignored", data: ["error": String(describing: error)]) and reportAgentHookFailure(..., error: error, ...). Without the error detail, a failed overlay publish is not diagnosable from telemetry alone.
🐛 Proposed fix to preserve the error detail
} catch {
- telemetry.breadcrumb("codex-hook.prompt-submit.overlay-failed")
+ telemetry.breadcrumb(
+ "codex-hook.prompt-submit.overlay-failed",
+ data: ["error": String(describing: error)]
+ )
}📝 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
| } | |
| if def.name == "codex", | |
| let prompt = feedPromptText(from: input.rawObject ?? input.object) { | |
| do { | |
| try publishLatestCodexUserMessageOverlay( | |
| prompt, | |
| workspaceId: workspaceId, | |
| surfaceId: surfaceId, | |
| client: client | |
| ) | |
| } catch { | |
| telemetry.breadcrumb("codex-hook.prompt-submit.overlay-failed") | |
| } | |
| } | |
| } | |
| if def.name == "codex", | |
| let prompt = feedPromptText(from: input.rawObject ?? input.object) { | |
| do { | |
| try publishLatestCodexUserMessageOverlay( | |
| prompt, | |
| workspaceId: workspaceId, | |
| surfaceId: surfaceId, | |
| client: client | |
| ) | |
| } catch { | |
| telemetry.breadcrumb( | |
| "codex-hook.prompt-submit.overlay-failed", | |
| data: ["error": String(describing: error)] | |
| ) | |
| } | |
| } |
🤖 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/cmux.swift` around lines 31680 - 31693, Update the catch block
surrounding publishLatestCodexUserMessageOverlay to include String(describing:
error) in the telemetry.breadcrumb data, while preserving the existing
breadcrumb name and overlay failure handling.
Comment on lines
+4
to
+36
| static let surfaceOverlayCommandUsageLine = String( | ||
| localized: "cli.surfaceOverlay.usageLine", | ||
| defaultValue: "surface overlay <set|list|remove|clear> [--workspace <id|ref|index>] [--surface <id|ref|index>] [--window <id|ref|index>]" | ||
| ) | ||
|
|
||
| static let surfaceOverlayCommandHelp = String( | ||
| localized: "cli.surfaceOverlay.help", | ||
| defaultValue: """ | ||
| Usage: cmux surface overlay set <id> <text> [--anchor <viewport|scrollback>] [--position <left|center|right>] [target flags] | ||
| cmux surface overlay list [target flags] | ||
| cmux surface overlay remove <id> [target flags] | ||
| cmux surface overlay clear [target flags] | ||
|
|
||
| Render passive text above a terminal without taking keyboard or mouse input. | ||
| A viewport overlay stays at the visible top. A scrollback overlay captures the current top row. | ||
|
|
||
| Target flags: | ||
| --workspace <id|ref|index> Workspace context (default: $CMUX_WORKSPACE_ID) | ||
| --surface <id|ref|index> Terminal context (default: $CMUX_SURFACE_ID) | ||
| --window <id|ref|index> Window context for workspace and surface refs/indexes | ||
|
|
||
| Set flags: | ||
| --anchor <viewport|scrollback> Vertical anchor (default: viewport) | ||
| --position <left|center|right> Horizontal position (default: center) | ||
|
|
||
| Use '-' as text to read the overlay from standard input. | ||
|
|
||
| Examples: | ||
| cmux surface overlay set latest-message "check the auth error" | ||
| printf 'build\\npassed' | cmux surface overlay set build-status - --position right | ||
| cmux surface overlay set review-note "inspect this output" --anchor scrollback --position left | ||
| """ | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Approve help-text constants; document the -- terminator convention.
The -- terminator, used by overlayArguments(splitAtTerminator:) to let literal text through the "unknown flag" check (Lines 69-77, 212-219), is never mentioned in surfaceOverlayCommandHelp. A user typing overlay text that starts with -- gets an "unknown flag" error with no documented workaround.
📝 Suggested help text addition
Use '-' as text to read the overlay from standard input.
+ Use '--' before text to stop flag parsing, so text can start with '--'.📝 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
| static let surfaceOverlayCommandUsageLine = String( | |
| localized: "cli.surfaceOverlay.usageLine", | |
| defaultValue: "surface overlay <set|list|remove|clear> [--workspace <id|ref|index>] [--surface <id|ref|index>] [--window <id|ref|index>]" | |
| ) | |
| static let surfaceOverlayCommandHelp = String( | |
| localized: "cli.surfaceOverlay.help", | |
| defaultValue: """ | |
| Usage: cmux surface overlay set <id> <text> [--anchor <viewport|scrollback>] [--position <left|center|right>] [target flags] | |
| cmux surface overlay list [target flags] | |
| cmux surface overlay remove <id> [target flags] | |
| cmux surface overlay clear [target flags] | |
| Render passive text above a terminal without taking keyboard or mouse input. | |
| A viewport overlay stays at the visible top. A scrollback overlay captures the current top row. | |
| Target flags: | |
| --workspace <id|ref|index> Workspace context (default: $CMUX_WORKSPACE_ID) | |
| --surface <id|ref|index> Terminal context (default: $CMUX_SURFACE_ID) | |
| --window <id|ref|index> Window context for workspace and surface refs/indexes | |
| Set flags: | |
| --anchor <viewport|scrollback> Vertical anchor (default: viewport) | |
| --position <left|center|right> Horizontal position (default: center) | |
| Use '-' as text to read the overlay from standard input. | |
| Examples: | |
| cmux surface overlay set latest-message "check the auth error" | |
| printf 'build\\npassed' | cmux surface overlay set build-status - --position right | |
| cmux surface overlay set review-note "inspect this output" --anchor scrollback --position left | |
| """ | |
| ) | |
| static let surfaceOverlayCommandUsageLine = String( | |
| localized: "cli.surfaceOverlay.usageLine", | |
| defaultValue: "surface overlay <set|list|remove|clear> [--workspace <id|ref|index>] [--surface <id|ref|index>] [--window <id|ref|index>]" | |
| ) | |
| static let surfaceOverlayCommandHelp = String( | |
| localized: "cli.surfaceOverlay.help", | |
| defaultValue: """ | |
| Usage: cmux surface overlay set <id> <text> [--anchor <viewport|scrollback>] [--position <left|center|right>] [target flags] | |
| cmux surface overlay list [target flags] | |
| cmux surface overlay remove <id> [target flags] | |
| cmux surface overlay clear [target flags] | |
| Render passive text above a terminal without taking keyboard or mouse input. | |
| A viewport overlay stays at the visible top. A scrollback overlay captures the current top row. | |
| Target flags: | |
| --workspace <id|ref|index> Workspace context (default: $CMUX_WORKSPACE_ID) | |
| --surface <id|ref|index> Terminal context (default: $CMUX_SURFACE_ID) | |
| --window <id|ref|index> Window context for workspace and surface refs/indexes | |
| Set flags: | |
| --anchor <viewport|scrollback> Vertical anchor (default: viewport) | |
| --position <left|center|right> Horizontal position (default: center) | |
| Use '-' as text to read the overlay from standard input. | |
| Use '--' before text to stop flag parsing, so text can start with '--'. | |
| Examples: | |
| cmux surface overlay set latest-message "check the auth error" | |
| printf 'build\\npassed' | cmux surface overlay set build-status - --position right | |
| cmux surface overlay set review-note "inspect this output" --anchor scrollback --position left | |
| """ | |
| ) |
🤖 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`+TerminalOverlay.swift around lines 4 - 36, Update the
surfaceOverlayCommandHelp text to document the -- argument terminator supported
by overlayArguments(splitAtTerminator:), including that it allows overlay text
beginning with -- to bypass unknown-flag parsing. Add a concise usage example or
instruction near the existing stdin/text guidance without changing command
behavior.
Comment on lines
+78
to
+93
| let textTokens = Array(split.before.dropFirst()) + split.after | ||
| guard !textTokens.isEmpty else { | ||
| throw CLIError(message: String( | ||
| localized: "cli.surfaceOverlay.error.setRequiresText", | ||
| defaultValue: "surface overlay set requires text or '-' for standard input" | ||
| )) | ||
| } | ||
| let text: String | ||
| if textTokens == ["-"] { | ||
| text = String( | ||
| data: FileHandle.standardInput.readDataToEndOfFile(), | ||
| encoding: .utf8 | ||
| ) ?? "" | ||
| } else { | ||
| text = textTokens.joined(separator: " ") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard against empty text after reading standard input.
textTokens.isEmpty is checked at Line 79, before - is resolved to stdin content. If textTokens == ["-"] and standard input is empty or closed, text becomes "" at Line 90, and the command proceeds to call surface.overlay.set with empty text, unlike publishLatestCodexUserMessageOverlay, which explicitly skips publishing when boundedPrompt is empty after trimming (Line 178).
Add the same empty-content check after resolving text from stdin, so set - with empty input fails with a clear error instead of creating a blank overlay.
🛡️ Suggested fix
} else {
text = textTokens.joined(separator: " ")
}
+ guard !text.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else {
+ throw CLIError(message: String(
+ localized: "cli.surfaceOverlay.error.setRequiresText",
+ defaultValue: "surface overlay set requires text or '-' for standard input"
+ ))
+ }
params["overlay_id"] = id📝 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
| let textTokens = Array(split.before.dropFirst()) + split.after | |
| guard !textTokens.isEmpty else { | |
| throw CLIError(message: String( | |
| localized: "cli.surfaceOverlay.error.setRequiresText", | |
| defaultValue: "surface overlay set requires text or '-' for standard input" | |
| )) | |
| } | |
| let text: String | |
| if textTokens == ["-"] { | |
| text = String( | |
| data: FileHandle.standardInput.readDataToEndOfFile(), | |
| encoding: .utf8 | |
| ) ?? "" | |
| } else { | |
| text = textTokens.joined(separator: " ") | |
| } | |
| let textTokens = Array(split.before.dropFirst()) + split.after | |
| guard !textTokens.isEmpty else { | |
| throw CLIError(message: String( | |
| localized: "cli.surfaceOverlay.error.setRequiresText", | |
| defaultValue: "surface overlay set requires text or '-' for standard input" | |
| )) | |
| } | |
| let text: String | |
| if textTokens == ["-"] { | |
| text = String( | |
| data: FileHandle.standardInput.readDataToEndOfFile(), | |
| encoding: .utf8 | |
| ) ?? "" | |
| } else { | |
| text = textTokens.joined(separator: " ") | |
| } | |
| guard !text.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else { | |
| throw CLIError(message: String( | |
| localized: "cli.surfaceOverlay.error.setRequiresText", | |
| defaultValue: "surface overlay set requires text or '-' for standard input" | |
| )) | |
| } |
🤖 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`+TerminalOverlay.swift around lines 78 - 93, After the
stdin-or-joined-token resolution in the overlay set command, validate the
resulting text is non-empty before calling surface.overlay.set. Reuse the
existing setRequiresText CLIError for empty stdin content, while preserving the
current handling of non-empty stdin and regular text tokens.
Comment on lines
+151
to
+273
| func testTerminalOverlaySocketRendersUpdatesAndRemovesPassiveCard() throws { | ||
| let workingDirectory = FileManager.default.temporaryDirectory | ||
| .appendingPathComponent("cmux-ui-terminal-overlay-\(UUID().uuidString)", isDirectory: true) | ||
| try FileManager.default.createDirectory( | ||
| at: workingDirectory, | ||
| withIntermediateDirectories: true | ||
| ) | ||
| temporaryRoots.append(workingDirectory) | ||
|
|
||
| let app = XCUIApplication.cmuxTestApplication() | ||
| app.launchArguments += [ | ||
| "-\(modeKey)", "allowAll", | ||
| "-AppleLanguages", "(en)", | ||
| "-AppleLocale", "en_US", | ||
| ] | ||
| app.launchEnvironment["CMUX_UI_TEST_MODE"] = "1" | ||
| app.launchEnvironment["CMUX_SOCKET_ENABLE"] = "1" | ||
| app.launchEnvironment["CMUX_SOCKET_MODE"] = "allowAll" | ||
| app.launchEnvironment["CMUX_SOCKET_PATH"] = socketPath | ||
| app.launchEnvironment["CMUX_ALLOW_SOCKET_OVERRIDE"] = "1" | ||
| app.launchEnvironment["CMUX_UI_TEST_SOCKET_SANITY"] = "1" | ||
| app.launchEnvironment["CMUX_UI_TEST_DIAGNOSTICS_PATH"] = diagnosticsPath | ||
| app.launchEnvironment["CMUX_TAG"] = launchTag | ||
| defer { app.terminate() } | ||
| app.launch() | ||
|
|
||
| XCTAssertTrue( | ||
| ensureForegroundAfterLaunch(app, timeout: 12.0), | ||
| "Expected app to launch for terminal overlay test. state=\(app.state.rawValue)" | ||
| ) | ||
| XCTAssertTrue( | ||
| waitForSocketPong(timeout: 12.0), | ||
| "Expected socket ping at \(socketPath). diagnostics=\(loadDiagnostics())" | ||
| ) | ||
|
|
||
| let workspace = try XCTUnwrap( | ||
| socketResult( | ||
| method: "workspace.create", | ||
| params: [ | ||
| "title": "Terminal overlay XCUITest", | ||
| "working_directory": workingDirectory.path, | ||
| "focus": true, | ||
| ] | ||
| ), | ||
| "Expected workspace.create to succeed" | ||
| ) | ||
| let surfaceID = try XCTUnwrap( | ||
| workspace["surface_id"] as? String, | ||
| "Expected workspace.create to return a terminal surface" | ||
| ) | ||
|
|
||
| let overlayID = "xcuitest.latest-message" | ||
| let initialText = "Latest user message: check the auth error" | ||
| let initial = try XCTUnwrap( | ||
| socketResult( | ||
| method: "surface.overlay.set", | ||
| params: [ | ||
| "surface_id": surfaceID, | ||
| "overlay_id": overlayID, | ||
| "text": initialText, | ||
| "anchor": "viewport", | ||
| "position": "center", | ||
| ] | ||
| ), | ||
| "Expected surface.overlay.set to succeed" | ||
| ) | ||
| XCTAssertEqual( | ||
| (initial["overlay"] as? [String: Any])?["id"] as? String, | ||
| overlayID | ||
| ) | ||
|
|
||
| let card = app.staticTexts["terminal-overlay-\(overlayID)"] | ||
| XCTAssertTrue( | ||
| card.waitForExistence(timeout: 8.0), | ||
| "Expected the passive terminal overlay card to render" | ||
| ) | ||
| XCTAssertEqual(card.label, initialText) | ||
|
|
||
| let updatedText = "Latest user message: rerun the focused test" | ||
| _ = try XCTUnwrap( | ||
| socketResult( | ||
| method: "surface.overlay.set", | ||
| params: [ | ||
| "surface_id": surfaceID, | ||
| "overlay_id": overlayID, | ||
| "text": updatedText, | ||
| "anchor": "viewport", | ||
| "position": "right", | ||
| ] | ||
| ), | ||
| "Expected keyed surface.overlay.set update to succeed" | ||
| ) | ||
| let updated = expectation( | ||
| for: NSPredicate(format: "label == %@", updatedText), | ||
| evaluatedWith: card | ||
| ) | ||
| wait(for: [updated], timeout: 8.0) | ||
|
|
||
| let listed = try XCTUnwrap( | ||
| socketResult( | ||
| method: "surface.overlay.list", | ||
| params: ["surface_id": surfaceID] | ||
| ), | ||
| "Expected surface.overlay.list to succeed" | ||
| ) | ||
| let overlays = listed["overlays"] as? [[String: Any]] ?? [] | ||
| XCTAssertEqual(overlays.count, 1, "A keyed update must replace rather than duplicate") | ||
| XCTAssertEqual(overlays.first?["position"] as? String, "right") | ||
|
|
||
| let removed = try XCTUnwrap( | ||
| socketResult( | ||
| method: "surface.overlay.remove", | ||
| params: ["surface_id": surfaceID, "overlay_id": overlayID] | ||
| ), | ||
| "Expected surface.overlay.remove to succeed" | ||
| ) | ||
| XCTAssertEqual(removed["removed"] as? Bool, true) | ||
| let disappeared = expectation( | ||
| for: NSPredicate(format: "exists == false"), | ||
| evaluatedWith: card | ||
| ) | ||
| wait(for: [disappeared], timeout: 8.0) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
Test terminal input pass-through while the overlay exists.
The test name states PassiveCard, but the test only checks rendering and removal. An overlay that consumes keyboard or pointer input still passes.
While the card exists, send terminal keyboard and mouse input. Assert that the target surfaceID receives both inputs before removal.
Based on PR objectives: overlay cards must preserve terminal keyboard and mouse input behavior.
🤖 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 `@cmuxUITests/AutomationSocketUITests.swift` around lines 151 - 273, Extend
testTerminalOverlaySocketRendersUpdatesAndRemovesPassiveCard to exercise input
pass-through while the overlay card exists: send terminal keyboard input and
mouse input targeting surfaceID after the card renders, then assert the socket
responses or observable terminal state confirm both inputs reached that surface
before calling surface.overlay.remove. Keep the existing render, update, list,
and removal assertions unchanged.
Comment on lines
+140
to
+153
| private func overlayStrings() -> ControlSurfaceOverlayStrings { | ||
| context?.controlSurfaceOverlayStrings() ?? ControlSurfaceOverlayStrings( | ||
| tabManagerUnavailable: "", | ||
| workspaceNotFound: "", | ||
| surfaceNotFound: "", | ||
| noFocusedSurface: "", | ||
| surfaceNotTerminal: "", | ||
| invalidIdentifier: "", | ||
| emptyText: "", | ||
| textTooLongFormat: "", | ||
| invalidAnchorFormat: "", | ||
| invalidAlignmentFormat: "", | ||
| scrollbackUnavailable: "" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return a nonempty unavailable error when the context is absent.
When context is nil, surfaceOverlay resolves .tabManagerUnavailable. This fallback sets tabManagerUnavailable to an empty string. The socket response then has message: "".
Require overlay strings before dispatch, or return the established unavailable response before this fallback. Add a context-absent test.
Based on learnings, resolve package socket strings through the app context rather than direct package-bundle localization.
🤖 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/Surface/ControlCommandCoordinator`+SurfaceOverlay.swift
around lines 140 - 153, The overlayStrings fallback currently produces an empty
tabManagerUnavailable message when context is nil. Update the surface overlay
dispatch around overlayStrings and surfaceOverlay to return the established
unavailable response before dispatch, or require context availability, while
preserving normal context-resolved strings. Add a test covering context absence
and ensure socket strings are resolved through the app context rather than
direct package-bundle localization.
Source: Learnings
Comment on lines
+5
to
+17
| @Test func validatesAndNormalizesProducerInput() throws { | ||
| let request = try TerminalOverlayRequest( | ||
| id: "agent.latest-message", | ||
| text: "first\r\n\u{001B}[31msecond\u{0000}", | ||
| anchor: .scrollbackTop, | ||
| horizontalAlignment: .right | ||
| ) | ||
|
|
||
| #expect(request.id == "agent.latest-message") | ||
| #expect(request.text == "first\n[31msecond") | ||
| #expect(request.anchor == .scrollbackTop) | ||
| #expect(request.horizontalAlignment == .right) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Add coverage for the clamped size limits.
TerminalOverlayRequest clamps maximumWidthColumns to 16...200 and maximumHeightRows to 1...50. No test covers that clamp. The renderer multiplies these values by the cell size, so a regression changes card geometry silently.
Add one case that passes out-of-range values and asserts the clamped result.
🤖 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/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/TerminalOverlayTests.swift`
around lines 5 - 17, The TerminalOverlayRequest tests lack coverage for clamping
maximum dimensions. Extend validatesAndNormalizesProducerInput, or add a focused
test, with out-of-range maximumWidthColumns and maximumHeightRows values, then
assert they normalize to 16...200 and 1...50 respectively.
Comment on lines
+35189
to
+35358
| "cli.surfaceOverlay.error.missingSubcommand": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "surface overlay requires set, list, remove, or clear" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "surface overlay には set、list、remove、または clear が必要です" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "cli.surfaceOverlay.error.removeExtraArguments": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "surface overlay remove accepts one id" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "surface overlay remove には 1 つの ID を指定できます" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "cli.surfaceOverlay.error.removeRequiresID": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "surface overlay remove requires an id" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "surface overlay remove には ID が必要です" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "cli.surfaceOverlay.error.setRequiresID": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "surface overlay set requires an id" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "surface overlay set には ID が必要です" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "cli.surfaceOverlay.error.setRequiresText": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "surface overlay set requires text or '-' for standard input" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "surface overlay set にはテキスト、または標準入力を表す '-' が必要です" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "cli.surfaceOverlay.error.unexpectedArgumentFormat": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "surface overlay %@: unexpected argument '%@'" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "surface overlay %@: 予期しない引数 '%@'" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "cli.surfaceOverlay.error.unknownFlagFormat": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "surface overlay: unknown flag '%@'" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "surface overlay: 不明なフラグ '%@'" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "cli.surfaceOverlay.error.unsupportedSubcommandFormat": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Unsupported surface overlay subcommand: %@" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "未対応の surface overlay サブコマンド: %@" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "cli.surfaceOverlay.help": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Usage: cmux surface overlay set <id> <text> [--anchor <viewport|scrollback>] [--position <left|center|right>] [target flags]\n cmux surface overlay list [target flags]\n cmux surface overlay remove <id> [target flags]\n cmux surface overlay clear [target flags]\n\nRender passive text above a terminal without taking keyboard or mouse input.\nA viewport overlay stays at the visible top. A scrollback overlay captures the current top row.\n\nTarget flags:\n --workspace <id|ref|index> Workspace context (default: $CMUX_WORKSPACE_ID)\n --surface <id|ref|index> Terminal context (default: $CMUX_SURFACE_ID)\n --window <id|ref|index> Window context for workspace and surface refs/indexes\n\nSet flags:\n --anchor <viewport|scrollback> Vertical anchor (default: viewport)\n --position <left|center|right> Horizontal position (default: center)\n\nUse '-' as text to read the overlay from standard input.\n\nExamples:\n cmux surface overlay set latest-message \"check the auth error\"\n printf 'build\\npassed' | cmux surface overlay set build-status - --position right\n cmux surface overlay set review-note \"inspect this output\" --anchor scrollback --position left" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "使用方法: cmux surface overlay set <id> <text> [--anchor <viewport|scrollback>] [--position <left|center|right>] [対象フラグ]\n cmux surface overlay list [対象フラグ]\n cmux surface overlay remove <id> [対象フラグ]\n cmux surface overlay clear [対象フラグ]\n\nキーボードやマウス入力を奪わず、ターミナルの上に受動的なテキストを表示します。\nviewport オーバーレイは表示領域の上端に残ります。scrollback オーバーレイは現在の上端行を基準に固定されます。\n\n対象フラグ:\n --workspace <id|ref|index> ワークスペース (既定: $CMUX_WORKSPACE_ID)\n --surface <id|ref|index> ターミナル (既定: $CMUX_SURFACE_ID)\n --window <id|ref|index> ワークスペースとサーフェスの参照・インデックスに使うウインドウ\n\nset フラグ:\n --anchor <viewport|scrollback> 垂直アンカー (既定: viewport)\n --position <left|center|right> 水平位置 (既定: center)\n\nテキストに '-' を指定すると標準入力から読み取ります。\n\n例:\n cmux surface overlay set latest-message \"check the auth error\"\n printf 'build\\npassed' | cmux surface overlay set build-status - --position right\n cmux surface overlay set review-note \"inspect this output\" --anchor scrollback --position left" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "cli.surfaceOverlay.usageLine": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "surface overlay <set|list|remove|clear> [--workspace <id|ref|index>] [--surface <id|ref|index>] [--window <id|ref|index>]" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "surface overlay <set|list|remove|clear> [--workspace <id|ref|index>] [--surface <id|ref|index>] [--window <id|ref|index>]" | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import json
from pathlib import Path
catalog = json.loads(Path("Resources/Localizable.xcstrings").read_text())
strings = catalog["strings"]
prefixes = ("cli.surfaceOverlay.", "socket.surfaceOverlay.")
catalog_locales = sorted({
locale
for entry in strings.values()
for locale in entry.get("localizations", {})
})
for key, entry in strings.items():
if key.startswith(prefixes):
present = set(entry.get("localizations", {}))
missing = [locale for locale in catalog_locales if locale not in present]
if missing:
print(f"{key}: missing {', '.join(missing)}")
PYRepository: manaflow-ai/cmux
Length of output: 3023
Add translations for every supported locale.
The new cli.surfaceOverlay.* and socket.surfaceOverlay.* entries currently include only en and ja. Add translated values for the remaining supported locales in Resources/Localizable.xcstrings.
🤖 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 `@Resources/Localizable.xcstrings` around lines 35189 - 35358, The new
cli.surfaceOverlay.* and socket.surfaceOverlay.* localization entries currently
contain only en and ja; add translated stringUnit values for every remaining
supported locale in Resources/Localizable.xcstrings, preserving each key’s
existing placeholders and formatting.
Sources: Path instructions, Learnings
Comment on lines
+53
to
+109
| func apply( | ||
| _ overlay: TerminalOverlay, | ||
| cellSize: CGSize, | ||
| availableWidth: CGFloat | ||
| ) -> CGSize { | ||
| let cellWidth = cellSize.width > 0 ? cellSize.width : 8 | ||
| let cellHeight = cellSize.height > 0 ? cellSize.height : 18 | ||
| let fontSize = max(10, min(20, cellHeight * 0.72)) | ||
| let font = NSFont.monospacedSystemFont(ofSize: fontSize, weight: .medium) | ||
| let maximumCardWidth = min( | ||
| max(0, availableWidth - 16), | ||
| CGFloat(overlay.maximumWidthColumns) * cellWidth + Metrics.horizontalPadding * 2 | ||
| ) | ||
| guard maximumCardWidth > Metrics.horizontalPadding * 2 else { return .zero } | ||
|
|
||
| let attributes: [NSAttributedString.Key: Any] = [.font: font] | ||
| let naturalLineWidth = overlay.text | ||
| .split(separator: "\n", omittingEmptySubsequences: false) | ||
| .map { line in | ||
| ceil((String(line) as NSString).size(withAttributes: attributes).width) | ||
| } | ||
| .max() ?? Metrics.minimumContentWidth | ||
| // `NSTextFieldCell` keeps a small horizontal text inset even for a | ||
| // borderless label. Reserve one terminal cell so the final word does | ||
| // not wrap into a clipped second line when the measured text otherwise | ||
| // fits exactly. | ||
| let contentWidth = min( | ||
| maximumCardWidth - Metrics.horizontalPadding * 2, | ||
| max(Metrics.minimumContentWidth, naturalLineWidth + cellWidth) | ||
| ) | ||
| let measuredTextWidth = max(1, contentWidth - cellWidth) | ||
| let maximumContentHeight = CGFloat(overlay.maximumHeightRows) * cellHeight | ||
| let measured = (overlay.text as NSString).boundingRect( | ||
| with: CGSize(width: measuredTextWidth, height: .greatestFiniteMagnitude), | ||
| options: [.usesLineFragmentOrigin, .usesFontLeading], | ||
| attributes: attributes | ||
| ) | ||
| let contentHeight = min(maximumContentHeight, max(cellHeight, ceil(measured.height))) | ||
| let size = CGSize( | ||
| width: contentWidth + Metrics.horizontalPadding * 2, | ||
| height: contentHeight + Metrics.verticalPadding * 2 | ||
| ) | ||
|
|
||
| label.stringValue = overlay.text | ||
| label.font = font | ||
| label.maximumNumberOfLines = overlay.maximumHeightRows | ||
| label.frame = CGRect( | ||
| x: Metrics.horizontalPadding, | ||
| y: Metrics.verticalPadding, | ||
| width: contentWidth, | ||
| height: contentHeight | ||
| ) | ||
| label.identifier = NSUserInterfaceItemIdentifier("terminal-overlay-\(overlay.id)") | ||
| label.setAccessibilityIdentifier("terminal-overlay-\(overlay.id)") | ||
| label.setAccessibilityLabel(overlay.text) | ||
| return size | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Cache the measurement. apply re-measures the full text on every scroll event.
synchronizeTerminalOverlays calls apply for every overlay. That function runs from three call sites in Sources/GhosttyTerminalView.swift: the .ghosttyDidUpdateScrollbar observer at line 11605, the .ghosttyDidUpdateCellSize observer at line 8795, and synchronizeGeometryAndContent at line 9006. The scrollbar notification fires on terminal output and during scrolling.
Each call performs, per overlay:
- one
splitof the text, whichTerminalOverlayRequestcaps at 16 KiB; - one
NSString.size(withAttributes:)per line, with no bound on line count; - one
boundingRectover the whole text.
The result depends only on overlay.text, cellSize, and availableWidth. None of those change when the user scrolls. Cache the last inputs and reuse the computed size.
⚡ Proposed measurement cache
`@MainActor`
final class TerminalOverlayCardView: NSVisualEffectView {
private enum Metrics {
static let horizontalPadding: CGFloat = 12
static let verticalPadding: CGFloat = 9
static let minimumContentWidth: CGFloat = 96
}
private let label = NSTextField(labelWithString: "")
+
+ private struct MeasurementKey: Equatable {
+ let text: String
+ let cellSize: CGSize
+ let availableWidth: CGFloat
+ }
+
+ private var lastMeasurementKey: MeasurementKey?
+ private var lastMeasuredSize: CGSize = .zero func apply(
_ overlay: TerminalOverlay,
cellSize: CGSize,
availableWidth: CGFloat
) -> CGSize {
+ let key = MeasurementKey(
+ text: overlay.text,
+ cellSize: cellSize,
+ availableWidth: availableWidth
+ )
+ if key == lastMeasurementKey {
+ return lastMeasuredSize
+ }
let cellWidth = cellSize.width > 0 ? cellSize.width : 8Store the result before returning:
label.setAccessibilityLabel(overlay.text)
+ lastMeasurementKey = key
+ lastMeasuredSize = size
return size
}Reset the cache on the early return too:
- guard maximumCardWidth > Metrics.horizontalPadding * 2 else { return .zero }
+ guard maximumCardWidth > Metrics.horizontalPadding * 2 else {
+ lastMeasurementKey = key
+ lastMeasuredSize = .zero
+ return .zero
+ }As per coding guidelines: "Avoid repeated per-element string interpolation or concatenation that creates large intermediate strings on hot paths" and "Avoid repeated full scans, sorting, filtering, or per-item nested scans over scalable collections in production code, especially in UI, event-driven... paths."
🤖 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/TerminalOverlayView.swift` around lines 53 - 109, Add a measurement
cache to TerminalOverlayView’s apply method keyed by overlay.text, cellSize, and
availableWidth, and reuse the cached CGSize when all inputs match. Store the
computed size after measurement and reset the cache when the maximumCardWidth
early return produces .zero. Keep label content and frame updates applying on
every call while avoiding repeated text measurement during scrolling.
Source: Coding guidelines
Comment on lines
+62
to
+66
| let maximumCardWidth = min( | ||
| max(0, availableWidth - 16), | ||
| CGFloat(overlay.maximumWidthColumns) * cellWidth + Metrics.horizontalPadding * 2 | ||
| ) | ||
| guard maximumCardWidth > Metrics.horizontalPadding * 2 else { return .zero } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Derive the width inset from OverlayMetrics.edgeMargin.
Line 63 subtracts the literal 16. That value is OverlayMetrics.edgeMargin * 2, which the placement code at lines 224-229 and 269-274 uses. The two values must stay equal, but nothing links them. A change to edgeMargin makes the card width disagree with its placement margin.
OverlayMetrics is currently private inside the GhosttySurfaceScrollView extension. Promote it to a file-scope type so apply can read it, then pass the margin in or reference it directly.
🤖 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/TerminalOverlayView.swift` around lines 62 - 66, Promote
OverlayMetrics from the private GhosttySurfaceScrollView extension to file
scope, then update the maximumCardWidth calculation to subtract
OverlayMetrics.edgeMargin * 2 instead of the literal 16. Keep the width
calculation and placement margins synchronized by reusing this shared metric.
Comment on lines
+250
to
+259
| guard let geometry = scrollbackGeometry, | ||
| geometry.rowSpaceRevision == rowSpaceRevision, | ||
| let originY = TerminalOverlayGeometry.scrollbackOverlayOriginY( | ||
| documentHeight: documentHeight(), | ||
| row: row, | ||
| totalRows: Int(clamping: geometry.scrollbar.total), | ||
| cellHeight: cellSize.height, | ||
| topPadding: topPadding, | ||
| overlayHeight: cardSize.height | ||
| ) else { |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
Hoist documentHeight() out of the loop.
Line 253 calls documentHeight() once per scrollback overlay. The value does not change during the loop. Compute it once beside scrollbackGeometry at line 196 and reuse it.
♻️ Proposed change
let topPadding = gridMetrics?.topPadding ?? 0
+ let currentDocumentHeight = scrollbackGeometry == nil ? 0 : documentHeight() let originY = TerminalOverlayGeometry.scrollbackOverlayOriginY(
- documentHeight: documentHeight(),
+ documentHeight: currentDocumentHeight,
row: row,🤖 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/TerminalOverlayView.swift` around lines 250 - 259, Compute
documentHeight() once before the scrollback overlay loop, alongside
scrollbackGeometry, then reuse that stored value in the
TerminalOverlayGeometry.scrollbackOverlayOriginY call instead of invoking
documentHeight() for each overlay.
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 `@cmuxUITests/AutomationSocketUITests.swift`:
- Around line 176-183: Constrain the XCTExpectFailure block around app.launch()
by adding an issueMatcher that accepts only the known headless CI activation
failure; do not allow isStrict = false to match arbitrary launch issues.
Alternatively, remove the broad expected-failure wrapper and handle the known
headless readiness failure explicitly while preserving normal reporting for
signing, bundle, crash, and other startup failures.
🪄 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 Plus
Run ID: b7232db6-cf01-4022-9900-49c6a8ad3ee1
📒 Files selected for processing (1)
cmuxUITests/AutomationSocketUITests.swift
Comment on lines
+176
to
+183
| let activationOptions = XCTExpectedFailure.Options() | ||
| activationOptions.isStrict = false | ||
| XCTExpectFailure( | ||
| "App activation may fail on headless CI runners", | ||
| options: activationOptions | ||
| ) { | ||
| app.launch() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)AutomationSocketUITests\.swift$|AutomationSocket' || true
echo
echo "== relevant source =="
if [ -f cmuxUITests/AutomationSocketUITests.swift ]; then
nl -ba cmuxUITests/AutomationSocketUITests.swift | sed -n '140,210p'
fi
echo
echo "== usages in tests around app.launch expectations =="
rg -n 'XCTExpectFailure|isStrict|issueMatcher|app\.launch|launch\(\)' cmuxUITests -S || trueRepository: manaflow-ai/cmux
Length of output: 272
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant source with line numbers =="
if [ -f cmuxUITests/AutomationSocketUITests.swift ]; then
awk '{printf "%6d %s\n", NR, $0}' cmuxUITests/AutomationSocketUITests.swift | sed -n '140,220p'
fi
echo
echo "== usages in tests around app.launch expectations =="
rg -n 'XCTExpectFailure|isStrict|issueMatcher|app\.launch|launch\(\)' cmuxUITests/AutomationSocketUITests.swift cmuxUITests -S || true
echo
echo "== CI/headless mentions in test file =="
rg -n 'headless|head|CI|XCTExpectedFailure|XCTExpectFailure|activation' cmuxUITests/AutomationSocketUITests.swift -S || trueRepository: manaflow-ai/cmux
Length of output: 50372
🌐 Web query:
Apple XCTest issueMatcher isStrict XCTIssueReference expected failure documentation
💡 Result:
In the XCTest framework, XCTExpectFailure is a tool used to mark known test failures as expected, preventing them from causing the overall test suite to fail [1][2]. When configuring this behavior using XCTExpectedFailure.Options, two important properties are isStrict and issueMatcher [3][4]. isStrict The isStrict property (a Boolean) determines how XCTest treats an XCTExpectFailure call if the expected failure does not actually occur [4][2]. - Default Behavior: By default, isStrict is true [4][2]. If the code inside the test block completes without triggering the expected failure, XCTest will flag this as an "unmatched expected failure" and report it as a test failure [4][5]. This ensures that once a bug is fixed, you are notified to remove the XCTExpectFailure call [4]. - Disabling Strict Mode: If you are dealing with non-deterministic or "flaky" tests where a failure might not occur every time, you can set isStrict to false [1][2]. This prevents the test from failing if no issue is recorded [4][5]. You can set this via the XCTExpectedFailure.Options object or by passing the strict parameter directly to the XCTExpectFailure function [1][4]. issueMatcher The issueMatcher property allows you to define a closure that inspects an XCTIssueReference to determine if a specific failure should be considered "expected" [3][6]. - Usage: The closure takes an XCTIssueReference as an argument and returns a Bool [3]. If it returns true, the issue is considered handled by the XCTExpectFailure call; if it returns false, the issue remains unhandled and will cause the test to fail [4][6]. - Granular Control: This is useful when a test block may encounter multiple types of issues, but you only want to suppress specific ones (e.g., matching only.assertionFailure or a specific error type) while letting others trigger a standard test failure [4][6]. For modern Swift projects, Apple recommends migrating to the Swift Testing framework, which provides the withKnownIssue function as a successor to XCTExpectFailure, offering similar functionality with more idiomatic syntax [7][8][9].
Citations:
- 1: https://developer.apple.com/la/videos/play/wwdc2021/10207/
- 2: https://wwdcnotes.com/notes/wwdc21/10207/
- 3: https://developer.apple.com/documentation/xctest/xctexpectedfailure/options/issuematcher
- 4: https://augmentedcode.io/2022/01/24/using-xctexpectfailure-in-xctests/
- 5: https://evnik.github.io/XcodeRN/Xcode12.5Beta3.html
- 6: https://jayeshkawli.com/xcode-13-introduces-new-way-for-handling-expected-unit-test-failures/
- 7: https://github.com/apple/swift-testing/blob/main/Sources/Testing/Testing.docc/MigratingFromXCTest.md
- 8: https://developer.apple.com/documentation/testing/migratingfromxctest
- 9: https://developer.apple.com/documentation/testing/withknownissue(_:isintermittent:sourcelocation:_:when:matching:)
Constrain the expected failure to the known headless activation issue.
At cmuxUITests/AutomationSocketUITests.swift:176-183, XCTExpectFailure uses isStrict = false without issueMatcher. XCTest may then match any app.launch() issue in this block as expected, including signing, bundle, crash-at-launch, or other startup regressions outside headless activation. Add an issueMatcher that matches the headless CI activation failure, or keep launch failures normal and handle readiness failure on the known headless 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 `@cmuxUITests/AutomationSocketUITests.swift` around lines 176 - 183, Constrain
the XCTExpectFailure block around app.launch() by adding an issueMatcher that
accepts only the known headless CI activation failure; do not allow isStrict =
false to match arbitrary launch issues. Alternatively, remove the broad
expected-failure wrapper and handle the known headless readiness failure
explicitly while preserving normal reporting for signing, bundle, crash, and
other startup failures.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmuxUITests/AutomationSocketUITests.swift (1)
189-192: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRequire a successful socket ping before overlay assertions.
waitForSocketPong(timeout:)usesallowDiagnosticsFallback: trueby default. The helper can return success from the diagnostics file without a successful ping. The followingworkspace.createcall can then fail later, without proving socket readiness. PassallowDiagnosticsFallback: falsefor this test.Proposed fix
- waitForSocketPong(timeout: 12.0), + waitForSocketPong(timeout: 12.0, allowDiagnosticsFallback: 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 `@cmuxUITests/AutomationSocketUITests.swift` around lines 189 - 192, Update the waitForSocketPong call in the socket readiness assertion to pass allowDiagnosticsFallback: false, ensuring success requires an actual socket pong before subsequent workspace.create and overlay assertions run.CLI/CMUXCLI+TerminalOverlay.swift (1)
114-120: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEscape tabs in non-JSON list output.
The row at Line 120 uses tabs as field separators, but Line 119 escapes only newlines. If overlay text contains a tab, it creates extra fields and makes the row ambiguous to scripts. Escape tabs before printing.
Proposed fix
let text = (overlay["text"] as? String ?? "") + .replacingOccurrences(of: "\t", with: "\\t") .replacingOccurrences(of: "\n", with: "\\n")🤖 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`+TerminalOverlay.swift around lines 114 - 120, Update the overlay text normalization in the payload["overlays"] output loop to escape tab characters as well as newlines before constructing the tab-separated print row. Preserve the existing id, anchor, position, and text field ordering.
🤖 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 `@cmuxUITests/AutomationSocketUITests.swift`:
- Around line 315-321: Strengthen the assertion after the surface.overlay.clear
call in the relevant UI test: verify the rendered overlay no longer contains
secondLine (and any remaining overlay content), rather than only unwrapping a
successful socketResult. Reuse the test’s existing UI-query or screenshot
assertion helpers to validate the clear postcondition.
- Around line 295-298: Strengthen the assertions in the keyed overlay update
test by explicitly locating an overlay with secondOverlayID and asserting it
remains present after the update. Keep the existing overlayID position and
anchor assertions, while retaining the count check to verify both keyed entries
coexist.
In `@Sources/TerminalOverlayView.swift`:
- Around line 66-73: Update the overlay application logic around the visible
label assignment to cache the last applied overlay text per overlay. Only when
overlay.text changes should it collapse whitespace, update label.stringValue,
rebuild the font, and refresh the accessibility label; otherwise skip all of
those operations during repeated synchronization and scrolling. Ensure the cache
is initialized and invalidated appropriately when overlays are created,
replaced, or removed.
---
Outside diff comments:
In `@CLI/CMUXCLI`+TerminalOverlay.swift:
- Around line 114-120: Update the overlay text normalization in the
payload["overlays"] output loop to escape tab characters as well as newlines
before constructing the tab-separated print row. Preserve the existing id,
anchor, position, and text field ordering.
In `@cmuxUITests/AutomationSocketUITests.swift`:
- Around line 189-192: Update the waitForSocketPong call in the socket readiness
assertion to pass allowDiagnosticsFallback: false, ensuring success requires an
actual socket pong before subsequent workspace.create and overlay assertions
run.
🪄 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 Plus
Run ID: 075a8cf5-6811-4a3a-9c3a-4e2822c7e48b
📒 Files selected for processing (14)
CLI/CMUXCLI+TerminalOverlay.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlCommandCoordinator+SurfaceOverlay.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Surface/ControlSurfaceOverlay.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorSurfaceTests.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Hosting/TerminalSurfacePaneHosting.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+Overlay.swiftPackages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/FakeTerminalSurfacePaneHost.swiftPackages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/Overlay/TerminalOverlay.swiftPackages/macOS/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/TerminalOverlayTests.swiftResources/Localizable.xcstringsSources/GhosttyTerminalView.swiftSources/TerminalController+ControlSurfaceOverlay.swiftSources/TerminalOverlayView.swiftcmuxUITests/AutomationSocketUITests.swift
Comment on lines
+295
to
+298
| XCTAssertEqual(overlays.count, 2, "Multiple keys coexist and a keyed update replaces in place") | ||
| let updatedOverlay = overlays.first { $0["id"] as? String == overlayID } | ||
| XCTAssertEqual(updatedOverlay?["position"] as? String, "right") | ||
| XCTAssertEqual(updatedOverlay?["anchor"] as? String, "viewport") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Assert both overlay identifiers after the keyed update.
overlays.count == 2 and the lookup for overlayID do not prove that secondOverlayID remains in the response. A faulty update can return two entries for overlayID and still pass.
Proposed fix
let overlays = listed["overlays"] as? [[String: Any]] ?? []
XCTAssertEqual(overlays.count, 2, "Multiple keys coexist and a keyed update replaces in place")
+ let listedIDs = Set(overlays.compactMap { $0["id"] as? String })
+ XCTAssertEqual(listedIDs, Set([overlayID, secondOverlayID]))
let updatedOverlay = overlays.first { $0["id"] as? String == overlayID }📝 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
| XCTAssertEqual(overlays.count, 2, "Multiple keys coexist and a keyed update replaces in place") | |
| let updatedOverlay = overlays.first { $0["id"] as? String == overlayID } | |
| XCTAssertEqual(updatedOverlay?["position"] as? String, "right") | |
| XCTAssertEqual(updatedOverlay?["anchor"] as? String, "viewport") | |
| XCTAssertEqual(overlays.count, 2, "Multiple keys coexist and a keyed update replaces in place") | |
| let listedIDs = Set(overlays.compactMap { $0["id"] as? String }) | |
| XCTAssertEqual(listedIDs, Set([overlayID, secondOverlayID])) | |
| let updatedOverlay = overlays.first { $0["id"] as? String == overlayID } | |
| XCTAssertEqual(updatedOverlay?["position"] as? String, "right") | |
| XCTAssertEqual(updatedOverlay?["anchor"] as? String, "viewport") |
🤖 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 `@cmuxUITests/AutomationSocketUITests.swift` around lines 295 - 298, Strengthen
the assertions in the keyed overlay update test by explicitly locating an
overlay with secondOverlayID and asserting it remains present after the update.
Keep the existing overlayID position and anchor assertions, while retaining the
count check to verify both keyed entries coexist.
Comment on lines
+315
to
+321
| _ = try XCTUnwrap( | ||
| socketResult( | ||
| method: "surface.overlay.clear", | ||
| params: ["surface_id": surfaceID] | ||
| ), | ||
| "Expected surface.overlay.clear to remove remaining lines" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Verify the clear postcondition.
The test only verifies that surface.overlay.clear returns successfully. It does not verify that the remaining overlay leaves the rendered UI. A clear implementation can acknowledge the command and leave secondLine visible without failing this test.
Proposed fix
_ = try XCTUnwrap(
socketResult(
method: "surface.overlay.clear",
params: ["surface_id": surfaceID]
),
"Expected surface.overlay.clear to remove remaining lines"
)
+ let cleared = expectation(
+ for: NSPredicate(format: "exists == false"),
+ evaluatedWith: secondLine
+ )
+ wait(for: [cleared], timeout: 8.0)📝 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
| _ = try XCTUnwrap( | |
| socketResult( | |
| method: "surface.overlay.clear", | |
| params: ["surface_id": surfaceID] | |
| ), | |
| "Expected surface.overlay.clear to remove remaining lines" | |
| ) | |
| _ = try XCTUnwrap( | |
| socketResult( | |
| method: "surface.overlay.clear", | |
| params: ["surface_id": surfaceID] | |
| ), | |
| "Expected surface.overlay.clear to remove remaining lines" | |
| ) | |
| let cleared = expectation( | |
| for: NSPredicate(format: "exists == false"), | |
| evaluatedWith: secondLine | |
| ) | |
| wait(for: [cleared], timeout: 8.0) |
🤖 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 `@cmuxUITests/AutomationSocketUITests.swift` around lines 315 - 321, Strengthen
the assertion after the surface.overlay.clear call in the relevant UI test:
verify the rendered overlay no longer contains secondLine (and any remaining
overlay content), rather than only unwrapping a successful socketResult. Reuse
the test’s existing UI-query or screenshot assertion helpers to validate the
clear postcondition.
Comment on lines
+66
to
+73
| let displayText = overlay.text | ||
| .split(whereSeparator: { $0.isWhitespace }) | ||
| .joined(separator: " ") | ||
| label.stringValue = displayText | ||
| label.font = NSFont.monospacedSystemFont( | ||
| ofSize: max(9, min(18, cellHeight * 0.65)), | ||
| weight: .regular | ||
| ) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Collapse the overlay text once, not on every scroll event.
synchronizeTerminalOverlays calls apply for every overlay. It runs from the .ghosttyDidUpdateScrollbar observer at Sources/GhosttyTerminalView.swift line 11605, the .ghosttyDidUpdateCellSize observer at line 8795, and synchronizeGeometryAndContent at line 9006. The scrollbar notification fires on terminal output and during scrolling.
Each call splits the full overlay text on whitespace and joins it into a new String. TerminalOverlayRequest caps that text at 16 KiB, so each scroll event allocates a fresh string per overlay. The result depends only on overlay.text, which does not change while the user scrolls. The font is also rebuilt on every call.
Cache the last applied text and skip the split, join, font build, and stringValue write when the text is unchanged.
As per coding guidelines: "Avoid repeated per-element string interpolation or concatenation that creates large intermediate strings on hot paths; use a preallocated buffer or a single reserved-capacity build."
⚡ Proposed text cache
final class TerminalOverlayLineView: NSVisualEffectView {
private let label = NSTextField(labelWithString: "")
+ private var appliedSourceText: String?
+ private var appliedCellHeight: CGFloat = 0- let displayText = overlay.text
- .split(whereSeparator: { $0.isWhitespace })
- .joined(separator: " ")
- label.stringValue = displayText
- label.font = NSFont.monospacedSystemFont(
- ofSize: max(9, min(18, cellHeight * 0.65)),
- weight: .regular
- )
- label.maximumNumberOfLines = 1
+ if appliedSourceText != overlay.text || appliedCellHeight != cellHeight {
+ let displayText = overlay.text
+ .split(whereSeparator: { $0.isWhitespace })
+ .joined(separator: " ")
+ label.stringValue = displayText
+ label.font = NSFont.monospacedSystemFont(
+ ofSize: max(9, min(18, cellHeight * 0.65)),
+ weight: .regular
+ )
+ label.maximumNumberOfLines = 1
+ label.setAccessibilityLabel(displayText)
+ appliedSourceText = overlay.text
+ appliedCellHeight = cellHeight
+ }Then drop the unconditional accessibility-label write near line 83.
🤖 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/TerminalOverlayView.swift` around lines 66 - 73, Update the overlay
application logic around the visible label assignment to cache the last applied
overlay text per overlay. Only when overlay.text changes should it collapse
whitespace, update label.stringValue, rebuild the font, and refresh the
accessibility label; otherwise skip all of those operations during repeated
synchronization and scrolling. Ensure the cache is initialized and invalidated
appropriately when overlays are created, replaced, or removed.
Source: Coding guidelines
lawrencecchen
changed the title
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 `@CLI/cmux.swift`:
- Around line 31592-31606: Update the catch block around
publishLatestCodexUserMessageOverlay in the codex prompt-strip flow to include
String(describing: error) in the recorded telemetry data, while preserving the
existing "codex-hook.prompt-submit.overlay-failed" event and surrounding
behavior.
In `@CLI/CMUXCLI`+CodexFireAndForgetHooks.swift:
- Around line 122-123: Clarify the production scope of CMUX_CODEX_HOOK_CMUX_BIN
by adding a nearby comment in the hook command setup, stating whether it is a
supported production executable override or restricted to test harnesses. Match
the existing documentation style around the CMUX_BUNDLED_CLI_PATH fallback
without changing the executable-selection behavior.
🪄 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 Plus
Run ID: bbae6f56-9b63-4d4f-9195-42f30b12afec
📒 Files selected for processing (5)
CLI/CMUXCLI+CodexFireAndForgetHooks.swiftCLI/CMUXCLI+TerminalOverlay.swiftCLI/cmux.swiftcmuxTests/CLICodexHookTimeoutRegressionTestSupport.swiftcmuxTests/CLICodexHookTimeoutRegressionTests.swift
Comment on lines
+31592
to
+31606
| // The prompt strip describes accepted user input, not the duration | ||
| // of the agent turn. Publish it before lifecycle/socket work so a | ||
| // fast Stop cannot race the strip out of existence. | ||
| if !suppressVisibleMutations, | ||
| !incomingCodexTurnIsTerminal, | ||
| def.name == "codex", | ||
| let prompt = feedPromptText(from: input.rawObject ?? input.object) { | ||
| do { | ||
| try publishLatestCodexUserMessageOverlay( | ||
| prompt, | ||
| workspaceId: workspaceId, | ||
| surfaceId: surfaceId, | ||
| client: client | ||
| ) | ||
| } catch { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Preserve the error detail in the overlay-publish catch block.
The catch block still discards error and records only the static string "codex-hook.prompt-submit.overlay-failed". Other failure paths in this same function preserve error detail. claude-hook.stop.ignored uses data: ["error": String(describing: error)]. reportAgentHookFailure also forwards error: error. Without the error detail, a failed overlay publish is not diagnosable from telemetry alone.
🐛 Proposed fix to preserve the error detail
} catch {
- telemetry.breadcrumb("codex-hook.prompt-submit.overlay-failed")
+ telemetry.breadcrumb(
+ "codex-hook.prompt-submit.overlay-failed",
+ data: ["error": String(describing: error)]
+ )
}This issue was flagged in a previous review pass on an earlier commit and remains unresolved in the current code.
🤖 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/cmux.swift` around lines 31592 - 31606, Update the catch block around
publishLatestCodexUserMessageOverlay in the codex prompt-strip flow to include
String(describing: error) in the recorded telemetry data, while preserving the
existing "codex-hook.prompt-submit.overlay-failed" event and surrounding
behavior.
Comment on lines
+122
to
+123
| "cmux_cli=\"${CMUX_CODEX_HOOK_CMUX_BIN:-}\"", | ||
| "if [ -z \"$cmux_cli\" ] || [ ! -x \"$cmux_cli\" ]; then cmux_cli=\"${CMUX_BUNDLED_CLI_PATH:-}\"; fi", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all references to CMUX_CODEX_HOOK_CMUX_BIN to confirm production vs. test-only usage.
rg -n "CMUX_CODEX_HOOK_CMUX_BIN"Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked files matching relevant names:"
git ls-files | rg '(^|/)(CMUXCLI\+CodexFireAndForgetHooks\.swift|CLICodexHookTimeoutRegressionTests\.swift|.*Codex.*Hook.*|.*FireAndForget.*|.*Codex.*)' || true
echo
echo "Locate file by name:"
fd -a -i 'CMUXCLI\+CodexFireAndForgetHooks.swift' . || true
fd -a -i 'CLICodexHookTimeoutRegressionTests.swift' . || true
echo
echo "Search related env/fallback references:"
rg -n "CMUX_CODEX_HOOK_CMUX_BIN|CMUX_BUNDLED_CLI_PATH|CODEX|cmux_cli|BUNDLED_CLI" . --glob '!build/**' --glob '!dist/**' --glob '!node_modules/**' | head -200 || trueRepository: manaflow-ai/cmux
Length of output: 26900
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if [ -f "CLI/CMUXCLI+CodexFireAndForgetHooks.swift" ]; then
echo "CLI/CMUXCLI+CodexFireAndForgetHooks.swift"
wc -l "CLI/CMUXCLI+CodexFireAndForgetHooks.swift"
sed -n '80,150p' "CLI/CMUXCLI+CodexFireAndForgetHooks.swift" | nl -ba -v80
else
echo "Missing CLI/CMUXCLI+CodexFireAndForgetHooks.swift"
fi
if [ -f "cmuxTests/CLICodexHookTimeoutRegressionTests.swift" ]; then
echo
echo "cmuxTests/CLICodexHookTimeoutRegressionTests.swift"
wc -l "cmuxTests/CLICodexHookTimeoutRegressionTests.swift"
rg -n -C 4 "CMUX_CODEX_HOOK_CMUX_BIN|CMUX_BUNDLED_CLI_PATH|hook|CMUX" "cmuxTests/CLICodexHookTimeoutRegressionTests.swift" || true
fi
echo
echo "Search repository for exact/likely identifiers:"
rg -n "CMUX_CODEX_HOOK_CMUX_BIN|CMUX_BUNDLED_CLI_PATH|CodexFireAndForget|codex.*hook|CMUXCLI\+CodexFireAndForgetHooks" . \
--glob '!build/**' --glob '!dist/**' --glob '!node_modules/**' --glob '!DerivedData/**' \
| head -300 || trueRepository: manaflow-ai/cmux
Length of output: 282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "CLI/CMUXCLI+CodexFireAndForgetHooks.swift"
wc -l "CLI/CMUXCLI+CodexFireAndForgetHooks.swift"
sed -n '80,150p' "CLI/CMUXCLI+CodexFireAndForgetHooks.swift"
echo
echo "Code generation occurrences for CMUX_CODEX_HOOK_CMUX_BIN / bundled path:"
rg -n -C 3 "CMUX_CODEX_HOOK_CMUX_BIN|CMUX_BUNDLED_CLI_PATH|cmux_cli=|Fallback|fallback" "CLI/CMUXCLI+CodexFireAndForgetHooks.swift" "cmuxTests/CLICodexHookTimeoutRegressionTestSupport.swift" "cmuxTests/CLICodexHookTimeoutRegressionTests.swift" "tests/test_codex_wrapper_resume_hooks.py" "tests/test_codex_feed_hooks.py" || true
echo
echo "Focused tracked file contexts:"
for f in "CLI/CMUXCLI+CodexFireAndForgetHooks.swift" "cmuxTests/CLICodexHookTimeoutRegressionTestSupport.swift" "cmuxTests/CLICodexHookTimeoutRegressionTests.swift" "tests/test_codex_wrapper_resume_hooks.py"; do
if [ -f "$f" ]; then
echo "--- $f ($(wc -l < "$f") lines) ---"
rg -n -C 4 "CMUX_CODEX_HOOK_CMUX_BIN|CMUX_BUNDLED_CLI_PATH|cmux_cli" "$f" || true
fi
doneRepository: manaflow-ai/cmux
Length of output: 18016
Add production scope guidance for CMUX_CODEX_HOOK_CMUX_BIN.
This line makes CMUX_CODEX_HOOK_CMUX_BIN the first checked executable for Codex hooks before CMUX_BUNDLED_CLI_PATH. Add a nearby comment stating whether this is a supported production override or intended only for test harnesses, matching the surrounding fallback-documentation style.
🤖 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`+CodexFireAndForgetHooks.swift around lines 122 - 123, Clarify
the production scope of CMUX_CODEX_HOOK_CMUX_BIN by adding a nearby comment in
the hook command setup, stating whether it is a supported production executable
override or restricted to test harnesses. Match the existing documentation style
around the CMUX_BUNDLED_CLI_PATH fallback without changing the
executable-selection behavior.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmuxTests/CLICodexHookTimeoutRegressionTests.swift (1)
914-916: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSynchronize both background-hook assertions before decoding.
The wrapper prints
{}before the detached hook process completes socket delivery. Both tests must wait for the realsurface.overlay.setpredicate before readingcommands.snapshot().
cmuxTests/CLICodexHookTimeoutRegressionTests.swift#L914-L916: poll until the overlay request exists, then decode it.cmuxTests/CLICodexHookTimeoutRegressionTests.swift#L989-L992: poll until the overlay request exists, then selectoverlayRequest.🤖 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 `@cmuxTests/CLICodexHookTimeoutRegressionTests.swift` around lines 914 - 916, Synchronize both background-hook assertions before decoding: at cmuxTests/CLICodexHookTimeoutRegressionTests.swift#L914-L916, poll until commands.snapshot() contains a request matching the surface.overlay.set predicate, then decode it; apply the same polling at cmuxTests/CLICodexHookTimeoutRegressionTests.swift#L989-L992 before selecting overlayRequest.Source: Path instructions
CLI/CMUXCLI+CodexFireAndForgetHooks.swift (1)
122-124: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject directories before selecting a hook executable.
test -xalso succeeds for an executable directory. IfCMUX_CODEX_HOOK_CMUX_BIN,CMUX_BUNDLED_CLI_PATH, or the PATH lookup resolves to a directory, the hook keeps that value and the background invocation fails instead of falling back.Check
-fand-xfor every candidate, including the result ofcommand -v, and clear the value when no regular executable file remains.As per retrieved learnings, custom executable paths must reject directories before checking executability.
Proposed validation
- "if [ -z \"$cmux_cli\" ] || [ ! -x \"$cmux_cli\" ]; then cmux_cli=\"${CMUX_BUNDLED_CLI_PATH:-}\"; fi", - "if [ -z \"$cmux_cli\" ] || [ ! -x \"$cmux_cli\" ]; then cmux_cli=\"$(command -v cmux 2>/dev/null || true)\"; fi", + "if [ -z \"$cmux_cli\" ] || [ ! -f \"$cmux_cli\" ] || [ ! -x \"$cmux_cli\" ]; then cmux_cli=\"${CMUX_BUNDLED_CLI_PATH:-}\"; fi", + "if [ -z \"$cmux_cli\" ] || [ ! -f \"$cmux_cli\" ] || [ ! -x \"$cmux_cli\" ]; then cmux_cli=\"$(command -v cmux 2>/dev/null || true)\"; fi", + "if [ ! -f \"$cmux_cli\" ] || [ ! -x \"$cmux_cli\" ]; then cmux_cli=\"\"; fi",#!/bin/sh set -eu root="$(mktemp -d)" trap 'rm -rf "$root"' EXIT mkdir "$root/cmux" chmod 755 "$root/cmux" test -x "$root/cmux" ! test -f "$root/cmux"🤖 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`+CodexFireAndForgetHooks.swift around lines 122 - 124, Update the hook executable selection in the generated shell command around CMUX_CODEX_HOOK_CMUX_BIN, CMUX_BUNDLED_CLI_PATH, and command -v cmux so every candidate is accepted only when it passes both -f and -x. Ensure invalid or directory candidates are cleared or skipped, allowing fallback to the next candidate, and leave the final value empty when no regular executable file is found.
🤖 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.
Outside diff comments:
In `@CLI/CMUXCLI`+CodexFireAndForgetHooks.swift:
- Around line 122-124: Update the hook executable selection in the generated
shell command around CMUX_CODEX_HOOK_CMUX_BIN, CMUX_BUNDLED_CLI_PATH, and
command -v cmux so every candidate is accepted only when it passes both -f and
-x. Ensure invalid or directory candidates are cleared or skipped, allowing
fallback to the next candidate, and leave the final value empty when no regular
executable file is found.
In `@cmuxTests/CLICodexHookTimeoutRegressionTests.swift`:
- Around line 914-916: Synchronize both background-hook assertions before
decoding: at cmuxTests/CLICodexHookTimeoutRegressionTests.swift#L914-L916, poll
until commands.snapshot() contains a request matching the surface.overlay.set
predicate, then decode it; apply the same polling at
cmuxTests/CLICodexHookTimeoutRegressionTests.swift#L989-L992 before selecting
overlayRequest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4937feb8-78dd-4366-ad5a-74eb7bb61f4e
📒 Files selected for processing (2)
CLI/CMUXCLI+CodexFireAndForgetHooks.swiftcmuxTests/CLICodexHookTimeoutRegressionTests.swift
lawrencecchen
added
the
stale-revisit
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.
Adds retained, surface-scoped terminal overlay strips with viewport, captured-scrollback, and sticky anchors.
Each key owns one full-grid-width strip exactly one Ghostty cell high. Multiple keys preserve insertion order and stack on adjacent rows. The AppKit views reject hit testing and focus, so terminal input passes through.
Viewport strips stay at row one. Sticky scrollback strips capture Ghostty top-row identity, follow that row, then pin when it reaches the viewport top. Reflow, reset, or bounded-history eviction invalidates stale captured anchors instead of attaching them to unrelated output.
The control socket and
cmux surface overlay set|list|remove|clearexpose the primitive. CodexUserPromptSubmitupsertsagent.codex.latest-user-messageas a left-aligned viewport strip. Generated hooks use the wrapper-pinned CLI plus explicit workspace and surface IDs, and publish accepted input before turn lifecycle work, so fast replies cannot suppress the strip.A Codex process must be launched through cmux after this build so hooks are injected. Non-empty top-level prompts publish; nested agent prompts and
CMUX_CODEX_HOOKS_DISABLED=1are intentionally suppressed. No scrollbar geometry is required for the Codex viewport strip.Verification: