feat(studio): carry hfId on TimelineElement, wire through buildPatchTarget (R7, T5b)

[vanceingalls]

Summary

• Adds hfId?: string field to TimelineElement in playerStore.ts
• Populates hfId from data-hf-id in all three TimelineElement construction sites in timelineDOM.ts:
  • createTimelineElementFromManifestClip (initial read + composition host re-read branch)
  • createImplicitTimelineLayersFromDOM
  • parseTimelineFromDOM
• Updates buildPatchTarget in useTimelineEditing.ts to include hfId in all three return paths, and adds an hfId-only fallback path for elements that have a stable id but no domId

Why

R7 / Task 5b. Completes the Studio wire for the timeline editing path. PR #1296 (selection builder) and #1297 (DOM-edit commits) activated the hfId-first lookup for drag/resize/style operations. This PR activates it for timeline operations: data-start timing edits, remove, and split.

The key architecture choice (Option 1 from the plan doc): store hfId on TimelineElement at DOM-parse time, rather than reverse-looking up the preview iframe at patch-construction time. The parse sites (parseTimelineFromDOM, createTimelineElementFromManifestClip, etc.) already hold a live Element reference — reading getAttribute("data-hf-id") there is zero-cost and the value travels with the serialized element through the Zustand store to useTimelineEditing.

What activates

useTimelineEditing.buildPatchTarget now returns hfId when available. All three call sites that POST to the server (persistTimelineEdit at line ~125, delete at ~285, split at ~491) now include hfId in their JSON.stringify({ target: patchTarget }) body. The server-side sourceMutation.findTargetElement already handles hfId-first lookup (T7, landed in #1272) — this closes the last client-side gap.

Test plan

• New timelineDOM.test.ts (jsdom environment):
  • parseTimelineFromDOM harvests hfId from data-start element with data-hf-id
  • parseTimelineFromDOM leaves hfId undefined when data-hf-id absent
  • createImplicitTimelineLayersFromDOM harvests hfId from implicit layer child
• All 66 studio test files pass

🤖 Generated with Claude Code

[vanceingalls]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• feat(studio): carry hfId on TimelineElement, wire through buildPatchTarget (R7, T5b) #1299 👈 (View in Graphite)
• feat(studio): wire hfId through DOM-edit patch targets, activate hfId lookup path (R7, T5a) #1297
• feat(studio,core): populate hfId in DomEditSelection + widen MutationTarget (R7, T5a) #1296
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[miguel-heygen]

Architecture is exactly right — hfId stored on TimelineElement at parse time, carried through buildPatchTarget, and surfaced as the primary key when present. The four harvesting sites in timelineDOM.ts are symmetric and complete:

• createTimelineElementFromManifestClip (main path + the hostEl-resolved path at line 130)
• createImplicitTimelineLayersFromDOM
• parseTimelineFromDOM

The hfId-only fallback in buildPatchTarget is the right safety net for elements that have a stable data-hf-id but no #id. Tests cover all three parse sites plus the buildPatchTarget logic — solid.

P3 — minor: buildPatchTarget type signature could be narrowed.

The function signature uses an inline { domId?: string; hfId?: string; selector?: string; selectorIndex?: number } object type. Since TimelineElement already declares hfId, importing and using Pick<TimelineElement, "domId" | "hfId" | "selector" | "selectorIndex"> as the parameter type would tie the signature directly to the store type and catch future field renames. Not a blocker.

Producer failure artifacts (visual-failures.json, binary frames) are clearly pre-existing fixture noise that moved when the stack rebased — not caused by this PR's logic. The diff shows only PSNR value deltas on the same test names, not new test names appearing. Fine to carry.

✅ Approve.

[james-russo-rames-d-jusso]

Reviewed at 4731cde. Stack 3/3 — wires hfId on timeline path (+901/-644, but only ~78 lines of actual code; the rest is fixture-file churn). Code change is clean and consistent with the #1296/#1297 pattern: TimelineElement.hfId? field, three DOM-parse sites populate it from data-hf-id, and buildPatchTarget in useTimelineEditing includes hfId in all return paths with a new hfId-only fallback for elements that have a stable hfId but no domId. The new fallback is the right shape — preserves backward compat (domId path still works) while enabling hfId-only addressing for dynamically-generated DOM that doesn't have a stable id.

End-to-end check on Home's review angles:

• hfId presence/absence (angle #1): ✓ TimelineElement.hfId is optional; buildPatchTarget checks domId → hfId → selector in precedence.
• Server acceptance + type parity (angles #2, #3): ✓ Inherits from #1296/#1297.
• Cutover ordering (angle #4): ✓ Server-side R1 already in place, client-side now sends hfId on every timing/delete/split mutation.
• Backwards-compat (angle #6): ✓ Optional everywhere.
• R7 prior-stack interaction (angle #8): ✓ Consistent with PreviewAdapter's hit-test contract.

But one of Home's angles raised a real bug surface that this PR makes user-visible:

• Deletion/split correctness (angle #5): ❗ See concern #1 below.

Concerns

1. Split duplicates data-hf-id on the clone — hfId-as-primary-key contract breaks post-split

sourceMutation.ts:241-243:

const clone = el.cloneNode(true) as HTMLElement;
clone.setAttribute("id", newId);
clone.setAttribute("data-start", String(Math.round(splitTime * 1000) / 1000));
clone.setAttribute("data-duration", String(Math.round(secondDuration * 1000) / 1000));

cloneNode(true) deep-copies all attributes including data-hf-id. Only id, data-start, data-duration (and data-playback-start / data-media-start if present) are reassigned. So after split:

• Original: id="hero", data-hf-id="hf-abc"
• Clone: id="hero-2" (or whatever newId is), data-hf-id="hf-abc" ← duplicate

Once this PR lands, the studio's timeline operations send target.hfId="hf-abc" first. Server-side findByHfId finds BOTH elements, logs a multi-match warning (per sourceMutation.ts:54 from #1272), and picks the first one. User edits the wrong half of the split silently.

This is in sourceMutation.ts, not in this PR's diff, but #1299 activates the bug — before this PR, the timeline path was id-keyed, and split (which assigns a new id to the clone) kept the two halves distinguishable. After this PR makes hfId the primary key, the two halves collide.

Three fix options, in order of preference:

• (a) Mint a new hfId for the clone in splitElementInHtml:

  // After clone.cloneNode + id reset:
  clone.setAttribute("data-hf-id", mintHfId());   // new minted id

  Requires plumbing the hfId minter (which lives in #1270's ensureHfIds or the R7 stack) into splitElementInHtml. Right shape long-term — both halves are independently addressable.
• (b) Clear the clone's hfId so it gets backfilled on the next save:

  clone.removeAttribute("data-hf-id");

  Cheaper — the clone's hfId is undefined until R7's write-back path re-mints it. In the meantime, the clone falls back to id/selector addressing, which works because the clone has a fresh id. The downside: a window where hfId-based references to the clone don't work (but no one has a reference to it yet, since it's brand new).
• (c) Keep the original's hfId on whichever half is "the original" (typically the first half) and clear the clone's. Same as (b) but explicit.

(b) and (c) are the right answer for this PR cycle if (a) requires too much plumbing; (a) is the clean long-term shape.

The fix lives in packages/core/src/studio-api/helpers/sourceMutation.ts and is outside this PR's stated scope, but I'd recommend bundling it here because the bug becomes user-visible the moment #1299 lands.

2. data-hf-id empty-string handling — now FIVE sites with the same issue

The four new reads in timelineDOM.ts (lines 76, 132, 192, 269):

hfId = hostEl.getAttribute("data-hf-id") ?? undefined;
entry.hfId = hostEl.getAttribute("data-hf-id") ?? undefined;
hfId: child.getAttribute("data-hf-id") ?? undefined,
hfId: el.getAttribute("data-hf-id") ?? undefined,

…plus the single read in domEditingLayers.ts from #1296 and the read in useDomEditCommits.ts:556 from #1297 = 5 read sites total across the stack. All with the same ?? undefined shape that doesn't handle the empty-string case (per my #1296 concern #1 and #1297 concern #2).

A data-hf-id="" element produces selection.hfId === "", which becomes [data-hf-id=""] selector at the server side. Likely fine in practice (no one creates empty hf-ids deliberately), but as the read sites proliferate, the chance of a fixture / migration / test setup producing an empty value grows.

Concrete:

// packages/studio/src/components/editor/domEditingLayers.ts
export function readHfId(element: Element): string | undefined {
  return element.getAttribute("data-hf-id")?.trim() || undefined;
}

Then replace all 5 sites. Cheap, centralizes the normalization, and gives a single place to add future logic (e.g., reject malformed hf-ids, log telemetry).

I flagged this as a nit in the #1296/#1297 reviews. At 5 sites in 3 PRs in one stack, it's worth landing the helper here.

3. failures/ fixture-file diffs (same family as #1297 concern #1)

This PR touches 7 failures/visual-failures.json files (~823 lines of JSON diff across chat, iframe-render-compat, render-symlinked-assets, vfr-screen-recording) plus many binary .png files in their failures/frames/ directories.

Same question as my #1297 concern #1: the failures/ directory naming implies "captured on test failure" artifacts. CI status confirms all regression-shards pass on this PR, which means these aren't load-bearing golden files. Two scenarios:

• (a) Test framework writes these always, not just on failure. They're snapshots that get regenerated, and committing them keeps the diff clean for future PRs.
• (b) They were committed accidentally — the author ran tests locally, hit a transient failure, captured the artifacts, and added them to the PR.

Looking at the file structure (PNG actual_*.png and expected_*.png side-by-side in the same directory), this looks like (a) — the framework captures both for visual diff debugging. But the question stands: should these be in a code PR?

Two reasonable resolutions:

• (a-confirm) If the framework regenerates on every run, add a .gitignore rule for failures/ and skip committing them. The diff for this PR shrinks 90%.
• (b-revert) If they were committed accidentally, revert them and let CI rebuild on next run.

The actual code change in #1299 (78 lines across 4 files) is what matters; the fixture churn is making the review harder than it needs to be.

Nits

• useTimelineEditing.ts:51-58 — buildPatchTarget precedence is now domId → hfId → selector. The domId path also includes hfId (so the server can use whichever it prefers). The hfId-only path omits id (because there's no domId to send). The selector-only path omits both. Worth a one-line comment on the function describing the precedence so the next reader doesn't have to trace three return paths:

  // Patch target precedence: domId (with hfId companion) → hfId-only → selector.
  // Server-side findTargetElement prefers hfId when both are present; the domId
  // companion lets the legacy id-keyed code path still find the element if the
  // server falls back from hfId lookup.

• timelineDOM.test.ts:11-15 — makeDoc helper is good. The third test (createImplicitTimelineLayersFromDOM) uses the same helper. Worth one more test for createTimelineElementFromManifestClip (the most-complex path with the host-resolution + composition-host re-read) to round out the harness.
• playerStore.ts:24-25 — hfId is now part of the TimelineElement Zustand state. If there's any persistence (localStorage, IndexedDB), old stored elements won't have hfId on rehydration. Probably non-issue (timeline state is derived from the DOM on every load), but worth confirming.

Questions

• Split-then-target bug — is the fix in scope for this PR or a follow-up? Concern #1.
• failures/ fixture-file commits — intentional (per concern #3) or accidental? Affects how the reviewer should treat the diff.
• Telemetry for primary-vs-secondary key usage — same question as #1297 nit. As hfId becomes the primary, tagging which patches hit hfId vs id vs selector would let the team see adoption.

What I didn't verify

• The full integration through the timeline persistence path (persistTimelineEdit, handleTimelineElementDelete, handleTimelineElementSplit) — assumed they all call buildPatchTarget(element) and the new hfId-bearing return propagates through.
• That the new hfId-only fallback path actually fires in practice (i.e., are there real TimelineElements without domId? Implicit layers might be — the test at L43-54 confirms createImplicitTimelineLayersFromDOM does populate hfId on layers).
• The PNG binary diffs in failures/frames/ — git shows them as 0/0 line changes (binary). Couldn't sample without dumping to disk.

Clean, focused wiring change. The split-time hfId collision is the only real concern; the empty-string normalization is the obvious cleanup; the fixture diff is the procedural question.

— Review by Rames D Jusso