Repository navigation
Fix laggy terminal sync during sidebar drags - #1598
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
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 drag-aware external-geometry synchronization and debug-only test hooks to terminal-portal scheduling; adds tests, CI/workflow steps and a new UI test, an Xcode project entry, app UITest window relocation logic, and a multi-mode virtual-display churn helper. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Job
participant Helper as create-virtual-display helper
participant Socket as Harness Socket
participant App as Test App (XCUIApplication)
participant Diagnostics as UITest Diagnostics File
CI->>Helper: start helper (modes, ready/start/done paths)
Helper-->>CI: write ready + display-id files
CI->>App: launch with env pointing to harness (socket + paths)
App->>Socket: connect (ping, render_stats)
CI->>Diagnostics: poll for render_stats & diagnostics
CI->>Helper: write start signal
Helper->>Helper: churn display modes (iterations)
Helper-->>Diagnostics: update status/done
CI->>Diagnostics: wait for done, assert render progression
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.
🧹 Nitpick comments (3)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (2)
13382-13387: Restore the prior drag-test flags instead of hard-codingfalse.These are process-global statics. Resetting them to
falseloses any preexisting value and makes later tests depend on execution order. Capture the old values and restore them indeferin both tests.♻️ Suggested change
- WindowTerminalPortal.isPointerDragActiveForTesting = true - TerminalWindowPortalRegistry.isPointerDragActiveForTesting = true + let previousWindowDragFlag = WindowTerminalPortal.isPointerDragActiveForTesting + let previousRegistryDragFlag = TerminalWindowPortalRegistry.isPointerDragActiveForTesting + WindowTerminalPortal.isPointerDragActiveForTesting = true + TerminalWindowPortalRegistry.isPointerDragActiveForTesting = true defer { - WindowTerminalPortal.isPointerDragActiveForTesting = false - TerminalWindowPortalRegistry.isPointerDragActiveForTesting = false + WindowTerminalPortal.isPointerDragActiveForTesting = previousWindowDragFlag + TerminalWindowPortalRegistry.isPointerDragActiveForTesting = previousRegistryDragFlag }Also applies to: 13476-13481
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 13382 - 13387, The test currently forces process-global statics WindowTerminalPortal.isPointerDragActiveForTesting and TerminalWindowPortalRegistry.isPointerDragActiveForTesting to false in the defer, which clobbers any prior state; fix by capturing the original values into local temporaries (e.g., oldWindowDrag and oldRegistryDrag) before setting them true, and then restore those saved values in the defer instead of hard-coding false so the global flags return to their previous state; apply the same change for the other occurrence at the noted lines.
68-79: Consider parameterizing the resize log path for test isolation under future parallel execution.The shared
/tmp/cmux-ghostty-size.logpath is properly reset before and after each test (line 13425, 13427, 13474), and theCMUX_UI_TEST_SPLIT_CLOSE_RIGHT_VISUALenv var is correctly captured and restored via defer (lines 13423–13432). However, if this test suite ever adopts parallel execution, the fixed log path and env var could still collide across concurrent workers. The test flags (WindowTerminalPortal.isPointerDragActiveForTesting,TerminalWindowPortalRegistry.isPointerDragActiveForTesting) are also properly isolated via defer blocks (lines 13382–13387, 13476–13481).Current isolation is sound, but for future-proofing, consider per-test log paths or a threadSafe debug hook that doesn't rely on process-global state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 68 - 79, The tests use a fixed global path ghosttySizeLogPath and helper functions resetGhosttySizeLog() and ghosttySizeLogLines(), which can collide under parallel runs; change the helpers to accept a per-test path (or read from an env var) and update callers to pass a unique temp path per test (e.g., using UUID or NSTemporaryDirectory), ensure each test creates/deletes its own path via resetGhosttySizeLog(path:) and reads via ghosttySizeLogLines(path:), and keep restoring process-global flags (WindowTerminalPortal.isPointerDragActiveForTesting, TerminalWindowPortalRegistry.isPointerDragActiveForTesting) and CMUX_UI_TEST_SPLIT_CLOSE_RIGHT_VISUAL as before.Sources/TerminalWindowPortal.swift (1)
686-694: Consider extracting drag-event detection into a shared helper.The drag-event detection block is duplicated in two places; a small shared helper would reduce drift risk.
Also applies to: 1810-1818
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalWindowPortal.swift` around lines 686 - 694, Extract the duplicated drag-detection logic into a single helper (e.g., a static method on TerminalWindowPortal like isCurrentEventDrag() or an NSEvent extension) that checks Self.isPointerDragActiveForTesting (preserve the DEBUG guard) and inspects NSApp.currentEvent?.type for .leftMouseDragged, .rightMouseDragged, .otherMouseDragged; then replace the inline closure currently assigned to isDragEvent in TerminalWindowPortal and the other duplicated block (the one around the 1810–1818 area) to call this new helper so both sites use the same implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 13382-13387: The test currently forces process-global statics
WindowTerminalPortal.isPointerDragActiveForTesting and
TerminalWindowPortalRegistry.isPointerDragActiveForTesting to false in the
defer, which clobbers any prior state; fix by capturing the original values into
local temporaries (e.g., oldWindowDrag and oldRegistryDrag) before setting them
true, and then restore those saved values in the defer instead of hard-coding
false so the global flags return to their previous state; apply the same change
for the other occurrence at the noted lines.
- Around line 68-79: The tests use a fixed global path ghosttySizeLogPath and
helper functions resetGhosttySizeLog() and ghosttySizeLogLines(), which can
collide under parallel runs; change the helpers to accept a per-test path (or
read from an env var) and update callers to pass a unique temp path per test
(e.g., using UUID or NSTemporaryDirectory), ensure each test creates/deletes its
own path via resetGhosttySizeLog(path:) and reads via
ghosttySizeLogLines(path:), and keep restoring process-global flags
(WindowTerminalPortal.isPointerDragActiveForTesting,
TerminalWindowPortalRegistry.isPointerDragActiveForTesting) and
CMUX_UI_TEST_SPLIT_CLOSE_RIGHT_VISUAL as before.
In `@Sources/TerminalWindowPortal.swift`:
- Around line 686-694: Extract the duplicated drag-detection logic into a single
helper (e.g., a static method on TerminalWindowPortal like isCurrentEventDrag()
or an NSEvent extension) that checks Self.isPointerDragActiveForTesting
(preserve the DEBUG guard) and inspects NSApp.currentEvent?.type for
.leftMouseDragged, .rightMouseDragged, .otherMouseDragged; then replace the
inline closure currently assigned to isDragEvent in TerminalWindowPortal and the
other duplicated block (the one around the 1810–1818 area) to call this new
helper so both sites use the same implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2560253-9484-4fb6-99da-a34463b61b3e
📒 Files selected for processing (2)
Sources/TerminalWindowPortal.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:424">
P2: Use an exact Zig version comparison; the current regex prefix check can incorrectly accept non-required versions.</violation>
<violation number="2" location=".github/workflows/ci.yml:432">
P2: Replace the Zig lib directory before copying; otherwise an existing install can produce nested `/usr/local/lib/zig/lib` and a broken Zig runtime layout.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| - name: Install zig | ||
| run: | | ||
| ZIG_REQUIRED="0.15.2" | ||
| if command -v zig >/dev/null 2>&1 && zig version 2>/dev/null | grep -q "^${ZIG_REQUIRED}"; then |
There was a problem hiding this comment.
P2: Use an exact Zig version comparison; the current regex prefix check can incorrectly accept non-required versions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/ci.yml, line 424:
<comment>Use an exact Zig version comparison; the current regex prefix check can incorrectly accept non-required versions.</comment>
<file context>
@@ -385,3 +385,128 @@ jobs:
+ - name: Install zig
+ run: |
+ ZIG_REQUIRED="0.15.2"
+ if command -v zig >/dev/null 2>&1 && zig version 2>/dev/null | grep -q "^${ZIG_REQUIRED}"; then
+ echo "zig ${ZIG_REQUIRED} already installed"
+ else
</file context>
Suggested change
| if command -v zig >/dev/null 2>&1 && zig version 2>/dev/null | grep -q "^${ZIG_REQUIRED}"; then | |
| if command -v zig >/dev/null 2>&1 && [ "$(zig version 2>/dev/null)" = "${ZIG_REQUIRED}" ]; then |
| tar xf /tmp/zig.tar.xz -C /tmp | ||
| sudo mkdir -p /usr/local/bin /usr/local/lib | ||
| sudo cp -f /tmp/zig-aarch64-macos-${ZIG_REQUIRED}/zig /usr/local/bin/zig | ||
| sudo cp -rf /tmp/zig-aarch64-macos-${ZIG_REQUIRED}/lib /usr/local/lib/zig |
There was a problem hiding this comment.
P2: Replace the Zig lib directory before copying; otherwise an existing install can produce nested /usr/local/lib/zig/lib and a broken Zig runtime layout.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/ci.yml, line 432:
<comment>Replace the Zig lib directory before copying; otherwise an existing install can produce nested `/usr/local/lib/zig/lib` and a broken Zig runtime layout.</comment>
<file context>
@@ -385,3 +385,128 @@ jobs:
+ tar xf /tmp/zig.tar.xz -C /tmp
+ sudo mkdir -p /usr/local/bin /usr/local/lib
+ sudo cp -f /tmp/zig-aarch64-macos-${ZIG_REQUIRED}/zig /usr/local/bin/zig
+ sudo cp -rf /tmp/zig-aarch64-macos-${ZIG_REQUIRED}/lib /usr/local/lib/zig
+ export PATH="/usr/local/bin:$PATH"
+ zig version
</file context>
Suggested change
| sudo cp -rf /tmp/zig-aarch64-macos-${ZIG_REQUIRED}/lib /usr/local/lib/zig | |
| sudo rm -rf /usr/local/lib/zig | |
| sudo cp -rf /tmp/zig-aarch64-macos-${ZIG_REQUIRED}/lib /usr/local/lib/zig |
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 (1)
Sources/AppDelegate.swift (1)
2328-2406:⚠️ Potential issue | 🟡 MinorUse a deterministic main test window for move + success reporting.
Line 2376 moves
NSApp.windows.first, while Lines 2342-2344 mark success if any window lands on the target display. If an auxiliary window/panel exists first, this can move the wrong window and still report success, making the regression test flaky or falsely green.♻️ Suggested fix
+ private func uiTestTargetWindow() -> NSWindow? { + if let keyWindow = NSApp.keyWindow, isMainTerminalWindow(keyWindow) { + return keyWindow + } + if let mainWindow = NSApp.mainWindow, isMainTerminalWindow(mainWindow) { + return mainWindow + } + return NSApp.orderedWindows.first(where: { isMainTerminalWindow($0) }) + ?? NSApp.windows.first + } + private func writeUITestDiagnosticsIfNeeded(stage: String) { let env = ProcessInfo.processInfo.environment guard let path = env["CMUX_UI_TEST_DIAGNOSTICS_PATH"], !path.isEmpty else { return } @@ - if let rawDisplayID = UInt32(targetDisplayID) { + if let rawDisplayID = UInt32(targetDisplayID) { let screenPresent = NSScreen.screens.contains(where: { $0.cmuxDisplayID == rawDisplayID }) - let movedWindow = windows.contains(where: { $0.screen?.cmuxDisplayID == rawDisplayID }) + let movedWindow = uiTestTargetWindow()?.screen?.cmuxDisplayID == rawDisplayID payload["targetDisplayPresent"] = screenPresent ? "1" : "0" payload["targetDisplayMoveSucceeded"] = movedWindow ? "1" : "0" } @@ - guard let window = NSApp.windows.first else { + guard let window = uiTestTargetWindow() else { if attempt < 20 { DispatchQueue.main.asyncAfter(deadline: .now() + 0.25) { [weak self] in self?.moveUITestWindowToTargetDisplayIfNeeded(attempt: attempt + 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 2328 - 2406, The current moveUITestWindowToTargetDisplayIfNeeded uses NSApp.windows.first and the diagnostics logic checks any window landing on the target display, which can pick an auxiliary panel and give false success; change window selection to a deterministic main test window (use NSApp.mainWindow ?? NSApp.keyWindow ?? NSApp.windows.first(where: { $0.isVisible }) ) inside moveUITestWindowToTargetDisplayIfNeeded and use that same selectedWindow when evaluating movedWindow and reporting via writeUITestDiagnosticsIfNeeded so both the move target and the success check consistently operate on the same window (update any references to NSApp.windows.first and the movedWindow calculation accordingly).
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
437-442: Consider adding DerivedData caching for build performance.Other jobs cache DerivedData to speed up incremental builds. This job omits it, which may increase build times. If this is intentional (e.g., to ensure clean builds for regression tests), a comment explaining the rationale would help.
♻️ Optional DerivedData cache addition
+ - name: Cache DerivedData + uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4 + with: + path: ~/Library/Developer/Xcode/DerivedData/GhosttyTabs-* + key: deriveddata-ui-display-${{ hashFiles('GhosttyTabs.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved') }}-${{ hashFiles('GhosttyTabs.xcodeproj/project.pbxproj') }} + restore-keys: | + deriveddata-ui-display-${{ hashFiles('GhosttyTabs.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved') }}- + deriveddata-ui-display- + - name: Cache Swift packages uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 437 - 442, Add a DerivedData cache to the CI job that currently has the "Cache Swift packages" step to improve incremental build performance: extend the existing actions/cache@... step (or add a new cache step near the "Cache Swift packages" block) to include path(s) for Xcode DerivedData (e.g., DerivedData or ~/Library/Developer/Xcode/DerivedData) and a unique key (for example include runner/os and the workspace/project hash similar to spm-ui-display-resolution-${{ hashFiles('GhosttyTabs.xcodeproj/.../Package.resolved') }}), plus appropriate restore-keys; alternatively, if skipping DerivedData is intentional, add a comment above the "Cache Swift packages" step stating the rationale so reviewers know the omission is deliberate.
418-419: Consider caching GhosttyKit.xcframework for faster builds.Other jobs (
tests,tests-build-and-lag) cache this artifact before downloading. Adding cache logic would reduce CI time when the framework hasn't changed.♻️ Proposed cache addition
+ - name: Cache GhosttyKit.xcframework + id: cache-ghosttykit-ui-display + uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4 + with: + path: GhosttyKit.xcframework + key: ghosttykit-${{ hashFiles('.gitmodules', 'ghostty') }} + - name: Download pre-built GhosttyKit.xcframework + if: steps.cache-ghosttykit-ui-display.outputs.cache-hit != 'true' run: ./scripts/download-prebuilt-ghosttykit.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 418 - 419, Add caching around the "Download pre-built GhosttyKit.xcframework" step: insert an actions/cache restore step before the existing run: ./scripts/download-prebuilt-ghosttykit.sh (use the step name "Restore GhosttyKit.xcframework cache") keyed to a stable identifier (e.g., GhosttyKit version or checksum of scripts/download-prebuilt-ghosttykit.sh) and with restore-keys for misses; keep the existing run step to populate the framework if cache misses; then add a subsequent actions/cache save step (or a cache step with path pointing to where the script places GhosttyKit.xcframework) so future runs (including jobs tests and tests-build-and-lag) can restore the framework instead of re-downloading.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxUITests/DisplayResolutionRegressionUITests.swift`:
- Around line 32-37: The tearDown() sequence is removing harness artifacts that
may be owned by an external helper (when CMUX_UI_TEST_DISPLAY_* is set), making
shared-harness runs non-reentrant; update tearDown() (and the helper cleanup
logic in removeTestArtifacts()) to avoid deleting caller-owned files by checking
for the external-harness signal (e.g. presence of CMUX_UI_TEST_DISPLAY_* env
vars or a helper-owned flag) before calling removeTestArtifacts(), while still
terminating and waiting for helperProcess; ensure removeTestArtifacts() itself
is guarded to no-op when the external-harness indicator is present so the same
protection applies wherever it's called (also apply the same guard to the other
teardown/cleanup sites referenced).
- Around line 185-190: The waitForTargetDisplayMove function currently uses
substring matching on diagnostics["windowScreenDisplayIDs"] which can yield
false positives; update the closure in waitForTargetDisplayMove to parse the
windowScreenDisplayIDs string into exact ID tokens (split by commas, whitespace,
newlines, or other separators) and then check that one of the tokens equals
targetDisplayID (exact equality) while still verifying
diagnostics["targetDisplayMoveSucceeded"] == "1"; handle the optional safely
(guard-let the string) and treat empty or malformed payloads as a failure.
In `@scripts/create-virtual-display.m`:
- Around line 227-236: Before signaling readiness, ensure any stale marker files
at startPath and donePath are cleared so old runs don't trigger immediate churn
or premature completion; locate the block that writes displayIDPath and
readyPath (uses writeString, display.displayID, displayIDPath, readyPath) and,
before calling writeString(@"ready\n", readyPath), remove or reset files at
startPath and donePath (check for and delete via NSFileManager or overwrite)
when iterations > 0 && modeSpecs.count > 1; keep the existing start-path wait
logic unchanged so fresh markers control this run.
---
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 2328-2406: The current moveUITestWindowToTargetDisplayIfNeeded
uses NSApp.windows.first and the diagnostics logic checks any window landing on
the target display, which can pick an auxiliary panel and give false success;
change window selection to a deterministic main test window (use
NSApp.mainWindow ?? NSApp.keyWindow ?? NSApp.windows.first(where: { $0.isVisible
}) ) inside moveUITestWindowToTargetDisplayIfNeeded and use that same
selectedWindow when evaluating movedWindow and reporting via
writeUITestDiagnosticsIfNeeded so both the move target and the success check
consistently operate on the same window (update any references to
NSApp.windows.first and the movedWindow calculation accordingly).
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 437-442: Add a DerivedData cache to the CI job that currently has
the "Cache Swift packages" step to improve incremental build performance: extend
the existing actions/cache@... step (or add a new cache step near the "Cache
Swift packages" block) to include path(s) for Xcode DerivedData (e.g.,
DerivedData or ~/Library/Developer/Xcode/DerivedData) and a unique key (for
example include runner/os and the workspace/project hash similar to
spm-ui-display-resolution-${{
hashFiles('GhosttyTabs.xcodeproj/.../Package.resolved') }}), plus appropriate
restore-keys; alternatively, if skipping DerivedData is intentional, add a
comment above the "Cache Swift packages" step stating the rationale so reviewers
know the omission is deliberate.
- Around line 418-419: Add caching around the "Download pre-built
GhosttyKit.xcframework" step: insert an actions/cache restore step before the
existing run: ./scripts/download-prebuilt-ghosttykit.sh (use the step name
"Restore GhosttyKit.xcframework cache") keyed to a stable identifier (e.g.,
GhosttyKit version or checksum of scripts/download-prebuilt-ghosttykit.sh) and
with restore-keys for misses; keep the existing run step to populate the
framework if cache misses; then add a subsequent actions/cache save step (or a
cache step with path pointing to where the script places GhosttyKit.xcframework)
so future runs (including jobs tests and tests-build-and-lag) can restore the
framework instead of re-downloading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f9e551c4-2e17-4fb2-9872-f2e90829d35d
📒 Files selected for processing (5)
.github/workflows/ci.ymlGhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftcmuxUITests/DisplayResolutionRegressionUITests.swiftscripts/create-virtual-display.m
Comment on lines
+185
to
+190
| private func waitForTargetDisplayMove(targetDisplayID: String, timeout: TimeInterval) -> Bool { | ||
| waitForCondition(timeout: timeout) { | ||
| guard let diagnostics = self.loadDiagnostics() else { return false } | ||
| return diagnostics["targetDisplayMoveSucceeded"] == "1" && | ||
| diagnostics["windowScreenDisplayIDs"]?.contains(targetDisplayID) == true | ||
| } |
There was a problem hiding this comment.
Avoid substring matching for the target display ID.
contains(targetDisplayID) can report success for the wrong display when another ID happens to include the same digits, which turns this into a false-positive gate. Parse exact IDs from the diagnostics payload before comparing.
🛠️ Suggested change
private func waitForTargetDisplayMove(targetDisplayID: String, timeout: TimeInterval) -> Bool {
waitForCondition(timeout: timeout) {
guard let diagnostics = self.loadDiagnostics() else { return false }
- return diagnostics["targetDisplayMoveSucceeded"] == "1" &&
- diagnostics["windowScreenDisplayIDs"]?.contains(targetDisplayID) == true
+ guard diagnostics["targetDisplayMoveSucceeded"] == "1" else { return false }
+ let displayIDs = diagnostics["windowScreenDisplayIDs"]?
+ .split(whereSeparator: { !$0.isNumber })
+ .map(String.init) ?? []
+ return displayIDs.contains(targetDisplayID)
}
}📝 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 waitForTargetDisplayMove(targetDisplayID: String, timeout: TimeInterval) -> Bool { | |
| waitForCondition(timeout: timeout) { | |
| guard let diagnostics = self.loadDiagnostics() else { return false } | |
| return diagnostics["targetDisplayMoveSucceeded"] == "1" && | |
| diagnostics["windowScreenDisplayIDs"]?.contains(targetDisplayID) == true | |
| } | |
| private func waitForTargetDisplayMove(targetDisplayID: String, timeout: TimeInterval) -> Bool { | |
| waitForCondition(timeout: timeout) { | |
| guard let diagnostics = self.loadDiagnostics() else { return false } | |
| guard diagnostics["targetDisplayMoveSucceeded"] == "1" else { return false } | |
| let displayIDs = diagnostics["windowScreenDisplayIDs"]? | |
| .split(whereSeparator: { !$0.isNumber }) | |
| .map(String.init) ?? [] | |
| return displayIDs.contains(targetDisplayID) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxUITests/DisplayResolutionRegressionUITests.swift` around lines 185 - 190,
The waitForTargetDisplayMove function currently uses substring matching on
diagnostics["windowScreenDisplayIDs"] which can yield false positives; update
the closure in waitForTargetDisplayMove to parse the windowScreenDisplayIDs
string into exact ID tokens (split by commas, whitespace, newlines, or other
separators) and then check that one of the tokens equals targetDisplayID (exact
equality) while still verifying diagnostics["targetDisplayMoveSucceeded"] ==
"1"; handle the optional safely (guard-let the string) and treat empty or
malformed payloads as a failure.
Comment on lines
+227
to
+236
| writeString([NSString stringWithFormat:@"%u\n", display.displayID], displayIDPath); | ||
| writeString(@"ready\n", readyPath); | ||
|
|
||
| if (iterations > 0 && modeSpecs.count > 1) { | ||
| dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0), ^{ | ||
| if (startPath.length > 0) { | ||
| while (![[NSFileManager defaultManager] fileExistsAtPath:startPath]) { | ||
| usleep(20 * 1000); | ||
| } | ||
| } |
There was a problem hiding this comment.
Clear stale start/done markers before signaling readiness.
If these paths are reused, an old start file makes churn begin immediately and an old done file can satisfy the test's completion check before this run finishes. Reset both markers before writing ready.
🛠️ Suggested change
printf("Virtual display created: %ux%u@60Hz (displayID: %u)\n", width, height, display.displayID);
printf("PID: %d\n", getpid());
fflush(stdout);
+ if (startPath.length > 0) {
+ [[NSFileManager defaultManager] removeItemAtPath:startPath error:nil];
+ }
+ if (donePath.length > 0) {
+ [[NSFileManager defaultManager] removeItemAtPath:donePath error:nil];
+ }
writeString([NSString stringWithFormat:@"%u\n", display.displayID], displayIDPath);
writeString(@"ready\n", readyPath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/create-virtual-display.m` around lines 227 - 236, Before signaling
readiness, ensure any stale marker files at startPath and donePath are cleared
so old runs don't trigger immediate churn or premature completion; locate the
block that writes displayIDPath and readyPath (uses writeString,
display.displayID, displayIDPath, readyPath) and, before calling
writeString(@"ready\n", readyPath), remove or reset files at startPath and
donePath (check for and delete via NSFileManager or overwrite) when iterations >
0 && modeSpecs.count > 1; keep the existing start-path wait logic unchanged so
fresh markers control this run.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test-e2e.yml:
- Around line 236-279: The workflow only enables the display harness when
TEST_FILTER exactly equals "DisplayResolutionRegressionUITests"; change that
conditional so it also matches method-scoped filters like
"DisplayResolutionRegressionUITests/..." by using a prefix test (for example
replace if [ "$TEST_FILTER" = "DisplayResolutionRegressionUITests" ] with a
shell pattern or case test such as [[ "$TEST_FILTER" ==
DisplayResolutionRegressionUITests* ]] or case "$TEST_FILTER" in
DisplayResolutionRegressionUITests*) to detect both class-only and Class/Method
values before running the block that creates the display harness and sets
CMUX_UI_TEST_DISPLAY_* env paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58bd81f0-44be-462c-8014-1e6fda462470
📒 Files selected for processing (1)
.github/workflows/test-e2e.yml
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxUITests/DisplayResolutionRegressionUITests.swift">
<violation number="1" location="cmuxUITests/DisplayResolutionRegressionUITests.swift:120">
P2: Using a fixed `/tmp` manifest as an unconditional fallback can make this UI test pick up stale harness metadata and become flaky across runs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
cmuxUITests/DisplayResolutionRegressionUITests.swift (2)
33-39:⚠️ Potential issue | 🟡 MinorDon't delete caller-owned harness artifacts when using external harness.
When
CMUX_UI_TEST_DISPLAY_*environment variables are supplied, the signal files belong to the CI workflow's display helper process.removeTestArtifacts()intearDown()would delete files that the external process may still be using or that are needed for post-run diagnostics.Proposed fix to track harness ownership
private var helperProcess: Process? + private var ownsDisplayHarness = false override func setUp() { // ...existing code... + ownsDisplayHarness = true // Will be set false if external harness is loaded } override func tearDown() { helperProcess?.terminate() helperProcess?.waitUntilExit() helperProcess = nil - removeTestArtifacts() + removeTestArtifacts(includeDisplayHarness: ownsDisplayHarness) super.tearDown() } - private func removeTestArtifacts() { - for path in [ + private func removeTestArtifacts(includeDisplayHarness: Bool = true) { + var paths = [ socketPath, diagnosticsPath, - displayReadyPath, - displayIDPath, - displayStartPath, - displayDonePath, helperBinaryPath, - helperLogPath, - ] { + ] + if includeDisplayHarness { + paths += [displayReadyPath, displayIDPath, displayStartPath, displayDonePath, helperLogPath] + } + for path in paths { // ... } }And in
prepareDisplayHarnessIfNeeded()after loading external harness:+ ownsDisplayHarness = false return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/DisplayResolutionRegressionUITests.swift` around lines 33 - 39, The tearDown() currently always calls removeTestArtifacts(), which will delete CI-owned display signal files when an external harness is used (triggered by CMUX_UI_TEST_DISPLAY_* env vars); change the logic so tearDown() only calls removeTestArtifacts() when this test process owns the harness artifacts (add a Boolean flag like ownsDisplayHarness set to false by default), set ownsDisplayHarness = true inside prepareDisplayHarnessIfNeeded() only when you create the helper process locally (and leave it false when you load the external harness), and then guard the removeTestArtifacts() call in tearDown() with if ownsDisplayHarness { removeTestArtifacts() } so external CI files are not removed.
210-216:⚠️ Potential issue | 🟡 MinorAvoid substring matching for the target display ID.
contains(targetDisplayID)can match incorrectly when one display ID is a substring of another (e.g., searching for "123" in "123456"). Parse exact IDs from the diagnostics before comparing.Proposed fix for exact matching
private func waitForTargetDisplayMove(targetDisplayID: String, timeout: TimeInterval) -> Bool { waitForCondition(timeout: timeout) { guard let diagnostics = self.loadDiagnostics() else { return false } - return diagnostics["targetDisplayMoveSucceeded"] == "1" && - diagnostics["windowScreenDisplayIDs"]?.contains(targetDisplayID) == true + guard diagnostics["targetDisplayMoveSucceeded"] == "1" else { return false } + guard let idsString = diagnostics["windowScreenDisplayIDs"] else { return false } + let displayIDs = idsString.split(separator: ",").map { $0.trimmingCharacters(in: .whitespaces) } + return displayIDs.contains(targetDisplayID) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/DisplayResolutionRegressionUITests.swift` around lines 210 - 216, In waitForTargetDisplayMove, avoid substring matching by parsing the diagnostics["windowScreenDisplayIDs"] into discrete display ID tokens (e.g., split into an array of IDs) and check for exact equality against targetDisplayID instead of using contains(targetDisplayID); update the closure that calls loadDiagnostics() to safely unwrap diagnostics["windowScreenDisplayIDs"], split into exact IDs, and verify diagnostics["targetDisplayMoveSucceeded"] == "1" and that the parsed ID array contains targetDisplayID..github/workflows/test-e2e.yml (1)
236-236:⚠️ Potential issue | 🟡 MinorHandle method-scoped test filters when enabling the display harness.
Line 236 only matches
DisplayResolutionRegressionUITestsexactly. The workflow input supportsClass/Method, soDisplayResolutionRegressionUITests/testRapid...currently skips harness setup.Proposed fix
- if [ "$TEST_FILTER" = "DisplayResolutionRegressionUITests" ]; then + if [[ "$TEST_FILTER" == "DisplayResolutionRegressionUITests" || "$TEST_FILTER" == "DisplayResolutionRegressionUITests/"* ]]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test-e2e.yml at line 236, The condition checks TEST_FILTER for an exact match to "DisplayResolutionRegressionUITests" so method-scoped filters like "DisplayResolutionRegressionUITests/testRapid..." are skipped; update the if that references TEST_FILTER and "DisplayResolutionRegressionUITests" to perform a prefix or glob match (e.g., treat any value that starts with or matches the pattern "DisplayResolutionRegressionUITests*" or uses shell pattern matching) so the display harness setup runs for both class-only and Class/Method filters.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
418-419: Consider caching GhosttyKit.xcframework like other jobs.This job downloads GhosttyKit directly without first checking a cache. Other jobs in this file (e.g.,
testsat lines 102-113,tests-build-and-lagat lines 264-274) use the cache-then-download pattern to avoid redundant downloads.Proposed fix to add caching
+ - name: Cache GhosttyKit.xcframework + id: cache-ghosttykit-display + uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4 + with: + path: GhosttyKit.xcframework + key: ghosttykit-${{ hashFiles('.gitmodules', 'ghostty') }} + - name: Download pre-built GhosttyKit.xcframework + if: steps.cache-ghosttykit-display.outputs.cache-hit != 'true' run: ./scripts/download-prebuilt-ghosttykit.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 418 - 419, Update the "Download pre-built GhosttyKit.xcframework" step to use the same cache-then-download pattern as other jobs: add a cache restore/save step keyed by the GhosttyKit.xcframework artifact (use the same cache key/paths pattern as `tests` and `tests-build-and-lag`), check the cache before running ./scripts/download-prebuilt-ghosttykit.sh, and save the framework into the cache after download so subsequent workflow runs reuse the cached GhosttyKit.xcframework instead of always downloading.cmuxUITests/DisplayResolutionRegressionUITests.swift (1)
199-208: FileHandle creation can silently fail, discarding helper output.If
FileHandle(forWritingAtPath:)returnsnilboth times (e.g., permissions issue),logHandlewill beniland the helper's stdout/stderr will be discarded. This makes debugging harder when the test fails.Proposed fix to log a warning
let logHandle = FileHandle(forWritingAtPath: helperLogPath) ?? { FileManager.default.createFile(atPath: helperLogPath, contents: nil) return FileHandle(forWritingAtPath: helperLogPath) }() + if logHandle == nil { + print("Warning: Could not create log file handle at \(helperLogPath)") + } proc.standardOutput = logHandle proc.standardError = logHandle🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/DisplayResolutionRegressionUITests.swift` around lines 199 - 208, The current creation of logHandle can end up nil and silently drop helper output; update the helper setup so after attempting FileHandle(forWritingAtPath: helperLogPath) and creating the file you check if logHandle is still nil, and if so: log a clear warning (e.g., use XCTFail/print/XCTContext or the test logger) indicating inability to open helperLogPath and then fall back to a safe handle such as FileHandle.standardError (or FileHandle.standardOutput) before assigning proc.standardOutput and proc.standardError; reference the symbols helperLogPath, logHandle, proc.standardOutput/proc.standardError, helperProcess and the try proc.run() call so the nil-handling and warning are colocated with the existing logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/test-e2e.yml:
- Line 236: The condition checks TEST_FILTER for an exact match to
"DisplayResolutionRegressionUITests" so method-scoped filters like
"DisplayResolutionRegressionUITests/testRapid..." are skipped; update the if
that references TEST_FILTER and "DisplayResolutionRegressionUITests" to perform
a prefix or glob match (e.g., treat any value that starts with or matches the
pattern "DisplayResolutionRegressionUITests*" or uses shell pattern matching) so
the display harness setup runs for both class-only and Class/Method filters.
In `@cmuxUITests/DisplayResolutionRegressionUITests.swift`:
- Around line 33-39: The tearDown() currently always calls
removeTestArtifacts(), which will delete CI-owned display signal files when an
external harness is used (triggered by CMUX_UI_TEST_DISPLAY_* env vars); change
the logic so tearDown() only calls removeTestArtifacts() when this test process
owns the harness artifacts (add a Boolean flag like ownsDisplayHarness set to
false by default), set ownsDisplayHarness = true inside
prepareDisplayHarnessIfNeeded() only when you create the helper process locally
(and leave it false when you load the external harness), and then guard the
removeTestArtifacts() call in tearDown() with if ownsDisplayHarness {
removeTestArtifacts() } so external CI files are not removed.
- Around line 210-216: In waitForTargetDisplayMove, avoid substring matching by
parsing the diagnostics["windowScreenDisplayIDs"] into discrete display ID
tokens (e.g., split into an array of IDs) and check for exact equality against
targetDisplayID instead of using contains(targetDisplayID); update the closure
that calls loadDiagnostics() to safely unwrap
diagnostics["windowScreenDisplayIDs"], split into exact IDs, and verify
diagnostics["targetDisplayMoveSucceeded"] == "1" and that the parsed ID array
contains targetDisplayID.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 418-419: Update the "Download pre-built GhosttyKit.xcframework"
step to use the same cache-then-download pattern as other jobs: add a cache
restore/save step keyed by the GhosttyKit.xcframework artifact (use the same
cache key/paths pattern as `tests` and `tests-build-and-lag`), check the cache
before running ./scripts/download-prebuilt-ghosttykit.sh, and save the framework
into the cache after download so subsequent workflow runs reuse the cached
GhosttyKit.xcframework instead of always downloading.
In `@cmuxUITests/DisplayResolutionRegressionUITests.swift`:
- Around line 199-208: The current creation of logHandle can end up nil and
silently drop helper output; update the helper setup so after attempting
FileHandle(forWritingAtPath: helperLogPath) and creating the file you check if
logHandle is still nil, and if so: log a clear warning (e.g., use
XCTFail/print/XCTContext or the test logger) indicating inability to open
helperLogPath and then fall back to a safe handle such as
FileHandle.standardError (or FileHandle.standardOutput) before assigning
proc.standardOutput and proc.standardError; reference the symbols helperLogPath,
logHandle, proc.standardOutput/proc.standardError, helperProcess and the try
proc.run() call so the nil-handling and warning are colocated with the existing
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1edfcf9-4fcb-4cec-b858-80e6123fd4d6
📒 Files selected for processing (3)
.github/workflows/ci.yml.github/workflows/test-e2e.ymlcmuxUITests/DisplayResolutionRegressionUITests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test-e2e.yml:
- Line 111: The workflow currently checks exact equality for inputs.test_filter
which misses method-scoped filters like
"DisplayResolutionRegressionUITests/testSomething"; change the condition to use
startsWith so both the suite name and suite/method forms are caught. Replace the
current if: ${{ inputs.test_filter != 'DisplayResolutionRegressionUITests' }}
with a negated startsWith check such as if: ${{
not(startsWith(inputs.test_filter, 'DisplayResolutionRegressionUITests')) }} so
the virtual display creation is skipped for either the suite name or any
method-scoped filter (reference: inputs.test_filter and the
"DisplayResolutionRegressionUITests" filter name).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd17f14b-078b-4184-a5b5-d50a6e4e7d04
📒 Files selected for processing (1)
.github/workflows/test-e2e.yml
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxUITests/DisplayResolutionRegressionUITests.swift">
<violation number="1" location="cmuxUITests/DisplayResolutionRegressionUITests.swift:72">
P2: Reassigning `socketPath` to a fallback-discovered socket can make teardown delete a socket the test did not create.</violation>
<violation number="2" location="cmuxUITests/DisplayResolutionRegressionUITests.swift:281">
P2: Fallback socket discovery is too broad and can select a different running cmux instance, making this UI test flaky or falsely passing against the wrong app.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:2382">
P2: Synchronous socket ping in diagnostics runs on the main thread and can stall UI-test startup/responsiveness.</violation>
<violation number="2" location="Sources/AppDelegate.swift:2550">
P2: The socket sanity check performs blocking ping I/O on `.main`; run the probe off-main and hop back only to update diagnostics/restart state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
This was referenced Mar 18, 2026
3 of 6 tasks
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
* Fix sidebar drag terminal resize lag * Add display resolution churn regression * Prelaunch display churn helper in e2e workflow * Use manifest handoff for display churn UI test * Fix e2e display churn harness startup * Resolve display churn UI test socket path * Use marker-based socket discovery in display UI test * Add failing sidebar drag portal regression tests * Fix sidebar drag terminal portal resize lag * Add failing scoped resize regression tests * Fix terminal portal resize scheduling lag * Add failing zsh resize prompt regression test * Fix zsh resize prompt duplication * Fix Sequoia sidebar resize regression * Guard display-resolution CI runner * Run display-resolution CI on WarpBuild * Allow backgrounded display regression app launch * Launch display regression app directly * Launch display regression app via NSWorkspace * Load display regression launch env from manifest * Write display regression manifest in runner temp dir * Write display regression manifest in shared tmp * Write display regression manifest in repo scratch dir * Launch display regression app with explicit env * Avoid xcodebuild broken pipe in compat CI * Launch display regression via XCUIApplication * Harden display regression socket readiness * Trust display socket diagnostics path * Replace display socket probe with render diagnostics * Write display churn start marker atomically * Move display churn harness out of /tmp --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
7 tasks done
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
1 participant
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.
Task: fix laggy terminal resize and extra prompt redraw when resizing the sidebar.
Summary
Testing
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-task-cyber-resize-lag-verify -only-testing:cmuxTests/TerminalWindowPortalLifecycleTests/testScheduledExternalGeometrySyncWaitsForQueuedLayoutShift -only-testing:cmuxTests/TerminalWindowPortalLifecycleTests/testScheduledExternalGeometrySyncKeepsDragDrivenResizeResponsive -only-testing:cmuxTests/TerminalWindowPortalLifecycleTests/testDragDrivenSidebarResizeDoesNotScheduleLateSecondTerminalResize test./scripts/reload.sh --tag task-cyber-resize-lagIssues
Summary by cubic
Fix laggy terminal updates during sidebar drags and stop duplicate
zshprompts on resize. Add a display‑resolution churn UI test and CI job that verify continuous renders and window placement on a rapidly changing virtual display.Bug Fixes
NSScrollViewlayout on width changes to eliminate a queue‑turn lag.zsh: drop spacer write on SIGWINCH to stop duplicate prompts.New Features
-cmuxUITestLaunchManifest, and skips the default virtual‑display step for this test.Written for commit 895124f. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests
New Features
Chores