fix(dom): reduce transient allocation during serialize (PER-10135)

[rishigupta1599]

Context — PER-10135

Customer (RS Components / LCNC, Selenium SDK) sees intermittent renderer OOM / browser disconnects during PercyDOM.serialize on long (~50 min) sessions, both Chrome and Firefox.

Root cause (investigated + reproduced)

Not a memory leak. A single serialize produces a large transient allocation burst (full DOM clone + full HTML string + string copies + JSON.stringify/bridge marshalling). On a customer SPA already near the renderer/VM memory ceiling (telemetry p50 heap ~1885 MB) in a constrained Automate VM, that one-shot burst intermittently tips the renderer over.

• Reproduced deterministically only under a hard memory cap (docker --memory): identical 45k-node/315 MB page survives at 2 GB (heap sawtooths & recovers) but crashes on the first serialize at 1.1 GB. Roomy hosts can't repro it (--max-old-space-size doesn't bound a renderer) — which is why earlier attempts failed.
• Leak ruled out: 100 serializes on one tab → post-GC heap flat (~0.0005 MB/snapshot), live DOM node count constant (90,004 → 90,004), heap-object graph +0.06% (noise).

What this PR changes (safe, output-preserving)

Two reductions to Percy's per-snapshot transient footprint:

1. serialize-dom.js — fold the two marker-rewrite .replace() passes (custom-element proxies + serialized attributes) into a single regex pass, halving large-string allocation churn on the hot tail of every capture. Output verified byte-identical against representative inputs.
2. clone-dom.js — in getOuterHTML, when there are no shadow roots to embed, return docElement.outerHTML directly instead of the getHTML() → textContent='' → outerHTML.replace() reassembly, avoiding a full-size intermediate string + a replace on the common no-shadow-DOM page.

Honest scope

This is a mitigation, not a complete OOM fix — the reproduction shows the full DOM clone dominates the transient peak, so these string-copy savings don't move a tight boundary on their own. Larger follow-ups (streamed/chunked snapshot transfer to avoid the in-renderer JSON.stringify + bridge double-marshal; capping huge inline content; customer guidance) are tracked in PER-10135.

Testing

• @percy/dom karma test suite passes.
• Combined-regex output verified byte-identical to the prior two passes.
• No-leak validated across 100 serializes (see above).

🤖 Generated with Claude Code

[rishigupta1599]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[rishigupta1599]

[Low] Merged regex silently widens the tag branch to case-insensitive

The old pair was asymmetric: the custom-element pass was case-sensitive (/(<\/?)data-percy-custom-element-/g) and only the attribute pass had i. Folding them under a single /gi extends i to the tag branch.

I ran a differential fuzz (70k generated inputs plus hand-written edge cases — $&/$1 in attribute values, both markers adjacent, </ closers, empty/hyphen/colon attribute names) against the old two-pass implementation. This is the only divergence found: with lowercase markers the two are byte-identical across 50k cases, but <DATA-PERCY-CUSTOM-ELEMENT-X> is now rewritten to <X> where it previously passed through untouched.

Not reachable from Percy's own output — clone-dom.js:36 builds the tag through document.createElement, which ASCII-lowercases in an HTML document. The exposure is customer page content that literally contains <DATA-PERCY-CUSTOM-ELEMENT-… in a text node, <pre>, <script>, or attribute value, which would now be corrupted in the snapshot.

Suggestion: both markers are always emitted lowercase, so i buys nothing and only widens the blast radius — drop it:

Suggested change

	const PERCY_MARKER_RE = /(<\/?)data-percy-custom-element-|( )data-percy-serialized-attribute-(\w+?)=/gi;
	const PERCY_MARKER_RE = /(<\/?)data-percy-custom-element-|( )data-percy-serialized-attribute-(\w+?)=/g;

If you'd rather keep the old attribute-branch tolerance exactly, keep i but note it in the comment so the widening is a recorded decision rather than a side effect of merging.

Reviewer: stack:pr-review (orchestrator verification)

[rishigupta1599]

[Low] Early return couples shadow-DOM fidelity to an invariant held in another file

The path being skipped did not need this invariant: getHTML({ serializableShadowRoots: true, … }) serializes any shadow root attached with serializable: true — which is exactly how line 126 attaches them — even when the shadowRoots array is empty. Gating on shadowRootElements.length === 0 makes correctness depend on that array being an exact mirror of the clone's shadow roots, and it is populated over in serialize-dom.js:50-66.

I traced it and the invariant holds today: line 126 is the only attachShadow call site in src/, it is always paired with the data-percy-shadow-host stamp under identical disableShadowDOM/forceShadowAsLightDOM conditions (prepare-dom.js:22-33), getShadowRoot() is the same condition clone-dom uses, and serializeElements runs before serializeHTML. So this is a robustness note, not a live bug.

One residual gap worth knowing about: a customer-supplied domTransformation runs at serialize-dom.js:145, after the array is finalized, and can mutate the clone freely — a serializable shadow root attached there would have been picked up by the old path and will now be dropped.

Suggestion: record the coupling where a future reader will see it, e.g. append to the new comment: // Invariant: serializeElements() pushes every clone shadow root into ctx.shadowRootElements (serialize-dom.js:50-66) before serializeHTML runs, so an empty array means the clone has no shadow roots to embed. A test that serializes a shadow-DOM page and asserts the <template shadowrootmode> output survives would pin it against future reordering of serializeDOM.

Reviewer: stack:pr-review (orchestrator verification)

[rishigupta1599]

Claude Code PR Review

PR: #2349 • Head: 9eb405c • Reviewers: orchestrator differential verification (stack-code-reviewer was invoked but returned no output)

Summary

Two output-preserving transient-allocation reductions in @percy/dom for PER-10135 (intermittent renderer OOM during PercyDOM.serialize): getOuterHTML now short-circuits to docElement.outerHTML when there are no shadow roots to embed, and the two marker-rewrite .replace() passes in serializeHTML are folded into a single regex pass with a replacer callback.

Verification performed

Beyond static review, the two claims of behavioral equivalence were checked directly:

• Regex equivalence (differential fuzz). 70,000 generated inputs (plus hand-written edge cases: $&/$1 in attribute values, adjacency of both markers, </ closing tags, empty attribute names, hyphen/colon attribute names) were run through the old two-pass implementation and the new single-pass implementation. With markers in the case the serializer actually emits (lowercase), 0 divergences in 50,000 cases. The alternation is unambiguous (branch 1 starts with <, branch 2 with a space — disjoint first characters), so no ordering hazard exists, and the tagPrefix !== undefined discrimination is sound because (<\/?) can never capture an empty string.
• Shadow-root invariant. packages/dom/src/clone-dom.js:126 is the only attachShadow call site in src/, and it is always paired with markElement stamping data-percy-shadow-host on the live host under identical disableShadowDOM/forceShadowAsLightDOM conditions (prepare-dom.js:22-33); getShadowRoot() (shadow-utils.js:33) is exactly the condition clone-dom uses. serializeElements (serialize-dom.js:50-66) pushes every such host's clone shadow root into ctx.shadowRootElements and runs before serializeHTML. So shadowRootElements.length === 0 does imply the clone has no shadow roots to embed, and the early return is equivalent. Same-origin iframes recurse through their own serializeDOM ctx (serialize-frames.js), so they are self-consistent.
• CI. All checks green on 9eb405c, including Test @percy/dom on both Node versions — the suite carries extensive shadow-DOM, custom-element, and base64-attribute coverage, so both changed paths are exercised indirectly.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No credentials or config touched.
High	Security	Authentication/authorization checks present	N/A	Client-side DOM serialization; no auth surface.
High	Security	Input validation and sanitization	Pass	No new HTML sinks. The regex change is a narrowing rewrite of already-serialized output; serialize-frames.js Trusted Types policy path is untouched.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No database access.
High	Correctness	Logic is correct, handles edge cases	Pass	Regex equivalence fuzz-verified (0 divergences on realistic input); shadow-root early-return invariant traced end-to-end and confirmed.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	No error paths added or removed.
High	Correctness	No race conditions or concurrency issues	Pass	serializeDOM remains fully synchronous.
Medium	Testing	New code has corresponding tests	Fail	No tests added for either new path. Both are covered indirectly by the existing @percy/dom suite (green in CI), but neither the early return nor the single-pass callback has a direct assertion. See finding 3.
Medium	Testing	Error paths and edge cases tested	Fail	The both-markers-in-one-string case (custom element carrying a serialized attribute) — the case the merged regex is most likely to get wrong on a future edit — has no dedicated test. See finding 3.
Medium	Testing	Existing tests still pass (no regressions)	Pass	All CI checks green on 9eb405c, Test @percy/dom included.
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 in-page serialization by design.
Medium	Quality	Follows existing codebase patterns	Pass	Module-scope hoisted regex and the /* istanbul ignore */ conventions match the package.
Medium	Quality	Changes are focused (single concern)	Pass	Both edits serve one goal (per-snapshot transient footprint), 20 lines total.
Low	Quality	Meaningful names, no dead code	Pass	PERCY_MARKER_RE, tagPrefix/attrSpace/attrName are clear. No dead code introduced.
Low	Quality	Comments explain why, not what	Pass	Both comment blocks explain the allocation rationale and the capture-group layout — genuinely useful given the merged regex.
Low	Quality	No unnecessary dependencies added	Pass	None added.

Findings

• File: packages/dom/src/serialize-dom.js:33

• Severity: Low

• Reviewer: orchestrator differential verification

• Issue: The merged regex carries the i flag, which now also applies to the custom-element tag branch. That branch was previously case-sensitive (/(<\/?)data-percy-custom-element-/g) — only the attribute branch had i. The fuzz run confirms this is the sole behavioral difference between old and new: with mixed/uppercase marker literals in the input, old left them alone and new rewrites them (<DATA-PERCY-CUSTOM-ELEMENT-X> → <X>). Percy never emits uppercase markers (clone-dom.js:36 builds the tag via document.createElement, which ASCII-lowercases in an HTML document), so the only way to hit this is customer page content that literally contains <DATA-PERCY-CUSTOM-ELEMENT-… inside a text node, <pre>, <script>, or attribute value — at which point the snapshot gets silently corrupted where it previously did not.

• Suggestion: Since both markers are always emitted lowercase, the i flag buys nothing and only widens the blast radius. Drop it: const PERCY_MARKER_RE = /(<\/?)data-percy-custom-element-|( )data-percy-serialized-attribute-(\w+?)=/g;. If you prefer to preserve the old attribute-branch tolerance exactly, keep i but say so in the comment so the widening is a recorded decision rather than an accident of merging the two patterns.

• File: packages/dom/src/clone-dom.js:169

• Severity: Low

• Reviewer: orchestrator differential verification

• Issue: The early return makes shadow-DOM fidelity depend on ctx.shadowRootElements being an exact mirror of the clone's serializable shadow roots — an invariant maintained in a different file (serialize-dom.js:50-66). The old path did not need that invariant: getHTML({ serializableShadowRoots: true, … }) would serialize any shadow root attached with serializable: true (which is how clone-dom.js:126 attaches them) even if the shadowRoots array were empty. The invariant does hold today for everything Percy itself does (traced above), so this is not a live bug. The one residual gap: a customer-supplied domTransformation runs at serialize-dom.js:145, after serializeElements has finished populating the array, and can mutate ctx.clone.documentElement freely — if it attaches a serializable shadow root, the old path could have serialized it and the early return will not. Note the historical context that makes this low-risk: PR Fix[ISSUE_1819]: Support for serialising ShadowRoot Element for Chrome 131 onwards #1820 ("Fix[ISSUE_1819] … Chrome 131 onwards") added the explicit shadowRoots array precisely because serializableShadowRoots: true alone was not sufficient to serialize the clone's shadow roots.

• Suggestion: Record the coupling where it can be seen, e.g. extend the new comment with // Invariant: serializeElements() pushes every clone shadow root into ctx.shadowRootElements (serialize-dom.js:50-66) before serializeHTML runs, so an empty array means the clone has no shadow roots to embed. A regression test that serializes a shadow-DOM page and asserts the <template shadowrootmode> output survives would pin the invariant against future reordering of serializeDOM.

• File: packages/dom/src/serialize-dom.js:38

• Severity: Low

• Reviewer: orchestrator differential verification

• Issue: No tests accompany either change. Both are behavior-preserving by intent and the existing suite passes, but the two things most likely to break on a future edit have no direct assertion: (a) that getOuterHTML with an empty shadowRootElements produces byte-identical output to the reassembly path, and (b) that a single string containing both markers at once — a custom element carrying a serialized attribute, e.g. <data-percy-custom-element-x data-percy-serialized-attribute-src="a.png"> — is un-mangled correctly by the one-pass callback. The PR description says equivalence was "verified byte-identical against representative inputs", but that verification lives outside the repo.

• Suggestion: Add the byte-identical assertion as a test rather than a claim in the PR body: a serialize-dom.test.js case asserting the both-markers-in-one-tag output, and a getOuterHTML unit test comparing the no-shadow early return against a fixture. Cheap, and it converts the manual verification into a permanent guard.

• File: packages/dom/src/serialize-dom.js:33

• Severity: Low

• Reviewer: orchestrator differential verification

• Issue: Pre-existing, not caused by this PR — flagged only because this PR is the natural place to fix it. serialize-base64.js:40 writes data-percy-serialized-attribute-xlink:href, but the attribute branch's name group is (\w+?), and \w matches neither : nor -. Verified against both the old and the new implementation: <image data-percy-serialized-attribute-xlink:href="data:…"> comes out unchanged in both, i.e. the marker is never un-mangled and the inlined SVG resource ships with a bogus attribute name that no browser will honor. Hyphenated attribute names have the same problem.

• Suggestion: Widen the name group to ([\w:-]+?). :, - and \w all exclude the space and = delimiters, so the lazy match still terminates correctly and no other marker shape changes. Worth a separate ticket if you'd rather keep this PR strictly no-behavior-change — but the current PR already touches this exact regex.

Note (positive): the early return also sidesteps a latent bug in the path it skips — docElement.outerHTML.replace('</html>', …) (clone-dom.js:180) replaces the first occurrence, so an <html> attribute value containing the literal </html> would splice the shadow content at the wrong offset. No-shadow pages no longer reach that code.

Note on the perf claim: the change reduces churn (one full-size string allocation instead of three) but not the peak — the DOM clone is still live while outerHTML is built, so peak stays at clone + 1×html. That matches the PR's own "honest scope" section; no correction needed, just confirming the framing is accurate rather than optimistic.

⚠️ stack-code-reviewer was invoked with the full diff and five targeted investigation points, but went idle twice without delivering a report, so none of its analysis is reflected here. Every finding above comes from the orchestrator's own verification, described in the "Verification performed" section. Discovery note: stack:security-review and stack-security-auditor were skipped — the security lens is temporarily disabled in this skill — and no nested .claude/ subproject reviewers exist in this repo.

---

Verdict: PASS — both changes are equivalence-verified (fuzz + invariant trace + green CI), scoped tightly, and well commented. The four findings are all Low: drop the now-unnecessary i flag, document the shadowRootElements coupling, land the byte-identical check as a test, and optionally fix the pre-existing xlink:href marker gap.

[rishigupta1599]

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

[rishigupta1599]

[High] Wrong predicate — silently drops shadow DOM attached during domTransformation

Upgraded from the Low note posted earlier on this line: this was verified as reachable, with a reproduction, rather than hypothetical.

getHTML({ serializableShadowRoots: true, shadowRoots: [] }) does serialize a shadow root marked serializable: true even when it is absent from the array (verified in Chrome). The array only matters for non-serializable roots — such as the constructor-created root reused at lines 122-124, which is why serialize-dom.test.js:332 expects <template shadowrootmode="open"> with no shadowrootserializable. So shadowRootElements.length === 0 is not equivalent to "the clone has no serializable shadow roots".

The reachable path: a domTransformation callback runs at serialize-dom.js:145, i.e. after serializeElements has finished populating the array, and can attach a serializable shadow root to the clone. Repro:

serializeDOM({ domTransformation: (docEl) => {
  const sr = docEl.querySelector('#content').attachShadow({ mode: 'open', serializable: true });
  sr.innerHTML = '<p>PROBETRANSFORM</p>';
}})

master → html contains PROBETRANSFORM; patched → <div id="content"></div>, content gone, no warning emitted. domTransformation / dom_transformation is a documented public SDK option.

Ruled out as divergent (no issue there): live-DOM shadow roots — markElement (prepare-dom.js:23-33) and clone-time attachShadow (lines 116-133) are gated on the identical condition; nested and CDP-captured closed roots share one array; iframes get their own ctx via serializeFrames. domTransformation is the one gap.

Suggestion: gate on both conditions — thread a flag from serialize-dom.js:29 (cloneMayHaveUncountedShadowRoots: !!domTransformation) and test if (!cloneMayHaveUncountedShadowRoots && !shadowRootElements?.length). Land the repro as a regression test. (Nit while here: the !shadowRootElements || arm is unreachable — the sole caller always passes an array — so !shadowRootElements?.length suffices.)

Reviewer: stack-code-reviewer

[rishigupta1599]

[Medium] Skipping textContent = '' works against the PR's own memory goal

The reassembly path clears the clone at line 177 before returning. That was not incidental: it released the entire cloned node tree before the caller built doctype(ctx.dom) + html (serialize-dom.js:35) and — with stringifyResponse — a second full-size JSON copy of the html (serialize-dom.js:183).

With this early return the clone stays fully reachable across all of that, so peak retention rises by roughly the size of the clone. The PR's own investigation identifies that clone as the dominant contributor (~315 MB for the 45k-node repro, against a ~30 MB html string), so on a tight memory boundary this plausibly costs more than the one saved string copy gains.

Nothing depends on the emptying: serializeHTML is the last consumer of ctx.clone, and cleanupInteractiveStateMarkers only walks ctx._liveMutations.

Suggestion: keep the release in the fast path:

Suggested change

	return docElement.outerHTML;
	const html = docElement.outerHTML;
	docElement.textContent = '';
	return html;

The pre-existing forceShadowAsLightDOM and old-Firefox early returns have the same omission and would benefit equally.

Reviewer: stack-code-reviewer

[rishigupta1599]

Claude Code PR Review (revised)

PR: #2349 • Head: 9eb405c • Reviewers: stack-code-reviewer, orchestrator differential verification

This supersedes the earlier PASS comment on this PR. That verdict was posted before stack-code-reviewer delivered its report. The reviewer applied the patch locally and ran browser probes rather than reasoning from spec, and it found (a) a reachable, silent shadow-DOM data-loss path that the first pass graded as a hypothetical Low, and (b) a memory regression that the first pass explicitly considered and got wrong. The commit status has been flipped to failure and the corrected findings are below.

Summary

Two intended-output-preserving transient-allocation reductions in @percy/dom for PER-10135 (intermittent renderer OOM during PercyDOM.serialize): getOuterHTML short-circuits to docElement.outerHTML when shadowRootElements is empty, and the two marker-rewrite .replace() passes in serializeHTML are folded into one regex pass with a replacer callback.

Verification performed

• Reviewer (empirical, in headless Chrome). Patch applied locally, full @percy/dom suite run (496 specs pass — nothing existing catches the issues below), plus targeted probe specs per question; then reverted.
• Orchestrator (differential fuzz). 70,000 generated inputs plus hand-written edge cases ($&/$1 in attribute values, both markers adjacent, </ closers, empty/hyphen/colon attribute names) through old two-pass vs. new one-pass. 0 divergences on lowercase markers across 50,000 cases; the only divergences involve uppercase marker literals (F3).
• CI. All checks green on 9eb405c including Test @percy/dom — which is precisely the problem: the existing suite cannot see either defect.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No credentials or config touched.
High	Security	Authentication/authorization checks present	N/A	Client-side DOM serialization; no auth surface.
High	Security	Input validation and sanitization	Pass	No new HTML sinks; the Trusted Types path in serialize-frames.js is untouched.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No database access.
High	Correctness	Logic is correct, handles edge cases	Fail	F1: the early return is guarded by the wrong predicate and silently drops shadow DOM attached during domTransformation — a documented public SDK option. Reproduced on-device: captured on master, gone with the patch, no warning.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	No error paths added or removed.
High	Correctness	No race conditions or concurrency issues	Pass	serializeDOM remains fully synchronous.
Medium	Testing	New code has corresponding tests	Fail	F4: no test anywhere in the repo references either marker string; neither changed path has a direct assertion.
Medium	Testing	Error paths and edge cases tested	Fail	F4: the domTransformation + shadow-root case (F1) and the both-markers-in-one-tag case have no coverage. .nycrc enforces 100%, but istanbul cannot flag either gap — both new branches are exercised and both `
Medium	Testing	Existing tests still pass (no regressions)	Pass	496 specs pass with the patch applied; CI green.
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 in-page serialization by design.
Medium	Performance	Change achieves its stated goal	Fail	F2: dropping docElement.textContent = '' keeps the whole clone tree reachable while the result string (and, with stringifyResponse, a second full JSON copy) is built — working against the PR's own memory objective on the exact path it targets.
Medium	Quality	Follows existing codebase patterns	Pass	Hoisted module-scope regex and /* istanbul ignore */ conventions match the package.
Medium	Quality	Changes are focused (single concern)	Pass	20 lines, one objective.
Low	Quality	Meaningful names, no dead code	Pass	Names are clear. F7 is a nit: the !shadowRootElements arm is unreachable from the sole caller.
Low	Quality	Comments explain why, not what	Fail	F5: the stated rationale for the regex half is inaccurate — .replace() with a non-matching global regex does not copy the string, and a per-match callback is slower than '$1' when it does match.
Low	Quality	No unnecessary dependencies added	Pass	None added.

Findings

• File: packages/dom/src/clone-dom.js:169

• Severity: High

• Reviewer: stack-code-reviewer

• Issue: The early return is guarded by the wrong predicate, causing silent snapshot data loss. getHTML({ serializableShadowRoots: true, shadowRoots: [] }) does serialize a shadow root marked serializable: true even when it is absent from the array (probe P1, verified in Chrome) — the array only matters for non-serializable roots, such as the constructor-created root reused at clone-dom.js:122-124, which is why serialize-dom.test.js:332 expects <template shadowrootmode="open"> with no shadowrootserializable. So shadowRootElements.length === 0 is not equivalent to "the clone has no serializable shadow roots". Reachable path (probe P2): a domTransformation callback runs at serialize-dom.js:145, i.e. after serializeElements has finished populating the array, and can attach a serializable shadow root to the clone. Attaching one to #content and setting innerHTML = '<p>PROBETRANSFORM</p>' yields html containing PROBETRANSFORM on master and <div id="content"></div> with the patch — content dropped, no warning emitted. domTransformation / dom_transformation is a documented public SDK option. Ruled out as divergent: live-DOM shadow roots (markElement at prepare-dom.js:23-33 and clone-time attachShadow at clone-dom.js:116-133 are gated on the identical condition), nested and CDP-captured closed roots (one shared array), and iframes (own ctx via serializeFrames). domTransformation is the one reachable gap.

• Suggestion: Gate the fast path on both conditions — thread a flag from serialize-dom.js:29, e.g. pass cloneMayHaveUncountedShadowRoots: !!domTransformation and test if (!cloneMayHaveUncountedShadowRoots && !shadowRootElements?.length). Land probe P2 as a regression test.

• File: packages/dom/src/clone-dom.js:170

• Severity: Medium

• Reviewer: stack-code-reviewer

• Issue: The early return skips the docElement.textContent = '' that the reassembly path performs at line 177. That assignment was not incidental — it released the entire cloned node tree before the caller went on to build doctype(ctx.dom) + html (serialize-dom.js:35) and, when stringifyResponse is set, a second full-size JSON copy of the html (serialize-dom.js:183). With the patch the clone stays fully reachable across all of that, so peak retention rises by roughly the size of the clone — which the PR's own investigation identifies as the dominant contributor (~315 MB for the 45k-node repro, against a ~30 MB html string). Nothing depends on the emptying: serializeHTML is the last consumer of ctx.clone, and cleanupInteractiveStateMarkers only walks ctx._liveMutations. This works directly against the PR's stated objective on the exact path it targets. (The first review pass asserted peak was unchanged; that was wrong — it accounted for the clone being live during the outerHTML allocation but missed that the old path freed it before the downstream concat and JSON.stringify.)

• Suggestion: Keep the release in the fast path: const html = docElement.outerHTML; docElement.textContent = ''; return html;. The pre-existing forceShadowAsLightDOM and old-Firefox early returns have the same omission and would benefit equally.

• File: packages/dom/src/serialize-dom.js:33

• Severity: Low

• Reviewer: stack-code-reviewer + orchestrator (fuzz)

• Issue: Merging the two patterns under /gi extends i to the custom-element tag branch, which was previously case-sensitive; only the attribute branch had i. The differential fuzz confirms this is the sole behavioral divergence between old and new: <DATA-PERCY-CUSTOM-ELEMENT-X></DATA-PERCY-CUSTOM-ELEMENT-X> → <X></X>, and title="<DATA-PERCY-CUSTOM-ELEMENT-FOO>" → title="<FOO>". The reviewer narrowed the reachability: the attribute-value vector is unreachable because Chrome escapes </> there, but <script>, <style>, and comment content serializes verbatim, so it is reachable — realistically near-zero. Note the flip side: the widening also fixes XHTML documents, where createElement does not lowercase.

• Suggestion: Either keep the two regexes as they were, or document the widening as an intentional (and in XHTML, beneficial) change so it is a recorded decision rather than a side effect of merging.

• File: packages/dom/src/serialize-dom.js:38

• Severity: Medium

• Reviewer: stack-code-reviewer

• Issue: Test coverage. A repo-wide grep finds no test anywhere referencing data-percy-custom-element- or data-percy-serialized-attribute-; the rewritten transform has zero direct assertions (serialize-dom.test.js:130-159 exercises the proxy path but asserts $('h2.callback'), which passes either way). Coverage is enforced — babel-plugin-istanbul in the test env (babel.config.cjs:58), merged to nyc (scripts/test.js:203-208), .nycrc at 100%, @percy/dom in the test.yml matrix — but it cannot flag any of this: the new if has both paths exercised, both || clauses are always evaluated (which is what istanbul's binary-expr counts), and no previously-covered branch becomes unreachable. This is why 496 green specs and green CI are not evidence of safety here.

• Suggestion: Add (a) the F1 domTransformation repro, (b) a getOuterHTML(clone, { shadowRootElements: [] }) byte-equality assertion against the reassembled form (the reviewer verified this equality holds today — probes P4/P5), and (c) direct un-mangling assertions for both markers, including a custom element carrying a serialized attribute so the one-pass callback discrimination is pinned.

• File: packages/dom/src/serialize-dom.js:27

• Severity: Low

• Reviewer: stack-code-reviewer

• Issue: The comment's rationale is inaccurate. "Each .replace() copies the whole string" is false on the common path — V8 returns the subject unchanged when a global regex finds no match — and when there are matches, a per-match function callback is slower than a '$1' replacement string. The measurable win in this PR is the clone-dom.js half.

• Suggestion: Drop this half of the change, or correct the comment to state the actual (narrower) benefit: one full-size copy instead of two when markers are present.

• File: packages/dom/src/serialize-dom.js:33

• Severity: Low

• Reviewer: stack-code-reviewer + orchestrator

• Issue: Pre-existing, flagged only because this PR rewrites the exact regex. \w excludes :, so data-percy-serialized-attribute-xlink:href (serialize-base64.js:40) is never un-mangled — confirmed identical on master and patch. Base64-inlined SVG xlink:href/<use> references ship with a bogus attribute name that no browser honors. Hyphenated attribute names have the same problem.

• Suggestion: Widen the name group to ([\w:-]+?). :, -, and \w all exclude the space and = delimiters, so the lazy match still terminates and no other marker shape changes. Reasonable as a separate ticket if you want this PR to stay strictly no-behavior-change.

• File: packages/dom/src/clone-dom.js:169

• Severity: Low (nit)

• Reviewer: stack-code-reviewer

• Issue: !shadowRootElements || is unreachable from the sole caller — serialize-dom.js:29 always passes an array.

• Suggestion: Collapse to !shadowRootElements?.length.

Settled, no action needed: the callback's tagPrefix !== undefined discrimination is sound — branch 1's group 1 is always < or </ (never empty or undefined when it matches), branch 2's groups are a literal space plus \w+? (≥1 char), and the two branches cannot match at the same offset (disjoint first characters). The clone-emptying question is answered by F2. Live-DOM shadow roots, nested/CDP-captured closed roots, and iframes were all ruled out as divergent for F1.

Out of scope: an automated scan flagged ssl: rejectUnauthorized: false in .percy.scanner.yml — a pre-existing untracked local file, unrelated to this PR (which touches only packages/dom). Not addressed here.

---

Verdict: FAIL — F1 is silent snapshot data loss on a documented public option, with a reproduction, and F2 means the change likely worsens peak retention on the very OOM path it targets. Both are small, well-understood fixes (thread a flag into the guard; keep the textContent = '' release), and F4 asks for the tests that would have caught them. The regex half is equivalence-verified apart from the F3 case widening, but F5 questions whether it earns its keep.

[rishigupta1599]

Claude Code PR Review (re-review after fixes)

PR: #2349 • Head: 6ddd033 • Reviewers: stack-code-reviewer, orchestrator differential verification

Third and final review on this PR. History: the first pass posted PASS prematurely; the second flipped to FAIL on two findings (silent shadow-DOM loss, and a memory regression working against the PR's own goal). Both are now fixed in fc7b9a0b + 6ddd0337, verified independently, and this pass returns to PASS.

Summary

Transient-allocation reductions in @percy/dom for PER-10135 — a getOuterHTML fast path for pages with no shadow roots to embed, and the two marker-rewrite .replace() passes folded into one — now correctly gated, with the clone release preserved and regression tests that actually fail without the fixes.

What changed since 9eb405c

Finding	Severity	Status
F1 — fast path dropped shadow roots attached during domTransformation	High	Fixed (fc7b9a0b) — gated on cloneMayHaveUncountedShadowRoots
F2 — fast path skipped textContent = '', retaining the clone	Medium	Fixed (fc7b9a0b) — release restored
F4 — no direct tests for either changed path	Medium	Fixed (fc7b9a0b, 6ddd0337) — 6 tests added, 4 verified failing without their fix
F3 — i flag widened the tag branch	Low	Documented as intentional (incl. the XHTML upside)
F5 — inaccurate rationale in the regex comment	Low	Corrected
F7 — unreachable `!shadowRootElements		` arm
F6 — pre-existing xlink:href marker never un-mangled	Low	Deferred by decision — see Open items

Verification performed

F1 closure (reviewer, in headless Chrome at fc7b9a0b). The original repro re-run with a different fixture now passes. Exhaustive route audit: attachShadow appears exactly once in the package (clone-dom.js:126); of everything that touches ctx.clone after serializeElements, serializePseudoClasses/rewriteCustomStateCSS only inject <style> elements, reshuffleInvalidTags only moves existing nodes (and a moved host's root is already in the array), and utils.js:42 shallow-clones a <style>, which cannot host a shadow root. domTransformation was the only route and it is now gated. The gate reads ctx.domTransformation — the option captured at ctx construction, before the window.eval reassignment — so it is correct for both the function and string forms, and stays true if a transformation throws part-way leaving a partially attached root.

F2 safety (reviewer). let html = docElement.outerHTML fully materializes the string before the mutation; docElement is always the detached ctx.clone.documentElement, never the live DOM; nothing reads ctx.clone afterwards (in the result literal html: is evaluated first, and the finally hook walks only ctx._liveMutations).

Tests are not tautological. Reverting the guard makes 3 of the new tests fail (clone release; shadow-roots-absent-from-array; domTransformation + shadow root) and passing again with it restored. Separately, neutering the tag branch of PERCY_MARKER_RE makes the marker un-mangling test fail. That last check was prompted by the reviewer catching the original version of that test as vacuous — it used an undefined custom element, which cloneElementWithoutLifecycle clones via the plain cloneNode() branch, emitting no markers at all, so every assertion passed on input that never contained one. 6ddd0337 replaces the fixture with a proxied custom element carrying a src, putting both markers in the same tag.

Coverage. clone-dom.js and serialize-dom.js are both at 100% statements/branches/functions/lines at the fix head — the new && and the optional chaining did not create an uncovered branch.

CI on 6ddd0337. All checks pass, including Test @percy/dom on both Node versions (2m58s / 3m40s) and Test @percy/core (20m18s) — the latter matters because @percy/core consumes the @percy/dom bundle in its snapshot tests. One duplicate Test @percy/core job on the second workflow run has been stuck in its "Run tests" step for ~57 minutes with no retry started, against its sibling's 20m completion on identical code; that is a hung runner, not a result. Re-run it if the branch protection needs the green tick.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No credentials or config touched.
High	Security	Authentication/authorization checks present	N/A	Client-side DOM serialization; no auth surface.
High	Security	Input validation and sanitization	Pass	No new HTML sinks; the Trusted Types path in serialize-frames.js is untouched.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No database access.
High	Correctness	Logic is correct, handles edge cases	Pass	F1 closed and independently re-probed; every other post-serializeElements route audited and ruled out. Regex fold fuzz-verified equivalent (50k cases) apart from the documented F3 widening.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	No error paths added or removed. The gate stays conservative when a transformation throws.
High	Correctness	No race conditions or concurrency issues	Pass	serializeDOM remains fully synchronous.
Medium	Testing	New code has corresponding tests	Pass	New test/clone-dom.test.js plus 3 cases in serialize-dom.test.js.
Medium	Testing	Error paths and edge cases tested	Pass	The domTransformation + shadow-root case, the shadow-roots-absent-from-array mechanism, the clone release, and both markers in one tag are each pinned — and each verified to fail without its fix.
Medium	Testing	Existing tests still pass (no regressions)	Pass	916/916 locally on Chrome + Firefox; CI green including @percy/core, which consumes the dom bundle.
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 in-page serialization by design.
Medium	Performance	Change achieves its stated goal	Pass	With the clone release restored, the fast path is a genuine reduction rather than a trade of one string copy for a retained node graph. Reach is narrower than ideal — see Open items.
Medium	Quality	Follows existing codebase patterns	Pass	Options-object threading, hoisted regex, and /* istanbul ignore */ conventions all match the package.
Medium	Quality	Changes are focused (single concern)	Pass	Still one objective; the fix commits address review findings only.
Low	Quality	Meaningful names, no dead code	Pass	cloneMayHaveUncountedShadowRoots is long but says exactly what it gates. F7's dead arm removed.
Low	Quality	Comments explain why, not what	Pass	The guard now documents why an empty array is insufficient — the non-obvious serializableShadowRoots semantics that caused F1. F5's inaccurate claim corrected.
Low	Quality	No unnecessary dependencies added	Pass	None added.

Findings

No blocking findings remain.

Open items (deliberate, for the PER-10135 follow-up)

• xlink:href marker never un-mangled (serialize-base64.js:40 + the (\w+?) group). Pre-existing on master; \w excludes :, so base64-inlined SVG xlink:href references ship with a bogus attribute name. Left unfixed here because ([\w:-]+?) changes serialized output, which this PR is explicitly scoped not to do.
• Iframe snapshots get none of the win. serialize-frames.js always passes a domTransformation (setBaseURI), so every nested iframe now takes the reassembly path. The reviewer proposed a trustedDomTransformation opt-out; declined for this PR, since it adds a bypass to a guard whose whole purpose is preventing silent data loss, and as an options key a caller could set it and re-open F1. Iframe-heavy pages are squarely in PER-10135's target, so this belongs with the streamed-transfer work the PR description already tracks.
• forceShadowAsLightDOM and the old-Firefox early returns still return without releasing the clone — the same F2 argument applies, and forceShadowAsLightDOM is used by App Automate flows. Out of scope here (pre-existing paths).
• Pre-existing coverage gap at serialize-pseudo-classes.js:184-186 (the :popover-open-unsupported catch, uncoverable on current Chrome/Firefox) fails the global test:coverage gate. Verified identical at base 768c93a6, so it is not attributable to this PR, and CI's Test @percy/dom does not gate on it.

---

Verdict: PASS — both blocking findings are fixed, the fixes are independently verified rather than asserted, and the regression tests were each confirmed to fail without their fix. The remaining items are pre-existing or deliberate scope decisions, recorded above for the follow-up.