feat(core): implement createPreviewAdapter (R7, Task 3)

[vanceingalls]

Summary

• Implements createPreviewAdapter in packages/core/src/studio-api/helpers/previewAdapter.ts, greening all 20 T10 tests from test(core): previewAdapter contract failing tests (T10 spec for R7) #1286
• elementAtPoint: walks ancestors from hit element, returns first data-hf-id match; stops at data-hf-root without data-hf-id (outermost stage root); filters elements whose computed opacity is 0
• applyDraft: sets --hf-studio-offset-x/y or --hf-studio-width/height CSS custom properties + data-hf-studio-manual-edit-gesture marker; tracks original translate for restore
• revertDraft: removes all draft CSS props and gesture marker; restores original translate
• commitPreview: extracts moveElement or resize patch from gesture state, clears marker
• getElementTimings: scans all [data-hf-id] elements for data-start/data-end attributes

Test plan

• bunx vitest run src/studio-api/helpers/previewAdapter.test.ts — 20 pass
• bunx vitest run — 1378 pass, 0 fail

[vanceingalls]

• refactor(core,studio): extract draft-marker constants + R7 code-review fixes (Task 4) #1292
• feat(core): implement createPreviewAdapter (R7, Task 3) #1291 👈 (View in Graphite)
• feat(core): hf-id write-back to disk + serve-time surfacing (R7, Tasks 1-2) #1289
• test(core): previewAdapter contract failing tests (T10 spec for R7) #1286
• feat(core): sourceMutation data-hf-id targeting (R1, T7) #1272
• feat(studio): sourcePatcher data-hf-id targeting (R1, T3) #1271
• feat(core): clip-model hf- ids minted at parse, emitted as data-hf-id (R1) #1270
• feat(core): ensureHfIds node-id pass + shared hf- mint helper (R1) #1269
• main

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

[miguel-heygen]

Core logic is sound and the adapter boundary is clean. Three things I'd flag before merge:

P1 — revertDraft cannot restore translate if applyDraft never set it
GestureState stores originalTranslate: string | undefined, captured from el.style.getPropertyValue("translate") at gesture start. revertDraft does if (state.originalTranslate !== undefined) el.style.translate = state.originalTranslate. So far fine.

The bug is: applyDraft sets --hf-studio-offset-x/y (CSS custom properties), not translate directly. commitPreview produces a moveElement patch from those custom properties, which the caller applies as a real translate. If a caller applies applyDraft → commitPreview → applyDraft again on the same element (e.g. nudging twice without a revert between), originalTranslate on the second gesture captures whatever commitPreview's caller set — which is correct. But if commitPreview is skipped and only revertDraft is called, translate may have been set by a previous committed gesture and revertDraft will leave it in place. Fine for the happy path, risky for error recovery or gesture-cancel flows. At minimum, add a contract test for "commit → revert after abort does not restore stale translate".

P2 — elementAtPoint opacity check uses inline style, not computed

const opacity = el.style.getPropertyValue("opacity")

This misses opacity inherited from ancestors (e.g. parent is opacity: 0 in a stylesheet). The spec tests set el.style.opacity = "0" which works in jsdom, but in a real browser an element whose parent is hidden via CSS class would slip through the hit test. Consider getComputedStyle(el).opacity — jsdom supports it when styles are applied via CSSOM.CSSStyleDeclaration.

P3 — hardcoded CSS property strings will be de-duped in #1292
"--hf-studio-offset-x" etc. appear as string literals throughout this file. #1292 extracts them to draftMarkers.ts. No action here — just confirming the duplication is transient.

Otherwise the adapter contract is well-shaped and the closure-scoped GestureState is the right approach.

[james-russo-rames-d-jusso]

Clean implementation — one focused file (previewAdapter.ts), 101 lines, satisfies the 20-test contract from #1286. Walked each method against the spec:

• elementAtPoint — resolvePoint?.(x,y) for hit, walks ancestors, returns first data-hf-id match (filtered by opacity), or null on outermost data-hf-root. ✅
• applyDraft — records originalTranslate, sets --hf-studio-offset-x/y for move or --hf-studio-width/height for resize, marker via data-hf-studio-manual-edit-gesture. ✅
• revertDraft — clears all 4 draft props + marker, restores translate from gesture state. ✅
• commitPreview — null on no-gesture, derives moveElement or resize patch, clears marker. ✅
• getElementTimings — scans [data-hf-id], reads start/end attrs. ✅

20 tests pass cleanly against this impl.

Concerns

[Mixed-type re-apply leaks CSS props] The applyDraft impl doesn't clean up previous-gesture state when called repeatedly. Walk through:

1. applyDraft({ type: "resize", hfId: "X", w: 200, h: 100 }) — sets --hf-studio-width: 200px, --hf-studio-height: 100px, captures originalTranslate.
2. applyDraft({ type: "move", hfId: "X", dx: 10, dy: 5 }) — gesture state is REPLACED, sets --hf-studio-offset-x: 10px, --hf-studio-offset-y: 5px.

After step 2, the element has ALL FOUR custom props set (width + height from step 1 still there, offset-x/y from step 2). commitPreview() returns a moveElement patch (correct for the last applyDraft), but the element's visual state has both move AND resize drafts applied — the studio's CSS rules consuming --hf-studio-* will render with both translate and explicit dimensions, which is not what the user gestured.

The double-apply test (previewAdapter.test.ts:165-172) only covers move→move, where the same two custom props get overwritten — so the test passes. The contract specifies only "second applyDraft overwrites first" without saying "and the previous payload's props are cleared first." Test passes, contract holds, behavior leaks.

#1292 fixes this by auto-reverting in-flight gestures before applying a new one:

if (gesture) {
  const prev = findById(gesture.payload.hfId);
  if (prev) revertGesture(prev, gesture);
  gesture = null;
}

Same merge-coupling pattern as #1289 — if the stack lands atomically the bug never reaches main; if not, fold the fix into this PR (and ideally add a test for resize→move switch).

[Opacity check is fragile in non-jsdom environments] L26-29:

function opacity(el: Element): number {
  const view = doc.defaultView;
  if (!view) return 1;
  return parseFloat(view.getComputedStyle(el).opacity) || 0;
}

When getComputedStyle(el).opacity returns "" (empty string — possible in linkedom or any DOM-like environment that doesn't compute CSS cascade), parseFloat("") → NaN → NaN || 0 → 0 → returns null from elementAtPoint. Every element without an explicit inline opacity becomes invisible.

The current test environment is jsdom, which returns "1" by default for unstyled elements, so this passes. But the file-header comment in previewAdapter.test.ts (#1286) references linkedom too — so this adapter is plausibly going to be exercised in a linkedom context at some point, and the contract will break silently.

#1292 fixes this:

const op = parseFloat(style.opacity);
return Number.isNaN(op) || op >= 0.01;

NaN treated as visible. Plus adds display: none / visibility: hidden checks. The full isVisible is the safer shape. Worth folding in.

[atTime param is silently ignored] L33: the per-call opts is _opts (intentionally unused). The signature accepts { atTime?: number } and the spec L83-88 / L182-188 of #1286 names atTime in two test cases. But the impl reads current getComputedStyle regardless of atTime.

This is OK as an MVP — the caller is expected to seek the GSAP timeline to atTime before invoking elementAtPoint, and the impl reads whatever the timeline-driven inline styles currently say. But:

• The contract isn't documented anywhere. A new caller will reasonably assume passing atTime is enough.
• There's no warning when atTime is passed but the timeline isn't at that position.

Two paths:

• JSDoc the method: "atTime is currently a marker; caller must seek the timeline to atTime before invoking. Future work may evaluate atTime against timeline state directly."
• Or: keep the param off the public signature until it's actually honored.

Concern not blocker — but worth picking one before downstream callers come to rely on the false promise.

Nits

[findById interpolates hfId into a CSS selector] L21:

function findById(hfId: string): HTMLElement | null {
  return doc.querySelector(`[data-hf-id="${hfId}"]`) as HTMLElement | null;
}

mintHfId in parsers/hfIds.ts produces hf-[a-z0-9]{4} — completely safe for CSS-selector interpolation. But the type signature of findById accepts any string. If a future contract change loosens the id format (or a caller passes user-controlled input), a payload like "hf-aaaa\"][data-evil]" breaks the selector or matches unintended elements. CSS.escape exists for this. Cheap defense:

return doc.querySelector(`[data-hf-id="${CSS.escape(hfId)}"]`) as HTMLElement | null;

Cosmetic now; load-bearing if the format ever evolves.

[Closure captures opts from outer scope, shadowing the _opts parameter] L33-34:

elementAtPoint(x, y, _opts) {
  const hit = opts?.resolvePoint?.(x, y) ?? null;

The bare opts resolves to the OUTER createPreviewAdapter's arg (which has resolvePoint). _opts is the per-call options. Both are valid scopes, but the visual proximity makes it easy to misread "are we using per-call opts or factory opts?" at a glance. Renaming the per-call param to something like _perCallOpts (or just _atTimeOpts) reduces the ambiguity. Cosmetic.

[Array.from(doc.querySelectorAll(...)) then iterate] L70:

for (const el of Array.from(doc.querySelectorAll("[data-hf-id]"))) {

NodeList is already iterable. Drop the Array.from:

for (const el of doc.querySelectorAll("[data-hf-id]")) {

(#1292 already drops it.) Cosmetic.

Questions

[Should elementAtPoint ignore display: none / visibility: hidden?] The current opacity-only filter would happily return elements that are display-none'd. Not exercised by the tests (#1286 only set inline opacity), but a draggable hidden element seems wrong. #1292's isVisible handles this. Worth confirming whether the use case ever exposes display-none'd or visibility-hidden'd elements (it probably does — GSAP display: none is common for "off-screen until time T" patterns).

What I didn't verify

• That the 20 tests from #1286 actually all pass on this impl's branch in CI. The regression-shards CI status is in_progress; if vitest unit tests run in a separate workflow I'm not seeing, I haven't observed them green.
• That getComputedStyle in jsdom returns "1" rather than "" for an unstyled element (the opacity-fragility concern above hinges on this). The "returns the nearest ancestor with data-hf-id" test in #1286 implicitly tests this — if jsdom returned "", that test would fail.
• That no other studio-side code reads data-hf-studio-manual-edit-gesture and expects a specific value beyond the truthy check. The studio's manualEditsDom.ts uses a token-based gesture marker (sets and compares tokens); the previewAdapter uses "true" as the value. If the studio's gesture-aware code is meant to coordinate with PreviewAdapter's gestures, the value mismatch could matter. Worth a sanity check.

Verdict

The implementation greens #1286's 20 tests and the shape is right. The three Concerns are all "the contract test happens to be loose enough that the bug doesn't surface" — and #1292 fixes all three independently. If the stack merges atomically, this is ready. Otherwise, fold #1292's previewAdapter.ts fixes into this PR. Ready from where I sit; leaving as a comment.

— Review by Rames D Jusso

[miguel-heygen]

Building on Rames's review.

Rames flagged three concerns and I'd escalate one of them.

Escalating: the mixed-type re-apply leak is the highest-risk bug in the stack. When the user applies a resize gesture, then applies a move gesture on the same element (without committing in between — likely in a rapid drag scenario), all four --hf-studio-* custom props are set simultaneously. The studio CSS that consumes these props will render the element with BOTH a translate AND explicit width/height — a combined transform the user never applied. #1292 fixes this with the auto-revert on re-apply, but if #1291 ever lands without #1292, studio gestures will produce visually corrupted intermediate states.

This is worse than my P1 (the revert-after-commit path) because it fires on the hot path — any user who drags twice without committing hits it immediately. Rames's framing is accurate; I'd categorize it P0 if the stack can land non-atomically.

Confirming: getComputedStyle vs inline style. Rames and I flagged the same gap. The NaN-guard fix in #1292 (Number.isNaN(op) || op >= 0.01) also resolves the linkedom empty-string case. If the stack lands atomically: fine. If not: the opacity(el) function needs the NaN guard now.

New: the opts vs _opts closure shadowing. Rames flagged this as cosmetic; I'd upgrade it to P2. In elementAtPoint, opts (the factory arg) is silently used instead of _opts (the per-call arg). This isn't just confusing to read — if a future call site passes per-call opts expecting them to be honored (e.g. a different resolvePoint per call for multi-layer hit testing), the factory opts win silently. The underscore prefix signals "intentionally unused" but it's the wrong signal — the per-call opts ARE the intended interface. Rename to clarify intent.

[james-russo-rames-d-jusso]

Round 2 — HEAD is unchanged at b32249b since my round-1 review. All three concerns I flagged (mixed-type re-apply leaks, fragile opacity check in non-jsdom DOMs, atTime ignored without documentation) are addressed in #1292's 2af2228 and 4afb5a7 commits at the top of stack rather than here:

• Auto-revert on applyDraft re-apply (resize → move no longer leaks width/height props).
• isVisible replacement (display / visibility / NaN-tolerant opacity).
• JSDoc on PreviewAdapter.elementAtPoint's atTime param spelling out caller-seek semantics.
• Plus the CSS.escape-fallback nit and _opts → _perCallOpts rename I'd raised — both landed.

So the round-1 concerns on this PR are merge-coupled to #1292. Same atomicity property as #1289 — stack lands as one unit and this is ready; lands alone and the fragile bits are exposed.

My round-1 review at this SHA still stands.

— Review by Rames D Jusso

[jrusso1020]

Approved on behalf of James.

The base branch was changed.