fix(sdk,studio): restore DOM edit cutover parity#1565
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
Read the corpus + parser fix at 0253fead. The shape matches issue #1550 cleanly — Carlos asked for byte-level SDK-vs-legacy parity on DOM-edit cutover ops, and the new sdkCutoverParity.test.ts is precisely that: each case applies the same PatchOperation[] through both patchElementInHtml and the SDK cutover mapping and asserts exact serialized-HTML equality. The parser bug the corpus surfaced — parseStyleAttr splitting on every ;, mangling url(data:image/svg+xml;utf8,...) — is a real silent drift, and the quote/paren-aware splitStyleDeclarations in packages/sdk/src/engine/model.ts:141-174 is the right fix. The decline path for shapes that cannot be both byte-identical and undo-safe (multi-child targets, single non-HTML children like svg/math) is honest scope demarcation, not a band-aid: the legacy server path stays authoritative for those shapes, and packages/studio/src/utils/sdkCutover.test.ts:208-227 pins the decline behavior.
Three observations — none blocking, two follow-up-corpus-extension suggestions and one verified-clean heads-up:
1. Shorthand-vs-longhand parity isn't directly exercised. Carlos's #1550 enumeration listed inset vs top/right/bottom/left as an expected drift shape. The corpus case "removes one inline style from a multi-property declaration" uses inset: 0 in the source HTML but only mutates opacity, so the shorthand survives untouched — what that case pins is "inset survives when other props change," not "mutating one component of inset (or setting inset over existing longhands) yields the legacy bytes." Worth a follow-up corpus case: setStyle({ top: "10px" }) on a source carrying inset: 0, and the converse — setStyle({ inset: "0" }) on a source carrying top/right/bottom/left. That's the one named shape from #1550 the current corpus skips.
2. splitStyleDeclarations skips CSS string escapes. The scanner toggles quote state on a literal ' or " but doesn't honor \ inside strings — content: "a\";b" would split at the embedded ; even though the \" is a CSS-escaped quote inside the string token. In practice inline style="" very rarely carries escape-bearing string values (content is the obvious case and isn't common on element styles), so this is narrow rather than load-bearing — but if the corpus ever extends into generative fuzzing or content-like ops, this surfaces. Logging it here so the gap is visible if those ops ever land in the cutover.
3. HyperFramesElement.text semantics widened — verified clean downstream. Before this PR, text returned null for <button><span>Old</span></button> (button had no direct text node, prior ownText only summed nodeType === 3 children). After this PR, text returns "Old" via the new resolveSingleChildTextTarget path through getOwnText. The types.ts:21 docstring change calls this out, and it's the intended legacy-parity alignment — I grepped the HF repo for el.text === null / element.text === null patterns and got zero hits, so no consumer was gating UX off the old null sentinel. Surface change is benign in practice; flagging the contract widening here because it's load-bearing for anyone reading this diff later as a regression hunt.
The browser-context parity assertion in your verification log (matched: true, equal: true, backgroundPreserved: true across a data:image/svg+xml;utf8,... background while changing color) is precisely the byte-level proof shape #1550 was missing. MERGEABLE against main, base is current, no stale-stack hazard.
Review by Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at HEAD 0253fead. Canonical-rubric pass (parity/sibling-asymmetry/scope); deferring DOM-edit dispatch runtime semantics to @vanceingalls who owns the cutover stack.
Verdict: Lean-approve with two concerns worth Miguel's read before stamp.
⚠️ Concern 1 — duplicate-fix coordination with #1562
#1562 (Carlos / calcarazgre646, opened ~4h before this PR) is the narrower form of the same inline-style semicolon fix — different splitStyleDeclarations shape, same root cause from issue #1550, same files (model.ts + mutate.test.ts). This PR supersedes it in scope (also addresses text targeting + adds the parity corpus Carlos asked about in #1550's Question section), but the two will conflict on merge.
Worth a heads-up to Carlos so #1562 can close in favor of this one, or coordinate which lands. The mechanical changes diverge slightly (this PR factors advanceStyleDeclarationScan into a state-mutating helper; #1562 inlines the same logic with a different shape) so a merge wouldn't pick the right one automatically.
⚠️ Concern 2 — shouldDeclineTextCutoverForTarget decline list vs isHTMLElementTarget
sdkCutover.ts:80-81 declines single-child text cutover when tag === "svg" || tag === "math" (hardcoded). The engine's isHTMLElementTarget (model.ts:223-227) uses el instanceof HTMLElement — a runtime check that catches any non-HTML namespace element, not just those two tags.
In practice, parsed-as-HTML body content effectively limits non-HTMLElement children to SVG + MathML, so this is functionally equivalent today. But the two assertions are answering "is this an HTML element child?" with different mechanisms, so they can drift silently if linkedom (or whatever the upstream parser becomes) starts surfacing other foreign-content elements as non-HTMLElement. The legacy sourceMutation.ts:323 also uses isHTMLElement(inner) — so the cutover decline gate is the only one with a hardcoded tag list.
Suggestion: have shouldDeclineTextCutoverForTarget consult a more authoritative shape (e.g. a list shared with the engine, or just widen the comment to say "linkedom-parsed-as-HTML namespaces only" with a pointer to isHTMLElementTarget). Non-blocking — purely a future-drift hardening.
💡 Suggestion — html-attribute safety asymmetry (pre-existing; surfaces with this PR's parity goal)
The PR's stated thesis is byte-level parity with legacy patch-element. The legacy applies isAllowedHtmlAttribute + isSafeAttributeValue (sourceMutation.ts:312-318) before mutating. The SDK cutover route through patchOpsToSdkEditOps for html-attribute (sdkOpMapping.ts:32) bypasses both — an onclick op or a javascript: URI on href would be DECLINED at the legacy boundary and ACCEPTED at the SDK boundary, with completely different bytes. The parity corpus only covers safe attrs (aria-label, data-mode), so this gap isn't tested.
Pre-existing (introduced when html-attribute cutover landed), not regressed here — but flagging because the PR's parity-corpus test could pin this with a "PR should decline" case to prevent future widening, and because "the legacy path remains the byte oracle" in the PR body is undermined for these inputs. Probably out of scope for this PR; could be follow-up.
💡 Suggestion — parity corpus doesn't cover mixed-type ops in one batch
All corpus cases (sdkCutoverParity.test.ts:23-71) use a single op type per case (just inline-style, just text, just attribute). patchOpsToSdkEditOps coalesces inline-style ops into one setStyle and unshifts it to the front, reordering relative to other op types. Legacy applies ops sequentially in the original order. If production ever batches [setStyle, setAttribute] together, the SDK applies setStyle-first regardless of input order. Worth a mixed-type case (or a comment explaining batches are single-type in practice).
✅ Verified
splitStyleDeclarations(model.ts:159-173) state machine correctly mirrors legacypatchStyleAttrString(sourceMutation.ts:235-262): quote tracking via single open-quote ref, paren-depth tracking, split only when outside both. Custom-property pass-through (toCamel/toKebabearly-return on--) preserved.resolveSingleChildTextTarget(model.ts:229-232) matches legacy text-target shape (sourceMutation.ts:322-323) byte-for-byte.getOwnText/setOwnTextroute through it. ✅snapshotText(document.ts:32) now routes throughgetOwnText, closing the resolver-shadow text-snapshot vs SDK-mutation-target asymmetry. The shadow's text comparison (sdkResolverShadow.ts:107) reads from the same target the SDK writes to.- Engine tests at
mutate.test.ts:190-219cover the three legacy-text-target shapes (single HTML child, single child + parent text, non-HTML single child fallthrough). - All 36 CI checks green at
0253fead.
🟦 Deferring to @vanceingalls
- DOM-edit dispatch ordering semantics inside the cutover batch (whether
unshift-setStyle-to-front is correct in his cutover state model). - The decision to make the SDK's
setTexton<div><svg/></div>append "New" as a sibling text node after the svg (mutate.test.ts:218) rather than match legacy's "wipe everything and write text" — this is intentional per the PR body, but Vance owns the call on whether that's the right SDK-side semantic forever (legacy wipes; declined cutover preserves the divergence). Currently safe because the cutover declines, but it's a permanent asymmetric corner. - Whether the resolver-shadow text alignment lands correctly relative to his open shadow-tripwire work (
#1547already merged at933b88ec, so should be fine).
Review by Rames D Jusso
vanceingalls
left a comment
There was a problem hiding this comment.
Strong PR — ship-worthy. This is exactly the DOM-edit serialization-parity coverage #1550 asked for (a differential corpus: same op through SDK serialize() and legacy patchElementInHtml, asserting byte-equal HTML) — plus it fixes the two real divergences the corpus exposed.
What it fixes (both genuine)
- Style splitting on raw
;—parseStyleAttrsplit on every;, corrupting semicolon-bearing values (url(data:image/svg+xml;utf8,…)). New quote/paren-awaresplitStyleDeclarationsis correct. - Single-child text targeting — SDK edited direct text nodes; legacy targets a lone HTML child's
textContent.resolveSingleChildTextTargetnow mirrors legacy exactly (incl.textContent =replacing the child subtree — matches legacy, not a regression). - Multi-child / non-HTML child → declines cutover (server stays authoritative), since the SDK scalar inverse can't be both byte-identical and undo-safe there. Principled partial cutover.
Findings
1. [Medium] Decline predicate (svg/math denylist) ≠ legacy predicate (isHTMLElement allowlist).
shouldDeclineTextCutoverForTarget declines a single-child text target only when tag === 'svg' || 'math'. Legacy edits the inner child only if isHTMLElement(inner) — it treats every non-HTML element as "replace the whole element":
const inner = htmlEl.children.length === 1 ? htmlEl.firstElementChild : null;
const textTarget = inner && isHTMLElement(inner) ? inner : htmlEl;A lone child that is neither HTML nor svg/math slips past the decline → SDK resolveSingleChildTextTarget returns null → setOwnText preserves the child via the direct-text-node path, while legacy replaces the whole element → byte divergence on a hard-cutover write. Low likelihood (svg/math are the realistic non-HTML cases in HF comps) and there's no test for it, but it's an asymmetry in a parity-critical path. Suggest mirroring legacy: decline when the lone child is not an HTML element (a snapshot isHtml/namespace flag, or known-HTML-tag check), rather than enumerating svg/math — and add a corpus/decline case for it.
2. [Low] splitStyleDeclarations doesn't handle backslash-escaped quotes — content: "a\";b" flips quote state on the escaped " and splits early. Rare in inline style, latent, not in the corpus.
3. [Low] parseStyleAttr dropped the !value skip (!rawProp || !value → !rawProp) — an empty-value declaration (prop:) is now retained where it was discarded. Presumably intentional for parity; worth a one-line comment, the corpus doesn't cover it.
4. [Low/cleanliness] Parity test deep-imports ../../../core/src/studio-api/helpers/sourceMutation.js (core doesn't export patchElementInHtml). Fine for a test; a core export would be less fragile.
Net
Approve with finding #1 addressed (or explicitly accepted + a test asserting the non-HTML single-child case declines). Closes the DOM-edit surface of #1550 and de-risks the STUDIO_SDK_CUTOVER_ENABLED flip.
🤖 Reviewed with Claude Code
- Fix CSS string escape gap in splitStyleDeclarations: honor backslash escapes inside quoted values so `content: "a\";b"` doesn't split on the embedded semicolon - Extract NON_HTML_CHILD_TAGS set in sdkCutover.ts with a comment pointing to the engine's isHTMLElementTarget for future-drift hardening - Add shorthand-vs-longhand parity corpus cases (inset over longhands and longhand over inset) per Via's review - Add mixed-type batch corpus cases (style+text, style+attribute) per Rames D Jusso's review Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The legacy sourceMutation path gates html-attribute ops through isAllowedHtmlAttribute (blocks event handlers, unlisted attrs) and isSafeAttributeValue (blocks javascript:/vbscript: URIs, data:text/html). The SDK cutover bypassed both checks, so onclick or javascript: href ops would be DECLINED by the legacy path but ACCEPTED by the cutover. Mirror both checks in shouldUseSdkCutover so unsafe html-attribute ops decline the whole batch to the server path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R2 — tight delta verification of R1 concerns
HEAD 9dc5980e (R1 was 0253fead). Two commits in the delta: 44b0f9de (review-feedback) + 9dc5980e (html-attr safety gap).
R1 findings
-
✅ Duplicate-fix overlap with #1562 — resolved. You posted the heads-up at #1562, Carlos closed in favor (#1562 closed
2026-06-18T23:00:28Z, no merge). Clean coordination, no conflicts. Carlos got his #1550 credit acknowledged. -
✅ Sibling asymmetry at decline gate — resolved-differently with rationale. The hardcoded list at
sdkCutover.ts:175is now extracted to a namedNON_HTML_CHILD_TAGSset with a docstring (sdkCutover.ts:163-169) explicitly noting the engine'sinstanceof HTMLElementcheck can't be reused becausetargetis a plain SDK object, not a DOM Element, and citing the foreign-content invariant. That's the right shape — future drift becomes detectable at the named-const, the invariant is documented in-tree, and there's a clear maintenance contract for adding tags. Not a shared helper, but the constraint is real. -
✅ Dispatch ordering inside batched cutover — untouched.
patchOpsToSdkEditOpscall site atsdkCutover.ts:325unchanged;sdkOpMapping.tsnot in the delta. Deferred-to-Vance item didn't regress. -
✅ Intentional
<div><svg/></div>setText divergence — untouched. Test atmutate.test.ts:218("keeps non-HTML single children out of the child text shortcut") still asserts<svg ...><text>...Old</text></svg>New</div>exactly. Deferred-to-Vance item didn't regress. -
✅ SDK cutover route bypassed
isAllowedHtmlAttribute+isSafeAttributeValue— closed. New module-private helpers atsdkCutover.ts:55-145mirror the canonical sourceMutation gates (event-handler block, allowlist, data-/aria- prefix carve-out, URI scheme block, data:text/html block). NewhasUnsafeHtmlAttributeOpparticipates inshouldUseSdkCutoveratsdkCutover.ts:196. Test corpus covers all four shapes:onclick/onload, off-allowlist (formaction),javascript:/vbscript:URIs,data:text/htmlURIs (sdkCutover.test.ts:101-129).
New findings (delta-introduced)
-
🟡 Three-copy allowlist / URI-attr drift risk —
ALLOWED_HTML_ATTRS,URI_ATTRS, andDANGEROUS_URI_SCHEMES/DANGEROUS_DATA_URIregexes now exist in THREE places:sdkCutover.ts:55-100,packages/core/src/studio-api/helpers/sourceMutation.ts:136-204, andpackages/sdk/src/engine/mutate.ts:60-67(the SDK engine'sURI_BEARING_ATTRSalready diverges — it includesxlink:href, the other two don't; not currently exploitable sincexlink:hrefisn't in any allowlist, but it's evidence the drift is already happening). The byte-identical extracted-helpers shape begs to be exported once and imported by all three sites. Even a// @keep-in-sync-withblock comment beats invisible drift. Not a blocker — current state is safe — but worth filing as follow-up. Reference:feedback_sibling_asymmetry_as_security_evidence. -
✅ Quoted-string escape handling in
splitStyleDeclarations(delta-only addition) — newskip-flag handling for\escapes inside quoted CSS string values atmodel.ts:144-176, covered bymutate.test.ts:406-412("handles escaped quotes inside CSS string values" assertingcontent: "a\";b"survives a siblingcolor: blueset). Tight fix, isolated, test pins the failure shape. -
✅ Parity test corpus expansion — four new cases at
sdkCutoverParity.test.ts:76-103(inset-over-longhands, top-over-inset, mixed inline-style + text-content, mixed inline-style + attribute). Good coverage for the cutover-vs-legacy byte parity.
Standing items (R1 adjacent, not regressed) — N/A. Cutover attribute path is now gated, so the R1 concern about bypassed safety is fully addressed at finding 5; the new findings layer above that.
Review by Rames D Jusso
Extract ALLOWED_HTML_ATTRS, URI_BEARING_ATTRS, DANGEROUS_URI_SCHEMES, DANGEROUS_DATA_URI, isAllowedHtmlAttribute, and isSafeAttributeValue to a single source of truth in core/src/utils/htmlAttrSafety.ts. Previously duplicated across sourceMutation.ts (core), sdkCutover.ts (studio), and mutate.ts (sdk) — the SDK copy already diverged by including xlink:href in URI_BEARING_ATTRS while the others omitted it. Now all three import from the shared module. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Problem
Studio DOM-edit cutover could route
setStyle,setText, andsetAttributethrough the SDK without byte-level proof that the serialized HTML matched the legacypatch-elementserver path. That left the cutover exposed to subtle drift instyle=""bytes, especially data URLs with semicolons, style removal/coalescing, and nested text targets.Fixes #1550.
What this fixes
PatchOperation[]throughpatchElementInHtmland the SDK cutover mapping, then asserts exact serialized HTML equality.url(data:image/svg+xml;utf8,...)survive when a different style property changes.Root cause
The SDK style parser split inline styles on every semicolon, so semicolons inside parenthesized CSS values were treated as declaration boundaries. The Studio cutover tests also proved dispatch shape, not final HTML bytes, so this drift was not caught. Text had a second mismatch: the legacy path redirects safe single-child text edits to the child element, while SDK snapshot/mutation helpers previously did not share that exact target model.
Verification
Local
bun run --filter @hyperframes/sdk test src/engine/mutate.test.tsbun run --filter @hyperframes/studio test src/utils/sdkCutoverParity.test.ts src/utils/sdkCutover.test.ts src/utils/sdkResolverShadow.test.tsbun run --filter @hyperframes/sdk typecheckbun run --filter @hyperframes/studio typecheckbunx oxfmt --check packages/sdk/src/document.ts packages/sdk/src/engine/model.ts packages/sdk/src/engine/mutate.test.ts packages/sdk/src/types.ts packages/studio/src/utils/sdkCutover.ts packages/studio/src/utils/sdkCutover.test.ts packages/studio/src/utils/sdkCutoverParity.test.tsbunx oxlint packages/sdk/src/document.ts packages/sdk/src/engine/model.ts packages/sdk/src/engine/mutate.test.ts packages/sdk/src/types.ts packages/studio/src/utils/sdkCutover.ts packages/studio/src/utils/sdkCutover.test.ts packages/studio/src/utils/sdkCutoverParity.test.tssdkCutover.test.ts, but the gate excludes those inherited findings and reports no new issues in the changed files.Browser
bun run --filter @hyperframes/core build:hyperframes-runtimebefore opening Studio.http://127.0.0.1:5190/#/sdk-parity-proofand usedagent-browserto open the proof project, select the styled target, and inspect the Fill panel.data:image/svg+xml;utf8,%3Csvg%3E%3C/svg%3Ein the selected element's External URL field.matched: true,equal: true, andbackgroundPreserved: truewhile changingcolorfromredtoblue.tmp/sdk-parity-proof/browser-proof-final.pngandtmp/sdk-parity-proof/dom-edit-parity-final.webm.Post-Deploy Monitoring & Validation
No additional operational monitoring required: this is SDK/Studio serialization logic with no server-side runtime, migration, feature flag, or background job impact. Healthy signal is the new parity corpus and focused cutover tests staying green in CI. Failure signal is any regression in
sdkCutoverParity.test.ts,sdkCutover.test.ts, or SDK mutation tests; rollback is reverting this PR to restore the previous server-fallback behavior.Notes
patchElementInHtmlpath remains the byte oracle for DOM-edit cutover parity.