Repository navigation
fix(desktop): restore fresh human background screenshots - #22335
Quicksaver wants to merge 1 commit into
Conversation
github-actions
Bot
added
vouch:unvouched
ApprovabilityVerdict: Needs Human Approval Macroscope's review found this PR requires human approval — This restores fresh background screenshots by adding compositor paint probing, a discarded warmup capture, and independent rendering leases around the existing production screenshot path. Because it changes throttling, debugger coordination, timing, and overlap behavior across screenshot, recording, and picture-in-picture flows, the runtime impact merits human review. You can add or adjust custom eligibility rules. Learn more. |
|
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:
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Capture workflow and lifecycle coverage apps/desktop/src/preview/Manager.ts, apps/desktop/src/preview/Manager.test.ts |
captureScreenshot now uses captureStillImage. The helper acquires and releases a still-image rendering lease, runs a warmup capture alongside a bounded paint-readiness probe, and obtains the final image through the retrying capture helper. Tests cover retries, overlapping and interrupted captures, guest replacement, debugger handling, timeouts, and throttling restoration. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant captureScreenshot
participant captureStillImage
participant WebContents
participant ControlDebugger
captureScreenshot->>captureStillImage: request image capture
captureStillImage->>WebContents: start warmup capture
captureStillImage->>ControlDebugger: probe paint readiness
ControlDebugger-->>captureStillImage: return readiness or timeout
WebContents-->>captureStillImage: return warmup result
captureStillImage->>WebContents: perform final retried capture
captureStillImage-->>captureScreenshot: return image
Possibly related issues
- [Bug]: Background viewer capture can hold captureLock and block agent calls past the 10s snapshot deadline (nightly 3457) #23022: Both concern background desktop guests that stall screenshot capture. This change does not cancel a pending native capture.
- [Bug]: off-window Electron preview guests stall snapshots and block later captures #24514: Both concern screenshot capture from off-window Electron guests. This change uses paint-readiness probing and rendering ownership rather than the issue’s proposed repositioning approach.
- [Bug]: preview_snapshot always times out for tabs in delegated child threads and for open:false tabs (macOS 0.1.1) #23936: Both concern background preview snapshot timeouts. This change adds a bounded paint probe and continues capture when freshness cannot be confirmed.
- [Bug]: Windows 0.1.1 preview_snapshot still times out (15s) on public and local pages with visible tab; then 'no automation host available' #23261: Both concern desktop screenshot timeouts. The supplied context does not establish that this change resolves the reported Windows timeout.
- [Bug]: preview_recording_start on a tab that draws no frames never returns and blocks every later action on that tab #23514: Both concern still-image capture and recording ownership. The supplied context notes that native capture promises remain uncancellable.
Possibly related PRs
- pingdotgg/t3code#24057: Both use the bounded retry path for desktop capture work.
- pingdotgg/t3code#25184: Both coordinate rendering ownership for hidden desktop guests during capture.
- pingdotgg/t3code#24056: Both address stale screenshot work; this change tests interruption and guest-replacement handling for warmup results.
Suggested reviewers: juliusmarminge
Merge Risk: 
f75f2
-
No concrete issue is identified that should block merging. Native screenshot validation on the reviewed head remains useful before release.
Security architecture risk: 
96715
-
The change adds local rendering coordination without introducing a new screenshot entrypoint or increasing debugger authority. Independent capture ownership, guest-identity checks and cleanup limit interference with other preview activity. Paint freshness remains best-effort when readiness cannot be confirmed.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — The additional runtime exposure is local to the desktop capture lifecycle: the main window's rendering throttling, participating preview guests and their shared recording or picture-in-picture ownership. The inspected change does not add a server capture path, credential access or a new artifact destination.
Trust Boundaries and Controls
- observed — The readiness probe uses only a control session already owned by the manager; it does not attach a debugger to obtain a screenshot. Existing control-session acquisition refuses DevTools or foreign debugger ownership. Added regressions assert that foreign ownership permits fallback capture without debugger attachment or commands.
Resilience and Maintainability Implications
- observed — Lifecycle regressions cover overlapping requests, recording and picture-in-picture coexistence, interruption, guest replacement and late warmup completion. Together with consumer-specific release and current-guest checks, these provide source-level counterevidence to premature ownership release or publication of replaced-guest pixels; they are not independent native-platform execution proof.
-
Pre-merge checks: 
-
Check name Status Explanation Description check The description includes all required sections. It explains the problem, implementation, scope and approval context, focused verification, platform limits, and evidence links. It also identifies the a… Title check The title clearly and concisely identifies the main change: restoring fresh screenshots for the desktop application. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description includes all required sections. It explains the problem, implementation, scope and approval context, focused verification, platform limits, and evidence links. It also identifies the agent and harness used.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
96715e4 to
cb74756
Compare
|
@coderabbitai full review |
✅ Action performed
|
cb74756 to
7a2b3e9
Compare
github-actions
Bot
added
size:M
afa93bf to
19fb1df
Compare
|
@coderabbitai review |
|
|
@coderabbitai full review |
✅ Action performed
|
Keep each human capture rendering through a bounded paint probe and warmup, then save the final native pixels. Let overlapping probes progress independently while retaining recording and PiP ownership. Co-authored-by: Julius Marminge <julius0216@outlook.com> > 🤖 Generated by GPT 6.1 Sol in Codex via T3 Code.
19fb1df to
f75f20b
Compare
|
@coderabbitai full review |
✅ Action performed
|
Problem
The human Screenshot action can save stale pixels or fail to save an image when Electron stops presenting a background preview, including when the desktop app is hidden. The saved image should contain the current page pixels.
Change
Each capture keeps rendering active while a bounded paint probe runs alongside a discarded warmup capture, then saves the final native readback.
Overlapping screenshots retain independent rendering ownership alongside recording and picture-in-picture. If paint readiness cannot be confirmed, capture uses available pixels without guaranteeing freshness. Native capture promises remain uncancellable.
Scope and approval
This restores the human native screenshot path from #19931. Julius requested a fresh PR with before/after evidence on each affected platform. It applies to Electron desktops on Windows, macOS and Linux. Human approval remains outstanding.
Verification
70 focused tests, changed-file checks and historical desktop proof
vp test run src/preview/Manager.test.ts src/ipc/methods/preview.test.tsfromapps/desktop. Desktop typecheck, touched-file lint, formatting andgit diff --check c677d5fc45e6d585e52bca03244a0870c588a555 HEADpassed atf75f20b7decd59aaa0ce3d7897b89ced222d0716, directly above pinned basec677d5fc45e6d585e52bca03244a0870c588a555.vp run check:changed c677d5fc45e6d585e52bca03244a0870c588a555passed on this HEAD: formatting, lint, both knip passes, affected-package typechecks and 65 Manager cases. Vitest reported a nonfatal shutdown warning that nine Vite servers did not exit within its close timeout; the command exited successfully.8eafe058ca7720219e9466e9c6ed8e59a2276deewith AFTER2e619fae61e66ecd85dd8494a431668920d06af2. It does not verify the current head. The multi-model review attribution below also applies only to that historical AFTER revision.vp run build:desktopon Windows 11 x64, macOS 27.0.1 ARM64 and Debian 12 x64, using Electron 44.4.5. Complete unpackaged apps contained imported conversation history. Tests used the human Screenshot control through production IPC/Manager and independently decoded the saved PNGs.Proof
Historical before/after PNGs and reproduction evidence
Windows/macOS proof package and Linux proof package include saved images, failed requests, pixel checks, hashes, reproduction drivers and limitations. Uploads were downloaded and hash-verified.
Hidden app, requested epoch 4:
The macOS originals are byte-identical and the upload service deduplicated them. Missing before images reflect failed captures. The Linux observation window is not an application timeout guarantee.