feat(sdk): addElement forward op — mint hf-id, inverse = removeElement (WS-D)#1571
Conversation
miga-heygen
left a comment
There was a problem hiding this comment.
Review — addElement forward op (WS-D)
Clean, well-structured addition. The forward/inverse symmetry with removeElement is airtight, the id-minting strategy against the live document's id set is the right call, and the test coverage (16 tests including undo/redo round-trip, serialize survival, and collision rehash) is thorough. A few observations worth discussing:
1. Missing parent guard in handler (medium)
handleAddElement resolves the parent via resolveScoped() but doesn't guard against a null return — it casts straight to Element and proceeds. validateOp would catch this if the consumer calls comp.can() first, but the hot path (_dispatch → applyOp) skips validation entirely. If resolveScoped returns null for a stale parent id, parentEl.children throws a runtime error with no useful context.
// line ~688
: (resolveScoped(parsed.document, parent) as Element);Suggest: early-return EMPTY (or throw with a clear message) if parentEl is null, matching the defensive if (!node) return EMPTY guard two lines below.
2. Excluded-tag root element → empty newId (low-medium)
mintFragmentIds skips stamping any element whose tag is in HF_EXCLUDED_TAGS. If the root element of the fragment happens to be one of these (e.g. someone passes <style>...</style> or <template>...</template>), the function returns "" — an empty string flows through meta.newId, the elementPath("") produces a malformed patch path, and the session method returns "" as the id.
validateOp blocks <script> explicitly, but the other excluded tags (style, template, meta, link, noscript, base) pass validation and would hit this edge case. Worth either:
- Extending
validateOpto reject root elements with excluded tags, or - Documenting that only renderable elements are supported (the docstring says "single-root HTML fragment" — maybe tighten to "single-root renderable element").
3. Double as unknown cast for body resolution (nit)
const parentEl =
parent === null
? ((parsed.document as unknown as { body: Element }).body as unknown as Element)
: ...This double-cast suggests the linkedom Document type doesn't expose .body. If that's a known linkedom gap, a one-line helper (or a type assertion module) would keep the cast localized and self-documenting. Not blocking — just a readability note.
4. Session method swallows failures silently (nit)
addElement(...): HfId {
const result = this._dispatch({ type: "addElement", parent, index, html }, ORIGIN_LOCAL);
return result.meta?.newId ?? "";
}If _dispatch returns EMPTY (bad parent, no firstElementChild), the caller gets "" with no signal that the insert failed. The existing addGsapTween has the same pattern, so this is consistent with the codebase — but worth noting that SDK consumers need to check for empty-string returns. A // Returns "" if insert fails JSDoc note on the interface method would help.
Verdict
Solid work. The architecture decisions (minting against the live doc, split pure helpers, HF_EXCLUDED_TAGS mirroring core) are well-reasoned. Test coverage is comprehensive. The medium-priority items (1 and 2) are worth addressing before merge — #1 is a latent runtime error on the hot path, #2 is a quiet data corruption vector — but neither is a showstopper for this stack layer.
LGTM — ready for stamp (pending the parentEl guard).
Review by Miga
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Review — feat(sdk): addElement forward op — mint hf-id, inverse = removeElement (WS-D)
Reviewing at HEAD 7915aa9e54598c43a1f1960cb908f5d3d56c38c8. Stacked on #1570 (WS-C). Net +391/-1.
This is the cleanest of the four. The forward/inverse symmetry with handleRemoveElement is right — minted-against-live-doc avoids the obvious collision pitfall, the patchAdd→patchRemove pair round-trips through applyOne's existing element case (apply-patches.ts:174-194), and the 16 new tests cover the actual landmines (collision rehash, nested-fragment id minting, undo→redo with id stability).
Concerns
1. HF_EXCLUDED_TAGS is duplicated from core/parsers/hfIds.ts:14. (packages/sdk/src/engine/mutate.ts:633-641)
// mutate.ts:633-641
const HF_EXCLUDED_TAGS = new Set([
"script", "style", "template", "meta", "link", "noscript", "base",
]);vs the canonical:
// core/parsers/hfIds.ts:14
const EXCLUDED_TAGS = new Set(["script", "style", "template", "meta", "link", "noscript", "base"]);The comment at mutate.ts:632 ("mirrors hfIds.ts EXCLUDED_TAGS") acknowledges this. The two are identical today, but ensureHfIds and mintHfId live in hfIds.ts and any future tag added there (e.g. iframe, slot) won't propagate to mutate.ts — silent drift, and the wrong side wins because mintFragmentIds is the gate for newly-minted content. Export EXCLUDED_TAGS from @hyperframes/core/hf-ids (the subpath you're already importing for mintHfId) and import it here.
2. validateOp doesn't reject HTML with multiple element roots. (packages/sdk/src/engine/mutate.ts:1424-1434)
The validator checks tmp.firstElementChild === null (zero elements rejected) but accepts <div>a</div><div>b</div> (two roots — only the first is inserted by handleAddElement because node = tmp.firstElementChild). The second silently dropped, no error, no meta. The PR body says "single-root HTML fragment" but the contract isn't enforced. Either reject tmp.children.length > 1 with E_INVALID_HTML, or extend the handler to wrap multi-root fragments in a sentinel parent. First is simpler and matches the type-definition comment at types.ts:99 ("Single-root HTML fragment").
Nits
handleAddElementreturnsEMPTY(line 678) whennode === null, butvalidateOpalready guarantees a non-nullfirstElementChild(line 1428). The defensive return is dead code if callers go through the SDK; leaving it for raw-dispatch safety is reasonable, just worth a one-line comment notingvalidateOpis the gate.addElementonCompositionreturns""whenmeta?.newIdis absent (session.ts:142-145).addGsapTweendoes the same pattern (returns""). Consider documenting that an empty-string return means "not minted, op rejected" — currently the JSDoc attypes.ts:357-360only describes the success case.
Verified
- Inverse symmetry:
handleAddElementemitspatchAdd → patchRemove(line 692-696).handleRemoveElementon main emitspatchRemove → patchAdd({html, parentId, siblingIndex})(mutate.ts:593-594). Round-trip undo→redo of an addElement replays through the sameapply-patches.tselementcase as remove's inverse — the test"add → undo → redo: element returns with the same id"(mutate.test.ts:469-486) confirms id stability. mintHfIdcollision safety:mintFragmentIdscollects all live doc ids intoassignedBEFORE minting (mutate.ts:644-651), and themintHfIdimpl's dup-rehash loop (hfIds.ts:80-96) covers the content-identical case. Test"content-collision with existing element yields a distinct rehashed id"(mutate.test.ts:393-412) verifies thehf-logoclash path.- Determinism:
mintHfIdis content-keyed FNV1a; same input → same id. The replay test (mutate.test.ts:469-486) confirms id stability across undo/redo. NoDate.now/Math.randominvolved. <script>rejection invalidateOp(mutate.ts:1430-1434): blocks the obvious XSS / GSAP-collision vector.parent: null → document.body(mutate.ts:670-672) and the validate side acceptsparent: null(mutate.ts:1416) — test"parent: null inserts at document body root level"(mutate.test.ts:489-505) confirms.meta.newIdsurfaced throughMutationResult.meta(mutate.ts:79) andsession.addElementreturns it (session.ts:142-145).- Recent commits to
mutate.ts(gh api commits?path=packages/sdk/src/engine/mutate.ts&since=2026-06-12) — no Graphite-restack silent-revert risk; all 5 most-recent commits are additive feature/fix landings.
What I didn't verify
- Whether
data-hf-idon a newly-inserted element survives Studio's serialize → persist → reload round-trip in the full editor flow. The SDK round-trip viaserializeDocument(mutate.test.ts:513-525) does survive, which is what the SDK contract owns. - GSAP interaction with newly-added elements — the PR body's "Deferred" section notes "inserting managed GSAP for new elements" is out of scope. Inserting GSAP via
<script>is blocked at validate, which is the right defense for this op.
Cleanest of the four; address (1) (cheap export) and (2) (cheap validation) before stamp.
Review by Rames D Jusso
902966f to
489b76b
Compare
C1: getElementTimings/setElementTiming typed session methods + setHold typed wrapper. getElementTimings reads data-duration (preferred) or data-end−data-start (fallback) — same attr-preference as handleSetTiming. setElementTiming dispatches a sparse map as one batch → one patch event → one undo step. setHold mirrors setVariableValue pattern. Also fixes a pre-existing apply-patches.ts gap: the timing/duration patch case was absent, causing undo of duration changes to silently no-op. Added the duration branch so inverse patches restore data-duration correctly. C2: packages/core/src/compiler/timingResolver.ts — shared pure resolveTimings() consumed by BOTH preview (sdk session) and render (timingCompiler) paths. Word- anchored elements get enterAt = wordTimings[k].start + offset; elastic hold = max(0, slotEnd − (enterAt + enterDuration + exitDuration)), clamped ≥ 0; never timescales animated content. Un-anchored elements keep authored timing (align-on- adjust). Deterministic + pure: no Date.now, no Math.random, no DOM. extractGsapLabels() added to gsapParserAcorn.ts to parse tl.addLabel() calls for the getElementTimings labels field. Tests: timingResolver.test.ts (10 pure-function tests including preview==render parity golden test); session.timings.test.ts (15 session-layer tests covering duration-authored, end-authored, label extraction, batching, undo, and setHold regression). Gates: build ✓ · bun test (sdk+core/compiler) 434/434 ✓ · oxlint 0 warnings ✓ · oxfmt --check ✓ · fallow --gate new-only ✓ (complexity suppressed on 2 new inline functions, duplication warn-only pre-existing) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
489b76b to
46e6277
Compare
…t (WS-D) Implements WS-D: the addElement EditOp and session.addElement() typed method. - types.ts: addElement op (parent/index/html) added to EditOp union; addElement(parent, index, html): HfId added to Composition interface - mutate.ts: handleAddElement inserts a single-root HTML fragment at parent+index, minting ids against the LIVE document's existing id set (not a fresh fragment set) via collectDocumentHfIds + mintFragmentIds; forward = patchAdd, inverse = patchRemove; MutationResult.meta.newId carries the minted root id - mutate.ts: validateOp case rejects missing parent, negative index, empty html, zero-element html, and <script> in html - session.ts: typed addElement(parent, index, html) returns minted id via result.meta.newId - mutate.test.ts: 16 tests covering insert position, append semantics, id uniqueness, content-collision rehash, nested fragments, forward/ inverse symmetry, undo, add/undo/redo stability, parent:null body insertion, serialize round-trip, and all five validateOp rejection codes Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
7915aa9 to
8727308
Compare
The base branch was changed.

WS-D — addElement forward op
Part of the AI Studio (Pacific) SDK integration. Stacked on #1570 (WS-C).
Problem
Only
removeElement(and an inverse-only add) existed. An embedded editor that adds elements needs a forward op.What this does
Adds a forward
addElement(parent, index, html): HfIdop and typed session method:hf-idagainst the live document; ids stay stable acrossserialize().removeElement, so undo/redo round-trips.Implementation notes
handleAddElementis split into small pure helpers —collectDocumentHfIds,mintFragmentIds— plus a module-levelHF_EXCLUDED_TAGS, to keep cyclomatic complexity under the fallow HIGH threshold.validateOprejects with 5 specific codes;MutationResult.meta.newIdsurfaces the minted id;mintHfIdis imported from@hyperframes/core/hf-ids.Files (4 changed)
packages/sdk/src/types.ts—addElementinEditOpunion +Compositioninterfacepackages/sdk/src/engine/mutate.ts— handler, helpers,applyOp+validateOpwiring,meta.newIdpackages/sdk/src/session.ts— typed methodpackages/sdk/src/engine/mutate.test.ts— 16 new testsGates
bun run build✅ ·bun test mutate.test.ts70/0 (+16) ✅bunx oxlint0/0 ✅ ·bunx oxfmt --check✅ ·fallow --gate new-only✅Deferred (per plan)
Structured/typed element specs, auto-positioning, inserting managed GSAP for new elements.
🤖 Generated with Claude Code