test(core): previewAdapter contract failing tests (T10 spec for R7)

[vanceingalls]

Summary

• Adds the T10 PreviewAdapter contract test suite as failing specs (TDD red phase for Task 3)
• Tests cover elementAtPoint ancestor walk + stage-root exclusion, applyDraft/revertDraft CSS custom property drafts, commitPreview patch extraction, and getElementTimings
• All 20 tests fail until Task 3 implements createPreviewAdapter — confirms tests are real (not tautological)

Test plan

• bunx vitest run src/studio-api/helpers/previewAdapter.test.ts — 20 fail on this branch, 20 pass after Task 3 (feat(core): implement createPreviewAdapter (R7, Task 3) #1291) merges

[vanceingalls]

• refactor(core,studio): extract draft-marker constants + R7 code-review fixes (Task 4) #1292
• feat(core): implement createPreviewAdapter (R7, Task 3) #1291
• 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 👈 (View in Graphite)
• 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]

Clean TDD red-phase setup. 20 real assertions that fail on the throw-stub and will pass once Task 3 lands — exactly the right shape for a spec PR.

One design note worth tracking into Task 3: elementAtPoint accepts opts?: { atTime?: number } but the contract tests don't verify that atTime triggers any time-seek on the GSAP player — they just verify live-opacity filtering (by setting el.style.opacity between calls). That's the right thing to test in isolation, but it means atTime semantics are unspecified in the contract. If the intent is to seek-then-read, that behavior should be captured in a getElementTimings or adapter-integration test before R7 ships.

Otherwise ✅ on the spec.

[james-russo-rames-d-jusso]

T10 spec is well-shaped — clean TDD red-phase. The conversion from it.todo stubs to 20 real assertions tightens the contract, and the test bodies are minimal-fixture-per-case (the make() helper is the right level of indirection) so the reviewer can read each test independently.

Walked the 20 tests against the contract surface declared in the stub (previewAdapter.ts:1-28):

• elementAtPoint — 4 tests cover root exclusion, ancestor walk, no-id-orphan, opacity skip.
• applyDraft / revertDraft — 5 tests cover move payload, resize payload, gesture marker, prop clear, original-translate restore.
• Edge cases — 4 tests pin double-apply overwrite, idempotent revert, mid-drag opacity re-eval, outer-vs-nested stage-root.
• commitPreview — 4 tests pin null-on-no-gesture, move patch, resize patch, marker clear.
• getElementTimings — 3 tests pin authored timing read, no-id ignore, defined-but-empty when id-present-but-no-timing.

That's 20 = 4+5+4+4+3, matching the body claim. The deletions (-52 on the test file) are pure it.todo placeholders going to real it() bodies — no coverage regression, just shape conversion.

Concerns

[atTime semantic is under-specified in the spec] Two of the tests pass { atTime: 1.0 } to elementAtPoint:

• L83-89: "skips elements whose computed opacity is 0 at the given playhead time" — sets opacity: "0" inline, then expects null at atTime: 1.0.
• L182-188: "elementAtPoint filtering is stable when playhead changes mid-drag — opacity re-evaluated per call" — mutates elem.style.opacity = "0" between calls and re-checks.

Both tests assert behavior that can be satisfied by reading current getComputedStyle and ignoring atTime entirely — which is exactly what #1291's impl does. The spec doesn't actually pin "the value at atTime is what's used"; it pins "the current computed style is what's read, regardless of atTime." That's a perfectly defensible MVP contract (caller seeks the timeline to atTime before invoking), but the test names imply the adapter uses atTime for evaluation.

Two options to tighten the spec:

1. Rename to make the current-read semantics explicit: "skips elements whose currently-computed opacity is 0" and drop the atTime arg from these two tests (since it's unused by the contract being pinned). The interface keeps atTime?: number as an unused-for-now hint.
2. Add an it.todo (or it.skip) for the true atTime semantic — "uses GSAP timeline state at atTime to evaluate opacity, not the current playhead" — so the contract carries the gap forward.

The second is better long-term; the first is fastest. Worth picking one before the spec is the contract that lives in main.

[Nested-root test is one assertion away from pinning a fragile heuristic] L189-201 verifies that an inner data-hf-root with data-hf-id IS a target. The contract this pins is "data-hf-id beats data-hf-root." But what about an inner data-hf-root without data-hf-id (a nested sub-comp that the author forgot to tag, or one that's tagged at a deeper level)? The current spec would return null for that case — same as the outer stage root — which is probably wrong for the use case (a nested sub-comp without an explicit id is still a draggable surface).

Either:

• Add a fourth assertion to the nested-roots test: a nested data-hf-root with NO data-hf-id returns null — pins "any data-hf-root without data-hf-id is treated as stage-root, including nested." This documents the limitation as intentional.
• Or: change the heuristic so only the OUTERMOST data-hf-root is excluded (e.g. by checking ancestor chain for another data-hf-root parent), and update both the impl and the test.

Picking one closes the latent ambiguity. The first is cheaper.

[createPreviewAdapter stub throws on call rather than at import] The stub at previewAdapter.ts:23-28:

export function createPreviewAdapter(
  _document: Document,
  _opts?: { resolvePoint?: (x: number, y: number) => Element | null },
): PreviewAdapter {
  throw new Error("not implemented — Task 3");
}

This is the correct red-phase shape (each test fails in its own assertion with a clear error rather than the whole file failing to load), but it means CI on #1286 alone reports 20 failing tests. If the merge model is "stack lands atomically through Graphite MQ," that's fine. If individual stack members can land independently — e.g. someone needs to revert #1289-#1292 but keep #1286 — main breaks for everyone pulling.

Worth confirming the merge-order guarantee, or noting in the PR body that this PR must NOT land alone (already says "20 fail on this branch, 20 pass after Task 3 (#1291) merges" in the test plan, but a banner-style "DO NOT LAND ALONE" in the description would help the MQ pickup decision).

Nits

[// fallow-ignore-file code-duplication] L1 lint suppression — assume this is a project-local linter directive (didn't see it elsewhere). If fallow is the typo'd intent of "follow" and this is a long-lived directive, it should be in a comment that lints don't strip. If it's a typo of an existing tool, worth normalizing.

[adapterWith helper hides the test's adapter creation] adapterWith at L46-48 wraps createPreviewAdapter(document, { resolvePoint }) — fine for hit-test paths, but tests that DON'T use hit-testing (applyDraft, revertDraft, commitPreview, getElementTimings) still call adapterWith(() => null). That's correct but reads slightly noisy. A second helper adapter() returning createPreviewAdapter(document) (no opts) for non-hit-test cases would tighten ~12 call sites. Cosmetic.

Questions

[Where does R7 plan getElementTimings get used?] The signature returns timings keyed by hfId, where the values can have start/end undefined — but the test L266-272 just verifies the entry "is defined." If the caller is going to treat missing-timing entries differently from absent-entry-entirely, the contract should be tighter ("returns the keys for all data-hf-id elements, with start/end undefined when not authored" vs "may or may not return such entries"). The current test allows both.

What I didn't verify

• That all 20 tests will pass on #1291's impl. Walked the impl logic against each test, looks aligned, but I didn't run the test suite locally.
• That getComputedStyle(el).opacity returns "1" in jsdom for elements without explicit opacity. The test "returns the nearest ancestor with data-hf-id" relies on this — if jsdom returns "" instead, parseFloat("") → NaN, and the #1291 impl's parseFloat(...) || 0 would treat it as opacity=0 → null. I trust jsdom returns "1" (standard behavior), but linkedom is referenced in the file header — if the test runner is linkedom-backed instead of jsdom, this changes.
• Whether the regression-shards in CI run unit tests too, or just the regression-style render fixtures. If unit tests live in a separate workflow that I'm not seeing in the rollup, the 20-fail state may not actually break CI.

Verdict

Solid test contract. The two contract-tightening items above are worth landing in this PR — they're the difference between "tests pin behavior" and "tests describe behavior accurately." Once those are sharpened, the spec is locked and #1291's impl just has to satisfy it. Ready from where I sit; leaving as a comment.

— Review by Rames D Jusso

[miguel-heygen]

Building on Rames's review.

Rames flagged three things I'd amplify:

Escalating: "DO NOT LAND ALONE" is not explicit enough in the PR body. The body says "20 tests fail on this branch, 20 pass after Task 3 merges" — that's correct but passive. If Graphite MQ ever picks up this PR in isolation (e.g. a rebase conflict drops one of the downstackers), CI on main fails for every downstream consumer. Add a bold ⚠️ Must merge atomically with #1289–#1292 via Graphite to the body description — MQ picks up from the body description metadata.

Confirming: nested inner data-hf-root without data-hf-id returns null — needs a pinned test. The current spec doesn't say whether a nested sub-composition data-hf-root with no data-hf-id is treated the same as the outermost stage root (correct) or as a valid drag target (wrong). Either add the fourth assertion to the nested-roots test now, or the ambiguity will produce a bug report later. Rames laid out both options — option 1 (pin the current null behavior) is the 3-line path.

New: getElementTimings "is defined" assertion is too loose. The test at L266-272 asserts timings["hf-notimed"] is defined but doesn't assert its shape ({start: undefined, end: undefined}). #1291's impl and #1292's NaN fix both happen to produce the same shape, but a future impl that returns timings["hf-notimed"] = { start: 0, end: 0 } (zeroed, not undefined) would also pass the test. One more assertion (expect(timings["hf-notimed"].start).toBeUndefined()) tightens the contract.

Everything else from Rames's pass is accurate. The spec is otherwise solid.

[james-russo-rames-d-jusso]

Round 2 — HEAD is unchanged at 7d1bf83 since my round-1 review. The round-2 work for the spec items I flagged (atTime semantic disambiguation in test names, nested data-hf-root without data-hf-id) landed in #1292's top commit d916901 rather than here:

• previewAdapter.test.ts:67-72 — test rename to "...currently-computed opacity is 0 (atTime is a caller-seek hint, not evaluated by the adapter)".
• previewAdapter.test.ts:74-82 — new test "returns null for nested data-hf-root without data-hf-id (treated same as outer stage root)".
• previewAdapter.test.ts:143-152 — new test "resize → move switch clears width/height props" (pins the cross-type prop leak I flagged on #1291).
• previewAdapter.test.ts:178-180 — test rename to "...inline opacity changes mid-drag — computed style re-evaluated per call" + drops the unused atTime args.

So the spec-tightening is now in the stack — just at the top rather than here. My round-1 review on the test file (the contract-shape walkthrough + the red-phase / stub structure) still stands as-is for the code at this SHA.

Ready from where I sit; leaving as a comment.

— Review by Rames D Jusso

[jrusso1020]

Approved on behalf of James.

The base branch was changed.