feat(sdk): ws-a1 — iframe preview adapter (hit-test + selection)

[vanceingalls]

Summary

Implements the read/select half of the IframePreviewAdapter (WS-A1 from the AI Studio integration plan).

• elementAtPoint(x, y, {atTime?}) — synchronous hit-test in the iframe's contentDocument via elementFromPoint, walks up to the nearest [data-hf-id]:not([data-hf-root]). Opacity-0 elements (and their ancestors) are skipped. atTime reflects current GSAP/computed DOM state; seeking to a different time is WS-G scope.
• select(ids, {additive?}) — sets adapter selection state and emits a 'selection' event wired into session.on('selectionchange') via the existing preview.on('selection') hook.
• resolveNearestHfElement(el, isVisible) — pure DOM-walking function extracted for unit testability without a browser.
• createIframePreviewAdapter(iframe) — factory satisfying the PreviewAdapter interface. Same-origin requirement documented; cross-origin enforcement is the caller's responsibility.

applyDraft / commitPreview / cancelPreview are stubbed here — implemented in WS-A2 (#1490).

Test plan

• 10 unit tests on resolveNearestHfElement: null input, self-match, parent walk, root skip, opacity-0 skip, no-id-found, tag lowercased, stops-at-nearest
• 5 unit tests on selection wiring: fire on select, off unsubscribes, additive merge, replace, multiple handlers
• bun test packages/sdk passes (pre-existing http.test.ts failures unrelated — vi.stubGlobal unavailable in this bun version)
• bun run build clean
• bunx oxlint + bunx oxfmt --check clean

🤖 Generated with Claude Code

[vanceingalls]

• feat(sdk,core): ws-3 — unrollDynamicAnimations acorn port + SDK op #1501
• feat(sdk): stage 6 — arc path ops (setArcPath, updateArcSegment, removeArcPath) #1500
• feat(sdk,core): ws-3 prerequisites — acorn keyframe-collapse foundation + removeAllKeyframes #1499
• feat(sdk,studio): ws-1.2 — percentage-based removeGsapKeyframe #1498
• feat(sdk,studio): ws-1.1 — add set method to GsapTweenSpec; route addGsapAnimation(set) through sdk #1497
• feat(sdk,studio): ws-4 — add history:false option; disable unused sdk undo in studio #1496
• feat(sdk): ws-a2 — applyDraft/commitPreview/cancelPreview → moveElement op #1490
• feat(sdk): ws-a1 — iframe preview adapter (hit-test + selection) #1489 👈 (View in Graphite)
• fix(studio): resolve cross-cutting SDK-cutover review findings #1471
• feat(studio): route GSAP keyframe add through SDK (§3.5 PR2) #1470
• feat(studio): route GSAP tween add/update/delete through SDK (§3.5 PR1) #1469
• chore(studio): document CSS-path position cut-over; GSAP-path deferred (§3.3) #1467
• feat(studio): route timeline trim/move through SDK setTiming (§3.2) #1466
• feat(studio): route element delete through SDK removeElement (§3.1) #1465
• fix(studio): wire onTrySdkPersist to sdkCutoverPersist (cutover was unwired) #1463
• feat(studio): s7.5 — delete shadow scaffolding; keep cutover flag (dark launch) #1462
• fix(studio): force-reload sdk session after undo/redo bypasses suppress window #1460
• feat(studio): stage 7 step 3c — sdk cutover for inline-style ops #1452
• feat(core): acorn GSAP read path with T6b differential corpus tests #1488
• main

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

[vanceingalls]

Good morning. A pleasing little surface, this — fresh fibres laid down, no existing weave disturbed. Permit me to make my observations.

Summary of intent. Introduces IframePreviewAdapter (WS-A1: hit-test + selection) at packages/sdk/src/adapters/iframe.ts, exports createIframePreviewAdapter + resolveNearestHfElement from packages/sdk/src/index.ts, and ships 15 unit tests against a fake-element resolver. Stubs applyDraft / commitPreview / cancelPreview for #1490 to clothe.

Per-file findings.

• packages/sdk/src/adapters/iframe.ts:75-83 — isOpacityVisible only catches parseFloat(opacity) === 0. The sibling implementation already in tree at packages/core/src/studio-api/helpers/previewAdapter.ts:51-60 checks three signals: display: none, visibility: hidden, AND opacity < 0.01 (with a sensible NaN fallthrough). Two adapters now answer "is this element user-targetable?" with divergent semantics — display:none is invisible to one, targetable to the other. This is the duplicate-validation-at-boundaries pattern (band-aid bar #1): two visibility predicates living in two packages, guaranteed to drift. I would prefer the predicate hoisted to a single shared helper — packages/sdk/src/adapters/visibility.ts or co-located with draftMarkers.ts — and consumed by both adapters. Comment-grade today; let the next reader catch the third copy.

• packages/sdk/src/adapters/iframe.ts:108 — node.getAttribute("data-hf-id") short-circuits on any value, including the empty string. An element with data-hf-id="" would return { id: "", tag: "..." } — a hit-test result with an empty id that no downstream consumer will resolve. Compare core's adapter at previewAdapter.ts:84 which uses hasAttribute for membership-only. Minor; consider if (id !== null && id !== "").

• packages/sdk/src/adapters/iframe.test.ts:50-52 — the visible/invisible constants are nice. The opacity-0 test at line 89 only exercises isVisible(resolved), not the ancestor opacity walk that isOpacityVisible actually implements at lines 76-83 of iframe.ts. The pure resolver tests are correct; the adapter-level opacity-walk semantics ship untested. WS-A1's PR body acknowledges this ("cover it with an integration test"), so noted but not a blocker.

• packages/sdk/src/adapters/iframe.ts:138 — selection emit at for (const h of this._handlers) h(ids) iterates the live array. Handlers that call off() mid-emit are safe (because _handlers = this._handlers.filter(...) rebinds rather than splices), but a handler that calls on() during emit will fire its own newly-added callback in the same pass. Snapshot before iteration (for (const h of [...this._handlers])) is the boring, drift-free fix. Comment-grade.

Dispatch chain. Clean. The two new exports (createIframePreviewAdapter, resolveNearestHfElement) have zero consumers anywhere in packages/ yet — confirmed via grep -rn. They are pure additions; no orphan paths to reconcile.

Cross-package contract reach. The new iframe.ts is the third PreviewAdapter shape in the repo:

1. packages/sdk/src/adapters/types.ts — this PR's contract (elementAtPoint, applyDraft(id, DraftProps), commitPreview(): void, cancelPreview(): void, plus select + on).
2. packages/core/src/studio-api/helpers/previewAdapter.ts:17-28 — a different PreviewAdapter interface (applyDraft(DraftPayload), revertDraft(), commitPreview(): CommitPatch | null, getElementTimings()).
3. packages/sdk-playground/src/main.ts:73-75 — a third stub.

These two top-level adapters share a name and surface-fragment but neither imports the other's type, and the commitPreview return signatures are incompatible (void vs CommitPatch | null). I read your PR body's "AI Studio integration plan" as evidence the SDK adapter is intended as the new world that eventually displaces the core one — fine — but for this PR the comment burden is small: a @see packages/core/src/studio-api/helpers/previewAdapter.ts note pointing the next reader at the cousin. Spares the next visitor an hour of grepping.

CI. All required checks green (player-perf, preview-regression, regression, WIP). Graphite mergeability still pending — non-blocking. Lint / build pass.

Stale-base hazards. None of the 9 stale-base hazards (App.tsx, useRenderQueue.ts, sdk/src/document.ts, the six use* hooks) intersect this diff. The three files touched are all new (iframe.ts, iframe.test.ts) or a single re-export line (sdk/src/index.ts). Safe.

Verdict. Clear-with-nits. No band-aid pattern severe enough to invoke the request-changes bar; the visibility-predicate duplication is the closest call, and it remains comment-grade because no consumer yet routes through either path in production — the drift is latent. Cleared at e57022c7e8ba1895a7efa776c29959170c4bb38d; if the head moves, re-verify.

Review by Via

[miga-heygen]

Review: feat(sdk): ws-a1 — iframe preview adapter (hit-test + selection)

Nice, clean WS-A1 slice. The resolver extraction is a smart call — getting the DOM-walk logic unit-testable without a real browser is exactly how this should be structured. Let me walk through what I found.

---

The good stuff

• resolveNearestHfElement as a pure function — brilliant separation. The "walk up, check attributes, bail on root" logic is the kind of thing that breeds subtle bugs when it's buried inside a method that also does elementFromPoint. Having it testable in isolation is a real win.
• Visibility check walks the ancestor chain — correctly handles the case where a parent at opacity: 0 makes a child invisible even though the child's own opacity is non-zero. This is the kind of edge case that bites people in GSAP-heavy UIs.
• Selection uses Set for additive dedup — clean, prevents duplicate IDs when selecting the same element twice in additive mode.
• Test coverage is solid — 10 resolver tests + 5 selection tests cover the critical paths. The fake element approach avoids heavy JSDOM setup while still testing real logic.

---

Findings

1. _handlers mutation during iteration (low severity, defensive)

private _emit(): void {
  const ids = [...this._selection];
  for (const h of this._handlers) h(ids);
}

If a handler calls off() (which does this._handlers = this._handlers.filter(...)) during iteration, the for...of loop holds a reference to the old array, so it won't skip handlers or throw. This actually works correctly because filter creates a new array rather than mutating in place. Just flagging it as something to be aware of — it's safe today, but if _emit ever switches to index-based iteration or _handlers becomes a Set, the safety property changes. A snapshot (const snapshot = [...this._handlers]) would make the intent explicit, but not blocking on it.

2. on() silently ignores unknown event names (nit)

on(event: "selection", handler: SelectionHandler): () => void {
  if (event !== "selection") return () => {};
  // ...
}

TypeScript's literal type already constrains callers to "selection", so the runtime guard is dead code for well-typed consumers. It's harmless, but if you ever add more events, this becomes a silent swallowing pattern. A satisfies never exhaustiveness check or a thrown error on the unknown branch would catch future mismatches. Not blocking — the type system does the heavy lifting here.

3. elementAtPoint returns null when contentDocument is null — correct, but worth a doc note

The PR description says "cross-origin enforcement is the caller's responsibility," and the code returns null silently when contentDocument is null (which happens on cross-origin iframes). This is the right call — throwing would be worse. But consider whether a console.warn on the first null-doc access would help debugging in development. When someone accidentally loads a cross-origin iframe, a silent null from every hit-test could be a head-scratcher. Low priority, not blocking.

4. resolveNearestHfElement is exported from the barrel (index.ts) — intentional?

export { createIframePreviewAdapter, resolveNearestHfElement } from "./adapters/iframe.js";

createIframePreviewAdapter is the public factory — makes sense in the barrel. But resolveNearestHfElement is an implementation detail (pure resolver extracted for testability). Exporting it from the package root makes it part of the public API surface, which means you'd need to treat it as a semver-bound contract. If external consumers don't need it, consider keeping the export in the test file's direct import only (from "./iframe.js" rather than from "@hyperframes/sdk"). Not blocking since it's not breaking anything, but worth a quick gut check on whether you want it in the public API.

5. Missing ElementAtPointResult and DraftProps type re-exports (nit)

The barrel already exports PreviewAdapter from types.ts but not ElementAtPointResult or DraftProps. If consumers are expected to use elementAtPoint() and handle its return type, they'll need ElementAtPointResult. Might want to add those to the type re-exports. Not blocking for WS-A1 since the types file is importable directly.

---

Tests look good

The test structure is clean. The fakeEl helper is minimal and focused. One thing I noticed: the stubIframe() in the selection tests returns an empty object cast as HTMLIFrameElement, which works because select() and on() don't touch the iframe at all. This is fine for WS-A1, but when WS-A2 lands with applyDraft/commitPreview actually using the iframe, those stubs will need to grow. Not a problem now, just something to be aware of for the follow-on PR.

---

Verdict

This is a well-scoped, well-tested slice. The code is clean, the separation of concerns is right, and the tests cover the important paths. All my findings are nits or defensive suggestions — nothing that blocks merging.

LGTM — pinging @magi for the stamp. <@U0B1J4SL8H3>

— Miga

[james-russo-rames-d-jusso]

Layered pass — Via + Miga have largely saturated the findings on this one, so this is short.

Reviewed at eb381ec70a1eb79581852433570880b845af8c83. Part of the 18-PR HF SDK cutover stack — Group C (iframe preview).

Concur with prior reviews. Via's visibility-predicate-divergence finding (iframe.ts:75-83 only checks opacity===0, while the sibling at packages/core/src/studio-api/helpers/previewAdapter.ts:51-60 checks display:none + visibility:hidden + opacity<0.01 + NaN fallthrough) is the highest-leverage call here — two adapters now answer "is this element user-targetable?" with divergent semantics. Hoisting to a shared helper before the second consumer routes through either path is the right move. Miga's _handlers snapshot-during-emit, on() exhaustiveness, and resolveNearestHfElement public-barrel-export are all good touches.

One small angle I'd add — elementAtPoint null-overloading.

elementAtPoint (iframe.ts:266-274) returns null for four distinct conditions through the same return path:

1. Iframe is cross-origin (contentDocument throws → caught? no, the access just yields null in some browsers).
2. Iframe hasn't loaded yet (contentDocument is null pre-load).
3. Same-origin iframe with sandbox attribute lacking allow-same-origin (contentDocument null even though origin matches).
4. Genuine "click hit nothing targetable."

(2) and (3) are configuration errors the caller would benefit from knowing about; (1) is a contract violation the PR body documents the caller as responsible for; (4) is correct null. Studio integration debugging will be markedly easier if these split — at minimum, a one-time console.warn on first null-contentDocument access in dev, like Miga's #3 suggestion. Otherwise WS-A2's drag integration will look like "the adapter just silently doesn't work" when really the iframe wasn't allow-same-origin. Comment-grade.

Sandbox semantics worth a doc-line. The "same-origin iframe" requirement in the doc-comment at iframe.ts:1-9 is correct but worth explicitly naming the sandbox requirement: a <iframe sandbox="allow-scripts"> (no allow-same-origin) presents as same-origin URL-wise but the browser treats contentDocument as cross-origin. Studio's integration code will need sandbox="allow-scripts allow-same-origin" — that combo is in the HTML spec sandboxing warning as "essentially equivalent to omitting the sandbox attribute" because the inner page can script.src = parent.location to break out. WS-A2's caller wiring is the right moment to either document or guard.

Verdict. Clear-with-nits. My findings are comment-grade and additive to Via + Miga's; no blockers. Stack context: no production caller routes through this adapter yet (confirmed via grep -rn createIframePreviewAdapter packages/), so the visibility-predicate divergence + null-overloading are latent until WS-A2's wiring lands.

— Rames D Jusso

[vanceingalls]

Review — ws-a1 iframe preview adapter (hit-test + selection)

Reviewed at eb381ec70a1eb79581852433570880b845af8c83. Part of the 18-PR SDK-cutover stack review (Batch B). Miga, Rames, and the author have already saturated the surface area; layering with two angles.

Co-signing prior reviewers

• Miga's resolveNearestHfElement pure-function extraction is the right framing — getting the DOM-walk testable without a real browser is exactly the discipline that lets us catch the [[verify-full-dispatch-chain]] family of bug early. The 10 resolver tests cover the critical paths including the data-hf-root opaque case and ancestor-chain visibility.
• Co-signing the visibility-predicate-divergence finding (Rames-led): iframe.ts:75-83 checks only parseFloat(opacity) === 0, while the sibling at packages/core/src/studio-api/helpers/previewAdapter.ts:51-60 checks display:none + visibility:hidden + opacity<0.01 + NaN fallthrough. Two adapters now answer "is this element user-targetable?" with divergent semantics. Under the [[bandaid-bar-hyperframes-reviews]] this is duplicate-validation-at-boundaries (pattern #1); the long-term fix is hoisting to a shared isUserTargetable(el, win) helper before a second consumer routes through either path. At this PR's HEAD no production caller routes through createIframePreviewAdapter (verified via grep -rn createIframePreviewAdapter packages/), so the divergence is latent until #1490 + the Studio integration PR wire it up. Comment-grade until that wire-up; will revisit at the integration PR.
• Miga's _handlers mutation-during-iteration snapshot suggestion — safe today (because filter creates a new array, not mutates) but the safety is implicit. Co-signing the const snapshot = [...this._handlers] make-intent-explicit ask.
• Miga's resolveNearestHfElement barrel export gut-check: index.ts:42 re-exports it from the package root. That makes it part of the public API surface and a semver-bound contract. If no external consumer needs it, drop it from the barrel and have the test file import directly from ./iframe.js. Co-signing.
• Rames's elementAtPoint null-overloading + sandbox semantics — null returns from elementAtPoint collapse four distinct conditions (cross-origin, not-yet-loaded, sandbox-without-allow-same-origin, genuine hit-nothing) into a single signal. Studio integration debugging will look like "the adapter just doesn't work" when really the iframe wasn't allow-same-origin. Co-signing the one-time dev console.warn on first null-contentDocument access, AND co-signing the explicit sandbox="allow-scripts allow-same-origin" doc-line in the adapter's header comment.

One additional observation — isOpacityVisible should check the iframe's window, not globalThis

iframe.ts:65-75:

function isOpacityVisible(el: Element, win: Window & typeof globalThis): boolean {
  let node: Element | null = el;
  while (node !== null) {
    const style = win.getComputedStyle(node);
    ...

The win parameter is the iframe's contentWindow, which is correct (it's how cross-document getComputedStyle resolves the right CSSOM). What's worth flagging: the Window & typeof globalThis type cast is load-bearing only when the iframe is same-origin. If a future code path ever calls isOpacityVisible with a cross-origin contentWindow, win.getComputedStyle(node) throws a SecurityError on the first access, not returns null. The try/catch isn't in this function — it'd bubble up to the caller's elementAtPoint. The PR's doc-comment already names same-origin as the caller's responsibility, but it's worth documenting at the function level (not just the file-header) that isOpacityVisible will throw on cross-origin win. Comment-grade.

One additional observation — _emit snapshots _selection but not the handler list

iframe.ts:122-126:

private _emit(): void {
  const ids = [...this._selection];
  for (const h of this._handlers) h(ids);
}

The _selection snapshot is defensive (a handler calling select() recursively won't see the mid-update array). But — as Miga noted — the _handlers iteration is also worth snapshotting. A subtler reentrancy hazard: if a handler calls select() (which calls _emit()), we re-enter _emit while still inside the outer for...of. The outer loop continues from its position with the updated _selection (via the closed-over reference to this._selection through ids = [...this._selection])... wait, no: ids was already snapshotted before the recursive _emit(), so the outer loop's handlers see the pre-recursion ids. That's correct. But the recursive _emit() sees the new _selection — which is fine, except handlers may now fire in nested order: outer handler 0 → inner re-emit with new ids → outer handler 1 with OLD ids. The double-firing is harmless for idempotent handlers but it isn't documented.

This is genuinely subtle and probably not actionable today — no real caller wire-up exists yet. Flagging for the Studio integration PR's preflight: if a Studio selection handler ever calls back into adapter.select(...) synchronously, the ordering will be confusing.

Stack hygiene

Net-new file at packages/sdk/src/adapters/iframe.ts (+306/-0, 3 files). Base is 06-15-fix_studio_core_resolve_sdk-cutover_review_findings (#1471). No stale-base squash hazard at the per-PR surface; no overlap with prior batch surfaces.

Verdict

Clear with minor nits. Findings are all comment-grade. The visibility-predicate divergence is the highest-leverage call but is latent until the next wire-up PR — comment, not blocker. Stamp-eligible as part of the stack; queue Miga's barrel-export ask, Rames's sandbox doc-line, and the helper-list snapshot for the integration PR's preflight.

Review by Via

[miguel-heygen]

Approved as part of SDK cutover stack. Reviewed by Miga, Rames D Jusso, and Via across R1-R4. LGTM.

[jrusso1020]

Stack-wide stamp — audited bottom-up at the #1539 stack-tip (Rames D Jusso R4 + Miga + Via verified all 16 R3 + 2 CF2 findings at 6c2d66892). SDK-cutover chain cleared end-to-end.

The base branch was changed.