feat(studio): wire hfId through DOM-edit patch targets, activate hfId lookup path (R7, T5a)

[vanceingalls]

Summary

• Extracts buildDomEditPatchTarget(selection) helper in domEditingLayers.ts — a pure function that maps DomEditSelection fields (including hfId) to a patch target object
• Replaces 3 inline patchTarget literal constructions in useDomEditCommits.ts with calls to this helper, forwarding hfId automatically at the patch-element and remove-element commit sites
• Adds hfId: entry.element.getAttribute("data-hf-id") ?? undefined to the z-index reorder cast object (this path lacks a DomEditSelection so reads the attribute directly)
• Updates the no-target guard (!id && !selector) to also allow hfId as a valid patch target identity

Why

Second and final step of R7 Task 5a (Studio wire). PR #1296 planted hfId into DomEditSelection via resolveDomEditSelection. This PR forwards it into the JSON body of every DOM-edit patch request, activating the hfId-first lookup branches in both patch engines:

• sourceMutation.ts:65 (findByHfId via [data-hf-id="…"] querySelector) — server path
• sourcePatcher.ts:278 (execDataAttrPattern regex) — client path

Prior to these two PRs, every edit targeted by CSS selector only, meaning an element's source position could drift if the selector matched a different element after an edit. With both PRs merged, DOM edits for elements that have a pinned data-hf-id now target by id directly, giving the stable-id guarantee R1 was built for.

Architecture note: timeline path not included (Task 5b)

The 3 sites in useTimelineEditing.ts build targets from a serialized TimelineElement with no DOM node — getAttribute isn't callable there. That path still falls through to the selector fallback and keeps working. Task 5b (carry hfId on TimelineElement) is tracked in the plan doc as a separate open decision.

Fixture file changes

style-2-prod and style-10-prod failures/actual.html grew data-hf-authored-id attributes on composition-level elements. These files are actual.html snapshots written by the producer test harness on failure — they capture live rendered output including whatever runtime attributes were present. data-hf-authored-id is a pre-existing core attribute (unrelated to data-hf-id); it appeared because a developer ran the producer tests locally while developing T5a and the snapshots were accidentally committed alongside the code change. These files are not load-bearing golden baselines — CI passes without reading them.

Test plan

• buildDomEditPatchTarget unit tests in domEditingLayers.test.ts:
  • includes hfId when selection has it
  • passes through id/selector when hfId is absent
• All 65 studio test files pass, all 72 core test files pass

🤖 Generated with Claude Code

[vanceingalls]

• feat(studio): carry hfId on TimelineElement, wire through buildPatchTarget (R7, T5b) #1299
• feat(studio): wire hfId through DOM-edit patch targets, activate hfId lookup path (R7, T5a) #1297 👈 (View in Graphite)
• 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]

The Studio logic is correct: buildDomEditPatchTarget is a clean extraction of the three previously-duplicated inline literals, and the updated guard (!id && !selector && !hfId) correctly admits hfId-only targets. The z-index reorder path reading data-hf-id directly off the entry element is the right call since there's no DomEditSelection available there.

P2 — producer fixture noise needs explanation before merge.

packages/producer/tests/style-10-prod/failures/actual.html and style-2-prod/failures/actual.html carry +589/-34 and +324/-30 of compiled output that includes __hfAuthoredRootId, __hfAuthoredRootAttr, __hfReplaceAuthoredRootIdSelectors, and data-hf-authored-id attributes — none of which are related to this PR. The chat test failure count jumped from 34 → 71 failed checkpoints. These look like they were carried in from an unrelated compiler PR somewhere in the stack. Two questions:

1. What PR introduced the data-hf-authored-id / authored-root-id runtime code that shows up in these fixtures?
2. Are those pre-existing test failures, or did this stack introduce new regressions in the chat producer test?

If these are pre-existing failures from an upstream PR and the count jump is explainable, they're fine to leave as-is. But the fixture diff should be acknowledged so reviewers aren't left wondering whether +958 lines of compiled HTML is expected.

Everything else is good.

[james-russo-rames-d-jusso]

Reviewed at 0e9eec4. Stack 2/3 — wires hfId through DOM-edit patch targets (+958/-81, 6 files). The actual code change is small (~45 lines across domEditing.ts, domEditingLayers.ts, useDomEditCommits.ts); the +913 lines of diff are in two regression-test fixture files (style-2-prod/failures/actual.html, style-10-prod/failures/actual.html) — those are the load-bearing oddity here.

Net on the code: clean extraction of buildDomEditPatchTarget(selection) into a single helper, replacing two duplicate hand-rolled patch-target constructions. The no-target check at useDomEditCommits.ts:475 correctly accepts hfId alone as a valid target (!patchTarget.id && !patchTarget.selector && !patchTarget.hfId), so hfId becomes a first-class identifier — exactly the contract the R1 server side has been waiting for. End-to-end check on Home's review angles:

• hfId presence/absence (angle #1): ✓ helper passes hfId through optionally; legacy elements fall back to id/selector via the existing R1 server-side findTargetElement precedence.
• Server acceptance (angle #2) + Type parity (angle #3): ✓ inherits from #1296. The patch target shape matches MutationTarget and PatchTarget.
• Cutover ordering (angle #4): ✓ Once deployed, clients now send hfId in patch bodies. Server-side findByHfId was already in place via R1. If server hasn't deployed R1 (theoretical — R1 is merged), client falls back via id/selector since server reads target.id / target.selector independently. Safe.
• Backwards-compat (angle #6): ✓ The new attribute read at useDomEditCommits.ts:556 (entry.element.getAttribute("data-hf-id") ?? undefined) handles missing attributes. Same empty-string caveat as my #1296 concern #1 applies here too.
• R7 prior-stack interaction (angle #8): ✓ Studio's selection-aware patch construction now matches the contract PreviewAdapter (R7 T1) uses for hit-testing — data-hf-id is read at both ends.

The fixture-file diff is the thing I'd want resolved before merge. Otherwise, two minor concerns.

Concerns

1. The 913-line failures/actual.html diff in two fixture files is the odd thing in this PR

Files affected:

• packages/producer/tests/style-2-prod/failures/actual.html (+324/-30)
• packages/producer/tests/style-10-prod/failures/actual.html (+589/-34)

Naming convention tests/<style>/failures/actual.html strongly implies "actual output captured when the regression test failed" — which would mean these are dev-time artifacts, not load-bearing golden files. The diff content (sampled the first 50 lines of the style-2 diff) shows a @font-face import being added with an inlined data:font/woff2 base64 payload, which is also not directly related to hfId wiring.

Three possible explanations:

• (a) These ARE the golden expected files despite the name — the regression compares against them, and they had to be regenerated to include freshly-minted data-hf-id attributes from R7's write-back path. If so, the naming is misleading and the diff is load-bearing.
• (b) They're dev-time captures committed accidentally — the test framework writes these on failure, the author ran tests locally, captured the output, and added the files to the PR without realizing. CI passes either way because the framework regenerates them.
• (c) Stale snapshots updated to match a separate (unrelated) producer change — the font-face import addition hints at something other than hf-id work.

CI status confirms all regression-shards pass (including style-2-prod in shard-5 and style-10-prod in shard-3), which means the test currently agrees with whatever shape these files are in. So they're consistent with the test, but the question is whether they SHOULD be in this PR at all.

Two paths to resolve:

• Revert the fixture diffs if they're (b) or (c) and confirm CI still passes (which it does — the tests don't depend on these files).
• Add a one-line PR-body note explaining the regen if they're (a): "fixture HTML updated to capture freshly-minted data-hf-id attributes from R7 write-back; this PR also picks up <style-2/10> font-face change from #NNNN."

(a)-as-confirmation costs zero; (b)-as-cleanup tightens the PR by ~75%. Either is fine — just want certainty.

2. Third site reads data-hf-id with the same empty-string issue as #1296

useDomEditCommits.ts:556:

{
  element: entry.element,
  id: entry.id ?? null,
  hfId: entry.element.getAttribute("data-hf-id") ?? undefined,
  ...
}

Same pattern as domEditingLayers.ts:372 (in #1296). The ?? undefined only converts null to undefined; an empty-string data-hf-id="" passes through as "", which then becomes [data-hf-id=""] in the selector and matches any other empty-hf-id elements.

There are now three sites reading this attribute (resolveDomEditSelection, buildDomEditPatchTarget's caller, and this batch path). Three opportunities for the same bug. Worth a one-line helper:

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

…then replace the three sites. Tightens the contract and gives a single place to handle whitespace, empty values, and any future normalization.

(Same root cause as my #1296 concern #1 — confirming it generalizes.)

Nits

• useDomEditCommits.ts:189 — the original code constructed patchTarget with an explicit type annotation ({ id?: string | null; selector?: string; selectorIndex?: number }) which carried documentation value. The replacement const patchTarget = buildDomEditPatchTarget(selection) loses that annotation. The helper's return type covers it, but a one-line // patch target including hfId for R7 lookup precedence comment would land in lieu of the lost type tag.
• buildDomEditPatchTarget returns a plain { id, hfId, selector, selectorIndex } object. If selection.hfId === undefined, the returned object still has the key with value undefined (e.g., { hfId: undefined, ... }). When serialized to JSON, undefined fields are dropped (good). But: if any caller does a shallow comparison or iterates Object.keys(target), the key appears. Defensive: omit undefined fields with ...(selection.hfId !== undefined && { hfId: selection.hfId }). Cosmetic.
• Tests for buildDomEditPatchTarget cover (a) hfId present and (b) id-without-hfId. Missing the empty-string case (per concern #2) and the "all three identifiers" case (id + hfId + selector — what does the server-side precedence pick?). The third case is more about documenting R1's server-side contract than verifying buildDomEditPatchTarget, but worth at least a comment in the test that exercises it.
• Telemetry: the trackStudioEvent calls in useDomEditCommits.ts don't tag whether a patch used hfId vs id vs selector. If the team wants to track adoption of the hf-id keyspace post-cutover (e.g. what % of patches hit the hf-id lookup branch), this is the moment to add a lookupKey: "hfId" | "id" | "selector" tag.

Questions

• Fixture HTML diffs — please confirm whether the style-2-prod and style-10-prod failures/actual.html updates are intentional regenerations (concern #1).
• Cutover deployment plan — is the studio + server going to deploy atomically, or is there a window where studio sends hfId to a server version that doesn't yet have R1's findByHfId? (My read: R1 is already in main and deployed, so this is theoretical. Confirming.)
• buildDomEditPatchTarget shape vs PatchTarget interface — the helper returns an inline object literal, not PatchTarget. Would typing it as PatchTarget lock the contract more cleanly?

What I didn't verify

• Whether the same fixture diff appears in other PRs in main that haven't merged yet (could indicate an upstream change being pulled in via this PR's rebase). Quick git log on those two fixture files in main would resolve this.
• Cross-package contract: studio's PatchTarget and server's MutationTarget are now both { id?, hfId?, selector?, selectorIndex? }. Verified the shape matches; didn't verify the wire format (JSON body) is byte-identical.
• The full chain from studio commit through to findByHfId lookup — the integration test is implicit in CI's regression-shards passing.

Clean, focused wiring change. The fixture-file diff is the only thing that requires a confirmation note rather than just a code review.

— Review by Rames D Jusso

[miguel-heygen]

Re-reviewing at 0e9eec4 (same commit as the prior round).

The code concern that Rames and I both raised — empty-string ?? undefined at useDomEditCommits.ts:554 — is resolved by the readHfId helper added in #1299's new commit (1a4f4e5). That fix propagates down when the stack merges, so no action needed in this PR.

Still open: fixture diff explanation.

packages/producer/tests/style-10-prod/failures/actual.html (+589/-34) and style-2-prod/failures/actual.html (+324/-30) are still in the diff without a PR-body note. These files contain compiled output with __hfAuthoredRootId / data-hf-authored-id that isn't related to hfId wiring. Two questions from both rounds of review are still open:

1. Are these framework-regenerated artifacts (framework writes them on every run) or accidentally-committed dev-time captures?
2. What caused the chat visual-failure count to jump 34→71? Pre-existing upstream regression, or newly introduced by this stack?

A one-liner in the PR description ("failure fixtures are framework-regenerated on each run; chat count jump is pre-existing from #NNNN") closes both. If they're accidental, reverting them keeps the PR diff focused.

No new code issues. Will approve once the fixture explanation is posted.

[miguel-heygen]

Force-push confirmed clean — failures/ artifacts gone, only 4 files remain (+45/-17 actual code). The context on those files is helpful; pre-existing data-hf-authored-id in rendered DOM is unrelated to this PR's scope.

✅ Approve.

[james-russo-rames-d-jusso]

Re-reviewed at d31b1536. Delta is fixture-artifact cleanup only — style-2-prod/failures/actual.html and style-10-prod/failures/actual.html shrink by ~849 lines net, no code change. Confirms the Slack note: failures/actual.html captures were locally committed alongside the code, and the data-hf-authored-id runtime hits in the artifact are unrelated upstream attribute additions.

No concerns from my side. Prior round-2 review's substantive items (clean buildDomEditPatchTarget helper extraction, hfId plumbed through the DOM-edit commit path, no-target check accepts hfId alone) carry forward unchanged.

— Review by Rames D Jusso

The base branch was changed.