iframe-proxy

aryanku-dev · 2026-05-11T09:52:25Z

Summary

Brings percy-selenium-java to parity with the canonical Percy CORS iframe + closed shadow DOM feature set.

Implemented

Nested cross-origin iframe capture (depth-capped, cycle-guarded)
data-percy-ignore attribute opt-out
ignoreIframeSelectors option
Post-switch URL re-check via isUnsupportedIframeSrc
PercyContextLostException recovery merges partialCapture
Closed shadow DOM via CDP (exposeClosedShadowRoots)
Inlined Java helpers (clampFrameDepth, normalizeIgnoreSelectors, resolveMaxFrameDepth, resolveIgnoreSelectors)

Skipped

ElementInternals preflight (Feature 8): N/A — selenium-java has no before-page-load hook.
@percy/sdk-utils bump (Feature 9): not applicable to Java; helpers inlined.

Reference

Mirrored from percy/percy-nightwatch#869 (PER-7292-add-cors-iframe-support); CDP from percy/percy-playwright#609.

Test plan

New IframeFeatureTest (11 tests) and existing CacheTest (3 tests) pass under mvn test.
SdkTest's integration tests require a local Firefox binary (@BeforeAll instantiates FirefoxDriver). They were not exercised in the sync environment because Firefox is not installed; this matches the pre-existing baseline on master and is not a regression introduced by this PR.
Manual smoke: cross-origin iframes
Manual smoke: closed shadow roots in Chrome

🤖 Generated with Claude Code via /percy-sdk-sync

Replace the flat top-level iframe loop with a recursive `processFrameTree` that switches into each cross-origin iframe, captures its DOM, and descends into any further cross-origin iframes nested inside it (up to a configurable depth). Cycles are detected by tracking the chain of ancestor frame URLs and skipping any frame whose `src` already appears in the chain — without this guard, pages that link to each other could produce up to `maxIframeDepth` duplicate corsIframes entries. The depth cap defaults to 5 (matching the canonical Percy SDK behaviour) and is configurable per-snapshot via `maxIframeDepth` or via `cliConfig.snapshot.maxIframeDepth`. Inputs are clamped to a 1..10 range through `clampFrameDepth`. Nested-frame origin is compared against the IMMEDIATE PARENT origin (not the top page origin) so a same-origin grandchild inside a cross-origin parent is correctly inlined by PercyDOM and a cross-origin grandchild inside a same-origin parent is still captured. Mirrors percy/percy-nightwatch#869 and percy/percy-playwright#609. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Skip iframes that carry the `data-percy-ignore` boolean attribute when enumerating both top-level and nested cross-origin iframes. Customers add this attribute to opt out of CORS iframe capture for a specific frame without having to maintain a selector list — useful for ad slots or analytics iframes whose contents are noisy. Selenium's `getAttribute` returns an empty string for boolean attributes with no value, so a non-null result is treated as a positive hit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Customers can now pass an `ignoreIframeSelectors` list (either in the per-snapshot options Map or via `cliConfig.snapshot.ignoreIframeSelectors`) to skip any cross-origin iframe whose element matches one of the supplied CSS selectors. Matching is performed in-browser via `Element.matches` so any selector the browser accepts is valid; invalid selectors are tolerated without aborting the snapshot. Inputs go through `normalizeIgnoreSelectors` which accepts a List<String>, a single String, or null and yields a sanitised List<String> with empty/ whitespace-only entries removed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

After switching into a cross-origin iframe, read `document.URL` and run the unsupported-src check again. The parent-side `src` attribute can be stale or misleading — the frame may have failed to load (leaving an about:blank document), or been navigated by script after attach to a data:/javascript: URL. Skipping these post-switch avoids attempting to serialize a placeholder document. When a post-switch URL is available it is also reported as the captured `frameUrl` and used as the parent context for any nested CORS iframe enumeration. Falls back to the parent-side `src` when the executor returns a non-String value (e.g. under mocking). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ostException When the driver fails to step back to a parent frame after recursing into a nested cross-origin iframe, we previously lost everything captured so far (a flaky network call inside a depth-3 frame would forfeit even the depth-1 snapshot). Introduce `PercyContextLostException` which carries a `partialCapture` list of every iframe snapshot collected before the failure; each recursion layer appends its own captures to the carried list and re-throws, and the top-level loop in `getSerializedDOM` merges the recovered captures into the snapshot and falls back to default content before aborting further sibling enumeration. Mirrors the `percyContextLost` flag in percy/percy-nightwatch#869 and percy/percy-webdriverio#... so the wire-format output stays consistent across SDKs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Closed shadow roots (`{mode: 'closed'}`) are invisible to JavaScript — `element.shadowRoot` is `null` and there is no API that returns the underlying ShadowRoot object. The PercyDOM serializer can pierce them through a window-bound `__percyClosedShadowRoots` WeakMap (host element → shadow root) populated before serialization, but Selenium has no way to obtain the closed shadow root from page script. Use Chrome DevTools Protocol to discover and resolve them: 1. `DOM.getDocument {depth: -1, pierce: true}` to walk the entire DOM tree including closed shadow subtrees. 2. For each closed shadow root, `DOM.resolveNode` on the host and the shadow root to obtain JS object handles. 3. `Runtime.callFunctionOn` to write the pair into the WeakMap. `contentDocument` nodes are skipped because their execution context is distinct and has no WeakMap. Non-Chromium drivers are detected with a single `instanceof ChromeDriver` check and silently fall through, so the SDK keeps working with Firefox/WebKit without changes. Mirrors percy/percy-playwright#609. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Add JUnit + Mockito unit tests for the new helper methods and the nested cross-origin iframe capture flow: - `clampFrameDepth` bounds + defaults - `normalizeIgnoreSelectors` accepts List<String> / String / null - `resolveMaxFrameDepth` precedence (option > cliConfig > default) - `resolveIgnoreSelectors` precedence - `data-percy-ignore` iframes are skipped without `switchTo` - `ignoreIframeSelectors` matches are skipped without `switchTo` - `processFrame` bails after switch when document.URL is unsupported - `PercyContextLostException.partialCapture` round-trips - `getSerializedDOM` recovers partial captures on context loss - `exposeClosedShadowRoots` is a no-op for non-Chrome drivers - `collectClosedShadowPairs` walks the CDP tree and skips iframes Tests live in a separate `IframeFeatureTest` class to avoid being blocked by `SdkTest`'s `@BeforeAll` Firefox initialisation in environments without a Firefox binary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…anup - responsive snapshot detection threw NPE when cliConfig.snapshot was missing or JSON-null; guard each layer before reading responsiveSnapshotCapture. - getSerializedDOM treated a null jse return as a Map and ClassCastException'd deep in the snapshot path. Detect non-Map results and raise a clear error pointing at the @percy/dom load failure as the root cause. - Pair every successful DOM.enable with DOM.disable in a finally block so the CDP session doesn't keep emitting DOM events after closed-shadow capture. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

- Null-safe origin equality in both top-level and nested iframe comparisons. Switched to Objects.equals so a child URI that resolves to no host (data:, mailto:, schemeless) can never trigger an NPE that escapes the per-iframe catch. - Document the clampFrameDepth semantic: maxIframeDepth=0 falls back to DEFAULT_MAX_FRAME_DEPTH (5), mirroring @percy/sdk-utils. Disabling CORS capture should use ignoreIframeSelectors or data-percy-ignore, not depth=0. Comment guards against a silent flip in future refactors. - Expose closed shadow roots inside each CORS frame after switchTo() — mirrors the top-page behaviour so closed shadow DOM inside cross- origin iframes is also captured. Per-pair try/catch in the existing helper keeps one bad backendNodeId from aborting the rest. TODO tracks moving to per-frame CDP sessions when BiDi stabilises. - Remove the dead processFrame method — fully replaced by processFrameTree. Keeping duplicate logic invited drift. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

- nestedIframeWithNullOriginIsNullSafeAndDoesNotAbortLoop: regression test for the NPE risk at the child-origin comparison; a data:... child must not abort the outer CORS frame capture. - clampFrameDepthZeroReturnsDocumentedDefault: semantic guard so any future change to treat 0 as "disable" trips a test. - exposeClosedShadowRootsIsAttemptedInsideCorsFrame: confirms the per-CORS-frame helper invocation and the TODO marker survive future refactors; ensures the call is safe on a non-Chrome driver. - collectClosedShadowPairsContinuesPastOneBadEntry: documents that the collector tolerates missing backendNodeId fields without throwing, and one bad pair does not abort the rest at runtime. - Update the post-switch unsupported-URL test to drive processFrameTree directly (processFrame removed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Picks up the latest CLI patch series for parity with sibling SDKs and to unblock the external percy/percy-java-selenium status check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The legacy processFrame(WebElement, Map) helper was removed as dead code once processFrameTree subsumed it. The reflection-based unit test still called the old name and failed with NoSuchMethodException in CI. Rewrite it to drive the same skip-when-percyElementId-missing path through processFrameTree, asserting an empty result and that the driver is never switched into the frame. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…nd-shadow-dom

…nd-shadow-dom # Conflicts: # package.json # src/main/java/io/percy/selenium/Percy.java

aryanku-dev · 2026-06-23T05:08:40Z

🤖 stack:pr-review — Automated review (BrowserStack AI harness)

Summary: Adds nested CORS iframe capture (depth cap + cycle guard), data-percy-ignore / ignoreIframeSelectors skipping, post-switch URL re-validation, partial-capture recovery on lost frame context, and closed-shadow-DOM exposure via CDP for Chromium — plus 15 new mock-based unit tests.

Priority	Category	Check	Status
High	Security	No hardcoded secrets	Pass
High	Security	Input validation/sanitization	Pass — selectors normalized, depth clamped, `Element.matches` per-selector try/catch, URLs parsed via `URI`
High	Security	AuthZ / IDOR / SQLi	N/A
High	Correctness	Logic & edge cases	Pass — cycle guard via `ancestorUrls`, null-safe `Objects.equals` origin compare, parent-frame restore with `defaultContent` fallback
High	Correctness	Explicit error handling	Pass — `PercyContextLostException` carries partial capture; `FatalIframeException` re-thrown; per-iframe failures debug-logged
High	Correctness	No race conditions	Pass — single-threaded; `DOM.disable` & script timeout released in `finally`
Medium	Testing	New code / error paths tested	Pass — 15 new tests in `IframeFeatureTest`
Medium	Testing	Existing tests pass	Pass — 55/56; sole error is Firefox-missing `SdkTest.testSetup` (env, CI green)
Medium	Performance	No unbounded fetch	Pass — recursion bounded by `maxFrameDepth`
Medium	Quality	Patterns / focused	Pass — mirrors canonical `percy-playwright-java`
Low	Quality	Names / comments / deps	Pass

Findings: none. Notable improvement: getSerializedDOM now throws a clear RuntimeException on non-Map serialize output (caught & degraded gracefully by snapshot()) instead of NPE-ing — strictly better than main.

Verification: mvn -o compile clean; IframeFeatureTest 15/15 pass; full suite 55/56 (only the env-dependent Firefox setup fails).

Overall: ✅ Pass — Approve. No code changes needed. ⚠️ Human blocker: the pending Percy visual-diff approval must be approved in the Percy UI.

_{Posted by the BrowserStack stack:pr-review harness.}

aryanku-dev

Claude Code Review (automated) — 5 inline finding(s). Full report in the PR comment below. Verdict: Failed - see PR comment.

aryanku-dev · 2026-06-26T13:49:24Z

+            // Selenium 4 BiDi support stabilises across versions.
+            try { exposeClosedShadowRoots(driver); } catch (Exception ignore) {}
+
+            Map<String, Object> iframeSnapshot = serializeCurrentFrame(options);


[High] Null iframeSnapshot silently poisons the CORS payload

serializeCurrentFrame unchecked-casts the PercyDOM.serialize result and returns null when the script yields null (e.g. PercyDOM failed to load in a sandboxed CORS frame). That null is then placed straight into the collected entry below, producing a malformed corsIframes payload. The main-page path getSerializedDOM (~line 1063) explicitly throws on a non-Map result — this path lacks the equivalent guard.

Suggestion: After the call, if the snapshot is null/non-Map, log a warning and return collected; (skip this frame) rather than adding an entry with a null snapshot.

Reviewer: stack:code-reviewer

aryanku-dev · 2026-06-26T13:49:25Z

+    // for host + shadow, then Runtime.callFunctionOn to write the pair into
+    // the WeakMap on the page.
+    @SuppressWarnings("unchecked")
+    private void exposeClosedShadowRoots(WebDriver driver) {


[Medium] CDP call bypasses the existing isCdpSupported guard

exposeClosedShadowRoots issues executeCdpCommand directly and relies on an outer try/catch, while changeWindowDimensionAndWait (~line 1275) guards with isCdpSupported(ChromeDriver). Use the same pattern for consistency.

Suggestion: if (!(driver instanceof ChromeDriver)) return; then if (!isCdpSupported(chrome)) return; before any CDP call.

Reviewer: stack:code-reviewer

aryanku-dev · 2026-06-26T13:49:25Z

+        if (cliConfig != null && cliConfig.has("snapshot") && !cliConfig.isNull("snapshot")) {
+            JSONObject snap = cliConfig.getJSONObject("snapshot");
+            if (snap.has("ignoreIframeSelectors") && !snap.isNull("ignoreIframeSelectors")) {
+                JSONArray arr = snap.optJSONArray("ignoreIframeSelectors");


[Medium] Single-string ignoreIframeSelectors CLI config is silently dropped

snap.optJSONArray("ignoreIframeSelectors") returns null for a JSON string value (e.g. "ignoreIframeSelectors": "iframe.ads"), so a string-valued config is dropped — inconsistent with the options-map path that normalizes strings.

Suggestion: When optJSONArray is null, fall back to optString and run it through normalizeIgnoreSelectors.

Reviewer: stack:code-reviewer

aryanku-dev · 2026-06-26T13:49:25Z

+        try {
+            JavascriptExecutor jse = (JavascriptExecutor) driver;
+            JSONArray sel = new JSONArray(selectors);
+            String script = "var el = arguments[0]; var selectors = " + sel.toString() + ";"


[Medium] Selector list interpolated into the script body

The script is built by concatenating sel.toString() (a JSON array literal) into the JS source. A selector containing a quote or control character breaks the literal and throws a script error; the inner el.matches try/catch only handles invalid CSS.

Suggestion: Pass the selector list as an executeScript argument (arguments[1]) instead of interpolating it.

Reviewer: stack:code-reviewer

aryanku-dev · 2026-06-26T13:49:25Z

+            log("Reached max iframe nesting depth (" + maxFrameDepth + "); stopping at " + frameSrc, "debug");
+            return collected;
+        }
+        if (ancestorUrls != null && frameSrc != null && ancestorUrls.contains(frameSrc)) {


[Medium] Cycle guard can miss a relative-vs-absolute src cycle

The guard checks ancestorUrls.contains(frameSrc) on the raw src. The ancestor set is seeded with both raw frameSrc and absolute reportedUrl, so identical recurring src strings are caught — but a cycle expressed as a relative src at one level and an absolute URL at another can slip through. Also untested.

Suggestion: Resolve frameSrc to absolute (against the parent URL) before seeding/checking the ancestor set, and add a cycle-guard test.

Reviewer: stack:code-reviewer

aryanku-dev · 2026-06-26T13:49:27Z

Claude Code PR Review

PR: #312 • Head: 4b7835d • Reviewers: stack:code-reviewer

Summary

Adds cross-origin (CORS) iframe capture and closed shadow DOM exposure to the Percy Selenium Java SDK: recursive iframe traversal with a depth cap and cycle guard, data-percy-ignore / ignoreIframeSelectors honouring, partial-capture recovery on lost frame context via PercyContextLostException, closed shadow DOM exposure via CDP DOM.getDocument(pierce=true) for Chromium, plus null-safety hardening of the responsive-config read and supporting tests.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None introduced.
High	Security	Authentication/authorization checks present	N/A	No auth surface in this change.
High	Security	Input validation and sanitization	Pass	Iframe `src`/scheme validated; see #6 (selector interpolation, defensive).
High	Security	No IDOR — resource ownership validated	N/A	Not applicable.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Fail	#2: null `iframeSnapshot` enters the CORS entry, unlike the guarded main-page path.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Frame errors logged; partial capture propagated via `PercyContextLostException`.
High	Correctness	No race conditions or concurrency issues	Pass	Sequential recursion; no shared mutable state across threads.
Medium	Testing	New code has corresponding tests	Pass	Extensive new tests; gaps noted below (cycle guard, depth-cap recursion).
Medium	Testing	Error paths and edge cases tested	Pass	NPE/clamp/bad-pair/context-lost paths covered.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Existing tests retargeted, not weakened.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Recursion bounded by depth cap.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Fail	#4: bypasses the existing `isCdpSupported` guard used elsewhere.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to CORS iframes + shadow DOM (+ related null-safety).
Low	Quality	Meaningful names, no dead code	Fail	#8: pre-existing unused imports in a touched file.
Low	Quality	Comments explain why, not what	Pass	Comments are explanatory; #12 minor inaccuracy.
Low	Quality	No unnecessary dependencies added	Pass	Only a `packageManager` field added (see #9).

Findings

High

File: src/main/java/io/percy/selenium/Percy.java:956 (helper at :874)
Severity: High
Reviewer: stack:code-reviewer (confirmed)
Issue: serializeCurrentFrame does an unchecked cast of jse.executeScript("return PercyDOM.serialize(...)") and returns null when the script yields null (e.g. PercyDOM failed to load in a sandboxed CORS frame). The call site puts that null straight into the collected entry (entry.put("iframeSnapshot", iframeSnapshot)), producing a poisoned corsIframes payload. The main-page path getSerializedDOM (lines 1063-1066) explicitly throws on a non-Map result — this CORS path lacks the equivalent guard, an asymmetry that surfaces a malformed snapshot silently to the CLI.
Suggestion: After the call, guard for null/non-Map: log a warning and return collected; (skip the frame) rather than adding an entry with a null snapshot — mirroring the main-page guard.

Medium

File: src/main/java/io/percy/selenium/Percy.java:1158
Severity: Medium
Reviewer: stack:code-reviewer
Issue: exposeClosedShadowRoots issues executeCdpCommand directly and relies on an outer try/catch, instead of the existing isCdpSupported(ChromeDriver) guard used by changeWindowDimensionAndWait (line 1275). Inconsistent with the established pattern.
Suggestion: Gate with if (!(driver instanceof ChromeDriver)) return; then if (!isCdpSupported(chrome)) return; before any CDP call.
File: src/main/java/io/percy/selenium/Percy.java:800
Severity: Medium
Reviewer: stack:code-reviewer
Issue: resolveIgnoreSelectors reads snap.optJSONArray("ignoreIframeSelectors"), which returns null for a JSON string value (e.g. "ignoreIframeSelectors": "iframe.ads"). The string config is then silently dropped, inconsistent with the options-map path that normalizes strings.
Suggestion: When optJSONArray is null, fall back to optString and run it through normalizeIgnoreSelectors.
File: src/main/java/io/percy/selenium/Percy.java:819
Severity: Medium
Reviewer: stack:code-reviewer
Issue: iframeMatchesIgnoreSelector builds the script by string-concatenating sel.toString() (a JSON array literal) into the JS body. A selector containing a quote or JS control character would break the literal and throw a script error (the inner el.matches try/catch only handles invalid CSS, not malformed JS).
Suggestion: Pass the selector list as an executeScript argument (arguments[1]) instead of interpolating it into the script body.
File: src/main/java/io/percy/selenium/Percy.java:916
Severity: Medium
Reviewer: stack:code-reviewer (severity adjusted by orchestrator)
Issue: The cycle guard checks ancestorUrls.contains(frameSrc) using the raw src attribute. The ancestor set is seeded with both the raw frameSrc and the absolute reportedUrl (lines 979-980), so an identical recurring src string is caught — but a cycle expressed as a relative src at one level and an absolute URL at another can slip through. Narrower than "fails for the common case," but a real edge-case gap, and it is untested.
Suggestion: Resolve frameSrc to an absolute URL (against the parent URL) before seeding and checking the ancestor set, so comparisons are consistently absolute. Add a cycle-guard test.

Low

File: src/main/java/io/percy/selenium/Percy.java:32-33
Severity: Low — Issue: Unused imports javax.swing.text.html.CSS and javax.xml.xpath.XPath (pre-existing) live in a file this PR touches. Suggestion: Remove them.
File: package.json:11
Severity: Low — Issue: The packageManager SHA-512 digest is 126 chars (Corepack requires the full 128-char hex digest); Corepack may reject or silently fall back. Suggestion: Regenerate via corepack prepare yarn@1.22.22 --activate, or drop the field.
File: src/test/java/io/percy/selenium/IframeFeatureTest.java:1101
Severity: Low — Issue: A test reads Percy.java via the cwd-relative path src/main/java/..., which breaks when the runner's working directory isn't the repo root; it also asserts on source text rather than behaviour. Suggestion: Resolve via classpath/test-class location, or replace the source-text assertion with a behavioural one.
File: src/main/java/io/percy/selenium/Percy.java:1089 — Severity: Low — Issue: ctx map uses stringly-typed keys ("options", "maxFrameDepth", "ignoreSelectors") with unchecked casts; a key typo fails at runtime. Suggestion: Replace with a small value object / record. (Style; non-blocking.)
File: src/test/java/io/percy/selenium/IframeFeatureTest.java — Severity: Low — Issue: Comment in getSerializedDomRecoversPartialCaptureOnContextLost says "Five executeScript phases" but describes four. Suggestion: Fix the comment.

Missing test coverage (non-gating)

Cycle detection (the ancestorUrls.contains(frameSrc) guard) is not exercised by any test.
Depth-cap enforcement via actual recursion (processFrameTree at depth = maxFrameDepth + 1) is not integration-tested; only the clamp helper is unit-tested.
resolveIgnoreSelectors with a single-string CLI config value (finding Use Java 8 for compiling SDK #5) is untested.

Verdict: FAIL — one High-severity correctness issue (#2: null iframe snapshot silently poisons the CORS payload). The Medium findings (#4–#7) are worth folding in before merge; the Low items can follow up.

- Skip CORS frame when PercyDOM.serialize returns null instead of emitting a poisoned snapshot entry (mirror getSerializedDOM guard) [High] - Guard exposeClosedShadowRoots with isCdpSupported before CDP calls [Med] - Honour single-string ignoreIframeSelectors CLI config value [Med] - Pass ignore selectors as executeScript argument, not interpolated [Med] - Add post-switch absolute-URL cycle check for relative-src cycles [Med] - Remove unused CSS/XPath imports [Low] - Add tests for null-snapshot skip, resolved-URL cycle guard, single-string CLI Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

aryanku-dev

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.

aryanku-dev · 2026-06-26T14:01:43Z

+
+    when(((JavascriptExecutor) mockedDriver).executeScript(any(String.class)))
+      .thenReturn(null)
+      .thenReturn("https://cdn.other.com/frame");


[Medium] Brittle positional stub in the cycle-guard test

This .thenReturn(null).thenReturn("https://cdn.other.com/frame") chain passes today, but if a future change inserts another executeScript call between the dom.js injection and readCurrentFrameUrl(), the URL stub fires at the wrong call site — postSwitchUrl becomes null, the cycle check is skipped, and the test stays green while no longer validating the cycle guard.

Suggestion: Use a script-content-keyed thenAnswer (match return document.URL), as the null-serialize and context-lost tests already do.

Reviewer: stack:code-reviewer

aryanku-dev · 2026-06-26T14:01:43Z

+      percy, "processFrameTree",
+      new Class[]{WebElement.class, int.class, Set.class, Map.class},
+      iframe, 1, new HashSet<String>(), ctx);
+    assertTrue(result.isEmpty(), "Frame must be skipped when PercyDOM.serialize returns null");


[Low] Test doesn't verify the frame was stepped out of

The test asserts the result is empty but doesn't confirm parentFrame() ran in the finally. Adding the verify would guard against a future change that bails before switchedIn = true.

Suggestion: Add verify(targetLocator, times(1)).parentFrame(); after the assertion.

Reviewer: stack:code-reviewer

aryanku-dev · 2026-06-26T14:01:45Z

Claude Code PR Review

PR: #312 • Head: c7ffcb1 • Reviewers: stack:code-reviewer

Continues the previous review — changes since 4b7835d (delta re-review).

Summary

Delta commit c7ffcb1 addresses the previous review's findings on the CORS iframe + closed shadow DOM work: null-snapshot skip in serializeCurrentFrame/processFrameTree, an absolute-URL post-switch cycle check, an isCdpSupported guard in exposeClosedShadowRoots, a single-string ignoreIframeSelectors CLI fallback, and selector values passed as a script argument instead of being interpolated — plus three new unit tests. Unused imports removed.

Review Table

Findings

Resolved since last review

~~src/main/java/io/percy/selenium/Percy.java:956 High — Null iframeSnapshot from serializeCurrentFrame poisons CORS entry~~ Resolved — instanceof Map guard + null early-return in processFrameTree; verified.
~~src/main/java/io/percy/selenium/Percy.java:1158 Medium — exposeClosedShadowRoots skips isCdpSupported guard~~ Resolved — if (!isCdpSupported(chrome)) return; added.
~~src/main/java/io/percy/selenium/Percy.java:800 Medium — resolveIgnoreSelectors drops single-string CLI config value~~ Resolved — optString fallback added + test.
~~src/main/java/io/percy/selenium/Percy.java:819 Medium — iframeMatchesIgnoreSelector interpolates selectors into script body~~ Resolved — selectors passed as arguments[1].
~~src/main/java/io/percy/selenium/Percy.java:916 Medium — Cycle guard can miss relative-vs-absolute src mismatch~~ Resolved — post-switch absolute-URL check added + test.
~~package.json:11 Low — Truncated packageManager SHA-512 hash~~ Resolved (false positive) — hash is a valid 128-char digest; no change needed.

Still open (non-gating)

File: src/test/java/io/percy/selenium/IframeFeatureTest.java:292
Severity: Medium
Reviewer: stack:code-reviewer
Issue: processFrameTreeSkipsCyclicFrameByResolvedUrl uses a positional .thenReturn(null).thenReturn("https://cdn.other.com/frame") stub. It passes today, but if a future change inserts another executeScript call between the dom.js injection and readCurrentFrameUrl(), the URL stub fires at the wrong site and the test silently exercises a different path while staying green.
Suggestion: Use a script-content-keyed thenAnswer (matching return document.URL), as the null-serialize and context-lost tests already do.
File: src/test/java/io/percy/selenium/IframeFeatureTest.java:269
Severity: Low
Reviewer: stack:code-reviewer
Issue: processFrameTreeSkipsWhenSerializeReturnsNull asserts the result is empty but doesn't verify the frame was stepped out of. Adding verify(targetLocator).parentFrame() would guard against a future change that bails before switchedIn = true.
Suggestion: Add verify(targetLocator, times(1)).parentFrame();.
File: src/test/java/io/percy/selenium/IframeFeatureTest.java:590
Severity: Low (carried forward)
Reviewer: stack:code-reviewer
Issue: exposeClosedShadowRootsIsAttemptedInsideCorsFrame reads Percy.java via the cwd-relative path src/main/java/..., which breaks when the runner's working directory isn't the repo root. This delta didn't touch that test, so it remains open.
Suggestion: Resolve the path from the class/code-source location, or assert behaviour rather than source text.
File: src/main/java/io/percy/selenium/Percy.java:1190
Severity: Low (informational, pre-existing)
Reviewer: stack:code-reviewer
Issue: After if (!(driver instanceof ChromeDriver)) return;, the subsequent try { chrome = (ChromeDriver) driver; } catch (ClassCastException e) is unreachable. Not introduced by this delta; the new isCdpSupported line sits right beside it.
Suggestion: Drop the redundant try/catch in a future cleanup.

Verdict: PASS — every gating finding from the previous review (1 High + 4 Mediums) is resolved and the fixes were independently verified as correct. The remaining items are non-gating test-quality nits and one pre-existing informational note.

aryanku-dev and others added 12 commits May 11, 2026 15:17

chore: bump @percy/cli to 1.31.14

5641a88

Picks up the latest CLI patch series for parity with sibling SDKs and to unblock the external percy/percy-java-selenium status check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

aryanku-dev marked this pull request as ready for review May 24, 2026 11:45

aryanku-dev requested a review from a team as a code owner May 24, 2026 11:45

aryanku-dev requested review from ninadbstack and rishigupta1599 May 24, 2026 11:45

aryanku-dev added 2 commits May 26, 2026 10:22

Merge remote-tracking branch 'origin/master' into feat/cors-iframes-a…

0c33f24

…nd-shadow-dom

Merge remote-tracking branch 'origin/master' into feat/cors-iframes-a…

4b7835d

…nd-shadow-dom # Conflicts: # package.json # src/main/java/io/percy/selenium/Percy.java

aryanku-dev commented Jun 26, 2026

View reviewed changes

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

aryanku-dev commented May 11, 2026

Summary

Implemented

Skipped

Reference

Test plan

Uh oh!

aryanku-dev commented Jun 23, 2026

🤖 stack:pr-review — Automated review (BrowserStack AI harness)

Overall: ✅ Pass — Approve. No code changes needed. ⚠️ Human blocker: the pending Percy visual-diff approval must be approved in the Percy UI.

Uh oh!

aryanku-dev left a comment

Choose a reason for hiding this comment

Uh oh!

aryanku-dev Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

aryanku-dev Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

aryanku-dev Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

aryanku-dev Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

aryanku-dev Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

aryanku-dev commented Jun 26, 2026

Claude Code PR Review

Summary

Review Table

Findings

High

Medium

Low

Missing test coverage (non-gating)

Uh oh!

aryanku-dev left a comment

Choose a reason for hiding this comment

Uh oh!

aryanku-dev Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

aryanku-dev Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

aryanku-dev commented Jun 26, 2026

Claude Code PR Review

Summary

Review Table

Findings

Resolved since last review

Still open (non-gating)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant