fix: merge .percy.yml config options into serializeDOM

[rishigupta1599]

Summary

• Previously only pseudoClassEnabledElements was manually forwarded from .percy.yml config
• Now all config.snapshot options are merged as defaults, with per-snapshot options taking priority
• The manual addPseudoClassEnabledElements call is now superseded by the general merge

Test plan

• Verify existing tests pass
• Verify config-level settings (e.g. enableJavaScript, disableShadowDOM) 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) — 2 inline finding(s). Full report in the PR comment below. Verdict: Failed - see PR comment.

[rishigupta1599]

[Critical] utils.mergeSnapshotOptions is not exported by @percy/sdk-utils

utils here is @percy/sdk-utils, whose index exports do not include mergeSnapshotOptions (it lives in @percy/core/src/snapshot.js). At runtime this is undefined(...) and throws TypeError: utils.mergeSnapshotOptions is not a function. The error is swallowed by the surrounding try/catch and logged as a generic "Could not take DOM snapshot", so every snapshot silently fails.

Suggestion: Either export mergeSnapshotOptions from @percy/sdk-utils (and bump the dependency), or merge inline: const mergedOptions = { ...(utils.percy?.config?.snapshot || {}), ...options };

Reviewer: stack:code-review

[rishigupta1599]

[High] Merge applied only to serialize; readiness gate and post payload still use raw options

...mergedOptions is spread here, but the readiness block and const { readiness, ...forwardOpts } = options still read the un-merged options. The old addPseudoClassEnabledELements mutated options in place so config values reached postSnapshot; now config-level snapshot settings (including pseudoClassEnabledElements) are dropped from the post payload.

Suggestion: Derive forwardOpts and the readiness inputs from mergedOptions so merged config flows consistently to readiness, serialize, and post.

Reviewer: stack:code-review

[rishigupta1599]

Claude Code PR Review

PR: #1151 • Head: 53d450d • Reviewers: stack:code-review

Summary

Replaces the manual addPseudoClassEnabledELements(options) call with utils.mergeSnapshotOptions(options) to merge global .percy.yml config.snapshot defaults with per-snapshot options (per-call priority), spreading the result (...mergedOptions) into PercyDOM.serialize() before serialization.

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	Fail	utils.mergeSnapshotOptions is not exported by @percy/sdk-utils; call throws TypeError, silently failing every snapshot. See Findings. index.js:79.
High	Correctness	Error handling is explicit, no swallowed exceptions	Fail	The new TypeError is swallowed by the broad try/catch (line 167) and surfaces only as a generic "Could not take DOM snapshot" — masking total breakage.
High	Correctness	No race conditions or concurrency issues	Pass	No new concurrency introduced; PercyDOM capture-once pattern preserved.
Medium	Testing	New code has corresponding tests	Fail	No tests added/updated; only index.js changed. Merge behavior and config flow-through are unverified.
Medium	Testing	Error paths and edge cases tested	Fail	No coverage for missing-config, config-only, or option-override precedence cases.
Medium	Testing	Existing tests still pass (no regressions)	Fail	Cannot confirm; given the undefined-function call, acceptance snapshots would error at runtime.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	No data-fetching changes.
Medium	Performance	Long-running tasks use background jobs	N/A	Client-side SDK; not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	Reusing a shared util is the right direction if the export existed.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to the config-merge concern.
Low	Quality	Meaningful names, no dead code	Fail	mergedOptions is only used at the serialize() spread; the readiness block and forwardOpts (line 156) still read raw options, so the merge is half-applied.
Low	Quality	Comments explain why, not what	Pass	Added comment states intent (per-snapshot priority).
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies.

Findings

• File: addon-test-support/@percy/ember/index.js:79

• Severity: Critical

• Reviewer: stack:code-review

• Issue: utils.mergeSnapshotOptions does not exist. utils is @percy/sdk-utils, whose index.js exports only logger, percy, request, isPercyEnabled, waitForPercyIdle, fetchPercyDOM, postSnapshot, postComparison, flushSnapshots, captureAutomateScreenshot, postBuildEvents, getResponsiveWidths, clampIframeDepth, waitForReadyScript, getReadinessConfig, isReadinessDisabled, runReadinessGate (plus iframe-depth constants). mergeSnapshotOptions lives in @percy/core/src/snapshot.js, not in sdk-utils. At runtime utils.mergeSnapshotOptions(options) evaluates to undefined(...) → TypeError: utils.mergeSnapshotOptions is not a function. Because the call is inside the try block, the error is caught at line 167 and logged as a generic "Could not take DOM snapshot", so every snapshot silently fails — a complete regression of the SDK.

• Suggestion: Either (a) add and export mergeSnapshotOptions from @percy/sdk-utils (and bump the @percy/sdk-utils dependency to a version that includes it) before relying on it here, or (b) implement the merge inline, e.g. const mergedOptions = { ...(utils.percy?.config?.snapshot || {}), ...options };. Verify the symbol is actually resolvable from the pinned dependency version.

• File: addon-test-support/@percy/ember/index.js:136 (and lines 97-105, 156)

• Severity: High

• Reviewer: stack:code-review

• Issue: Even if mergeSnapshotOptions resolved, the merge is only applied to the PercyDOM.serialize() spread (...mergedOptions). The readiness gate (lines 97-105) and forwardOpts (const { readiness: _readiness, ...forwardOpts } = options;, line 156) still destructure the raw options. The old addPseudoClassEnabledELements(options) mutated options in place, so config-sourced values reached postSnapshot. Now they do not — config-level snapshot settings (including the previously-working pseudoClassEnabledElements) are dropped from the postSnapshot payload, narrowing rather than fixing the stated goal of "merge all config.snapshot options".

• Suggestion: Derive forwardOpts and the readiness inputs from mergedOptions rather than options (e.g. const { readiness: _readiness, ...forwardOpts } = mergedOptions; and use mergedOptions?.readiness/mergedOptions in the readiness block) so the merged config flows consistently to readiness, serialize, and post.

• File: addon-test-support/@percy/ember/index.js (file-level)

• Severity: Medium

• Reviewer: stack:code-review

• Issue: No tests accompany the behavior change. There is no coverage asserting that config defaults are applied, that per-snapshot options override config, or that the prior pseudoClassEnabledElements path still forwards to the post payload.

• Suggestion: Add acceptance tests under tests/acceptance/ stubbing utils.percy.config.snapshot and asserting both the serialize options and the postSnapshot payload reflect the merged result with correct precedence.

---

Verdict: FAIL — depends on a mergeSnapshotOptions export that @percy/sdk-utils does not provide; the call throws and silently breaks all snapshots.

[rishigupta1599]

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

[rishigupta1599]

[High] Config-merged options never reach postSnapshot

mergedOptions is spread only into PercyDOM.serialize (L129). The posted payload is built from forwardOpts, which is destructured from the raw options (const { readiness: _readiness, ...forwardOpts } = options; at L149) — so .percy.yml config.snapshot keys not passed per-snapshot are dropped from the post body. On base master the old helper mutated options in place, so the merged value reached both serialize and post. The existing test uses pseudoClassEnabledElements from percy config when option is not passed (tests/acceptance/index-test.js:129-140) asserts the /percy/snapshot post body and will now fail. This is a genuine regression, separate from the sdk-utils publish ordering.

Suggestion: Derive the posted options from the merged set:

const { readiness: _readiness, ...forwardOpts } = mergedOptions;

And base the readiness checks (L90, L96-98) on mergedOptions too for consistency.

Reviewer: stack:code-review

[rishigupta1599]

Claude Code PR Review

PR: #1151 • Head: f08fb0c • Reviewers: stack:code-review

Summary

Replaces the narrow local helper addPseudoClassEnabledELements (which patched only pseudoClassEnabledElements from .percy.yml onto snapshot options) with a general utils.mergeSnapshotOptions(options) from @percy/sdk-utils, merging all .percy.yml config.snapshot keys as defaults while per-snapshot options take priority. Lint is now green (dead helper removed).

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Out of scope for this change (no secrets touched).
High	Security	Authentication/authorization checks present	N/A	Out of scope; no auth surface.
High	Security	Input validation and sanitization	N/A	Out of scope; pure option-merge logic.
High	Security	No IDOR — resource ownership validated	N/A	Out of scope; no resource access.
High	Security	No SQL injection (parameterized queries)	N/A	Out of scope; no DB/query code.
High	Correctness	Logic is correct, handles edge cases	Fail	mergedOptions reaches PercyDOM.serialize (L129) but postSnapshot (L153) uses forwardOpts derived from RAW options (L149). Config-only snapshot keys are dropped from the posted payload — a behavior change from base master, where the old helper mutated options in place so the value reached both serialize and post.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Broad try/catch unchanged; consistent with existing SDK pattern.
High	Correctness	No race conditions or concurrency issues	Pass	No new async/shared state introduced.
Medium	Testing	New code has corresponding tests	Fail	No new tests for the generalized merge. Existing test uses pseudoClassEnabledElements from percy config when option is not passed (index-test.js:129-140) asserts the post body and will now FAIL — config-only value no longer reaches postSnapshot. This is independent of the release-order item below.
Medium	Testing	Error paths and edge cases tested	Fail	No coverage for config-only vs per-snapshot override across keys other than pseudoClassEnabledElements, nor for config.snapshot being undefined.
Medium	Testing	Existing tests still pass (no regressions)	Fail	index-test.js:129 regresses (see above). Separately, Test job fails on clean install (see Performance/release-order note) — that part is the exempt, accepted item.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data fetching in scope.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to client-side snapshot SDK.
Medium	Quality	Follows existing codebase patterns	Pass	Delegating to a shared @percy/sdk-utils helper matches cross-SDK convention.
Medium	Quality	Changes are focused (single concern)	Pass	Single file, single concern (config merge).
Low	Quality	Meaningful names, no dead code	Pass	Removes dead addPseudoClassEnabledELements; mergedOptions is clear.
Low	Quality	Comments explain why, not what	Pass	Added comment explains merge priority.
Low	Quality	No unnecessary dependencies added	Pass	No new deps; reuses existing @percy/sdk-utils.

Findings

• File: addon-test-support/@percy/ember/index.js:74 (effect at L149/L153)

• Severity: High

• Reviewer: stack:code-review

• Issue: mergedOptions is a new immutable object spread only into PercyDOM.serialize (L129). postSnapshot (L153) is built from forwardOpts, which is destructured from the raw options (L149) — so .percy.yml config.snapshot keys that are NOT passed per-snapshot are dropped from the posted payload. On base master, the old addPseudoClassEnabledELements(options) mutated options in place, so the merged value reached both serialize AND post. The existing test uses pseudoClassEnabledElements from percy config when option is not passed (tests/acceptance/index-test.js:129-140) asserts snapshotReq.body.pseudoClassEnabledElements?.selector on the /percy/snapshot post body and will now fail. This is a genuine correctness regression, separate from the release-order item below.

• Suggestion: Derive the posted options from the merged set: const { readiness: _readiness, ...forwardOpts } = mergedOptions; (and, for consistency, base the readiness checks at L90/L96-98 on mergedOptions too).

• File: addon-test-support/@percy/ember/index.js:74

• Severity: Medium (KNOWN, ACCEPTED — release-order item; not verdict-driving)

• Reviewer: stack:code-review

• Issue: utils.mergeSnapshotOptions exists in the locally-linked @percy/sdk-utils (1.32.0-beta.9) but is not yet in the published range pinned by package.json (^1.31.15-beta.0; published latest is 1.31.14). A clean install therefore fails the Test job. (The prior reviewer's claim that this helper lives in @percy/core is incorrect — it is @percy/sdk-utils/merge-snapshot-options.js.)

• Suggestion: Publish @percy/sdk-utils with mergeSnapshotOptions and bump the package.json minimum post-approval, per the agreed release order. Optionally add a typeof utils.mergeSnapshotOptions === 'function' fallback for older CLIs. Treated like the exempt Test job: a note, not a blocker.

Note: the stack:code-reviewer agent reported the missing-function as [CRITICAL] against the published package; verified that the function does exist in the linked sdk-utils, so it is downgraded here to the accepted release-order item per the agreed exemption. Its finding #2 (post-side merge gap) is confirmed and is the verdict-driving issue.

---

Verdict: FAIL — the config-merged options never reach postSnapshot, regressing existing test index-test.js:129; fix by deriving forwardOpts from mergedOptions. (The sdk-utils publish ordering is a separate, accepted release-order item and not the reason for this verdict.)

[rishigupta1599]

Claude Code PR Review

PR: #1151 • Head: ac11973 • Reviewers: stack:code-review

Summary

Replaces the single-key addPseudoClassEnabledELements helper with utils.mergeSnapshotOptions(options), which shallow-merges .percy.yml config snapshot options under per-call options (per-call wins). The merged result now feeds BOTH PercyDOM.serialize(...) and postSnapshot (forwardOpts is destructured from mergedOptions, stripping readiness), restoring POST-body parity so config-level keys reach the POST body, not just per-call options.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Per task scope (Security 5 N/A). No secrets in diff.
High	Security	Authentication/authorization checks present	N/A	Per task scope. SDK test helper; no auth surface.
High	Security	Input validation and sanitization	N/A	Per task scope. Options object passed through to serializer/POST.
High	Security	No IDOR — resource ownership validated	N/A	Per task scope. Not applicable to client SDK.
High	Security	No SQL injection (parameterized queries)	N/A	Per task scope. No DB access.
High	Correctness	Logic is correct, handles edge cases	Pass	config-merge-before-serialize + merged-options-to-postSnapshot is correct; per-call precedence preserved by mergeSnapshotOptions; readiness correctly stripped before POST.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Unchanged try/catch around capture; no new error paths introduced.
High	Correctness	No race conditions or concurrency issues	Pass	Stable PercyDOM capture unchanged; merge is synchronous, no new async.
Medium	Testing	New code has corresponding tests	N/A	No tests in this diff; mergeSnapshotOptions behavior tested in @percy/sdk-utils. Coverage gap is pre-existing, non-blocking.
Medium	Testing	Error paths and edge cases tested	N/A	No new error paths; non-blocking.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Behavior-preserving for per-call options; broadens config pass-through only.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	Client-side; no data fetching changes.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	mergeSnapshotOptions is the cross-SDK shared helper; replaces bespoke single-key helper. Shallow merge is intentional cross-SDK parity (CLI deep-merges server-side).
Medium	Quality	Changes are focused (single concern)	Pass	Single concern: config+per-call merge parity for serialize and POST.
Low	Quality	Meaningful names, no dead code	Pass	Dead addPseudoClassEnabledELements helper removed; mergedOptions is clear.
Low	Quality	Comments explain why, not what	Pass	New comments explain the POST-parity rationale and per-call precedence.
Low	Quality	No unnecessary dependencies added	Pass	Uses existing @percy/sdk-utils import; no new deps.

Findings

No blocking findings. THIS diff introduces no new correctness regression. Notes below are non-blocking.

• File: addon-test-support/@percy/ember/index.js:90

• Severity: Low (non-blocking note)

• Reviewer: stack:code-review

• Issue: The readiness gate computes hasExplicitReadinessOpt = options?.readiness !== undefined from raw per-call options, so a readiness set only in .percy.yml (not per-call) is not treated as "explicit" and the gate is default-skipped in QUnit/mocha. This pre-existing readiness-resolution code is NOT touched by this diff (the diff only changes the merge at L74, serialize at L129, and forwardOpts at L152). Readiness has its own resolution path (getReadinessConfig/isReadinessDisabled, with a manual config-merge fallback) and is stripped before POST, so it is intentionally outside the merge scope of this PR.

• Suggestion: If desired in a follow-up, derive hasExplicitReadinessOpt from mergedOptions?.readiness !== undefined for full config/per-call parity. Out of scope here.

• File: addon-test-support/@percy/ember/index.js:74 (utils.mergeSnapshotOptions)

• Severity: Medium (KNOWN / ACCEPTED — release-order item)

• Reviewer: stack:code-review

• Issue: utils.mergeSnapshotOptions is not yet present in the published @percy/sdk-utils; it ships post-approval. Same root cause as the exempt Test job.

• Suggestion: Accepted release-ordering: publish @percy/sdk-utils with mergeSnapshotOptions before/with this SDK release. No code change required in this PR.

• File: addon-test-support/@percy/ember/index.js:129

• Severity: Low / N/A

• Reviewer: stack:code-review

• Issue: ...mergedOptions now forwards all .percy.yml config snapshot keys (some not recognized by PercyDOM.serialize) to the serializer. serializeDOM uses a named destructure and ignores unknown keys, so it does not throw.

• Suggestion: N/A — intentional cross-SDK parity; CLI deep-merges config server-side. Safe today.

---

Verdict: PASS — config-merge-before-serialize + merged-options-to-postSnapshot is correct and restores POST-body parity; no new correctness regression. The mergeSnapshotOptions publish dependency is a known/accepted release-order item.