fix(dom): stop stamping live stylesheet <link>s, duplicating them (PER-10610) - #2406
Conversation
7c666f5 to
d235049
Compare
…R-10610) Reported as #2398: since @percy/cli 1.32.6, some archived DOM snapshots contain the same stylesheet <link> twice, changing the effective CSS source order so unintended styles win. 1.32.5 emits it once. packages/dom changed in exactly one functional commit across 1.32.5 → 1.32.7 — 5c39872 (PR #2342, PER-10077) — and nothing in packages/dom/src changed at all in 1.32.7, which matches the reporter seeing it in both. That commit added the only write @percy/dom has ever made to a live stylesheet <link>: prepare-dom.js markElement() stamps data-percy-element-id on <link rel=stylesheet> so injectAtSheetPosition can find the sheet's clone and anchor its rewritten interactive-state rules there. Nothing in the package emits a second <link> for an http(s) sheet — every <link>-creating site was audited (serialize-cssom's blob and adopted branches only, both pre-1.32.6 and neither matching the report). The duplicate is created by the page itself: unlike the pseudo-class markers, which stampOnce records on ctx._liveMutations for cleanupInteractiveStateMarkers to undo, this setAttribute is raw and never cleaned off. Head-managing frameworks (next/head's head-manager, react-helmet, vue-meta) reconcile <head> children with isEqualNode(), so a managed <link> carrying an unexpected attribute compares unequal and the manager re-inserts its own copy at the end of <head> — a duplicate href in a new cascade position. Because markElement runs inside the synchronous clone walk, the reconcile lands after that snapshot: the first is clean and every later one carries the duplicate, which is why only *some* archived DOMs showed it. Fix the cause rather than the symptom: don't touch the live page at all. cloneNodeAndShadow now records a live→clone WeakMap (ctx.styleSheetClones) for every <style>/<link> as it clones them, and injectAtSheetPosition resolves its anchor by node identity through that map. PER-10077's cascade anchoring is fully preserved — verified by spec — and no stylesheet <link> is mutated or stamped, on the live page or in the output. serialize-cssom repoints the map when it rebuilds a <style> clone in place, so the anchor never lands on a detached node; the data-percy-element-id lookup remains as a fallback (<style> is still stamped, unchanged from 1.32.5), then the pre-PER-10077 end-of-<head> append. sheet.ownerNode is now null-guarded, which the previous code dereferenced unconditionally. Contained to @percy/dom: no other package reads data-percy-element-id off a <link> (core/webdriver-utils reference it only for iframes). Four specs in the PER-10610 block, three of which fail with the stamp restored and pass without it — including one that reproduces the reported symptom end to end by reconciling a head-managed <link> between two serializeDOM passes and asserting the href still appears exactly once. The inverted PER-10077 spec that asserted links *are* stamped is replaced. @percy/dom is at parity with the pre-change baseline: identical set of 81 pre-existing failures on ChromeHeadless (readiness font/js_idle, video poster timeouts, focus specs needing real window focus), 0 new. eslint clean. Note: this is independent of PER-10588, which gates the interactive-state injection but leaves markElement's <link> stamp unconditional. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
d235049 to
0a4710f
Compare
pranavz28
left a comment
pranavz28
left a comment
There was a problem hiding this comment.
Claude Code Review (automated) — 1 inline finding(s). Full report in the PR comment below. Verdict: Passed.
| const ownerNode = sheet.ownerNode; | ||
| let anchor = ownerNode ? ctx.styleSheetClones?.get(ownerNode) : null; | ||
|
|
||
| if (!anchor?.parentNode && ownerNode) { |
There was a problem hiding this comment.
[Low] data-percy-element-id fallback anchor is now unreachable for <link> owners
Since prepare-dom.js no longer stamps data-percy-element-id on stylesheet <link>s, this middle fallback step (scopeRoot.querySelector('[data-percy-element-id=...]')) can never resolve an anchor for a <link>-owned sheet anymore — only for <style> owners, which are still stamped. In production this is harmless: cloneNodeAndShadow always populates ctx.styleSheetClones before this runs, so the WeakMap lookup succeeds and this branch is not reached for links. It is exercised today only by a pre-existing hand-built-ctx unit test (cascade position of injected rules) that pre-stamps a <style> tag directly, so nothing currently regresses. Not a bug — the PR description documents this as an intentional 3-step degrade chain (map → id lookup → end-of-head) — flagging only because the middle step is dead for one of the two owner-node kinds it nominally handles.
Suggestion: No action required; optional follow-up would be a short inline note (or drop the redundant middle step for <link>-only lookups) so a future reader doesn't assume this fallback still helps stylesheet links. Given the repo convention here of no explanatory code comments, this is best left as-is or addressed only if it becomes confusing in practice.
Reviewer: code-review (built-in fallback)
There was a problem hiding this comment.
Adjudicated: accurate observation, keeping the code as-is. Recording the reasoning rather than implementing either suggestion.
The observation is correct — for a <link>-owned sheet this middle step can no longer resolve, because not stamping links is the whole point of the fix. But the suggestion to "drop the redundant middle step" would be a regression, because the step is not redundant: it is load-bearing for the other owner-node kind and for direct callers.
ctx.styleSheetClones?.get(...) is optional-chained precisely because serializePseudoClasses is a public entry point that callers invoke with their own ctx. In test/serialize-pseudo-classes.test.js there are 33 calls to serializePseudoClasses(...) and zero that supply styleSheetClones. For every one of those, the WeakMap lookup yields undefined and the data-percy-element-id step is the only thing that resolves a <style> anchor — including the cascade position of injected rules spec that guards PER-10077. Dropping it would break them and silently downgrade those callers to append-at-end-of-head.
So the accurate framing is not "dead middle step" but "step 1 covers <link> and <style> for the serializeDOM() path; step 2 covers <style> for callers that build their own ctx." Each step degrades to something correct instead of throwing, which is the documented intent.
On the optional inline note: declining per this repo's convention that rationale lives in the PR description, not the diff. The fallback chain and its ordering are already written up there under "Fallback chain, in order."
No code change.
Claude Code PR ReviewPR: #2406 • Head: 0a4710f • Reviewers: code-review (built-in fallback) SummaryFixes PER-10610: Review Table
Findings
Both findings were independently verified (code paths read directly, cross-checked against the pipeline order in Independently verified beyond the reviewer's findings:
Verdict: PASS |
Review adjudication — both findings resolved, no code changeRecording the reasoning for each finding from the automated review, per the "reply with reasoning, accepts and rejects alike" convention. 1. Replied in the inline thread. Summary: the step is not redundant. 2.
Different key spaces, different value types, different questions. Shadow hosts are stamped by CI: 47 checks passing, 0 pending. The Windows The Percy visual check reports 1 change needing review. Verified as not caused by this PR: building |
3 participants
Fixes #2398 · PER-10610
Summary
Since 1.32.6, some archived DOM snapshots contain the same stylesheet
<link>twice, changing the effective CSS source order so unintended styles win. 1.32.5 emits it once.packages/domchanged in exactly one functional commit across 1.32.5 → 1.32.7 — 5c39872 (#2342, PER-10077) — andpackages/dom/srcdidn't change at all in 1.32.7, which matches the reporter seeing it in both. That commit added the only write@percy/domhas ever made to a live stylesheet link:Nothing in the package emits a second
<link>for an http(s) sheet — every<link>-creating site was audited (serialize-cssom's blob and adopted branches only; both pre-1.32.6, neither matching the report). The duplicate is created by the page itself.Unlike the pseudo-class markers, which
stampOncerecords onctx._liveMutationsforcleanupInteractiveStateMarkersto undo, thissetAttributeis raw and never cleaned off the live page. Head-managing frameworks (next/head's head-manager, react-helmet, vue-meta) reconcile<head>children withisEqualNode(), so a managed<link>carrying an unexpected attribute compares unequal and the manager re-inserts its own copy at the end of<head>— a duplicate href in a new cascade position.Because
markElementruns inside the synchronous clone walk, the reconcile lands after that snapshot: the first is clean and every later one carries the duplicate. That is why only some archived DOMs showed it.Fix
Fix the cause, not the symptom: don't touch the live page at all.
cloneNodeAndShadowrecords a live→cloneWeakMap(ctx.styleSheetClones) for every<style>/<link>as it clones them, andinjectAtSheetPositionresolves its anchor by node identity through that map.PER-10077's cascade anchoring is fully preserved (covered by spec). No stylesheet
<link>is mutated or stamped — on the live page or in the output.Design decisions
Recording these here rather than as code comments.
Why a live→clone map instead of a marker attribute. The anchor only has to be found during serialization, so it never needed to be persisted in the DOM at all. Keying on node identity gives the same O(1) lookup with zero observable effect on the customer's page — which is the actual invariant that was violated. It also makes the anchor lookup shadow-safe by construction, where the previous
scopeRoot.querySelectorcould not pierce shadow roots.Why
WeakMap. Keys are live DOM nodes and the map is never iterated. Weak refs mean a node detached mid-serialization isn't retained for the lifetime ofctx.Why
<style>is in the map too, not just<link>.<style>is still stamped (unchanged from 1.32.5 — pre-existing behavior, and it isn't the reported regression), so it would work via the fallback. Including it keeps one resolution path for both owner-node kinds instead of branching on tag name.Why
serialize-cssomrepoints the map. It replaces a CSSOM<style>clone in place (insertBefore+remove) and runs beforeserializePseudoClasses. Without repointing, the map would hand back a detached node. The?.setis a no-op for direct callers that build actxwithout the map.Fallback chain, in order. map →
data-percy-element-idlookup → append at end of<head>(the pre-PER-10077 behavior). Each step degrades to something correct rather than throwing, andanchor.parentNodeis checked so a detached hit falls through instead of being used.Why
markElementkeeps thetagNamelocal. #2342 extracted it; it's still used by theincludes()check, so only theisStylesheetLinkbranch is removed. This leavesmarkElementfunctionally identical to 1.32.5.sheet.ownerNodeis now null-guarded. The previous code dereferenced it unconditionally. Not reachable today (collectStyleSheetsreads onlyscope.styleSheets, which excludes adopted sheets), but an unguarded throw here aborts the entire snapshot, and the guard is free.Scope. Contained to
@percy/dom: no other package readsdata-percy-element-idoff a<link>(core/webdriver-utils reference it only for iframes). Independent of PER-10588, which gates the interactive-state injection but leavesmarkElement's<link>stamp unconditional — so it would not have fixed this.Rebased onto master (PER-10588, #2399) rather than merged. The branch was behind and conflicted. The only conflict was positional in
serialize-dom.test.js— master inserted aninteractive-state serialization opt-in gate (PER-10588)block immediately above thestylesheet <link> stamping (PER-10077)block this PR replaces; git could not tell "new neighbour" from "rewritten block". Master's gate block is kept verbatim and this PR'sstylesheet <link> handling (PER-10610)block takes the PER-10077 block's place. All five source files merged clean, and the fix is unchanged by the rebase. Rebase over merge keeps the PR one reviewable commit, matching the surrounding history.The PER-10077 anchoring spec now passes
enablePseudoClassSerialization: true. PER-10588 landed after this branch was cut and made interactive-state serialization opt-in, default off. Without the flag that spec would callserializeDOM()with the rewriting disabled, find no.lbtn[data-percy-hover]copy, and fail — or worse, pass vacuously if the assertion were loosened. The flag restores what the spec is there to guard. The other three specs deliberately stay on the default path: they assert onmarkElement, which the PER-10588 gate does not touch, so they must hold with the feature off — which is how the reporter hit the bug in the first place.Test plan
Four specs in a new
stylesheet <link> handling (PER-10610)block. Three fail with the stamp restored and pass without it — verified in both directions:<link>equal to what a head manager rendered (isEqualNode)<link>s in the serialized output<link>s when a head manager reconciles between snapshots<link>, not at end of head (PER-10077)The third reproduces the reported symptom end to end: it reconciles a head-managed
<link>between twoserializeDOMpasses and asserts the href still appears exactly once.isEqualNodeis used rather than an attribute allowlist because it catches any change serialization makes to the live element, and it is the same predicate the real head managers use. The inverted PER-10077 spec that asserted links are stamped is replaced.Re-verified after the rebase, on
ChromeHeadlessagainstorigin/master(719021a) in the same worktree:common the sorted failure names)The 80 are pre-existing local failures (readiness font/js_idle, video poster timeouts, focus specs needing real window focus), unrelated to this change and equally present on master. All four PER-10610 specs pass on the fix. Root
yarn lintis clean. Firefox was not run locally (no binary available); CI covers it.🤖 Generated with Claude Code