fix(studio): keyframe system foundation — 6 root cause fixes + 25 bug fixes#1344
fix(studio): keyframe system foundation — 6 root cause fixes + 25 bug fixes#1344miguel-heygen wants to merge 1 commit into
Conversation
Fallow audit reportFound 133 findings. Duplication (72, showing 50)
Showing 50 of 72 findings. Run fallow locally or inspect the CI output for the full report. Health (61, showing 50)
Showing 50 of 61 findings. Run fallow locally or inspect the CI output for the full report. Generated by fallow. |
miguel-heygen
force-pushed
the
fix/keyframes-foundation
branch
from
0436e19 to
8a02fa8
Compare
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
PR review — hyperframes#1344 (Studio keyframe-system foundation)
Verdict: concerns-to-address (one cheap-fix CI blocker + a handful of canonical-rubric concerns; the HF-runtime substance is for Vai)
CI: red. Format fails on a single file (packages/studio/src/hooks/useDomEditSession.ts) — oxfmt --check reports "1 file"; trivially fixed with bun run format:fix. Typecheck + Test + CLI smoke still pending at review time; gating on those. CodeQL JS analyze still in progress. Fallow audit FAIL: 126 findings (65 duplication, 61 health). Lint, Build, Studio load smoke, runtime-contract test all PASS. Required regression / preview-regression / player-perf jobs are red but the failures are only in their Preflight (lint + format) upstream — same root cause as the Format check, so they'll likely flip green once formatting is fixed.
Diff scope:
25 files, +801/-154, focused on packages/core/src/parsers/gsap* (parser/serializer) and packages/studio/src/hooks/gsap* + studio gesture/recording hooks. No drive-by changes to unrelated modules — scope is coherent for the title. Heavy concentration: gsapParser.ts (+195/-40), gsapParser.test.ts (+167/-5), gsapDragCommit.ts (+138/-24), gsapRuntimeBridge.ts (+126/-31). One server-side touch (studio-api/routes/files.ts +5/-3) for the RC5 changed: false return.
"6 root causes + 25 bug fixes" — framing check:
- The 6 RCs map to distinct, named problems with a clean type-shape change to back them (
resolvedStart,implicitPosition,tweenPercentage). Not symptom-patching — the position-resolver + default-inheritance are legitimately new capabilities the cache/UI/serializer now consume consistently. RC5 (changed: false) and RC6 (id rescue) are the weakest — they read more like "make optimistic state survive parser idempotence" than root-cause fixes, but they are real. - The "25 bug fixes" are NOT a grab-bag — every one I sampled touches the same call path (drag/resize/rotate intercept → keyframe cache → diamond render → server mutation → soft reload). That said, this would have been easier to review as ~3 separately reviewable PRs: (a) parser + types + tests, (b) studio runtime bridge/drag/resize, (c) gesture-recording + soft-reload hardening. Flag for future discipline, not a blocker on this one.
- No hallucinated APIs in what I sampled —
resolveTweenStart/resolveTweenDurationexisted and were updated; the new helpers (resolvePositionString,applyTimelineDefaults,resolveTimelinePositions,sortBySourcePosition) are internal and consistent with the file's recast patterns.
Canonical-rubric findings (my lane):
-
Format CI failure → CI blocker.
useDomEditSession.tsdoesn't passoxfmt --check. One-shot fix, but it's gating four required workflows. Runbun run format:fix && git commit -am "chore: format". -
Fallow audit: 126 findings, 9 critical. The audit flags critical CRAP scores on hot paths this PR touches:
addKeyframeToScript(CRAP 38, cyclo 34),parseMotionPathNode(CRAP 211, cyclo 29),unrollDynamicAnimations(CRAP 482, cyclo 45),executeGsapMutation(CRAP 660, cyclo 53), and the new-ishtryGsapResizeIntercept(cyclo 27, cognitive 43). This PR is making them worse, not better —tryGsapResizeInterceptgrew from a tightfind → maybe-convert → commit-keyframeshape into a 100+ line method with two parallel branches (in-range vs outside-range), nested IIFE-style synthesized keyframes, and IDENTITY_ONE inlined twice. (concern, not blocker — fallow is allowed to fail on this repo today AFAICT, but it's the kind of complexity that lands tech debt the next time someone touches this hook.) -
Duplicate code in
tryGsapResizeIntercept(gsapRuntimeBridge.ts ~L200-230): the from-properties / to-properties synthesis blocks are near-identical IIFEs differing only inanim.method === "from"vsanim.method === "fromTo". ExtractsynthesizeIdentityProps(anim.properties, IDENTITY_ONE)and inline twice. (concern — Claude-Code "look ma, no helpers" smell.) -
gsapKeyframeCacheHelpers.ts—buildCacheKeyun-exported. Diff dropsexportfrom the function (line 69). I traced all its callers in the same file (readKeyframeSnapshot,writeKeyframeCache) and they're the only consumers — safe tightening. No external import I could find. (good change, just calling it out so it's not missed.) -
Implicit dependency-array fix in
useGsapTweenCache.ts(line 392): addingelementCountto the fetchKey + deps array is correct — was previously stale when elements were added/removed without sourceFile/version changing. But this re-runs the AST fetch on every element-list mutation, which could be chatty in a heavy timeline. Acceptable trade-off, but worth a comment in the code explaining whyelementCountand not the elements ref. (nit) -
Spurious
setKeyframeCache(elementId, merged)write alongsidesetKeyframeCache(\${sourceFile}#${elementId}`, merged)inuseGsapAnimationsForElement(useGsapTweenCache.ts:308): writing under two keys for the same data — this looks like a workaround for some consumer reading by bareelementId`. If so, the consumer should be fixed to use the namespaced key, or this should at least carry a comment naming the consumer. Currently it reads as "I didn't want to track down who's reading the wrong key." (concern, drift-risk) -
Discriminated
MutationResultchange (useGsapScriptCommits.ts):if (!result?.ok) { throw new Error(...) }flipped from a silentreturnto throwing. Callers up the chain need to handle the throw — I didn't trace every caller; worth verifying that the existing try/catch aroundmutateGsapScriptcalls actually catches this (and that it surfaces a user-visible toast rather than just logging). (question — Vai may also care here re: rollback semantics.) -
Test shape: good but uneven. +13 well-scoped unit tests for the parser (position resolution, defaults inheritance, default duration) — those are the right shape. But the 25 studio bugfixes (drag-extend, resize-extend, gesture sign inversion, wheel startPointer capture, captured-selection ref, RDP per-property epsilon) carry zero new unit tests in
packages/studio. The PR body lists "Manual regression test" + "Verify diamond positions stable" as unchecked checkboxes — those are the bugs most likely to silently regress. (concern — recommend at least one unit test per behaviorally-distinct studio fix where the seam is testable.) -
Type-shape changes are non-breaking.
resolvedStart?,implicitPosition?,tweenPercentage?are all optional. Serializer readsresolvedStart ?? positionso old data withoutresolvedStartstill works. Good migration discipline. - No new
@ts-expect-error,@ts-ignore, oreslint-disablecomments I caught. - No
console.logs introduced (only the pre-existingconsole.warnlines I saw).
Cross-repo consumer impact:
GsapAnimationis exported from@hyperframes/corevia the parser barrel. New fields are all optional, so external consumers (any pacific/EF code that pulls types) won't type-break. But anyone serializing+deserializing animations across a network boundary will silently dropresolvedStartunless the wire format is rev'd — worth confirming the studio-api round-trip preserves it (the parser regenerates it on every parse, so probably moot, but verify). (question for Vai or Miguel)KeyframeCacheEntry.keyframes[].tweenPercentageadded toplayerStore.ts. Same story — optional, additive, no consumer break.- No
package.jsonexports field changed. No.d.tsshape regression.
HF-runtime-lane flags (for Vai cross-check):
- RC1 position resolver math:
<resolves toprevStartand>resolves tocursor. The labelled-position cases ("label+=1"etc.) and the<offset/>offsetforms are handled, but I'm not sure the GSAP semantics for">"are "cursor" vs "previous tween's end." Vai should verify against GSAP's actual position-string contract. - RC4 non-destructive from/fromTo conversion: the new
resolvedFromValuesthreading throughconvert-to-keyframesmutations needs to be cross-checked against the server-side mutation handler instudio-api/routes/files.ts— did the schema for that mutation type get widened to accept the field? Serverchanged: falsesemantics (RC5) and how the client rolls back optimistic state are also Vai's call. tryGsapResizeInterceptoutside-range "extend" path: synthesizesfromProps/toPropsfor flat tweens with hardcodedIDENTITY_ONE = {opacity, autoAlpha, scale, scaleX, scaleY}defaulting to 1, everything else to 0. Is that the right inverse-of-from semantic for arbitrary GSAP-animatable props (e.g.rotation,skewX,xPercent)? Vai's lane.- Soft-reload
globalTimeline.getChildren(false).kill()sweep: kills bare gsap.to/from tweens not registered on__timelines. Could this stomp on tweens the host page registered for its own purposes (e.g. UI animations the studio chrome doesn't own)? Probably not in practice for the studio iframe context, but worth a Vai sanity check. - Gesture recording
pointerElementOffsetadjustment: subtracting from the first sample to remove the snap-jump looks reasonable, but the math (r.pointerElementOffset.x / (r.scale || 1)) is in the recording lane.
Stack context:
Base = main, no Graphite stack chain in the body. mergeStateStatus = BLOCKED (CI red). Standalone PR despite the Graphite link.
Peer reviewers (so far):
None human. Only github-actions[bot] (fallow audit) has commented. Vai is reviewing separately on the HF-runtime semantics lane.
— Rames D Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
CI is red — required Test job fails on 5 golden snapshot mismatches. The resolvedStart/implicitPosition fields added to GsapAnimation appear in serialized parser output but the golden snapshots in gsapParser.golden.test.ts haven't been updated. These need a snapshot refresh commit before merge. The Fallow audit failure (CRAP scores on tryGsapResizeIntercept, executeGsapMutation, readAllAnimatedProperties, resolvePositionString, etc.) is real complexity debt — see below.
Blockers
1. RC5: ok: changed inverts the mutation success contract, breaks multi-step sequences
files.ts now returns ok: changed, meaning a successful-but-no-op mutation returns { ok: false }. useGsapScriptCommits.ts throws on !result?.ok. This was meant to trigger optimistic rollback, but:
- The
executeOptimisticcallers (addKeyframe,removeKeyframe) catch the throw correctly — rollback fires. This path is fine. - The multi-step sequences in
gsapDragCommit.ts(extendTweenAndAddKeyframe, the new from()-extend path,commitFlatViaKeyframes) callcommitMutationdirectly withouttry/catch. A no-op on an intermediate step throws, the rest of the sequence is abandoned, and the element is left in a half-mutated state (e.g. tween deleted, never re-added). No recovery path exists. tryGsapDragInterceptandtryGsapResizeIntercepthave no outertry/catch. The throw propagates up throughhandleGsapAwarePathOffsetCommit(also no catch) into the React event handler — unhandled Promise rejection.useGestureCommit.ts'sstopAndCommitRecordinghastry/finallybut nocatch. A thrown commitMutation error goes unhandled (thefinallyruns, cleaning up UI state, but the commit silently failed).
Fix: keep ok: true for a successful-but-unchanged mutation; use changed: false as a skip signal in commitMutation for the skipReload path only, so multi-step sequences don't break:
// files.ts: ok stays true for HTTP success
ok: true,
changed,// useGsapScriptCommits.ts
if (!result?.ok) throw new Error(`Mutation failed: ${mutation.type}`);
if (result.changed === false) {
// no-op: skip history, reload, cache update
return;
}2. Golden snapshot tests must be updated before merge
5 gsapParser.golden.test.ts tests fail because resolvedStart and implicitPosition now appear in parser output and the snapshot files haven't been refreshed. This is blocking CI, not a flake.
GSAP-Semantics Lane (RC verification)
RC1 — Position resolver: contract is correct but resolvePositionString returns null for label offsets
The implementation correctly handles the GSAP position-string contract:
<→prevStart✓>→cursor✓<+0.2/>-0.1✓ (viastartsWith("<")/startsWith(">")withparseFloat)+=n/-=nrelative to cursor ✓- Absolute numeric → pass-through ✓
- Negative clamp to 0 ✓
One gap: named labels (e.g. "myLabel" or "myLabel+=0.5") fall through to Number.parseFloat → NaN → return null. When start == null, resolveTimelinePositions silently skips the cursor update for that tween, which leaves prevStart and cursor at their prior values. In practice this means any tween after a labeled one gets wrong positions. This is the same behavior as before (labels weren't resolved before either), but the PR description doesn't call it out. Label support would be a separate PR; worth a comment in resolvePositionString noting that labels are not yet resolved and will not advance the cursor.
Also: resolveTimelinePositions runs on the source-order-sorted call list, which is correct for chained .from() calls. But findAllTweenCalls traverses the AST in declaration order, not source-line order. The new sortBySourcePosition call uses callee.property.loc.start — this correctly sorts method-call chains by the .from/.to method position. Verified correct.
RC4 — Non-destructive from/fromTo conversion: logic inversion in the from() case
resolveConversionProps for method === "from":
// Before this PR:
if (resolvedFromValues) {
return { fromProps: { ...anim.properties }, toProps: resolvedFromValues };
}
// After this PR:
const identityTo = { /* CSS identity values */ };
const toProps = resolvedFromValues ? { ...identityTo, ...resolvedFromValues } : identityTo;
return { fromProps: { ...anim.properties }, toProps };For from(), anim.properties are the from-values (animation starts there). The to-values are the live CSS state. The old code set toProps = resolvedFromValues directly, which was correct when resolvedFromValues came from readAllAnimatedProperties (actual DOM values). The new code spreads resolvedFromValues on top of identityTo. If the caller passes { x: 100, y: 50 } as resolvedFromValues, toProps becomes { x: 100, y: 50, opacity: 1, scale: 1, ... } — the identity defaults are now mixed in, which is correct. This is a net improvement, not a regression.
However: the fromTo case has a semantic issue:
// fromTo: merge resolvedFromValues into toProps
const toProps = resolvedFromValues
? { ...anim.properties, ...resolvedFromValues }
: { ...anim.properties };
return { fromProps: { ...(anim.fromProperties ?? {}) }, toProps };For fromTo, anim.properties are already the to-values (that's what fromTo(el, fromVars, toVars) produces — toVars maps to properties). Spreading resolvedFromValues (which is the current DOM state, i.e. where the element is) into toProps overwrites the authored destination values with the current position. The result is a keyframe-format tween whose endpoint is the element's current position, not its intended destination. This will produce incorrect animations for any fromTo that has been converted mid-play.
IDENTITY_ONE defaults in tryGsapResizeIntercept and gsapDragCommit
The hardcoded set { opacity, autoAlpha, scale, scaleX, scaleY } → 1, everything else → 0. GSAP's actual identity defaults:
rotation: 0✓skewX: 0,skewY: 0✓xPercent: 0,yPercent: 0✓transformOrigin: "50% 50%"— string, not number, so it won't hit the number branch; safex: 0,y: 0✓
The set is correct for the numeric properties GSAP targets. No bug here, but the scattered hardcoded sets (appears in 3 files now) should be a shared constant.
globalTimeline.getChildren(false).kill() stomp risk
if (win.gsap?.globalTimeline?.getChildren) {
for (const child of win.gsap.globalTimeline.getChildren(false)) {
child.kill?.();
}
}getChildren(false) returns only immediate children (not nested). The __timelines-registered timelines are already killed above. What remains are bare gsap.to/from calls not registered on __timelines — these are typically authored in the user's GSAP script, which is what we want to kill.
The risk is host-page tweens. But the iframe is a HyperFrames-controlled sandbox, not the host page. win.gsap is the iframe's GSAP instance, not the parent window's. The stomp risk is essentially zero for the iframe context. Safe as-is.
Gesture recording pointerElementOffset math
The offset is computed as pointer - elCenterViewport at recording start. In recordSample, the subtracted offset is cssVarOffset.x + pointerElementOffset.x / scale. The / scale accounts for the iframe-to-document coordinate transform. This matches how cssVarOffset is computed in startRecording (also divided by iframeScale). The sign and scale are correct.
The wheel-on-first-move path:
r.basePosition.x += r.pointerElementOffset.x / iframeScale;This advances basePosition by the pointer-element snap offset so the first sample sees the element moving with the pointer rather than snapping. Directionally correct.
Important
ID-rescue regex copied five times
animationId.replace(/-from-|-fromTo-/, "-to-")Appears verbatim in removeAnimationFromScript, locateKeyframeCtx, convertToKeyframesInScript, removeAllKeyframesFromScript, materializeKeyframesInScript. Should be a resolveAnimationId(parsed, id) helper. When a new mutation function is added, it will silently miss the fallback.
clearProps: "all" wipes user-authored inline styles
After killing timelines, gsap.set(allTargets, { clearProps: "all" }) removes all GSAP-managed inline styles. clearProps: "all" in GSAP clears all inline style properties that GSAP has touched, including those that may have been in the user's original markup. Scoping to "x,y,rotation,scale,scaleX,scaleY,opacity,transform" would be safer than "all".
Soft-reload verify fires before async MotionPath script load
For scripts using motionPath, executeScript() runs in pluginScript.onload — after the verify block. The check will always find an empty __timelines for MotionPath animations and return false, causing a full reload every time. The verify should either move inside executeScript() or be deferred to after all code paths complete.
Cache double-write with bare elementId key
setKeyframeCache(`${sourceFile}#${elementId}`, merged);
setKeyframeCache(elementId, merged); // newIn three places. If two source files contain #box, the bare key is last-write-wins. At minimum, a comment explaining why the bare key is needed. The cache lookup in StudioPreviewArea uses domEditSelection?.id ?? "" which is the bare ID — that's why this was added. The fix is to make those lookups use the qualified key, not to add a second write.
Fallow CRAP scores are new violations
tryGsapResizeIntercept is now complexity 27/43 (was under threshold). executeGsapMutation was already high; the new case didn't reduce it. These are flagged as critical in the Fallow audit. The PR adds fallow-ignore-next-line comments in some places — the resize function needs one or a decomp of the outside-range path into a helper.
Nits
- Prior open follow-ups not addressed: fallow mutation switch per-case dispatch (#1314),
COMPUTED_BASELINEfilter defaults map (theallTweenedPropsgate sidesteps NaN but the map is still incomplete), dead codeMotionPathOverlay/StaggerControls, arc formula duplication (#1301-#1309). unrollDynamicAnimationsnow usesloc.parsed.timelineVar— correct fix, unlisted in the description; add a changelog line.resolveTweenDurationdefault 1→0.5 — correct, but silently changes range-check tolerance for all no-duration tweens. The test is updated. Low risk, worth a mention in the description.- Zero studio-side regression tests — 25 fixes, parser tests are solid, but
manualEditsDombase capture, drag cancel restore, from()-extend layout, soft-reload clearProps, and resize-extend remap have no automated coverage.
Requesting changes on the two blockers (CI-breaking golden snapshots + RC5 throw semantics in multi-step sequences). The RC4 fromTo case is also a blocker in the sense that it will produce wrong animations. The rest can land as follow-ups.
— Vai
miguel-heygen
force-pushed
the
fix/keyframes-foundation
branch
9 times, most recently
from
f1f5f13 to
8e59c27
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES — two required CI checks still red at HEAD 8e59c27; golden-snapshot failure is more complex than R1 described.
B1 — Golden snapshots: still failing, with a new wrinkle
CI Test is still failing at HEAD. R1 described this as "5 snapshot mismatches from new resolvedStart/implicitPosition fields — one-shot refresh." That description is no longer accurate. The actual failure is:
- Expected: "id": "#subscribe-btn-to-1000"
+ Received: "id": "#subscribe-btn-to-1000-scale"
The parseGsapScript output now includes a property-group suffix on IDs (e.g. -scale). The golden files don't reflect this. This is a behaviorally non-trivial change to the serialized parser output — the ID format changed, which affects lookup keys, coalesce keys, and any persisted compositions that store animation IDs. A snapshot refresh will make the test pass, but the ID-format break needs to be either (a) acknowledged with a migration path for stored IDs, or (b) confirmed that IDs are always re-parsed from source and never stored cross-session. Based on the ID-rescue regex pattern (animationId.replace(/-from-|-fromTo-/, "-to-")), ID shapes are assumed stable across server round-trips — adding a -scale suffix breaks that assumption.
Action needed: refresh snapshots AND verify that the ID change is safe (server mutations won't fail to locate animations by the old ID shape).
B2 — RC5 throw semantics: RESOLVED
Fixed correctly. useGsapScriptCommits.ts now throws only on result === null (network/server failure) and treats result.changed === false as a silent no-op return. The multi-step sequences in gsapDragCommit.ts were restructured to eliminate mid-sequence single-mutation throw exposure: extendTweenAndAddKeyframe is now a single atomic replace-with-keyframes call, commitFlatViaKeyframes uses a coalesceKey. The from/fromTo outside-range path does two sequential commitMutation calls (split-into-property-groups + add-with-keyframes), but neither is expected to be a no-op — the failure path is correct.
B3 — RC4 fromTo inversion: partially addressed, comment doesn't close it
The from() case was correctly fixed. The fromTo case still does:
const toProps = resolvedFromValues
? { ...anim.properties, ...resolvedFromValues }
: { ...anim.properties };A comment was added: "This is intentional and not inverted." I disagree. For fromTo, anim.properties = the authored toVars (the destination the user wrote). Spreading resolvedFromValues (current DOM state) over it overwrites the authored destination with where the element currently is. The result is a keyframe that ends at the element's current position, not its authored target. That's the inversion R1 flagged.
The comment's argument is that resolvedFromValues contains only drag-captured position props ({ x, y }) so it's "updating the endpoint to reflect the drag." That is the caller's intent, not a fix to the semantic. If resolvedFromValues is meant to be the new destination, it should be named differently and the comment should say so explicitly. As written, the variable is named after what it reads (current DOM values), spread in the position where authored targets live. Either rename + document the intent, or restructure so the drag offset is applied as a delta to anim.properties rather than overwriting it wholesale.
Rames' R1 findings — status check
Format CI (oxfmt --check): still failing. Run at HEAD 8e59c27. Rames' original file (useDomEditSession.ts) is no longer listed, but now 5 files fail: gsapConstants.ts, gsapParser.test.ts, gsapParser.ts, gsapKeyframeCacheHelpers.ts, useAnimatedPropertyCommit.ts. The format PR he described as "one-shot fix" was apparently done but new unformatted files were introduced. bun run format:fix + commit needed.
Duplicate IIFEs in tryGsapResizeIntercept: synthesizeIdentityProps() helper was introduced and is called in some paths, but the outside-range branch still contains an inline IIFE-style (() => { ... })() expression for the keyframe synthesis. Partially addressed; not fully extracted.
Double-key cache write (useGsapTweenCache.ts:308): Now has an explaining comment pointing to PropertyPanel. The better fix (lookup uses the qualified key) was not done, but the comment at least documents the technical debt. Acceptable as a follow-up.
MutationResult throw: Fixed as part of B2.
Studio unit test gap: Unchanged — 25 studio bugfixes with zero new studio unit tests.
Fallow CRAP scores: The tryGsapResizeIntercept function grew substantially (40+ new lines, new resolveGroupTween call, outside-range extend path). The Fallow critical flag on it (CRAP 51.8, cyclo 45) is higher now. Not a merge blocker if fallow is allowed to fail, but the gap widened.
Additional finding — soft-reload verify races MotionPath async load
R1 flagged this and it's still present. After the if (needsMotionPath) branch, executeScript() runs inside pluginScript.onload — after the verify block. The verify check at line 1177-1180 runs synchronously after the else branch fires, but for the MotionPath path it fires before onload, so win.__timelines is still empty. The if (tlKeys.length === 0) return false check will always return false for MotionPath scripts, causing a full reload on every soft-reload attempt. The else branch at line 1173 calls doReload() followed immediately by the verify — that's the non-MotionPath path, which is fine. The MotionPath path needs the verify moved inside onload or wrapped in a setTimeout(0) after executeScript() completes.
Blocking on: Format CI + Test CI (B1 — snapshot mismatch with ID format change), B3 fromTo semantic, and MotionPath verify race. B2 is clear.
— Vai
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R2 review — canonical-rubric lane, layering on top of @vanceingalls's R2 REQUEST_CHANGES. Verdict: concerns-to-address. Material progress (architectural rework lands, RC5 throw flip resolved, +1666 net studio test LOC), but CI still red on Format + Test, and one R2-NEW finding looks like an accidental flag-default regression that's especially worth surfacing.
R2-NEW (not in R1, not in Vai's lane)
🚨 STUDIO_GSAP_DRAG_INTERCEPT_ENABLED default silently flipped back to true
At packages/studio/src/components/editor/manualEditingAvailability.ts:50-54:
export const STUDIO_GSAP_DRAG_INTERCEPT_ENABLED = resolveStudioBooleanEnvFlag(
env,
["VITE_STUDIO_ENABLE_GSAP_DRAG_INTERCEPT"],
true,
);Commit a5bc52b ("fix(studio): add GSAP drag intercept feature flag, disabled by default (#1341)") landed 12 hours ago and added this flag with default false. This PR ships it as true. The corresponding test manualEditingAvailability.test.ts:30 — "disables GSAP drag intercept by default" — is now failing in CI for that exact reason.
Given #1341's explicit "disabled by default" framing, the flip looks accidental. If intentional, the test needs to update and the PR body should call out the default change. Worth confirming with the author before merge — this is a behavioral regression with the same shape as the recently-merged intent.
3 NEW studio test failures (not previously broken on main)
Beyond Vai's golden-snapshot fail:
manualEditingAvailability.test.ts:30— flag-default flip above.gsapSoftReload.test.ts:67— "kills existing timelines, rebinds, and re-seeks on success" → expectsapplySoftReload === true, receivesfalse.gsapSoftReload.test.ts:83— "wraps execution in__hfSuppressSceneMutationswhen available" → same shape.
Both gsapSoftReload fails correlate with Vai's MotionPath async-load race concern — either the verify returns false due to load timing or the mock-iframe shape shifted at R2. The soft-reload happy path is the workhorse for every successful mutation reload; not safe to merge red.
propertyGroup ID-suffix is a cross-package wire change
gsapParser.ts:1033 now emits IDs as ${baseId}-${propertyGroup} (e.g. #subscribe-btn-to-1000-scale). playerStore.ts:11-12 adds propertyGroup as an optional keyframe field — backwards-compat for new field shape is fine.
But the ID-rescue regex (R1 Blocker 6, still present in the codebase) hardcodes /-from-|-fromTo-/ and does NOT know about -${group}. If any persisted client state (URL hashes, localStorage selection refs, telemetry) stores pre-rework animation IDs, lookups will miss after rollout. Cross-repo GsapAnimation.id consumers in pacific could also see drift.
Vai flagged this as B1 wire-impact; confirming from the consumer-API lane that it's real and worth either (a) a migration codepath, or (b) an explicit "no persistence of these IDs" assertion in the PR body.
R1 carry-over verdicts (head 8e59c27)
| # | Finding | Verdict | Evidence |
|---|---|---|---|
| 1 | useDomEditSession.ts format fail |
🔴 open — different files | R1 file fixed; now gsapConstants.ts, gsapParser.test.ts, gsapParser.ts, gsapKeyframeCacheHelpers.ts, useAnimatedPropertyCommit.ts all fail oxfmt --check. bun run format:fix. |
| 2 | tryGsapResizeIntercept Fallow CRAP |
Now at gsapRuntimeBridge.ts:248-417 (170 LOC, up from ~100). Fallow total 126 → 133, 9 critical. executeGsapMutation CRAP 660 → 708 (files.ts:443). Trend is bad; not a merge blocker but a real follow-up. |
|
| 3 | Duplicate IIFE in resize intercept | synthesizeIdentityProps helper extracted at gsapRuntimeBridge.ts:235-244 and called L351/L354 ✅. Outside-range branch still has inline (() => {...})() at L347-360 for kfs synthesis — the IIFE wrapping itself wasn't dropped. |
|
| 4 | Cache double-write | gsapKeyframeCacheHelpers.ts:56-58 writes under ${targetPath}#${id}, bare id, AND index.html#${id} for same data. Bare-id + cross-file index.html# write still risks last-write-wins between sub-compositions sharing element IDs. |
|
| 5 | MutationResult throw flip | ✅ resolved | useGsapScriptCommits.ts:137-145 throws only on !result; treats result.changed === false as silent no-op. Server files.ts:102,888,919 returns {ok:true, changed:false} consistently. Closes Vai R1 Blocker 2 — the load-bearing half-mutation risk. |
| 6 | Studio test coverage gap | ✅ substantially resolved | +1666 net studio test LOC including snapEngine.test.ts (+657), manualEditsDomPatches.test.ts (+395), globalTimeCompiler.test.ts (+169), keyframeSnapping.test.ts (+74). Strong correction. Caveat: 3 of those tests fail (see R2-NEW). |
| 7 | elementCount dep-array comment |
Low-pri carryover. |
Vance's UX-bug verdict (architectural-plumbing lane only)
Vance's two manual-test bugs map to the architectural rework cleanly:
- "Editing one keyframe applies to all" → addressed by new
PROPERTY_GROUPStaxonomy atgsapConstants.ts:60-67decomposing mixed tweens into independent group tweens (position / scale / size / rotation / visual / other), each mapping to a separate GSAP tween.split-into-property-groupsmutation atfiles.ts:681is the enabler. Right surface. - "Keyframes added when I expect the selected to be edited" → addressed by new
activeKeyframePctstate onplayerStore.ts:83-84,182-183, consulted ingsapRuntimeBridge.ts:291-293andgsapDragCommit.ts:151-153— last-clicked diamond's pct overrides "compute from playhead."
The architectural plumbing is in this diff and looks coherent on inspection. Runtime semantics adjudication is Vai's lane — I'm not certifying the keyframe runtime works, only that the shape of the fix matches the reported failure modes.
CI gate status
| Check | State | Nature |
|---|---|---|
Format |
🔴 | Cosmetic; 5 files; bun run format:fix one-shot. |
Preflight (lint + format) ×4 |
🔴 | Upstream of Format; flips green when Format does. |
Test |
🔴 | Behavioral. 1 golden (Vai B1) + 3 studio (R2-NEW above). 1455/1459 core pass, 792/795 studio pass. |
Fallow audit |
🔴 | 133 findings (was 126), 9 critical. Tolerated repo-wide; tracked above. |
Validate docs |
🔴 | New docs/guides/keyframes.mdx + docs/plans/2026-06-11-001-feat-per-property-keyframe-tracks-plan.md. Likely Mintlify schema/nav-ref miss. |
Net
Ready to merge once: (1) Format re-run, (2) golden snapshot refresh + acknowledge the -${group} ID-format wire impact for cross-repo consumers, (3) the 3 NEW studio test failures dealt with — especially the manualEditingAvailability.test.ts accidental flag-default flip which silently re-enables a feature that was just gated off in #1341, (4) Vai's RC4 fromTo resolution, (5) Docs validation. Substance is genuinely better than R1.
— Review by Rames D Jusso
miguel-heygen
force-pushed
the
fix/keyframes-foundation
branch
11 times, most recently
from
c8ce770 to
28ade34
Compare
Replaces the bundled percentage-keyframe model with per-property-group tweens. Each studio operation creates/edits only its own group's tween: drag → position, resize → scale/size, rotate → rotation. Includes off-screen element indicators with drag-to-move, unclipped overlay for off-screen interaction, gesture recording replacement, and all 6 root cause foundation fixes from the keyframe trace investigation.
miguel-heygen
force-pushed
the
fix/keyframes-foundation
branch
from
28ade34 to
eb58848
Compare
3 participants
Summary
Addresses the 6 root causes identified in the keyframe trace investigation (#1314 follow-up) plus 25 additional bug fixes discovered during live testing with
sdk-test.html.Parser foundations
+=,-=,<,>) and computeresolvedStart, fixing tween discovery, diamond placement, and parse ordertweenPercentagethreading through cache/UI for correct delete/updatetimeline({defaults})and inherit ease/duration onto child tweensresolvedFromValues, don't replace)changed: falseon no-op mutations, client rolls back optimistic state-from-→-to-fallback) on all mutation functions includingremoveAnimationFromScriptKeyframe operations
readAllAnimatedPropertiesfilters out untargeted GSAP props (scaleZ, brightness, etc.)Soft reload, UX, recording
clearProps: "all"after timeline kill, globalTimeline sweep,__proxiedskip, success verificationTest plan
sdk-test.html(chained from() tweens with relative positions and inherited ease)