Replace legacy window screenshot capture - #9066
Conversation
|
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:
📝 WalkthroughWalkthroughUpdates window screenshot capture to use the AppKit PNG path exclusively and adds a UI regression test that verifies the returned PNG contains non-blank terminal content. ChangesWindow screenshot validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxUITests/AutomationSocketUITests.swift (1)
253-256: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReplace the fixed
sleep 60with a non-time-based blocker.This test fixture uses an arbitrary wall-clock delay only to keep the shell alive. Use a blocking command such as
tail -f /dev/null, allowing teardown to end it without coupling test correctness to a 60-second timeout.Proposed fix
- i=0; while [ $i -lt 8 ]; do printf '\033[48;2;245;40;210m%-80s\033[0m\n' '\(marker)'; i=$((i+1)); done; sleep 60 + i=0; while [ $i -lt 8 ]; do printf '\033[48;2;245;40;210m%-80s\033[0m\n' '\(marker)'; i=$((i+1)); done; tail -f /dev/null🤖 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 253 - 256, Replace the fixed sleep in the markerCommand fixture with a non-time-based blocking command such as tail -f /dev/null, keeping the shell alive until teardown terminates it. Preserve the existing marker output loop and command behavior before the blocker.Source: Coding guidelines
🤖 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 239-243: In the app launch setup around app.launch(), narrow the
handling to the known headless foreground-activation race rather than marking
all launch failures as expected. Remove the broad XCTExpectFailure wrapper and
use targeted conditional handling with authoritative diagnostics, skipping only
when that specific headless behavior is detected while preserving genuine launch
failures.
---
Outside diff comments:
In `@cmuxUITests/AutomationSocketUITests.swift`:
- Around line 253-256: Replace the fixed sleep in the markerCommand fixture with
a non-time-based blocking command such as tail -f /dev/null, keeping the shell
alive until teardown terminates it. Preserve the existing marker output loop and
command behavior before the blocker.
🪄 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: 23cc9170-e37c-423a-bb9a-1277b7b81d3b
📒 Files selected for processing (1)
cmuxUITests/AutomationSocketUITests.swift
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. |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
@austinywang I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,631 of the 240,000 allowed lines of code this month. Reviews resume on 1 August 2026 (in 4 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
✅ Action performedReview finished.
|
|
To use Codex here, create a Codex account and connect to github. |
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)
cmuxUITests/AutomationSocketUITests.swift (2)
284-287: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWait for rendered content, not only terminal-buffer content.
surface.read_textproves that the marker reached the terminal model, but not that AppKit has rendered it beforedebug.window.screenshotruns. Poll the screenshot result untilmarkerPixelsis above the threshold, or use an authoritative render-completion signal, to prevent CI flakes.🤖 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 284 - 287, Update the screenshot synchronization around waitForTerminalText so it waits for rendered output rather than only terminal-buffer state. Poll debug.window.screenshot until markerPixels exceeds the required threshold, or use an equivalent authoritative render-completion signal, before capturing the final screenshot.
334-352: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid direct wall-clock polling in the test helper.
Date()plusRunLoop.current.runintroduces a real wall-clock dependency. Use an XCTest expectation/waiter around the socket predicate, or an injected virtual clock, while retaining a bounded timeout.As per coding guidelines and path instructions, tests must avoid direct wall-clock APIs and use completion signals, virtual clocks, or approved bounded predicate polling.
🤖 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 334 - 352, Update waitForTerminalText to remove direct Date() and RunLoop.current polling, and use an XCTest expectation/waiter with a bounded timeout around the socket.read_text predicate. Preserve the existing surfaceID lookup, expectedText matching, and Bool result behavior.Sources: Coding guidelines, Path instructions
🤖 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 `@cmuxUITests/AutomationSocketUITests.swift`:
- Around line 284-287: Update the screenshot synchronization around
waitForTerminalText so it waits for rendered output rather than only
terminal-buffer state. Poll debug.window.screenshot until markerPixels exceeds
the required threshold, or use an equivalent authoritative render-completion
signal, before capturing the final screenshot.
- Around line 334-352: Update waitForTerminalText to remove direct Date() and
RunLoop.current polling, and use an XCTest expectation/waiter with a bounded
timeout around the socket.read_text predicate. Preserve the existing surfaceID
lookup, expectedText matching, and Bool result behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 692c7c10-c246-448f-a453-265d76044ceb
📒 Files selected for processing (1)
cmuxUITests/AutomationSocketUITests.swift
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@lawrencecchen The final implementation is stable at |
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. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/TerminalController.swift">
<violation number="1" location="Sources/TerminalController.swift:13308">
P3: When the AppKit capture times out (socketAwaitCallback returns nil after 5s), the caller returns "Failed to create PNG data", but `captureTask?.cancel()` does not stop the already-scheduled `Task { @MainActor in ... }`. `Task.cancel()` only flips a flag; the body here never checks `Task.isCancelled`, so the queued main-actor closure still runs: it performs the full `BrowserScreenshotWebViewSnapshotter.captureVisibleViewport` (up to 2s more), draws overlays into the now-discarded bitmap, and fires the detached `completion` (an extra `semaphore.signal()` into a waiter that already gave up). This is orphaned, side-effecting main-actor work and a stray completion racing with the next screenshot request. Consider cancel-and-returning only the synchronous value, or checking `Task.isCancelled` in the body and bailing before the WebKit snapshot/overlay work.</violation>
<violation number="2" location="Sources/TerminalController.swift:13352">
P2: Windows with three stalled browser panes fail screenshot capture after 5 seconds: each viewport snapshot may consume 2 seconds serially, exceeding the enclosing AppKit waiter. Use one shared deadline/budget (or otherwise bound/cancel the aggregate work) so the inner capture finishes before the outer timeout.</violation>
<violation number="3" location="Sources/TerminalController.swift:13426">
P2: Screenshots show externally composited browser/terminal content at full opacity during ancestor fade/hidden-by-alpha states. Track effective ancestor alpha and apply it when drawing the overlay (or skip an effectively transparent subtree).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
4 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Wire/ControlCommandExecutionPolicy.swift">
<violation number="1" location="Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Wire/ControlCommandExecutionPolicy.swift:148">
P2: Concurrent v2 screenshot requests can return a successful but incomplete AppKit PNG rather than the promised explicit busy error. The `.busy` path falls back after AppKit already reported missing external content, so WebKit-backed pixels can disappear; make a busy ScreenCaptureKit admission fail the request instead of selecting that fallback.</violation>
</file>
<file name="Sources/TerminalController.swift">
<violation number="1" location="Sources/TerminalController.swift:13193">
P2: The AppKit capture is gated first and is required to succeed; the ScreenCaptureKit path is only consulted afterwards and only when the AppKit capture returned a (partial) PNG. If the AppKit path times out — for example a WKWebView snapshot hangs or the window can no longer be located in `NSApp.windows` — `captureScreenshot` returns `ERROR: Failed to create PNG data` and never reaches SCK. Since on macOS 14.4+ SCK is the permission-free, compositor-based backend that doesn't depend on WebKit drawing, this ordering means a slow AppKit loss turns into a hard screenshot failure instead of falling back to the more robust backend. Consider attempting the SCK capture when the AppKit delivery times out (returning a `.unavailable`-style signal rather than nil on timeout) so SCK can still serve as the fallback.</violation>
<violation number="2" location="Sources/TerminalController.swift:13463">
P2: Overlapping native views are captured first and then overwritten by terminal/WebKit pixels, so screenshots can show terminal or browser content through UI that should occlude it. Composite these replacements at their hierarchy z-position, or use the compositor whenever an external layer overlaps other captured content.</violation>
</file>
<file name="cmuxUITests/AutomationSocketUITests.swift">
<violation number="1" location="cmuxUITests/AutomationSocketUITests.swift:686">
P3: The new responseTimeout parameter is honored by the primary ControlSocketClient path, but the netcat fallback (controlSocketJSONViaNetcat) that runs when the primary path fails still uses a fixed/short timeout. Callers in this PR pass 10–12s timeouts for browser.navigate/browser.wait/debug.window.screenshot because those commands legitimately take seconds; when the primary socket client happens to fail, the fallback ignores that longer timeout and can time out early, making an otherwise-valid capture appear to fail and flaking the new regression test. Consider routing the same timeout into the netcat fallback (or skipping the fallback for these long-running commands) so both paths honor the requested response timeout.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 13 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…indowlistcreateimage
…indowlistcreateimage # Conflicts: # Packages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandExecutionPolicyTests.swift # Sources/TerminalController.swift
…indowlistcreateimage
1 participant
Summary
Fixes #9065
Testing
Demo Video
N/A — this is a DEBUG socket screenshot path; the focused E2E validates generated PNG pixels.
Review Trigger (for human review)
@codex review
@coderabbitai review
@greptile-apps review
@cubic-dev-ai review
Checklist