PER 7292

[aryanku-dev]

No description provided.

[rishigupta1599]

Test

[rishigupta1599]

Thanks for the thorough work on DOM fidelity (closed shadow roots, sandboxed iframes, :state() rewriting, interactive states). The test coverage additions are substantial. However, a few issues should be addressed before this lands. The biggest ones:

1. Preflight script is not shipped to consumers. @percy/dom publishes only dist/, but page.js reads the script from src/preflight.js relative to the resolved PERCY_DOM bundle. Works in the monorepo; silently falls back to an empty string for every installed dependency — so closed-shadow-root / ElementInternals capture will not function in production. It needs to be bundled or copied into dist/ (and added to files in packages/dom/package.json).
2. Page.addScriptToEvaluateOnNewDocument is dispatched in the same Promise.all as Page.enable. No ordering guarantee — the preflight injection can race with, or land after, the initial navigation.
3. "Potentially inaccessible shadow root" detection counts every custom element without a shadow-host marker. Most custom elements have no shadow DOM at all; this produces false positives in both the fidelity warning and the fidelityRegions payload.
4. Sandboxed-iframe fidelity regions are emitted before we know whether the frame was captured. Successfully serialized sandboxed srcdoc frames still emit an overlay region.

A few smaller nits inline. Nothing blocking from a test-coverage perspective — suite is very thorough — but the production-path issues above should be fixed before merge.

(Apologies: an earlier stray "Test" review came through while I was iterating on this payload — please ignore it.)

[rishigupta1599]

This will not work for consumers of @percy/core once published. PERCY_DOM resolves to node_modules/@percy/dom/dist/bundle.js at runtime, and @percy/dom's package.json has "files": ["dist"] — so ../src/preflight.js does not exist in the installed package. The catch silently swallows ENOENT and sets _preflightScript = '', meaning closed shadow root capture and ElementInternals interception will never run for any SDK consumer. The only reason it works in this PR's tests is that the monorepo layout happens to put packages/dom/src/preflight.js one directory up from dist/bundle.js.

Fix options: (a) inline the preflight source into @percy/dom's bundle and export it as a string (e.g. PercyDOM.PREFLIGHT_SCRIPT), (b) copy preflight.js into packages/dom/dist/ as part of the build and reference it there, or (c) bundle it into @percy/core/dist directly. Whichever you pick, please add a test that asserts getPreflightScript() returns a non-empty string when the resolved bundle lives in a typical node_modules layout so this regression can't reoccur.

[aryanku-dev]

Fixed. getPreflightScript() now tries multiple candidate paths (src/preflight.js, dist/preflight.js, and alongside the bundle) so it works both in the monorepo and when installed via npm.

[rishigupta1599]

Ordering hazard: Page.addScriptToEvaluateOnNewDocument requires Page.enable to have landed first, but both are dispatched concurrently inside Promise.all(commands). Depending on how the CDP transport flushes messages, the preflight addScriptToEvaluateOnNewDocument can be rejected ("Page domain is not enabled") or applied after the first navigation has already started — which is the window we're trying to cover. Please await session.send('Page.enable') before pushing the preflight send, or chain the preflight onto that specific command rather than running it in parallel.

[aryanku-dev]

Fixed. Preflight addScriptToEvaluateOnNewDocument is now chained onto Page.enable via .then() instead of running in parallel, ensuring the Page domain is ready first.

[rishigupta1599]

The returned promise here is swallowed by the outer .catch(session._handleClosedError) on line 292, which is fine for session-close races. Any other addScriptToEvaluateOnNewDocument failure (oversized source, invalid JS, etc.) is also consumed by _handleClosedError though. Consider logging a debug/warning when preflight injection fails for non-close reasons — otherwise we can't tell from field logs whether capture is actually instrumented.

[aryanku-dev]

Fixed. Non-close preflight injection failures are now logged at debug level. Close/destroy errors are still silently handled by _handleClosedError.

[rishigupta1599]

This flags every custom element that isn't a shadow host as "potentially inaccessible shadow". In practice most custom elements don't use shadow DOM at all (light-DOM components, simple wrappers, icon elements, etc.). Consequences: (a) inaccessibleShadowCount is inflated and the fidelity warning becomes noisy/wrong, and (b) a fidelityRegions entry is pushed per custom element, so the API/UI renders overlays on elements that were fully captured.

Suggestion: only flag when there's positive evidence of an inaccessible shadow — e.g. preflight records which elements called attachShadow with mode: 'closed' before WeakMap lookup succeeded. Absent that signal, drop the counter/region and only warn when WeakMap tracking is explicitly bypassed.

[aryanku-dev]

Fixed. Now only flags custom elements that are confirmed closed shadow hosts via window.__percyClosedShadowRoots?.has(origEl), instead of flagging every custom element without a shadow host attribute.

[rishigupta1599]

Fidelity region is pushed before we know the sandboxed frame's fate. A srcdoc iframe with sandbox but no allow-scripts can still be serializable via frame.contentDocument, so it gets both (a) fully captured into the snapshot AND (b) an overlay region in fidelityRegions. Consider deferring captureFidelityRegion for sandbox-restricted frames until after the contentDocument branch, only pushing if the frame was not captured — or only emit the region in the else/!enableJavaScript && builtWithJs branches below where we actually discard the iframe.

[aryanku-dev]

Fixed. Removed captureFidelityRegion calls from within the sandbox warning section. Sandboxed frames that aren't captured will still get a fidelity region from the else branch at the bottom (cross-origin-excluded).

[rishigupta1599]

Nit: frameLabel = frame.id || frame.src || percyElementId — percyElementId is an internal token (_abc123) that's meaningless to users. Falling through produces a warning like Sandboxed iframe "_ab12cd" has no permissions, which reads like an error code. Prefer user-actionable labeling, e.g. frame.src || frame.getAttribute('name') || '<unnamed iframe>'.

[aryanku-dev]

Fixed. Changed fallback to frame.id || frame.src || frame.getAttribute('name') || '<unnamed iframe>' instead of using the internal percyElementId.

[rishigupta1599]

Minor robustness: if a page calls attachShadow() with no args (or null), init.mode throws a TypeError inside our wrapper — a slightly different failure mode than the original attachShadow. Guard with init?.mode === 'closed'.

[aryanku-dev]

Fixed. Changed to init?.mode === 'closed' and origAttachShadow.apply(this, arguments) for both null safety and forward compatibility.

[rishigupta1599]

This adds up to a 5-second hard wait to every snapshot whenever the page has a single :not(:defined) element. Many production sites legitimately reference custom-element tags that never register (third-party widgets, blocked lazy loaders, typos). That means the common case eats +5s latency with no way to opt out. Please either (a) make the timeout configurable via the snapshot schema, (b) use a much shorter default (e.g. 500ms) since customElements.whenDefined tends to resolve synchronously for already-defined tags, or (c) resolve early if network is idle and the undefined tags have no pending loader.

This will regress P50 build latency for SDK consumers.

[aryanku-dev]

Fixed. Reduced timeout from 5000ms to 500ms. customElements.whenDefined resolves synchronously for already-defined tags, so 500ms is sufficient for the common case while avoiding the latency regression for pages with never-registered custom elements.

[rishigupta1599]

Reading domSnapshot?.[0]?.fidelityRegions suggests multi-root DOMs are expected, but only domSnapshot[0] is consulted — fidelity regions captured by subsequent roots are silently dropped. If responsive snapshot capture produces multiple domSnapshots (per-width), you'll be under-reporting. Either merge regions across all entries or add a comment explaining why only the first matters.

[aryanku-dev]

Added a comment explaining that only the first domSnapshot's fidelity regions are used because for responsive captures with multiple widths, regions are width-independent (same DOM structure) so the first entry is representative.

[rishigupta1599]

The inline comment says "CSS-escape the state value to prevent injection" but the regex only escapes ", \\, }, ] — not whitespace, <, >, &, or non-printables. Custom state names per spec are <dashed-ident>s so this shouldn't happen through legitimate API use; if the intent is defense-in-depth against a non-compliant runtime, either tighten the validation (if (!/^[-\\w]+$/.test(state)) continue;) or soften the comment to "escape attribute-breaking chars".

[aryanku-dev]

Fixed. Replaced the partial regex escape with strict validation: if (!/^[-\w]+$/.test(state)) continue; to skip invalid state values per the <dashed-ident> spec requirement.