fix(studio): retime flat tween keyframe diamonds

[ukimsanov]

Summary

• make synthetic start/end diamonds for flat GSAP tweens draggable in the Studio timeline
• resize the flat tween while preserving the opposite endpoint
• keep explicit keyframed-tween retiming behavior unchanged
• avoid mutating ambiguous timelines when a unique flat tween cannot be resolved

Root cause

The timeline renders synthetic 0% and 100% diamonds for ordinary to, from, and fromTo tweens. The drag callback only resolved explicit keyframed animations or property-group animations, so a sole mixed flat tween such as fromTo("#title", ...) had no retime target and silently no-op'd.

Safety and edge cases

• moving the start preserves the absolute end time
• moving the end preserves the absolute start time
• crossing the opposite endpoint remains blocked
• zero-duration/static holds remain excluded
• multiple ambiguous flat tweens remain unchanged rather than retiming the wrong animation
• values and easing are untouched; only position and duration change

Validation

• focused Studio tests: 14 passed
• Studio TypeScript typecheck passed
• Studio production build passed
• pre-commit lint, formatting, typecheck, artifact, and complexity checks passed
• browser acceptance against the kitchen-sink project using local workspace packages: dragged the title end diamond from 8.75% to 33.5%, verified source duration changed from 0.7s to 2.68s, then verified undo, redo, reload persistence, and fixture restoration

[Copilot]

Pull request overview

This PR improves Studio timeline keyframe-diamond dragging so that synthesized start/end diamonds for “flat” GSAP tweens (to/from/fromTo) resolve back to a real animation target and retime by resizing the tween window (updating position/duration) instead of attempting keyframe mutations.

Changes:

• Added resolveTimelineKeyframeTarget to map rendered diamonds back to a GSAP animation (including single mixed flat tweens) and added unit tests.
• Updated resolveKeyframeRetime to treat flat tween boundary diamonds as true start/end handles (resize behavior preserving the opposite endpoint), with expanded test coverage.
• Updated timeline edit callbacks to apply meta updates (position/duration) when retiming a flat tween, while keeping keyframed-tween behavior on the keyframe mutation path.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
packages/studio/src/components/nle/useTimelineEditCallbacks.ts	Adds diamond→animation resolver and routes flat-tween retiming to meta updates.
packages/studio/src/components/nle/useTimelineEditCallbacks.test.ts	Unit tests for diamond→animation resolution, including flat tween ambiguity cases.
packages/studio/src/components/editor/keyframeRetime.ts	Adds flat-boundary retiming path returning resize for synthesized 0%/100% diamonds.
packages/studio/src/components/editor/keyframeRetime.test.ts	Updates/extends tests to validate flat boundary resizing semantics.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[miga-heygen]

SSOT Review: fix(studio): retime flat tween keyframe diamonds

PR #2670 — 4 files, +183/-32

---

SSOT inventory

Concept	Source of truth	Consumers	Verdict
Flat tween boundary retime	resolveFlatTweenBoundaryRetime	resolveKeyframeRetime	Single owner
Diamond → animation resolution	resolveTimelineKeyframeTarget (extracted)	useTimelineEditCallbacks	Single owner
Resize dispatch (keyframed vs flat)	anim.keyframes branch in onMoveKeyframe	—	Single owner
Synthesized diamond identity	keyframeCount === 0 + boundary pct (0/100)	—	Consistent with !animation.keyframes in target resolution

What I checked

1. resolveFlatTweenBoundaryRetime entry guard — if (keyframeCount > 0 || (draggedTweenPct !== 0 && draggedTweenPct !== 100)) return null. Returns null (delegate to existing logic) for keyframed tweens or interior-percentage drags. Correct — flat tweens only have boundary diamonds.

2. Endpoint preservation — dragging pct 0 moves newStart, keeps newEnd = tweenEnd. Dragging pct 100 moves newEnd, keeps newStart = tweenStart. The opposite endpoint stays at its absolute time in both cases. Tests verify: start-drag from position 2 to 3 yields position: 3, duration: 3 (end stays at 6); end-drag from position 6 to 5 yields position: 2, duration: 3 (start stays at 2). Correct.

3. Crossing guard — if (newEnd <= newStart + EPSILON_TIME) return { kind: "noop" }. Prevents collapsing the tween to zero or negative duration by dragging one endpoint past the other. Correct.

4. No-op guard — if (Math.abs(dropAbsTime - draggedTime) <= EPSILON_TIME) return { kind: "noop" }. Drop at same position = no-op. Correct.

5. Empty pctRemap — flat tweens return pctRemap: [] since there are no authored keyframes to remap. The caller checks this at the resize dispatch: anim.keyframes is truthy → handleGsapResizeKeyframedTween (remaps keyframes); falsy → handleGsapUpdateMeta (just position/duration). The anim variable is looked up at line 164 via selectedGsapAnimations.find(a => a.id === target.animId) and null-checked at line 166, so it's safe by the time the branch executes. Correct.

6. resolveTimelineKeyframeTarget resolution priority — groupedKeyframed ?? soleGroupedFlat ?? keyframed ?? soleFlat. Property-group match with keyframes wins, then sole grouped flat, then any keyframed, then sole flat. Multiple ungrouped flat tweens → null (don't guess). Tests cover all four resolution paths plus the ambiguity guard. Correct.

7. Test migration — old test "moves a flat (keyframe-less) tween without needing the keyframes array" expected kind: "move" with toTweenPct: 75. Now expects kind: "resize" with position: 2, duration: 3. This is the core behavioral change: flat tween diamonds no longer produce a move (which would try to re-key a non-existent keyframe), they produce a resize (which updates the tween window). The old "no-ops a boundary drop on a flat tween" test now expects resize too — extending the tween by dragging the end diamond past the original boundary is now supported.

8. Sentinel-value scan:

   • kf?.propertyGroup — handles undefined keyframe. Correct.
   • kf?.tweenPercentage ?? pct — falls back to raw percentage. Correct.
   • cached?.keyframes ?? [] — handles undefined cache. Correct.
   • groupedCandidates[0]?.keyframes — safe optional access. Correct.
   • anim.keyframes?.keyframes ?? [] — handles flat tweens with no .keyframes sub-object. Correct.

Blocking SSOT issues

None found.

Verdict

Approve. Well-scoped fix with a clean separation: resolveFlatTweenBoundaryRetime owns the "what should happen" decision for flat tweens, resolveTimelineKeyframeTarget owns the "which animation did the user drag" resolution, and the dispatch branch owns the "how to apply it" fork. Tests cover all four flat-tween scenarios (shorten, extend, move start, same-position no-op) plus the target resolution edge cases (sole flat, ambiguous multiples, grouped match).

--- Miga

[james-russo-rames-d-jusso]

Reviewed at f25a136.

Clean, focused fix — extracted resolveFlatTweenBoundaryRetime owns the timing decision, resolveTimelineKeyframeTarget owns diamond-to-anim resolution with a real ambiguity guard, and the caller's dispatch branches correctly between the keyframed-resize op and handleGsapUpdateMeta for flat tweens. Manual UI validation (drag → undo → redo → reload persistence) is exactly the empirical evidence this class of change needs. CI green across 30+ checks.

One concern, one question, two nits inline. Nothing blocking from where I sit.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

🟠 Concern — min-duration inconsistency between the two resize paths. The flat-tween boundary allows a resize down to > EPSILON_TIME (1e-4s ≈ 0.1ms). The keyframed boundary resize just below (L130) floors at Math.max(0.01, newEnd - newStart) — a 10ms minimum. So a flat tween can be dragged to a 0.5ms window while a keyframed tween can't. GSAP will likely render both fine at first, but sub-millisecond durations are the kind of thing that produces snap-to-frame artifacts and undo-history noise. Suggest aligning to the 0.01s floor here (either the same Math.max clamp on newEnd - newStart, or bumping the noop threshold from EPSILON_TIME to 0.01). Non-blocking — the gesture layer's clip clamp keeps most drags away from this, but the floor is worth having explicitly. — Rames D Jusso

[james-russo-rames-d-jusso]

🟡 Nit — silent fall-through when draggedTweenPct is neither 0 nor 100. If a caller ever emits onMoveKeyframe with a tweenPct that isn't exactly 0 or 100 for a flat tween (e.g. because resolveTimelineKeyframeTarget fell back to soleFlat and the cached-keyframe find() missed via the 0.2 tolerance → tweenPct = pct raw clip-%), this guard returns null, control falls through to the within-tween branch (L118) which returns move, and the caller invokes handleGsapMoveKeyframe — the old silently-no-op path for flat tweens. In practice the timeline layer only fires this handler on synthesized 0%/100% diamond clicks, so this is a defensive gap rather than a live bug. Worth either an explicit keyframeCount === 0 fast-path noop return here, or a comment documenting that the 0-or-100 assumption is enforced upstream. — Rames D Jusso

[james-russo-rames-d-jusso]

❓ Question — why the two-branch dispatch instead of handleGsapResizeKeyframedTween(id, pos, dur, [])? resolveFlatTweenBoundaryRetime already returns pctRemap: [], and the keyframed-resize op has to handle empty remaps anyway (a keyframed tween can be extended past a boundary without cross-remapping the rest — well, actually it always remaps, but an empty array is a degenerate case). Was there a specific reason handleGsapUpdateMeta was preferred here — e.g. different undo-history entry, different downstream cache invalidation, or the resize op asserts keyframes.length > 0 somewhere? Fine either way; just curious about the semantic distinction so the next PR touching this branch knows which path to reach for. — Rames D Jusso

[james-russo-rames-d-jusso]

🟡 Nit — priority chain has 4 tiers, tests cover 3. The resolver's fallback chain is groupedKeyframed ?? soleGroupedFlat ?? keyframed ?? soleFlat. Tests cover: soleFlat (test 1), null on multi-ungrouped-flat (test 2), null on multi-grouped-flat (test 3), and soleGroupedFlat (test 4 — despite the name «prefers an explicit property-group match», both animations are flat here, so it's actually testing the soleGroupedFlat branch). What's NOT tested: groupedKeyframed winning over a sibling flat in the same group (the intentional behavior change from the old .find(a => a.propertyGroup === group) which took ANY match). A one-liner test with a keyframed + flat in the same group would lock in that priority. — Rames D Jusso

[miga-heygen]

Re-review: fix(studio): retime flat tween keyframe diamonds (R2)

R2 tightens three things from R1:

1. Interior percentage guard split. The flat tween check now separates if (keyframeCount > 0) return null (delegate to keyframed logic) from if (draggedTweenPct !== 0 && draggedTweenPct !== 100) return { kind: "noop" } (unexpected interior pct on flat tween → explicit noop). In R1 these were a single combined condition returning null, which would let an unexpected interior drag fall through to the authored-keyframe move path. Now it's correctly caught. New test: "no-ops an unexpected interior percentage on a flat tween."

2. MIN_TWEEN_DURATION constant. Replaces the hardcoded 0.01 in the existing resize path AND the EPSILON_TIME comparison in the flat boundary check. The old newEnd <= newStart + EPSILON_TIME (0.0001s) could allow sub-10ms tweens; the new newDuration < MIN_TWEEN_DURATION (0.01s = 10ms) is a clear UI policy. The existing keyframed resize path's Math.max(0.01, ...) now uses the same named constant. New test: "no-ops instead of rounding a sub-10ms flat tween to zero" (drop at 2.0004 → would be 0.4ms duration → noop).

3. Simplified resolveTimelineKeyframeTarget. R1 had a 4-tier priority chain (groupedKeyframed ?? soleGroupedFlat ?? keyframed ?? soleFlat) that could resolve ambiguously when a group had both keyframed and flat tweens. R2 collapses to: filter candidates by group (or filter ungrouped), then candidates.length === 1 ? candidates[0] : undefined. ANY ambiguity → null. Also added if (!kf) return null early exit for cache misses. Three new tests cover the tighter ambiguity rules.

All changes are strictly more defensive. No new SSOT concerns.

Approve. R2 is the better version.

--- Miga

[jrusso1020]

Approved on James's go. CI green (perf checks are informational); Miga + RDJ reviewed and RDJ's R1 concerns are addressed in R2 — MIN_TWEEN_DURATION unifies the flat/keyframed min-duration floor (sub-10ms flat drags no-op instead of collapsing to zero), the unexpected interior-percentage case is an explicit no-op instead of falling through to authored-keyframe logic, and the diamond resolver is single-candidate (any ambiguity → null).