fix(screenshot): surface real render errors, refcount CORS rule, bound webp re-encode#130
Merged
ronaldeddings merged 3 commits intoJul 5, 2026
Conversation
…d webp re-encode Three fixes to the DOM-render screenshot path. - dom-screenshot: the render can reject with a raw DOM Event (an <img>/SVG onerror when the embedded SVG fails to decode on a large page). A DOM Event has no .message, so (err as Error).message rendered the literal 'undefined'. describeRenderError coerces every error shape into a real message and sanitizes after coercion, so 'undefined'/'null'/'[object Object]' can't leak. - screenshot-cors: the CORS DNR rule id is keyed on tabId, so two concurrent screenshots of the same tab share one rule. The first to finish tore it down under the second, dropping ACAO:* mid-fetch and failing that render. Add a per-tab refcount: install on the first acquire, remove only when the last releases. A failed install rolls the count back so it leaves no phantom ref. - screenshot: bound the post-render webp re-encode with the same 30s timeout the render already uses, so a large image that re-encodes slowly can't hang past the CLI ceiling. Tests: describeRenderError coercion (Error/Event/string/primitive), CORS refcount (single/concurrent/independent-tabs/re-acquire/failed-install).
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds per-tab CORS rule reference counting, bounds WebP re-encoding with a timeout, and updates DOM screenshot rasterization, output validation, and error formatting. Tests cover the CORS lifecycle and render-error/output-guard behavior. ChangesScreenshot reliability improvements
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/screenshot-cors-refcount.test.ts (1)
91-103: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding a test for a concurrent acquirer during a failing install.
This suite covers rollback only for a single, sequential acquirer. Given the race flagged in
screenshot-cors.ts(a secondinstallScreenshotCorsRulecall arriving while the first is still installing, then the first fails), a test simulating that interleaving would catch the reported bug and guard against regression once fixed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/screenshot-cors-refcount.test.ts` around lines 91 - 103, The rollback coverage in installScreenshotCorsRule only checks a single failed acquire, but it misses the concurrent interleaving where a second caller arrives while the first install is still in progress. Add a test in screenshot-cors-refcount.test.ts that exercises two overlapping installScreenshotCorsRule calls, forces the first install path to fail, and verifies the refcount and session rule state are fully rolled back for both callers so no phantom reference remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extension/src/background/capabilities/screenshot-cors.ts`:
- Around line 65-94: The shared refcount in
installScreenshotCorsRule/uninstallScreenshotCorsRule can be dropped by a failed
first updateSessionRules() while another same-tab acquire is still active.
Change the rollback in installScreenshotCorsRule so it only undoes the increment
for the current acquire (preserving any concurrent acquires in
corsRuleRefcount), and make uninstallScreenshotCorsRule continue to remove the
rule only when the last reference is released. Add or update a test around
overlapping installs of installScreenshotCorsRule to verify a failed install
does not clear another caller’s refcount entry.
---
Nitpick comments:
In `@test/screenshot-cors-refcount.test.ts`:
- Around line 91-103: The rollback coverage in installScreenshotCorsRule only
checks a single failed acquire, but it misses the concurrent interleaving where
a second caller arrives while the first install is still in progress. Add a test
in screenshot-cors-refcount.test.ts that exercises two overlapping
installScreenshotCorsRule calls, forces the first install path to fail, and
verifies the refcount and session rule state are fully rolled back for both
callers so no phantom reference remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a224488-5d38-47dc-80a1-57d85a331165
📒 Files selected for processing (5)
extension/src/background/capabilities/screenshot-cors.tsextension/src/background/capabilities/screenshot.tsextension/src/content/dom-screenshot.tstest/dom-screenshot-error-describe.test.tstest/screenshot-cors-refcount.test.ts
…stall Addresses the concurrency edge flagged in review. When a same-tab acquire arrives while the first install's updateSessionRules() is still in flight and that first install then fails, the rollback used delete(tabId), which wiped the whole refcount entry — stranding the concurrent acquire with no live rule and no reference, so a later uninstall's accounting could remove the rule under an active operation. Roll back only this acquire's increment: decrement by one, delete only if that leaves zero. Add a test that interleaves two same-tab installs with a failing first install and asserts the second acquire's reference survives (fails against the old delete-the-entry rollback).
…rasterize time Live testing PR Hacker-Valley-Media#130 on macOS surfaced the bug it missed: a page past the browser's canvas limits (e.g. 300,000px tall) returned success with size: 0 — toDataURL() silently yields "data:," when the backing store fails to allocate. checkRasterizeOutput now turns that into an actionable error naming --region / --target-max-long-edge / --scale. To make --region a real remedy on such pages, region mode now crops at rasterize time: the canvas is allocated at the crop size and the decoded SVG is drawn offset into it. Decoding a huge SVG image succeeds where the full-page canvas fails, so region captures work far past the limits — and the post-render cropDataUrl re-encode step is gone entirely. PRD-123. Live-verified: huge page → actionable error; --region on the same page → valid 2560x4000 capture; full/region/element/webp parity on normal pages; concurrent same-tab race harness still clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ronaldeddings
approved these changes
Jul 5, 2026
ronaldeddings
left a comment
ronaldeddings
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed statically + dynamically (PRD-123): 14/14 PR tests pass, live race harness clean, maintainer follow-up adds the oversized-render guard + raster-time region crop. Merging with contributor commits intact.
This was referenced Jul 5, 2026
Collaborator
|
Thanks @atabisz! Last week I ripped out |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three fixes to the DOM-render screenshot path, found while running screenshots against large real-world pages on Windows.
What's here
1. Surface the real render error instead of "undefined"
html-to-imagecan reject with a raw DOMerrorEvent — an<img>/SVGonerrorwhen the serialized, resource-embedded SVG fails to decode on a large page. A DOM Event has no.message, so(err as Error).messagerendered the literal stringdom render failed: undefined, which tells you nothing.describeRenderErrorcoerces every error shape (Error, DOM Event, string, opaque object) into a real message, and sanitizes after coercion so a thrown literal"undefined"/"null"/"[object Object]"can't leak either. An<img>failure now readsimage load failed (error on <img>) — the rendered SVG could not be decoded, likely too large or a resource blocked.2. Refcount the CORS DNR rule
The screenshot CORS rule id is keyed on
tabId, so two concurrent screenshots of the same tab share one rule. Whichever finishes first tore the rule down while the other was still fetching subresources — that render lostAccess-Control-Allow-Origin: *mid-flight and tainted or failed.installScreenshotCorsRule/uninstallScreenshotCorsRulenow keep a per-tab refcount: install adds the rule on the first acquire, uninstall removes it only when the last concurrent operation releases. A failed install rolls the count back so it leaves no phantom reference (callers place install outside their try/finally, so a throw there means uninstall never runs).3. Bound the webp re-encode
The render is already guarded by
DOM_RENDER_TIMEOUT_MS, but the post-render webp re-encode wasn't. A large image that re-encodes slowly could hang past the CLI's timeout with no diagnostic. The re-encode now uses the same 30s guard and returns an actionable error.Tests
describeRenderError— Error / DOM Event /<img>Event / string / bare primitive coercion, and that no shape leaksundefined.Summary by CodeRabbit