Skip to content

fix(desktop): restore fresh human background screenshots - #22335

Open
Quicksaver wants to merge 1 commit into
pingdotgg:mainfrom
Quicksaver:fix/restore-preview-background-screenshots
Open

Quicksaver wants to merge 1 commit into
pingdotgg:mainfrom
Quicksaver:fix/restore-preview-background-screenshots

Conversation

@Quicksaver

@Quicksaver Quicksaver commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Summary

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. Each capture now keeps rendering active through a paint probe and discarded warmup, then saves the final native readback.

This restores the human native screenshot path from #19931 on Windows, macOS and Linux Electron desktops. Julius requested a fresh PR with before/after evidence on every affected platform. Human approval remains outstanding.

What changed

  • Overlapping screenshots retain independent rendering ownership and paint probes alongside recording and picture-in-picture.
  • When paint readiness or warmup fails, capture falls back to available pixels with freshness unconfirmed. A frozen renderer can delay page-side probe expiry, and native capture promises remain uncancellable.

Validation

Exact-head source checks, upstream control and remaining acceptance
  • Head 068596e60020aa5b71a34f3e2ea196b8a9643ac1 is one customization commit directly above fixed boundary ae142932d07c0596cf0497dd28cfee07ae7c0722. Both customized source files match the prior published revision byte-for-byte. Author, author date, coauthorship, complete commit message and attribution are preserved.
  • The retained exact-head frozen-lockfile install passed under managed Node 26.12.1 and pnpm 12.14.0. Installation reported slow tarball downloads, an available pnpm update and an e2e skill awaiting approval; no dependency override or tracked dependency change was applied.
  • Focused Manager, GuestInput, DesktopWindow, DesktopBrowserHost and BrowserSession tests passed 165 cases and failed one Windows download-path assertion. Preview IPC ran separately at its actual ipc/methods/preview.test.ts path and passed all five cases, bringing the six executed files to 170 passing tests and one failure. The initial filter also named a nonexistent preview/ipc.test.ts path. All Manager cases passed. The main-owned helper produced a fresh same-host single-file control at the exact boundary with 37 passing cases and the identical failure. Its overall result is failed and typechecking was not run. These failures remain recorded; no fully green aggregate is claimed.
  • The retained exact-head vp run check:changed ae142932d07c0596cf0497dd28cfee07ae7c0722 --concurrency 1 passed with exit zero under managed Node 26.12.1, pnpm 12.14.0 and at most two Vitest workers. Formatting, lint, both uncached Knip passes, uncached desktop/scripts typechecks at package concurrency one, and all 76 Manager cases passed. Typechecking emitted advisory Effect suggestions. Scripts typechecking was not cached because it modified its inputs; those inputs were not independently identified. The overall gate was not cached because files changed during the run. Final tracked source stayed clean. This passing gate does not erase the focused/control failure.
  • Evidence reuse was checked against the exact local head, source hashes and retained logs. No install, test, typecheck or Knip run was repeated for this metadata refresh. Committed-range and working-tree git diff --check passed, and both customization files use LF in the index and working tree. No repository-wide check ran. A Vite shutdown warning belongs to the previous-head publication gate and is historical evidence, not a result for this head.
  • Current-head native freshness, shown/hidden/minimized/display-off behavior, overlapping captures, real recording/PiP coexistence, interruption during warmup/final capture, guest replacement, app close, focus retention, throttling restoration and annotation-dictation coexistence remain unverified on Windows, macOS and Linux with Electron 44.10.1. First-document loading, desktop reload with deferred tab attachment, panel collapse/focus restoration, page dialogs/popovers, fullscreen guests and host fullscreen transitions also need current-head acceptance. No development server, native build or integrated UI runtime ran. Standalone web and mobile do not execute this native human capture path. Local, remote/relay and tunnel routing is unchanged; native captures run on the owning Electron desktop. Agent CDP screenshots cannot establish acceptance of the human Screenshot control. Fallback captures do not guarantee freshness. A frozen renderer can delay page-side probe expiry, and native capture promises remain uncancellable.
  • Earlier CI, CodeRabbit and Magi results cover only their recorded revisions. CodeRabbit remains paused with historical source/covered marker 8c45433f7a5b3a5320593a95954b285e4cae445d. The submitted COMMENTED review covers 685999b7969a8e875161148f74b3e05de4a3cde8, without merge approval. Current-head source review, human runtime approval and native acceptance remain outstanding. CodeRabbit's scope/approval warning and Macroscope's human approval gate remain open. Passing description checks and historical security assessments supply neither approval nor an exemption. The multi-model review attribution below applies only to the historical AFTER revision.

Proof

Historical before/after PNGs and reproduction evidence
  • Native proof compares BEFORE 8eafe058ca7720219e9466e9c6ed8e59a2276dee with AFTER 2e619fae61e66ecd85dd8494a431668920d06af2.
  • Both historical revisions passed vp run build:desktop on Windows 11 x64, macOS 27.0.1 ARM64 and Debian 12 x64 with 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.
  • Windows hidden and overlapping-hidden captures failed BEFORE and saved fresh PNGs AFTER, including a hidden target with the display off. Linux saved five of eight requested PNGs BEFORE and eight distinct fresh PNGs AFTER. Both macOS revisions saved fresh PNGs in the measured cases, including locked with the display off. Normal, inactive, minimized, PiP and close/reopen cases saved fresh PNGs on all three platforms.
  • Limits: Linux used Docker Desktop WSL2, Xvfb/Openbox/picom and software rendering. Physical display/lock, hardware GPU, Wayland and installed AppImage behavior are unverified. No natural macOS failure was observed; its test-keychain flag leaves OS-backed credential sealing unverified. Recording quality, pending macOS cancellation, frozen-probe fallback, replacement retaining the exact runtime tab identity and OS-wide foreground-process preservation were not demonstrated. The human control stops recording instead of acquiring a concurrent still.

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:

Platform Before After
Windows No PNG returned; failed request retained in the package. Windows Screenshot saves fresh epoch 4
Linux No PNG within the 12-second observation window. Linux Screenshot saves fresh epoch 4
macOS macOS before saves fresh epoch 4 macOS after saves fresh 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.

🤖 Generated by GPT 6.1 Sol in Codex via T3 Code.
🤖 Reviewed until clean by GPT 6 Astra, GPT 6.1 Sol, Claude Fable 5.1, Claude Opus 5.5, CodeRabbit. Findings that reach consensus across the majority were addressed by the generation model.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Oct 20, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: 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.

@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review.

Walkthrough

Still-image screenshot capture now uses a separate rendering lease. It runs a warmup capture while checking paint readiness with a bounded probe, then performs a final retried capture. Tests cover overlapping and interrupted captures, guest replacement, debugger handling, timeouts, and throttling restoration.

Changes: Still-image capture

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.

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
Loading

Possibly related issues

Possibly related PRs

  • pingdotgg/t3code#24057: Both define failure handling for stalled desktop captures; this change uses the bounded capture path for warmup and final readback.
  • 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 during warmup.

Suggested reviewers: juliusmarminge

Merge Risk: Minimal · up to 8c454

No actionable issue is established from the supplied evidence. Merge after the normal checks and any desired current-head desktop validation.

Security architecture risk: Low · up to 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: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Description check Warning The description covers the problem, change, and verification in detail, and it discloses that native acceptance remains unverified on the current head. However, the required scope approval is outstand… Add a link to a triaged issue or discussion that includes explicit maintainer approval of the direction and scope, including the approval comment. If this qualifies as a small, focused fix of an obvious bug, explain why it qualifies for the…
Passed Passed checks: 3 passed

Check name Status Explanation
Title check Passed The title clearly and concisely identifies the desktop fix for restoring fresh human background screenshots.
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.
Full details: Description check

Explanation

The description covers the problem, change, and verification in detail, and it discloses that native acceptance remains unverified on the current head. However, the required scope approval is outstanding; the linked discussion does not include explicit maintainer approval.

Resolution

Add a link to a triaged issue or discussion that includes explicit maintainer approval of the direction and scope, including the approval comment. If this qualifies as a small, focused fix of an obvious bug, explain why it qualifies for the exception.

Finishing Touches

🧪 Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 7b87466 and 8c45433.

Files selected for processing: 2

  • apps/desktop/src/preview/Manager.test.ts
  • apps/desktop/src/preview/Manager.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

@Quicksaver
Quicksaver force-pushed the fix/restore-preview-background-screenshots branch 2 times, most recently from 96715e4 to cb74756 Compare October 24, 2026 00:02
@Quicksaver

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@Quicksaver
Quicksaver force-pushed the fix/restore-preview-background-screenshots branch from cb74756 to 7a2b3e9 Compare October 25, 2026 19:05
@github-actions github-actions Bot added size:M 30-99 changed lines (additions + deletions). and removed size:L 100-499 changed lines (additions + deletions). labels Oct 25, 2026
@Quicksaver
Quicksaver force-pushed the fix/restore-preview-background-screenshots branch 2 times, most recently from afa93bf to 19fb1df Compare October 27, 2026 14:41
@Quicksaver

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

No files to review.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@Quicksaver

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@Quicksaver
Quicksaver force-pushed the fix/restore-preview-background-screenshots branch from 19fb1df to f75f20b Compare October 28, 2026 15:49
@Quicksaver

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@Quicksaver
Quicksaver force-pushed the fix/restore-preview-background-screenshots branch from f75f20b to 8c45433 Compare October 29, 2026 02:02

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@Quicksaver
Quicksaver force-pushed the fix/restore-preview-background-screenshots branch 4 times, most recently from 06da759 to dd42c67 Compare November 4, 2026 11:12

Copy link
Copy Markdown
Contributor Author

In response to the description warning, the PR now uses the template's Problem, Change, Scope and approval, and Verification sections. Scope and approval retains Julius's re-landing directive. Existing proof and its limitations are unchanged.

Human approval, current-head source review and native acceptance on Electron 44.10.1 remain outstanding. The native proof covers historical revisions; this correction does not claim a passing description check.

🤖 Generated by GPT 6.1 Sol in Codex via T3 Code.

@Quicksaver
Quicksaver force-pushed the fix/restore-preview-background-screenshots branch 2 times, most recently from e420339 to 7f2083e Compare November 6, 2026 00:40
@Quicksaver
Quicksaver force-pushed the fix/restore-preview-background-screenshots branch 2 times, most recently from 2725136 to 685999b Compare November 8, 2026 00:40
Wladefant

This comment was marked as spam.

@Quicksaver
Quicksaver force-pushed the fix/restore-preview-background-screenshots branch 2 times, most recently from deecac8 to f5e06b7 Compare November 10, 2026 01:23
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.
@Quicksaver
Quicksaver force-pushed the fix/restore-preview-background-screenshots branch from f5e06b7 to 068596e Compare November 11, 2026 01:22

This branch has not been deployed

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

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants