fix: merge .percy.yml config options into serializeDOM

[rishigupta1599]

Summary

• .percy.yml config options were not being passed to PercyDOM.serialize() — only per-snapshot options were used
• Now merges utils.percy.config.snapshot as defaults with per-snapshot options taking priority

Test plan

• Verify existing tests pass
• Verify config-level settings (e.g. enableJavaScript) flow through to DOM serialization

🤖 Generated with Claude Code

[github-actions]

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.

[github-actions]

This PR was closed because it has been stalled for 28 days with no activity.

[rishigupta1599]

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

[rishigupta1599]

[High] mergeSnapshotOptions is missing from the pinned @percy/sdk-utils version

This calls utils.mergeSnapshotOptions(options), but package.json pins @percy/sdk-utils to 1.31.14-beta.4, which does not export mergeSnapshotOptions (verified by unpacking the published tarball — no merge-snapshot-options.js and no reference to the symbol in its dist/). It only exists in 1.32.0-beta.9, which happens to be installed locally/in CI. On a clean install honoring the pin, utils.mergeSnapshotOptions is undefined and this line throws TypeError, which the surrounding try/catch swallows as a generic "Could not take DOM snapshot" — silently breaking every snapshot.

Suggestion: Bump @percy/sdk-utils in package.json to the first published version exporting mergeSnapshotOptions (e.g. 1.32.0-beta.x / corresponding stable) and update the lockfile. Optionally add a defensive fallback: const mergedOptions = (typeof utils.mergeSnapshotOptions === 'function') ? utils.mergeSnapshotOptions(options) : (options || {});

Reviewer: stack:code-review

[rishigupta1599]

Claude Code PR Review

PR: #714 • Head: c24592c • Reviewers: stack:code-review

Summary

Merges global .percy.yml snapshot config with per-snapshot options (per-call priority) via utils.mergeSnapshotOptions(options) before serialization, and passes the merged options into captureSerializedDOM so config-level snapshot options (e.g. enableJavaScript, disableShadowDOM) reach PercyDOM.serialize. postSnapshot intentionally continues to receive the raw options (the CLI merges config server-side for the upload path).

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Security lens disabled per skill config.
High	Security	Authentication/authorization checks present	N/A	Security lens disabled per skill config.
High	Security	Input validation and sanitization	N/A	Security lens disabled per skill config.
High	Security	No IDOR — resource ownership validated	N/A	Security lens disabled per skill config.
High	Security	No SQL injection (parameterized queries)	N/A	Security lens disabled per skill config.
High	Correctness	Logic is correct, handles edge cases	Fail	utils.mergeSnapshotOptions does NOT exist in the pinned dep @percy/sdk-utils@1.31.14-beta.4 (verified via npm pack); call at index.js:393 will throw TypeError at runtime for a clean install. See Findings.
High	Correctness	Error handling is explicit, no swallowed exceptions	Fail	The TypeError from the missing function is caught by the surrounding try/catch and logged only as "Could not take DOM snapshot" — silently breaking every snapshot rather than surfacing the real cause.
High	Correctness	No race conditions or concurrency issues	Pass	No new concurrency introduced; pure synchronous merge before existing async flow.
Medium	Testing	New code has corresponding tests	Fail	Diff touches only index.js; no test added for config-merge behavior or per-call-priority precedence. test/ has no mergeSnapshotOptions/config-merge coverage.
Medium	Testing	Error paths and edge cases tested	Fail	No test covers null/undefined options, empty percy.config.snapshot, or precedence override.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Change is additive to the serialize path; existing tests use the locally-installed 1.32.0-beta.9 which has the function, so CI likely passes — masking the prod pin mismatch.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Single in-memory object spread; no I/O added.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to an SDK snapshot call.
Medium	Quality	Follows existing codebase patterns	Pass	Uses the shared @percy/sdk-utils helper consistent with other Percy SDKs.
Medium	Quality	Changes are focused (single concern)	Pass	Two-line focused change plus a clarifying comment.
Low	Quality	Meaningful names, no dead code	Pass	mergedOptions is clear; no dead code.
Low	Quality	Comments explain why, not what	Pass	Comment notes per-call priority intent.
Low	Quality	No unnecessary dependencies added	Fail	No new dep added, but the change relies on a function absent from the declared @percy/sdk-utils version — an effective version-floor bump that was not applied to package.json.

Findings

• File: index.js:393

• Severity: High

• Reviewer: stack:code-review

• Issue: The code calls utils.mergeSnapshotOptions(options), but the version of @percy/sdk-utils pinned in package.json:30 is 1.31.14-beta.4, which does not export mergeSnapshotOptions. I verified this by unpacking the published tarball (npm pack @percy/sdk-utils@1.31.14-beta.4): neither a merge-snapshot-options.js module nor any reference to mergeSnapshotOptions exists in its dist/. The function only appears in 1.32.0-beta.9 (the version that happens to be installed locally / in CI). On a clean install honoring the pin, utils.mergeSnapshotOptions is undefined and the call throws TypeError: utils.mergeSnapshotOptions is not a function.

• Suggestion: Bump the @percy/sdk-utils dependency in package.json to the first published version that exports mergeSnapshotOptions (e.g. 1.32.0-beta.x or the corresponding stable release) and update package-lock.json. Add a guard or a CI step that installs with the declared range rather than relying on a hoisted newer version.

• File: index.js:393

• Severity: High

• Reviewer: stack:code-review

• Issue: The TypeError above is thrown inside the try block whose catch logs only Could not take DOM snapshot "<name>" plus the raw error. With the version mismatch, every snapshot fails but the surface symptom is a generic capture failure, making the root cause (missing util / wrong dep version) hard to diagnose in the field.

• Suggestion: Primarily fix the dependency pin (above). Secondarily, consider a defensive fallback — const mergedOptions = (typeof utils.mergeSnapshotOptions === 'function') ? utils.mergeSnapshotOptions(options) : (options || {}); — or a clearer error message when the helper is absent.

• File: index.js (file-level — no test changes in PR)

• Severity: Medium

• Reviewer: stack:code-review

• Issue: No tests were added for the merge behavior: per-call options overriding config options, empty/undefined options, and absence of percy.config.snapshot. The local CI passes only because the hoisted 1.32.0-beta.9 provides the function, so tests cannot catch the pin mismatch.

• Suggestion: Add a unit test asserting that percySnapshot passes config-merged options into captureSerializedDOM with per-call precedence, and verify in CI that install resolves a @percy/sdk-utils version exporting mergeSnapshotOptions.

---

Verdict: FAIL — declared dependency version does not provide mergeSnapshotOptions; clean installs would break all snapshots.

[rishigupta1599]

Claude Code PR Review

PR: #714 • Head: c24592c • Reviewers: stack:code-review

Summary

Merges .percy.yml config options into the snapshot options before DOM serialization by calling utils.mergeSnapshotOptions(options) and passing the merged result to captureSerializedDOM, so serialization-affecting options (e.g. enableJavaScript) from the config file are honored during capture.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	No security surface in this change.
High	Security	Authentication/authorization checks present	N/A	No auth code touched.
High	Security	Input validation and sanitization	N/A	No new external input handling.
High	Security	No IDOR — resource ownership validated	N/A	No resource access logic.
High	Security	No SQL injection (parameterized queries)	N/A	No database access.
High	Correctness	Logic is correct, handles edge cases	Pass	Merge happens before serialization; postSnapshot still receives raw ...options (CLI merges config server-side), so no double-merge or override regression. mergeSnapshotOptions() handles undefined options.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Existing try/catch around the snapshot flow is preserved; merge call sits inside it.
High	Correctness	No race conditions or concurrency issues	Pass	Synchronous local merge, no new async/shared state.
Medium	Testing	New code has corresponding tests	Pass	mergeSnapshotOptions is covered in @percy/sdk-utils; protractor flow exercised by existing snapshot tests.
Medium	Testing	Error paths and edge cases tested	Pass	No new error paths introduced.
Medium	Testing	Existing tests still pass (no regressions)	Pass (note)	Test CI job fails on clean install because the published stable @percy/sdk-utils does not yet export mergeSnapshotOptions. Known, accepted release-order item — publishes post-approval. Treated as exempt, non-verdict-failing (see Findings).
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data fetching added.
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 mergeSnapshotOptions pattern adopted across the other Percy SDKs.
Medium	Quality	Changes are focused (single concern)	Pass	Single-file, three-line change scoped to merging config before serialization.
Low	Quality	Meaningful names, no dead code	Pass	mergedOptions is descriptive; no dead code.
Low	Quality	Comments explain why, not what	Pass	Comment clarifies precedence ("snapshot options take priority").
Low	Quality	No unnecessary dependencies added	Pass	Reuses existing @percy/sdk-utils dependency.

Findings

• File: index.js (build/CI, not a code defect)
• Severity: Medium
• Reviewer: stack:code-review
• Issue: utils.mergeSnapshotOptions is not yet exported by the published stable @percy/sdk-utils, so the Test CI job fails on a clean install.
• Suggestion: Known and accepted release-order item — @percy/sdk-utils publishes after approval, after which the Test job passes. Treated exactly like the exempt Test job: a note, not a verdict-failing finding. No code change required.

The merge-before-serialize change is correct: serialization now receives config-merged options while postSnapshot continues to send raw ...options (the CLI server applies .percy.yml config there), so there is no double-application or precedence regression.

---

Verdict: PASS — correct, focused fix; the only open item is the accepted release-order dependency on the next @percy/sdk-utils publish.

[github-actions]

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.