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]

[Critical] Merged config not forwarded to the CLI POST payload

mergedOptions is built here and used for DOM capture, but the call at the end of snapshot() still reads return postSnapshot(domSnapshot, name, page.url(), options); with the original un-merged options. postSnapshot builds the /percy/snapshot body entirely from the map it receives (new JSONObject(options)), so any global .percy.yml key absent from the per-call map (widths, minHeight, percyCSS, etc.) is captured in the DOM but silently dropped from the payload sent to the CLI agent.

Suggestion: Pass mergedOptions to postSnapshot instead of options:

return postSnapshot(domSnapshot, name, page.url(), mergedOptions);

Reviewer: stack:code-review

[rishigupta1599]

[Low] Redundant cliConfig != null guard

cliConfig is initialized to new JSONObject() and only ever reassigned to another non-null JSONObject, so cliConfig != null is always true. Harmless but slightly misleading.

Suggestion: Drop the != null check (or document the non-null invariant); do not flip the field to null without auditing the other cliConfig.has(...) call sites.

Reviewer: stack:code-review

[rishigupta1599]

Claude Code PR Review

PR: #35 • Head: 52cd4d7 • Reviewers: stack:code-review

Summary

Merges global .percy.yml snapshot config (cliConfig.snapshot) into a mergedOptions HashMap with per-call options taking priority, then uses the merged map for DOM serialization (isCaptureResponsiveDOM, captureResponsiveDom, getSerializedDOM) in Percy.snapshot().

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Security lens disabled for this run.
High	Security	Authentication/authorization checks present	N/A	Security lens disabled for this run.
High	Security	Input validation and sanitization	N/A	Security lens disabled for this run.
High	Security	No IDOR — resource ownership validated	N/A	Security lens disabled for this run.
High	Security	No SQL injection (parameterized queries)	N/A	Security lens disabled for this run.
High	Correctness	Logic is correct, handles edge cases	Fail	Merged config reaches DOM serialization but postSnapshot (Percy.java:354) still passes the original un-merged options, so global-only keys (widths, minHeight, percyCSS, etc.) are dropped from the /percy/snapshot POST payload — feature is incomplete end-to-end.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Null-guards on cliConfig and options are present; no new exception handling needed.
High	Correctness	No race conditions or concurrency issues	Pass	Single-threaded snapshot path; mergedOptions is a fresh local map.
Medium	Testing	New code has corresponding tests	Fail	No test file changed; merge precedence, global-only keys, null options, and POST-payload propagation are all untested.
Medium	Testing	Error paths and edge cases tested	Fail	options == null guard and empty cliConfig paths have no coverage.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Change is additive to the capture path; existing isCaptureResponsiveDOM tests unaffected.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No DB/query work; single in-memory map merge.
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 existing cliConfig.snapshot access and HashMap option-building patterns in the file.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to the config-merge concern in snapshot().
Low	Quality	Meaningful names, no dead code	Pass	mergedOptions/snapshotConfig are clear; no dead code introduced.
Low	Quality	Comments explain why, not what	Pass	Inline comment states the merge precedence intent.
Low	Quality	No unnecessary dependencies added	Pass	Uses already-imported java.util.* (HashMap) and org.json; no new deps.

Findings

• File: src/main/java/io/percy/playwright/Percy.java:354

• Severity: Critical

• Reviewer: stack:code-review

• Issue: After building mergedOptions, the method still calls postSnapshot(domSnapshot, name, page.url(), options) with the original un-merged options. postSnapshot builds the /percy/snapshot POST body entirely from the map it receives (new JSONObject(options)). Global .percy.yml keys absent from the per-call map are therefore captured in the DOM but silently dropped from the payload sent to the CLI agent — a split-brain state that defeats half the feature.

• Suggestion: Pass mergedOptions instead of options: return postSnapshot(domSnapshot, name, page.url(), mergedOptions); (this line is outside the PR diff hunk, so the inline anchor is placed on the mergedOptions declaration).

• File: src/main/java/io/percy/playwright/Percy.java:319-332

• Severity: Medium

• Reviewer: stack:code-review

• Issue: No tests accompany the new merge logic. Untested scenarios: (1) global-only key flows into both DOM serialization and POST payload, (2) per-call key overrides global, (3) nested JSONArray/JSONObject values from cliConfig.snapshot survive new JSONObject(options) round-trips, (4) options == null with a populated cliConfig.snapshot, (5) empty cliConfig (the default new JSONObject()).

• Suggestion: Add JUnit tests mirroring the existing isCaptureResponsiveDOM cases in SDKTest, capturing the maps passed to getSerializedDOM/postSnapshot to assert merge precedence and full propagation.

• File: src/main/java/io/percy/playwright/Percy.java:321

• Severity: Low

• Reviewer: stack:code-review

• Issue: The guard cliConfig != null is always true — cliConfig is initialized to new JSONObject() and only ever reassigned to another non-null JSONObject. The check is harmless but slightly misleading.

• Suggestion: Drop the redundant != null check or document the non-null invariant; do not flip the field to null without auditing the other cliConfig.has(...) call sites.

---

Verdict: FAIL — the config merge is not wired through to the CLI POST payload (postSnapshot still uses un-merged options); fix before merge and add coverage.

[rishigupta1599]

Claude Code PR Review

PR: #35 • Head: e108a71 • Reviewers: stack:code-review

Summary

Merges .percy.yml snapshot config options with per-call snapshot options (per-call wins) before serializing the DOM, so config-only keys like enableJavaScript/percyCSS reach PercyDOM.serialize(...). The new commit adds a unit test asserting merge precedence: config-only keys survive and per-call values override config values.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Per scope, security lens skipped; no secrets in diff.
High	Security	Authentication/authorization checks present	N/A	Security lens skipped; not applicable to a config-merge change.
High	Security	Input validation and sanitization	N/A	Security lens skipped.
High	Security	No IDOR — resource ownership validated	N/A	Security lens skipped.
High	Security	No SQL injection (parameterized queries)	N/A	Security lens skipped; no DB access.
High	Correctness	Logic is correct, handles edge cases	Pass	Config copied first, then options.putAll makes per-call win. Null-guards on cliConfig, snapshot node, and options.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Merge happens outside the try/catch; existing capture try/catch unchanged. No new swallowing.
High	Correctness	No race conditions or concurrency issues	Pass	Method-local mergedOptions; no shared mutable state introduced.
Medium	Testing	New code has corresponding tests	Pass	perSnapshotOptionsOverrideConfigInSerializedOptions covers both merge directions via the serialize JS payload.
Medium	Testing	Error paths and edge cases tested	Pass (minor)	Happy path + override path covered. No explicit test for null/absent cliConfig.snapshot, but guards are simple and exercised by existing tests.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Change is additive; isCaptureResponsiveDOM already reads cliConfig internally so passing mergedOptions is idempotent.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	One small map build per snapshot; negligible.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to an SDK snapshot path.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors readiness-merge precedence pattern already in the file; uses same JSONObject guards.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to config<->options merge for serialization plus its test.
Low	Quality	Meaningful names, no dead code	Pass	mergedOptions/snapshotConfig are clear; test helper setPrivateField is used.
Low	Quality	Comments explain why, not what	Pass	Comment states precedence intent; test javadoc documents expected merge behavior.
Low	Quality	No unnecessary dependencies added	Pass	No new deps; HashMap covered by import java.util.*.

Findings

No Fail items. One non-blocking observation:

• File: src/main/java/io/percy/playwright/Percy.java:353
• Severity: Low
• Reviewer: stack:code-review
• Issue: postSnapshot(...) is still called with the raw options rather than mergedOptions, so the posted snapshot options do not carry the .percy.yml config keys.
• Suggestion: Intended cross-SDK pattern — the CLI merges config into posted options server-side, so the SDK posts raw per-call options on purpose. No change required; recorded for clarity.

---

Verdict: PASS — focused, correctly-ordered config/options merge with a targeted precedence test; security lens intentionally out of scope.

[rishigupta1599]

Claude Code PR Review

PR: #35 • Head: e2b174c • Reviewers: stack:code-review

Summary

Replaces the previous flat/shallow handling of .percy.yml snapshot config with a recursive deep merge of CLI config and per-call snapshot options. Adds jsonToJava (org.json -> plain Java collections), deepMerge (recursive, per-call wins at leaves, arrays/scalars replace wholesale, null per-call values skipped), and castToStringMap. The merged options feed the DOM-serialization path (isCaptureResponsiveDOM, captureResponsiveDom, getSerializedDOM). Two JUnit tests cover nested-block merging, leaf override, array survival, null-skip, and end-to-end precedence into PercyDOM.serialize.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Per scope: security lens skipped. No secrets in diff.
High	Security	Authentication/authorization checks present	N/A	Security skipped; not applicable to a client SDK merge helper.
High	Security	Input validation and sanitization	N/A	Security skipped; inputs are local config/option maps.
High	Security	No IDOR — resource ownership validated	N/A	Security skipped; no resource access in diff.
High	Security	No SQL injection (parameterized queries)	N/A	Security skipped; no DB/query code.
High	Correctness	Logic is correct, handles edge cases	Pass	Deep merge verified: copies base, skips null overrides, recurses only when both sides are Maps, replaces arrays/scalars wholesale. jsonToJava maps JSONObject.NULL->null. No input mutation or shared-mutable aliasing (fresh maps via recursion). cliConfig is field-initialized to new JSONObject(), so null guards are belt-and-suspenders.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	New helpers are pure; no try/catch needed. Existing snapshot try/catch unchanged. converted instanceof Map guard prevents bad casts.
High	Correctness	No race conditions or concurrency issues	Pass	Pure, stateless static helpers operating on locals; no shared state introduced.
Medium	Testing	New code has corresponding tests	Pass	deepMergesConfigWithPerCallOptions (unit) + perSnapshotOptionsOverrideConfigInSerializedOptions (e2e into serialize JS) cover nested merge, leaf override, array survival, null-skip, config-only key retention.
Medium	Testing	Error paths and edge cases tested	Pass	Null per-call value and config-only-key cases covered. Minor gap: no explicit test for array-replace-on-override or scalar-over-map type-conflict — non-blocking.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Imports (java.util.*, Arrays, Mockito spy/captor, JUnit assertions) all present; tests compile. Merge path is additive; readiness merge unchanged.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Single recursive pass over small config/option maps; no I/O.
Medium	Performance	Long-running tasks use background jobs	N/A	No long-running work introduced.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors the existing resolveReadinessConfig merge precedence pattern; consistent with cross-SDK config<->per-call semantics.
Medium	Quality	Changes are focused (single concern)	Pass	Diff scoped to the config/per-call merge and its tests.
Low	Quality	Meaningful names, no dead code	Pass	jsonToJava / deepMerge / castToStringMap are clear; no dead code.
Low	Quality	Comments explain why, not what	Pass	Comments explain merge precedence rationale and null-skip intent.
Low	Quality	No unnecessary dependencies added	Pass	Uses existing org.json + java.util only.

Findings

No blocking findings.

Notes (non-blocking, intentional/pre-existing per adjudication):

• postSnapshot (Percy.java:355) and the POST body (new JSONObject(options), Percy.java:479) intentionally use the raw per-call options, not mergedOptions — this is the established cross-SDK pattern where the CLI merges .percy.yml config server-side. Not a finding.
• Minor test-coverage gap (array-replace-on-override; scalar-vs-map type conflict) — non-blocking.

---

Verdict: PASS — deep-merge implementation is correct, focused, and well-tested; no new regression.