Add cross-origin iframe capture support#869
Conversation
Enumerate iframes on the page, filter to cross-origin ones with valid src and data-percy-element-id, switch into each via WebDriver frame API, inject PercyDOM, serialize, and attach results as corsIframes on the snapshot payload. Failures are logged at debug level and never crash the snapshot. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rishigupta1599
left a comment
rishigupta1599
left a comment
There was a problem hiding this comment.
Thanks for tackling this. The shape of the feature looks right — enumerate, filter, switch-inject-serialize, restore — and the sequential processing with a finally to restore context is the safe choice. A few substantive concerns to look at before shipping:
- The options passed to
PercyDOM.serializeinside the iframe skip theignoreCanvasSerializationErrors/ignoreStyleSheetSerializationErrorsresolution that the top-level call applies. Customers who rely on those flags will see iframe serialization fail while the parent succeeds. frameUrl: iframe.srcis read from the attribute in the parent context; if the iframe redirected after navigation, this may not match the actual document URL inside the frame.- Everything fails silently at
debuglevel — including the wrappingtry/catchthat swallows all errors. For a brand-new customer-facing capability that is effectively invisible in support scenarios. Worth elevating genuine failures (switch-failed, serialize-failed) towarnso users and support can see why iframes are missing without DEBUG logs. querySelectorAll('iframe')on the top-level document only sees top-level iframes. Nested iframes (iframe inside a same-origin iframe) will not be enumerated or reachable via a numeric index switch from the root. This is probably acceptable for v1 but worth either documenting or noting in the PR description.
Otherwise the defensive filtering (unsupported schemes, srcdoc, same-origin, missing data-percy-element-id) is thorough and the tests cover the main branches. Nice work.
| let iframeSnapshot = await executeScript(browser, function(opts) { | ||
| /* eslint-disable-next-line no-undef */ | ||
| return PercyDOM.serialize(opts); | ||
| }, [{ ...options, enableJavaScript: true }]); |
There was a problem hiding this comment.
The inner iframe serialize only spreads the raw options and adds enableJavaScript, but the outer captureSerializedDOM builds a serializeOptions with ignoreCanvasSerializationErrors / ignoreStyleSheetSerializationErrors resolved from utils?.percy?.config?.snapshot. A customer who configures those via percy config (not inline options) will have them honored on the parent DOM but not inside cross-origin iframes — likely producing confusing partial-failure serializations.
Suggest threading the resolved serializeOptions (or at minimum re-resolving via the helpers) into processFrame/captureCorsIframes so iframe serialization behaves consistently with the parent.
There was a problem hiding this comment.
Fixed. captureCorsIframes and processFrame now receive the resolved serializeOptions (with ignoreCanvasSerializationErrors/ignoreStyleSheetSerializationErrors) instead of raw options, so iframe serialization is consistent with the parent page.
| log?.debug?.(`Successfully captured cross-origin iframe: ${iframe.src} (percyElementId: ${iframe.percyElementId})`); | ||
|
|
||
| return { | ||
| frameUrl: iframe.src, |
There was a problem hiding this comment.
frameUrl: iframe.src is the src attribute captured in the parent context. If the frame has navigated (client-side redirect, 30x, meta-refresh), this will not match the document that was actually serialized. Since we are already inside the frame when processFrame serializes, consider returning document.URL from the in-frame script (alongside the snapshot) and using that as frameUrl. Minor but it will save debugging a class of "captured URL does not match rendered content" tickets.
There was a problem hiding this comment.
Fixed. The in-frame serialize script now returns { snapshot, frameUrl: document.URL } and we use frameResult.frameUrl instead of iframe.src, so it reflects the actual document URL even after client-side navigation.
| iframeSnapshot | ||
| }; | ||
| } catch (error) { | ||
| log?.debug?.(`Failed to process cross-origin iframe ${iframe.src}: ${error.message}`); |
There was a problem hiding this comment.
All failures here — frame-switch errors, PercyDOM injection errors, serialization exceptions — get logged at debug only. For a new feature where iframes will silently be missing from snapshots, customers and support will not see anything unless they flip DEBUG. I would recommend promoting genuine failures (as opposed to expected per-iframe skips) to warn so missing iframes are discoverable without re-running with verbose logging. Same consideration for the outer catch in captureCorsIframes and the frame(null) restore-failure in the finally.
There was a problem hiding this comment.
Fixed. Genuine failures (serialization errors, frame processing errors, frame restore failures, outer catch) are now logged at warn level. Expected per-iframe skips (unsupported src, same-origin, missing percy-element-id) remain at debug.
| log?.debug?.(`Processing cross-origin iframe: ${iframe.src}`); | ||
|
|
||
| // Switch to the iframe by its DOM index | ||
| await promisifyBrowserCommand(browser, 'frame', iframe.index); |
There was a problem hiding this comment.
Numeric frame indexing is a legacy-protocol affordance; the W3C WebDriver spec removed integer frame IDs from Switch To Frame (the spec calls for a WebElement reference). Most drivers still accept integers for back-compat, but if this ever runs against a strict W3C-only driver it will throw. Safer pattern: locate the <iframe> element via CSS (e.g. by data-percy-element-id) and pass the element reference to frame(). It also removes the indexes-drift-if-the-DOM-changes-between-enumeration-and-switch risk.
There was a problem hiding this comment.
Fixed. Now using document.querySelector('iframe[data-percy-element-id="..."]') to locate the iframe element and passing the element reference to frame(), instead of numeric indexing. This eliminates the drift risk if DOM changes between enumeration and switch.
| try { | ||
| // Get all iframe metadata from the parent page context | ||
| let iframeInfo = await executeScript(browser, function() { | ||
| let iframes = document.querySelectorAll('iframe'); |
There was a problem hiding this comment.
Heads-up (not necessarily a blocker): document.querySelectorAll('iframe') on the top frame only returns iframes directly in that document. Iframes nested inside a same-origin child iframe will not be enumerated, and cannot be reached with a numeric-index switch from the root in one hop. Fine for v1, but worth calling out in the PR description / docs so customers with nested embedding know it is not supported.
There was a problem hiding this comment.
Good callout. Added a comment noting this is a known v1 limitation — only top-level iframes are enumerated, nested iframes inside same-origin children are not captured.
…ame switching, document.URL, warn logging - Thread resolved serializeOptions into processFrame/captureCorsIframes for consistent iframe serialization with parent page options - Use CSS selector (data-percy-element-id) instead of numeric frame index to avoid DOM drift between enumeration and switch - Return document.URL from inside the frame instead of iframe.src for accuracy after client-side navigation - Promote genuine iframe capture failures from debug to warn level logging - Add comment noting nested iframe limitation (v1 only captures top-level) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move eslint-disable comment to cover PercyDOM.serialize line
- Update test mocks to handle querySelector for element-based frame switching
- Update test to expect {snapshot, frameUrl} return format from frame serialize
- Update error test to capture warn-level messages instead of debug
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| domSnapshot.cookies = await getCookies(browser); | ||
|
|
||
| // Capture cross-origin iframes | ||
| if (domScript) { |
There was a problem hiding this comment.
why based on domScript check?
There was a problem hiding this comment.
domScript will always be there
There was a problem hiding this comment.
The if (domScript) guard exists for the test/direct-import code paths. captureSerializedDOM is part of the public export (module.exports = { ..., captureDOM, captureSerializedDOM, ... }) and its signature is (browser, options = {}, utils, domScript = null, log = null) — so a consumer or test calling it directly without going through percySnapshot will pass null. Without the guard that path would crash with executeScript(browser, null). Same defensive pattern as maybeReloadPage at line 564.
There was a problem hiding this comment.
It's always there in the percySnapshot flow, but captureDOM / captureSerializedDOM are exported as public surface and accept domScript = null. There's an explicit regression test in this PR ("captureDOM without domScript/log completes without throwing") that exercises that path.
| } | ||
| } | ||
|
|
||
| async function captureSerializedDOM(browser, options = {}, utils, domScript, log) { |
There was a problem hiding this comment.
why need to pass? domScript and log both not gonna change so we can save it in global variable
There was a problem hiding this comment.
The per-call ctx is intentional — there's a regression test in this PR ("concurrent captures do not cross-contaminate per-call context") that asserts two simultaneous captureDOM invocations don't see each other's domScript/log. Module-level state would race those invocations: with multiple browser instances or parallel percySnapshot calls in the same Node process, one snapshot could end up logging to another snapshot's logger or injecting a stale domScript. Rationale also captured in the inline comment at lib/snapshot.js:395-397.
Recursively descend into each cross-origin iframe and re-enumerate iframes inside it, so cross-origin frames at any depth (cross-origin inside cross-origin) are captured as their own corsIframes entry. Uses Nightwatch's frameParent() to step up exactly one level on each return rather than unwinding all the way to the top, with a defaultContent() fallback when parentFrame is unavailable. MAX_FRAME_DEPTH = 10 prevents runaway recursion. The new processFrameTree wraps the previous single-frame logic. The existing top-level path continues to behave the same for same-origin and unsupported iframe sources. Inline tests cover three-level nesting, same-origin descendant skipping, and frame-detached error handling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When frameParent() throws inside a deep recursion, falling back to frame(null) (top) leaves outer recursion levels iterating child iframe references against the wrong context — sibling lookups silently match the top document and produce wrong percyElementId resolutions. Surface the failure as a tagged error (percyContextLost) so the outer captureCorsIframes loop breaks instead of misclassifying remaining siblings. Only triggered when depth > 1; at the top level falling back to the page context is the right destination anyway. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Track ancestor frame URLs through processFrameTree and skip frames that already appear in their own ancestor chain (A->B->A pages were emitting up to MAX_FRAME_DEPTH duplicate corsIframes entries). - When parentFrame restoration fails mid-recursion at depth>1, attach the partial capture to the percyContextLost error so already-serialized frames make it into the final corsIframes array instead of being discarded along with the abort signal. - Preserve original cause when re-throwing from finally so the original error message isn't swallowed. - Tests: cycle protection, parentFrame failure mid-recursion with partial-capture handoff. Stack-aware mock fixed to expose currentFrame as a live getter (Object.assign was snapshotting it as a stale value). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rs filters - maxIframeDepth (snapshot option, also via percy.config.snapshot): caps the recursive walk. Default 10, hard ceiling 25 to prevent abuse from misconfigured values. - ignoreIframeSelectors (snapshot option / config): array of CSS selectors; matching iframes are skipped before the SDK pays the cost of switching into them. - data-percy-ignore attribute on an <iframe> element: per-element opt-out that skips capture without needing a selector. Closes the TB requirement: "Users can optionally exclude specific iframes from capture via CSS selector or data-percy-ignore attribute" and "Nested iframes ... captured recursively up to a configurable depth." Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| await maybeScrollForLazyLoad(browser); | ||
|
|
||
| let { domSnapshot, url } = await captureSerializedDOM(browser, options, utils); | ||
| let { domSnapshot, url } = await captureSerializedDOM(browser, options, utils, domScript, log); |
There was a problem hiding this comment.
Same rationale as the thread on line 211 — domScript and log are deliberately scoped per-call via ctx to keep concurrent captureDOM invocations isolated. A failing test ("concurrent captures do not cross-contaminate per-call context") guards this. Module-level state would race the loggers/scripts across simultaneous snapshot calls.
| } | ||
|
|
||
| async function enumerateIframesInCurrentContext(browser, ignoreSelectors = []) { | ||
| let info = await executeScript(browser, function(selectors) { |
There was a problem hiding this comment.
shift it in dom script only as it is just execution of script
There was a problem hiding this comment.
Could you clarify what to shift? Line 104 is options, — the positional argument to utils.runReadinessGate. The wrapper at line 103 ((script) => executeAsyncScript(browser, script)) is the executor callback that sdk-utils' runReadinessGate invokes with its own generated script body. The script content lives inside sdk-utils, not here, so there's nothing for us to inline. Happy to refactor if you meant a different line.
Address ce:review findings: - Move UNSUPPORTED_IFRAME_SRCS and frame-depth clamping into lib/iframe-utils.js so the constants don't drift across SDKs. - isUnsupportedIframeSrc now lowercases the src so JavaScript:/DATA: prefixes are caught. - resolveMaxFrameDepth/resolveIgnoreSelectors now compose clampFrameDepth and normalizeIgnoreSelectors from the shared utils module. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Firefox replaces failed cross-origin loads with about:neterror, which slipped past the explicit 'about:blank'/'about:srcdoc' enumeration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removes the transitional local iframe-utils.js shim that duplicated constants now exported from @percy/sdk-utils. SDK PR is now blocked on the CLI PR (PER-7292) merging and a sdk-utils 1.32.x release. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…undant check, inline iframe-enumeration script - domScript and log are constant for the duration of a snapshot. Pin them at module scope via a new setSnapshotContext(domScript, log) setter invoked once at the top of the percySnapshot command, instead of threading them through every helper signature. - Drop the if (domScript) guard in captureSerializedDOM — domScript is always present after setSnapshotContext runs. - Replace the enumerateIframesInCurrentContext wrapper with an enumerateIframesScript function that callers invoke directly via executeScript. The wrapper added indirection without value since the body is just the in-browser script. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@percy/sdk-utils 1.31.14-beta.3 doesn't yet export the iframe resolver helpers we use; revert lib/snapshot.js to import from the local shim until a stable sdk-utils release ships them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
^1.32.0 doesn't exist on npm (latest stable is 1.31.14); pin to the beta that actually publishes the iframe constants we consume. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses three CE review MAJORs in lib/snapshot.js: 1. maybeReloadPage no longer calls executeScript(browser, null) when a consumer imports captureDOM directly without setting context. The PercyDOM re-injection is now guarded on a non-null domScript. 2. Module-level domScript + log are removed. Both flow through ctx / function arguments so two concurrent percySnapshot calls in the same Node process cannot race on shared module state. setSnapshotContext is kept as a no-op shim to preserve backward compatibility for any external caller. commands/percySnapshot.js threads the new args. 3. parentFrame failure now always raises PercyContextLost regardless of depth. The previous depth>1 guard silently dropped the signal at depth=1, after which the outer sibling enumeration would continue against a stale top-document context. captureCorsIframes already breaks the outer loop on PercyContextLost and preserves partial capture, so the depth=1 case now gets the same treatment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vbscript: URLs cannot be navigated cross-process and are unsafe to recurse into. The local iframe shim's BROWSER_INTERNAL_PREFIXES list now matches the unit-test expectations and aligns with the broader unsupported-protocol set already covered (javascript:, data:, blob:). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Update existing captureSerializedDOM tests to pass (domScript, log) as explicit arguments now that context flows through the call chain. The "injects serialization flags and cookies" stub now captures only the first execute() call's args so the trailing cors-iframe enumeration (an empty selectors array) doesn't overwrite the assertion target. New tests under "context threading (CE MAJORs)": - captureDOM without domScript/log completes without throwing: exercises the direct-import path that previously crashed inside maybeReloadPage when setSnapshotContext was never called. - maybeReloadPage path is safe when domScript is null: drives the responsive capture path with PERCY_RESPONSIVE_CAPTURE_RELOAD_PAGE set and asserts the null-script branch never reaches executeScript(browser, null). - concurrent captures do not cross-contaminate per-call context: runs two captureDOM invocations through Promise.all with distinct browser stubs and loggers, then asserts each log only mentions its own origin. With the old module-scope log this would have failed. - parentFrame failure at depth=1 raises PercyContextLost and preserves partial capture: makes frameParent always reject so the depth=1 unwind exercises the new always-throw branch and verifies the outer loop breaks before iterating the sibling iframe while retaining the inner capture. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pin to the released 1.31.14 to match the @percy/sdk-utils dep and keep the dev/runtime CLI versions aligned for nightwatch test exec. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…frame-support # Conflicts: # commands/percySnapshot.js # lib/snapshot.js # package.json # test/unit/snapshot.test.js
pranavz28
left a comment
pranavz28
left a comment
There was a problem hiding this comment.
Cross-SDK review across the 4 CORS-iframe PRs. This is the canonical implementation — siblings (webdriverio #1441, protractor #708, selenium-dotnet #436) mirror it.
Additional findings (no precise line anchor)
captureDOM arg-order swap is a silent breaking change — commands/percySnapshot.js was updated to call captureDOM(browser, options, utils, domScript, log). Any external caller (custom runner, downstream SDK) using the previous positional order will pass log where domScript is expected, and executeScript(browser, log) will throw at runtime. Either move to an object-arg signature or add a runtime warning when the second positional looks like a logger.
Top-level ancestor seeded with pageUrl only — lib/snapshot.js (processFrameTree entry): the ancestor Set is seeded as new Set([pageUrl]). A redirect on the first cross-origin frame where src !== document.URL won't detect a cycle against the parent page on the first hop. Seed both the attribute src and the post-switch document.URL consistently.
_iframe_shim.js placement — repo root rather than lib/. Move alongside the other helpers, or make it a lib/_internal/sdk-utils-shim.js so its transient-hack status is unambiguous.
Verdict: approve with minor changes. Fix the inline file: + percyId-escape findings before merge; the other items can land as follow-ups but should mirror to the 3 sibling SDK PRs.
| const BROWSER_INTERNAL_PREFIXES = [ | ||
| 'about:', 'chrome:', 'chrome-extension:', 'devtools:', | ||
| 'edge:', 'opera:', 'view-source:', 'data:', 'javascript:', 'blob:', | ||
| 'vbscript:' |
There was a problem hiding this comment.
MEDIUM — BROWSER_INTERNAL_PREFIXES missing file: (and arguably ws:/wss:/ftp:).
The protractor port (#708) added file: after CI surfaced regressions. file: iframes can leak local FS paths in serialized URLs if encountered; ws:/wss:/ftp: are also non-navigable for iframe content.
Add at minimum 'file:' so the four SDKs ship the same scheme set:
'edge:', 'opera:', 'view-source:', 'data:', 'javascript:', 'blob:',
'vbscript:', 'file:'Also worth tracking: once @percy/sdk-utils ≥ 1.32.0 exports this list canonically, delete this shim — three SDKs currently carry near-identical copies that will drift.
There was a problem hiding this comment.
| // Switch to the iframe by its data-percy-element-id attribute instead of numeric | ||
| // index, which avoids drift if the DOM changes between enumeration and switch | ||
| let iframeElement = await executeScript(browser, function(percyId) { | ||
| return document.querySelector('iframe[data-percy-element-id="' + percyId + '"]'); |
There was a problem hiding this comment.
LOW — unescaped percyId in CSS selector concatenation.
return document.querySelector('iframe[data-percy-element-id="' + percyId + '"]');percyId is sdk-generated so practical risk is low, but if it ever contains ", \, or a newline the selector breaks and the lookup silently fails. The protractor port (#708) explicitly escapes \ and " before injection — port the same fix here:
const escaped = String(percyId).replace(/\\/g, '\\\\').replace(/"/g, '\\"');
return document.querySelector(`iframe[data-percy-element-id="${escaped}"]`);Or use CSS.escape(percyId).
There was a problem hiding this comment.
Fixed in d2f450e. The serialized in-browser function now uses CSS.escape() (with a String.replace(/["\\]/g, '\\$&') fallback for older browsers) before interpolating percyId into the attribute selector. Defence-in-depth since the id is SDK-generated.
- _iframe_shim.js: add file:, ws:, wss:, ftp: to BROWSER_INTERNAL_PREFIXES to match protractor canonical (#708) and block local-FS leaks - lib/snapshot.js: CSS.escape() percyElementId before attribute-selector concatenation as defence-in-depth against non-UUID ids - test/unit/snapshot.test.js: cover new prefixes Addresses PR #869 comments from pranavz28. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ESLint flags `CSS` as undefined in this Node-env file even though the function ships to the browser via executeScript. typeof-guard the access, add eslint-disable-next-line, and use const instead of var. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This PR is stale because it has been open for more than 14 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
* feat(snapshot): add nested CORS iframe capture with depth, cycle, ignore, and recovery controls Replace the single-level processFrame helper with a recursive ProcessFrameTree pipeline that mirrors the canonical Percy CORS iframe spec from percy/percy-nightwatch#869. Adds: - DEFAULT_MAX_FRAME_DEPTH/MAX_ALLOWED_FRAME_DEPTH, ClampFrameDepth, NormalizeIgnoreSelectors, ResolveMaxFrameDepth, ResolveIgnoreSelectors inlined since .NET has no @percy/sdk-utils equivalent. - ENUMERATE_IFRAMES_SCRIPT + IframeInfo to collect iframe metadata in one round-trip per frame context. - ShouldSkipIframe centralizes filtering: data-percy-ignore attribute, ignoreIframeSelectors matches, unsupported / srcdoc / same-origin frames, and frames without data-percy-element-id are dropped early. - ProcessFrameTree recurses cross-origin descendants up to MaxFrameDepth, with a HashSet ancestor-URL chain to break cyclic A->B->A graphs. - Post-switch URL re-check: after SwitchTo().Frame, the loaded document.URL is rechecked against IsUnsupportedIframeSrc to handle late navigations. - PercyContextLostException carries the partial capture so a failed SwitchTo().ParentFrame() unwind still surfaces every frame serialized up to the failure. CaptureCorsIframes wires the helpers into getSerializedDom, replacing the prior ad-hoc top-level iframe loop. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(snapshot): expose closed shadow DOM via CDP for Chrome drivers Mirrors percy/percy-playwright#609. Walks the CDP DOM tree (DOM.getDocument with depth=-1 and pierce=true), collects backendNodeId pairs for every closed shadow root, resolves both sides via DOM.resolveNode, then uses Runtime.callFunctionOn to register each shadow root in a window-bound WeakMap (window.__percyClosedShadowRoots) that PercyDOM.serialize() reads during cloning. Nodes inside child frame documents are skipped because their execution contexts don't share the WeakMap. Wired into Snapshot() before serialization and into the responsive capture reload path so the WeakMap survives page.reload(). No-op on non-Chrome drivers and when ExecuteCdpCommand isn't available, so existing Firefox / non-Chrome test paths are unaffected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(test): add unit tests for CORS iframe helpers Covers the inlined helpers (GetOrigin, IsUnsupportedIframeSrc, ClampFrameDepth, NormalizeIgnoreSelectors), the ShouldSkipIframe skip matrix (data-percy-ignore, ignoreIframeSelectors, unsupported src, srcdoc, same-origin, missing percyElementId, and immediate-parent origin comparison), and the PercyContextLostException carrier. Tests rely on the existing InternalsVisibleTo("Percy.Test") in AssemblyInfo.cs and use reflection for the private IframeInfo / ShouldSkipIframe symbols so the production API stays sealed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Refresh package-lock.json Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Mark _http volatile; release DOM domain after closed shadow exposure - Add volatile to the _http field so the unlocked outer read in getHttpClient sees a fully-published HttpClient (Timeout set) on weak memory models. Without it the DCL pattern provides no happens-before guarantee on ARM/Apple Silicon. - Pair DOM.enable with DOM.disable in a finally block so the CDP session doesn't keep emitting DOM events after every snapshot + resize/reload. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Pin HttpClient init invariant under DCL + volatile Two reflection-based unit tests confirm the invariant the newly-volatile _http field exists to protect: - First getHttpClient() call returns a client whose Timeout is already TimeSpan.FromMinutes(10) (the bug we fixed: setting Timeout AFTER assigning the field could let concurrent callers observe a default 100s timeout). - Subsequent calls return the same instance — DCL must not rebuild. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(snapshot): document ClampFrameDepth(0) -> default semantic CE re-review flagged ClampFrameDepth silently mapping user-supplied 0 to DEFAULT_MAX_FRAME_DEPTH (5) as a hidden behavior. The intent matches @percy/sdk-utils clampFrameDepth and the JS SDKs: 0 means "use default" (unset / config placeholder), not "disable iframe capture". To disable nested capture, users should omit the option or use data-percy-ignore / ignoreIframeSelectors. No behavior change — pure documentation so future readers don't re-flag. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(snapshot): recurse closed shadow walk into same-origin iframes CE re-review flagged that CollectClosedShadowRoots early-returned on any node carrying contentDocument, so closed shadow roots living inside same-origin iframes were silently skipped — even though those frames share the parent's JS realm and therefore the same window.__percyClosedShadowRoots WeakMap that PercyDOM.serialize reads. Walker now decides per-frame: * Same origin (compared via GetOrigin against the page URL) -> recurse INTO contentDocument; found closed roots use the parent realm's map. * Cross origin -> still skipped (different JS realm, the resolveNode objectIds don't belong to our execution context anyway). * Missing documentURL / unknown origin -> defensive skip (pre-fix behavior for the unknown case). Also factored the CDP flow inside ExposeClosedShadowRoots into a pure internal RunClosedShadowRootExposure that takes a CDP invoker delegate, a script-runner delegate, and a page-URL getter. The WebDriver-bound entrypoint now adapts the reflected ExecuteCdpCommand to those delegates, which makes the DOM.enable / DOM.disable lifecycle directly unit-testable without a real browser. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(snapshot): cover DOM.disable lifecycle and same-origin shadow walk Addresses CE re-review MINORs and MAJOR coverage gaps: * Same-origin closed-shadow-in-iframe recursion: three new tests drive CollectClosedShadowRoots with synthesized CDP DOM payloads — one same-origin iframe (closed root inside contentDocument is captured), one cross-origin iframe (skipped), one with missing documentURL (defensive skip). Plus a sanity test that a top-level closed root still works. * DOM.disable invocation regression coverage: a FakeCdp recorder exercises RunClosedShadowRootExposure end-to-end and asserts: - DOM.disable is sent after a successful run, as the final command. - DOM.disable is still sent if DOM.getDocument throws. - DOM.disable is NOT sent when DOM.enable itself throws (domEnabled stays false in the finally block — avoids spurious disable on a session that never enabled the domain). * HttpClient state serialization: the GetHttpClient_* tests mutate the static volatile _http field. Tag the suite with [Collection("HttpClientStateSerial")] (DisableParallelization = true) so a future flip of parallelizeTestCollections in xunit.runner.json can't race them against Percy.Test.cs / PercyDriver.Test.cs, which also touch setHttpClient. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(deps): bump @percy/cli to 1.31.14 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address review: full iframe prefix list and serial test collection - Percy.cs: expand IsUnsupportedIframeSrc to the canonical list (about:, chrome:, chrome-extension:, devtools:, edge:, opera:, view-source:, data:, javascript:, blob:, vbscript:, file:), matching the nightwatch/protractor ports. Blocks about:blank, blob:, and file:// iframes that previously slipped through. - Percy.Test.cs / PercyDriver.Test.cs: opt UnitTests and PercyDriverTest into the HttpClientStateSerial collection so GetHttpClient_* tests no longer race them on Percy._http. - CorsIframesTest.cs: cover the new prefixes and case-insensitivity. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: regenerate package-lock.json to match package.json Master's lockfile was stale (pinned @percy/cli 1.31.10-beta.2 while package.json declared ^1.31.15-beta.0), and the previous regeneration on this branch was done with an npm version that stripped the "license" fields — producing ~124 lines of phantom diff. Regenerated with npm 10.8.2 so license fields are preserved and the lockfile resolves the version package.json actually asks for. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
captureDOM is an exported helper that was published in v2.2.1-beta.0 with the trailing argument order (browser, options, utils, log, domScript). This branch reordered the trailing pair to (..., domScript, log) so the per-call domScript + logger could be threaded through ctx (avoiding a concurrency race on shared module state). Silently swapping two published positional arguments is a breaking change: an external caller importing captureDOM directly and still passing (log, domScript) would have its logger injected as the PercyDOM script and its script used as the logger. captureDOM now normalizes the trailing pair by type — domScript is always a string, log is always an object — so both the legacy (log, domScript) and the new (domScript, log) call shapes route each value to the correct slot. Internal helpers keep the new order; only the public entry point performs the normalization. Adds a unit test asserting both argument orders produce identical results and that the logger is never injected as a script. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🤖 stack:pr-review — Automated review (BrowserStack AI harness)Summary: Adds CORS iframe capture — recursive nested-iframe walking with depth cap, cycle detection,
Findings (the one real Fail — now fixed): Verification: lint clean; 38/38 unit tests pass. Commits pushed: Overall: ✅ Pass — Approve. The breaking-change concern is resolved deterministically (old/new shapes are type-disjoint), so it did not require a product decision. No human-decision blockers.Posted by the BrowserStack |
Delete the local _iframe_shim.js (which bridged the _FRAME_ helper names to the published sdk-utils and hand-copied the unsupported-scheme list) and import the canonical helpers from @percy/sdk-utils (percy/cli#2319), aliasing DEFAULT_MAX_IFRAME_DEPTH/clampIframeDepth to the local _FRAME_DEPTH names this module already uses. - Bump @percy/sdk-utils to 1.32.3-beta.1 and re-lock. - Depth bounds now sourced from sdk-utils (DEFAULT=3 / HARD=10) instead of the shim's 10/25 fallback. - normalizeIgnoreSelectors is now the canonical value-shaped helper, which resolveIgnoreSelectors already passes a raw value to (the shim's options-shaped variant silently returned []). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude Code PR ReviewPR: #869 • Head: cf169bd • Reviewers: stack:code-reviewer SummaryConsumes canonical iframe-capture helpers from Review Table
FindingsNo blocking issues found. Non-blocking observations:
Verdict: PASS |
3 participants
Summary
Adds cross-origin iframe capture to
@percy/nightwatch. This is the canonical WebDriver implementation of the PER-7292 iframe feature that the other WebDriver SDKs (selenium-js/java/python, capybara/ruby) are ported from.What's included
Cross-origin iframe capture (recursive, WebDriver frame API)
src+data-percy-element-id, switches into each via the WebDriver frame API, injects PercyDOM, serializes, and attaches the results ascorsIframeson the snapshot payload.data-percy-element-idCSS selector (not numeric index) to avoid DOM drift between enumeration and switch; readsdocument.URLfrom inside the frame for post-navigation accuracy;CSS.escape()s the id as defence-in-depth.Nested capture & safety (the WebDriver-specific hard parts)
processFrameTree), stepping up exactly one level withframeParent()on return (with adefaultContent()fallback).maxIframeDepth(default 10, hard cap 25),ignoreIframeSelectors, anddata-percy-ignorecontrols — also viapercy.config.snapshot.frameParent()restoration fails mid-recursion, a taggedPercyContextLostis raised (at any depth) so the outer loop breaks instead of resolving siblingpercyElementIds against the wrong document — with already-serialized frames preserved aspartialCapture.about:,javascript:,data:,blob:,vbscript:,file:,ws:/wss:/ftp:, etc., case-insensitive).Correctness / API hygiene
domScript,log) threaded through arguments rather than module scope, so concurrentpercySnapshotcalls in one process can't race on shared state.captureDOMnormalizes its trailing(domScript, log)pair by type, keeping the previously-published(log, domScript)call shape backward-compatible.@percy/sdk-utils(local shim as a transitional fallback until a stable sdk-utils export ships).Tests
Unit coverage for three-level nesting, same-origin-descendant skipping, frame-detached errors, cycle protection,
parentFrame-failure partial-capture handoff, concurrency isolation, and backward-compatiblecaptureDOMarg order.🤖 Generated with Claude Code