From cc055f318d59623741fffa96e816ed68a635c2ef Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 16 Jun 2026 12:30:54 -0700 Subject: [PATCH] fix(sdk): agree removeElement/getElement on duplicate bare ids (#1511) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(sdk): setStyle removes hyphenated properties (was kebab/camel key mismatch) Co-Authored-By: Claude Opus 4.8 (1M context) * fix(sdk): agree removeElement/getElement on duplicate bare ids A bare hf-id duplicated across a sub-composition element and a top-level element resolved to different instances: removeElement → resolveScoped → querySelector (document-order-first, the inner sub-comp dup) while getElement preferred the canonical match (scopedId === id, the top-level dup). So removeElement(bareId) removed the inner instance and getElement(bareId) still found the surviving top-level one — they disagreed. resolveScoped now resolves an ambiguous BARE id to the canonical (top-level) instance via isCanonicalScope (walks ancestors for isNewHostBoundary), falling back to document order when no canonical match exists — matching getElement. Fully-scoped paths (hf-host/hf-dup) and non-duplicated bare ids are unchanged. Surfaced by SDK shadow parity (op:delete expected removed, actual present). Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- packages/sdk/src/engine/model.ts | 40 +++++++++- packages/sdk/src/session.subcomp.test.ts | 84 ++++++++++++++++++++- packages/studio/src/utils/sdkShadow.test.ts | 11 ++- 3 files changed, 126 insertions(+), 9 deletions(-) diff --git a/packages/sdk/src/engine/model.ts b/packages/sdk/src/engine/model.ts index 38b46536d..2efae69eb 100644 --- a/packages/sdk/src/engine/model.ts +++ b/packages/sdk/src/engine/model.ts @@ -29,25 +29,57 @@ export function parseMutable(html: string): ParsedDocument { // ─── Element lookup ─────────────────────────────────────────────────────────── export function findById(document: Document, id: string): Element | null { - // CSS.escape is browser-only; hf-ids are restricted identifiers so simple quote-escaping is safe. - const escaped = id.replace(/\\/g, "\\\\").replace(/"/g, '\\"'); - return document.querySelector(`[data-hf-id="${escaped}"]`); + // Delegate to resolveScoped so patch replay (undo/redo, override-set apply) + // resolves an id the SAME way forward dispatch does: canonical-first for an + // ambiguous bare id, and scoped-path ("hf-host/hf-leaf") aware. Otherwise the + // two paths disagree on which duplicate a bare id targets and undo reverts the + // wrong element. (function declaration is hoisted.) + return resolveScoped(document, id); } function escapeHfId(id: string): string { return id.replace(/\\/g, "\\\\").replace(/"/g, '\\"'); } +/** + * True when an element lives at the top-level (canonical) scope — i.e. its + * scopedId equals its bare id because no ancestor opens a sub-composition + * boundary. This mirrors document.ts's scopedId construction (childPrefix only + * changes at isNewHostBoundary elements) without rebuilding the snapshot tree. + */ +function isCanonicalScope(el: Element): boolean { + for (let cur = el.parentElement; cur; cur = cur.parentElement) { + if (isNewHostBoundary(cur)) return false; + } + return true; +} + /** * Resolve a bare or scoped hf-id to its DOM element. * - * Bare id ("hf-x"): equivalent to findById — top-level document search. + * Bare id ("hf-x"): top-level document search. When the bare id is ambiguous + * (duplicated across a sub-composition and the top level), prefer the canonical + * (top-level) instance — the one whose scopedId equals the bare id — falling + * back to document order when no canonical match exists. This matches + * getElement()'s resolution rule so removeElement / getElement agree on which + * instance an ambiguous bare id targets. + * * Scoped id ("hf-HOST/hf-LEAF", any depth): each segment narrows the search * into the subtree of the previous match. This unambiguously addresses an * element inside a sub-composition even when bare ids collide. */ export function resolveScoped(document: Document, id: string): Element | null { const parts = id.split("/"); + + // Bare id: prefer the canonical (top-level) match when one exists, so + // resolution agrees with getElement (scopedId === id wins over document order). + if (parts.length === 1) { + const escaped = escapeHfId(id); + const matches = Array.from(document.querySelectorAll(`[data-hf-id="${escaped}"]`)); + if (matches.length === 0) return null; + return matches.find((el) => isCanonicalScope(el)) ?? matches[0] ?? null; + } + let context: Element | Document = document; for (const part of parts) { const escaped = escapeHfId(part); diff --git a/packages/sdk/src/session.subcomp.test.ts b/packages/sdk/src/session.subcomp.test.ts index 91a99e0dd..c52182de6 100644 --- a/packages/sdk/src/session.subcomp.test.ts +++ b/packages/sdk/src/session.subcomp.test.ts @@ -13,7 +13,7 @@ import { describe, it, expect } from "vitest"; import { parseHTML } from "linkedom"; import { ensureHfIds } from "@hyperframes/core/hf-ids"; -import { resolveScoped } from "./engine/model.js"; +import { resolveScoped, findById } from "./engine/model.js"; import { parseMutable } from "./engine/model.js"; import { buildRoots, flatElements } from "./document.js"; import { openComposition } from "./session.js"; @@ -49,6 +49,24 @@ describe("resolveScoped — flat id", () => { ); expect(resolveScoped(doc as unknown as Document, "hf-xxxx")).toBeNull(); }); + + // Regression: findById is the patch-replay/undo resolver. It must agree with + // resolveScoped (forward dispatch) on an ambiguous bare id — both pick the + // canonical (top-level) instance — or undo reverts the wrong duplicate. + it("findById resolves an ambiguous bare id to the canonical instance (== resolveScoped)", () => { + const doc = makeDoc( + inlinedHtml(` +
+

inside

+
+

outside

+ `), + ) as unknown as Document; + const viaFind = findById(doc, "hf-dup"); + const viaResolve = resolveScoped(doc, "hf-dup"); + expect(viaFind).toBe(viaResolve); + expect(viaFind?.getAttribute("class")).toBe("outside"); + }); }); describe("resolveScoped — scoped id", () => { @@ -366,6 +384,70 @@ describe("find({ composition })", () => { }); }); +// ─── 5b. Ambiguous bare id: removeElement / getElement agreement ────────────── + +describe("ambiguous bare id — removeElement and getElement agree", () => { + // Inner sub-comp dup appears FIRST in document order; the canonical top-level + // dup appears AFTER it. querySelector document-order would return the inner one, + // but getElement prefers the canonical (top-level) match. The two APIs must agree. + const ambiguousHtml = () => + inlinedHtml(` +
+
+

inner

+
+

outer

+
+ `); + + it("bare id resolves to the canonical (top-level) instance, matching getElement", async () => { + const comp = await openComposition(ambiguousHtml()); + + // getElement prefers the canonical match (scopedId === id) → top-level "outer". + const got = comp.getElement("hf-dup"); + expect(got?.scopedId).toBe("hf-dup"); + expect(got?.classNames).toContain("outer"); + + // removeElement(bareId) must remove the SAME instance getElement returned. + comp.removeElement("hf-dup"); + + // The canonical top-level instance is gone — getElement(bareId) no longer + // finds it (and does NOT silently fall through to the inner sub-comp dup). + expect(comp.getElement("hf-dup")).toBeNull(); + + // The inner instance survives, addressable only via its fully-scoped path. + const inner = comp.getElement("hf-host/hf-dup"); + expect(inner?.classNames).toContain("inner"); + }); + + it("fully-scoped path still targets the inner instance exactly", async () => { + const comp = await openComposition(ambiguousHtml()); + comp.removeElement("hf-host/hf-dup"); + + // Inner gone; canonical top-level survives. + const inner = comp.getElement("hf-host/hf-dup"); + expect(inner).toBeNull(); + const top = comp.getElement("hf-dup"); + expect(top?.scopedId).toBe("hf-dup"); + expect(top?.classNames).toContain("outer"); + }); + + it("non-duplicated bare id still resolves and removes normally", async () => { + const html = inlinedHtml(` +
+
+

inside

+
+

solo

+
+ `); + const comp = await openComposition(html); + expect(comp.getElement("hf-solo")?.scopedId).toBe("hf-solo"); + comp.removeElement("hf-solo"); + expect(comp.getElement("hf-solo")).toBeNull(); + }); +}); + // ─── 6. Scoped id stability across serialize ────────────────────────────────── describe("scopedId stability across serialize/re-parse", () => { diff --git a/packages/studio/src/utils/sdkShadow.test.ts b/packages/studio/src/utils/sdkShadow.test.ts index b1baa8497..462b8d2dc 100644 --- a/packages/studio/src/utils/sdkShadow.test.ts +++ b/packages/studio/src/utils/sdkShadow.test.ts @@ -312,12 +312,15 @@ describe("runShadowDelete", () => { `; - it("CORRECTLY reports present/removed for the SDK duplicate-bare-id delete divergence", async () => { + it("reports clean delete for a duplicate bare id (SDK resolves removeElement/getElement to the same instance)", async () => { const session = await openComposition(DUP_ID_HTML); runShadowDelete(session, "hf-dup"); - // removeElement dropped the inner instance; the top-level one survives, so the - // readback truthfully flags one mismatch (not silently passed). - expect(lastShadow()).toMatchObject({ op: "delete", dispatched: true, mismatchCount: 1 }); + // SDK fix (agree removeElement/getElement on duplicate bare ids): both now + // resolve a bare id to the canonical (top-level) instance, so removeElement + // drops exactly the element the readback checks → no mismatch. (Previously + // removeElement dropped the inner instance while the top-level survived, + // which this shadow correctly flagged; that divergence is now fixed.) + expect(lastShadow()).toMatchObject({ op: "delete", dispatched: true, mismatchCount: 0 }); }); });