feat: add mergeSnapshotOptions utility to @percy/sdk-utils

[rishigupta1599]

Summary

• Adds mergeSnapshotOptions(options) to @percy/sdk-utils that centralizes the .percy.yml config merge logic
• Merges percy.config.snapshot options with per-snapshot options (snapshot options take priority)
• This was previously duplicated as inline code across all 10 Percy JS SDKs

Test plan

• Unit tests added for mergeSnapshotOptions covering: merge behavior, priority, undefined config, no options
• All 5 new tests pass
• Once published, all SDK PRs will bump @percy/cli version to use this utility

🤖 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) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[rishigupta1599]

Claude Code PR Review

PR: #2213 • Head: da98300 • Reviewers: stack:code-reviewer

Summary

Adds a new mergeSnapshotOptions utility to @percy/sdk-utils that shallow-merges .percy.yml config-level snapshot options (percy.config.snapshot) with per-snapshot call-site options, giving per-snapshot options priority. Wires the new export into src/index.js and adds 5 unit tests.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None present.
High	Security	Authentication/authorization checks present	N/A	Pure data-merge utility.
High	Security	Input validation and sanitization	N/A	No external/untrusted input.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No DB access.
High	Correctness	Logic is correct, handles edge cases	Pass	Matches stated intent; undefined options and undefined/missing config.snapshot are guarded and tested. Shallow-vs-deep merge is a design concern (see Findings), not a logic error.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	No throwing paths; optional chaining guards missing state.
High	Correctness	No race conditions or concurrency issues	Pass	Synchronous, no shared mutable state.
Medium	Testing	New code has corresponding tests	Pass	5 tests cover merge, priority, default, and undefined-config cases.
Medium	Testing	Error paths and edge cases tested	Partial	Missing: null per-snapshot value clobbering config; percy.config absent (pre-isPercyEnabled).
Medium	Testing	Existing tests still pass (no regressions)	Pass	Additive change; export wiring correct.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No I/O.
Medium	Performance	Long-running tasks use background jobs	N/A	Synchronous trivial op.
Medium	Quality	Follows existing codebase patterns	Partial	Same conceptual op in core/src/snapshot.js uses PercyConfig.merge (deep); this uses a shallow spread. Name also collides with a private mergeSnapshotOptions in core.
Medium	Quality	Changes are focused (single concern)	Pass	Single, well-scoped utility.
Low	Quality	Meaningful names, no dead code	Partial	Clear name, but no callers exist yet — exported API with no consumer.
Low	Quality	Comments explain why, not what	Pass	Header comment documents merge precedence.
Low	Quality	No unnecessary dependencies added	Pass	No new deps.

Findings

• File: packages/sdk-utils/src/merge-snapshot-options.js:7

• Severity: Medium

• Reviewer: stack:code-reviewer

• Issue: return { ...configOptions, ...options } is a shallow merge. config.snapshot can hold nested objects (e.g. discovery: {...}). A caller passing a nested key (e.g. { discovery: { networkIdleTimeout: 100 } }) clobbers the entire config discovery block instead of deep-merging. packages/core/src/snapshot.js performs the same conceptual merge via PercyConfig.merge (deep). No callers exist yet, so this is a latent design risk rather than a current bug — but it's a public API, so the semantics are hard to change later.

• Suggestion: Either use PercyConfig.merge([configOptions, options]) to match core's deep-merge semantics, or document explicitly (JSDoc) that this utility only merges flat/top-level keys and callers must not pass nested overrides.

• File: packages/sdk-utils/src/merge-snapshot-options.js:5

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: Calling mergeSnapshotOptions() relies on options being undefined and spreading undefined. Works in JS but is implicit and not TypeScript-strict friendly.

• Suggestion: Use a default parameter: export function mergeSnapshotOptions(options = {}) {.

• File: packages/sdk-utils/src/merge-snapshot-options.js (file-level)

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: Name collision with the private mergeSnapshotOptions(prev, next) in packages/core/src/snapshot.js:299, which has a different signature and purpose. Confusing for maintainers reading both packages.

• Suggestion: Consider a distinct name such as applySnapshotConfig / mergeSnapshotConfig.

• File: packages/sdk-utils/src/merge-snapshot-options.js (file-level)

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: The new export has no callers anywhere in the monorepo (confirmed via repo-wide grep) — only the test references it. An exported public API with no consumer can't be validated end-to-end and risks shipping a signature that doesn't fit real needs.

• Suggestion: Land alongside (or just ahead of) the consuming code, or note in the PR description which follow-up will use it.

• File: packages/sdk-utils/test/index.test.js (undefined-config tests)

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: Two tests save/restore utils.percy.config manually; if the assertion or call throws, the restore line never runs, leaking mutated state into later tests.

• Suggestion: Restore in an afterEach, or wrap the mutation in try/finally.

• File: packages/sdk-utils/test/index.test.js (coverage gaps)

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: Missing coverage for (a) a per-snapshot value of null/undefined overwriting a config value, and (b) mergeSnapshotOptions invoked before isPercyEnabled() populates percy.config (the exact scenario the percy?.config?.snapshot guard exists for).

• Suggestion: Add tests for both to lock in the guard behavior and the null-override semantics.

---

Verdict: PASS — Small, focused, well-tested additive utility with correct export wiring. No High/Critical issues. The shallow-merge semantics (vs. core's deep PercyConfig.merge) and the name collision are worth addressing before any consumer adopts this public API, but neither blocks merge.

[rishigupta1599]

Claude Code PR Review

PR: #2213 • Head: 33cf5cb • Reviewers: stack:code-review

Summary

Adds a mergeSnapshotOptions utility to @percy/sdk-utils (PER-8053) that shallow-merges .percy.yml config snapshot options with per-snapshot options (per-snapshot wins), plus the index.js export and unit tests. Security lens skipped per orchestrator policy.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Security lens skipped per orchestrator policy.
High	Security	Authentication/authorization checks present	N/A	Security lens skipped per orchestrator policy.
High	Security	Input validation and sanitization	N/A	Security lens skipped per orchestrator policy.
High	Security	No IDOR — resource ownership validated	N/A	Security lens skipped per orchestrator policy.
High	Security	No SQL injection (parameterized queries)	N/A	Security lens skipped per orchestrator policy.
High	Correctness	Logic is correct, handles edge cases	Pass	Shallow merge { ...configOptions, ...options } with options = {} default and percy?.config?.snapshot || {} guard handles missing config and missing args (merge-snapshot-options.js:39-41).
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	No throwing paths; optional chaining + || {} fallback covers undefined config (merge-snapshot-options.js:40).
High	Correctness	No race conditions or concurrency issues	Pass	Pure synchronous function reading module-level percy singleton; no shared mutable state.
Medium	Testing	New code has corresponding tests	Pass	5 new specs cover merge, precedence, default param, and undefined-config paths (index.test.js:54-97).
Medium	Testing	Error paths and edge cases tested	Pass	Undefined config.snapshot with and without options both tested (index.test.js:78-96).
Medium	Testing	Existing tests still pass (no regressions)	Pass	Additive change; new describe block appended, no existing specs touched. Local harness has an unrelated stale-dist/missing-replay.js build error preventing a run; not caused by this PR.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Single in-memory object spread; no I/O.
Medium	Performance	Long-running tasks use background jobs	N/A	Synchronous pure utility; not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors percy.config?.snapshot?.readiness access in serialize-dom.js:15; dual named+default export matches sibling modules.
Medium	Quality	Changes are focused (single concern)	Pass	One util, one export line, one test block.
Low	Quality	Meaningful names, no dead code	Pass	Clear names; no dead code.
Low	Quality	Comments explain why, not what	Pass	Header comment documents the intentional shallow-merge rationale (parity + server-side deep merge), addressing prior review feedback (merge-snapshot-options.js:3-12).
Low	Quality	No unnecessary dependencies added	Pass	Only imports existing local percy-info.js.

Findings

No failing items. The two prior review comments (default parameter and documented intentional shallow merge) are resolved in head commit 33cf5cb: the options = {} default is present (merge-snapshot-options.js:39) and the shallow-merge rationale is documented (merge-snapshot-options.js:3-12).

One non-blocking observation (not a finding): the local npm test run failed due to an unrelated stale dist/ and missing @percy/cli-exec/dist/replay.js in the workspace, which is an environment/build artifact issue independent of this diff.

---

Verdict: PASS

[rishigupta1599]

PR: #2213 • Head: 465b72e • Reviewers: stack:code-review

Summary

Adds a self-contained mergeSnapshotOptions utility to @percy/sdk-utils that deep-merges .percy.yml config snapshot options with per-snapshot options. Nested plain objects merge recursively (a per-snapshot override of one nested key keeps the config's sibling keys), per-snapshot values win at the leaves, and arrays are replaced rather than concatenated. The util is exported from the package index. Resolves the earlier "[Medium] shallow merge drops nested config keys" finding; PercyConfig.merge itself can't be imported (sdk-utils is CommonJS, @percy/config is ESM-only and would break the browser bundle), so the self-contained deepMerge reproduces its effective semantics.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Pure data-merge util; no secrets. Security lens skipped per scope.
High	Security	Authentication/authorization checks present	N/A	Not an auth surface.
High	Security	Input validation and sanitization	N/A	No external input boundary; in-process option merge.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No DB access.
High	Correctness	Logic is correct, handles edge cases	Pass	deepMerge recurses only when both sides are plain non-array objects; arrays/scalars/null/functions replace wholesale. isPlainObject correctly excludes null/arrays. Default param options = {} and percy?.config?.snapshot || {} cover empty/undefined cases. Effective semantics match PercyConfig.merge.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	No try/catch needed; no throwing paths. Optional chaining guards missing config.
High	Correctness	No race conditions or concurrency issues	Pass	Synchronous, pure function over a module-level config snapshot.
Medium	Testing	New code has corresponding tests	Pass	7 specs: base merge, per-snapshot priority, no-options, undefined config.snapshot (with/without options), nested deep merge keeping siblings, array replacement.
Medium	Testing	Error paths and edge cases tested	Pass	Undefined config.snapshot and empty-options paths covered. Minor gap: no explicit test for null/scalar override replacing a config object, or function values — covered by logic but not asserted.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Additive change; new describe block appended; export added without altering existing exports.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	In-memory recursion bounded by option-tree depth; no I/O.
Medium	Performance	Long-running tasks use background jobs	N/A	Synchronous trivial work.
Medium	Quality	Follows existing codebase patterns	Pass	Matches sdk-utils module style: default export + named export, imported and re-exported in index.js alongside siblings.
Medium	Quality	Changes are focused (single concern)	Pass	One new util file, one export wiring, one test block. No scope creep.
Low	Quality	Meaningful names, no dead code	Pass	deepMerge, isPlainObject, mergeSnapshotOptions are descriptive; no dead code.
Low	Quality	Comments explain why, not what	Pass	Comments document merge semantics and the per-snapshot-wins / array-replace rules — the load-bearing rationale.
Low	Quality	No unnecessary dependencies added	Pass	Zero new deps; only imports the existing percy-info.js.

Findings

No blocking findings.

Non-blocking observation (Low, not anchored): deepMerge copies nested objects/arrays that exist only on base (config) into the result by reference rather than deep-cloning them. The top-level config object is never mutated, so percy.config is safe, but a caller that later mutates a returned nested branch could mutate the shared config sub-object. This matches PercyConfig.merge's effective semantics and the existing usage pattern (returned options are consumed, not mutated), so no change is required.

---

Verdict: PASS — correct, well-scoped deep merge with solid test coverage; matches the intended PercyConfig.merge semantics without the ESM/bundle hazard.