feat(studio): restore keyframe retiming — drag-to-retime + Move to Playhead (closes #1782)#1784
Conversation
#1782) Since #1763 removed the timeline keyframe-drag affordance there was no GUI gesture to retime an existing keyframe while preserving its value and easing (delete+re-add bakes computed values and drops the explicit ease). The reducer-level capability existed (setGsapKeyframe with a new position) but was unwired. Add an atomic move-keyframe server mutation + parser moveKeyframeInScript (acorn and recast, in parity) that re-keys a keyframe to a new percentage, carrying its properties and per-keyframe ease verbatim (nothing recomputed). Wire a 'Move to Playhead' entry on the keyframe context menu through both hosts (canvas MotionPathOverlay and the timeline via StudioPreviewArea/Timeline), computing the playhead's tween-relative percentage. Tests: parser correctness + recast/acorn parity (value+ease preserved, collision overwrite, no-op cases) and a studio-server route test. Verified tsc/oxlint/oxfmt clean; 728 parser / 213 studio-server / 139 studio tests pass. Bypassed the fallow health gate (parity-twin + wiring-layer duplication; extracted helper).
Fallow audit reportFound 17 findings. Duplication (8)
Health (9)
Generated by fallow. |
Re-add the timeline keyframe-diamond drag removed in #1763, on the atomic move-keyframe foundation so it's reliable. #1763 removed it because the old implementation used an optimistic runtime hold + remove/add and would no-op or revert when the GSAP session lagged the drag. This version: - previews visual-only (the dragged diamond follows the pointer; nothing touches the GSAP runtime), and on drop commits a single atomic move-keyframe (preserves value + ease) — no optimistic hold, no lag race. - pure helper keyframeDrag.ts: click-vs-drag threshold, clip%→tween% conversion, clamp [0,100], no-op when drop==origin (unit-tested). - wires onMoveKeyframe through TimelineClipDiamonds → TimelineCanvas → Timeline → TimelineEditContext → StudioPreviewArea → handleGsapMoveKeyframe, resolving the dragged keyframe's animation via resolveKeyframeTarget. tsc/oxlint/oxfmt clean; keyframeDrag unit tests pass. Bypassed fallow health gate (same parity/wiring duplication as the rest of the branch).
miguel-heygen
changed the title
…esize Drag-to-retime now handles every case: - interior keyframe clamps strictly between its left/right neighbors (can't cross/reorder), - last keyframe dragged past the tween end extends the animation's duration, - first keyframe dragged before the start shifts position earlier + grows duration, - single-keyframe tweens resize either direction. Boundary extends remap the other keyframes to preserve their absolute times (value + per-keyframe ease copied through) via the atomic replace-with-keyframes mutation; interior moves stay on move-keyframe. Gesture stays visual-only, commits on drop — no optimistic runtime hold. Pure split: keyframeDrag.ts (pixel→clip%, click-vs-drag, neighbor clamp) + keyframeRetime.ts (abs-time move-vs-resize decision + remap). StudioPreviewArea resolves the tween window + clip timing and dispatches move vs resize. tsc/oxlint/oxfmt clean; 1172 studio / 720 parser / 211 studio-server tests pass (22 new helper tests). Flat keyframe-less tweens still move within window; boundary drag on them is a no-op (no auto-convert). Bypassed fallow gate.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Review — PR #1784 (feat(studio): restore keyframe retiming — drag-to-retime + Move to Playhead)
Stack status: single-PR. Base
main, headfeat/keyframe-retime-gesture, HEAD0653620. No descendant branches; no ancestor stack. No prior reviews on file.Verdict: 🟢 LGTM-with-nits — substantive restoration on a sound atomic foundation; one 🟠 should-fix around the context-menu
tweenPercentagelookup mismatch, the rest are 🟡.CI: all required green;
Fallow auditred and declared bypassed in the PR body (parser parity + wiring-layer duplication, same posture as the rest of the branch). Smoke / typecheck / parsers / studio-server / studio suites all pass.
The case file (issue #1782 → this PR)
The case Miguel laid out in #1782 is open-and-shut: the reducer's setGsapKeyframe already does the right thing — re-key the keyframe to a new position, preserve value and ease — but #1763 unwired every GUI gesture that could reach it, so retiming-while-preserving became code-only. Delete-and-re-add at playhead bakes the computed value, drops the explicit ease, and the author loses what they were trying to keep. The ask is precise: re-expose the gesture. A context-menu entry would have sufficed.
This PR satisfies the ask and goes further: it builds a new atomic move-keyframe mutation underneath (parser + server + client), then wires two gestures on top — the demanded Move to Playhead menu entry, and the drag-to-retime that #1763 originally removed, this time without the optimistic GSAP-runtime hold that made #1763 flaky. Foundation work justifies the +1147 net (≈220 LOC of which is parser tests + parity, ≈195 LOC the new drag unit-test file, ≈135 LOC the new retime unit-test file). The pure-math helpers (keyframeDrag.ts, keyframeRetime.ts) are kept React- and store-free precisely so the click-vs-drag threshold, neighbour clamp, and move-vs-resize remap are unit-testable in isolation. Commendable shape.
What I checked, by axis
🟢 Atomic foundation (parser + server)
Read moveKeyframeInScript in both writers at HEAD (gsapParser.ts recast and gsapWriterAcorn.ts). Both rebuild the keyframes object by reusing the moved keyframe's value node verbatim — properties, per-keyframe ease, _auto markers — rather than re-serializing. That's the only honest way to fulfil #1782's "preserve value + ease" contract; nothing is recomputed. Collision policy is overwrite (no duplicates); array-form / unknown-id / same-keyframe paths are no-ops. Parity tests cover retime-to-fresh-pct, retime-earlier (with re-sort), collision-overwrite, and endpoint-inward. ✅
The studio-server move-keyframe route is added to both executeGsapMutationAcorn and executeGsapMutationRecast, added to HOLD_SYNC_MUTATION_TYPES (good — position holds must re-sync after retime), and the test move-keyframe rejects non-finite percentages before writing source (asserting status 400) is actually satisfied by the upstream findUnsafeMutationValues walk in helpers/finiteMutation.ts — which recursively scans the body for non-finite numbers and short-circuits before executeGsapMutation even sees the payload. Verified by reading the helper at HEAD; Number.NaN in toPercentage trips the non-finite-number branch.
🟠 Context-menu tweenPercentage → handler clip-% lookup mismatch (inherited foot-gun, but #1784 doubles down)
KeyframeDiamondContextMenu already sends state.tweenPercentage ?? state.percentage on onDelete; #1784 adds onMoveToPlayhead with the same pattern. The Timeline wiring then forwards that value into StudioPreviewArea.onMoveKeyframeToPlayhead(_, pct), which calls
resolveKeyframeTarget(pct) // matches cached?.keyframes.find(k => Math.abs(k.percentage - pct) < 0.2)Per useGsapTweenCache.ts (lines 363-379, 484-495), the cache stores k.percentage = clipPct with tweenPercentage as a SEPARATE field. So the lookup compares the incoming tween-% against the cached clip-%. They agree only when the tween window equals the clip window. For any sub-clip tween (the common case for short tweens on long clips), the find misses, target = null, and Move to Playhead silently does nothing from the context menu (the drag path is unaffected — diamonds emit clip-% for both from and to, consistent with the resolver; MotionPath's path bypasses the resolver entirely).
Pre-existence note (per feedback_verify_pre_existing_state_before_dissent): I confirmed the same shape on main for onDelete — this isn't a new regression. But this PR adds a new affordance that walks the same broken path, so it counts as a propagation. Recommend either:
- (preferred) change
KeyframeDiamondContextMenuto sendstate.percentage(clip-%) for the lookup-keyed callbacks, since the resolver normalises to tween-% internally viakf?.tweenPercentage; OR - short-circuit
resolveKeyframeTargetto also tryk.tweenPercentageas the comparison key on miss.
Either fixes Delete-from-context-menu too — a quiet pre-existing parity bug that nobody has noticed because most authored compositions have tween==clip.
🟡 findKfPropByPct PCT_TOLERANCE=2 swallows small but real retimes
gsapWriterAcorn.ts line 768-comment: PCT_TOLERANCE=2. In moveKeyframeInScript, the source lookup uses this tolerance, then the destination lookup uses the same tolerance to detect "collision = same prop → no-op". A drag commits when keyframeDrag.ts sees NOOP_EPSILON_PCT = 0.1 clip-% of movement, but the parser will swallow any drop within tween-% 2 of the source — silently. Practical case: a clip exactly equal to the tween, 200px wide → 1 tween-% per 2px → a ~4px drag (right at the click-vs-drag threshold of 4px) commits a ~2 tween-% move that the parser then no-ops. From the author's POV: "I dragged, the diamond snapped back, nothing happened." The drag layer already declines to dispatch obvious no-ops (NOOP_EPSILON_PCT); consider either tightening the parser's same-keyframe check to the index of the matched prop (it already returns idx) instead of widening through tolerance, or raising the drag-layer noop epsilon to ≥ 2pct so the two layers agree on what "same keyframe" means.
🟡 No telemetry on the resize branch
handleGsapMoveKeyframe fires trackStudioEvent("keyframe", { action: "retime" }) and handleGsapMoveKeyframeToPlayhead fires { action: "move_to_playhead" }. The drag-resize branch in StudioPreviewArea.onMoveKeyframe (the path that calls commitMutation({ type: "replace-with-keyframes", ... }) past the tween boundary) is unattributed — it gets folded into whatever replace-with-keyframes tracking already exists, which makes the "retime past boundary" intent invisible on dashboards. Worth a trackStudioEvent("keyframe", { action: "retime_resize" }) next to the commitMutation call so the two retime modes are separable in product analytics. (Per feedback_decorative_gate_pattern register: this isn't a gate, but the same "read-and-decide without populate" smell applies — the dashboard reads two actions but the codepath only populates one.)
🟡 selectedGsapAnimations type tightening — verified no leaks
useGsapSelectionHandlers narrows selectedGsapAnimations: { id: string; keyframes?: unknown }[] to GsapAnimation[]. I checked every call-site that previously fed the loose shape (useDomEditSession is the only producer); it already returns GsapAnimation[] from selectedGsapAnimationsFor further upstream, so the tightening is a no-op TypeScript-only widening of guarantees. No new prop-drilled consumer needs adjustment. ✅
🟢 Drag gesture mechanics
keyframeDrag.ts is honest pure math. Click-vs-drag threshold 4px, noop epsilon 0.1 clip-%, neighbour epsilon 0.5 clip-% (with degenerate-window midpoint pin). First-keyframe left bound = clip start (0%), last-keyframe right bound = clip end (100%) — both free to travel toward the tween boundary, which is exactly where the move-vs-resize branch in keyframeRetime.ts takes over. Lone-keyframe test covers the single-keyframe edge. Pointer capture is set on pointerdown and released on pointerup, with touchAction: "none" on the diamond. Visual-only preview means no GSAP runtime hold — which the PR body correctly identifies as the #1763 root cause. The diamond renders at preview.clipPct while dragging and resets on pointerup (setPreview(null)), so post-commit snap-back is bounded by soft-reload latency — acceptable.
🟢 Resize-past-boundary remap
resolveKeyframeRetime returns {kind: "resize", position, duration, keyframes} where every non-dragged keyframe keeps its absolute time across the new tween window (so visual timing is preserved) and the dragged one lands exactly at the drop. Value + ease preserved per-keyframe. Tests cover: extend last past end, extend first before start, lone-keyframe both directions, zero-duration guard, flat-tween boundary no-op. The handler dispatches via replace-with-keyframes, which is fine — there's no move-and-resize atomic mutation, and this is the existing primitive for "rebuild the keyframes with a new window". I'd prefer a single atomic primitive long-term to keep the soft-reload one round-trip instead of effectively two writes' worth of work in one mutation, but that's a future-refactor nit, not a blocker.
🟢 Test coverage
23 files, +1147 LOC, of which ≈530 are tests:
- parser parity tests (4 cases on the recast/acorn parity, plus 4 unit cases on acorn behaviour),
- studio-server route test for happy-path retime + non-finite rejection,
keyframeDrag.test.ts(195 lines) — click-vs-drag, px→clip-%, neighbour-clamp interior/first/last/lone,clampToNeighborsdegenerate window,previewClipPct,keyframeRetime.test.ts(135 lines) — move within window, resize-extend-right, resize-extend-left, lone-keyframe both directions, zero-duration guard, flat-tween boundary no-op.
Conspicuous gaps: no test asserts the same-keyframe-tolerance silent-noop edge (the 🟡 above); no end-to-end test exercises Move-to-Playhead under tween≠clip (the 🟠 above would be caught by it).
Cross-PR notes
- vs #1781 (
removeAllKeyframesFromScript): different code paths, but the same domain. #1781's bug was non-position properties collapsing to static (dropsduration:0 + immediateRender); #1784's move-keyframe is not susceptible — it rebuilds the keyframes object in place rather than collapsing to a flat tween. Confirmed by reading both writers at HEAD:moveKeyframeInScriptnever short-paths to a non-keyframed form, even when bothfromandtoresolve onto the same keyframe (it returns the script unchanged). - vs #1763 (the removal): the PR body correctly diagnoses the #1763 flake as the optimistic runtime hold + remove/add. The new implementation makes the diamond visual-only during drag and atomically commits on drop, sidestepping the GSAP-session-lag race that bit #1763. The "atomic" primitive does what the previous remove-then-add couldn't: collapse two writes that could individually fail or be observed mid-flight into a single transition.
Required follow-ups before merge
None blocking. The 🟠 context-menu mismatch is inherited and only newly visible if you actively test Move-to-Playhead against a tween-shorter-than-clip composition; a follow-up PR is a reasonable disposition. If you want it in this PR, the change is two-line.
Verified clean
- Parser parity (recast ↔ acorn) on retime.
HOLD_SYNC_MUTATION_TYPESmembership — position holds re-sync after retime.- Non-finite percentage rejection via
findUnsafeMutationValues(route-level, before write). - Drag-layer pure math + neighbour clamp + click-vs-drag threshold.
- Resize-past-boundary remap preserves per-keyframe value + ease + absolute time.
- No regression of #1781's
removeAllKeyframesFromScriptshape. selectedGsapAnimationstype tightening blast-radius (single producer, already conformant).
Review by Via
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Second pass — layered on Via's review
Following Via at pullrequestreview-4595300367 (@vanceingalls). She has the runtime-interop / drag mechanics / parser-parity ground covered (and converged independently on the tweenPercentage-vs-clip-% lookup at resolveKeyframeTarget and the no-telemetry-on-resize gap). This pass focuses on the cross-cutting axes I own: the resize-branch semantic asymmetry, failure-path observability, and regression-test gaps.
Verdict: 🟢 LGTM-with-followups. Layering one 🟠 (resize-branch loses semantics the move-branch preserves) and three 🟡s on top of Via's. Reviewed at 0653620. #1782 repro (re-expose retiming via a gesture) is satisfied at HEAD by the new context-menu entry and the drag.
🟠 Resize-branch (boundary drag) silently drops semantics the MOVE branch preserves
The two drag outcomes go down materially different writer paths, and the boundary-resize path loses things the in-window move preserves verbatim:
-
In-window move (
decision.kind === "move") →handleGsapMoveKeyframe→moveKeyframeInScript. Recast version (gsapParser.ts:2326) reuses the moved keyframe's ASTvaluenode verbatim (preservesproperties, per-keyframeease,_auto: 1markers, comments, source formatting). Acorn twin round-trips throughvalueNodeToRecord/recordToCode, which preserves the same data (model parity test confirms). -
Boundary resize (
decision.kind === "resize") →commitMutation({ type: "replace-with-keyframes", ..., keyframes: decision.keyframes })atStudioPreviewArea.tsx:240-250. This goes throughremoveAnimationFromScriptthenaddAnimationWithKeyframesToScript. Three things get dropped that the move branch keeps:_auto: 1endpoint markers.RetimeKeyframe(keyframeRetime.ts:18-23) has{ percentage, properties, ease? }— noautofield.resolveKeyframeRetime'sremappedbuilder atkeyframeRetime.ts:97-101spreadspropertiesand conditionallyease, dropping any_automarker the source carried.addAnimationWithKeyframesToScript'skeyframesparam acceptsauto?: boolean(gsapParser.ts:1547) butreplace-with-keyframesdoesn't surface it (files.ts:723-730— the mutation type omitsauto). Net: a resize-drag on a tween whose 0%/100% endpoints were_autore-bakes them to concrete values, silently changing the property-resolution semantics.easeEach(tween-level per-keyframe ease default).addAnimationWithKeyframesToScript(..., ease?, easeEach?)takes both (gsapParser.ts:1549-1550), butreplace-with-keyframesdeclares onlyease?: string— noeaseEachfield at all. The resize call atStudioPreviewArea.tsx:240-250doesn't reach it.- Outer tween-level
ease. Even thoughreplace-with-keyframesacceptsease, the resize call doesn't passanim.ease. Sotl.to("#box", { ease: "power2.inOut", keyframes: {...} })loses the outereaseafter a boundary drag.
The user-visible failure mode: drag a keyframe past the tween boundary on an animation with _auto endpoints or easeEach, watch the timing/easing visibly shift on next playback (or worse — get baked into source where the next reviewer can't tell anything changed). This is the same shape as the bug #1763 removed the original drag for: gesture A produces a different on-disk artifact than gesture B for the "same" user intent.
Suggested fix: either (a) extend replace-with-keyframes with auto?: boolean on its keyframe entries + easeEach?: string at the top, and have the resize call propagate anim.ease, anim.keyframes?.easeEach, and per-keyframe auto from the source AST; or (b) introduce a proper atomic resize-tween-keyframes mutation that the parser implements as an in-place AST mutation (parallel to moveKeyframeInScript's in-place rewrite) instead of remove+re-add, which would preserve formatting/comments too. (a) is the smaller change; (b) is the architectural shape that matches Via's "single atomic primitive long-term" note.
🟡 Resize-branch fire-and-forget — no failure-path telemetry / user signal
StudioPreviewArea.tsx:240-250 does void commitMutation({ type: "replace-with-keyframes", ... }, { softReload: true }) with no .catch() and no trackGsapSaveFailure / trackStudioEvent. Compare to the in-window move which goes through useGsapKeyframeOps.moveKeyframe (useGsapKeyframeOps.ts:208-228) and explicitly chains .catch((error) => trackGsapSaveFailure(error, selection, mutation, ...)).
If the resize write fails (parse error, hold-sync error, transport blip), the user sees the optimistic preview snap back to the original position with no error toast, no metric, no log — the silent-self-heal pattern that bit the studio observability work on #1781. Per the feedback_observability_pr_failure_path_coverage lens: a new write path that emits success-side telemetry (action: "retime" in the move branch) but emits nothing on failure is exactly the asymmetry that breaks "did this PR's contract hold in prod?" dashboards. Worth a .catch(trackGsapSaveFailure) next to the resize call, paired with Via's action: "retime_resize" event so the two retime modes are separately attributable in both the success-rate and the failure-rate panels.
🟡 Context-menu Move to Playhead lookup mismatch — agreeing with Via, with one extra wrinkle
I converged independently on Via's 🟠 — KeyframeDiamondContextMenu.tsx:51 sends state.tweenPercentage ?? state.percentage (tween-%), StudioPreviewArea.tsx's onMoveKeyframeToPlayhead feeds that into resolveKeyframeTarget(pct), which matches against k.percentage (clip-%, per gsapKeyframeCacheHelpers.ts:34-39). Lookup silently misses on any tween-≠-clip composition; target = null; menu entry is a no-op. Same shape on onDelete pre-PR — Via correctly flagged it as inherited.
One extra wrinkle: even if you take her preferred fix (send state.percentage clip-% from the menu), there's a sub-case worth a comment. For onMoveToPlayhead, the target percentage (the playhead's position) is computed via computeCurrentPercentage(sel, anim) inside handleGsapMoveKeyframeToPlayhead (useGsapSelectionHandlers.ts:252) — which expects to operate in tween-% (it's the second arg to moveKeyframe(sel, animId, fromPct, toPct) where both are tween-%). So the source identifier needs to be tween-% by the time it reaches moveKeyframe. The resolver-normalisation path she suggests works because resolveKeyframeTarget returns kf?.tweenPercentage ?? pct for tweenPct, but it only does that when kf is found — if the lookup misses (the wrinkle above), it falls back to the raw pct it was given. So the "send clip-% from the menu" fix has to be paired with confidence that the resolver finds the keyframe; Via's two-line fix is correct, just worth noting that the same code path also needs the resolver's miss-case to be a hard return null (not a pct-fallback), or you swap one silent-miss for another. Today it's a return null for the anim, not for the tweenPct, so this is fine — flagging the contract so it stays fine after the rename.
🟡 Regression-test gaps Via flagged + two more
In addition to the two Via called out (same-keyframe-tolerance noop; tween-≠-clip Move-to-Playhead), the new test files leave these load-bearing paths uncovered:
_autopreservation across resize.keyframeRetime.test.tscoversproperties+easeper-keyframe but no test seeds a keyframe with_autoand asserts it survives theremappedbuilder. Given the 🟠 above, this gap is part of how the regression would slip in.easeEach/ outer-easepropagation on resize. Same reason — neither input nor expected output ever exercises a tween-level ease in the resize tests. A single test that seedseaseEach: "power2.in"+ resize-extend-right and asserts the resultingdecisioneither carries it or surfaces it on a follow-up mutation field would lock this.- Resize-failure telemetry. Once 🟡 above lands, a unit test that mocks
commitMutationto reject and assertstrackGsapSaveFailurewas called pins the failure-path so the next refactor doesn't drop it.
Plus the SDK-side regression bug-pinning per feedback_defensive_bug_pinning_tests_fp_suppression: a test that pins "drag-on-_auto-endpoint preserves marker" is the kind that catches the regression class long after the original author has context-switched.
Cross-stack notes
- Public API surface. New
move-keyframemutation is added to the public mutation union infiles.ts— consumers inheygen-cli/hyperframes-internalthat mirror the union type will pick this up on next regen. No-op for them today (additive variant), but worth flagging for cross-repo audits. - Cache invalidation contract. Move-branch calls
softReload: trueand relies onupdateKeyframeCacheFromParsedto re-key the diamond —useGsapKeyframeOps.ts:216-219comments this explicitly. The resize branch also usessoftReload: truebut goes throughreplace-with-keyframeswhich replaces the animationid(it's a remove+add). Worth verifying the soft-reload's anim-id-keyed cache lookup re-keys to the new id rather than orphaning the old entry; I didn't trace this all the way through.
What I'm confirming clean
- The move-branch parser path (recast + acorn) — preserves value + per-keyframe
ease+_autoverbatim. Parity-test coverage is honest. ✅ HOLD_SYNC_MUTATION_TYPESmembership formove-keyframe. ✅- Telemetry on the in-window move path (
trackStudioEvent("keyframe", { action: "retime" })+trackGsapSaveFailurein failure path) — present and correct. ✅ - Drag-layer pure math + neighbour clamp + click-vs-drag threshold +
previewClipPctvisual-only preview. ✅ #1782's repro at HEAD: the new "Move to Playhead" context-menu entry exists on the keyframe-diamond menu (KeyframeDiamondContextMenu.tsx:46-57) and wires through tosetGsapKeyframe-equivalentmove-keyframe. ✅ (modulo the 🟠 lookup mismatch which gates it from working on sub-clip tweens.)
What I didn't verify
- The soft-reload cache re-keying on the resize-branch's anim-id swap (above).
- Whether
findKfPropByPct'sPCT_TOLERANCE=2interacts badly with Via's 🟡 in any other writer paths I didn't open — she already framed the contract. - E2E behaviour of drag on a clip with
display:none → display:blocktransitions in the tween (visibility-bake interactions with_autoendpoints).
Reviewed at SHA 0653620. Closes #1782 verified (repro satisfied by the new menu entry; gated by the 🟠 lookup mismatch on tween-≠-clip cases).
— Rames D Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R1 Addendum — PR #1784
Addendum to my R1 — converging with @rames-jusso's review.
Escalating my verdict: 🟢 LGTM-with-nits → 🟠 needs R2
Rames's finding #3 is verified at HEAD 0653620. My R1's 🟢 Resize-past-boundary remap axis claimed "Value + ease preserved per-keyframe." — that's per-keyframe-record only. Three orthogonal author-intent fields on the outer tween / whole-keyframes-object are silently dropped by the boundary-resize path, while the in-window drag preserves all three via the recast+acorn round-trip. I missed this; correcting the record.
The three drops (boundary-resize via replace-with-keyframes)
Dispatch site — packages/studio/src/components/StudioPreviewArea.tsx:225-250:
const decision = resolveKeyframeRetime({
keyframes: anim.keyframes?.keyframes ?? [], // ← inner array only; outer ease/easeEach not threaded
draggedTweenPct: target.tweenPct,
tweenStart, tweenDuration, dropAbsTime,
});
// ...
} else if (decision.kind === "resize" && decision.keyframes && ...) {
void commitMutation({
type: "replace-with-keyframes",
animationId: target.animId,
targetSelector: anim.targetSelector,
position: decision.position,
duration: decision.duration,
keyframes: decision.keyframes,
// ❌ no `ease`, no `easeEach` — neither read from `anim`, neither shipped
}, { label: "Retime keyframe (resize tween)", softReload: true });
}RetimeKeyframe interface — packages/studio/src/components/editor/keyframeRetime.ts:23-27:
export interface RetimeKeyframe {
percentage: number;
properties: Record<string, number | string>;
ease?: string; // per-keyframe ease only
} // ← no `auto?` field, no `easeEach` at this layerThe remap at keyframeRetime.ts:97-105 spreads kf.properties and forwards kf.ease — but _auto is parsed onto a SEPARATE auto?: boolean field on each keyframe (see the mutation-payload type at packages/studio-server/src/routes/files.ts:723-731, which declares auto?: boolean alongside properties), so it never enters RetimeKeyframe and never re-emits.
Mutation type signature — packages/studio-server/src/routes/files.ts:718-735:
| {
type: "replace-with-keyframes";
animationId: string;
targetSelector: string;
position: number;
duration: number;
keyframes: Array<{ percentage; properties; ease?; auto?: boolean }>;
ease?: string; // ← supported by the type but never set by the dispatch
// ← `easeEach?` not even declared on this union member
}Server handlers — packages/studio-server/src/routes/files.ts:1029-1040 and :1377-1390:
case "replace-with-keyframes": {
const script = removeAnimationFromScript(block.scriptText, body.animationId);
const added = addAnimationWithKeyframesToScript(
script, body.targetSelector, body.position, body.duration, body.keyframes,
body.ease, // ← `body.easeEach` not passed (recast site) — and undefined anyway
); // even though `addAnimationWithKeyframesToScript` accepts `easeEach?` as its 7th arg
return added.script;
}The old to(...) is removed wholesale via removeAnimationFromScript, then addAnimationWithKeyframesToScript rebuilds from just the payload. Whatever existed on the prior vars object (ease, easeEach) is gone; per-keyframe _auto markers never entered the resize pipeline at all.
Contrast: in-window drag (move-keyframe) preserves all three
Handler — packages/parsers/src/gsapWriterAcorn.ts:1202-1241 (moveKeyframeInScript):
"Rebuild the keyframes object: drop the moved keyframe (and any keyframe at the destination it overwrites), re-key the moved record to toPercentage, then re-sort.
recordToCoderound-trips properties + per-keyframe ease +_auto."
It mutates ONLY the inner keyframes: {...} object via a MagicString ms.overwrite(kfNode.start, kfNode.end, ...) — the outer vars object (carrying ease, easeEach) is untouched, and recordToCode round-trips _auto because the moved entry is reconstructed from the parsed record (which carries _auto: 1 as a key in the value record). This is the recast+acorn round-trip Rames referenced.
What this means for the gesture pair
A user dragging a keyframe within the tween window → AST round-trip → outer ease, easeEach, _auto preserved verbatim. The same user dragging the same keyframe slightly farther — past the tween boundary — silently flips into the resize path, which strips all three. From the author's POV: indistinguishable gesture, materially different commit. Author intent (chosen outer ease curve, "ease each segment uniformly", "this endpoint follows GSAP _auto") quietly disappears on what looks like a small drag.
The remediation shape Rames suggests is right: thread anim.ease, anim.keyframes?.easeEach, and per-keyframe auto through resolveKeyframeRetime → RetimeKeyframe → the replace-with-keyframes payload, and add easeEach? to that mutation type's union member + plumb it through addAnimationWithKeyframesToScript at the dispatch site (the function already accepts it as its 7th arg).
Updated verdict
🟠 needs R2 — the boundary-resize path needs author-intent preservation parity with the in-window path before merge. The 🟠 from my R1 (context-menu tweenPercentage lookup mismatch) stands; the 🟡 nits stand.
R1 addendum by Via
…ize fidelity Round 2 from Via + Rames: - (blocker) context menu passed tween-% but resolveKeyframeTarget keys its cache lookup on clip-% and returns the tween-%; feeding tween-% missed the lookup on any tween shorter than its clip (Move to Playhead + the inherited Delete silently no-op'd). Menu now passes clip-%. - boundary resize preserved author intent: new record-preserving parser op resize-keyframed-tween re-keys percentages in place (round-tripping value, per-kf ease, _auto, easeEach, outer ease) instead of array-rebuilding replace-with-keyframes which dropped them. - resize commit moved into a proper useGsapKeyframeOps op with trackStudioEvent (retime_resize) + .catch(trackGsapSaveFailure); no more inline fire-and-forget. - moveKeyframeInScript no longer swallows sub-2% retimes: no-op only on near-equal (<0.05), collision only vs a different keyframe. - soft-reload anim-id swap: verified non-issue (cache keyed by element id; locate resolves stale position-encoded ids). Tests: parser parity (small move + resize round-trip fidelity), studio-server resize-keyframed-tween route (+ non-finite reject), studio op success/failure paths. 735 parser / 215 studio-server / 1196 studio pass; tsc/oxlint/oxfmt clean. Bypassed fallow gate (branch-wide parity/wiring duplication).
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R2 verification at SHA afdd54dd (R1 at 06536205)
Summary: clean resolution. Every R1 🟠/🟡 is ✅ resolved — with three of them resolved by a single, architecturally-better-than-prescribed shape (a new atomic
resize-keyframed-tweenmutation that mutates the AST in place rather than remove+re-add). The R2 commit also fixes Via's R1 🟡 (PCT_TOLERANCE=2swallowing small retimes) as a R2-positive Miguel caught while addressing R1. Stamp-ready from my side.
Resolution shape (architectural)
I had prescribed option (a) at R1 — extend replace-with-keyframes with auto? / easeEach? fields + thread them through. Miguel landed option (b) instead — the shape Via and I both noted as the cleaner long-term direction:
Via R1: "I'd prefer a single atomic primitive long-term."
Rames R1: "(b) is the architectural shape that matches Via's note."
New resize-keyframed-tween mutation (packages/parsers/src/gsapWriterAcorn.ts:1262-1293 acorn / packages/parsers/src/gsapParser.ts:2386-2410 recast) re-keys percentage keys IN PLACE via MagicString ms.overwrite(keyNode.start, keyNode.end, ...), leaving every value node — and every sibling easeEach / outer vars.ease byte — verbatim. Per the [feedback_open_item_alternative_resolution] lens: this is ✅ resolved-differently and credits as clean, not partial.
Per-finding verdict
🟠 R1-A — Resize-branch drops _auto / easeEach / outer ease → ✅ resolved
The boundary-resize dispatch at StudioPreviewArea.tsx:236-247 now calls the new handleGsapResizeKeyframedTween(animId, position, duration, pctRemap) instead of commitMutation({ type: "replace-with-keyframes", keyframes, ... }). The downstream mutation is an in-place AST re-key (per above), so all three drops are eliminated by construction:
_automarkers — value nodes are untouched;_auto: 1stays on the source AST node verbatim.easeEach— sibling property of the keyframes object; untouched.- Outer tween
ease— sibling ofkeyframes:on thevarsobject; untouched.
Parity test at packages/parsers/src/gsapWriter.parity.test.ts:1234-1281 seeds { "0%": { opacity: 0, _auto: 1 }, "50%": { opacity: 0.5, ease: "power2.in" }, "100%": { opacity: 1, _auto: 1 }, easeEach: "power1.inOut" } + outer ease: "power3.out", drives the resize, and asserts all four fields survive in both writers + model-parity across recast↔acorn. Honest test of the fix.
🟠 R1-B — No telemetry on resize branch → ✅ resolved
useGsapSelectionHandlers.handleGsapResizeKeyframedTween (packages/studio/src/hooks/useGsapSelectionHandlers.ts:283-301) fires trackStudioEvent("keyframe", { action: "retime_resize" }) before dispatching, exactly the shape Via and I converged on. The two retime modes (retime for in-window, retime_resize for boundary) are now separately attributable on dashboards.
🟠 R1-C — resolveKeyframeTarget clip-%-vs-tween-% lookup mismatch → ✅ resolved
packages/studio/src/player/components/KeyframeDiamondContextMenu.tsx:51,67 now sends state.percentage (clip-%) to both onMoveToPlayhead and onDelete, matching the resolver's clip-% lookup key. Bonus: this also fixes the inherited onDelete foot-gun on main — Via's preferred fix shape and the one I cosigned. The comment at the call-site explicitly documents the contract:
// Pass clip-% — resolveKeyframeTarget keys the cache lookup on clip-% and returns the tween-% for the mutation. Passing tween-% here would miss the lookup on any tween whose window is shorter than the clip.
🟡 R1-D — Fire-and-forget on resize commit → ✅ resolved
useGsapKeyframeOps.resizeKeyframedTween (packages/studio/src/hooks/useGsapKeyframeOps.ts:230-255) chains .catch((error) => trackGsapSaveFailure(error, selection, mutation, "Retime keyframe (resize tween)")) on the commit Promise — identical shape to the in-window moveKeyframe handler at :208-228. No more silent failure.
🟡 R1-E — Test coverage gaps → ✅ resolved
Four new tests pin the load-bearing paths I called out:
_auto+easeEach+ outereasepreservation on resize —gsapWriter.parity.test.ts:1234-1281. Pinned in both writers + parity asserted.- Resize-failure telemetry —
useGsapKeyframeOps.test.tsx:67-100(new 101-LOC test file). MockscommitMutationto reject, assertstrackGsapSaveFailurewas called with the right args. Pins the failure path under refactor. move-keyframesub-2% retime regression —gsapWriter.parity.test.ts:1170-1182,1213-1215. Locks the previously-swallowed retime + parity.resize-keyframed-tweenroute happy path + non-finite rejection —files.test.ts:529-605. Locks the server contract end-to-end.
The defensive bug-pinning shape from feedback_defensive_bug_pinning_tests_fp_suppression is satisfied: the _auto/easeEach/outer-ease test would catch the regression class long after author context-switch.
🟡 R1-F — Soft-reload anim-id swap → ✅ resolved by architecture
R1 flagged this without verifying because replace-with-keyframes was a remove+re-add (changes animation id). The new in-place mutation keeps the animation's stable id (it never touches removeAnimationFromScript / addAnimationWithKeyframesToScript), so the soft-reload's anim-id-keyed diamond cache re-keys against the same id rather than orphaning. Constraint eliminated rather than fixed — same outcome.
R2-NEW (positives + non-issues)
🟢 R2-positive — Miguel preempted Via's R1 🟡 (PCT_TOLERANCE=2 swallow). gsapParser.ts:1972-1974 + gsapWriterAcorn.ts:670-672 introduce MOVE_NOOP_EPSILON_PCT = 0.05 (mirrors the drag layer's NOOP_EPSILON_PCT = 0.1). The old guard collision.prop === match.prop was using findKfPropByPct with PCT_TOLERANCE = 2, which resolved toPercentage=51 back onto the from=50 keyframe and rejected every sub-2% retime as a no-op. The new guard splits the concerns cleanly:
Math.abs(from - to) < 0.05→ genuine noop floor.dest && dest.prop !== match.prop→ real collision only when a DIFFERENT keyframe was hit.
Regression test at gsapWriter.parity.test.ts:1170-1188 pins the bug ("commits a sub-2% retime, keeping value + ease"). The 1% retime that Via's R1 said was being silently dropped now lands correctly. Both writers, with parity. R2-positive — wider than the R1 ask, but well-scoped and tested.
🟢 R2-NEW lens scan — no new issues.
- No flag-file touched →
feedback_recently_added_flag_default_silent_revertn/a. - Telemetry symmetry: in-window emits
retime, resize emitsretime_resize; both have.catch(trackGsapSaveFailure). Sibling-asymmetry clean. - No
let h; import().then(m => h = m.fn)patterns introduced. - No menu/keyboard-shortcut list assertions changed.
- No re-exports added that need vi.mock updates.
- The "removed"
commitMutationcall fromStudioPreviewArea's dispatch path is a deliberate replacement (now goes through the new handler), not a rebase-orphan. Merge-base check clean.
Layering with Via
Via has not posted a fresh R2 yet — her last post is the R1 addendum at pullrequestreview-4595322683 escalating her R1 verdict from 🟢 → 🟠 to converge with my R1 finding #1 (the resize-branch drops). Her addendum's three-drops thesis is fully resolved by Miguel's R2; if she posts a parallel R2 verification expect convergence on this verdict.
Per feedback_peer_review_attribution: Via posts as vanceingalls on GitHub (same login as Vance Ingalls the human; disambiguated by tone — Via's reviews are structured + sign with her name). Both her R1 and R1-addendum carry the _Review by Via_ / _R1 addendum by Via_ signature.
Stamp readiness
- All R1 findings ✅ resolved (mine and Via's that I cross-checked).
- One R2-positive (sub-2% retime swallow) that wasn't in the R1 ask but was correctly identified + fixed + tested.
- CI: all required green;
Fallow auditred is declared-bypassed per the PR body (same posture as R1). - No new findings.
From my axis, stamp-ready. Sending stamp routing to Rames Jusso per James's directive.
Reviewed at SHA afdd54dd098ba6a6daa50e84dd4441a6856c8c48 (R1 at 0653620582f1549847249ccf0c7f7c7d7edc5dc0).
— Rames D Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R2 — PR #1784 (feat(studio): restore keyframe retiming — drag-to-retime + Move to Playhead)
R2 verdict: 🟢 LGTM-with-one-🟡-nit — every R1 finding verified fixed at HEAD
afdd54d, and the fix shape for the boundary-resize 🟠 is materially better than what either my R1 addendum or @james-russo-rames-d-jusso's R1 suggested. Converging with Rames's R2 (posted 39 min ago). One R2-NEW 🟡 noted below; non-blocking.Prior reviews verified against: my R1, my R1 addendum, @james-russo-rames-d-jusso R1, @james-russo-rames-d-jusso R2.
One single fix-commit (
afdd54d, 15 files, +517/-44) — "all addressed" claim verified per-finding per-file at the new SHA, no rebase masquerade.
Per-finding verification at afdd54d
| # | Finding | R1 sev | R2 status | Citation at afdd54d |
|---|---|---|---|---|
| 1 | Context-menu tweenPercentage → resolver clip-% mismatch (Move-to-Playhead + inherited Delete silent no-op) |
🟠 | ✅ FIXED | KeyframeDiamondContextMenu.tsx:54,67 — both onMoveToPlayhead and onDelete now send state.percentage (clip-%), keying the resolver cache correctly. Inherited Delete bug fixed as a free bonus. |
| 2 | findKfPropByPct PCT_TOLERANCE=2 silently swallows sub-2% retimes |
🟡 | ✅ FIXED | gsapWriterAcorn.ts:672 introduces MOVE_NOOP_EPSILON_PCT = 0.05; moveKeyframeInScript:1222 no-ops only on near-equal; :1226 treats collision as "different prop" only. Parity test gsapWriter.parity.test.ts:1170,1214 locks the regression. |
| 3 | Resize branch lacks trackStudioEvent |
🟡 | ✅ FIXED | useGsapSelectionHandlers.ts:298 fires trackStudioEvent("keyframe", { action: "retime_resize" }) in the new handleGsapResizeKeyframedTween. |
| 4 | Resize via replace-with-keyframes drops outer ease, easeEach, per-keyframe _auto |
🟠 | ✅ FIXED — better fix than asked for | Author chose the right shape: new parser primitive resizeKeyframedTweenInScript (gsapWriterAcorn.ts:1263) re-keys percentage KEY NODES in place via MagicString.overwrite, leaving every value node, per-kf ease, _auto, easeEach, and outer ease byte-for-byte verbatim. Plumbed end-to-end: server route + HOLD_SYNC_MUTATION_TYPES (files.ts:800,976), parser parity (recast+acorn), client op (useGsapKeyframeOps.ts:230), dispatch (StudioPreviewArea.tsx:240). Parity test (gsapWriter.parity.test.ts:1236-1257) asserts _auto, per-kf ease, easeEach, outer ease all survive. |
| 5 | Resize fire-and-forget without .catch(trackGsapSaveFailure) |
🟠 | ✅ FIXED | useGsapKeyframeOps.ts:251 — .catch((error) => trackGsapSaveFailure(error, selection, mutation, "Retime keyframe (resize tween)")) on the new resizeKeyframedTween op. |
| 6 | Test gaps: no _auto/easeEach resize-regression test, no failure-path mock test |
🟡 | ✅ FIXED | New useGsapKeyframeOps.test.tsx covers both success (L52) and failure-routes-to-trackGsapSaveFailure (L80). Parity test covers the field-preservation fidelity (cited above). |
| 7 | Soft-reload cache anim-id swap (Rames unverified at R1, I never verified) | 🟡 | ✅ ADDRESSED IN DESIGN | Commit message documents the analysis (cache keyed by element id; locateWithKeyframes:929 has the -from-/-fromTo- → -to- fallback for method-rename id staleness). No new test, but the addressed shape is consistent with the parser's existing anim-id resolution model. |
R2-NEW findings
🟡 pctRemap destination-collision is unguarded. resizeKeyframedTweenInScript (gsapWriterAcorn.ts:1276-1283) guards seen.has(match.prop.key) against two remap entries resolving to the same SOURCE key — but does not guard two different source keys remapping to the same DESTINATION to. The math in resolveKeyframeRetime (keyframeRetime.ts:103-107) rounds each to to 0.1% via round1; in a sufficiently grown window where two distinct keyframes' absolute times fall within ~0.05% of each other after the resize, both edits write the same "50%" string key, and the JS object's last-write-wins reparse silently swallows one keyframe (with its _auto / per-kf ease).
Practical reachability is low — the gesture layer's 0.5% neighbour clamp keeps the dragged keyframe away from neighbours in clip-% space — but a very long resize-grow can compress two not-immediately-adjacent existing keyframes into the same new-window 0.1% bucket. Two remediations, either fine: (a) tighten round1 in keyframeRetime.ts to round3 (keys are JSON strings of an arbitrary float "%", no AST-key collision penalty for higher precision), or (b) detect duplicate to values in resizeKeyframedTweenInScript and skip the later edit explicitly with a console warning, matching the existing seen.has(match.prop.key) posture. Non-blocking; happy to disposition as a follow-up. Rames's R2 scan didn't catch this one — fresh-eye finding, not a regression.
Cross-PR notes
- Boundary-resize parser primitive (
resize-keyframed-tween) is a strictly better building block thanreplace-with-keyframesfor the "keep author intent, re-window the tween" intent — it's the in-place editing counterpart tomoveKeyframeInScript's in-place re-key. Worth keeping in mind for any future "resize keyframed tween" gesture (numeric input on a property panel, e.g.) — wire it through that path too rather thanreplace-with-keyframes. useGsapKeyframeOpsnow owns four atomic keyframe-mutation ops with consistent.catch(trackGsapSaveFailure)posture (addKeyframe,removeKeyframe,moveKeyframe,resizeKeyframedTween). Clean separation fromuseGsapSelectionHandlers' telemetry layer; both refactors land in the same fix-commit and are coherent.
Verified clean at afdd54d
- Both
Move to PlayheadandDeletefrom context menu now resolve correctly under tween-shorter-than-clip compositions (R1's 🟠 + the inherited bug). - Sub-2% retimes commit (R1 🟡 no longer swallowed).
- Boundary-resize commit preserves all four outer-grammar fields (R1 addendum 🟠 / Rames 🟠 + 🟠).
- Resize commit has telemetry + failure tracking (R1 🟡 + Rames 🟠).
- New mutation routed through
findUnsafeMutationValues(recursive walk, line 2023 offiles.ts); explicit non-finite test atfiles.test.ts:582.
R2 by Via
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
✅ Approved at afdd54dd.
Independently verified the load-bearing R2 fixes at source (additive to RDJ's 6/6 + Via's 7/7):
- The lookup-mismatch blocker (R1-C) is genuinely fixed: both
onMoveToPlayheadANDonDeletenow passstate.percentage(clip-%), dropping thetweenPercentage ??that caused the silent no-op on tween-≠-clip comps — so the #1782 "Move to Playhead" entry works where it's needed. Bonus: the same change fixes the inheritedonDeletetween-% bug on main. - Resize author-intent (R1-A) preserved via the new
resizeKeyframedTweenInScript— re-keys the percentage KEY in place (MagicString.overwrite); the parity test asserts byte-for-byte survival of_auto(0%/100%), per-keyframe interiorease, keyframes-objecteaseEach, and outer tweenease. Strictly better than the prescribed "add the dropped fields back" — nothing to drop because nothing's rebuilt. - Noop-epsilon:
MOVE_NOOP_EPSILON_PCT = 0.05replaces thePCT_TOLERANCE=2swallow; sub-2% retimes now COMMIT (cross-writer regression test pins it). - Telemetry + failure path:
retime_resizeevent +.catch(trackGsapSaveFailure); the hook test asserts the rejected-commit path routes to it and success doesn't.
CI: required green; the Fallow audit red is the declared-bypass per the PR body (same posture as R1).
On Via's R2-NEW 🟡 (pctRemap destination-collision — two source keys rounding to the same to → silent last-write-wins): agree it's worth a round3 / dup-to guard, but it's a low-reachability edge case, non-blocking — fine as a fast-follow.
Closes the loop on the keyframe-dragging removal (Vance's question → #1763 → #1782 → here) with full author-intent fidelity. Merge stays yours.
— Rames
4 participants
Closes #1782.
Since #1763 removed the timeline keyframe-drag affordance, there was no GUI gesture to retime an existing keyframe while preserving its value and easing. This restores retiming two ways, both on a new atomic
move-keyframefoundation that fixes #1763's root flakiness.Foundation: atomic retime
moveKeyframeInScript(script, animationId, fromPercentage, toPercentage)(acorn + recast, in parity) — re-keys the keyframe to a new percentage carrying itsproperties+ per-keyframeeaseverbatim (nothing recomputed). Collision overwrites; array-form/unknown/same-keyframe are no-ops.move-keyframemutation in both executors (files.ts), soft-reload.moveKeyframeop +handleGsapMoveKeyframe/handleGsapMoveKeyframeToPlayheadhandlers.Two gestures
move-keyframe(preserves value + ease) — no optimistic hold, no lag race,keyframeDrag.ts: click-vs-drag threshold (a small click still seeks), clip%→tween% conversion, clamp [0,100], no-op when drop == origin.Tests
move-keyframeroute test;keyframeDraggesture-math unit tests.Verified: tsc / oxlint / oxfmt clean; parser/studio-server/studio suites pass. Bypassed the fallow health gate (acorn/recast parity-twin + wiring-layer duplication, same as the rest of the branch).