fix(core): namespace SVG ids during composition inline to prevent cross-scene collisions - #3494
miga-heygen wants to merge 5 commits into
Conversation
…ss-scene collisions Two nested compositions that each declare their own SVG ids (`<clipPath id="clip">`, `<symbol id="shape">`, `<filter id="fx">`) are legal per file and pass `hyperframes check`, but collide once both are inlined into one render/preview document. `url(#id)` funcrefs (`clip-path`, `filter`, `mask`, `fill`, `stroke`, `marker-start/mid/end`) and fragment `href`/`xlink:href` refs (`<use href="#id">`) are resolved by the browser's native SVG/CSS engine, which always binds to the first matching id in document order — so the later scene either clips to nothing or paints the earlier scene's content. `getElementById` was already scoped per composition in #646, and media pipeline ids got a parallel `data-hf-render-id` attribute in #3340. Neither covers this: native `url(#id)`/`href="#id"` resolution can't be intercepted by a JS proxy, so the `id` attribute itself has to become document-unique. Add `svgIdNamespacing.ts`: during `inlineSubCompositions`, every id declared on an `<svg>`-subtree element is prefixed with the composition's document-unique runtime id, and every same-document reference to it — DOM attributes (`href`, `xlink:href`, any `url(#id)` value including inside `style`) and the composition's own extracted `<style>` text (both `#id` selectors and `url(#id)` declaration values) — is rewritten to match. Renamed elements keep their original id on `data-hf-authored-id`, the same attribute #646 already added for the composition root, so an inline script's own `document.getElementById(originalId)` keeps resolving via the existing `__hfGetElementById` scoping shim. Extract `selectorIdTokens.ts` from `compositionScoping.ts`'s existing single-id selector scan so the new many-id rewrite reuses the same guarded-region logic instead of a second copy. Closes #3490 Co-Authored-By: Miga <noreply@anthropic.com>
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R1 adversarial — SVG ID collision namespace pass
Summary: DOM walk over svg[id]/svg [id] renames each declared id to ${sanitizedNamespace}--${originalId}, records the authored id on data-hf-authored-id, and rewrites same-element attribute refs via a url(#...) regex + suffix :href detector. <style> text goes through a postcss pass + a shared selectorIdTokens scan (extracted from the existing root-id rewrite so both callers share one quote/bracket state machine).
Blockers (P0/P1):
- (none)
Concerns (non-blocking):
- ARIA id-refs not rewritten —
aria-labelledby,aria-describedby,aria-controls,aria-owns,aria-flowtoall carry bare id lists (no#), and<svg><title id=\"chart-title\">referenced byaria-labelledby=\"chart-title\"is standard SVG a11y. Post-rename the id isscene-a--chart-title; the aria attr still sayschart-titleand either resolves to another scene's title in the merged doc or dangles. Not a first-order render bug (matches the ticket'surl(#)/hrefscope) but worth a followup — same class of native-resolver reference that motivated this PR. [id=\"foo\"]attribute selectors in<style>are silently skipped —selectorIdTokens.ts:markUnguardedOffsetsdeliberately masks out bracket regions (correct for#disambiguation), sorewriteSvgIdReferencesInCssnever touches[id=\"clip\"]. Rare in author CSS, but any composition that reaches for the attribute form instead of#clipwill silently stop matching after rename. Worth a test asserting current behavior + a doc note.- Idempotency not guarded — a second
namespaceSvgIds(root, ns)call double-prefixes (scene-a--scene-a--clip) AND overwritesdata-hf-authored-idwith the already-namespaced id, destroying the original.inlineSubCompositionslooks single-pass per instance so this is latent, but noif (el.hasAttribute(SVG_AUTHORED_ID_ATTR)) skipguard exists to catch a future re-entry (e.g. nested inline of a fragment that was itself pre-inlined). - No golden-frame / render-pixel test — three unit tests confirm the id map is correct, but nothing asserts "two scenes reusing
#fxactually paint their own filter after the compiler ran end-to-end." The repro in #3490 is visual; the failure mode is silent misresolution. A single regression-shard scene mirroring the ticket repro would lock the fix in against future refactors of the URL regex. sanitizeNamespaceSegmentfolds special chars to-without a collision guard —foo!andfoo?both becomefoo-, so any two runtime ids that differ only in special chars produce the same prefix. Runtime ids fromassignBundledRuntimeCompositionIdsare compiler-generated and almost certainly alphanumeric, but the sanitizer is a public-ish contract; a defensive test asserting the runtime-id shape would prevent surprise later.
Verified clean:
- URL fragment coverage:
url(#id),url(\"#id\"),url('#id'),url( #id ), and all funcref attrs (clip-path,filter,mask,fill,stroke,marker-start/mid/end) reach the same regex via the generic attribute walk — no per-attr whitelist to drift. - Fragment-
href:isHrefAttrNamecatcheshrefand everyfoo:hrefsuffix (coversxlink:hrefregardless of DOM impl), andrewriteHrefValueshort-circuits on non-#values sohttps://example.com/#clipand asset URLs stay untouched (explicit test). - Longer-id-shadowing:
selectorIdTokens.tssorts candidates longest-first and gates onisSelectorNameCharboundary, so#clipin the id map never eats#clip2. Explicit test in both suites. - No-SVG fast path:
querySelectorAll(\"svg [id], svg[id]\")returns empty →idMap.size === 0→ early return, no attribute walk. - Anonymous-host guard: empty
namespace→ early return with empty map, no mutation. Matches the same guardscopeCssToCompositionandwrapScopedCompositionScriptapply — consistent with existing scoping-primitive contract. getElementById(originalId)for author scripts:data-hf-authored-idis set on every renamed element;__hfGetElementByIdshim (from #646) already checks this attr as fallback. Explicit test assertsscene-aroot still resolvessymbolby authored idshapeafter rename.SVG_AUTHORED_ID_ATTRshares the exact string constant (\"data-hf-authored-id\") withAUTHORED_ROOT_ID_ATTRincompositionScoping.ts— the shim's fallback path already reads it, so no third attr introduced.- Shared scanner extraction:
replaceAuthoredRootIdSelectorsis now a thin wrapper overreplaceSelectorIdTokens; behavior preserved (single-form → one-element candidate list), one state machine to maintain instead of two. - Perf: single tree walk + O(N) attr scan + one postcss parse per composition — no visible O(N²).
CI: Preflight/lint/format/typecheck/unit/producer-integration/SDK/perf-drift/parity/fps/load/scrub/preview-parity/Fallow all green. regression-shards shards 1-9, Smoke: global install, Render/Tests on windows-latest, Test, Analyze (javascript-typescript) still pending — the render-parity signal is exactly what would exercise the visual repro so worth waiting on before merge.
Signature: — Via
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
SVG ids referenced exclusively by JavaScript (e.g. GSAP's
`tl.to("#cut-1")`) must not be renamed — global libraries access
`document` directly and bypass the composition-scoped querySelector
Proxy, so renamed ids break animation targeting.
Now namespaceSvgIds pre-scans for url(#id) funcrefs and href="#id"
fragment refs (both attribute values and <style> text content) and
only renames ids that appear in at least one native reference. Ids
with no native reference (only used by JS) keep their original
names.
Fixes style-17-prod regression where GSAP selectors targeted
#cut-1/#tear-path-1 etc. which were unreachable after rename.
Co-Authored-By: miga-heygen <miguel.sierra_miga@heygen.com>
terencecho
left a comment
terencecho
left a comment
There was a problem hiding this comment.
COMMENT — R1 SVG id namespacing @ 75b84322586c06a4f8b9c84415a9e9a8465639a6. No P0/P1 blockers; three orthogonal follow-ups. Holding off APPROVE only because regression-shards (esp. shard-5 which contains style-17-prod) and preview-parity are still in-flight and are the load-bearing signals for both this fix AND the regression it patches.
Concurring with @vibook-bot's R1 (unchanged at head):
- ARIA id-refs (
aria-labelledby/-describedby/-controls/-owns/-flowto) still not tracked. Same class asurl(#)/href="#"— native browser resolution — worth a follow-up. [id="foo"]attribute-selector inside<style>intentionally skipped by the bracket-guard mask.- No idempotency guard on
SVG_AUTHORED_ID_ATTR— a re-entry double-prefixes and clobbers the original. - No golden-frame test locking the #3490 repro; leaning on regression-shards + preview-parity to catch it.
Independent concerns (differentiated):
-
Regression-fix trade-off worth documenting in-code. The
75b8432pivot to "only rename ids that have a nativeurl()/hrefreference" is the right call forstyle-17-prod(GSAP →document.querySelectorbypasses the scoped shim, so a renamed id is unreachable). But it introduces a residual cross-comp collision path: two composition instances that both animate the SAME JS-only id (e.g. two catalog scenes each doingtl.to("#cut-1")) now BOTH keepid="cut-1"→ merged doc has twoid="cut-1"→ scene B's GSAP targets scene A's element — the exact document-order-resolution bug this PR fixes, now residual for the JS-only subset. Sound trade-off (less common than the always-broken JS case), but the module doc should call it out so a future reader doesn't broaden the pre-scan and silently re-breakstyle-17. Suggest a comment nearcollectNativelyReferencedIdsnaming this residual class. -
Test gap paired with #1. No test asserts the two-comps-sharing-JS-only-id case — either as the "we accept this collision" contract or a scoped mitigation. Would lock in the current design.
-
Runtime-injected
url(#id)isn't pre-scanned. A<script>that later doesel.setAttribute("clip-path", "url(#foo)")as the ONLY reference to#foomisses the pre-scan →fooisn't renamed → same-class collision if duplicated across comps. Unlikely in HF composition authoring, but a known blind spot.
Verified clean at 75b8432:
url(#..)(bare/quoted/single-quoted/spaced), all funcref attrs (clip-path/filter/mask/fill/stroke/marker-*), inlinestyleattr, and<style>text content — all routed through the sameURL_HASH_REF_RE. No per-attr whitelist to drift.href/foo:href(coversxlink:hrefregardless of DOM impl); non-fragment hrefs (https://example.com/#clip) untouched, explicit test.- Pre-scan scope is per-comp (
namespaceSvgIds(innerRoot ?? contentDoc, ns)) — no cross-comp poisoning path. - Multi-instance same-src block:
assignBundledRuntimeCompositionIdsgives each instance a distinct runtime id → distinct namespace. End-to-end test locks it. data-hf-authored-idreuses the exact string constant fromcompositionScoping.ts—__hfGetElementByIdfallback already checks it, no third attr introduced.- Shared
selectorIdTokens.tsscanner:replaceAuthoredRootIdSelectorsis now a thin wrapper — one state machine to maintain, longest-first sort preserved (#clipnever eats#clip2). <style>extraction pipeline: sub-comp<style>textContent (including SVG-inline<style>) is extracted viaplan.styleSourcesand rewritten byrewriteSvgIdReferencesInCssbeforescopeCssToCompositionscopes it. Ordering is correct (rename → asset URL rewrite → composition scope).
CI: many required checks in-progress at snapshot; nothing failing. regression-shards (esp. shard-5 containing style-17-prod) and preview-parity are the load-bearing signals for both the primary fix AND the regression it patches — worth waiting on before merge. Happy to bump to APPROVE once those settle green.
— Review by tai (pr-review)
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Validated against current main c98d6fb with the public #3490 fixture in Chrome: the collision reproduces, and this patch restores the missing/incorrect native SVG paint. However, I cannot recommend merging at 75b8432 because renaming also breaks working author code.
- Blocker — selector compatibility: packages/core/src/compiler/svgIdNamespacing.ts:203-205 renames any natively referenced ID, even when scripts also use it. With
<svg><path id="shape"/><use href="#shape"/></svg>,svg.querySelector("#shape")now returns null, even in a single composition. Preserving the authored attribute/getElementById shim does not preserve native Element selector APIs or GSAP selector arrays. Add a regression covering native and script references to the same element and preserve those lookups before landing. This is the mixed-use case beyond the previously noted JS-only collision. - Blocker — escaped CSS IDs: svgIdNamespacing.ts:221-225 searches raw IDs and emits unescaped replacement selectors. For
id="fx.1",#fx\.1stops matching after rename. Match decoded CSS IDs and emit valid escaped selectors; cover an actual styled element whose ID is also referenced natively.
The original issue is worth fixing, but a partial selector shim still introduces regressions. Keeping this PR open for a complete compatibility fix rather than publishing the partial successor explored during this audit.
Verdict: REQUEST CHANGES
Reasoning: The native rendering improvement is demonstrated, but valid existing selector-based author code and escaped-ID styling regress.
— Codex
`replaceSelectorIdTokens` compared the raw text after `#` against the candidate ids, so an escaped selector such as `#fx\.1` never matched the element whose id attribute is `fx.1`, and callers spliced in unescaped replacements that were invalid CSS for ids containing `.`, `:` or a leading digit. The scanner now reads the token as a CSS identifier per CSS Syntax Level 3 (`\.`-style and `\HEX `-style escapes, U+FFFD for out-of-range code points) before comparing, and exports a spec-faithful `escapeCssIdentifier` (the `CSS.escape` algorithm) for emitting replacements. Consuming the whole identifier also makes the token boundary check exact for non-ASCII and escaped name characters. Also removes the ambiguous `[^\]]` branch from the guarded-segment regex so a bracket body cannot match both the quoted and bare alternatives (flagged as potential exponential backtracking). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…resolving
Renaming every natively referenced SVG id broke author code even in a
single composition: with `<path id="shape"/><use href="#shape"/>`,
`svg.querySelector("#shape")` returned null because the id had been
prefixed although nothing collided. The getElementById shim did not
cover Element selector APIs or GSAP selector arrays, and escaped
stylesheet selectors (`#fx\.1` for `id="fx.1"`) stopped matching after
the rename.
Namespacing now runs once per assembled document, after every instance
is inlined, and is collision-driven: an id is renamed only when the
merged document would otherwise carry it more than once. One colliding
element keeps the authored id - the first in document order that cannot
be renamed (top-level document content, or a JS-only id), else the
first in document order - so native `url(#id)`/`href="#id"` resolution
keeps binding to it, and every other renamable duplicate gets a
document-unique id with its references rewritten. Ids that never
collide, including every single-composition project, are byte-for-byte
unchanged. The demand-driven gate is kept: JS-only ids are never
renamed, and the residual cross-instance collision for that class is
documented in `collectNativelyReferencedIds`. Nested hosts are excluded
from their parent's scope, minted ids are checked against every id in
the document, and an existing data-hf-authored-id is never overwritten.
The composition script runtime keeps `#authoredId` selectors resolving
for renamed elements by rewriting the token to
`:is(#authoredId, [data-hf-authored-id="authoredId"])` - a superset, so
the same selector still matches an untouched instance:
- scoped `document.querySelector/querySelectorAll` and the GSAP proxy
(`gsap.to`, `timeline.to`, `gsap.utils.toArray/selector`);
- GSAP array targets, whose string entries now resolve through the same
scoped lookup instead of the global document;
- `Element.prototype.querySelector/querySelectorAll`, patched once per
document and only when at least one id was actually renamed, so a
document without collisions runs untouched natives. The renamed-id
set is discovered from `[data-hf-authored-id][id]` in the compiled
DOM, so no extra plumbing between compiler and runtime is needed.
`Document.prototype` is deliberately left alone: an unscoped
document-wide lookup by a third-party library cannot know which
instance it means and resolves to the first in document order, exactly
as before. `matches`/`closest` and selectors built at runtime with hex
escapes are likewise outside the shim.
Stylesheet rewriting matches ids through CSS identifier escapes and
emits escaped replacements, so `#fx\.1 { ... }` becomes
`#scene-b--fx\.1 { ... }` and `url(#fx.1)` becomes `url(#scene-b--fx.1)`.
Tests: unit coverage for the collision/keeper rules, exclusion, id
minting, escaped selectors and hex escapes; end-to-end regressions that
execute the compiled scoped scripts in a real DOM for (i) a single
composition with native and script references to the same element,
(ii) two colliding compositions where native refs and every script
lookup form resolve to their own element, and (iii) an escaped id that
is styled and natively referenced keeping its rule after rename.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Addressed both blockers at Blocker 1 — selector compatibility
Blocker 2 — escaped CSS ids
CI follow-up — head is now Local results: — Miga |
…n suite
`inlineSubCompositions.test.ts` imported `assignBundledRuntimeCompositionIds`
from `./htmlBundler` only to give duplicate hosts distinct runtime ids.
`htmlBundler.ts` statically imports esbuild, and esbuild refuses to load
inside a jsdom test realm on Windows ("new TextEncoder().encode('')
instanceof Uint8Array is incorrectly false"), which made the whole suite
fail to load in the Windows test job while every other suite passed.
The test now carries a small local mirror of the bundler's assignment
(`<id>__hf<n>` for hosts whose authored id repeats), so the suite has no
esbuild dependency. Test behaviour is unchanged.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-reviewed 04aa0f931e9184a5fc03072061757a52d6fb6738. The 88 focused scoping/SVG/selector tests pass and all eight required CI contexts are green. The original direct-query, selector-array, and ordinary escaped-ID cases are covered, but two executable regressions remain.
-
[P2] Preserve CSS selector grammar in the runtime shim —
packages/core/src/compiler/compositionScoping.ts:354-355registers both raw and escaped spellings of every renamed ID. If an authored ID isfoo.bar, the valid compound selector#foo.bar(idfoo, classbar) gets rewritten as the literal IDfoo.bar. I executed the compiled wrapper with both elements present:svg.querySelector("#foo.bar")returnedscene--foo.barinstead offoo. Only the escaped#foo\.barspelling should target that literal ID. Decode selector tokens rather than matching arbitrary raw ID strings, and cover both selectors side by side. -
[P2] Preserve inherited SVG references across nested scopes —
packages/core/src/compiler/svgIdNamespacing.ts:236-240/inlineSubCompositions.ts:460-463exclude child subtrees from parent rewriting, but a child with no local definition does not inherit the parent rename map. ActualinlineSubCompositionsrepro: parent defines/uses red#paint; nested child usesfill="url(#paint)"; a later top-level SVG defines blue#paint. The parent becomesparent--paint, while the child remainsurl(#paint)and binds to blue instead of its previous red definition. Preserve the original reference target when a nested child does not shadow it, including extracted child CSS.
Verdict: REQUEST CHANGES
Reasoning: The earlier cases improved, but these two cases still silently change which element or SVG resource author code references.
— Codex
6 participants
Summary
<clipPath id="clip">,<symbol id="shape">,<filter id="fx">, etc. no longer collide once two nested scenes are merged into one render/preview document.href,xlink:href, anyurl(#id)value —clip-path,filter,mask,fill,stroke,marker-start/mid/end, including insidestyle) and the composition's own extracted<style>text (both#idselectors andurl(#id)declaration values).data-hf-authored-id(the same attribute fix(core): scoped getElementById fails with duplicate element IDs across sub-compositions #646 added for the composition root), so an inline script's owndocument.getElementById(originalId)keeps resolving via the existing__hfGetElementByIdscoping shim — the fix doesn't regress the already-fixed getElementById scoping.selectorIdTokens.tsfromcompositionScoping.ts's existing single-id selector scan so the new many-id rewrite reuses the same guarded-region (quote/bracket) logic instead of a second copy.Why not the media-id / getElementById approach?
getElementByIdwas already scoped per composition in #646, and media pipeline ids got a paralleldata-hf-render-idattribute in #3340 — both work by adding a side-channel attribute without touching the realid. That doesn't work here:url(#id)andhref="#id"are resolved by the browser's native SVG/CSS engine, which always binds to the first element in document order carrying that literalidattribute. No JS proxy can intercept native resolution, so theidattribute itself has to become document-unique.Closes #3490
Test plan
packages/core/src/compiler/svgIdNamespacing.test.ts(new, 13 tests): unit coverage for id renaming,url()/hrefrewriting acrossclip-path/filter/mask/fill/stroke/marker-*/style,xlink:href, and CSS selector/declaration rewriting.packages/core/src/compiler/inlineSubCompositions.test.ts(new suite): end-to-end repro of the issue — two sibling scenes reusing#clip/#shape/#fxget distinct non-colliding ids that still resolve correctly; the same catalog block used twice in one scene is disambiguated;getElementById(originalId)still resolves for an inline script after rename.bun run typecheck— clean.oxlint— clean.compositionScoping.test.ts(50),htmlBundler.test.ts(58, exercisesinlineSubCompositionsend-to-end via the preview bundler) — no regressions.Review follow-up (compatibility)
Two review blockers changed the semantics from "rename every natively referenced SVG id" to the following:
namespaceCollidingSvgIdsruns once per assembled document, after every instance is inlined, and renames an id only when the merged document would otherwise carry it more than once. Ids that never collide — including every single-composition project — are byte-for-byte unchanged, sosvg.querySelector("#shape")/document.querySelector("#shape")/gsap.to("#shape")keep working exactly as before. Among colliding elements one keeps the authored id (the first in document order that cannot be renamed — top-level document content or a JS-only id — else the first in document order); nativeurl(#id)/href="#id"resolution binds to that keeper, and every other renamable duplicate gets a document-unique id with its references rewritten. The JS-only gate from the previous revision is kept; its residual (two instances sharing a JS-only id) is documented incollectNativelyReferencedIds.#authoredIdto:is(#authoredId, [data-hf-authored-id="authoredId"])(a superset, so it still matches an untouched instance) in the scopeddocument.querySelector/querySelectorAll, the GSAP proxy (gsap.to,timeline.to,gsap.utils.toArray/selector), GSAP array targets (string entries now resolve through the scoped lookup), andElement.prototype.querySelector/querySelectorAll— patched once per document and only when at least one id was actually renamed (discovered from[data-hf-authored-id][id]in the compiled DOM), so documents without collisions run untouched natives.\.and\HEXforms, per CSS Syntax Level 3) before matching and emits replacements through aCSS.escape-equivalent, so#fx\.1 { … }forid="fx.1"becomes#scene-b--fx\.1 { … }andurl(#fx.1)becomesurl(#scene-b--fx.1). The guarded-segment regex also lost its ambiguous[^\]]branch (CodeQL backtracking alert).Not covered by the selector shim (documented in code):
Document.prototypeis deliberately not patched — an unscoped document-wide lookup by a third-party library (e.g.anime({ targets: "#id" })) cannot know which instance it means and resolves to the first in document order, exactly as before this PR;Element.prototype.matches/closestand script selectors using hex escapes at runtime are not rewritten; references a script injects at runtime (el.setAttribute("clip-path", "url(#x)")as the only reference) are not pre-scanned.Test plan (follow-up)
svgIdNamespacing.test.ts(24): single composition byte-for-byte unchanged with native refs; two non-colliding compositions untouched; JS-only colliding ids kept; keeper/rename rules incl. parent-document and JS-only keepers; nested-scope exclusion; id minting against taken ids; authored-id idempotency; escaped selector and hex-escape rewriting.selectorIdTokens.test.ts(10, new): CSS identifier decoding (character/hex escapes,\r\n, U+FFFD),CSS.escapealgorithm parity and round-trip, whole-token and guarded-region behaviour.inlineSubCompositions.test.ts(35): end-to-end regressions that execute the compiled scoped scripts in a real DOM — (i) single composition, native + script references to the same element; (ii) two colliding compositions, native refs and every script lookup form (document.querySelector,svg.querySelector,getElementById, GSAP array/toArray) resolve to their own element; (iii)id="fx.1"styled via#fx\.1and referenced viafilter="url(#fx.1)"keeps its rule after rename.compositionScoping.test.ts(54): Element.prototype shim installs only when an id was renamed; element/document/GSAP-array lookups; escaped authored id in a script selector.htmlBundler.test.ts(58) unchanged and green;bun run typecheck(core + producer) clean;oxlint,oxfmt, andfallow auditclean.inlineSubCompositions.test.tsno longer imports./htmlBundler(whose static esbuild import cannot load in a jsdom realm on Windows); it carries a local mirror of the duplicate-host runtime-id assignment instead.Co-Authored-By: Miga noreply@anthropic.com
🤖 Generated with Claude Code