Skip to content

Harden Mac VM screenshots against blank VNC frames#315

Merged
arul28 merged 2 commits into
mainfrom
ade/vm-black-screen-audit-20260516
May 17, 2026
Merged

Harden Mac VM screenshots against blank VNC frames#315
arul28 merged 2 commits into
mainfrom
ade/vm-black-screen-audit-20260516

Conversation

@arul28
Copy link
Copy Markdown
Owner

@arul28 arul28 commented May 17, 2026

Summary

  • detect blank/transparent-black VNC frame updates before encoding Mac VM screenshots
  • wake the VM display with pointer movement plus a Shift tap and retry full-frame updates
  • refresh Work lanes lightly when session state loads before lane state, so Mac VM tools keep the active lane/root

Verification

  • live VNC probe: first capture reproduced a black PNG; after the fix, capture waited through 1 blank frame and produced a real 1440x900 login-screen screenshot
  • npm install in apps/desktop, apps/ade-cli, apps/web; lockfiles stayed clean
  • npm run typecheck in apps/desktop, apps/ade-cli, apps/web
  • npm run lint in apps/desktop (0 errors, existing warnings)
  • npx vitest run --shard=1/8 through --shard=8/8 in apps/desktop
  • npm test in apps/ade-cli
  • npm run build in apps/desktop, apps/ade-cli, apps/web
  • node scripts/validate-docs.mjs plus internal docs map/link checks
  • git diff --check

ADE PR creation note: attempted the ADE CLI path first, but this renamed branch could not be imported/attached to the existing worktree lane; using gh fallback per shipLane.

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced VNC screenshot capture to better handle and recover from blank frames on macOS VMs, including automatic retries with wake actions and improved reporting.
    • Improved lane data recovery when lane information is temporarily unavailable.
  • Tests

    • Added comprehensive test coverage for blank frame detection across various scenarios.
    • Added test coverage for lane refresh behavior during work session loading.

Review Change Stack

Greptile Summary

This PR hardens Mac VM VNC screenshot capture against blank/black framebuffer responses, adding retry logic with display-wake actions, and fixes a race where Work sessions load before lane state is available.

  • VNC blank-frame detection and retry: rfbDirectClient.ts gains isLikelyBlankRgbaFrame (sampled RGBA analysis at 0.3% threshold) and a retry loop that wakes the display with a pointer move and Shift tap every 1–3 blank frames, then re-requests a full frame every 250 ms until a real frame or the outer timeout arrives.
  • Lane recovery on session load: useWorkSessions.ts adds a one-shot refreshLanes call when sessions reference a lane but lanes[] is still empty, gated by a ref so failures and subsequent session refreshes don't trigger additional calls.

Confidence Score: 5/5

The changes are well-scoped and the retry/gating logic is correctly structured; no paths introduce data loss or incorrect state.

Both the VNC retry loop and the lane-recovery effect are correctly guarded: settled prevents double-resolution, cleanup() always cancels the retry timer before the promise settles, and the lane-recovery ref gate survives errors without enabling rapid re-fires. Tests exercise the key edge cases (cursor-only patch, failure gating). The one timing nuance (retryTimer nulled before requestFullFrame) is extremely unlikely to manifest in production and has no safety impact.

No files require special attention; rfbDirectClient.ts carries the most logic density and warrants a second read, but all paths are covered by the new tests.

Important Files Changed

Filename Overview
apps/desktop/src/main/services/macosVm/rfbDirectClient.ts Core VNC capture rewritten with persistent on listeners, blank-frame detection loop, display-wake side effects, and proper cleanup; logic is sound with one minor timing nuance around retryTimer nulling.
apps/desktop/src/main/services/macosVm/rfbDirectClient.test.ts New unit tests cover all-black, transparent, cursor-patch, and dark-real-content frames; threshold and sampling logic are well validated.
apps/desktop/src/main/services/macosVm/macosVmService.ts Minor log-message enhancement to include retry count in the success message; no logic changes to the capture path itself.
apps/desktop/src/renderer/components/terminals/useWorkSessions.ts Adds a gated one-shot lane-recovery effect; ref guard correctly prevents duplicate calls; failure path intentionally keeps the gate closed to avoid retry loops.
apps/desktop/src/renderer/components/terminals/useWorkSessions.test.ts Two new tests confirm the one-shot recovery fires on empty lanes and that a failed call does not repeat on subsequent session refreshes.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant captureVncScreenshot
    participant VncClient
    participant retryTimer

    Caller->>captureVncScreenshot: captureVncScreenshot(connection, 15s)
    captureVncScreenshot->>VncClient: connect + authenticate
    captureVncScreenshot->>VncClient: requestFullFrame()
    captureVncScreenshot->>captureVncScreenshot: register on(firstFrameUpdate, frameUpdated)

    VncClient-->>captureVncScreenshot: frameUpdated (blank frame)
    captureVncScreenshot->>captureVncScreenshot: isLikelyBlankRgbaFrame → true
    captureVncScreenshot->>captureVncScreenshot: blankFrameAttempts++
    captureVncScreenshot->>VncClient: wakeVncDisplay (pointer + Shift)
    captureVncScreenshot->>retryTimer: setTimeout(250ms)

    retryTimer-->>captureVncScreenshot: fires
    captureVncScreenshot->>VncClient: requestFullFrame()

    VncClient-->>captureVncScreenshot: frameUpdated (real frame)
    captureVncScreenshot->>captureVncScreenshot: isLikelyBlankRgbaFrame → false
    captureVncScreenshot->>captureVncScreenshot: makeOpaqueRgba + encodeRgbaPng
    captureVncScreenshot->>captureVncScreenshot: cleanup() — remove listeners, clear timers
    captureVncScreenshot-->>Caller: "DirectVncScreenshot { pngData, blankFrameAttempts }"

    note over captureVncScreenshot: If outer timeout fires first → finishWithError with retry count in message
Loading

Fix All in Claude Code

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
apps/desktop/src/main/services/macosVm/rfbDirectClient.ts:253-260
The retry timer clears `retryTimer` to `null` before calling `requestFullFrame()`. In JavaScript's single-threaded event loop this is safe against true races, but a spontaneous `frameUpdated` event that arrives between when the timer callback starts and when `requestFullFrame()` completes would find `retryTimer === null` and schedule an extra timer — resulting in two outstanding full-frame requests instead of one. Setting `retryTimer = null` after `requestFullFrame()` eliminates that window entirely.

```suggestion
        retryTimer = setTimeout(() => {
          try {
            requestFullFrame();
          } catch (error) {
            onError(error);
          } finally {
            retryTimer = null;
          }
        }, BLANK_FRAME_RETRY_MS);
```

Reviews (2): Last reviewed commit: "ship: iteration 1 - address Greptile fee..." | Re-trigger Greptile

@vercel
Copy link
Copy Markdown

vercel Bot commented May 17, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
ade Ignored Ignored Preview May 17, 2026 7:17am

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 17, 2026

📝 Walkthrough

Walkthrough

VNC screenshot capture now detects blank/black frames, counts retry attempts, sends periodic wake actions, and includes attempt counts in operation messages. Work sessions hook detects when lanes are missing but referenced by sessions, then triggers a lightweight lane refresh once per project root with retry on failure.

Changes

VNC Blank Frame Handling

Layer / File(s) Summary
Type contracts and blank-frame detection
apps/desktop/src/main/services/macosVm/rfbDirectClient.ts
DirectVncScreenshot gains optional blankFrameAttempts field. Frame sampling constants and thresholds are introduced. isLikelyBlankRgbaFrame (exported) samples RGBA pixels to detect likely-blank frames, and makeOpaqueRgba forces alpha to opaque for PNG encoding.
Screenshot capture with blank-frame retry
apps/desktop/src/main/services/macosVm/rfbDirectClient.ts
captureVncScreenshot now tracks blankFrameAttempts, samples each frame event, conditionally accepts or retries frames, sends periodic wake actions, schedules delayed full-frame retries on blank detection, and augments timeout errors with blank-frame context. VNC event wiring switches to frameUpdated and performs initial requestFullFrame after listeners are registered.
Blank-frame detection test suite
apps/desktop/src/main/services/macosVm/rfbDirectClient.test.ts
Vitest suite verifies isLikelyBlankRgbaFrame correctly identifies fully black, fully transparent, cursor-marked, and noisy frames as blank, and rejects frames with sufficient non-black content.
Operation message with blank-frame context
apps/desktop/src/main/services/macosVm/macosVmService.ts
Screenshot completion message now conditionally appends blank-frame attempt count when blankFrameAttempts is non-zero.

Work Sessions Lane Recovery

Layer / File(s) Summary
Hook setup and lane recovery guard
apps/desktop/src/renderer/components/terminals/useWorkSessions.ts
useWorkSessions reads refreshLanes action from useAppStore, introduces laneRecoveryRefreshProjectRef guard to track recovery state, and resets the guard when projectRoot changes alongside other query/filter refs.
Lane recovery effect
apps/desktop/src/renderer/components/terminals/useWorkSessions.ts
New useEffect runs when lanes are empty but sessions include lane IDs, calls refreshLanes once per project root (guarded), and clears the guard on failure to enable retry.
Lane recovery test coverage
apps/desktop/src/renderer/components/terminals/useWorkSessions.test.ts
Test infrastructure adds refreshLanesSpy and mocks refreshLanes in fakeAppStoreState. New test verifies hook triggers lightweight lane refresh with correct include flags when sessions load before lane recovery.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

desktop

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Harden Mac VM screenshots against blank VNC frames' directly and clearly summarizes the main objective of the changeset—improving robustness of macOS VM screenshot capture by handling blank frame detection and retry logic.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ade/vm-black-screen-audit-20260516

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@capy-ai
Copy link
Copy Markdown

capy-ai Bot commented May 17, 2026

Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews.

@arul28
Copy link
Copy Markdown
Owner Author

arul28 commented May 17, 2026

@copilot review but do not make fixes

Copy link
Copy Markdown
Contributor

Copilot AI commented May 17, 2026

@copilot review but do not make fixes

Reviewed commit 70a2fcf as requested (review-only, no fixes made). I don’t see any blocking issues in these changes; current CI run for this SHA is still in progress and has no failed jobs so far.

Copilot finished work on behalf of arul28 May 17, 2026 06:52
Comment thread apps/desktop/src/main/services/macosVm/rfbDirectClient.ts Outdated
Comment thread apps/desktop/src/main/services/macosVm/rfbDirectClient.ts
Comment thread apps/desktop/src/renderer/components/terminals/useWorkSessions.ts Outdated
@arul28
Copy link
Copy Markdown
Owner Author

arul28 commented May 17, 2026

@copilot review but do not make fixes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants