Skip to content

fix(sdk,studio): restore DOM edit cutover parity#1565

Open
miguel-heygen wants to merge 3 commits into
mainfrom
fix/dom-edit-sdk-parity
Open

fix(sdk,studio): restore DOM edit cutover parity#1565
miguel-heygen wants to merge 3 commits into
mainfrom
fix/dom-edit-sdk-parity

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

Problem

Studio DOM-edit cutover could route setStyle, setText, and setAttribute through the SDK without byte-level proof that the serialized HTML matched the legacy patch-element server path. That left the cutover exposed to subtle drift in style="" bytes, especially data URLs with semicolons, style removal/coalescing, and nested text targets.

Fixes #1550.

What this fixes

  • Adds a DOM-edit parity corpus that applies the same PatchOperation[] through patchElementInHtml and the SDK cutover mapping, then asserts exact serialized HTML equality.
  • Makes SDK inline-style parsing quote/paren-aware so values like url(data:image/svg+xml;utf8,...) survive when a different style property changes.
  • Aligns safe SDK text targeting with the legacy single-HTML-child behavior and makes resolver-shadow text snapshots read the same target the SDK mutates.
  • Declines SDK cutover for text shapes that cannot be both byte-identical and undo-safe with the current scalar text patch model, keeping the legacy server path authoritative for multi-child and non-HTML single-child targets.

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.ts
  • bun run --filter @hyperframes/studio test src/utils/sdkCutoverParity.test.ts src/utils/sdkCutover.test.ts src/utils/sdkResolverShadow.test.ts
  • bun run --filter @hyperframes/sdk typecheck
  • bun run --filter @hyperframes/studio typecheck
  • bunx 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.ts
  • bunx 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.ts
  • Lefthook pre-commit passed, including Fallow. Fallow still reports inherited duplication in sdkCutover.test.ts, but the gate excludes those inherited findings and reports no new issues in the changed files.

Browser

  • Built runtime inline assets with bun run --filter @hyperframes/core build:hyperframes-runtime before opening Studio.
  • Started Studio at http://127.0.0.1:5190/#/sdk-parity-proof and used agent-browser to open the proof project, select the styled target, and inspect the Fill panel.
  • Confirmed the Inspector preserved data:image/svg+xml;utf8,%3Csvg%3E%3C/svg%3E in the selected element's External URL field.
  • Ran a browser-context SDK-vs-legacy parity assertion through Vite modules; it returned matched: true, equal: true, and backgroundPreserved: true while changing color from red to blue.
  • No page errors were reported. The only console warnings during the parity assertion were Vite externalization warnings from deliberately importing the Node-oriented legacy mutation helper in the browser context.
  • Local evidence artifacts: tmp/sdk-parity-proof/browser-proof-final.png and tmp/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

  • This does not add a new public SDK API.
  • The legacy patchElementInHtml path remains the byte oracle for DOM-edit cutover parity.
  • The SDK still preserves undo-safe scalar text behavior outside the safe legacy-parity text shapes.

Compound Engineering
GPT-5

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 legacy patchStyleAttrString (sourceMutation.ts:235-262): quote tracking via single open-quote ref, paren-depth tracking, split only when outside both. Custom-property pass-through (toCamel/toKebab early-return on --) preserved.
  • resolveSingleChildTextTarget (model.ts:229-232) matches legacy text-target shape (sourceMutation.ts:322-323) byte-for-byte. getOwnText/setOwnText route through it. ✅
  • snapshotText (document.ts:32) now routes through getOwnText, 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-219 cover 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 setText on <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 (#1547 already merged at 933b88ec, so should be fine).

Review by Rames D Jusso

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

  1. Style splitting on raw ;parseStyleAttr split on every ;, corrupting semicolon-bearing values (url(data:image/svg+xml;utf8,…)). New quote/paren-aware splitStyleDeclarations is correct.
  2. Single-child text targeting — SDK edited direct text nodes; legacy targets a lone HTML child's textContent. resolveSingleChildTextTarget now mirrors legacy exactly (incl. textContent = replacing the child subtree — matches legacy, not a regression).
  3. 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 quotescontent: "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

miguel-heygen and others added 2 commits June 18, 2026 22:46
- 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 james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.

  2. Sibling asymmetry at decline gate — resolved-differently with rationale. The hardcoded list at sdkCutover.ts:175 is now extracted to a named NON_HTML_CHILD_TAGS set with a docstring (sdkCutover.ts:163-169) explicitly noting the engine's instanceof HTMLElement check can't be reused because target is 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.

  3. Dispatch ordering inside batched cutover — untouched. patchOpsToSdkEditOps call site at sdkCutover.ts:325 unchanged; sdkOpMapping.ts not in the delta. Deferred-to-Vance item didn't regress.

  4. Intentional <div><svg/></div> setText divergence — untouched. Test at mutate.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.

  5. SDK cutover route bypassed isAllowedHtmlAttribute + isSafeAttributeValue — closed. New module-private helpers at sdkCutover.ts:55-145 mirror the canonical sourceMutation gates (event-handler block, allowlist, data-/aria- prefix carve-out, URI scheme block, data:text/html block). New hasUnsafeHtmlAttributeOp participates in shouldUseSdkCutover at sdkCutover.ts:196. Test corpus covers all four shapes: onclick/onload, off-allowlist (formaction), javascript:/vbscript: URIs, data:text/html URIs (sdkCutover.test.ts:101-129).

New findings (delta-introduced)

  1. 🟡 Three-copy allowlist / URI-attr drift riskALLOWED_HTML_ATTRS, URI_ATTRS, and DANGEROUS_URI_SCHEMES / DANGEROUS_DATA_URI regexes now exist in THREE places: sdkCutover.ts:55-100, packages/core/src/studio-api/helpers/sourceMutation.ts:136-204, and packages/sdk/src/engine/mutate.ts:60-67 (the SDK engine's URI_BEARING_ATTRS already diverges — it includes xlink:href, the other two don't; not currently exploitable since xlink:href isn'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-with block comment beats invisible drift. Not a blocker — current state is safe — but worth filing as follow-up. Reference: feedback_sibling_asymmetry_as_security_evidence.

  2. Quoted-string escape handling in splitStyleDeclarations (delta-only addition) — new skip-flag handling for \ escapes inside quoted CSS string values at model.ts:144-176, covered by mutate.test.ts:406-412 ("handles escaped quotes inside CSS string values" asserting content: "a\";b" survives a sibling color: blue set). Tight fix, isolated, test pins the failure shape.

  3. 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No SDK-vs-legacy serialization parity for DOM-edit cutover ops

3 participants