screenshot#523
Open
aidenybai wants to merge 122 commits into
Open
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
38 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/screenshot/scripts/debug-capture.local.mjs">
<violation number="1" location="packages/screenshot/scripts/debug-capture.local.mjs:6">
P1: No error handling around the browser operations. Any rejection (e.g., page navigation timeout, missing dist file, fixture not found) will leak the Chromium process. Wrap the capture logic in try/catch/finally and move `browser.close()` into the finally block.</violation>
<violation number="2" location="packages/screenshot/scripts/debug-capture.local.mjs:22">
P0: The addScriptTag path is a hardcoded absolute path that only works on the author's machine. Use `import.meta.url` to derive the path relative to the script so the debug capture runs on any checkout.</violation>
</file>
<file name="packages/screenshot/src/capture/clone-tree.ts">
<violation number="1" location="packages/screenshot/src/capture/clone-tree.ts:176">
P2: `filterNode` exclusions can be bypassed for SVG template containers, so elements intentionally filtered out may still appear in the captured output. This branch deep-clones `defs/symbol/...` before any snapshot/filter gate; consider carrying `filterNode` into clone context (or equivalent gate) and short-circuiting this path when it returns false.</violation>
</file>
<file name="packages/screenshot/e2e/fixtures/ms-img-formats-and-broken.html">
<violation number="1" location="packages/screenshot/e2e/fixtures/ms-img-formats-and-broken.html:30">
P3: Same pattern as the first — alt text is absent on all four images in this fixture. These are decorative test images so they should use `alt=""` to match codebase conventions.</violation>
</file>
<file name="packages/screenshot/src/utils/is-css-grouping-rule.ts">
<violation number="1" location="packages/screenshot/src/utils/is-css-grouping-rule.ts:1">
P2: The type predicate `rule is CSSGroupingRule` is too permissive: `CSSKeyframesRule` also has a `cssRules` property, so `"cssRules" in rule` returns true for keyframe rules even though they are not `CSSGroupingRule` instances. The current usage in `visitDocumentCssRules` only reads `rule.cssRules`, which works at runtime, but the incorrect narrowing is a type safety gap that could hide bugs if someone later calls `CSSGroupingRule`-specific methods like `insertRule` or `deleteRule` (different signatures on `CSSKeyframesRule`). Prefer `rule instanceof CSSGroupingRule` for an accurate type guard, or use a broader type like `CSSRule & { cssRules: CSSRuleList }` if cross-realm safety is a concern.</violation>
</file>
<file name="packages/screenshot/src/constants.ts">
<violation number="1" location="packages/screenshot/src/constants.ts:35">
P2: ZERO_SCALE_TRANSFORMS is missing the Chromium matrix serialization for scaleY(0). The set includes the keyword form `scaleY(0)` for non-Chromium engines and includes matrix forms for `scaleX(0)` and `scale(0)`, but the matrix form for `scaleY(0)` — `matrix(1, 0, 0, 0, 0, 0)` — is absent. If Chromium serializes `transform: scaleY(0)` to that matrix in `getComputedStyle`, `isZeroScaleOverlay` would return `false` and the overlay would not be skipped during snapshotting.</violation>
</file>
<file name="packages/screenshot/src/capture/inline-resources.ts">
<violation number="1" location="packages/screenshot/src/capture/inline-resources.ts:26">
P1: Aborting a capture can still leave resource fetches running until timeout, so canceled screenshots continue network/CPU work in the background. This happens because inlining calls `loadResourceAsDataUrl` without propagating the caller abort signal; threading `AbortSignal` through this path would align cancellation with `captureNode`.</violation>
</file>
<file name="packages/screenshot/src/utils/extract-font-face-blocks.ts">
<violation number="1" location="packages/screenshot/src/utils/extract-font-face-blocks.ts:8">
P2: The keyword search for `@font-face` doesn't skip occurrences inside CSS string literals. A `@font-face` substring inside a `content` or other property value can cause the function to match unrelated braces and extract a garbage block. Consider using a CSS-aware tokenizer or skipping string contexts when scanning for at-rules.</violation>
</file>
<file name="packages/screenshot/src/capture/inline-svg-defs.ts">
<violation number="1" location="packages/screenshot/src/capture/inline-svg-defs.ts:52">
P2: Some SVG defs can stay unresolved even after external documents are loaded because failed fragment lookups are permanently deduped in `queuedIds`. Consider allowing retries when lookup scope expands (for example, clear the queued marker when a pending ID is drained but not found).</violation>
<violation number="2" location="packages/screenshot/src/capture/inline-svg-defs.ts:150">
P1: External `<use>` references can resolve to the wrong symbol when a local element already uses the same ID, because the code always rewrites to `#fragmentId` even if the external fragment was not imported. It would be safer to skip rewriting when the ID was already present (or otherwise confirm the external fragment was actually inlined first).</violation>
</file>
<file name="packages/screenshot/src/utils/select-srcset-candidate.ts">
<violation number="1" location="packages/screenshot/src/utils/select-srcset-candidate.ts:9">
P1: `srcset` entries containing commas in the URL are parsed incorrectly here, so the freeze step can pick a broken candidate instead of the browser-equivalent `currentSrc`. That can degrade image fidelity or fall through to transparent-pixel fallback even when a valid candidate exists.</violation>
<violation number="2" location="packages/screenshot/src/utils/select-srcset-candidate.ts:12">
P2: Invalid descriptors are currently treated as implicit `1x`, which can select a candidate browsers would reject. Filtering unknown descriptor formats keeps selection behavior aligned with native `currentSrc` resolution.</violation>
</file>
<file name="packages/screenshot/e2e/fixtures/ms-css-counter.html">
<violation number="1" location="packages/screenshot/e2e/fixtures/ms-css-counter.html:17">
P2: The fixture tests CSS counter rendering but never increments the counter — all four `<span>` elements display the same value "0". Without a `counter-increment` rule, the fixture doesn't verify that the library correctly captures progressing counter values across sibling elements. Add `counter-increment: number` to the `span` rule so each span renders an incremented value (1, 2, 3, 4) — that would properly exercise the counter capture path and distinguish this fixture from a static-content test.</violation>
</file>
<file name="packages/screenshot/e2e/fixtures/ms-illegal-xml-characters.html">
<violation number="1" location="packages/screenshot/e2e/fixtures/ms-illegal-xml-characters.html:35">
P2: Off-by-one in U+0000–U+0008 range: `Array.from({length: 0x0008})` produces only 8 items (U+0000–U+0007). U+0008 is also an illegal XML character and should be included for coverage of the full C0-control block that the fixture is exercising. Use `length: 0x0009` to include U+0008.</violation>
<violation number="2" location="packages/screenshot/e2e/fixtures/ms-illegal-xml-characters.html:40">
P2: Off-by-one in U+000E–U+001F range: `length: 0x001f - 0x000e` gives 17 items (U+000E–U+001E). The fixture misses U+001F, the boundary character just before the legal U+0020 (space). Use `length: 0x001f - 0x000e + 1` (or `0x0012`).</violation>
<violation number="3" location="packages/screenshot/e2e/fixtures/ms-illegal-xml-characters.html:44">
P2: Off-by-one in U+D800–U+DFFF range: `length: 0xdfff - 0xd800` gives 2047 items (U+D800–U+DFFE). U+DFFF, the last lone surrogate and the boundary character before the legal U+E000 block, is excluded. Use `length: 0xdfff - 0xd800 + 1` (or `0x0800`).</violation>
</file>
<file name="packages/screenshot/src/index.ts">
<violation number="1" location="packages/screenshot/src/index.ts:150">
P2: Concurrent captures can interfere with iframe handling because recursion tracking is process-global and not reference-counted per in-flight capture. This can cause same-origin iframe snapshots to be skipped or guard state to clear too early; isolating tracking per capture stack (or using ref counts) avoids cross-request bleed-through.</violation>
</file>
<file name="packages/screenshot/src/utils/is-replaced-element.ts">
<violation number="1" location="packages/screenshot/src/utils/is-replaced-element.ts:3">
P2: Add unit tests for `isReplacedElement`. The project tests similar utilities like `isZeroScaleOverlay`, and this function drives size-freezing policy for inline replaced elements. Missing coverage means a regression in replaced-element detection could silently affect screenshot fidelity.</violation>
</file>
<file name="packages/screenshot/src/capture/freeze-fixed.ts">
<violation number="1" location="packages/screenshot/src/capture/freeze-fixed.ts:37">
P1: Fixed descendants can render at the wrong position when the capture root itself is the containing block trigger (e.g. scrolled root with wrapper, or transformed static root). The ancestor walk excludes `rootElement`, so root-specific containing-block and scroll-wrapper offsets are not applied; include root in the search so the existing containing-block branch handles it.</violation>
</file>
<file name="packages/screenshot/bench/utils/score-png-pair.ts">
<violation number="1" location="packages/screenshot/bench/utils/score-png-pair.ts:9">
P3: Scoring logic now exists in two places, so future threshold/cropping/formula updates can diverge between bench and fidelity results. Consider extracting one shared scorer utility and calling it from both paths.</violation>
</file>
<file name="packages/screenshot/e2e/fixtures/hti-webp-image.html">
<violation number="1" location="packages/screenshot/e2e/fixtures/hti-webp-image.html:24">
P0: The asset `hti-image.webp` is not a valid WebP image — it is a plain text placeholder. The fixture `hti-webp-image.html` references it via `<img src="./assets/hti-image.webp" />` but the browser will fail to decode it, so the test captures an empty/broken-image slot instead of a rendered WebP. Replace the file with a genuine WebP image (e.g., convert an existing fixture PNG via `cwebp` or a simple canvas-to-blob script) so the fixture actually exercises WebP rendering in the screenshot pipeline.</violation>
</file>
<file name="packages/screenshot/src/utils/wait-for-animation-frames.ts">
<violation number="1" location="packages/screenshot/src/utils/wait-for-animation-frames.ts:13">
P2: The settle step can now complete without any animation frame when the timeout fires first, which changes double-rAF behavior in the rasterize pipeline. Consider keeping this helper strictly frame-driven (or moving non-frame fallback behavior out of this utility) so decode settling still matches the documented contract.</violation>
</file>
<file name="packages/screenshot/src/capture/embed-fonts.ts">
<violation number="1" location="packages/screenshot/src/capture/embed-fonts.ts:37">
P1: Failed font URL inlining currently falls back to the original remote URL, so timed-out/failed font fetches can still trigger later unbounded network fetches during SVG render instead of degrading deterministically. Using the same transparent-pixel fallback as other resource inlining keeps capture behavior consistent and bounded.</violation>
</file>
<file name="packages/screenshot/src/utils/parse-font-families.ts">
<violation number="1" location="packages/screenshot/src/utils/parse-font-families.ts:6">
P2: Quoted font names with internal commas are incorrectly split. A value like `"Chronicle Display, Italic", serif` would produce `["chronicle display", "italic"]` instead of `["chronicle display, italic"]`, causing font-face rule matching to miss the real font. Consider a parser that tracks quote state while splitting.</violation>
</file>
<file name="packages/screenshot/src/utils/fetch-as-data-url.ts">
<violation number="1" location="packages/screenshot/src/utils/fetch-as-data-url.ts:3">
P2: Capture cancellation cannot stop in-flight resource fetches early because this helper only listens to its internal timeout signal. Accepting an optional `abortSignal` and merging it with the timeout signal keeps resource inlining cancellable between pipeline stages.</violation>
</file>
<file name="packages/screenshot/src/utils/sanitize-svg-subtree.ts">
<violation number="1" location="packages/screenshot/src/utils/sanitize-svg-subtree.ts:7">
P1: Serialized SVG can still carry inline event-handler attributes (for example `onclick`) because plain-name attributes are accepted solely by XML-name shape. Filtering `on*` handler names during attribute validation would align this sanitizer with the no-dynamic-content requirement.</violation>
<violation number="2" location="packages/screenshot/src/utils/sanitize-svg-subtree.ts:21">
P1: Link-bearing attributes can retain `javascript:` payloads because value sanitization only removes invalid XML code points. Removing `href`/`xlink:href` attributes when they resolve to a `javascript:` scheme would enforce the documented security constraint.</violation>
</file>
<file name="packages/screenshot/bench/utils/compute-quantile.ts">
<violation number="1" location="packages/screenshot/bench/utils/compute-quantile.ts:1">
P2: Consider validating that quantile is in [0, 1] at the top of the function. An out-of-range value would compute invalid indices and silently produce incorrect results — the `?? 0` / `?? lowerValue` fallbacks mask the out-of-bounds access rather than surfacing the misuse.</violation>
</file>
<file name="packages/screenshot/e2e/fixtures/ms-max-height-important-override.html">
<violation number="1" location="packages/screenshot/e2e/fixtures/ms-max-height-important-override.html:31">
P3: The class `.min-height` sets `max-height`, not `min-height`. This makes the fixture harder to read at a glance — someone scanning the CSS or HTML to understand the test scenario has to reconcile a `min-height` class name with `max-height` property logic. Consider renaming the class (e.g., `.max-height-override`) to reflect the property it actually controls.</violation>
</file>
<file name="packages/screenshot/src/capture/pseudo-elements.ts">
<violation number="1" location="packages/screenshot/src/capture/pseudo-elements.ts:63">
P2: Some `::first-letter` styling can be dropped for elements that only get leading text from generated content. The new `!element.firstChild` guard exits before computing `::first-letter`, so those valid pseudo styles are never snapshotted; consider removing the guard or gating on actual first-letter applicability instead of DOM children.</violation>
</file>
<file name="packages/screenshot/src/capture/default-styles.ts">
<violation number="1" location="packages/screenshot/src/capture/default-styles.ts:12">
P2: Baseline dedup can become incorrect across captures because cached defaults are global to the module, not scoped to the current document context. Scoping the cache to each `createStyleSandbox` instance (or including document-mode context in the key) would avoid cross-document baseline reuse.</violation>
</file>
<file name="packages/screenshot/src/capture/freeze-sticky.ts">
<violation number="1" location="packages/screenshot/src/capture/freeze-sticky.ts:36">
P1: Frozen sticky offsets can be wrong when a sticky element sits under an ancestor that establishes an absolute containing block. The calculation uses scroll-container coordinates, but absolute top/left resolve against the nearest containing block, so this path likely needs containing-block resolution similar to `freezeFixedDescendants`.</violation>
<violation number="2" location="packages/screenshot/src/capture/freeze-sticky.ts:38">
P2: Sticky synthesized elements can render incorrectly after freezing because existing inline styles are replaced by the absolute-position style string. Appending the positioning declarations to the current `style` attribute preserves required styles like video frame fitting and indeterminate checkbox paint.</violation>
</file>
<file name="packages/screenshot/src/capture/margin-collapse.ts">
<violation number="1" location="packages/screenshot/src/capture/margin-collapse.ts:23">
P2: Escaped bottom margin can be over-applied when a parent has definite height or min-height. This helper currently treats those boxes as collapsible, so matching parent/child bottoms can incorrectly emit extra `margin-bottom` in the clone.</violation>
</file>
<file name="packages/screenshot/src/utils/parse-scale-linear.ts">
<violation number="1" location="packages/screenshot/src/utils/parse-scale-linear.ts:8">
P1: Percentage values (e.g., `scale: 50%`) produce scaling factors 100x too large because `Number.parseFloat("50%")` returns `50`, not `0.5`. Add a `%` suffix check that divides by 100, or strip `%` and divide before returning.</violation>
</file>
<file name="packages/screenshot/src/utils/visit-document-css-rules.ts">
<violation number="1" location="packages/screenshot/src/utils/visit-document-css-rules.ts:31">
P2: Imported stylesheet rules can be processed twice, which inflates scan cost and can duplicate collected CSS (like repeated `@font-face` blocks). The top-level loop includes all `document.styleSheets` while `@import` traversal already descends into those same sheets; filtering top-level sheets to `!ownerRule` avoids double traversal.</violation>
</file>
Note: This PR contains a large number of files. cubic only reviews up to 200 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
Re-trigger cubic
| <body> | ||
| <div id="target"> | ||
| <div> | ||
| <img src="./assets/hti-image.webp" alt="" /> |
Contributor
There was a problem hiding this comment.
P0: The asset hti-image.webp is not a valid WebP image — it is a plain text placeholder. The fixture hti-webp-image.html references it via <img src="./assets/hti-image.webp" /> but the browser will fail to decode it, so the test captures an empty/broken-image slot instead of a rendered WebP. Replace the file with a genuine WebP image (e.g., convert an existing fixture PNG via cwebp or a simple canvas-to-blob script) so the fixture actually exercises WebP rendering in the screenshot pipeline.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/e2e/fixtures/hti-webp-image.html, line 24:
<comment>The asset `hti-image.webp` is not a valid WebP image — it is a plain text placeholder. The fixture `hti-webp-image.html` references it via `<img src="./assets/hti-image.webp" />` but the browser will fail to decode it, so the test captures an empty/broken-image slot instead of a rendered WebP. Replace the file with a genuine WebP image (e.g., convert an existing fixture PNG via `cwebp` or a simple canvas-to-blob script) so the fixture actually exercises WebP rendering in the screenshot pipeline.</comment>
<file context>
@@ -0,0 +1,28 @@
+ <body>
+ <div id="target">
+ <div>
+ <img src="./assets/hti-image.webp" alt="" />
+ </div>
+ </div>
</file context>
|
|
||
| const fixtureId = process.argv[2]; | ||
| const dumpSvg = process.argv.includes("--svg"); | ||
| const browser = await chromium.launch({ |
Contributor
There was a problem hiding this comment.
P1: No error handling around the browser operations. Any rejection (e.g., page navigation timeout, missing dist file, fixture not found) will leak the Chromium process. Wrap the capture logic in try/catch/finally and move browser.close() into the finally block.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/scripts/debug-capture.local.mjs, line 6:
<comment>No error handling around the browser operations. Any rejection (e.g., page navigation timeout, missing dist file, fixture not found) will leak the Chromium process. Wrap the capture logic in try/catch/finally and move `browser.close()` into the finally block.</comment>
<file context>
@@ -0,0 +1,43 @@
+
+const fixtureId = process.argv[2];
+const dumpSvg = process.argv.includes("--svg");
+const browser = await chromium.launch({
+ args: [
+ "--force-device-scale-factor=1",
</file context>
| if (externalDocument.querySelector("parsererror")) continue; | ||
| for (const { fragmentId, useElement } of references) { | ||
| enqueueFragmentId(fragmentId, [externalDocument, sourceDocument]); | ||
| useElement.setAttribute("href", `#${fragmentId}`); |
Contributor
There was a problem hiding this comment.
P1: External <use> references can resolve to the wrong symbol when a local element already uses the same ID, because the code always rewrites to #fragmentId even if the external fragment was not imported. It would be safer to skip rewriting when the ID was already present (or otherwise confirm the external fragment was actually inlined first).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/src/capture/inline-svg-defs.ts, line 150:
<comment>External `<use>` references can resolve to the wrong symbol when a local element already uses the same ID, because the code always rewrites to `#fragmentId` even if the external fragment was not imported. It would be safer to skip rewriting when the ID was already present (or otherwise confirm the external fragment was actually inlined first).</comment>
<file context>
@@ -0,0 +1,157 @@
+ if (externalDocument.querySelector("parsererror")) continue;
+ for (const { fragmentId, useElement } of references) {
+ enqueueFragmentId(fragmentId, [externalDocument, sourceDocument]);
+ useElement.setAttribute("href", `#${fragmentId}`);
+ if (useElement.hasAttributeNS(XLINK_NAMESPACE_URI, "href")) {
+ useElement.setAttributeNS(XLINK_NAMESPACE_URI, "xlink:href", `#${fragmentId}`);
</file context>
| devicePixelRatio: number, | ||
| ): string | null => { | ||
| const candidates: SrcsetCandidate[] = []; | ||
| for (const candidateEntry of srcsetValue.split(",")) { |
Contributor
There was a problem hiding this comment.
P1: srcset entries containing commas in the URL are parsed incorrectly here, so the freeze step can pick a broken candidate instead of the browser-equivalent currentSrc. That can degrade image fidelity or fall through to transparent-pixel fallback even when a valid candidate exists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/src/utils/select-srcset-candidate.ts, line 9:
<comment>`srcset` entries containing commas in the URL are parsed incorrectly here, so the freeze step can pick a broken candidate instead of the browser-equivalent `currentSrc`. That can degrade image fidelity or fall through to transparent-pixel fallback even when a valid candidate exists.</comment>
<file context>
@@ -0,0 +1,28 @@
+ devicePixelRatio: number,
+): string | null => {
+ const candidates: SrcsetCandidate[] = [];
+ for (const candidateEntry of srcsetValue.split(",")) {
+ const [candidateUrl, descriptor] = candidateEntry.trim().split(/\s+/);
+ if (!candidateUrl) continue;
</file context>
| } | ||
| return false; | ||
| }; | ||
| for (const sheet of [...sourceDocument.styleSheets, ...sourceDocument.adoptedStyleSheets]) { |
Contributor
There was a problem hiding this comment.
P2: Imported stylesheet rules can be processed twice, which inflates scan cost and can duplicate collected CSS (like repeated @font-face blocks). The top-level loop includes all document.styleSheets while @import traversal already descends into those same sheets; filtering top-level sheets to !ownerRule avoids double traversal.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/src/utils/visit-document-css-rules.ts, line 31:
<comment>Imported stylesheet rules can be processed twice, which inflates scan cost and can duplicate collected CSS (like repeated `@font-face` blocks). The top-level loop includes all `document.styleSheets` while `@import` traversal already descends into those same sheets; filtering top-level sheets to `!ownerRule` avoids double traversal.</comment>
<file context>
@@ -0,0 +1,38 @@
+ }
+ return false;
+ };
+ for (const sheet of [...sourceDocument.styleSheets, ...sourceDocument.adoptedStyleSheets]) {
+ try {
+ if (visitRuleList(sheet.cssRules, sheet.href ?? sourceDocument.baseURI)) return;
</file context>
| const blocks: string[] = []; | ||
| let searchIndex = 0; | ||
| while (searchIndex < strippedCss.length) { | ||
| const atRuleIndex = strippedCss.indexOf("@font-face", searchIndex); |
Contributor
There was a problem hiding this comment.
P2: The keyword search for @font-face doesn't skip occurrences inside CSS string literals. A @font-face substring inside a content or other property value can cause the function to match unrelated braces and extract a garbage block. Consider using a CSS-aware tokenizer or skipping string contexts when scanning for at-rules.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/src/utils/extract-font-face-blocks.ts, line 8:
<comment>The keyword search for `@font-face` doesn't skip occurrences inside CSS string literals. A `@font-face` substring inside a `content` or other property value can cause the function to match unrelated braces and extract a garbage block. Consider using a CSS-aware tokenizer or skipping string contexts when scanning for at-rules.</comment>
<file context>
@@ -0,0 +1,18 @@
+ const blocks: string[] = [];
+ let searchIndex = 0;
+ while (searchIndex < strippedCss.length) {
+ const atRuleIndex = strippedCss.indexOf("@font-face", searchIndex);
+ if (atRuleIndex === -1) break;
+ const openBraceIndex = strippedCss.indexOf("{", atRuleIndex);
</file context>
| <body> | ||
| <div id="target"> | ||
| <img src="./assets/ms-image.png" width="100" height="100" /> | ||
| <img src="./assets/ms-image.jpeg" width="100" height="100" /> |
Contributor
There was a problem hiding this comment.
P3: Same pattern as the first — alt text is absent on all four images in this fixture. These are decorative test images so they should use alt="" to match codebase conventions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/e2e/fixtures/ms-img-formats-and-broken.html, line 30:
<comment>Same pattern as the first — alt text is absent on all four images in this fixture. These are decorative test images so they should use `alt=""` to match codebase conventions.</comment>
<file context>
@@ -0,0 +1,35 @@
+ <body>
+ <div id="target">
+ <img src="./assets/ms-image.png" width="100" height="100" />
+ <img src="./assets/ms-image.jpeg" width="100" height="100" />
+ <img src="./assets/ms-image.webp" width="100" height="100" />
+ <img src="./assets/ms-error.png" width="100" height="100" />
</file context>
| import type { PngPairScore } from "../types"; | ||
|
|
||
| export const scorePngPair = (expectedPng: PNG, actualPng: PNG): PngPairScore => { | ||
| const intersectionWidthPx = Math.min(expectedPng.width, actualPng.width); |
Contributor
There was a problem hiding this comment.
P3: Scoring logic now exists in two places, so future threshold/cropping/formula updates can diverge between bench and fidelity results. Consider extracting one shared scorer utility and calling it from both paths.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/bench/utils/score-png-pair.ts, line 9:
<comment>Scoring logic now exists in two places, so future threshold/cropping/formula updates can diverge between bench and fidelity results. Consider extracting one shared scorer utility and calling it from both paths.</comment>
<file context>
@@ -0,0 +1,25 @@
+import type { PngPairScore } from "../types";
+
+export const scorePngPair = (expectedPng: PNG, actualPng: PNG): PngPairScore => {
+ const intersectionWidthPx = Math.min(expectedPng.width, actualPng.width);
+ const intersectionHeightPx = Math.min(expectedPng.height, actualPng.height);
+ const croppedExpectedPng = cropPng(expectedPng, intersectionWidthPx, intersectionHeightPx);
</file context>
| background-color: blue; | ||
| margin-bottom: 50px; | ||
| } | ||
| .container .min-height { |
Contributor
There was a problem hiding this comment.
P3: The class .min-height sets max-height, not min-height. This makes the fixture harder to read at a glance — someone scanning the CSS or HTML to understand the test scenario has to reconcile a min-height class name with max-height property logic. Consider renaming the class (e.g., .max-height-override) to reflect the property it actually controls.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/e2e/fixtures/ms-max-height-important-override.html, line 31:
<comment>The class `.min-height` sets `max-height`, not `min-height`. This makes the fixture harder to read at a glance — someone scanning the CSS or HTML to understand the test scenario has to reconcile a `min-height` class name with `max-height` property logic. Consider renaming the class (e.g., `.max-height-override`) to reflect the property it actually controls.</comment>
<file context>
@@ -0,0 +1,52 @@
+ background-color: blue;
+ margin-bottom: 50px;
+ }
+ .container .min-height {
+ max-height: unset !important;
+ }
</file context>
…props on appearance:auto controls Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
…nableIframeBridge postMessage bridge Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
… bridge Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
commit: |
…tline-offset in auto bleed Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Additional Suggestions:
- The capture root's
backdrop-filteris emitted unchanged into the clone but cannot be rendered from an SVGforeignObject, so directly capturing a frosted-glass element produces wrong output (the filter effect is lost/mispainted).
- Cross-origin iframe capture ignores the abort signal during the bridge/hook wait, so aborted captures keep running until the bridge timeout instead of rejecting promptly.
Contributor
There was a problem hiding this comment.
10 issues found across 27 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/screenshot/src/utils/apply-baked-backdrop-background.ts">
<violation number="1" location="packages/screenshot/src/utils/apply-baked-backdrop-background.ts:34">
P1: The baked backdrop cleanup removes only `backdrop-filter`, but not the vendor-prefixed `-webkit-backdrop-filter`. Because the screenshot pipeline captures and stores vendor-prefixed properties (e.g., `-webkit-text-fill-color`, `-webkit-user-select`), a `-webkit-backdrop-filter` value can remain in the cloned element's styles after baking. On WebKit browsers the clone would then apply a live backdrop filter on top of the already-baked PNG background layer, producing a double-filtered output. Consider also deleting `diffed["-webkit-backdrop-filter"]` alongside `backdrop-filter`.</violation>
</file>
<file name="packages/screenshot/src/constants.ts">
<violation number="1" location="packages/screenshot/src/constants.ts:62">
P1: Cross-origin iframe captures can silently fall back to placeholders when the bridge times out before the child finishes its own capture. The hardcoded 2000ms bridge response timeout is shorter than the 8000ms resource timeout used by the same capture pipeline, so any font or image load in the iframe that takes more than 2 seconds will cause a false timeout even though the library is prepared to wait up to 8 seconds for resources in the main document. Consider aligning the bridge timeout with `DEFAULT_RESOURCE_TIMEOUT_MS` (with a small buffer) or making it configurable.</violation>
<violation number="2" location="packages/screenshot/src/constants.ts:89">
P2: These four `border-*-width` values are duplicated in `CONCRETE_VALUE_STYLE_PROPS`. Because both sets are consumed together in `diff-styles.ts` to decide whether a concrete value is forced, any future addition to one set that isn’t mirrored in the other would silently change behavior for `appearance: auto` controls. Deriving the concrete-value set from the border-width set removes that drift risk.</violation>
</file>
<file name="packages/screenshot/src/utils/find-document-background-color.ts">
<violation number="1" location="packages/screenshot/src/utils/find-document-background-color.ts:13">
P2: `findDocumentBackgroundColor` can misclassify transparent document backgrounds as opaque, causing the screenshot pipeline to pick the wrong background color. `getComputedStyle(...).backgroundColor` may return fully-transparent values like `rgba(255, 255, 255, 0)` or the keyword `"transparent"`, but the current predicate only excludes the exact constant `"rgba(0, 0, 0, 0)"`. When a root background is one of those other transparent forms, `.find(...)` short-circuits on it instead of falling through to a visible body background, which hurts screenshot fidelity. Consider also rejecting `"transparent"` and any `rgba(..., 0)` color, or parse the alpha channel to treat all alpha-0 values as transparent.</violation>
</file>
<file name="packages/screenshot/src/utils/compute-filter-extent.ts">
<violation number="1" location="packages/screenshot/src/utils/compute-filter-extent.ts:8">
P2: The regex-based filter parser only handles one level of nested parentheses, which makes it fragile for nested CSS functional syntax. If a computed filter value contains nested functions (for example, certain color functions or browser-specific normalization), the `blur`/`drop-shadow` branch may silently fail to match and the resulting bleed `extent` will be too small, causing clipped output in the final screenshot. Consider replacing the regex with a character-level tokenizer that tracks parenthesis depth, or add defensive test coverage for computed filter values with nested parentheses.</violation>
</file>
<file name="packages/screenshot/src/capture/iframe-bridge.ts">
<violation number="1" location="packages/screenshot/src/capture/iframe-bridge.ts:60">
P2: The bridge uses a single global `isBridgeCaptureInFlight` flag that silently drops all incoming requests while any capture is in progress. This prevents mutual-embed recursion as intended, but it also suppresses legitimate concurrent bridge requests to the same iframe document. When multiple `captureNode` calls run in parallel and target the same cross-origin iframe, only the first bridge request is honored; the rest time out and fall back to flat placeholder output, causing nondeterministic fidelity regressions. Consider using a per-request lock keyed by `event.source` (or a request queue) so that unrelated concurrent requests are not dropped.</violation>
<violation number="2" location="packages/screenshot/src/capture/iframe-bridge.ts:77">
P1: The iframe bridge message handler responds to capture requests from any origin without validating `event.origin`, and it replies with `targetOrigin: "*"`. Once `enableIframeBridge()` is active, any window that can `postMessage` to the page can request and receive a rendered PNG snapshot of the document. Consider adding an `allowedOrigins` option to `enableIframeBridge` so callers can restrict which origins are permitted to request captures, and validate `event.origin` against that list before invoking `captureDocumentRoot`.</violation>
</file>
<file name="packages/screenshot/src/index.ts">
<violation number="1" location="packages/screenshot/src/index.ts:217">
P2: Cross-origin iframe resolution via `requestIframeContentViaBridge` waits up to `IFRAME_BRIDGE_RESPONSE_TIMEOUT_MS` (2 s) without racing against `options.abortSignal`. Unlike other pipeline stages that use `raceWithAbortSignal`, an aborted capture will continue waiting here until the bridge times out. Consider racing the `Promise.all` (or each individual bridge request) with the abort signal so the capture rejects promptly on abort.</violation>
</file>
<file name="packages/screenshot/src/capture/bake-backdrop-filters.ts">
<violation number="1" location="packages/screenshot/src/capture/bake-backdrop-filters.ts:10">
P2: `collectBackdropFilterElements` explicitly skips the root element (`element === rootElement`), so when `captureNode` is called directly on an element that carries `backdrop-filter`, the property is never baked and is still emitted in the serialized styles. Since SVG `foreignObject` cannot evaluate `backdrop-filter`, the frosted effect will be absent in the output. At minimum the property should be stripped from the root's emitted styles even when baking isn't feasible (the backdrop content lives outside the capture root). Consider removing the early `continue` and handling the root alongside other backdrop elements, or explicitly deleting `backdrop-filter` from the root's diffed styles when it can't be baked.</violation>
<violation number="2" location="packages/screenshot/src/capture/bake-backdrop-filters.ts:82">
P2: Rounding `elementRect.width` and `elementRect.height` to integers before passing them into `renderFilteredBackdropRegion` can introduce a 1-screen-pixel off-by-one in canvas dimensions for fractional layout sizes. Because `pixelRatio` is applied and rounded *after* the initial `Math.round`, the path `Math.round(Math.round(cssPx) * pixelRatio)` diverges from `Math.round(cssPx * pixelRatio)` by up to ~`pixelRatio` pixels (e.g., `100.6` at DPR 2 yields `202` instead of `201`). Since the position offsets are not rounded, the source-draw region and destination canvas size are misaligned, which can clip or expand the baked backdrop-filter underlay by a visible pixel when composited against the element. Consider passing the raw fractional CSS dimensions to `renderFilteredBackdropRegion` and rounding only the final screen-pixel canvas size (`Math.round(cssPx * pixelRatio)`) so the underlay footprint matches the element’s exact subpixel bounds.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| styles: StyleDeclarationMap, | ||
| bakedPngDataUrl: string, | ||
| ): void => { | ||
| delete diffed["backdrop-filter"]; |
Contributor
There was a problem hiding this comment.
P1: The baked backdrop cleanup removes only backdrop-filter, but not the vendor-prefixed -webkit-backdrop-filter. Because the screenshot pipeline captures and stores vendor-prefixed properties (e.g., -webkit-text-fill-color, -webkit-user-select), a -webkit-backdrop-filter value can remain in the cloned element's styles after baking. On WebKit browsers the clone would then apply a live backdrop filter on top of the already-baked PNG background layer, producing a double-filtered output. Consider also deleting diffed["-webkit-backdrop-filter"] alongside backdrop-filter.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/src/utils/apply-baked-backdrop-background.ts, line 34:
<comment>The baked backdrop cleanup removes only `backdrop-filter`, but not the vendor-prefixed `-webkit-backdrop-filter`. Because the screenshot pipeline captures and stores vendor-prefixed properties (e.g., `-webkit-text-fill-color`, `-webkit-user-select`), a `-webkit-backdrop-filter` value can remain in the cloned element's styles after baking. On WebKit browsers the clone would then apply a live backdrop filter on top of the already-baked PNG background layer, producing a double-filtered output. Consider also deleting `diffed["-webkit-backdrop-filter"]` alongside `backdrop-filter`.</comment>
<file context>
@@ -0,0 +1,72 @@
+ styles: StyleDeclarationMap,
+ bakedPngDataUrl: string,
+): void => {
+ delete diffed["backdrop-filter"];
+ const existingImageValue = styles["background-image"] ?? "none";
+ const existingImageLayers =
</file context>
Suggested change
| delete diffed["backdrop-filter"]; | |
| delete diffed["backdrop-filter"]; | |
| delete diffed["-webkit-backdrop-filter"]; |
| export const IFRAME_PLACEHOLDER_BACKGROUND_COLOR = "#d9d9d9"; | ||
| export const IFRAME_BRIDGE_REQUEST_MESSAGE_TYPE = "react-grab-screenshot:iframe-capture-request"; | ||
| export const IFRAME_BRIDGE_RESPONSE_MESSAGE_TYPE = "react-grab-screenshot:iframe-capture-response"; | ||
| export const IFRAME_BRIDGE_RESPONSE_TIMEOUT_MS = 2000; |
Contributor
There was a problem hiding this comment.
P1: Cross-origin iframe captures can silently fall back to placeholders when the bridge times out before the child finishes its own capture. The hardcoded 2000ms bridge response timeout is shorter than the 8000ms resource timeout used by the same capture pipeline, so any font or image load in the iframe that takes more than 2 seconds will cause a false timeout even though the library is prepared to wait up to 8 seconds for resources in the main document. Consider aligning the bridge timeout with DEFAULT_RESOURCE_TIMEOUT_MS (with a small buffer) or making it configurable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/src/constants.ts, line 62:
<comment>Cross-origin iframe captures can silently fall back to placeholders when the bridge times out before the child finishes its own capture. The hardcoded 2000ms bridge response timeout is shorter than the 8000ms resource timeout used by the same capture pipeline, so any font or image load in the iframe that takes more than 2 seconds will cause a false timeout even though the library is prepared to wait up to 8 seconds for resources in the main document. Consider aligning the bridge timeout with `DEFAULT_RESOURCE_TIMEOUT_MS` (with a small buffer) or making it configurable.</comment>
<file context>
@@ -57,6 +57,9 @@ export const DEFAULT_ACCENT_COLOR = "rgb(0, 117, 255)";
export const IFRAME_PLACEHOLDER_BACKGROUND_COLOR = "#d9d9d9";
+export const IFRAME_BRIDGE_REQUEST_MESSAGE_TYPE = "react-grab-screenshot:iframe-capture-request";
+export const IFRAME_BRIDGE_RESPONSE_MESSAGE_TYPE = "react-grab-screenshot:iframe-capture-response";
+export const IFRAME_BRIDGE_RESPONSE_TIMEOUT_MS = 2000;
// box-sizing: Chromium's border-box theming for form controls comes from
</file context>
Suggested change
| export const IFRAME_BRIDGE_RESPONSE_TIMEOUT_MS = 2000; | |
| export const IFRAME_BRIDGE_RESPONSE_TIMEOUT_MS = DEFAULT_RESOURCE_TIMEOUT_MS + 1000; |
| heightPx: capture.heightPx, | ||
| backgroundColor: findDocumentBackgroundColor(document), | ||
| }, | ||
| { targetOrigin: "*" }, |
Contributor
There was a problem hiding this comment.
P1: The iframe bridge message handler responds to capture requests from any origin without validating event.origin, and it replies with targetOrigin: "*". Once enableIframeBridge() is active, any window that can postMessage to the page can request and receive a rendered PNG snapshot of the document. Consider adding an allowedOrigins option to enableIframeBridge so callers can restrict which origins are permitted to request captures, and validate event.origin against that list before invoking captureDocumentRoot.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/src/capture/iframe-bridge.ts, line 77:
<comment>The iframe bridge message handler responds to capture requests from any origin without validating `event.origin`, and it replies with `targetOrigin: "*"`. Once `enableIframeBridge()` is active, any window that can `postMessage` to the page can request and receive a rendered PNG snapshot of the document. Consider adding an `allowedOrigins` option to `enableIframeBridge` so callers can restrict which origins are permitted to request captures, and validate `event.origin` against that list before invoking `captureDocumentRoot`.</comment>
<file context>
@@ -0,0 +1,90 @@
+ heightPx: capture.heightPx,
+ backgroundColor: findDocumentBackgroundColor(document),
+ },
+ { targetOrigin: "*" },
+ );
+ })
</file context>
| // the UA default - counts as author styling and switches Chromium off the | ||
| // native themed painting (white select chrome since ~M143) onto legacy | ||
| // ButtonFace rendering, so equal-to-baseline widths must stay omitted there. | ||
| export const BORDER_WIDTH_STYLE_PROPS = new Set([ |
Contributor
There was a problem hiding this comment.
P2: These four border-*-width values are duplicated in CONCRETE_VALUE_STYLE_PROPS. Because both sets are consumed together in diff-styles.ts to decide whether a concrete value is forced, any future addition to one set that isn’t mirrored in the other would silently change behavior for appearance: auto controls. Deriving the concrete-value set from the border-width set removes that drift risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/src/constants.ts, line 89:
<comment>These four `border-*-width` values are duplicated in `CONCRETE_VALUE_STYLE_PROPS`. Because both sets are consumed together in `diff-styles.ts` to decide whether a concrete value is forced, any future addition to one set that isn’t mirrored in the other would silently change behavior for `appearance: auto` controls. Deriving the concrete-value set from the border-width set removes that drift risk.</comment>
<file context>
@@ -79,6 +82,17 @@ export const CONCRETE_VALUE_STYLE_PROPS = new Set([
+// the UA default - counts as author styling and switches Chromium off the
+// native themed painting (white select chrome since ~M143) onto legacy
+// ButtonFace rendering, so equal-to-baseline widths must stay omitted there.
+export const BORDER_WIDTH_STYLE_PROPS = new Set([
+ "border-top-width",
+ "border-right-width",
</file context>
| : ""; | ||
| return ( | ||
| [rootBackground, bodyBackground].find( | ||
| (backgroundColor) => backgroundColor && backgroundColor !== TRANSPARENT_BACKGROUND_COLOR, |
Contributor
There was a problem hiding this comment.
P2: findDocumentBackgroundColor can misclassify transparent document backgrounds as opaque, causing the screenshot pipeline to pick the wrong background color. getComputedStyle(...).backgroundColor may return fully-transparent values like rgba(255, 255, 255, 0) or the keyword "transparent", but the current predicate only excludes the exact constant "rgba(0, 0, 0, 0)". When a root background is one of those other transparent forms, .find(...) short-circuits on it instead of falling through to a visible body background, which hurts screenshot fidelity. Consider also rejecting "transparent" and any rgba(..., 0) color, or parse the alpha channel to treat all alpha-0 values as transparent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/src/utils/find-document-background-color.ts, line 13:
<comment>`findDocumentBackgroundColor` can misclassify transparent document backgrounds as opaque, causing the screenshot pipeline to pick the wrong background color. `getComputedStyle(...).backgroundColor` may return fully-transparent values like `rgba(255, 255, 255, 0)` or the keyword `"transparent"`, but the current predicate only excludes the exact constant `"rgba(0, 0, 0, 0)"`. When a root background is one of those other transparent forms, `.find(...)` short-circuits on it instead of falling through to a visible body background, which hurts screenshot fidelity. Consider also rejecting `"transparent"` and any `rgba(..., 0)` color, or parse the alpha channel to treat all alpha-0 values as transparent.</comment>
<file context>
@@ -0,0 +1,16 @@
+ : "";
+ return (
+ [rootBackground, bodyBackground].find(
+ (backgroundColor) => backgroundColor && backgroundColor !== TRANSPARENT_BACKGROUND_COLOR,
+ ) ?? null
+ );
</file context>
| export const computeFilterExtent = (filterValue: string | undefined): number => { | ||
| if (!filterValue || filterValue === "none") return 0; | ||
| let extent = 0; | ||
| const functionPattern = /([a-z-]+)\(([^()]*(?:\([^()]*\)[^()]*)*)\)/g; |
Contributor
There was a problem hiding this comment.
P2: The regex-based filter parser only handles one level of nested parentheses, which makes it fragile for nested CSS functional syntax. If a computed filter value contains nested functions (for example, certain color functions or browser-specific normalization), the blur/drop-shadow branch may silently fail to match and the resulting bleed extent will be too small, causing clipped output in the final screenshot. Consider replacing the regex with a character-level tokenizer that tracks parenthesis depth, or add defensive test coverage for computed filter values with nested parentheses.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/src/utils/compute-filter-extent.ts, line 8:
<comment>The regex-based filter parser only handles one level of nested parentheses, which makes it fragile for nested CSS functional syntax. If a computed filter value contains nested functions (for example, certain color functions or browser-specific normalization), the `blur`/`drop-shadow` branch may silently fail to match and the resulting bleed `extent` will be too small, causing clipped output in the final screenshot. Consider replacing the regex with a character-level tokenizer that tracks parenthesis depth, or add defensive test coverage for computed filter values with nested parentheses.</comment>
<file context>
@@ -0,0 +1,24 @@
+export const computeFilterExtent = (filterValue: string | undefined): number => {
+ if (!filterValue || filterValue === "none") return 0;
+ let extent = 0;
+ const functionPattern = /([a-z-]+)\(([^()]*(?:\([^()]*\)[^()]*)*)\)/g;
+ let functionMatch = functionPattern.exec(filterValue);
+ while (functionMatch) {
</file context>
| // recurse forever (capture -> bridge request -> capture -> ...); ignoring | ||
| // requests while a bridge capture is in flight breaks the cycle and lets | ||
| // the requester time out to the flat placeholder. | ||
| let isBridgeCaptureInFlight = false; |
Contributor
There was a problem hiding this comment.
P2: The bridge uses a single global isBridgeCaptureInFlight flag that silently drops all incoming requests while any capture is in progress. This prevents mutual-embed recursion as intended, but it also suppresses legitimate concurrent bridge requests to the same iframe document. When multiple captureNode calls run in parallel and target the same cross-origin iframe, only the first bridge request is honored; the rest time out and fall back to flat placeholder output, causing nondeterministic fidelity regressions. Consider using a per-request lock keyed by event.source (or a request queue) so that unrelated concurrent requests are not dropped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/src/capture/iframe-bridge.ts, line 60:
<comment>The bridge uses a single global `isBridgeCaptureInFlight` flag that silently drops all incoming requests while any capture is in progress. This prevents mutual-embed recursion as intended, but it also suppresses legitimate concurrent bridge requests to the same iframe document. When multiple `captureNode` calls run in parallel and target the same cross-origin iframe, only the first bridge request is honored; the rest time out and fall back to flat placeholder output, causing nondeterministic fidelity regressions. Consider using a per-request lock keyed by `event.source` (or a request queue) so that unrelated concurrent requests are not dropped.</comment>
<file context>
@@ -0,0 +1,90 @@
+ // recurse forever (capture -> bridge request -> capture -> ...); ignoring
+ // requests while a bridge capture is in flight breaks the cycle and lets
+ // the requester time out to the flat placeholder.
+ let isBridgeCaptureInFlight = false;
+ const handleMessage = (event: MessageEvent): void => {
+ const request = parseIframeBridgeRequestMessage(event.data);
</file context>
| // A failed nested capture falls back to the flat iframe placeholder. | ||
| } | ||
| } | ||
| const crossOriginSnapshots = await Promise.all( |
Contributor
There was a problem hiding this comment.
P2: Cross-origin iframe resolution via requestIframeContentViaBridge waits up to IFRAME_BRIDGE_RESPONSE_TIMEOUT_MS (2 s) without racing against options.abortSignal. Unlike other pipeline stages that use raceWithAbortSignal, an aborted capture will continue waiting here until the bridge times out. Consider racing the Promise.all (or each individual bridge request) with the abort signal so the capture rejects promptly on abort.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/src/index.ts, line 217:
<comment>Cross-origin iframe resolution via `requestIframeContentViaBridge` waits up to `IFRAME_BRIDGE_RESPONSE_TIMEOUT_MS` (2 s) without racing against `options.abortSignal`. Unlike other pipeline stages that use `raceWithAbortSignal`, an aborted capture will continue waiting here until the bridge times out. Consider racing the `Promise.all` (or each individual bridge request) with the abort signal so the capture rejects promptly on abort.</comment>
<file context>
@@ -156,47 +203,39 @@ const captureSameOriginIframeContents = async (
// A failed nested capture falls back to the flat iframe placeholder.
}
}
+ const crossOriginSnapshots = await Promise.all(
+ crossOriginIframes.map((iframe) => resolveCrossOriginIframeContent(iframe, options)),
+ );
</file context>
| const backdropFilterValue = snapshot?.styles["backdrop-filter"]; | ||
| if (!backdropFilterValue || backdropFilterValue === "none") continue; | ||
| const elementRect = backdropElement.getBoundingClientRect(); | ||
| const regionWidthPx = Math.round(elementRect.width); |
Contributor
There was a problem hiding this comment.
P2: Rounding elementRect.width and elementRect.height to integers before passing them into renderFilteredBackdropRegion can introduce a 1-screen-pixel off-by-one in canvas dimensions for fractional layout sizes. Because pixelRatio is applied and rounded after the initial Math.round, the path Math.round(Math.round(cssPx) * pixelRatio) diverges from Math.round(cssPx * pixelRatio) by up to ~pixelRatio pixels (e.g., 100.6 at DPR 2 yields 202 instead of 201). Since the position offsets are not rounded, the source-draw region and destination canvas size are misaligned, which can clip or expand the baked backdrop-filter underlay by a visible pixel when composited against the element. Consider passing the raw fractional CSS dimensions to renderFilteredBackdropRegion and rounding only the final screen-pixel canvas size (Math.round(cssPx * pixelRatio)) so the underlay footprint matches the element’s exact subpixel bounds.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/src/capture/bake-backdrop-filters.ts, line 82:
<comment>Rounding `elementRect.width` and `elementRect.height` to integers before passing them into `renderFilteredBackdropRegion` can introduce a 1-screen-pixel off-by-one in canvas dimensions for fractional layout sizes. Because `pixelRatio` is applied and rounded *after* the initial `Math.round`, the path `Math.round(Math.round(cssPx) * pixelRatio)` diverges from `Math.round(cssPx * pixelRatio)` by up to ~`pixelRatio` pixels (e.g., `100.6` at DPR 2 yields `202` instead of `201`). Since the position offsets are not rounded, the source-draw region and destination canvas size are misaligned, which can clip or expand the baked backdrop-filter underlay by a visible pixel when composited against the element. Consider passing the raw fractional CSS dimensions to `renderFilteredBackdropRegion` and rounding only the final screen-pixel canvas size (`Math.round(cssPx * pixelRatio)`) so the underlay footprint matches the element’s exact subpixel bounds.</comment>
<file context>
@@ -0,0 +1,97 @@
+ const backdropFilterValue = snapshot?.styles["backdrop-filter"];
+ if (!backdropFilterValue || backdropFilterValue === "none") continue;
+ const elementRect = backdropElement.getBoundingClientRect();
+ const regionWidthPx = Math.round(elementRect.width);
+ const regionHeightPx = Math.round(elementRect.height);
+ if (regionWidthPx <= 0 || regionHeightPx <= 0) continue;
</file context>
| ): Element[] => { | ||
| const backdropFilterElements: Element[] = []; | ||
| for (const [element, snapshot] of snapshotByElement) { | ||
| if (element === rootElement) continue; |
Contributor
There was a problem hiding this comment.
P2: collectBackdropFilterElements explicitly skips the root element (element === rootElement), so when captureNode is called directly on an element that carries backdrop-filter, the property is never baked and is still emitted in the serialized styles. Since SVG foreignObject cannot evaluate backdrop-filter, the frosted effect will be absent in the output. At minimum the property should be stripped from the root's emitted styles even when baking isn't feasible (the backdrop content lives outside the capture root). Consider removing the early continue and handling the root alongside other backdrop elements, or explicitly deleting backdrop-filter from the root's diffed styles when it can't be baked.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/src/capture/bake-backdrop-filters.ts, line 10:
<comment>`collectBackdropFilterElements` explicitly skips the root element (`element === rootElement`), so when `captureNode` is called directly on an element that carries `backdrop-filter`, the property is never baked and is still emitted in the serialized styles. Since SVG `foreignObject` cannot evaluate `backdrop-filter`, the frosted effect will be absent in the output. At minimum the property should be stripped from the root's emitted styles even when baking isn't feasible (the backdrop content lives outside the capture root). Consider removing the early `continue` and handling the root alongside other backdrop elements, or explicitly deleting `backdrop-filter` from the root's diffed styles when it can't be baked.</comment>
<file context>
@@ -0,0 +1,97 @@
+): Element[] => {
+ const backdropFilterElements: Element[] = [];
+ for (const [element, snapshot] of snapshotByElement) {
+ if (element === rootElement) continue;
+ const backdropFilterValue = snapshot.styles["backdrop-filter"];
+ if (backdropFilterValue && backdropFilterValue !== "none") {
</file context>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+14
to
+22
| const MIME_TYPES_BY_EXTENSION = { | ||
| ".html": "text/html; charset=utf-8", | ||
| ".css": "text/css; charset=utf-8", | ||
| ".js": "text/javascript; charset=utf-8", | ||
| ".mjs": "text/javascript; charset=utf-8", | ||
| ".png": "image/png", | ||
| ".svg": "image/svg+xml", | ||
| ".woff2": "font/woff2", | ||
| }; |
Contributor
There was a problem hiding this comment.
🟡 Fixture server serves JPEG and WebP images with wrong content type, potentially corrupting inlined data URLs
Image assets are served with an incorrect MIME type (application/octet-stream at packages/screenshot/scripts/serve-fixtures.mjs:14-22) because the extension-to-type map omits .jpeg and .webp entries, so inlined data URLs carry the wrong type prefix.
Impact: Captures of fixtures using JPEG or WebP images may embed data URLs with application/octet-stream instead of the correct image MIME type, risking rendering failures in stricter environments.
Missing MIME type entries cause wrong Content-Type for fetched image resources
The MIME_TYPES_BY_EXTENSION map at packages/screenshot/scripts/serve-fixtures.mjs:14-22 only includes .png and .svg for images. However, multiple fixtures reference .jpeg files (e.g., dtim-image.jpeg, hti-image.jpeg, ms-image.jpeg) and .webp files (hti-image.webp, ms-image.webp).
When captureNode runs, it calls loadResourceAsDataUrl → fetchAsDataUrl → fetch() → response.blob() → FileReader.readAsDataURL. The blob's type comes from the response's Content-Type header. Since the server returns application/octet-stream for these files, the resulting data URLs will be data:application/octet-stream;base64,... instead of data:image/jpeg;base64,... or data:image/webp;base64,....
Chromium content-sniffs and renders these correctly anyway, which is why the fidelity tests pass, but the data URLs are technically malformed for their purpose.
Suggested change
| const MIME_TYPES_BY_EXTENSION = { | |
| ".html": "text/html; charset=utf-8", | |
| ".css": "text/css; charset=utf-8", | |
| ".js": "text/javascript; charset=utf-8", | |
| ".mjs": "text/javascript; charset=utf-8", | |
| ".png": "image/png", | |
| ".svg": "image/svg+xml", | |
| ".woff2": "font/woff2", | |
| }; | |
| const MIME_TYPES_BY_EXTENSION = { | |
| ".html": "text/html; charset=utf-8", | |
| ".css": "text/css; charset=utf-8", | |
| ".js": "text/javascript; charset=utf-8", | |
| ".mjs": "text/javascript; charset=utf-8", | |
| ".png": "image/png", | |
| ".jpg": "image/jpeg", | |
| ".jpeg": "image/jpeg", | |
| ".webp": "image/webp", | |
| ".svg": "image/svg+xml", | |
| ".woff2": "font/woff2", | |
| }; | |
Was this helpful? React with 👍 or 👎 to provide feedback.
…it and Firefox with per-engine budgets - 10 hard- fixtures: paused CSS/WAAPI animations, stacked and rotated backdrop-filter glass, blend+clip+mask, vertical/RTL writing modes, styled and nested cross-origin iframes, sticky+fixed+bleed stress combo - 3 real- fixtures modeled on open-source UI patterns (repo file browser, analytics dashboard, glass-morphism landing page) - webkit-fidelity and firefox-fidelity Playwright projects; per-browser budget overrides in the manifest with documented engine quirks - fix iframe bridge listening on the wrong realm's window for nested same-origin frames - transform-aware backdrop-filter baking: map the device-space filtered region back into the untransformed layout box and composite panes onto a shared underlay in paint order Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/screenshot/src/index.ts">
<violation number="1" location="packages/screenshot/src/index.ts:132">
P2: Using `Object.hasOwn` introduces an ES2022 runtime requirement that may not be supported by all browsers your global bundle targets. Because the package builds with `ESNext` and no explicit downlevel `target`, this call ships untranspiled in the IIFE output. Switching to `Object.prototype.hasOwnProperty.call(styles, propertyName)` avoids this compatibility regression while keeping the same semantics.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const seedStyles: StyleDeclarationMap | null = Object.getPrototypeOf(styles); | ||
| for (let propertyIndex = 0; propertyIndex < perElementPropertyNames.length; propertyIndex++) { | ||
| const propertyName = perElementPropertyNames[propertyIndex]; | ||
| if (!Object.hasOwn(styles, propertyName)) continue; |
Contributor
There was a problem hiding this comment.
P2: Using Object.hasOwn introduces an ES2022 runtime requirement that may not be supported by all browsers your global bundle targets. Because the package builds with ESNext and no explicit downlevel target, this call ships untranspiled in the IIFE output. Switching to Object.prototype.hasOwnProperty.call(styles, propertyName) avoids this compatibility regression while keeping the same semantics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/src/index.ts, line 132:
<comment>Using `Object.hasOwn` introduces an ES2022 runtime requirement that may not be supported by all browsers your global bundle targets. Because the package builds with `ESNext` and no explicit downlevel `target`, this call ships untranspiled in the IIFE output. Switching to `Object.prototype.hasOwnProperty.call(styles, propertyName)` avoids this compatibility regression while keeping the same semantics.</comment>
<file context>
@@ -116,18 +116,20 @@ const bakedBackdropPngCache = createFifoCache<string[]>(BAKED_BACKDROP_CACHE_CAP
- if (propertyIndex === undefined) continue;
+ for (let propertyIndex = 0; propertyIndex < perElementPropertyNames.length; propertyIndex++) {
+ const propertyName = perElementPropertyNames[propertyIndex];
+ if (!Object.hasOwn(styles, propertyName)) continue;
const ownValue = styles[propertyName];
if (seedStyles !== null && seedStyles[propertyName] === ownValue) continue;
</file context>
Suggested change
| if (!Object.hasOwn(styles, propertyName)) continue; | |
| if (!Object.prototype.hasOwnProperty.call(styles, propertyName)) continue; |
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/screenshot/src/capture/rasterize.ts">
<violation number="1" location="packages/screenshot/src/capture/rasterize.ts:24">
P1: The nested cache structure introduces an unbounded inner map for PNG param variants. `createFifoCache` only evicts on insertion of a new outer key; when the same `svgMarkup` already exists in the outer cache, updating it does not trigger eviction, and the inner plain `Map` has no capacity limit. Capturing the same DOM repeatedly with different `scale`, `pixelRatio`, `clipRect`, or `backgroundColor` values will continuously grow memory without bound, degrading the intended `RASTER_PNG_CACHE_CAP` semantics. Consider bounding the inner map (e.g., with its own FIFO eviction or by tracking total entries across all inner maps) or restoring a single-tier key so the cap limits total cached variants.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // already-encoded PNG instead of paying decode + native PNG encode again. | ||
| // Nesting params under the markup key avoids concatenating a fresh | ||
| // multi-megabyte cache-key string per capture. | ||
| const encodedPngCache = createFifoCache<Map<string, Promise<Blob>>>(RASTER_PNG_CACHE_CAP); |
Contributor
There was a problem hiding this comment.
P1: The nested cache structure introduces an unbounded inner map for PNG param variants. createFifoCache only evicts on insertion of a new outer key; when the same svgMarkup already exists in the outer cache, updating it does not trigger eviction, and the inner plain Map has no capacity limit. Capturing the same DOM repeatedly with different scale, pixelRatio, clipRect, or backgroundColor values will continuously grow memory without bound, degrading the intended RASTER_PNG_CACHE_CAP semantics. Consider bounding the inner map (e.g., with its own FIFO eviction or by tracking total entries across all inner maps) or restoring a single-tier key so the cap limits total cached variants.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/src/capture/rasterize.ts, line 24:
<comment>The nested cache structure introduces an unbounded inner map for PNG param variants. `createFifoCache` only evicts on insertion of a new outer key; when the same `svgMarkup` already exists in the outer cache, updating it does not trigger eviction, and the inner plain `Map` has no capacity limit. Capturing the same DOM repeatedly with different `scale`, `pixelRatio`, `clipRect`, or `backgroundColor` values will continuously grow memory without bound, degrading the intended `RASTER_PNG_CACHE_CAP` semantics. Consider bounding the inner map (e.g., with its own FIFO eviction or by tracking total entries across all inner maps) or restoring a single-tier key so the cap limits total cached variants.</comment>
<file context>
@@ -19,7 +19,9 @@ import { lastCaptureTimings } from "./phase-timings";
-const encodedPngCache = createFifoCache<Promise<Blob>>(RASTER_PNG_CACHE_CAP);
+// Nesting params under the markup key avoids concatenating a fresh
+// multi-megabyte cache-key string per capture.
+const encodedPngCache = createFifoCache<Map<string, Promise<Blob>>>(RASTER_PNG_CACHE_CAP);
// A decoded SVG image is immutable, so repeat rasterizations of identical
</file context>
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/screenshot/scripts/prewarm-probe.local.mjs">
<violation number="1" location="packages/screenshot/scripts/prewarm-probe.local.mjs:8">
P2: The `measure` async function opens a Chromium browser but does not guard `browser.close()` with a `try/finally` block. If any of `chromium.launch()`, `browser.newPage()`, `page.goto()`, `page.addScriptTag()`, or `page.evaluate()` rejects, the browser process is leaked. In a repeated benchmarking script this skews runs and accumulates orphaned Chromium instances.
Wrap the body in `try/finally` so `await browser.close()` always runs regardless of failures.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Contributor
There was a problem hiding this comment.
1 issue found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/screenshot/scripts/debug-capture.local.mjs">
<violation number="1" location="packages/screenshot/scripts/debug-capture.local.mjs:6">
P1: No error handling around the browser operations. Any rejection (e.g., page navigation timeout, missing dist file, fixture not found) will leak the Chromium process. Wrap the capture logic in try/catch/finally and move `browser.close()` into the finally block.</violation>
</file>
<file name="packages/screenshot/src/capture/clone-tree.ts">
<violation number="1" location="packages/screenshot/src/capture/clone-tree.ts:26">
P2: The new `attributeNameValidityCache` is an unbounded module-level Map that accumulates permanently across all captures. In long-lived sessions (e.g., the editor app integration described in this PR), framework-generated or randomized attribute names can cause the cache to grow indefinitely. Given that the cached operation is a trivial regex test, the cache may not even improve performance—making it pure risk with little upside. Consider either removing the cache or replacing it with a size-bounded LRU.</violation>
<violation number="2" location="packages/screenshot/src/capture/clone-tree.ts:176">
P2: `filterNode` exclusions can be bypassed for SVG template containers, so elements intentionally filtered out may still appear in the captured output. This branch deep-clones `defs/symbol/...` before any snapshot/filter gate; consider carrying `filterNode` into clone context (or equivalent gate) and short-circuiting this path when it returns false.</violation>
<violation number="3" location="packages/screenshot/src/capture/clone-tree.ts:213">
P2: SVG template containers bypass the `prunedElements` subtree-pruning contract in `cloneElementNode`. The `isSvgTemplateContainer` early-return path does a full `cloneNode(true)` and returns before the `prunedElements` check, so if a `<defs>`, `<symbol>`, or similar SVG container falls outside the capture region and is added to `prunedElements`, its children are still serialized anyway. This defeats the region-based culling optimization and can bloat the serialized output with unnecessary definitions.
Consider honoring pruning in the SVG template branch — for example, by doing `cloneNode(!context.prunedElements?.has(element))` or stripping children after sanitization when the element is in `prunedElements`. Fix should ensure pruned SVG template containers are still cloned themselves (to preserve any referencing structure) but without their descendant content.</violation>
</file>
<file name="packages/screenshot/e2e/fixtures/ms-img-formats-and-broken.html">
<violation number="1" location="packages/screenshot/e2e/fixtures/ms-img-formats-and-broken.html:30">
P3: Same pattern as the first — alt text is absent on all four images in this fixture. These are decorative test images so they should use `alt=""` to match codebase conventions.</violation>
</file>
<file name="packages/screenshot/src/utils/is-css-grouping-rule.ts">
<violation number="1" location="packages/screenshot/src/utils/is-css-grouping-rule.ts:1">
P2: The type predicate `rule is CSSGroupingRule` is too permissive: `CSSKeyframesRule` also has a `cssRules` property, so `"cssRules" in rule` returns true for keyframe rules even though they are not `CSSGroupingRule` instances. The current usage in `visitDocumentCssRules` only reads `rule.cssRules`, which works at runtime, but the incorrect narrowing is a type safety gap that could hide bugs if someone later calls `CSSGroupingRule`-specific methods like `insertRule` or `deleteRule` (different signatures on `CSSKeyframesRule`). Prefer `rule instanceof CSSGroupingRule` for an accurate type guard, or use a broader type like `CSSRule & { cssRules: CSSRuleList }` if cross-realm safety is a concern.</violation>
</file>
<file name="packages/screenshot/src/constants.ts">
<violation number="1" location="packages/screenshot/src/constants.ts:35">
P2: ZERO_SCALE_TRANSFORMS is missing the Chromium matrix serialization for scaleY(0). The set includes the keyword form `scaleY(0)` for non-Chromium engines and includes matrix forms for `scaleX(0)` and `scale(0)`, but the matrix form for `scaleY(0)` — `matrix(1, 0, 0, 0, 0, 0)` — is absent. If Chromium serializes `transform: scaleY(0)` to that matrix in `getComputedStyle`, `isZeroScaleOverlay` would return `false` and the overlay would not be skipped during snapshotting.</violation>
<violation number="2" location="packages/screenshot/src/constants.ts:62">
P1: Cross-origin iframe captures can silently fall back to placeholders when the bridge times out before the child finishes its own capture. The hardcoded 2000ms bridge response timeout is shorter than the 8000ms resource timeout used by the same capture pipeline, so any font or image load in the iframe that takes more than 2 seconds will cause a false timeout even though the library is prepared to wait up to 8 seconds for resources in the main document. Consider aligning the bridge timeout with `DEFAULT_RESOURCE_TIMEOUT_MS` (with a small buffer) or making it configurable.</violation>
<violation number="3" location="packages/screenshot/src/constants.ts:89">
P2: These four `border-*-width` values are duplicated in `CONCRETE_VALUE_STYLE_PROPS`. Because both sets are consumed together in `diff-styles.ts` to decide whether a concrete value is forced, any future addition to one set that isn’t mirrored in the other would silently change behavior for `appearance: auto` controls. Deriving the concrete-value set from the border-width set removes that drift risk.</violation>
</file>
<file name="packages/screenshot/src/capture/inline-resources.ts">
<violation number="1" location="packages/screenshot/src/capture/inline-resources.ts:26">
P1: Aborting a capture can still leave resource fetches running until timeout, so canceled screenshots continue network/CPU work in the background. This happens because inlining calls `loadResourceAsDataUrl` without propagating the caller abort signal; threading `AbortSignal` through this path would align cancellation with `captureNode`.</violation>
</file>
<file name="packages/screenshot/src/utils/extract-font-face-blocks.ts">
<violation number="1" location="packages/screenshot/src/utils/extract-font-face-blocks.ts:8">
P2: The keyword search for `@font-face` doesn't skip occurrences inside CSS string literals. A `@font-face` substring inside a `content` or other property value can cause the function to match unrelated braces and extract a garbage block. Consider using a CSS-aware tokenizer or skipping string contexts when scanning for at-rules.</violation>
</file>
<file name="packages/screenshot/src/capture/inline-svg-defs.ts">
<violation number="1" location="packages/screenshot/src/capture/inline-svg-defs.ts:52">
P2: Some SVG defs can stay unresolved even after external documents are loaded because failed fragment lookups are permanently deduped in `queuedIds`. Consider allowing retries when lookup scope expands (for example, clear the queued marker when a pending ID is drained but not found).</violation>
<violation number="2" location="packages/screenshot/src/capture/inline-svg-defs.ts:150">
P1: External `<use>` references can resolve to the wrong symbol when a local element already uses the same ID, because the code always rewrites to `#fragmentId` even if the external fragment was not imported. It would be safer to skip rewriting when the ID was already present (or otherwise confirm the external fragment was actually inlined first).</violation>
</file>
<file name="packages/screenshot/src/utils/select-srcset-candidate.ts">
<violation number="1" location="packages/screenshot/src/utils/select-srcset-candidate.ts:9">
P1: `srcset` entries containing commas in the URL are parsed incorrectly here, so the freeze step can pick a broken candidate instead of the browser-equivalent `currentSrc`. That can degrade image fidelity or fall through to transparent-pixel fallback even when a valid candidate exists.</violation>
<violation number="2" location="packages/screenshot/src/utils/select-srcset-candidate.ts:12">
P2: Invalid descriptors are currently treated as implicit `1x`, which can select a candidate browsers would reject. Filtering unknown descriptor formats keeps selection behavior aligned with native `currentSrc` resolution.</violation>
</file>
<file name="packages/screenshot/e2e/fixtures/ms-css-counter.html">
<violation number="1" location="packages/screenshot/e2e/fixtures/ms-css-counter.html:17">
P2: The fixture tests CSS counter rendering but never increments the counter — all four `<span>` elements display the same value "0". Without a `counter-increment` rule, the fixture doesn't verify that the library correctly captures progressing counter values across sibling elements. Add `counter-increment: number` to the `span` rule so each span renders an incremented value (1, 2, 3, 4) — that would properly exercise the counter capture path and distinguish this fixture from a static-content test.</violation>
</file>
<file name="packages/screenshot/e2e/fixtures/ms-illegal-xml-characters.html">
<violation number="1" location="packages/screenshot/e2e/fixtures/ms-illegal-xml-characters.html:35">
P2: Off-by-one in U+0000–U+0008 range: `Array.from({length: 0x0008})` produces only 8 items (U+0000–U+0007). U+0008 is also an illegal XML character and should be included for coverage of the full C0-control block that the fixture is exercising. Use `length: 0x0009` to include U+0008.</violation>
<violation number="2" location="packages/screenshot/e2e/fixtures/ms-illegal-xml-characters.html:40">
P2: Off-by-one in U+000E–U+001F range: `length: 0x001f - 0x000e` gives 17 items (U+000E–U+001E). The fixture misses U+001F, the boundary character just before the legal U+0020 (space). Use `length: 0x001f - 0x000e + 1` (or `0x0012`).</violation>
<violation number="3" location="packages/screenshot/e2e/fixtures/ms-illegal-xml-characters.html:44">
P2: Off-by-one in U+D800–U+DFFF range: `length: 0xdfff - 0xd800` gives 2047 items (U+D800–U+DFFE). U+DFFF, the last lone surrogate and the boundary character before the legal U+E000 block, is excluded. Use `length: 0xdfff - 0xd800 + 1` (or `0x0800`).</violation>
</file>
<file name="packages/screenshot/src/index.ts">
<violation number="1" location="packages/screenshot/src/index.ts:132">
P2: Using `Object.hasOwn` introduces an ES2022 runtime requirement that may not be supported by all browsers your global bundle targets. Because the package builds with `ESNext` and no explicit downlevel `target`, this call ships untranspiled in the IIFE output. Switching to `Object.prototype.hasOwnProperty.call(styles, propertyName)` avoids this compatibility regression while keeping the same semantics.</violation>
<violation number="2" location="packages/screenshot/src/index.ts:150">
P2: Concurrent captures can interfere with iframe handling because recursion tracking is process-global and not reference-counted per in-flight capture. This can cause same-origin iframe snapshots to be skipped or guard state to clear too early; isolating tracking per capture stack (or using ref counts) avoids cross-request bleed-through.</violation>
<violation number="3" location="packages/screenshot/src/index.ts:217">
P2: Cross-origin iframe resolution via `requestIframeContentViaBridge` waits up to `IFRAME_BRIDGE_RESPONSE_TIMEOUT_MS` (2 s) without racing against `options.abortSignal`. Unlike other pipeline stages that use `raceWithAbortSignal`, an aborted capture will continue waiting here until the bridge times out. Consider racing the `Promise.all` (or each individual bridge request) with the abort signal so the capture rejects promptly on abort.</violation>
<violation number="4" location="packages/screenshot/src/index.ts:333">
P1: In `captureNodeInternal`, the underlay capture for `backdrop-filter` baking disables clipping (`clip: undefined`) but inadvertently preserves `prunedElements` from a `captureRegion` call through `...resolvedOptions`. This means elements outside the capture region are still pruned during the underlay pass, so the baked backdrop can miss background contributors near region edges and render incorrectly. Remove `prunedElements` during the underlay recursion just like `clip` is removed: add `prunedElements: undefined` to the options spread.</violation>
<violation number="5" location="packages/screenshot/src/index.ts:446">
P1: `lastCaptureTimings` is a module-level mutable shared object that is mutated at multiple checkpoints inside `captureNodeInternal` and `createCaptureResult`. Because `captureNodeInternal` recursively calls itself for backdrop underlay capture, and because `captureIframeContents` calls `captureNode` for every same-origin iframe, nested/inner captures overwrite the outer capture's timing fields. A consumer reading `lastCaptureTimings` after capture gets a mixed set (e.g., `snapshotMs`, `buildMs`, and `serializeMs` from the innermost nested call, and only `backdropMs` from the outer call). This global state also creates a race-condition risk for concurrent captures.
Instead of writing to a global singleton, pass a capture-scoped timings object through `captureNodeInternal` (and into `createCaptureResult` for `decodeMs`/`rasterMs`/`encodeMs`) so each top-level capture gets its own isolated telemetry.</violation>
<violation number="6" location="packages/screenshot/src/index.ts:474">
P2: `prefetchExternalResources` runs on the full subtree before `snapshotComposedTree` applies `filterNode` and `prunedElements`, so resources from excluded nodes are still fetched unnecessarily (and potentially surprisingly). Consider moving the prefetch after the snapshot so it can skip filtered/pruned elements, or passing the filter criteria into the prefetch function.</violation>
<violation number="7" location="packages/screenshot/src/index.ts:481">
P2: `captureRegion` now defaults `backgroundColor` to the document background, which overrides the conditional transparency-preserving logic that `captureNodeInternal` still applies for `captureNode`. On a typical page with a colored `<body>` background, a transparent region captured without an explicit `backgroundColor` will now render opaque instead of preserving transparency. Callers also cannot opt back into the old conditional behavior by omitting the option because the `??` fallback always intervenes.
Please confirm this behavioral change is intentional API-wide, and consider adding test coverage (or documentation) for transparent-region expectations so consumers aren't surprised when the default switches from transparent to opaque.</violation>
<violation number="8" location="packages/screenshot/src/index.ts:483">
P2: The reusable-capture cache key is built from `ResolvedCaptureOptions` before `backgroundColor` is resolved. When no explicit background is provided, the key encodes it as an empty string (`""`). Later, the actual background is inferred from computed root/inherited/document styles and stored alongside the cached result. On a subsequent capture with the same explicit options, the same `""`-based key matches, and `getReusableCapture` returns the prior `resolvedBackgroundColor` without verifying whether the implicit background has changed.
This can serve a stale background in cases where computed styles change without a DOM mutation that the epoch tracker observes — for example, a `@media (prefers-color-scheme)` system preference switch, or a CSS transition completing. The current invalidation checks (`documentEpoch`, CSS rule count, scroll/form state) do not capture implicit background dependencies, so the cached result may not reflect the current page.
Consider including the resolved background color in the cache key, or move the implicit background resolution before the cache lookup so the key is grounded in the actual rendered state.</violation>
<violation number="9" location="packages/screenshot/src/index.ts:604">
P2: On WebKit, the backdrop bake cache key can collide when `underlayClip` width or height differs but x and y stay the same. The cache key includes position but omits clip size, and `toSvgDataUrl()` on WebKit drops clip information because `serializeToSvgMarkup` passes `clip: null` there to avoid a viewBox/foreignObject bug. Two captures with identical markup and pane geometry but different underlay extents can reuse the wrong baked pane PNGs. Consider adding `${underlayClip?.width ?? 0},${underlayClip?.height ?? 0}` to the cache key.</violation>
</file>
<file name="packages/screenshot/src/utils/is-replaced-element.ts">
<violation number="1" location="packages/screenshot/src/utils/is-replaced-element.ts:3">
P2: Add unit tests for `isReplacedElement`. The project tests similar utilities like `isZeroScaleOverlay`, and this function drives size-freezing policy for inline replaced elements. Missing coverage means a regression in replaced-element detection could silently affect screenshot fidelity.</violation>
</file>
<file name="packages/screenshot/src/capture/freeze-fixed.ts">
<violation number="1" location="packages/screenshot/src/capture/freeze-fixed.ts:37">
P1: Fixed descendants can render at the wrong position when the capture root itself is the containing block trigger (e.g. scrolled root with wrapper, or transformed static root). The ancestor walk excludes `rootElement`, so root-specific containing-block and scroll-wrapper offsets are not applied; include root in the search so the existing containing-block branch handles it.</violation>
<violation number="2" location="packages/screenshot/src/capture/freeze-fixed.ts:85">
P1: The style concatenation here doesn't guarantee a semicolon separator between the existing inline style and the new declarations. If the element's inline style lacks a trailing semicolon (valid HTML/CSS), the result merges into a single invalid declaration — e.g., `color:redposition:absolute...` — which swallows the `position:absolute` entirely during CSS parsing. The frozen positioning (top, left, etc.) may then apply to the wrong layout context, causing screenshot fidelity regressions for fixed descendants that carry inline styles without a trailing semicolon.
Consider inserting a semicolon separator when an existing style is present and doesn't already end with one, or unconditionally add `;` before the new declarations since leading empty declarations are safely skipped by browsers.</violation>
</file>
<file name="packages/screenshot/bench/utils/score-png-pair.ts">
<violation number="1" location="packages/screenshot/bench/utils/score-png-pair.ts:9">
P3: Scoring logic now exists in two places, so future threshold/cropping/formula updates can diverge between bench and fidelity results. Consider extracting one shared scorer utility and calling it from both paths.</violation>
</file>
<file name="packages/screenshot/e2e/fixtures/hti-webp-image.html">
<violation number="1" location="packages/screenshot/e2e/fixtures/hti-webp-image.html:24">
P0: The asset `hti-image.webp` is not a valid WebP image — it is a plain text placeholder. The fixture `hti-webp-image.html` references it via `<img src="./assets/hti-image.webp" />` but the browser will fail to decode it, so the test captures an empty/broken-image slot instead of a rendered WebP. Replace the file with a genuine WebP image (e.g., convert an existing fixture PNG via `cwebp` or a simple canvas-to-blob script) so the fixture actually exercises WebP rendering in the screenshot pipeline.</violation>
</file>
<file name="packages/screenshot/src/utils/wait-for-animation-frames.ts">
<violation number="1" location="packages/screenshot/src/utils/wait-for-animation-frames.ts:13">
P2: The settle step can now complete without any animation frame when the timeout fires first, which changes double-rAF behavior in the rasterize pipeline. Consider keeping this helper strictly frame-driven (or moving non-frame fallback behavior out of this utility) so decode settling still matches the documented contract.</violation>
</file>
<file name="packages/screenshot/src/capture/embed-fonts.ts">
<violation number="1" location="packages/screenshot/src/capture/embed-fonts.ts:37">
P1: Failed font URL inlining currently falls back to the original remote URL, so timed-out/failed font fetches can still trigger later unbounded network fetches during SVG render instead of degrading deterministically. Using the same transparent-pixel fallback as other resource inlining keeps capture behavior consistent and bounded.</violation>
</file>
<file name="packages/screenshot/src/utils/parse-font-families.ts">
<violation number="1" location="packages/screenshot/src/utils/parse-font-families.ts:6">
P2: Quoted font names with internal commas are incorrectly split. A value like `"Chronicle Display, Italic", serif` would produce `["chronicle display", "italic"]` instead of `["chronicle display, italic"]`, causing font-face rule matching to miss the real font. Consider a parser that tracks quote state while splitting.</violation>
</file>
<file name="packages/screenshot/src/utils/fetch-as-data-url.ts">
<violation number="1" location="packages/screenshot/src/utils/fetch-as-data-url.ts:3">
P2: Capture cancellation cannot stop in-flight resource fetches early because this helper only listens to its internal timeout signal. Accepting an optional `abortSignal` and merging it with the timeout signal keeps resource inlining cancellable between pipeline stages.</violation>
</file>
<file name="packages/screenshot/src/utils/sanitize-svg-subtree.ts">
<violation number="1" location="packages/screenshot/src/utils/sanitize-svg-subtree.ts:7">
P1: Serialized SVG can still carry inline event-handler attributes (for example `onclick`) because plain-name attributes are accepted solely by XML-name shape. Filtering `on*` handler names during attribute validation would align this sanitizer with the no-dynamic-content requirement.</violation>
<violation number="2" location="packages/screenshot/src/utils/sanitize-svg-subtree.ts:21">
P1: Link-bearing attributes can retain `javascript:` payloads because value sanitization only removes invalid XML code points. Removing `href`/`xlink:href` attributes when they resolve to a `javascript:` scheme would enforce the documented security constraint.</violation>
</file>
<file name="packages/screenshot/bench/utils/compute-quantile.ts">
<violation number="1" location="packages/screenshot/bench/utils/compute-quantile.ts:1">
P2: Consider validating that quantile is in [0, 1] at the top of the function. An out-of-range value would compute invalid indices and silently produce incorrect results — the `?? 0` / `?? lowerValue` fallbacks mask the out-of-bounds access rather than surfacing the misuse.</violation>
</file>
<file name="packages/screenshot/e2e/fixtures/ms-max-height-important-override.html">
<violation number="1" location="packages/screenshot/e2e/fixtures/ms-max-height-important-override.html:31">
P3: The class `.min-height` sets `max-height`, not `min-height`. This makes the fixture harder to read at a glance — someone scanning the CSS or HTML to understand the test scenario has to reconcile a `min-height` class name with `max-height` property logic. Consider renaming the class (e.g., `.max-height-override`) to reflect the property it actually controls.</violation>
</file>
<file name="packages/screenshot/src/capture/pseudo-elements.ts">
<violation number="1" location="packages/screenshot/src/capture/pseudo-elements.ts:63">
P2: Some `::first-letter` styling can be dropped for elements that only get leading text from generated content. The new `!element.firstChild` guard exits before computing `::first-letter`, so those valid pseudo styles are never snapshotted; consider removing the guard or gating on actual first-letter applicability instead of DOM children.</violation>
</file>
<file name="packages/screenshot/src/capture/default-styles.ts">
<violation number="1" location="packages/screenshot/src/capture/default-styles.ts:12">
P2: Baseline dedup can become incorrect across captures because cached defaults are global to the module, not scoped to the current document context. Scoping the cache to each `createStyleSandbox` instance (or including document-mode context in the key) would avoid cross-document baseline reuse.</violation>
</file>
<file name="packages/screenshot/src/capture/freeze-sticky.ts">
<violation number="1" location="packages/screenshot/src/capture/freeze-sticky.ts:36">
P1: Frozen sticky offsets can be wrong when a sticky element sits under an ancestor that establishes an absolute containing block. The calculation uses scroll-container coordinates, but absolute top/left resolve against the nearest containing block, so this path likely needs containing-block resolution similar to `freezeFixedDescendants`.</violation>
<violation number="2" location="packages/screenshot/src/capture/freeze-sticky.ts:44">
P1: The inline style concatenation for pinned sticky clones can create invalid CSS when the existing style doesn't end with a semicolon, which can silently drop the `position:absolute` pinning and break the sticky freeze behavior in screenshots. Consider inserting a semicolon delimiter when there is an existing style value. For example, add a separator between the existing style and the new declarations so that `color:red` does not merge into `color:redposition:absolute`.</violation>
</file>
<file name="packages/screenshot/src/capture/margin-collapse.ts">
<violation number="1" location="packages/screenshot/src/capture/margin-collapse.ts:23">
P2: Escaped bottom margin can be over-applied when a parent has definite height or min-height. This helper currently treats those boxes as collapsible, so matching parent/child bottoms can incorrectly emit extra `margin-bottom` in the clone.</violation>
</file>
<file name="packages/screenshot/src/utils/parse-scale-linear.ts">
<violation number="1" location="packages/screenshot/src/utils/parse-scale-linear.ts:8">
P1: Percentage values (e.g., `scale: 50%`) produce scaling factors 100x too large because `Number.parseFloat("50%")` returns `50`, not `0.5`. Add a `%` suffix check that divides by 100, or strip `%` and divide before returning.</violation>
</file>
<file name="packages/screenshot/src/utils/visit-document-css-rules.ts">
<violation number="1" location="packages/screenshot/src/utils/visit-document-css-rules.ts:31">
P2: Imported stylesheet rules can be processed twice, which inflates scan cost and can duplicate collected CSS (like repeated `@font-face` blocks). The top-level loop includes all `document.styleSheets` while `@import` traversal already descends into those same sheets; filtering top-level sheets to `!ownerRule` avoids double traversal.</violation>
</file>
<file name="packages/screenshot/src/utils/compute-filter-extent.ts">
<violation number="1" location="packages/screenshot/src/utils/compute-filter-extent.ts:8">
P2: The regex-based filter parser only handles one level of nested parentheses, which makes it fragile for nested CSS functional syntax. If a computed filter value contains nested functions (for example, certain color functions or browser-specific normalization), the `blur`/`drop-shadow` branch may silently fail to match and the resulting bleed `extent` will be too small, causing clipped output in the final screenshot. Consider replacing the regex with a character-level tokenizer that tracks parenthesis depth, or add defensive test coverage for computed filter values with nested parentheses.</violation>
</file>
<file name="packages/screenshot/src/capture/bake-backdrop-filters.ts">
<violation number="1" location="packages/screenshot/src/capture/bake-backdrop-filters.ts:10">
P2: `collectBackdropFilterElements` explicitly skips the root element (`element === rootElement`), so when `captureNode` is called directly on an element that carries `backdrop-filter`, the property is never baked and is still emitted in the serialized styles. Since SVG `foreignObject` cannot evaluate `backdrop-filter`, the frosted effect will be absent in the output. At minimum the property should be stripped from the root's emitted styles even when baking isn't feasible (the backdrop content lives outside the capture root). Consider removing the early `continue` and handling the root alongside other backdrop elements, or explicitly deleting `backdrop-filter` from the root's diffed styles when it can't be baked.</violation>
<violation number="2" location="packages/screenshot/src/capture/bake-backdrop-filters.ts:18">
P2: `computeBackdropUnderlayClip` can return invalid `Infinity` coordinates when `backdropFilterElements` is empty, and it does not fully clamp the clip rectangle to the root bounds when panes lie outside the root box. When the input array is empty, `clippedLeft` and `clippedTop` evaluate to `Infinity` because the infinities are never replaced by real pane coordinates; downstream canvas APIs and cache keys then receive `x: Infinity, y: Infinity`. Even with a non-empty array, `clippedLeft`/`clippedTop` are only clamped at `0` and not at the root width/height, so a pane fully outside the root to the right or bottom produces a 1×1 rect positioned beyond the canvas. A robust fix is to return a zero-area rectangle (or `undefined`) when `left` is still `Infinity`, and to clamp `clippedLeft`/`clippedTop` to not exceed `clippedRight`/`clippedBottom`.</violation>
<violation number="3" location="packages/screenshot/src/capture/bake-backdrop-filters.ts:57">
P1: `composeChainLinearTransform` traverses ancestors using `currentElement.parentElement`, which becomes `null` at Shadow DOM boundaries and stops before reaching a transformed host. This omits host/ancestor transforms from `chainLinear`, so `mapFilteredRegionIntoLayoutBox` computes an incorrect inverse and `compositeBakedPaneOntoUnderlay` reapplies the backdrop in the wrong place. Use `getRenderedParentElement` (already in `packages/screenshot/src/utils/get-rendered-parent-element.ts`) or `snapshot.parentElement` from the composed-tree snapshot to cross shadow boundaries instead.</violation>
<violation number="4" location="packages/screenshot/src/capture/bake-backdrop-filters.ts:82">
P2: Rounding `elementRect.width` and `elementRect.height` to integers before passing them into `renderFilteredBackdropRegion` can introduce a 1-screen-pixel off-by-one in canvas dimensions for fractional layout sizes. Because `pixelRatio` is applied and rounded *after* the initial `Math.round`, the path `Math.round(Math.round(cssPx) * pixelRatio)` diverges from `Math.round(cssPx * pixelRatio)` by up to ~`pixelRatio` pixels (e.g., `100.6` at DPR 2 yields `202` instead of `201`). Since the position offsets are not rounded, the source-draw region and destination canvas size are misaligned, which can clip or expand the baked backdrop-filter underlay by a visible pixel when composited against the element. Consider passing the raw fractional CSS dimensions to `renderFilteredBackdropRegion` and rounding only the final screen-pixel canvas size (`Math.round(cssPx * pixelRatio)`) so the underlay footprint matches the element’s exact subpixel bounds.</violation>
</file>
<file name="packages/screenshot/src/utils/apply-baked-backdrop-background.ts">
<violation number="1" location="packages/screenshot/src/utils/apply-baked-backdrop-background.ts:34">
P1: The baked backdrop cleanup removes only `backdrop-filter`, but not the vendor-prefixed `-webkit-backdrop-filter`. Because the screenshot pipeline captures and stores vendor-prefixed properties (e.g., `-webkit-text-fill-color`, `-webkit-user-select`), a `-webkit-backdrop-filter` value can remain in the cloned element's styles after baking. On WebKit browsers the clone would then apply a live backdrop filter on top of the already-baked PNG background layer, producing a double-filtered output. Consider also deleting `diffed["-webkit-backdrop-filter"]` alongside `backdrop-filter`.</violation>
</file>
<file name="packages/screenshot/src/utils/find-document-background-color.ts">
<violation number="1" location="packages/screenshot/src/utils/find-document-background-color.ts:13">
P2: `findDocumentBackgroundColor` can misclassify transparent document backgrounds as opaque, causing the screenshot pipeline to pick the wrong background color. `getComputedStyle(...).backgroundColor` may return fully-transparent values like `rgba(255, 255, 255, 0)` or the keyword `"transparent"`, but the current predicate only excludes the exact constant `"rgba(0, 0, 0, 0)"`. When a root background is one of those other transparent forms, `.find(...)` short-circuits on it instead of falling through to a visible body background, which hurts screenshot fidelity. Consider also rejecting `"transparent"` and any `rgba(..., 0)` color, or parse the alpha channel to treat all alpha-0 values as transparent.</violation>
</file>
<file name="packages/screenshot/src/capture/iframe-bridge.ts">
<violation number="1" location="packages/screenshot/src/capture/iframe-bridge.ts:60">
P2: The bridge uses a single global `isBridgeCaptureInFlight` flag that silently drops all incoming requests while any capture is in progress. This prevents mutual-embed recursion as intended, but it also suppresses legitimate concurrent bridge requests to the same iframe document. When multiple `captureNode` calls run in parallel and target the same cross-origin iframe, only the first bridge request is honored; the rest time out and fall back to flat placeholder output, causing nondeterministic fidelity regressions. Consider using a per-request lock keyed by `event.source` (or a request queue) so that unrelated concurrent requests are not dropped.</violation>
<violation number="2" location="packages/screenshot/src/capture/iframe-bridge.ts:77">
P1: The iframe bridge message handler responds to capture requests from any origin without validating `event.origin`, and it replies with `targetOrigin: "*"`. Once `enableIframeBridge()` is active, any window that can `postMessage` to the page can request and receive a rendered PNG snapshot of the document. Consider adding an `allowedOrigins` option to `enableIframeBridge` so callers can restrict which origins are permitted to request captures, and validate `event.origin` against that list before invoking `captureDocumentRoot`.</violation>
</file>
<file name="packages/screenshot/src/utils/invert-linear-transform.ts">
<violation number="1" location="packages/screenshot/src/utils/invert-linear-transform.ts:5">
P2: The determinant guard uses an exact `=== 0` comparison, which is fragile for floating-point values parsed from CSS transforms. A near-zero determinant can produce extremely large inverse coefficients that cascade into unstable canvas transforms or oversized rasterization during backdrop-filter baking. The codebase already uses `LINEAR_TRANSFORM_IDENTITY_EPSILON` (0.0001) for identity checks and `Number.isFinite()` in other transform parsers, so adding both guards here is consistent and safer. The caller already skips the element when `null` is returned, so tightening this condition won't break downstream logic.</violation>
</file>
<file name="packages/screenshot/src/utils/is-css-keyframes-rule.ts">
<violation number="1" location="packages/screenshot/src/utils/is-css-keyframes-rule.ts:2">
P1: This type guard uses `instanceof CSSKeyframesRule`, which breaks the package's own convention for CSS rule detection and creates cross-realm/iframe correctness issues. The adjacent `isCssStyleRule`, `isCssImportRule`, and `isCssGroupingRule` intentionally avoid `instanceof` because CSS rules from iframes have constructors from the child window's realm, so the parent window's `instanceof` returns `false`. Since this package supports iframes via `enableIframeBridge`, keyframe rules from framed documents would be misidentified here. Additionally, calling this helper in environments where `CSSKeyframesRule` is undeclared (SSR, Node tests) throws a `ReferenceError`. Use the same `rule.type` numeric check as the other guards: `rule.type === CSSRule.KEYFRAMES_RULE`.</violation>
</file>
<file name="packages/screenshot/bench/utils/capture-cpu-profile.ts">
<violation number="1" location="packages/screenshot/bench/utils/capture-cpu-profile.ts:22">
P2: `raceDeadline` creates a `setTimeout` for every call but never clears it when the work promise settles early. Even though `Promise.race` resolves, the timer remains scheduled until `deadlineMs` elapses, causing unnecessary event-loop churn during benchmark runs. Clear the timeout when `work` settles.</violation>
</file>
<file name="packages/screenshot/src/utils/waapi-keyframe-prop-to-css-prop.ts">
<violation number="1" location="packages/screenshot/src/utils/waapi-keyframe-prop-to-css-prop.ts:4">
P2: CSS custom properties (`--*`) are case-sensitive, and when they contain uppercase letters they are corrupted by the generic camelCase→kebab-case conversion. For example, `--MyColor` becomes `---my-color`, which is a different (and likely non-existent) property. Since WAAPI `getKeyframes()` returns custom properties with their literal `--` prefix, they should bypass the regex transformation entirely. Consider adding an early-return guard for properties starting with `--`.</violation>
</file>
<file name="packages/screenshot/scripts/analyze-perf-trace.mjs">
<violation number="1" location="packages/screenshot/scripts/analyze-perf-trace.mjs:26">
P2: The `--top` CLI argument is parsed with `Number()` but never validated, which means `--top=abc` silently becomes `NaN`. When `NaN` flows into `slice(0, topCount)`, it returns an empty array, producing a report with headers but zero hotspot rows — confusing output instead of a clear error. Also, `--top=0` or `--top=-5` would clip data unexpectedly. Consider validating `topCount` with `Number.isFinite()` and ensuring it's a positive integer, exiting with a helpful error message for bad input.</violation>
<violation number="2" location="packages/screenshot/scripts/analyze-perf-trace.mjs:30">
P2: The `LIBRARY_URL_PATTERN` regex only matches `/src/(capture|utils)/`, which excludes the root-level source files `src/index.ts`, `src/constants.ts`, and `src/types.ts` from library-frame detection when their URLs don't also contain `react-grab`. Broadening the pattern to cover the entire `src/` directory would make hotspot reports more complete and accurate.</violation>
<violation number="3" location="packages/screenshot/scripts/analyze-perf-trace.mjs:44">
P2: The trace profile aggregation in `collectProfilesFromTraceEvents` groups Profile and ProfileChunk events only by `traceEvent.id`, defaulting missing IDs to `"default"`. Chrome trace events can carry CPU profiles from multiple processes and threads; the official Chromium DevTools handler explicitly keys profiles by both `pid` and `id` (nested map) to prevent cross-process merging. Without `pid` in the key, unrelated profiles can be merged if IDs collide or are absent, corrupting self-time attribution and producing misleading hotspot data. Consider including `pid` (and optionally `tid`) in the profile key.</violation>
</file>
<file name="packages/screenshot/src/capture/relevant-style-props.ts">
<violation number="1" location="packages/screenshot/src/capture/relevant-style-props.ts:38">
P2: The `#` check for adding `"id"` to `styleRelevantAttributeNames` is over-broad: it also fires when `#` appears inside attribute selector values like `[href="#section"]` or `[data-color="#fff"]`. Because `id` is not in the always-relevant list, this can cause elements differing only by `id` to be excluded from memo sharing even when no CSS actually targets them by `id`. Removing `[...]` blocks first before testing for `#` would eliminate the most common false positives without affecting genuine ID selectors.</violation>
<violation number="2" location="packages/screenshot/src/capture/relevant-style-props.ts:41">
P2: `instanceof` checks against `CSSKeyframeRule` and `KeyframeEffect` are not guarded for undefined constructors, which will throw a `ReferenceError` in browsers that lack these globals. The file already guards `getAnimations` with a `typeof` check, so add similar guards here (e.g., `typeof CSSKeyframeRule !== 'undefined' && ... instanceof CSSKeyframeRule` and `typeof KeyframeEffect !== 'undefined' && ... instanceof KeyframeEffect`) to avoid aborting style-registry creation in partial-support environments.</violation>
<violation number="3" location="packages/screenshot/src/capture/relevant-style-props.ts:64">
P1: The `transition-property` instability detection uses `includes()` against `CLASS_STABLE_CANDIDATE_STYLE_PROPS`, which only contains longhand properties (e.g. `margin-top`). When CSS declares `transition-property: margin` (or `padding`, or `inset`), the transition affects all corresponding longhands but the string check never matches the shorthand name, so those longhands remain incorrectly treated as class-stable. This skips per-element computed-style re-reads during an active transition and can produce incorrect captured layout. Consider expanding shorthand transition properties to their longhands before checking stability (e.g. when `transitionedValue` contains `"margin"`, mark all `margin-*` candidates unstable).</violation>
<violation number="4" location="packages/screenshot/src/capture/relevant-style-props.ts:79">
P1: The `checkDeclaredValueStability` function handles longhands (`margin-top`) and prefix-matched properties (`inset-block-start`), but the bare shorthands `margin` and `padding` fall through all branches and never taint their related longhands. For inline `CSSStyleDeclaration`s, CSSOM iteration often enumerates the shorthand (e.g., `margin`) without expanding it to longhands, so an unstable value like `auto` or `10%` on the shorthand goes undetected. Adding explicit branches for `margin` and `padding` similar to the existing `inset` prefix handling would close the gap.</violation>
<violation number="5" location="packages/screenshot/src/capture/relevant-style-props.ts:93">
P2: The `addShadowRootStyleProps` function is meant to degrade gracefully by returning `false` when a shadow-root stylesheet can't be inspected, but the spread `...shadowRoot.adoptedStyleSheets` in the `for...of` header is evaluated outside the `try/catch`. If `adoptedStyleSheets` is missing — which occurs in older browsers, partial implementations, and environments like jsdom — the spread throws `TypeError: undefined is not iterable` before the fallback logic can run, causing capture to hard-fail instead of falling back. Consider guarding it with `...(shadowRoot.adoptedStyleSheets ?? [])`.</violation>
<violation number="6" location="packages/screenshot/src/capture/relevant-style-props.ts:272">
P2: The fast-path `addParsedInlineDeclaration` route uses case-sensitive string and regex checks against raw `declaredValue` strings from `parseInlineStyleDeclarations`, which preserves author casing. CSS values like `ATTR(...)`, `COUNTER(...)`, and `CALC(...)` are valid per spec but will not match the current `includes("attr(")`, `includes("counter")`, `BOX_RELATIVE_VALUE_PATTERN`, or `STABLE_DECLARED_VALUE_PATTERN` checks, allowing unsafe styles to be incorrectly treated as memo-safe. The existing CSSOM path normalizes values via `getPropertyValue`; the parsed path should do the same for value-based stability checks.</violation>
</file>
<file name="packages/screenshot/src/capture/rasterize.ts">
<violation number="1" location="packages/screenshot/src/capture/rasterize.ts:24">
P1: The nested cache structure introduces an unbounded inner map for PNG param variants. `createFifoCache` only evicts on insertion of a new outer key; when the same `svgMarkup` already exists in the outer cache, updating it does not trigger eviction, and the inner plain `Map` has no capacity limit. Capturing the same DOM repeatedly with different `scale`, `pixelRatio`, `clipRect`, or `backgroundColor` values will continuously grow memory without bound, degrading the intended `RASTER_PNG_CACHE_CAP` semantics. Consider bounding the inner map (e.g., with its own FIFO eviction or by tracking total entries across all inner maps) or restoring a single-tier key so the cap limits total cached variants.</violation>
<violation number="2" location="packages/screenshot/src/capture/rasterize.ts:81">
P2: The `toBlob` cache key is built by pipe-concatenating string fields, including `backgroundColor` and `svgMarkup`, both of which can contain the `|` delimiter. This creates a real collision risk where two different captures could map to the same key and return the wrong cached PNG blob. Consider using `JSON.stringify([…])` or another delimiter-safe encoding instead of raw pipe concatenation.</violation>
<violation number="3" location="packages/screenshot/src/capture/rasterize.ts:131">
P2: An empty string passed as `backgroundColor` bypasses the JPEG fallback background and produces black output. `options.backgroundColor ?? fallbackBackgroundColor` preserves empty strings (not nullish), but the `if (backgroundColor)` gate treats them as "no color" and skips the fill. For JPEG captures, this silently reintroduces the black-background bug the fallback is meant to prevent. Changing `??` to `||` would consistently treat empty strings as "unspecified" so the JPEG fallback to `JPEG_FALLBACK_BACKGROUND_COLOR` applies correctly.</violation>
</file>
<file name="packages/screenshot/scripts/generate-site-fixtures.mjs">
<violation number="1" location="packages/screenshot/scripts/generate-site-fixtures.mjs:1471">
P2: The fixture ID uses a running counter derived from `Object.entries(archetypes)` iteration order: `site-${String(fixtureIndex).padStart(2, "0")}-${archetypeName}-${themeName}`. Because these IDs are persisted as filenames and hardcoded in `fixture-manifest.ts` with per-engine budgets, inserting or reordering an archetype renumbers every subsequent fixture and breaks manifest references. Use a stable key such as `site-${archetypeName}-${themeName}` so fixture identity survives reordering.</violation>
</file>
<file name="packages/screenshot/e2e/fixtures/site-calendar-week-slate.html">
<violation number="1" location="packages/screenshot/e2e/fixtures/site-calendar-week-slate.html:1">
P2: The calendar event grid-row spans are systematically too large. For example,</violation>
</file>
<file name="packages/screenshot/README.md">
<violation number="1" location="packages/screenshot/README.md:33">
P2: The README adds the `captureRegion` ESM example, but the adjacent IIFE sentence still claims the build only exposes `window.FastHtmlToImage.captureNode`. The `vite.config.ts` bundles all named exports (including `captureRegion`) into the `FastHtmlToImage` global, and the e2e suite already calls `window.FastHtmlToImage.captureRegion(...)`. Updating the IIFE line to mention both entry points keeps the API contract documentation consistent.</violation>
</file>
<file name="packages/screenshot/src/capture/serialize-svg.ts">
<violation number="1" location="packages/screenshot/src/capture/serialize-svg.ts:24">
P2: When a `clip` rectangle is provided, its dimensions are used directly as the SVG root `width`/`height` and `viewBox`. There is no validation that `clip.width` and `clip.height` are positive, finite numbers. A caller (or an upstream computation) can pass `0`, negative values, or `NaN`, which produces an invalid SVG and can result in a blank rasterized image. Consider adding an explicit guard or validation before emitting the SVG so that non-finite or non-positive clip dimensions do not silently corrupt output.</violation>
</file>
<file name="packages/screenshot/src/capture/snapshot-styles.ts">
<violation number="1" location="packages/screenshot/src/capture/snapshot-styles.ts:57">
P2: The ancestor pre-scan introduced to capture inherited inline style properties from ancestors outside the capture root uses `isHtmlElement(ancestorElement)` before calling `relevantProps.addInlineStyleProps(ancestorElement.style)`. This excludes non-HTML ancestors (e.g., SVG elements containing `<foreignObject>` with HTML descendants) that also support inline `style` attributes and can cascade inherited properties into the capture root.
Because `snapshotComputedStyle` only reads properties listed in `relevantPropertyNames` for HTML elements, missing these ancestor-declared property names means inherited values (like `font-family`, `color`, `font-size`) won't be captured in the snapshot, leading to incorrect rendering in mixed-namespace DOM trees.
Since `addInlineStyleProps` accepts a generic `CSSStyleDeclaration` with no HTML-specific dependency, the `isHtmlElement` guard should be removed or broadened for the ancestor traversal.</violation>
<violation number="2" location="packages/screenshot/src/capture/snapshot-styles.ts:126">
P1: The document-scoped persistent memo cache does not distinguish between different capture roots in the same document, so inherited computed styles from outside-root ancestors can leak across captures. Consider including the `rootElement` identity or its descriptor in `memoStoreSignature` (or scoping `persistentMemoStoreByDocument` per root) so that captures of different subtrees within the same document do not reuse each other's memoized styles.</violation>
</file>
<file name="packages/screenshot/src/utils/build-style-memo-descriptor.ts">
</file>
<file name="packages/screenshot/src/capture/prefetch-resources.ts">
</file>
<file name="packages/screenshot/src/utils/strip-invalid-xml-characters.ts">
</file>
<file name="packages/screenshot/src/capture/document-change-tracker.ts">
</file>
<file name="packages/screenshot/src/capture/capture-reuse.ts">
</file>
<file name="packages/screenshot/scripts/opt-loop.local.mjs">
</file>
<file name="packages/screenshot/src/utils/build-inline-style-scan.ts">
</file>
<file name="packages/screenshot/src/utils/build-parsed-inline-style-scan.ts">
</file>
<note>Content truncated. 21 files omitted due to size limits.</note>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| renderingContext.setTransform(1, 0, 0, 1, 0, 0); | ||
| renderingContext.clearRect(0, 0, canvasWidth, canvasHeight); | ||
| } | ||
| const backgroundColor = options.backgroundColor ?? fallbackBackgroundColor; |
Contributor
There was a problem hiding this comment.
P2: An empty string passed as backgroundColor bypasses the JPEG fallback background and produces black output. options.backgroundColor ?? fallbackBackgroundColor preserves empty strings (not nullish), but the if (backgroundColor) gate treats them as "no color" and skips the fill. For JPEG captures, this silently reintroduces the black-background bug the fallback is meant to prevent. Changing ?? to || would consistently treat empty strings as "unspecified" so the JPEG fallback to JPEG_FALLBACK_BACKGROUND_COLOR applies correctly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/screenshot/src/capture/rasterize.ts, line 131:
<comment>An empty string passed as `backgroundColor` bypasses the JPEG fallback background and produces black output. `options.backgroundColor ?? fallbackBackgroundColor` preserves empty strings (not nullish), but the `if (backgroundColor)` gate treats them as "no color" and skips the fill. For JPEG captures, this silently reintroduces the black-background bug the fallback is meant to prevent. Changing `??` to `||` would consistently treat empty strings as "unspecified" so the JPEG fallback to `JPEG_FALLBACK_BACKGROUND_COLOR` applies correctly.</comment>
<file context>
@@ -122,8 +128,9 @@ export const createCaptureResult = (
}
- if (options.backgroundColor) {
- renderingContext.fillStyle = options.backgroundColor;
+ const backgroundColor = options.backgroundColor ?? fallbackBackgroundColor;
+ if (backgroundColor) {
+ renderingContext.fillStyle = backgroundColor;
</file context>
Suggested change
| const backgroundColor = options.backgroundColor ?? fallbackBackgroundColor; | |
| const backgroundColor = options.backgroundColor || fallbackBackgroundColor; |
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Contributor
There was a problem hiding this comment.
Additional Suggestions:
- An empty-string
backgroundColorbypasses the JPEG white fallback because??keeps"", producing a black JPEG instead of the intended opaque white.
- An empty-string
backgroundColorbypasses the JPEG opaque-white fallback, producing a black JPEG instead of a white one.
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
…lect Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
…Element probe, subtree assessment) Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
…de text Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
| didCopy = copyContent(finalContent, { | ||
| componentName: options.componentName, | ||
| entries, | ||
| }); |
There was a problem hiding this comment.
Empty metadata on custom getContent
Medium Severity
When getContent is customized, getMetadataEntries returns undefined, but copyContentWithScreenshot passes entries ?? [] into the clipboard metadata and screenshot notes. The execCommand fallback still calls copyContent, which synthesizes a default entry when entries is missing, so the async screenshot path can write entries: [] and a note bar with only the header while the text fallback would include full metadata.
Reviewed by Cursor Bugbot for commit 4da26f3. Configure here.
…rag-selection e2e Co-Authored-By: Aiden Bai <aiden.bai05@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ce8c2c0. Configure here.
| return captureNode(elements[0], { bleed: "auto", filterNode }); | ||
| } | ||
| const region = combineBounds(elements.map((element) => element.getBoundingClientRect())); | ||
| return captureRegion(region, { filterNode }); |
There was a problem hiding this comment.
Multi-select screenshots omit bleed padding
Low Severity
Annotated screenshots use captureNode with bleed: "auto" for a single element, but multi-element copies use captureRegion on the combined bounds with no bleed expansion. Box shadows, outlines, and blur that extend outside the union rect can be clipped in drag/multi-select copies while single-click copies include them.
Reviewed by Cursor Bugbot for commit ce8c2c0. Configure here.
Comment on lines
+125
to
+133
| if (options.screenshot !== false) { | ||
| didCopy = await copyContentWithScreenshot(finalContent, entries ?? [], () => | ||
| renderAnnotatedScreenshot(elements, buildAgentNoteLines(entries ?? [])), | ||
| ); | ||
| } | ||
| if (!didCopy) { | ||
| didCopy = copyContent(finalContent, { | ||
| componentName: options.componentName, | ||
| entries, |
Contributor
There was a problem hiding this comment.
Suggested change
| if (options.screenshot !== false) { | |
| didCopy = await copyContentWithScreenshot(finalContent, entries ?? [], () => | |
| renderAnnotatedScreenshot(elements, buildAgentNoteLines(entries ?? [])), | |
| ); | |
| } | |
| if (!didCopy) { | |
| didCopy = copyContent(finalContent, { | |
| componentName: options.componentName, | |
| entries, | |
| // When getContent is customized there are no per-element entries, so mirror | |
| // copyContent's default-entry synthesis here. This keeps the custom | |
| // clipboard metadata populated identically regardless of whether the async | |
| // screenshot path or the text-only fallback lands the write. | |
| const resolvedEntries: ReactGrabEntry[] = entries ?? [ | |
| { | |
| componentName: options.componentName ?? "div", | |
| content: finalContent, | |
| }, | |
| ]; | |
| if (options.screenshot !== false) { | |
| didCopy = await copyContentWithScreenshot(finalContent, resolvedEntries, () => | |
| renderAnnotatedScreenshot(elements, buildAgentNoteLines(resolvedEntries)), | |
| ); | |
| } | |
| if (!didCopy) { | |
| didCopy = copyContent(finalContent, { | |
| componentName: options.componentName, | |
| entries: resolvedEntries, |
Custom getContent copies write empty entries: [] in the custom clipboard metadata via the screenshot path, diverging from the text-only fallback which synthesizes a default entry — so consumers get inconsistent metadata for the same grab.
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
1 participant
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.
Summary
fast-html-to-image(formerly@react-grab/screenshot): the fastest, most accurate in-browser DOM screenshot library, proven by a three-engine fidelity harness and a head-to-head benchmark against snapdom, modern-screenshot, html-to-image, html2canvas, and dom-to-image-more.Package
fast-html-to-image(globalFastHtmlToImage);npm packverified: 53.9 kB tarball, 8 files.Fidelity suite — 206 fixtures × Chromium/WebKit/Firefox, all green
site-01…site-50) generated byscripts/generate-site-fixtures.mjs— 25 archetypes (GitHub-style repo view, social feed, video grid, product page, pricing, wiki, link aggregator, comment thread, inbox, team chat, kanban, music player, streaming rows, search results, blog, profile, listing grid, checkout, analytics dashboard, docs site, calendar, Q&A thread, newspaper front, settings, landing hero) each in light + dark themes, self-contained with bundled Inter webfonts.0.005ratchet on Chromium and WebKit. On Firefox, 30 text-dense pages carry the pre-existing documentedfirefoxTextMetricsOverride(Gecko rasterizes foreignObject text with its own line-height rounding/glyph AA).#targetis taller than the viewport, Playwright's capture-beyond-viewport screenshot renders hinted system-font (monospace/serif) glyphs with different AA than in-viewport rendering. Fixture heights are capped at 700px so native ground truth stays in-viewport.Performance (chromium bench, median)
Bench now covers ten fixtures (six synthetic + the four heaviest real-website pages: video grid, kanban, analytics dashboard, newspaper front).
Cumulative perf work (earlier rounds in this PR): author-CSS-scoped computed-style reads, computed-style + diff-map memoization across memo-identical elements (animation-aware), minimal-escape SVG data URLs,
Uint8Array.toBase64, parallel font-embed fetches, raster caching — 70-stress warm 964ms → 86ms.Verification
Link to Devin session: https://app.devin.ai/sessions/40a61026a4354472b6a55982d708879e
Requested by: @aidenybai
Note
Medium Risk
Large new capture/clipboard surface in the default copy path; failures degrade to text-only but bundle size and browser clipboard quirks affect all users.
Overview
Introduces
fast-html-to-imageunderpackages/screenshot— a browser DOM-to-PNG library (captureNode/captureRegion, foreignObject pipeline) with architecture docs, a 400+ fixture Playwright fidelity suite (Chromium/WebKit/Firefox), and a benchmark against snapdom, modern-screenshot, html-to-image, html2canvas, and dom-to-image-more.react-grab now depends on it (bundled in Vite). Default copy tries
copyContentWithScreenshot: plain/HTML text,image/pngof the selection (with a source-file note bar), andweb application/x-react-grabJSON metadata, with stepped fallbacks when the async Clipboard API rejects parts. Newscreenshotoption (defaulttrue) and a Screenshot toolbar plugin; overlay UI is filtered out of captures.E2e fixtures gain
waitForClipboardContent, clipboard clearing between tests, and assertions on multi-type clipboard items. Perf CI runspnpm buildbefore tests so workspace package dists exist.Reviewed by Cursor Bugbot for commit ce8c2c0. Bugbot is set up for automated code reviews on this repo. Configure here.