Skip to content

fix(web): keep screenshots when attaching preview annotations - #58

Merged
kalvenschraut merged 1 commit into
rtvisionfrom
fix/preview-annotation-screenshot-csp
Sep 15, 2026
Merged

kalvenschraut merged 1 commit into
rtvisionfrom
fix/preview-annotation-screenshot-csp

Conversation

@kalvenschraut

@kalvenschraut kalvenschraut commented Sep 15, 2026

Copy link
Copy Markdown
Member

Problem

Picking an element in the browser preview always ends with "Could not capture the picked element. The annotation was kept without the screenshot." Electron captures the crop fine and hands it to the renderer as a data URL. The renderer then converts it with a fetch of that data URL, and the desktop Content Security Policy only allows http, https, ws, and wss connections, so the fetch is refused and the crop is dropped every time. The preview webview always renders in the local desktop app, so this hits SSH-hosted environments the same as local ones.

Fix

Port of upstream pingdotgg#10374 (tracked upstream in pingdotgg#10366 and pingdotgg#11867; the upstream PR is currently conflicting and unmerged). Decode the data URL locally with the existing dataUrlToFile helper instead of fetching it, and drop the conversion timeout that only existed to bound the fetch. Invalid data URLs still resolve to a dropped crop with the same toast.

Focused tests (previewAnnotation, PreviewView) pass and the web typecheck passes. Not verified in a running desktop build.

Written by Claude Fable 5.1 through Claude Code in T3 Code.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved preview annotation screenshot capture by processing image data locally.
    • Added clearer handling for missing, invalid, unsupported, or blocked screenshot data.
    • Valid image data is now correctly converted into downloadable PNG captures.

The element picker captured its crop in Electron, then dropped it in the
renderer: the conversion fetched the data URL, and the desktop Content
Security Policy only allows http, https, ws, and wss connections, so every
attach ended with "The annotation was kept without the screenshot". Decode
the data URL locally with the existing dataUrlToFile helper and remove the
now-redundant conversion timeout. Failed decodes still keep the annotation
without promising an attached image.

Ported from upstream pingdotgg#10374.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@kalvenschraut

Copy link
Copy Markdown
Member Author

@coderabbitai review

Please review this four-file port of upstream pingdotgg#10374 at head 8d2412e. The renderer converted the annotation crop with fetch(dataUrl), which the desktop CSP connect-src refuses, so every picked element lost its screenshot. The change decodes the data URL locally with the existing dataUrlToFile helper and updates the tests to match.

@github-actions github-actions Bot added the vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. label Sep 15, 2026
@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown

@kalvenschraut I will review the four-file port in #58. I will check the local data URL conversion, error handling, and test updates.

✅ Action performed

Review finished.

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.

@github-actions

Copy link
Copy Markdown

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 13.6 KiB 13.6 KiB +11 B (+0.1%) 15.1 KiB
Codex Thread snapshot wire 7.0 KiB 7.0 KiB −1 B (−0.0%) 7.3 KiB
Codex Live turn WebSocket wire 6.5 KiB 6.6 KiB +12 B (+0.2%) 7.8 KiB
Codex Live turn WebSocket decoded 57.0 KiB 57.1 KiB +44 B (+0.1%) 66.4 KiB
Codex Live turn messages 9 10 +1 (+11.1%) 21
Claude Total thread wire 13.6 KiB 13.6 KiB +52 B (+0.4%) 15.1 KiB
Claude Thread snapshot wire 7.1 KiB 7.0 KiB −9 B (−0.1%) 7.3 KiB
Claude Live turn WebSocket wire 6.5 KiB 6.6 KiB +61 B (+0.9%) 7.8 KiB
Claude Live turn WebSocket decoded 57.8 KiB 57.9 KiB +88 B (+0.1%) 66.4 KiB
Claude Live turn messages 8 10 +2 (+25.0%) 21

Baseline: 52839dc · PR result: 8d2412e · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 113.9 KiB
  • Claude decoded thread snapshot: 114.6 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 4bc00d97-47c2-448a-a410-42b401783e77

📥 Commits

Reviewing files that changed from the base of the PR and between 52839dc and 8d2412e.

📒 Files selected for processing (4)
  • apps/web/src/components/preview/PreviewView.test.tsx
  • apps/web/src/components/preview/PreviewView.tsx
  • apps/web/src/lib/previewAnnotation.test.ts
  • apps/web/src/lib/previewAnnotation.ts

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


📝 Walkthrough

Walkthrough

The screenshot capture helper now decodes embedded image data synchronously. It returns explicit statuses for missing, invalid, and valid screenshots. Preview handling and tests now use the synchronous API.

Changes

Preview screenshot capture

Layer / File(s) Summary
Synchronous screenshot decoding
apps/web/src/lib/previewAnnotation.ts
Screenshot capture validates and decodes local image data URLs without fetch or timeout handling. It returns none, failed, or captured results.
Preview integration and validation
apps/web/src/components/preview/PreviewView.tsx, apps/web/src/components/preview/PreviewView.test.tsx, apps/web/src/lib/previewAnnotation.test.ts
Preview handling and mocks use synchronous capture results. Tests cover valid PNG decoding, missing screenshots, CSP-blocked data URLs, malformed data, and unsupported URLs.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 8d241

The preview flow accepts the producer’s PNG data URLs and all current consumers use the synchronous capture result correctly.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary change: preserving screenshots when attaching preview annotations.
Description check ✅ Passed The description clearly explains the problem, root cause, fix, test coverage, and verification limits. It uses Problem and Fix sections instead of the template headings and omits the checklist, but it…
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 4 files.
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.
✨ 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 fix/preview-annotation-screenshot-csp

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

@kalvenschraut
kalvenschraut merged commit eeaa19b into rtvision Sep 15, 2026
17 checks passed
@kalvenschraut
kalvenschraut deleted the fix/preview-annotation-screenshot-csp branch September 15, 2026 16:22
kalvenschraut added a commit that referenced this pull request Sep 15, 2026
Bump release package versions to 0.0.54 so the RTVision release workflow
builds installers, CLI archives, and the @rtvision/t3 npm tarball for the
upstream sync in #59 and the fixes in #57 and #58.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant