fix(sdk): code-review follow-ups (WS-B/C/3.C, #1569/#1570/#1572)

[vanceingalls]

Code-review follow-ups (#1569 / #1570 / #1572)

Branched from main. Addresses the still-outstanding review concerns from the merged SDK-hotspot PRs that were not already hoisted into #1573 (which carries the variableModel dedup, parseInsertableFragment guard, EXCLUDED_TAGS export, stale-id guard, isObjectVariableValue array guard, and the WS-C "not yet wired" timing-resolver honesty fix — intentionally not duplicated here).

WS-B (#1569)

• validateVariables now requires the discriminant fields for object-valued font/image (the MEDIUM both reviewers flagged). A {name: 42} font or {foo: 42} image previously passed runtime validation and surfaced as a bogus font-family / missing image at render. Font now requires {name: string, source: string}, image requires {url: string}; fallback strings still accepted. (+5 tests)
• Dropped the ImageValue [key: string]: unknown index signature (MEDIUM) — it let any {url}-shaped object through and swallowed key typos, and was inconsistent with FontValue. Replaced with explicit alt? / fit? optionals.
• Documented the OverrideSet widening for SDK consumers (object values require narrowing).

WS-C (#1570)

• getElementTimings caches parsed GSAP labels by exact script text — avoids a full acorn re-parse on every read; the content key invalidates on any edit so renumbered tweens never yield stale labels.
• Documented the end-inclusive label window and the best-effort extractGsapLabels catch.

WS-3.C (#1572)

• Added typed Composition.addWithKeyframes / replaceWithKeyframes — previously only the op-union entries existed, leaving the surface asymmetric with addGsapTween (Studio had to drop to raw dispatch). replaceWithKeyframes doc notes the returned id is the NEW (renumbered) one. (+2 round-trip tests)
• Extracted a shared KeyframeSpec type (was duplicated inline across both ops + Studio mirrors) and documented position as seconds / number-only.

Deferred to #1573 (avoid double-conflict)

These nits live in files #1573 heavily rewrites (mutate.ts, gsapWriterAcorn.ts, mutate tests); landing them from main would guarantee conflicts for low value, so they ride with #1573:

• replaceWithKeyframes meta.replacedAnimationId breadcrumb
• _auto: 1 numeric inline comment in the acorn writer
• parity-test "assert the remove actually removed" guard
• feat(sdk): ws-b variables/brand — object-valued font/image + B1 JSON model #1569 rootId-null inverse-path test

Gates

• bun run build ✅
• core validateVariables 18/18 ✅ · sdk session.dispatch 19/19 ✅
• oxlint 0/0 ✅ · oxfmt --check ✅ · fallow 0 issues ✅ · typecheck ✅

🤖 Generated with Claude Code

[james-russo-rames-d-jusso]

R2 follow-up review — HEAD 6e64a35a

Cross-read against my R1 findings on #1569 / #1570 / #1572 (and verified the deferred items are correctly NOT in this PR). Layout below is a per-claim verdict matrix.

---

WS-B (#1569)

✅ validateVariables discriminant-field check — validateVariables.ts:90-105 (font) and :113-128 (image) now require name: string, source: string (font) / url: string (image) on the object branch. The reviewer-flagged {name: 42} and {foo: 42} shapes are now caught. Spotted the small structural refactor (early-return for typeof value === "string" to keep the object-branch type-narrowed) — cleaner than the prior conditional. +5 tests cover the new failing cases AND the still-accepted fallback-string case. Solid resolution.

✅ Dropped ImageValue [key: string]: unknown index signature — types.ts:263-268. Now explicit alt?: string / fit?: "cover" | "contain" | "fill" | "none" | "scale-down". FontValue and ImageValue are now symmetric (both closed object shapes). Good.

✅ OverrideSet widening doc-comment — types.ts:56-61. Explicit consumer-narrowing reminder.

---

WS-C (#1570) — parity-unverifiable flag, alternative-resolution credit

⤴️ Parity-unverifiable flag deferred to #1573 — my R1 flag was that resolveTimings had zero non-test consumers and the parity test asserted f(x).toEqual(f(x)) (reflexivity tautology). #1588 doesn't wire it; per body, the "not yet wired" honesty fix lives in #1573 (which adds packages/core/src/compiler/timingResolver.{ts,test.ts} — 157 + 219 LOC). Crediting per feedback_open_item_alternative_resolution — the operational goal (honest status for the unwired resolver + real test coverage of the resolver's actual behavior) is addressed in the sibling PR. I'll re-verify there when it lands.

✅ Content-keyed label cache — session.ts:160-176. _gsapLabelCache: { script, labels } keyed by script text. Verified: getGsapScript returns el.textContent (string snapshot, not a live reference), and setGsapScript writes new text — so primitive === cache-key compare invalidates correctly on every edit. Renumbered tweens cannot yield stale labels. Worst-case is an O(n) string compare per getElementTimings call on unchanged scripts, but that's much cheaper than the acorn re-parse it replaces.

✅ Docstrings — end-inclusive [enterAt, exitAt] window (session.ts:205-206) and best-effort extractGsapLabels catch (gsapParserAcorn.ts:1202-1203) are now documented inline.

---

WS-3.C (#1572)

✅ Typed Composition.addWithKeyframes / replaceWithKeyframes — session.ts:247-282 (impls) and types.ts:433-456 (Composition interface). Studio no longer needs to drop to raw dispatch. Round-trip tests at session.dispatch.test.ts:204-235 cover add → replace → undo → undo → serialize-equals-before.

✅ replaceWithKeyframes doc note about renumbered id — session.ts:280-281 AND interface types.ts:451-452: "Returns the replacement's animationId (treat as NEW — position-derived IDs renumber after the remove)". Addresses my R1 "no signal that op.animationId is stale" finding. The breadcrumb (meta.replacedAnimationId) is correctly deferred to #1573.

🟡 KeyframeSpec "single source of truth" — partial resolution

types.ts:225-237 declares the shared KeyframeSpec interface and the doc-comment says "Studio-side mirrors (KeyframeEntry/KeyframeSpec) should import this rather than redeclare the shape." But the Studio-side mirrors are NOT updated in this PR — they're still locally-scoped duplicate declarations with identical fields:

• packages/studio/src/utils/sdkCutover.ts:438 — type KeyframeSpec = { percentage, properties, ease?, auto? } (file-local)
• packages/studio/src/hooks/useGsapAnimationOps.ts:186 — type KeyframeEntry = { percentage, properties, ease?, auto? } (function-local)
• packages/studio/src/components/editor/propertyPanel3dTransform.tsx:7 — type KeyframeEntry = Array<{...}>
• packages/studio/src/player/components/TimelineClipDiamonds.tsx:4 — interface KeyframeEntry

Also: KeyframeSpec is not re-exported from packages/sdk/src/index.ts (only FontValue / ImageValue / ElementTimingSnapshot were added in #1588's index updates). So even if a Studio consumer wanted to import { KeyframeSpec } from "@hyperframes/sdk", they can't — the canonical shape isn't reachable through the barrel.

Suggested follow-up (either here or a Studio cleanup PR):

1. Add KeyframeSpec to the export type {…} block at sdk/src/index.ts:4-19.
2. Update at least sdkCutover.ts:438 and useGsapAnimationOps.ts:186 to import from @hyperframes/sdk rather than redeclare.

This isn't a blocker for #1588 — the SDK-side dedup is done — but the body's claim "was duplicated inline across both ops + Studio mirrors" hasn't fully landed. Worth flagging.

---

Deferred-to-#1573 audit (sanity check)

Verified absent from #1588's diff (per body's deferred list):

• ✅ replaceWithKeyframes meta.replacedAnimationId breadcrumb — not present
• ✅ _auto: 1 numeric inline comment in the acorn writer — gsapWriterAcorn.ts not in this diff
• ✅ parity-test "assert the remove actually removed" guard — not present
• ✅ #1569 rootId-null inverse-path test — not present

---

Heads-up — cross-PR conflict surface

#1573 also modifies validateVariables.ts, session.ts, and types.ts (the same files as #1588). The deferred items themselves don't conflict (different regions). But #1573's diff currently adds the OLD weaker case "font" / case "image" checks (without discriminant fields) AND introduces ImageValue with the OLD [key: string]: unknown index signature. When #1573 rebases after #1588 lands, expect small merge conflicts in those exact regions — the resolution is "take #1588's version." Non-blocking for #1588; just noting so #1573's rebase isn't surprising.

---

CI

All completed required checks ✅ at HEAD 6e64a35a (build, typecheck, lint, fallow, validateVariables 18/18, sdk session.dispatch 19/19, runtime contract, SDK contract/smoke, CLI smoke, preview parity, player perf, global install smoke). Windows render + regression shards still in-progress at review time — typical, not blockers.

---

Verdict

Lean toward 🟢 with the KeyframeSpec export/mirror cleanup as a strong-suggestion follow-up. WS-B is fully resolved. WS-C parity-flag is resolved via alternative-resolution in #1573 (will verify there). WS-3.C typed methods + doc note land cleanly; only the mirror cleanup is partial. Not blocking on it because (a) the SDK-side dedup is real, (b) Studio mirrors are a separate-package concern, (c) the body sets clear expectations about scope.

Review by Rames D Jusso

[jrusso1020]

LGTM per Rames D Jusso's R2 routing. Verified the per-PR-R1 resolution against the actual diffs at HEAD 6e64a35a:

• #1569 (WS-B) — validateVariables.ts correctly rejects {name:42} / {foo:42} via discriminant-field requirement (validateVariables.test.ts covers the cases); ImageValue now has explicit alt? / fit? symmetric with FontValue. Clean fix on the type-safety widening RDJ flagged.
• #1570 (WS-C) — content-keyed cache landed in session.ts:160-176; pairs with the timingResolver that #1573 introduces. Will treat the parity-unverifiable concern from R1 as fully resolved once #1573 lands.
• #1572 (WS-3.C) — typed Composition.addWithKeyframes / replaceWithKeyframes added with round-trip tests; "treat returned id as NEW" doc note closes the stale-id-signal gap.

Standing follow-up (non-blocking): per RDJ, KeyframeSpec single-source-of-truth is partial — Studio mirrors at sdkCutover.ts:438, useGsapAnimationOps.ts:186, propertyPanel3dTransform.tsx:7, TimelineClipDiamonds.tsx:4 are still locally-redeclared, and KeyframeSpec isn't re-exported from packages/sdk/src/index.ts. Worth a barrel-export + Studio-import-cleanup PR after these land. Not gating this stamp.

CI all-green at this HEAD (Analyze, Build, CLI smoke, CodeQL, Format, Lint, Perf suite). Stamp matches the routing.

Stamp by Rames Jusso (per Rames D Jusso's R2 substance review at HEAD 6e64a35a).