Skip to content

Commit 0923bc0

Browse files
vanceingallsclaude
andauthored
feat(studio): carry hfId on TimelineElement, wire through buildPatchTarget (R7, T5b) (#1299)
* feat(studio): carry hfId on TimelineElement, wire through buildPatchTarget (R7, T5b) * refactor(studio): extract readHfId helper, fix empty-string normalization, add comments (R7 review) - Extract readHfId(el) to domEditingLayers.ts — centralizes `?.trim() || undefined` normalisation; guards against empty-string data-hf-id reaching findTagByTarget - Wire readHfId into domEditingLayers.ts and useDomEditCommits.ts (the one site that still used `?? undefined` instead of `|| undefined`) - Re-export readHfId through domEditing.ts public API - Add readHfId unit tests: present, absent, empty-string, whitespace-only - Add comment on PatchTarget: runtime validation lives in findTagByTarget, type is docs-only - Suppress pre-existing unused re-exports in timelineDOM.ts (backward-compat re-exports brought into fallow scope by the T5b hfId changes) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(core): clear data-hf-id on split clone to prevent dual-match (R7 review) cloneNode(true) copies all attributes including data-hf-id. Without clearing it, both halves of a split share the same hf-id; the server's findByHfId picks the first match and silently patches the wrong clip. Remove the attribute from the clone so write-back re-mints a fresh id on the next preview load. Adds a test: splitElementInHtml — hfId clone isolation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(studio): add hfId to DomEditLayerItem + getDomLayerPatchTarget return type (R7 review) - Add hfId to DomEditLayerItem interface (domEditingTypes.ts) so layer item construction in collectDomEditLayerItems compiles - Widen getDomLayerPatchTarget return type to include hfId + populate it from data-hf-id attribute (domEditingElement.ts) - Widen findDomEditSelectionTarget to check hfId-first when no id/selector - Widen Pick types in domEditOverlayGeometry.ts and useGsapScriptCommits.ts - Add hfId to buildMissingCompositionElements element construction - Add hfId-targeted test coverage in domEditing.test.ts, domEditOverlayGeometry.test.ts, timelineIframeHelpers.test.ts - Update hfIds.test.ts KNOWN LIMITATION labels — write-back landed in R7 T1-2 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 82c754f commit 0923bc0

20 files changed

Lines changed: 282 additions & 33 deletions

packages/core/src/parsers/hfIds.test.ts

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -100,35 +100,34 @@ describe("ensureHfIds", () => {
100100
});
101101

102102
// Lock the edit-lifecycle behavior. These pin BOTH the guarantee that holds
103-
// once ids are persisted to source (pinning) AND the two limitations that hold
104-
// while they are not (design §3 write-back is not yet wired — see
105-
// notes/r1-stable-hf-ids-design.md "Implementation status & verified lifecycle gap").
103+
// once ids are persisted to source (pinning) AND the behavior for truly unpinned
104+
// HTML (no data-hf-id in the input — unreachable in production after write-back
105+
// landed in R7 Task 1-2, but still the correct contract for that path).
106106
describe("ensureHfIds — edit lifecycle (R1 stability)", () => {
107107
it("pinned id survives a content edit (the §3 write-back guarantee)", () => {
108108
// Element already carries data-hf-id in source (as it would after write-back).
109109
const edited = doc(`<p class="body" data-hf-id="hf-abcd">Hello world</p>`);
110110
expect(idOf(ensureHfIds(edited), "p.body")).toBe("hf-abcd");
111111
});
112112

113-
it("KNOWN LIMITATION: an unpinned id changes when the element's text is edited", () => {
114-
// No data-hf-id in source → every parse re-mints from content. Editing the
115-
// text changes the hash, so the id drifts. This is the "pure-hash" mode the
116-
// design rejected; flip this assertion to .toBe once write-back lands.
113+
it("unpinned id drifts when element text is edited (pure-hash, unreachable after write-back)", () => {
114+
// No data-hf-id in source → every parse re-mints from content. This path is
115+
// unreachable in production after R7 write-back: the first serve pins the id.
117116
const before = idOf(ensureHfIds(doc(`<p class="body">Hello</p>`)), "p.body");
118117
const after = idOf(ensureHfIds(doc(`<p class="body">Hello world</p>`)), "p.body");
119118
expect(before).not.toBe(after);
120119
});
121120

122-
it("KNOWN LIMITATION: an unpinned id changes when an attribute is edited", () => {
121+
it("unpinned id drifts when attribute is edited (pure-hash, unreachable after write-back)", () => {
123122
const before = idOf(ensureHfIds(doc(`<p class="body">x</p>`)), "p");
124123
const after = idOf(ensureHfIds(doc(`<p class="lead">x</p>`)), "p");
125124
expect(before).not.toBe(after);
126125
});
127126

128-
it("KNOWN LIMITATION: identical-content siblings have no content-stable id for the 2nd occurrence", () => {
127+
it("identical-content siblings: second occurrence gets a position-derived dedup id", () => {
129128
// Insertion stability holds for DISTINCT content (covered elsewhere), but a
130-
// second identical sibling collides and gets a position-derived dedup id
131-
// there is no content-stable handle for it. The first keeps the base id.
129+
// second identical sibling collides and gets a position-derived dedup id.
130+
// First element keeps the base (content-derived) id; documented in project_hfid_dedup_tiebreak.
132131
const single = idOf(ensureHfIds(doc(`<p class="x">same</p>`)), "p.x");
133132
const pair = ids(ensureHfIds(doc(`<p class="x">same</p><p class="x">same</p>`)));
134133
expect(pair[0]).toBe(single); // first identical element: stable, content-derived

packages/core/src/studio-api/helpers/sourceMutation.test.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import {
33
removeElementFromHtml,
44
patchElementInHtml,
55
probeElementInSource,
6+
splitElementInHtml,
67
} from "./sourceMutation.js";
78

89
describe("removeElementFromHtml", () => {
@@ -455,3 +456,14 @@ describe("T7 — data-hf-id targeting (spec for R1)", () => {
455456
expect(html).toContain('data-hf-id="hf-a1b2"');
456457
});
457458
});
459+
460+
describe("splitElementInHtml — hfId clone isolation", () => {
461+
it("does not copy data-hf-id to the cloned second half", () => {
462+
const source = `<html><body><div data-composition-id="root"><div id="clip1" class="clip" data-start="0" data-duration="10" data-hf-id="hf-abc123"></div></div></body></html>`;
463+
const { html, matched } = splitElementInHtml(source, { id: "clip1" }, 5, "clip2");
464+
465+
expect(matched).toBe(true);
466+
const occurrences = (html.match(/data-hf-id="hf-abc123"/g) ?? []).length;
467+
expect(occurrences).toBe(1);
468+
});
469+
});

packages/core/src/studio-api/helpers/sourceMutation.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,7 @@ export function splitElementInHtml(
275275

276276
const clone = el.cloneNode(true) as HTMLElement;
277277
clone.setAttribute("id", newId);
278+
clone.removeAttribute("data-hf-id");
278279
clone.setAttribute("data-start", String(Math.round(splitTime * 1000) / 1000));
279280
clone.setAttribute("data-duration", String(Math.round(secondDuration * 1000) / 1000));
280281

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
// @vitest-environment jsdom
2+
import { describe, expect, it } from "vitest";
3+
import { selectionCacheKey } from "./domEditOverlayGeometry";
4+
5+
describe("selectionCacheKey — hfId collision (R7)", () => {
6+
it("produces distinct keys for two elements that differ only by hfId", () => {
7+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
8+
const a = selectionCacheKey({ sourceFile: "index.html", hfId: "hf-111" } as any);
9+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
10+
const b = selectionCacheKey({ sourceFile: "index.html", hfId: "hf-222" } as any);
11+
expect(a).not.toBe(b);
12+
});
13+
});

packages/studio/src/components/editor/domEditOverlayGeometry.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,10 +190,11 @@ export function filterNestedDomEditGroupItems<T extends { element: HTMLElement }
190190
}
191191

192192
export function selectionCacheKey(
193-
selection: Pick<DomEditSelection, "id" | "selector" | "selectorIndex" | "sourceFile">,
193+
selection: Pick<DomEditSelection, "id" | "hfId" | "selector" | "selectorIndex" | "sourceFile">,
194194
): string {
195195
return [
196196
selection.sourceFile ?? "",
197+
selection.hfId ?? "",
197198
selection.id ?? "",
198199
selection.selector ?? "",
199200
selection.selectorIndex ?? "",

packages/studio/src/components/editor/domEditing.test.ts

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1156,3 +1156,46 @@ describe("patch builders and prompt builder", () => {
11561156
).not.toThrow();
11571157
});
11581158
});
1159+
1160+
describe("hfId — find, key, capabilities (R7 fixes)", () => {
1161+
it("getDomEditTargetKey keeps two hfId-only elements distinct", () => {
1162+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
1163+
const a = getDomEditTargetKey({ sourceFile: "index.html", hfId: "hf-aaa" } as any);
1164+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
1165+
const b = getDomEditTargetKey({ sourceFile: "index.html", hfId: "hf-bbb" } as any);
1166+
expect(a).not.toBe(b);
1167+
});
1168+
1169+
it("findElementForSelection finds element by data-hf-id when no id or selector", () => {
1170+
const doc = createDocument(`
1171+
<div data-composition-id="root">
1172+
<div data-hf-id="hf-xyz789" class="clip" style="position:absolute;left:0;top:0;width:100px;height:100px;"></div>
1173+
</div>
1174+
`);
1175+
const el = doc.querySelector('[data-hf-id="hf-xyz789"]') as HTMLElement;
1176+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
1177+
const found = findElementForSelection(doc, { hfId: "hf-xyz789" } as any);
1178+
expect(found).toBe(el);
1179+
});
1180+
1181+
it("resolveDomEditCapabilities enables editing for hfId-only element (no CSS selector)", () => {
1182+
const result = resolveDomEditCapabilities({
1183+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
1184+
hfId: "hf-abc" as any,
1185+
selector: undefined,
1186+
inlineStyles: { left: "10px", top: "20px", width: "100px", height: "50px" },
1187+
computedStyles: {
1188+
position: "absolute",
1189+
left: "10px",
1190+
top: "20px",
1191+
width: "100px",
1192+
height: "50px",
1193+
},
1194+
isCompositionHost: false,
1195+
isInsideLockedComposition: false,
1196+
isMasterView: false,
1197+
});
1198+
expect(result.canSelect).toBe(true);
1199+
expect(result.canMove).toBe(true);
1200+
});
1201+
});

packages/studio/src/components/editor/domEditing.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ export {
3535
getDomEditNonEditableReason,
3636
getDomEditTargetKey,
3737
isTextEditableSelection,
38+
readHfId,
3839
refreshDomEditSelection,
3940
resolveDomEditCapabilities,
4041
resolveDomEditSelection,

packages/studio/src/components/editor/domEditingElement.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ function isInspectableLayerElement(el: HTMLElement): boolean {
9595
export function getDomLayerPatchTarget(
9696
el: HTMLElement,
9797
activeCompositionPath: string | null,
98-
): Pick<DomEditSelection, "id" | "selector" | "selectorIndex" | "sourceFile"> | null {
98+
): Pick<DomEditSelection, "id" | "hfId" | "selector" | "selectorIndex" | "sourceFile"> | null {
9999
if (!isInspectableLayerElement(el)) return null;
100100
if (el.hasAttribute("data-composition-id")) return null;
101101

@@ -105,6 +105,7 @@ export function getDomLayerPatchTarget(
105105
const { sourceFile } = getSourceFileForElement(el, activeCompositionPath);
106106
return {
107107
id: el.id || undefined,
108+
hfId: el.getAttribute("data-hf-id") || undefined,
108109
selector,
109110
selectorIndex: getSelectorIndex(
110111
el.ownerDocument,
@@ -229,9 +230,14 @@ export function isLargeRasterDomEditSelection(
229230

230231
export function findElementForSelection(
231232
doc: Document,
232-
selection: Pick<DomEditSelection, "id" | "selector" | "selectorIndex" | "sourceFile">,
233+
selection: Pick<DomEditSelection, "id" | "hfId" | "selector" | "selectorIndex" | "sourceFile">,
233234
activeCompositionPath: string | null = null,
234235
): HTMLElement | null {
236+
if (selection.hfId) {
237+
const byHfId = doc.querySelector(`[data-hf-id="${selection.hfId}"]`);
238+
if (isHtmlElement(byHfId)) return byHfId;
239+
}
240+
235241
if (selection.id) {
236242
const byId = doc.getElementById(selection.id);
237243
if (

packages/studio/src/components/editor/domEditingLayers.test.ts

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// @vitest-environment jsdom
22
import { describe, expect, it } from "vitest";
3-
import { resolveDomEditSelection, buildDomEditPatchTarget } from "./domEditingLayers";
3+
import { resolveDomEditSelection, buildDomEditPatchTarget, readHfId } from "./domEditingLayers";
44

55
const opts = { activeCompositionPath: "index.html", isMasterView: true, skipSourceProbe: true };
66

@@ -27,6 +27,31 @@ describe("buildDomEditPatchTarget", () => {
2727
});
2828
});
2929

30+
describe("readHfId", () => {
31+
it("returns the attribute value when present", () => {
32+
const el = document.createElement("div");
33+
el.setAttribute("data-hf-id", "hf-abc");
34+
expect(readHfId(el)).toBe("hf-abc");
35+
});
36+
37+
it("returns undefined when attribute is absent", () => {
38+
const el = document.createElement("div");
39+
expect(readHfId(el)).toBeUndefined();
40+
});
41+
42+
it("returns undefined when attribute is empty string", () => {
43+
const el = document.createElement("div");
44+
el.setAttribute("data-hf-id", "");
45+
expect(readHfId(el)).toBeUndefined();
46+
});
47+
48+
it("returns undefined when attribute is whitespace-only", () => {
49+
const el = document.createElement("div");
50+
el.setAttribute("data-hf-id", " ");
51+
expect(readHfId(el)).toBeUndefined();
52+
});
53+
});
54+
3055
describe("resolveDomEditSelection — hfId from data-hf-id", () => {
3156
it("populates hfId from the element data-hf-id attribute", async () => {
3257
const el = document.createElement("div");

packages/studio/src/components/editor/domEditingLayers.ts

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,7 @@ export function buildDefaultDomEditTextField(base?: Partial<DomEditTextField>):
173173
// fallow-ignore-next-line complexity
174174
export function resolveDomEditCapabilities(args: {
175175
selector?: string;
176+
hfId?: string;
176177
tagName?: string;
177178
className?: string;
178179
inlineStyles: Record<string, string>;
@@ -182,7 +183,7 @@ export function resolveDomEditCapabilities(args: {
182183
isMasterView: boolean;
183184
existsInSource?: boolean;
184185
}): DomEditCapabilities {
185-
if (!args.selector || args.isInsideLockedComposition) {
186+
if ((!args.selector && !args.hfId) || args.isInsideLockedComposition) {
186187
return {
187188
canSelect: !args.isInsideLockedComposition,
188189
canEditStyles: false,
@@ -289,7 +290,7 @@ export function buildElementLabel(el: HTMLElement): string {
289290
async function probeSourceElement(
290291
projectId: string,
291292
sourceFile: string,
292-
target: { id?: string; selector?: string; selectorIndex?: number },
293+
target: { id?: string; hfId?: string; selector?: string; selectorIndex?: number },
293294
): Promise<boolean> {
294295
try {
295296
const response = await fetch(
@@ -321,7 +322,8 @@ export async function resolveDomEditSelection(
321322
let current: HTMLElement | null = getSelectionCandidate(startEl, options);
322323
while (current && current !== doc.body && current !== doc.documentElement) {
323324
const selector = buildStableSelector(current);
324-
if (!selector) {
325+
const hfId = readHfId(current);
326+
if (!selector && !hfId) {
325327
current = current.parentElement;
326328
continue;
327329
}
@@ -330,13 +332,9 @@ export async function resolveDomEditSelection(
330332
current,
331333
options.activeCompositionPath,
332334
);
333-
const selectorIndex = getSelectorIndex(
334-
doc,
335-
current,
336-
selector,
337-
sourceFile,
338-
options.activeCompositionPath,
339-
);
335+
const selectorIndex = selector
336+
? getSelectorIndex(doc, current, selector, sourceFile, options.activeCompositionPath)
337+
: undefined;
340338
const compositionSrc =
341339
current.getAttribute("data-composition-src") ??
342340
current.getAttribute("data-composition-file") ??
@@ -346,15 +344,18 @@ export async function resolveDomEditSelection(
346344
const textFields = collectDomEditTextFields(current);
347345
const isInsideLocked = Boolean(findClosestByAttribute(current, ["data-timeline-locked"]));
348346
let existsInSource: boolean | undefined;
349-
if (!options.skipSourceProbe && options.projectId && (current.id || selector)) {
350-
const probeTarget: { id?: string; selector?: string; selectorIndex?: number } = {};
347+
if (!options.skipSourceProbe && options.projectId && (current.id || selector || hfId)) {
348+
const probeTarget: { id?: string; hfId?: string; selector?: string; selectorIndex?: number } =
349+
{};
351350
if (current.id) probeTarget.id = current.id;
351+
if (hfId) probeTarget.hfId = hfId;
352352
if (selector) probeTarget.selector = selector;
353353
if (selectorIndex != null) probeTarget.selectorIndex = selectorIndex;
354354
existsInSource = await probeSourceElement(options.projectId, sourceFile, probeTarget);
355355
}
356356
const capabilities = resolveDomEditCapabilities({
357357
selector,
358+
hfId,
358359
tagName: current.tagName.toLowerCase(),
359360
className: current.className,
360361
inlineStyles,
@@ -369,7 +370,7 @@ export async function resolveDomEditSelection(
369370
return {
370371
element: current,
371372
id: current.id || undefined,
372-
hfId: current.getAttribute("data-hf-id") ?? undefined,
373+
hfId,
373374
selector,
374375
selectorIndex,
375376
sourceFile,
@@ -452,6 +453,7 @@ export function collectDomEditLayerItems(
452453
if (!root) return [];
453454

454455
const items: DomEditLayerItem[] = [];
456+
// fallow-ignore-next-line complexity
455457
const visit = (el: HTMLElement, depth: number) => {
456458
if (items.length >= maxItems) return;
457459

@@ -465,6 +467,7 @@ export function collectDomEditLayerItems(
465467
depth,
466468
childCount: getDirectLayerChildren(el, options).length,
467469
id: target.id ?? undefined,
470+
hfId: target.hfId ?? undefined,
468471
selector: target.selector ?? undefined,
469472
selectorIndex: target.selectorIndex,
470473
sourceFile: target.sourceFile,
@@ -536,10 +539,11 @@ export function getDomEditNonEditableReason(
536539
}
537540

538541
export function getDomEditTargetKey(
539-
selection: Pick<DomEditSelection, "id" | "selector" | "selectorIndex" | "sourceFile">,
542+
selection: Pick<DomEditSelection, "id" | "hfId" | "selector" | "selectorIndex" | "sourceFile">,
540543
): string {
541544
return [
542545
selection.sourceFile || "index.html",
546+
selection.hfId ?? "",
543547
selection.id ?? "",
544548
selection.selector ?? "",
545549
selection.selectorIndex ?? "",
@@ -556,6 +560,10 @@ export function isTextEditableSelection(selection: DomEditSelection): boolean {
556560

557561
// buildElementAgentPrompt is in domEditingAgentPrompt.ts
558562

563+
export function readHfId(element: Element): string | undefined {
564+
return element.getAttribute("data-hf-id")?.trim() || undefined;
565+
}
566+
559567
export function buildDomEditPatchTarget(
560568
selection: Pick<DomEditSelection, "id" | "hfId" | "selector" | "selectorIndex">,
561569
): { id?: string | null; hfId?: string; selector?: string; selectorIndex?: number } {

0 commit comments

Comments
 (0)