Sign up
View stargazersSign up

fix(desktop): avoid decoding snapshots to read image dimensions#23129

closed2
main
fix/issue-19220
opened17d agobyJJulius Marminge
2 files changed
View on GitHubTwo-way sync enabledPR last imported 9d ago

Problem

With SnapShot accessibility enabled, the desktop main process decodes an already produced PNG again just to read its dimensions. The reporter’s synthetic PNG benchmark reproduces the synchronous sizing work in Electron 44.4.5 on Linux x64.

Change

Read dimensions with the existing shared image-header reader. Preserve the rounded, minimum-one logical-bounds fallback, capture bytes, and accessibility coordinate processing. Add a service regression test covering pixel dimensions distinct from logical bounds, saved metadata, unchanged PNG bytes, and absence of the redundant decode.

Scope and approval

Fixes #19220

Triaged bug via the automated issue flow: #19220. Focused desktop performance fix; no settings, permission behavior, contracts, or renderer changes. Dispatcher owns merging; native desktop integration remains unverified.

Verification

  • Red regression: 49 passed, 1 failed at the assertion expecting zero sizing decodes (observed one). Red transcript.
  • vp test run apps/desktop/src/snapShot/DesktopSnapShot.test.ts: 50/50 passed after the fix. Green transcript.
  • Final scoped run of desktop snapshot and shared dimension-reader tests: 58/58 passed after confirming latest main is already an ancestor. Targeted lint/format checks and desktop typecheck passed. Both independent code reviewers returned MERGE; the independent proof audit returned SUFFICIENT after rerunning every runtime assertion.

Evidence compares exact helper bodies at 70e5e384da and b8955af5cf, in Electron 44.4.5 / Node 24.21.0 / Linux x64 under Xvfb. Each size uses identical complete synthetic PNG bytes for both bodies. Three warmup rounds, 15 measured rounds; medians below. This is an isolated CPU-operation benchmark, not full app latency; VM load and batching affect timings.

FixtureBeforeAfterFunctional result
1440×900 PNG, logical 720×4508.874 ms0.000073 msNative/original/fixed/header dimensions agree; zero extra sizing decodes
2560×1600 PNG, logical 1280×80027.827 ms0.000078 msNative/original/fixed/header dimensions agree; zero extra sizing decodes
Unreadable dimensions, logical 700.6×519.4701×519701×519Fallback preserved
Unreadable dimensions, logical 0×−21×11×1Minimum-one clamp preserved
Accessibility disabled / identity absentBypassedBypassedUnchanged caller gate; source audit

Proof matrix · Evidence index and reproduction commands · Runtime transcript · Runtime script.

No renderer changes or visible comparison; screenshots and pixel diffs do not establish this CPU operation. The real desktop app launched under Xvfb after installing the missing libsecret build prerequisite. Real shortcut/accessibility captures remain unverified on Linux, macOS, and Windows, including compositor and scaling integration; route those checks to Julius. Web, mobile, providers, and remote transports are unaffected.

Implemented with Claude Opus 5.5 through the Claude harness; triaged and verified with GPT-6.1-Sol through the Codex harness in T3 Code.

Checks

23 successful · 13 skipped
All checks passed
b8955af

Added 2 commits

  • test(desktop): reproduce #19220 snapshot decodes PNG only to size it3177e33
  • fix(desktop): snapshots read capture size from the PNG headerb8955af
MmacroscopeappBot17d ago
Synced from GitHub

Approvability

Verdict: Approved at b8955af

Macroscope's review found this PR approvable — This focused snapshot fix reads dimensions from image headers instead of decoding the image, while retaining the existing fallback and leaving captured bytes and metadata flow unchanged. The accompanying test verifies the no-decode behavior, with no product-default or static-analysis configuration changes.

You can add or adjust custom eligibility rules. Learn more.

CcoderabbitaiBot17d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Team
  • Run ID: 6da55db9-ed07-41af-b9ff-db4e2b58c8bd
📥 Commits

Reviewing files that changed from the base of the PR and between a12d6ed9428d8e64d76ca86d178abbf564b57cee and b8955af5cf9e6bb785baab9ad2161dd642b56063.

📒 Files selected for processing: 2
  • apps/desktop/src/snapShot/DesktopSnapShot.test.ts
  • apps/desktop/src/snapShot/DesktopSnapShot.ts

Limit details: You’ve used all 10 included reviews currently available.

📝 Walkthrough

Snapshot sizing now reads dimensions from PNG data instead of decoding the buffer with Electron’s nativeImage. When PNG dimensions are unavailable, it uses the existing rounded bounds fallback. A Linux capture test checks accessibility metadata and PNG persistence.

Changes: Snapshot dimension handling

Layer / File(s)Summary
PNG dimension parsing and capture validation
apps/desktop/src/snapShot/DesktopSnapShot.ts, apps/desktop/src/snapShot/DesktopSnapShot.test.ts		snapShotImageSize reads dimensions from PNG data and retains the rounded bounds fallback. The Linux capture test verifies accessibility dimensions, captured-image metadata, unchanged PNG bytes, and that native image decoding is not called.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: bil0000

Merge Risk: ⚪ Minimal · up to b8955

Snapshot sizing retains its fallback for unreadable image dimensions. No issue identified here needs resolution before merge.

Pre-merge checks: Passed 4
Check nameStatusExplanation
Description checkPassedThe description includes all required sections. It explains the problem, change, scope and approval, focused verification results, benchmark context, and known verification limits.
Title checkPassedThe title is concise, specific, and accurately describes the main change: avoiding redundant decoding of desktop snapshot images to read dimensions.
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Usage-based review receipt

Note

This review was completed with usage-based billing: files reviewed beyond your plan's included limits are billed at $0.25/file. View usage-based billing.

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

JJulius Marminge9d ago

Closing: current main already covers this as of 3d00499936. If you still see it on a build that includes that commit, reply and we'll reopen.