From 42c247d28fc1d40feb21f739a28631008bb21280 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 9 Jun 2026 00:27:28 -0700 Subject: [PATCH] feat(studio): sourcePatcher data-hf-id targeting (R1, T3) (#1271) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(core): clip-model hf- ids minted at parse, emitted as data-hf-id (R1) * docs(core): document legacy-id round-trip in clip-model readback (R1 review) Addresses Rames' review on #1270: clarifies that a pre-R1 clip authored with id="my-title" round-trips as data-hf-id="my-title" (non-hf-shaped but stable, exact-match) by design — targeting uses exact [data-hf-id="…"] match and does not require the hf- shape; legacy values re-mint only at the R7 write-back. Not a bug. Comment-only. Co-Authored-By: Claude Opus 4.8 (1M context) * docs(core): fix misleading legacy-id migration comment in htmlParser.ts The original comment said legacy data-hf-id values "are re-minted only once the R7 write-back persists freshly-minted ids to source" — which is incorrect. ensureHfIds skips elements that already carry data-hf-id, so legacy values (e.g. data-hf-id="my-title") persist indefinitely and are NOT automatically re-minted. Exact-match targeting still works correctly. Update comment to reflect actual behaviour. Co-Authored-By: Claude Sonnet 4.6 * feat(studio): sourcePatcher data-hf-id targeting (R1, T3) * fix(studio): warn on duplicate match in execDataAttrPattern (R1, T3 review) Addresses Rames' review on #1271: execDataAttrPattern returned the first regex match without checking for a second. A duplicate id/data-hf-id in source (id drift) would silently patch one element and leave the other stale. Now warns when more than one element matches. By the mint contract it should never fire. Co-Authored-By: Claude Opus 4.8 (1M context) * test(studio): pin hfId-is-authoritative-over-selector contract (R1, T3 review) Adds test: "hfId match is authoritative — selector is not used as a narrowing filter". When hfId matches element A and selector points at element B, findTagByTarget returns A without consulting selector as a narrowing filter. Pins the intended behaviour so a future refactor cannot silently start narrowing by selector. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .../studio/src/utils/sourcePatcher.test.ts | 87 ++++++++++++++++-- packages/studio/src/utils/sourcePatcher.ts | 89 ++++++++++--------- 2 files changed, 127 insertions(+), 49 deletions(-) diff --git a/packages/studio/src/utils/sourcePatcher.test.ts b/packages/studio/src/utils/sourcePatcher.test.ts index 7697d24e6..ee4ab514c 100644 --- a/packages/studio/src/utils/sourcePatcher.test.ts +++ b/packages/studio/src/utils/sourcePatcher.test.ts @@ -517,17 +517,88 @@ describe("motion attribute round-trip via sourcePatcher", () => { }); }); -// T3 — id-based targeting (spec for R1). -// R1 adds `hfId?: string` to PatchTarget and a `[data-hf-id="…"]` lookup branch -// in findTagByTarget. Convert from it.todo to real assertions in the R1 PR. +// T3 — id-based targeting (R1). describe("T3 — hfId targeting (spec for R1)", () => { - it.todo("updates inline style by data-hf-id"); + it("updates inline style by data-hf-id", () => { + const html = `

Hello

`; + const result = applyPatchByTarget( + html, + { hfId: "hf-x7k2" }, + { + type: "inline-style", + property: "color", + value: "blue", + }, + ); + expect(result).toContain("color: blue"); + expect(result).toContain('data-hf-id="hf-x7k2"'); + }); - it.todo("updates text content by data-hf-id"); + it("updates text content by data-hf-id", () => { + const html = `

Old text

`; + const result = applyPatchByTarget( + html, + { hfId: "hf-a1b2" }, + { + type: "text-content", + property: "", + value: "New text", + }, + ); + expect(result).toContain(">New text<"); + }); - it.todo("updates attribute by data-hf-id"); + it("updates attribute by data-hf-id", () => { + const html = `
`; + const result = applyPatchByTarget( + html, + { hfId: "hf-c3d4" }, + { + type: "attribute", + property: "start", + value: "2.5", + }, + ); + expect(result).toContain('data-start="2.5"'); + }); - it.todo("data-hf-id attribute is preserved after a style patch"); + it("data-hf-id attribute is preserved after a style patch", () => { + const html = `

Hello

`; + const patched = applyPatchByTarget( + html, + { hfId: "hf-x7k2" }, + { + type: "inline-style", + property: "color", + value: "blue", + }, + ); + expect(readAttributeByTarget(patched, { hfId: "hf-x7k2" }, "data-hf-id")).toBe("hf-x7k2"); + }); - it.todo("hfId lookup falls through to selector when hfId not found"); + it("hfId lookup falls through to selector when hfId not found", () => { + const html = `

Hello

`; + const result = applyPatchByTarget( + html, + { hfId: "hf-missing", selector: ".headline" }, + { type: "inline-style", property: "color", value: "blue" }, + ); + expect(result).toContain("color: blue"); + }); + + it("hfId match is authoritative — selector is not used as a narrowing filter", () => { + // hfId matches h1; selector points at h2. hfId wins — patch lands on h1, h2 untouched. + const html = `

A

B

`; + const result = applyPatchByTarget( + html, + { hfId: "hf-x7k2", selector: ".b" }, + { type: "inline-style", property: "color", value: "blue" }, + ); + expect(result).toContain('data-hf-id="hf-x7k2"'); + const h1End = result.indexOf(""); + const bluePos = result.indexOf("color: blue"); + expect(bluePos).toBeGreaterThan(-1); + expect(bluePos).toBeLessThan(h1End); + expect(result).toContain('

B

'); + }); }); diff --git a/packages/studio/src/utils/sourcePatcher.ts b/packages/studio/src/utils/sourcePatcher.ts index 2d89eea2e..9d19114d4 100644 --- a/packages/studio/src/utils/sourcePatcher.ts +++ b/packages/studio/src/utils/sourcePatcher.ts @@ -94,6 +94,7 @@ export interface PatchOperation { export interface PatchTarget { id?: string | null; + hfId?: string; selector?: string; selectorIndex?: number; } @@ -232,61 +233,67 @@ function replaceTagAtMatch(html: string, match: TagMatch, newTag: string): strin return `${html.slice(0, match.start)}${newTag}${html.slice(match.end)}`; } -export function findTagByTarget(html: string, target: PatchTarget): TagMatch | null { - if (target.id) { - const idPattern = new RegExp(`(<[^>]*\\bid=(["'])${escapeRegex(target.id)}\\2[^>]*)>`, "i"); - const match = idPattern.exec(html); - if (match?.index != null) { +function execDataAttrPattern(html: string, attr: string, value: string): TagMatch | null { + const pattern = new RegExp(`(<[^>]*\\b${attr}=(["'])${escapeRegex(value)}\\2[^>]*)>`, "i"); + const match = pattern.exec(html); + if (match?.index == null) return null; + // Defensive: a second exact match means a duplicate id/attr in the source + // (id drift). Don't silently patch the first while leaving the other stale — + // surface it. By the mint contract this should never fire. + const all = html.match(new RegExp(`<[^>]*\\b${attr}=(["'])${escapeRegex(value)}\\1[^>]*>`, "gi")); + if (all && all.length > 1) { + // eslint-disable-next-line no-console + console.warn( + `sourcePatcher: ${attr}="${value}" matched ${all.length} elements; patching the first. ids/attrs must be unique per document.`, + ); + } + return { tag: match[1], start: match.index, end: match.index + match[1].length }; +} + +function findTagByClass(html: string, target: PatchTarget): TagMatch | null { + const classMatch = target.selector?.match(/^\.([a-zA-Z0-9_-]+)$/); + if (!classMatch) return null; + const cls = classMatch[1]; + const pattern = new RegExp( + `(<[^>]*\\bclass=(["'])[^"']*\\b${escapeRegex(cls)}\\b[^"']*\\2[^>]*)>`, + "gi", + ); + const selectorIndex = target.selectorIndex ?? 0; + let match: RegExpExecArray | null; + let currentIndex = 0; + while ((match = pattern.exec(html)) !== null) { + if (currentIndex === selectorIndex && match.index != null) { return { tag: match[1], start: match.index, end: match.index + match[1].length, }; } + currentIndex += 1; + } + return null; +} + +export function findTagByTarget(html: string, target: PatchTarget): TagMatch | null { + if (target.hfId) { + const result = execDataAttrPattern(html, "data-hf-id", target.hfId); + if (result) return result; + } + + if (target.id) { + const result = execDataAttrPattern(html, "id", target.id); + if (result) return result; } if (!target.selector) return null; const compositionIdMatch = target.selector.match(/^\[data-composition-id="([^"]+)"\]$/); if (compositionIdMatch) { - const compId = compositionIdMatch[1]; - const pattern = new RegExp( - `(<[^>]*\\bdata-composition-id=(["'])${escapeRegex(compId)}\\2[^>]*)>`, - "i", - ); - const match = pattern.exec(html); - if (match?.index != null) { - return { - tag: match[1], - start: match.index, - end: match.index + match[1].length, - }; - } + const result = execDataAttrPattern(html, "data-composition-id", compositionIdMatch[1]); + if (result) return result; } - const classMatch = target.selector.match(/^\.([a-zA-Z0-9_-]+)$/); - if (classMatch) { - const cls = classMatch[1]; - const pattern = new RegExp( - `(<[^>]*\\bclass=(["'])[^"']*\\b${escapeRegex(cls)}\\b[^"']*\\2[^>]*)>`, - "gi", - ); - const selectorIndex = target.selectorIndex ?? 0; - let match: RegExpExecArray | null; - let currentIndex = 0; - while ((match = pattern.exec(html)) !== null) { - if (currentIndex === selectorIndex && match.index != null) { - return { - tag: match[1], - start: match.index, - end: match.index + match[1].length, - }; - } - currentIndex += 1; - } - } - - return null; + return findTagByClass(html, target); } export function readAttributeByTarget(