feat: add nested CORS iframes, ignore options, post-switch URL re-check, context-lost recovery, and closed shadow DOM CDP support#739
Merged
Conversation
…ck, context-lost recovery, and closed shadow DOM CDP Brings @percy/selenium-webdriver to feature parity with the canonical Percy CORS iframe + closed shadow DOM work landed across sibling SDKs (percy-nightwatch#869, percy-playwright#609). Key changes: - processFrameTree recurses into cross-origin descendants up to maxIframeDepth (default 5, capped at 20), with an ancestorUrls Set as a cycle guard. - Cross-origin frame enumeration is done in-browser via enumerateIframesScript (returns src / srcdoc / percyElementId / dataPercyIgnore / matchesIgnoreSelector / index for every iframe on the current document). - Per-frame data-percy-ignore attribute is honored. - ignoreIframeSelectors option (per-snapshot or percy.config.snapshot) is honored and matched in-browser via Element.matches. - After switching into a frame we re-read document.URL and re-check isUnsupportedIframeSrc so failed navigations (about:blank / about:srcdoc / net errors) don't get shipped as captured content. - If parent-frame restoration fails deeper than depth=1, we throw a percyContextLost error carrying partialCapture; the outer caller merges the partial capture and stops iterating siblings (whose enumeration was performed in a now-lost context). - exposeClosedShadowRoots walks the CDP DOM tree (DOM.getDocument depth=-1 pierce=true) for shadowRootType === 'closed' nodes, resolves each to a JS object handle and stores it in window.__percyClosedShadowRoots via Runtime.callFunctionOn. No-op on non-Chromium drivers. Called from captureSerializedDOM and re-primed after refresh() in captureResponsiveDOM. - Inlined DEFAULT_MAX_FRAME_DEPTH, isUnsupportedIframeSrc, clampFrameDepth, normalizeIgnoreSelectors, resolveMaxFrameDepth, resolveIgnoreSelectors, getOrigin, shouldSkipIframe locally — @percy/sdk-utils has not yet shipped these shared helpers, so we do NOT bump that dep. Skipped: - Feature 8 (ElementInternals preflight): N/A — selenium-webdriver has no before-page-load hook. Tests: existing CORS-iframe tests rewritten around the new enumeration-based path; new tests cover nested capture + depth cap + cycle guard, data-percy-ignore, ignoreIframeSelectors, post-switch URL re-check, helper unit tests, and exposeClosedShadowRoots CDP behaviour (including contentDocument skipping and graceful no-op for non-Chromium drivers). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The driver.wait callback awaited the resizeCount check but never returned the boolean, so wait() always saw undefined and timed out. Return the comparison so the predicate resolves correctly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous form
const { root } = await driver.sendAndGetDevToolsCommand
? await driver.sendAndGetDevToolsCommand(...)
: await driver.sendDevToolsCommand(...) || {};
parsed as
const { root } = (await driver.sendAndGetDevToolsCommand) ? ... : ...;
`await` resolved the function reference (truthy), the ternary picked the
first branch, but the chosen call's Promise was never awaited.
Destructuring an unresolved Promise yielded `root === undefined`, the
no-root early-return triggered, and the closed-shadow-DOM capture path
was silently dropped on every driver that exposes
sendAndGetDevToolsCommand.
Switch to a single, awaited call through a `cdp` binding that prefers
sendAndGetDevToolsCommand (returns the result) and falls back to
sendDevToolsCommand. The early-return guard now accepts drivers that
expose either function — previously the guard required
sendDevToolsCommand, which excluded Appium-style drivers that only
provide sendAndGetDevToolsCommand.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The pre-existing cycle guard only checked `meta.src` (the element's src attribute) against the ancestor URL set. After switching into the frame the document may resolve to a different URL — redirect chains, location.replace, server-side 30x — and that resolved URL might already appear higher in the ancestor chain. Without re-checking we could recurse into a frame that points back at its own parent or the top page, producing duplicate captures or runaway recursion before the depth cap kicks in. Re-check `ancestorUrls.has(frameUrl)` immediately after the existing post-switch URL safety probe, and bail out of the frame the same way we do for unsupported URLs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mocks each driver shape — sendDevToolsCommand only, sendAndGetDevToolsCommand only, and both present — and asserts that `root` is correctly destructured after the await so Runtime.callFunctionOn runs on the resolved host/shadow objectIds. The both-present case also locks in the preference for sendAndGetDevToolsCommand. A direct test for the no-CDP-at-all driver guard and the undefined-getDocument response complete the matrix; this is the case that the previous ternary precedence bug papered over. Two new tests cover the post-switch URL cycle guard: a child frame whose document.URL resolves to the top page, and a nested frame whose document.URL resolves to its parent frame — both must short-circuit before serialize is called on the cyclic frame. Bump @percy/cli devDependency to 1.31.14. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@percy/cli's @percy/cli-upload transitively requires image-size, whose
1.2.x line bumped its `engines.node` to >=16. Lint, Test (Node 14), and
Typecheck all run on Node 14 (.github/workflows/{lint,test,typecheck}.yml),
so `yarn install` aborted with EBADENGINE before any check could run.
Pin image-size to 1.0.2 via yarn resolutions — the version master is
already on, and the last one that still supports Node 14. No source
change is needed: image-size is a transitive dep used only inside
@percy/cli's upload command, and 1.0.2 / 1.2.1 share the same surface
we care about.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The closed-shadow-DOM CDP path was running unconditionally on every snapshot. When `deferUploads` is true the CLI serializes the DOM later from its own context, so the SDK should never: 1. issue CDP commands that mutate `window.__percyClosedShadowRoots` on a page it isn't about to upload (observable side effect on the user's app), and 2. spend the I/O on a discovery walk that nothing consumes. The fix mirrors `isResponsiveDOMCaptureValid`'s existing gating: a new `isClosedShadowRootsExposureSkipped` helper checks the options-level `deferUploads` first, then `utils.percy.config.percy.deferUploads`. The ternary-paren BLOCKER fix from a2c49bb is untouched — this only changes when the call site fires. Also updates the minHeight responsive test from 3 to 5 expected executeScript calls. The extra two calls (enumerateIframesScript + document.URL probe) come from the CORS iframe + post-switch URL paths that landed in this PR; exposeClosedShadowRoots itself adds zero executeScript calls in that test because the mocked sendDevToolsCommand returns Promise.resolve() (no `root`, early return). The count change is documented inline in the test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The closed-shadow + CORS-iframe work introduced three error-recovery paths in processFrameTree / captureCorsIframes that weren't reachable through any of the existing test fixtures, so coverage on the new code slipped to ~85% branches / ~91% lines (gate is 100%). Adds three driver-shape-specific mock tests: 1. Nested-frame serialize returns falsy → outer captured, inner skipped (lines 199-202). 2. Depth-2 frame with NO `parentFrame` on switchTo() → falls back to defaultContent (line 252). Drivers without parentFrame are common in older Appium-based stacks. 3. Depth-2 frame whose parentFrame restoration rejects → wraps as percyContextLost and re-throws (lines 254-263); outer loop catches, merges partial capture, breaks on remaining siblings (lines 295-302). The third test exercises the cross-call coordination: the percyContextLost error must round-trip from inner finally → outer catch (which appends inner's partial capture) → captureCorsIframes catch (which appends outer+inner's partial capture and aborts sibling iteration). A sibling sentinel frame is included in the metadata list and asserted absent from the final snapshot to prove the break actually fires. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…y code The first push got coverage to 94% lines / 89% branches, with three residual gaps in index.js: 1. `enumerateIframesScript` (lines 116-136): a browser-injected function that nyc can't instrument — it never runs in Node. Marked with `/* istanbul ignore next */` to match the convention used elsewhere in this file for executeScript callbacks. 2. Defensive `depth > maxFrameDepth` guard (lines 146-148): unreachable because the recursive caller already bounds with `depth < maxFrameDepth` (line 214). Kept for forward safety, ignored for coverage. 3. Defensive `throw error` in captureCorsIframes catch (line 302/306): processFrameTree only re-throws percyContextLost from its own catch; any non-context-lost throw would have to come from a future contract change. Kept for forward safety, ignored for coverage. Two new tests fill the remaining real branches: - `pre-switch src equals an ancestor URL`: nested iframe whose src matches an outer frame URL → cycle guard at lines 150-152 fires. Distinct from the post-switch URL guard (already covered). - `findElement cannot locate the iframe`: findElement resolves to null for one pid and a real element for another → confirms lines 166-167 log-and-return path and that the outer captureCorsIframes loop continues with subsequent siblings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…aths
After the previous push CI reported 98.25% lines / 93.33% branches with
six residual uncovered ranges. This fills them with targeted unit tests
against the internal helpers (no new mock-driver fixtures needed where
the helper can be called directly):
- `normalizeIgnoreSelectors(42 | {})`: hits the final `return []` for
truthy non-string non-array shapes.
- `shouldSkipIframe` with `srcdoc` set: hits the srcdoc-skip branch
that was added for inline iframes (their content is already part of
the parent DOM serialization).
- `shouldSkipIframe` with an unparseable src ('not a url at all'):
hits the `getOrigin returns null` branch — distinct from the
`isUnsupportedIframeSrc` branch covered by `about:blank`.
- `captureCorsIframes` outer catch: first `executeScript(enumerate)`
throws → whole CORS path returns [] and the parent snapshot still
posts (lines 317-319). This protects the contract that CORS errors
never break the primary capture.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Roots
After the previous push CI reported branches at 93.96% (gate is 100%).
This adds five focused tests for the remaining defensive sub-expression
branches that aren't reachable through any of the existing fixtures:
processFrameTree:
- Empty frameUrl + non-array enumerate result: drives the `frameUrl ||
meta.src`, `Array.isArray(childrenRaw) ? : []` and `if (frameUrl)`
fallback branches around the nested-recursion call.
- percyContextLost with EMPTY partialCapture: drives the false branch of
`Array.isArray && length` in both the depth-2 catch (line 234-237) and
captureCorsIframes outer loop (lines 299-303).
- capturedError attached as `cause`: requires the depth-2 frame to throw
inside its try (sets capturedError) AND have its parentFrame fail in
the finally — the only sequence that hits the true branch of
`if (capturedError) err.cause = capturedError;` (line 265).
exposeClosedShadowRoots:
- walk() encounters a null child node: drives the defensive `if (!node)
return;` (line 357).
- resolveNode returns no `object` payload: drives the
`!hostObjectId || !shadowObjectId` continue (line 395).
percySnapshot:
- Called with no `options` argument: drives the `options || {}` fallback
in captureSerializedDOM → captureCorsIframes (line 520).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three categories of remaining branches after the previous push:
1. Dead defensive arms — kept for forward-compat, marked as ignored:
- `normalizeIgnoreSelectors`: the `input.length ? [input] : []` ternary's
empty-string arm is unreachable because `!input` above catches it.
Simplified to plain `[input]` with a one-line comment.
- `ancestorUrls || []` inside processFrameTree's nested-recursion block:
the public entry point (captureCorsIframes) always passes a Set, so the
`||` falsy arm protects internal-API callers only.
- `error && error.percyContextLost` and `Array.isArray(error.partialCapture)
&& error.partialCapture.length` inside captureCorsIframes' outer catch:
processFrameTree always populates partialCapture with at least the outer
frame entry before throwing context-lost. The `else` arm of the inner
guard is marked `istanbul ignore else` (we keep statement coverage on
the truthy path).
2. Default-parameter branches — testable, added unit tests:
- `resolveMaxFrameDepth()` called with no args.
- `resolveIgnoreSelectors()` called with no args.
3. Sub-expression fallback branches — testable, added integration tests:
- `meta.src || '(no src)'` inside shouldSkipIframe's dataPercyIgnore and
matchesIgnoreSelector log lines: src omitted.
- `shouldSkipIframe` true inside a nested-frame iteration: a srcdoc child
mixed with a real cross-origin child confirms the for-loop continues
past the skipped sibling.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI reported 99.35% branches (gate 100%). The two remaining gaps are
both forward-compat guards that cannot fire from any production call
site:
- Line 305 (`error && error.percyContextLost` inside captureCorsIframes'
per-meta catch): processFrameTree only re-throws percyContextLost,
so the `else` arm and the `error == null` short-circuit are dead.
Wrapped with `/* istanbul ignore else */`.
- Line 531 (`options || {}` in captureSerializedDOM → captureCorsIframes
argument): captureDOM normalises `undefined` to `{}` via its default
parameter before reaching here, so the `||` falsy arm only protects
direct internal callers passing `null` explicitly (which default
parameters do NOT catch). Split into a named `safeOpts` const so the
`istanbul ignore next` annotation lands cleanly on just the fallback.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nd-shadow-dom # Conflicts: # index.js # test/index.test.mjs
|
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. |
The CORS-iframe src filter and frame-depth constants had diverged from the canonical implementations in percy-protractor / percy-nightwatch `_iframe_shim.js` and percy-playwright `index.js`: - isUnsupportedIframeSrc() compared schemes case-sensitively, so an iframe with `JavaScript:` / `ABOUT:` / `Data:` slipped past the filter and was treated as a capturable cross-origin frame. Browsers resolve scheme names case-insensitively; lowercase the src before matching. - The scheme blocklist was missing devtools:, edge:, opera:, view-source: and file:. Replace the ad-hoc OR chain with the shared BROWSER_INTERNAL_PREFIXES list used by the sibling SDKs. - DEFAULT_MAX_FRAME_DEPTH was 5 and the hard cap 20; the canonical values are 10 (default) and 25 (hard cap). Introduce HARD_MAX_FRAME_DEPTH and use it in clampFrameDepth. Tests updated to assert the new schemes, case-insensitive matching, and the 10/25 depth contract; HARD_MAX_FRAME_DEPTH exported via _internals. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
processFrameTree built its iframe locator by interpolating the raw data-percy-element-id attribute value into a By.css attribute selector. The id is normally an internally-generated PercyDOM value, but a malicious page can set its own data-percy-element-id containing a `"` to break out of the attribute selector and alter which element is matched. Escape quotes and backslashes first, mirroring the `safeId` hardening already present in percy-nightwatch's snapshot.js (CSS.escape is unavailable in this Node-side context, so use the same quote/ backslash fallback). Adds a unit test asserting a quote/backslash-laden id is escaped and the selector stays a single well-formed attribute selector. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
🤖 stack:pr-review — Automated review (BrowserStack AI harness)Summary: Adds CORS nested-iframe capture, ignore options, post-switch URL re-check, context-lost recovery, and closed shadow-DOM exposure via CDP.
Findings (fixed): (1) Verification: all 10 CI checks green (CodeQL, Lint, Typecheck, Semgrep ×2, Test Node 14/16/18). Added unit tests pass; no new failures vs baseline. Commits pushed: Overall: ✅ Pass (code) —
|
aryanku-dev
commented
Jun 26, 2026
aryanku-dev
left a comment
aryanku-dev
left a comment
Contributor
Author
There was a problem hiding this comment.
Claude Code Review (automated) — 1 inline finding(s). Full report in the PR comment below. Verdict: Passed.
| await driver.navigate().refresh(); | ||
| await driver.executeScript(await utils.fetchPercyDOM()); | ||
| // Re-prime closed shadow root WeakMap — refresh creates a new execution context | ||
| await exposeClosedShadowRoots(driver); |
Contributor
Author
There was a problem hiding this comment.
[Medium] exposeClosedShadowRoots runs twice per width in the reload-responsive path
When PERCY_RESPONSIVE_CAPTURE_RELOAD_PAGE=true, this re-prime runs here, and then captureSerializedDOM (called just below) calls exposeClosedShadowRoots again. With deferUploads off, the full CDP DOM walk + per-shadow-root resolveNode calls execute twice per width.
Suggestion: Drop the redundant call — the refreshed context still flows through captureSerializedDOM, which exposes shadow roots itself. Or gate with a per-document "already exposed" flag.
Reviewer: stack:code-reviewer
Contributor
Author
Claude Code PR ReviewPR: #739 • Head: ba860d3 • Reviewers: stack:code-reviewer SummaryBrings
Review Table
Findings
Notes / not attributable to this PR
Raised by other reviewers (not independently re-blocked)
Verdict: PASS — No surviving High/Critical issue; the feature set is justified and correctly implemented for sibling-SDK parity. Address the Medium double-CDP-walk and trim the self-admittedly-dead defensive branches (the bulk of what makes the diff feel "this many changes") before/at merge; the Low items are optional hardening. |
Replace the locally-inlined iframe-capture helpers (isUnsupportedIframeSrc, clampFrameDepth, normalizeIgnoreSelectors, resolveMaxFrameDepth, resolveIgnoreSelectors + scheme list) with the canonical exports published in @percy/sdk-utils via percy/cli#2319, the single source of truth. - Bump @percy/sdk-utils to 1.32.3-beta.1 (first version with the exports) and re-lock yarn.lock. - Depth bounds now sourced from sdk-utils (DEFAULT=3 / HARD=10) instead of the old local DEFAULT=10 / HARD=25; tests updated accordingly. - UNSUPPORTED_IFRAME_SRCS is a superset of the old list (adds ws/wss/ftp), so strictly more unsupported schemes are skipped; no previously-skipped scheme is now captured. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
Claude Code PR ReviewPR: #739 • Head: 5249e5d • Reviewers: stack:code-reviewer SummaryReplaces locally-inlined CORS-iframe helpers with canonical imports from Review Table
FindingsNo blocking issues found. Non-blocking notes (informational only):
Verdict: PASS |
pranavz28
approved these changes
Jun 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Brings @percy/selenium-webdriver to parity with the canonical Percy CORS iframe + closed shadow DOM feature set landed across sibling JS SDKs.
Implemented
maxIframeDepth, cycle-guarded viaancestorUrlsSet)data-percy-ignoreattribute opt-out per iframeignoreIframeSelectorsoption (per-snapshot and globalpercy.config.snapshot.ignoreIframeSelectors)isUnsupportedIframeSrc(drops about:blank navigation failures)percyContextLostrecovery mergespartialCaptureon driver-context failureexposeClosedShadowRootsusingdriver.sendDevToolsCommand); re-primed after page reload in responsive captureDEFAULT_MAX_FRAME_DEPTH,clampFrameDepth,normalizeIgnoreSelectors,isUnsupportedIframeSrc,resolveMaxFrameDepth,resolveIgnoreSelectors,getOrigin,shouldSkipIframe(exposed undermodule.exports._internalsfor unit tests)Skipped
@percy/sdk-utilsversion bump (Feature 9): the shared helpers (DEFAULT_MAX_FRAME_DEPTH, etc.) are not yet exported from the published@percy/sdk-utils. Inlined locally instead.Reference
Mirrored from percy/percy-nightwatch#869 (branch
PER-7292-add-cors-iframe-support), with closed-shadow-DOM CDP pattern from percy/percy-playwright#609 (branchfeat/closed-shadow-dom-cdp).Test plan
masterin environments without Firefox installed (thepercySnapshotdescribe block builds a real Firefox driver inbeforeAll). On CI with Firefox available, this branch passes 71 more tests thanmasterdid in the same environment locally (master: 79 failed / feat: 38 failed).🤖 Generated with Claude Code via /percy-sdk-sync