fix(sdk): keep <br> line breaks editable and uncorrupted in setText

[hblee12294]

What

Fixes text elements that contain <br> line breaks (e.g. <h1>First<br>Second</h1>) being reported as non-editable, and being corrupted when their text is set. Touches getOwnText / setOwnText / resolveSingleChildTextTarget in packages/sdk/src/engine/model.ts.

Why

<br> is a void element, but resolveSingleChildTextTarget treated a lone <br> child as the element's text target (a <br> is an HTMLElement). Two consequences:

• getOwnText(<h1>First<br>Second</h1>) read the <br>'s (empty) textContent and returned "" → the document snapshot's text became null, which downstream editors (HeyGen AI Studio's inner-element panel) surface as "not editable."
• setOwnText wrote into the <br> (br.textContent = ...), corrupting it — the <br> gained a text child and serialized as invalid </br>. In the render/bake path (openComposition({ overrides }).serialize()) this showed up as duplicated lines (First / First / … / Second / Second).

How

• resolveSingleChildTextTarget — exclude void elements (<br>, <img>, <hr>, …) from
  the single-child text-target shorr hold text.
• getOwnText — surface <br> line breaks as "\n", so a multi-line leaf reads as
  editable, newline-joined text ins
• setOwnText — for a flat text leaf (no element children, or only <br>), rebuild the
  text/<br> run from the "\n"-sf getOwnText). Existing <br>nodes are reused in order, so their identity (data-hf-id, etc.) survives an in-place edit
  and undo is byte-exact; a new <be is added. Never writes text intoa
, so no more
corruption. Elements with non-
  ` element children keep the
  existing multi-child behavior.

Test plan

Added setText > <br> line-break .ts covering: reads as editablenewline-joined text (not ""), <br> preserved on edit (no </br>), line break dropped when
the new value has no newline, and-id`) round-trip on undo.

• Unit tests added/updated
• Manual testing performed — verified end-to-end in AI Studio: <br> headings are now editable, and both the live edit der bake produce correct,non-duplicated output
• Documentation updated (if a

[vanceingalls]

A crisp little forensic piece — the bug is precisely as diagnosed, the remedy is symmetric where symmetry matters, and the fixture prints seal each of the four claims the PR body advances. I have walked the diff at head d231fb7b1aefce3cca240a7ff38219224adb7de7 end-to-end and find no fracture to raise. What follows is concurrence with a small handful of curiosities, offered as lantern-light for the next maintainer rather than pressure on this one.

---

On the diagnosis. The report reads with the precision of a well-kept case-file: resolveSingleChildTextTarget returning a lone <br> for <h1>First<br>Second</h1> (because el.children.length === 1 and isHTMLElementTarget(<br>) === true), which then poisoned two consumers in opposite directions — getOwnText reading the <br>'s empty textContent as "" (surfaced as text: null and thence "not editable" in the studio panel), and setOwnText writing text into the void element, corrupting it into </br> and duplicating lines under openComposition({ overrides }).serialize(). The symmetry of the two failure modes is what convinces me: the same misclassification of a void element as a text-target caused both symptoms.

On the instrument selected. The VOID_TEXT_TARGET_TAGS set enumerates all fourteen candidates — AREA / BASE / BR / COL / EMBED / HR / IMG / INPUT / LINK / META / PARAM / SOURCE / TRACK / WBR. HTML5 living-standard names thirteen (dropping the obsolete PARAM); the inclusion of PARAM here is a defensive courtesy to older documents that DOM implementations still parse as void, and I would keep it. Casing matches the standard uppercase-tagName return for HTML documents, which is what parseMutable produces in your test fixtures. I could contrive no void tag the set omits.

On the two turns of the screw. getOwnText now surfaces <br> as "\n" inside the childNodes walk, and setOwnText — for a flat text leaf (isTextOrBrLeaf: zero element children, or only <br>s) — rebuilds the run from the newline-split value. The rebuild reuses existing <br> nodes in DOM order (spareBrs.shift() ?? doc.createElement("br")) so their identity — including data-hf-id — survives an in-place edit that preserves the line count. Line added: a fresh <br>. Line removed: the trailing <br> (and its id) is gracefully dropped. The if (line) el.appendChild(doc.createTextNode(line)) guard correctly refuses to mint empty text nodes for consecutive newlines, producing a clean <br><br> run rather than "" <br> "" <br> "". getOwnText and setOwnText are now genuine inverses across the flat-leaf domain: set(get(el)) === el in serialization, and the fourth fixture at mutate.test.ts:305 proves the byte-exact undo through the RFC 6902 inverse patch — precisely the property one wants.

On the fixture bench. Four rows, each locking a distinct property of the fix: (1) the pre-edit read is "Centrifugal\nForce" (the inverse-patch value proves it, which is a cleverer proof than eyeballing getOwnText directly since it also exercises the mutate-op codepath); (2) after setOwnText("Centripetal\nForce"), both textual halves survive, a <br> remains, and it holds no text child (guarding against the </br> corruption); (3) a value with no newline drops the <br> cleanly; (4) an existing <br data-hf-id="hf-x5px"> survives an in-place edit with the same line count, and the RFC 6902 inverse restores byte-exact. The expect(serializeDocument(parsed)).toBe(before) at the tail is the load-bearing assertion — anything that mutated <br> identity would fail here.

Curiosities, non-blocking

On \r\n line endings. text.split("\n") will leave a stray \r on the previous line under a Windows-authored template with CRLF terminators. In practice HyperFrames composition documents look LF-normalised and I could find no CRLF fixture in the repo; a defensive .split(/\r?\n/) would cost nothing and close the aperture, but I would not gate merge on it.

On the pure-text-leaf case (no <br>s to begin with). isTextOrBrLeaf returns vacuously true for zero element children, so the rebuild branch now fires for any simple <h1>Hello</h1>. Two consequences are worth noting for the record:

1. Text-node identity is no longer preserved. The old firstTextIdx-restore path wrote into the pre-existing text node's nodeValue; the new path removes all children and creates a fresh one. Serialisation is identical, so the existing suite at mutate.test.ts:190–264 continues to pass by construction (nothing there asserts text-node identity). If any downstream tooling had cached a reference to the text node — a Range, a MutationObserver filter, an editor selection anchor — the reference would be invalidated on every setText, whereas it survived previously. I could find no such consumer in the SDK or Studio, so this reads as safe; noting it purely because the change of shape is invisible from the diff.
2. Raw \n-in-text is normalised to <br> on first edit. A template authored as <h1>Line1\nLine2</h1> (raw newline inside a single text node — HTML permits it) reads as "Line1\nLine2" under the new getOwnText (unchanged for that case), but the next setText will rebuild it as <h1>Line1<br>Line2</h1>. A one-way transform. In practice HeyGen's authoring tools emit <br> for hard breaks and I doubt this ever fires, but it means the pre-edit and post-edit serialisation differ before any user-facing text has changed. Again — not a blocker; simply a shape the next reader may want to know about.

On the extension beyond <br>. VOID_TEXT_TARGET_TAGS also excludes IMG, HR, and the others from the single-child text-target computation. The isTextOrBrLeaf predicate, by contrast, only accepts <br> as an acceptable "leaf child" — so <div><img></div> is correctly not a flat leaf, and its setOwnText falls through to the legacy firstTextIdx branch (no regression). The two guards do subtly different work — VOID_TEXT_TARGET_TAGS protects reads and writes on single-child cases in general, isTextOrBrLeaf gates the rebuild-from-newlines path specifically. That asymmetry is correct; I flag it only so a future edit that widens isTextOrBrLeaf to include, say, <img> (in a misguided attempt at symmetry) understands why it must not.

On CI

At the head reviewed, only the WIP gate has landed (SUCCESS). Producer, unit, typecheck, Windows and macOS matrices, Fallow audit, and the runtime-contract suite are still in flight. The diff is confined to two files inside packages/sdk/src/engine and I do not anticipate any cross-package churn; still, I would wait for the standard bench to fill in before final merge.

Verdict

Approve. A fracture correctly located, remedied at both the read and write ends of the same predicate, and locked by fixtures that would have caught precisely the regression it fixes. The curiosities above are non-blocking and offered as lantern-light for the next maintainer.

— Review by Via

[miguel-heygen]

Post-merge advisory review (this PR is already merged into main). Findings flagged for follow-up, not gating.

The core fix is correctly scoped: resolveSingleChildTextTarget no longer treats void elements as writable text targets, and the new fixtures cover readable newline text, non-corruption, and same-line-count identity reuse. Via already covered those paths in depth.

Should-be-follow-up-ticket — packages/sdk/src/engine/model.ts:373: undo loses <br> identity after a line-break removal. The rebuild saves existing <br> nodes only for the current call, then removes them all (:374-375). If setText changes A<br data-hf-id="hf-x">B to A B, the <br> is discarded. Applying the inverse text patch ("A\nB") later has no spare node and creates a fresh unannotated <br> at :378, so undo does not restore the original DOM or data-hf-id. I reproduced this at exact head d231fb7b1aefce3cca240a7ff38219224adb7de7: the focused SDK suite ran 99 tests, with the added remove-then-undo case as the sole failure (98 passed); expected <br data-hf-id="hf-x5px">, received <br>. This matters anywhere that ID anchors selection, edits, or animation. Please preserve enough structure in the inverse patch (or otherwise restore removed <br> attributes/identity) and add the line-count-decrease undo regression alongside the same-count test.

— Magi

Verdict: COMMENT (post-merge: With fixes)
Reasoning: The shipped corruption fix works, but the newly supported line-count-decrease path breaks the mutation engine’s exact undo/identity contract.

[jrusso1020]

Post-merge advisory review (this PR is already merged into main at d231fb7). Findings are for follow-up, not gating.

Concurring with Via's APPROVE — the direct-leaf fix is correct: the void-tag exclusion in resolveSingleChildTextTarget, the getOwnText/setOwnText inverse over the flat <br> leaf, and the id-preserving <br> reuse all hold, and the four fixtures lock them. One CI update on top of Via's pass (he reviewed before the bench filled in): the full matrix has since gone green at the merged SHA — CI, CodeQL, regression, preview-regression, Player perf, and Windows render all ✓ — so the "unit tests added" claim is CI-confirmed.

Additive follow-up — the same <br> bug still bites one nesting level down (should-be-follow-up).

The fix makes getOwnText/setOwnText <br>-aware only for the element's own direct children. When the text target is a single-child wrapper, both functions delegate to singleChild.textContent, which is not <br>-aware:

• getOwnText (model.ts:349): if (singleChild) return singleChild.textContent ?? ""; — textContent skips <br>, so a nested break is flattened.
• setOwnText (model.ts:362): singleChild.textContent = text; — replaces all of the wrapper's children with a single text node, destroying the <br> (and its data-hf-id) and injecting a literal \n that HTML collapses to whitespace.

Concrete repro (a wrapper resolved by resolveSingleChildTextTarget):

<button data-hf-id="btn"><span>Line1<br>Line2</span></button>

• getOwnText(button) → "Line1Line2" (break lost — editable but wrong; note this is not the ""/not-editable symptom, so the original report wouldn't have surfaced it).
• setOwnText(button, "Line1\nLine2") → <span>Line1\nLine2</span> — the <br> is gone, replaced by a raw newline; the visible line break disappears on render and the <br>'s id is lost.

This is the identical failure class the PR fixes for <h1>First<br>Second</h1>, just when the <br> lives inside the single-child text target rather than being a direct child. It's pre-existing (the singleChild.textContent path is unchanged by this PR), so not a regression — but the fix is incomplete for that structure. Reachability hinges on whether Studio ever addresses a wrapper element (vs. the innermost text leaf) that contains a <br>; resolveSingleChildTextTarget exists precisely because wrappers do get addressed, so it's worth confirming rather than assuming safe. Fix direction: route the single-child target through the same <br>-aware read/rebuild (recurse getOwnText/setOwnText into singleChild) instead of .textContent.

Minor: the new tests don't cover consecutive <br><br>, leading/trailing <br>, or the wrapper case above — a wrapper fixture is the one that would guard the follow-up.

Complementary to Magi's post-merge finding (review #4765952400), same <br>-identity theme from a different angle: Magi's is id-loss on a line-count decrease (a removed line's <br data-hf-id> isn't restored byte-exact on undo); mine is id-loss / break-loss by nesting depth (a <br> inside a single-child wrapper). Both point at the reuse/textContent path as the place to harden for a follow-up.

Verdict: COMMENT (post-merge) — Ready: the shipped fix is correct for its target case; the wrapper-nested <br> gap is a follow-up, not a revert.

— Jerrai