fix: merge .percy.yml config options into serializeDOM by rishigupta1599 · Pull Request #754 · percy/percy-nightmare

rishigupta1599 · 2026-05-05T18:40:41Z

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

🤖 Generated with Claude Code

…izeDOM Previously, only per-snapshot options were passed to PercyDOM.serialize(), ignoring config-level settings (enableJavaScript, disableShadowDOM, etc.) from .percy.yml. Now config.snapshot is used as defaults, with per-snapshot options taking priority. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Replace inline config merge with centralized utility from sdk-utils. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

github-actions · 2026-05-19T19:34:29Z

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 · 2026-06-02T20:02:28Z

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: Passed.

rishigupta1599 · 2026-06-16T14:23:39Z

        await nEval(new Function(await utils.fetchPercyDOM()));

        // Serialize and capture the DOM
+        const mergedOptions = utils.mergeSnapshotOptions(options);


[Medium] mergeSnapshotOptions requires a newer @percy/sdk-utils than the pinned range

utils.mergeSnapshotOptions was only added to @percy/sdk-utils in 1.32.0-beta.9, but package.json pins @percy/sdk-utils: ^1.0.0. An environment resolving an older 1.x sdk-utils satisfies the range yet lacks this export, so this line throws TypeError: utils.mergeSnapshotOptions is not a function and the snapshot is silently swallowed by the surrounding catch.

Suggestion: Bump the minimum @percy/sdk-utils to the version that ships mergeSnapshotOptions (e.g. ^1.32.0), or guard the call: const mergedOptions = utils.mergeSnapshotOptions?.(options) ?? options;

Reviewer: stack:code-review

rishigupta1599 · 2026-06-16T14:23:41Z

Claude Code PR Review

PR: #754 • Head: ca9b0cb • Reviewers: stack:code-review

Summary

Merges the global .percy.yml snapshot config (utils.percy.config.snapshot) with per-snapshot options (per-call priority) into mergedOptions and passes that to PercyDOM.serialize() before serializing the DOM, so config-level snapshot options now reach serialization in the Nightmare SDK (PER-8053).

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Security lens skipped per instruction; no secrets in diff.
High	Security	Authentication/authorization checks present	N/A	Security lens skipped; no auth surface touched.
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	`mergeSnapshotOptions` (sdk-utils) is a shallow merge `{...configOptions, ...options}` with per-call priority; handles `undefined` options. Matches PR intent.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Call sits inside the existing try/catch (`index.js:48`); no new error paths introduced.
High	Correctness	No race conditions or concurrency issues	Pass	Pure synchronous transform of `options`; no shared state or new async.
Medium	Testing	New code has corresponding tests	Fail	No test in `test/index.test.js` asserts config options are merged into the serialized options; new behavior is unverified in this repo (`test/index.test.js`).
Medium	Testing	Error paths and edge cases tested	N/A	No new error path added by this change.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Change is additive; serialize call still receives an options object. The serialize injection is `istanbul ignore`d so coverage is unaffected.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	No I/O; single in-memory object spread.
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 percy-puppeteer/percy-playwright/percy-selenium-js exactly: `mergedOptions`→serialize, raw `...options`→postSnapshot.
Medium	Quality	Changes are focused (single concern)	Pass	One-line addition + one substitution; single concern.
Low	Quality	Meaningful names, no dead code	Pass	`mergedOptions` is clear and used immediately.
Low	Quality	Comments explain why, not what	Pass	Existing comments unchanged; no misleading comments added.
Low	Quality	No unnecessary dependencies added	Fail	`mergeSnapshotOptions` only exists in `@percy/sdk-utils` >= 1.32.0-beta.9, but `package.json` pins `@percy/sdk-utils: ^1.0.0`. An installed older 1.x satisfies the range yet lacks the function → `utils.mergeSnapshotOptions is not a function` TypeError at runtime.

Findings

File: package.json (dependency @percy/sdk-utils)
Severity: Medium
Reviewer: stack:code-review
Issue: The change calls utils.mergeSnapshotOptions(options), but that export was only added to @percy/sdk-utils in 1.32.0-beta.9. The dependency range is ^1.0.0, so an environment that resolves any older 1.x sdk-utils will throw TypeError: utils.mergeSnapshotOptions is not a function and the snapshot will be swallowed by the catch block, silently losing the snapshot.
Suggestion: Bump the minimum @percy/sdk-utils to the version that ships mergeSnapshotOptions (e.g. ^1.32.0), consistent with the other SDKs adopting this helper, or guard the call ((utils.mergeSnapshotOptions?.(options)) ?? options).
File: test/index.test.js
Severity: Low
Reviewer: stack:code-review
Issue: No test verifies the new merge behavior — that .percy.yml config snapshot options are applied to serialization with per-call options winning. The serialize injection is istanbul ignored, so the merge path is entirely unverified in this repo.
Suggestion: Add a test that sets percy.config.snapshot (or stubs utils.mergeSnapshotOptions) and asserts merged options reach the serialize call, mirroring coverage added in sibling SDKs.

Verdict: PASS — correct, minimal, and consistent with sibling SDKs; recommend bumping the @percy/sdk-utils minimum version and adding a merge-behavior test before release.

…regen lockfile Ref: PER-8053

This SDK has no committed lockfile, so CI's bare `yarn` resolves dependencies fresh to the latest versions each run. Via @percy/core, that now pulls node-releases@2.0.47 (node>=18, through nyc->babel->browserslist) and snyk-nodejs-lockfile-parser@2.7.1 (node>=18, which also drags in @yarnpkg/core@4 + tar@7) — and yarn hard-fails on the engine mismatch, breaking the Node 14 leg on install. Pin both back to their last Node-14-compatible line via resolutions (yarn honors these on the fresh install); the snyk pin also drops the @yarnpkg/tar@7 subtree. Temporary workaround pending the upstream CLI fix (percy/cli#2301). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Shivanshu-07 · 2026-06-24T15:59:24Z

-# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
-# yarn lockfile v1
-


why is this removed?

Shivanshu-07 · 2026-06-24T16:00:00Z

          domSnapshot: PercyDOM.serialize(options),
          url: document.URL
-        }), options);
+        }), mergedOptions);


No test coverage for this? is it covered in the existing UTs?

Commit 639b9ec ("regen lockfile") accidentally *deleted* yarn.lock entirely instead of regenerating it, leaving the SDK with no committed lockfile. Restore it from base and regenerate against the current package.json (@percy deps + node-14 resolutions) so there is a proper, consistent, committed lockfile again. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Restoring/regenerating the lockfile pulled event-loop-spinner@2.3.3 (node>=18) via snyk-nodejs-lockfile-parser -> @snyk/dep-graph, breaking the Node 14 lint/ typecheck legs. Pin to 2.3.2 (last node-14-compatible line) alongside the other transitive pins. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

github-actions · 2026-07-28T19:31:27Z

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.

rishigupta1599 requested a review from a team as a code owner May 5, 2026 18:40

rishigupta1599 requested review from amandeepsingh333 and pankaj443 May 5, 2026 18:40

refactor: use utils.mergeSnapshotOptions() from @percy/sdk-utils

ca9b0cb

Replace inline config merge with centralized utility from sdk-utils. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

github-actions Bot added the 🍞 stale Closed due to inactivity label May 19, 2026

github-actions Bot closed this Jun 2, 2026

rishigupta1599 reopened this Jun 15, 2026

rishigupta1599 commented Jun 16, 2026

View reviewed changes

github-actions Bot removed the 🍞 stale Closed due to inactivity label Jun 16, 2026

rishigupta1599 and others added 2 commits June 17, 2026 16:53

chore: bump @percy deps to ^1.32.0 (published mergeSnapshotOptions); …

639b9ec

…regen lockfile Ref: PER-8053

Shivanshu-07 reviewed Jun 24, 2026

View reviewed changes

rishigupta1599 and others added 2 commits July 10, 2026 16:04

github-actions Bot added the 🍞 stale Closed due to inactivity label Jul 28, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: merge .percy.yml config options into serializeDOM - #754

fix: merge .percy.yml config options into serializeDOM#754
rishigupta1599 wants to merge 6 commits into
masterfrom
fix/merge-percy-yml-config-with-serialize-dom

rishigupta1599 commented May 5, 2026

Uh oh!

github-actions Bot commented May 19, 2026

Uh oh!

github-actions Bot commented Jun 2, 2026

Uh oh!

rishigupta1599 left a comment

Uh oh!

rishigupta1599 Jun 16, 2026

Uh oh!

rishigupta1599 commented Jun 16, 2026

Uh oh!

Shivanshu-07 Jun 24, 2026

Uh oh!

Shivanshu-07 Jun 24, 2026

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
		# yarn lockfile v1

Uh oh!

Conversation

rishigupta1599 commented May 5, 2026

Summary

Test plan

Uh oh!

github-actions Bot commented May 19, 2026

Uh oh!

github-actions Bot commented Jun 2, 2026

Uh oh!

rishigupta1599 left a comment

Choose a reason for hiding this comment

Uh oh!

rishigupta1599 Jun 16, 2026

Choose a reason for hiding this comment

Uh oh!

rishigupta1599 commented Jun 16, 2026

Claude Code PR Review

Summary

Review Table

Findings

Uh oh!

Shivanshu-07 Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

Shivanshu-07 Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants