fix: merge .percy.yml config options with snapshot options for serializeDOM

[rishigupta1599]

Summary

• Config options from .percy.yml were not being passed to PercyDOM.serialize()
• Only per-snapshot options were used for DOM serialization
• Now merges cliConfig.snapshot into options Map before calling getSerializedDOM(), with per-snapshot options taking priority

Test plan

• Existing tests pass
• Verify .percy.yml config options are applied during DOM serialization

🤖 Generated with Claude Code

[rishigupta1599]

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

[rishigupta1599]

[Low] Config-supplied widths becomes a JSONArray, not a List

snapshotConfig.get(key) returns org.json native types. A widths array from .percy.yml is copied into mergedOptions as a JSONArray; Java-side consumers that branch on instanceof List would silently ignore it (the serialize/POST new JSONObject(options) path is unaffected).

Suggestion: convert JSONArray values to List when copying, or add a test confirming config-supplied widths flow through.

Reviewer: stack:code-review

[rishigupta1599]

Claude Code PR Review

PR: #310 • Head: 65470c8 • Reviewers: stack:code-review

Summary

Merges global .percy.yml snapshot config into per-call snapshot options (per-call priority) inside snapshot(String, Map), and feeds the merged map to isCaptureResponsiveDOM, captureResponsiveDom, and getSerializedDOM so DOM serialization honors .percy.yml defaults (PER-8053).

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Security lens out of scope for this review.
High	Security	Authentication/authorization checks present	N/A	Security lens out of scope for this review.
High	Security	Input validation and sanitization	N/A	Security lens out of scope for this review.
High	Security	No IDOR — resource ownership validated	N/A	Security lens out of scope for this review.
High	Security	No SQL injection (parameterized queries)	N/A	Security lens out of scope for this review.
High	Correctness	Logic is correct, handles edge cases	Fail	mergedOptions.putAll(options) clobbers config values with null. Positional overloads (Percy.java:267-274) build options with explicit nulls for unset params, so .percy.yml defaults (e.g. percyCSS, minHeight) are silently overwritten with null for the common snapshot(name, widths, ...) path. Per-call priority should treat null as "unset".
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	New merge block adds no swallowing; null/has/isNull guards on cliConfig.snapshot are correct (Percy.java:379).
High	Correctness	No race conditions or concurrency issues	Pass	New local mergedOptions HashMap is method-scoped; no shared mutable state introduced.
Medium	Testing	New code has corresponding tests	Fail	No test files changed. Merge precedence (config-only, caller-wins, null-clobber, array widths) is untested.
Medium	Testing	Error paths and edge cases tested	Fail	No coverage for cliConfig == null, missing snapshot key, or JSONArray widths from config.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Behavior preserved for empty snapshot config in existing tests; no signatures changed.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	One bounded keySet iteration over config; no I/O added.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to a synchronous SDK snapshot path.
Medium	Quality	Follows existing codebase patterns	Pass	Uses the established cliConfig.has/isNull guard idiom and org.json access patterns.
Medium	Quality	Changes are focused (single concern)	Pass	Diff is limited to the merge step and three call sites in snapshot().
Low	Quality	Meaningful names, no dead code	Pass	mergedOptions is clear. Note: postSnapshot (Percy.java:410) still receives raw options, not mergedOptions — see Findings.
Low	Quality	Comments explain why, not what	Pass	Inline comment states the per-call-priority intent.
Low	Quality	No unnecessary dependencies added	Pass	Uses existing HashMap/org.json; no new imports/deps.

Findings

• File: src/main/java/io/percy/selenium/Percy.java:385

• Severity: High

• Reviewer: stack:code-review

• Issue: mergedOptions.putAll(options) copies every entry from options, including explicit null values. The positional overloads at Percy.java:267-274 build options with put("percyCSS", null), put("minHeight", null), put("scope", null), etc. for params the caller did not supply. So percy.snapshot("name", widths) first seeds percyCSS/minHeight/scope from .percy.yml and then immediately overwrites them with null, defeating the merge for the most common call paths.

• Suggestion: Skip null values when overlaying caller options: options.forEach((k, v) -> { if (v != null) mergedOptions.put(k, v); });

• File: src/main/java/io/percy/selenium/Percy.java:410

• Severity: Medium

• Reviewer: stack:code-review

• Issue: DOM is serialized with mergedOptions, but postSnapshot(..., options) is still called with the raw options. If the CLI does not re-apply .percy.yml config to the POST body, merged keys present during serialization (e.g. enableJavaScript, scope) are dropped from the upload payload, yielding an inconsistency between what was captured and what is reported. If the CLI does re-merge server-side this is intentional — add a comment stating so.

• Suggestion: Either pass mergedOptions to postSnapshot, or add a comment documenting that the CLI merges global config into the POST body so raw options is intentional here.

• File: src/main/java/io/percy/selenium/Percy.java:381

• Severity: Low

• Reviewer: stack:code-review

• Issue: snapshotConfig.get(key) returns org.json native types; a config-supplied widths array becomes a JSONArray, not a List<Integer>. Java-side consumers that branch on instanceof List would silently ignore config-supplied widths (the new JSONObject(options) serialize/POST path is unaffected since it handles JSONArray).

• Suggestion: Convert JSONArray values to List when copying into mergedOptions, or add a test confirming config-supplied widths flow through correctly.

• File: src/main/java/io/percy/selenium/Percy.java:363

• Severity: Low

• Reviewer: stack:code-review

• Issue: isCaptureResponsiveDOM re-reads cliConfig.getJSONObject("snapshot") directly even though it now receives the already-merged mergedOptions. This is redundant (the merged map already carries responsiveSnapshotCapture) and the bare cliConfig.getJSONObject("snapshot") access lacks a has/null guard — pre-existing, but the merge makes the divergence more confusing. Not changed by this PR.

• Suggestion: Read responsiveSnapshotCapture from the passed-in merged options and add a has("snapshot") guard.

---

Verdict: FAIL — core intent is sound and the diff is small, but the null-clobber in putAll(options) defeats the merge for the common positional-overload call paths and there are no tests; fix the null handling (and confirm the postSnapshot options choice) before merge.

[rishigupta1599]

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

[rishigupta1599]

[Medium] Config values land in mergedOptions as raw org.json types

snapshotConfig.get(key) puts JSONObject/JSONArray instances into mergedOptions. buildSnapshotJS re-wraps via new JSONObject(options) so serialization is fine, but extractResponsiveWidths checks instanceof List<?> and resolveReadinessConfig checks instanceof Map — a widths/readiness value sourced from .percy.yml would be a JSONArray/JSONObject and silently miss those branches.

Suggestion: Seed from fully-unwrapped Java types, e.g. mergedOptions.putAll(snapshotConfig.toMap());

Reviewer: stack:code-reviewer

[rishigupta1599]

Claude Code PR Review

PR: #310 • Head: fef3e61 • Reviewers: stack:code-reviewer

Summary

Fixes a config-clobber bug in Percy.snapshot(name, options): instead of putAll(options) over .percy.yml config, the merge now seeds mergedOptions from cliConfig.snapshot and overlays only non-null per-call options, so null values injected by the positional snapshot() overloads no longer wipe out real .percy.yml config values used for DOM serialization / responsive capture.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Security lens skipped for this review.
High	Security	Authentication/authorization checks present	N/A	Security lens skipped for this review.
High	Security	Input validation and sanitization	N/A	Security lens skipped for this review.
High	Security	No IDOR — resource ownership validated	N/A	Security lens skipped for this review.
High	Security	No SQL injection (parameterized queries)	N/A	Security lens skipped for this review.
High	Correctness	Logic is correct, handles edge cases	Pass	The non-null overlay correctly fixes the prior FAIL: null per-call params no longer clobber .percy.yml config (Percy.java:385-394). cliConfig is null-guarded at line 379.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Merge logic adds no new throwing paths; existing try/catch around serialization unchanged.
High	Correctness	No race conditions or concurrency issues	Pass	Pure local-map construction, no shared mutable state introduced.
Medium	Testing	New code has corresponding tests	Fail	No unit test proves the null-clobber regression is fixed (e.g. positional snapshot(name) preserving a .percy.yml minHeight). Existing tests stub cliConfig but do not assert config survives an unset positional param. Severity Medium.
Medium	Testing	Error paths and edge cases tested	Pass	No new error paths added; existing edge-case tests (responsive, minHeight, deferUploads) still apply.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Change is additive to the merge; existing cliConfig/responsiveSnapshotCapture tests remain valid.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Single bounded loop over config keys + options entries.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to SDK snapshot path.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors the null-aware merge pattern used elsewhere (resolveConfiguredMinHeight, resolveReadinessConfig).
Medium	Quality	Changes are focused (single concern)	Pass	One file, one concern: the config/options merge for serialization.
Low	Quality	Meaningful names, no dead code	Pass	mergedOptions is clear; comments explain the null-overlay rationale.
Low	Quality	Comments explain why, not what	Pass	Inline comment explains why only non-null values are overlaid.
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies.

Findings

• File: src/test/java/io/percy/selenium/SdkTest.java

• Severity: Medium

• Reviewer: stack:code-reviewer

• Issue: The fix adds no test that demonstrates the regression. No test asserts that a .percy.yml snapshot value (e.g. minHeight, percyCSS) is preserved when the same key is left unset by a positional snapshot(name) call (which injects null).

• Suggestion: Add a unit test that sets cliConfig.snapshot.minHeight and calls the positional snapshot("name"), then verifies the serialize JS / merged options still carry the config minHeight rather than null.

• File: src/main/java/io/percy/selenium/Percy.java:270

• Severity: Medium

• Reviewer: stack:code-reviewer

• Issue: The positional overloads default enableJavaScript to the primitive false (Percy.java:191/202/214), which boxes to a non-null Boolean.FALSE. The non-null overlay therefore always writes enableJavaScript=false into mergedOptions, so a .percy.yml enableJavaScript: true can never be honored via the short-form overloads. This is pre-existing behavior of the positional API (the old putAll had the same effect), not a regression introduced by this fix — flagged for awareness, not as a blocker.

• Suggestion: Change enableJavaScript in the positional overloads from boolean to @Nullable Boolean (consistent with sync / responsiveSnapshotCapture) so an unset value stays null and config can win.

• File: src/main/java/io/percy/selenium/Percy.java:381

• Severity: Medium

• Reviewer: stack:code-reviewer

• Issue: snapshotConfig.get(key) returns raw org.json types (JSONObject/JSONArray) into mergedOptions. buildSnapshotJS re-wraps via new JSONObject(options) so serialization is fine, but extractResponsiveWidths (Percy.java:922) checks instanceof List<?> and resolveReadinessConfig checks instanceof Map — a widths/readiness value sourced from .percy.yml would be a JSONArray/JSONObject and silently miss those branches. Narrow latent path (only when config supplies those keys), so Medium.

• Suggestion: Seed from snapshotConfig.toMap() (fully-unwrapped Java types) instead of the keySet()/get loop.

• File: src/main/java/io/percy/selenium/Percy.java:417

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: postSnapshot receives raw options, not mergedOptions. This is the INTENDED cross-SDK pattern — the CLI merges .percy.yml config server-side on POST — so it is correct, not a bug.

• Suggestion: Optionally add a one-line comment at line 417 noting options (not mergedOptions) is passed intentionally because the CLI merges config server-side.

---

Verdict: PASS — The fix correctly resolves the prior null-clobber FAIL and is well-scoped. Remaining findings are Medium/Low latent or pre-existing concerns (missing regression test, enableJavaScript boolean default, org.json type leakage), none of which are High/Critical, so they do not block.

[rishigupta1599]

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

[rishigupta1599]

[Low] POST body not asserted

The request(...) stub uses any(JSONObject.class) and never asserts on the posted payload. This SDK intentionally posts the raw per-call options to the CLI (the documented cross-SDK pattern, with config applied server-side), so this is a coverage observation rather than a defect.

Suggestion: Optional — add an ArgumentCaptor<JSONObject> on request(...) if future behavior ever depends on the posted body. Non-blocking.

Reviewer: stack:code-review

[rishigupta1599]

[Low] Fragile serialize-arg extraction

The serialized options are recovered via substring/indexOf string slicing of the JS script. This silently breaks if buildSnapshotJS formatting ever changes (e.g. added comment or multiline output).

Suggestion: Optional — capture/verify the serialized map more directly (e.g. spy on the builder) instead of parsing the script string. Non-blocking.

Reviewer: stack:code-review

[rishigupta1599]

Claude Code PR Review

PR: #310 • Head: ede01f6 • Reviewers: stack:code-review

Summary

Merges .percy.yml snapshot config into the snapshot(name, options) path so global config keys are inherited by every snapshot while non-null per-call options take precedence; the current head commit (ede01f6) adds a unit test asserting that config-only keys survive and per-call values override config (merge precedence).

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	No secrets/credentials involved; security lens skipped for this config-merge change.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	N/A	Inputs are caller-supplied snapshot options / local YAML config; no external/untrusted input.
High	Security	No IDOR — resource ownership validated	N/A	No resource access by id.
High	Security	No SQL injection (parameterized queries)	N/A	No database access.
High	Correctness	Logic is correct, handles edge cases	Pass	Merge guards cliConfig != null, has("snapshot"), !isNull("snapshot"), and skips null per-call values so positional-overload nulls do not clobber config. One residual edge: a positional-overload enableJavaScript=false (primitive default) is non-null and overrides a config true — Medium, see Findings.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	No new catch blocks; existing WebDriver error handling unchanged.
High	Correctness	No race conditions or concurrency issues	Pass	mergedOptions is a method-local map; no shared mutable state introduced.
Medium	Testing	New code has corresponding tests	Pass	snapshotMergesCliConfigWithPerCallOptionsPrecedence covers config-only key survival and per-call override of a conflicting key.
Medium	Testing	Error paths and edge cases tested	Partial	Happy-path precedence covered; no test for null-cliConfig path, the enableJavaScript=false clobber edge, or the POST payload (the SDK intentionally posts raw options — see Findings). Non-blocking.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Change is additive; existing snapshot tests pass mergedOptions through the same capture path.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	No queries.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to an SDK snapshot call.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors the existing readiness shallow-merge pattern in the same file; matches the cross-SDK config-merge approach.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to config<->option merge plus its test.
Low	Quality	Meaningful names, no dead code	Pass	mergedOptions / snapshotConfig are clear; no dead code.
Low	Quality	Comments explain why, not what	Pass	The null-overlay comment explains the positional-overload rationale.
Low	Quality	No unnecessary dependencies added	Pass	Uses already-imported java.util.* / org.json; no new deps.

Findings

• File: src/main/java/io/percy/selenium/Percy.java:263-276 (positional overloads; merge consumed at lines 388-392)

• Severity: Medium

• Reviewer: stack:code-review

• Issue: The null-overlay guard skips null per-call values, but enableJavaScript is a primitive boolean in the positional overloads and defaults to false. A false is non-null, so a positional call that does not set it can silently override enableJavaScript: true coming from .percy.yml.

• Suggestion: Model enableJavaScript as a Boolean (nullable) in the options map so an unset value is null and falls through to config, or document this limitation in the Javadoc. Pre-existing API tension; not verdict-failing.

• File: src/test/java/io/percy/selenium/SdkTest.java:1216-1218

• Severity: Low

• Reviewer: stack:code-review

• Issue: The test stubs request(...) with any(JSONObject.class) and never asserts on the POST body. (Note: this SDK intentionally posts the raw per-call options to the CLI — the documented cross-SDK pattern — so this is a coverage observation, not a defect.)

• Suggestion: Optional: add an ArgumentCaptor<JSONObject> on request(...) if future behavior ever depends on the posted payload. Non-blocking.

• File: src/main/java/io/percy/selenium/Percy.java:380-383

• Severity: Low

• Reviewer: stack:code-review

• Issue: The shallow copy carries org.json-typed values (e.g. JSONArray) from config into a map that elsewhere holds Java-native types (e.g. List<Integer> for widths). Harmless for scalar config keys (enableJavaScript, percyCSS); a latent type-mismatch only if config ever supplies a structured key like widths.

• Suggestion: Add a clarifying comment; no functional change needed for the keys in scope.

• File: src/test/java/io/percy/selenium/SdkTest.java:1237-1240

• Severity: Low

• Reviewer: stack:code-review

• Issue: The test extracts the serialize argument via substring/indexOf string slicing, which is fragile if buildSnapshotJS formatting changes.

• Suggestion: Optional: capture/verify the serialized map more directly. Non-blocking.

Note: The reviewer raised a Critical that postSnapshot is passed raw options rather than mergedOptions. Per the established cross-SDK design, the SDK intentionally posts raw per-call options to the CLI (the CLI applies .percy.yml config server-side); this is the intended pattern and is explicitly not verdict-failing for this review.

---

Verdict: PASS — Merge logic is correct and well-guarded with a focused precedence test; remaining items are Medium/Low and non-blocking (the lone Critical was the intended cross-SDK raw-options POST pattern).

[rishigupta1599]

Claude Code PR Review

PR: #310 • Head: 449acb4 • Reviewers: stack:code-review

Summary

Changes Percy.snapshot(...) to deep-merge .percy.yml CLI config (cliConfig.snapshot) with per-call options before DOM serialization. Adds two private helpers — jsonToJava (recursively converts org.json JSONObject→HashMap, JSONArray→ArrayList, scalars as-is, JSONObject.NULL→null) and deepMerge (recursive map merge where per-call override wins, nested maps recurse, arrays/scalars replace wholesale, and null override values are skipped so they never clobber config). The merged map feeds isCaptureResponsiveDOM, captureResponsiveDom, and getSerializedDOM. Adds two JUnit tests covering top-level precedence and nested deep-merge sibling preservation (mvn test 86/86).

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Security lens disabled per scope; no secrets in diff regardless.
High	Security	Authentication/authorization checks present	N/A	Security lens disabled; not applicable to a config-merge change.
High	Security	Input validation and sanitization	N/A	Security lens disabled.
High	Security	No IDOR — resource ownership validated	N/A	Security lens disabled.
High	Security	No SQL injection (parameterized queries)	N/A	Security lens disabled; no DB access.
High	Correctness	Logic is correct, handles edge cases	Pass	jsonToJava handles nested objects/arrays + JSONNULL; deepMerge correctly copies base, skips null overrides, recurses Map↔Map, replaces otherwise. Guarded cliConfig != null && has("snapshot") && !isNull("snapshot"). Per-call wins.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	No new try/catch; merge runs before the existing WebDriver try block. converted instanceof Map guard avoids ClassCastException.
High	Correctness	No race conditions or concurrency issues	Pass	All locals; no shared mutable state introduced.
Medium	Testing	New code has corresponding tests	Pass	Two tests: top-level precedence (config-only key survives, per-call wins) and nested deep merge (sibling leaf preserved).
Medium	Testing	Error paths and edge cases tested	Partial	Happy paths + nested merge covered; null-clobber-skip and array-replace not directly asserted. Non-blocking test gap.
Medium	Testing	Existing tests still pass (no regressions)	Pass	mvn test reported 86/86.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data fetching; merge is over small in-memory maps.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to SDK merge logic.
Medium	Quality	Follows existing codebase patterns	Pass	Consistent with cross-SDK config-merge approach; aligns with existing cliConfig.snapshot access elsewhere in file.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to config↔per-call merge plus its tests.
Low	Quality	Meaningful names, no dead code	Pass	jsonToJava/deepMerge/mergedOptions/baseOptions are clear; no dead code.
Low	Quality	Comments explain why, not what	Pass	Javadoc + inline comment explain merge precedence and the null-skip rationale.
Low	Quality	No unnecessary dependencies added	Pass	Uses existing java.util.* and org.json imports; no new deps.

Findings

No blocking findings. The recursive jsonToJava + deepMerge implementation is correct: base is copied, per-call override wins at the leaves, nested maps merge recursively (sibling config leaves preserved), arrays/scalars replace wholesale, and null per-call values are skipped so they cannot clobber real config values. Imports (java.util.*, org.json.JSONObject, org.json.JSONArray) are present and JSONObject.NULL is normalized to Java null.

Adjudicated (not a finding, per review instructions): the POST body in postSnapshot is built from the raw options (new JSONObject(options)) rather than mergedOptions. This is the INTENDED cross-SDK pattern — the CLI merges config server-side — and the merge here exists to make the SDK-side DOM-capture decisions (isCaptureResponsiveDOM, responsive vs. serialized DOM) reflect config. Not a regression.

Non-blocking: the null-skip branch and array-wholesale-replace branch of deepMerge are exercised indirectly but not directly asserted by a test. Pre-existing nearby code accesses cliConfig.getJSONObject("snapshot") without the new null/has guards in other methods — out of scope for this PR.

---

Verdict: PASS — correct, focused deep-merge with tests; no new regression.