From f25a1369279c99081c56966b14991649138bc035 Mon Sep 17 00:00:00 2001 From: ukimsanov Date: Mon, 20 Jul 2026 18:56:14 -0700 Subject: [PATCH 1/2] fix(studio): retime flat tween keyframe diamonds --- .../components/editor/keyframeRetime.test.ts | 50 +++++++++----- .../src/components/editor/keyframeRetime.ts | 44 ++++++++++++- .../nle/useTimelineEditCallbacks.test.ts | 55 ++++++++++++++++ .../nle/useTimelineEditCallbacks.ts | 66 +++++++++++++++---- 4 files changed, 183 insertions(+), 32 deletions(-) create mode 100644 packages/studio/src/components/nle/useTimelineEditCallbacks.test.ts diff --git a/packages/studio/src/components/editor/keyframeRetime.test.ts b/packages/studio/src/components/editor/keyframeRetime.test.ts index fbc642aa79..5c1d9eb085 100644 --- a/packages/studio/src/components/editor/keyframeRetime.test.ts +++ b/packages/studio/src/components/editor/keyframeRetime.test.ts @@ -9,6 +9,7 @@ const KEYFRAMES: RetimeKeyframe[] = [ { percentage: 100, properties: { x: 100 }, ease: "power2.in" }, ]; const WINDOW = { tweenStart: 2, tweenDuration: 4 }; +const LEFT_BOUNDARY_DROP = { ...WINDOW, dropAbsTime: 0.5 }; describe("resolveKeyframeRetime — move (within the tween window)", () => { it("re-keys an interior keyframe to the tween-% of the drop", () => { @@ -32,15 +33,30 @@ describe("resolveKeyframeRetime — move (within the tween window)", () => { expect(r.kind).toBe("noop"); }); - it("moves a flat (keyframe-less) tween without needing the keyframes array", () => { + it("shortens a flat tween when its synthesized end diamond moves left", () => { const r = resolveKeyframeRetime({ ...WINDOW, keyframes: [], draggedTweenPct: 100, - dropAbsTime: 5, // (5-2)/4 = 75% + dropAbsTime: 5, }); - expect(r.kind).toBe("move"); - expect(r.toTweenPct).toBeCloseTo(75, 5); + expect(r.kind).toBe("resize"); + expect(r.position).toBe(2); + expect(r.duration).toBe(3); + expect(r.pctRemap).toEqual([]); + }); + + it("moves a flat tween's start while preserving its absolute end", () => { + const r = resolveKeyframeRetime({ + ...WINDOW, + keyframes: [], + draggedTweenPct: 0, + dropAbsTime: 3, + }); + expect(r.kind).toBe("resize"); + expect(r.position).toBe(3); + expect(r.duration).toBe(3); + expect(r.pctRemap).toEqual([]); }); }); @@ -67,10 +83,9 @@ describe("resolveKeyframeRetime — resize (past the tween boundary)", () => { it("extends the FIRST keyframe before the start, shifting position earlier", () => { const r = resolveKeyframeRetime({ - ...WINDOW, + ...LEFT_BOUNDARY_DROP, keyframes: KEYFRAMES, draggedTweenPct: 0, - dropAbsTime: 0.5, // before start (2) → move position back + grow duration }); expect(r.kind).toBe("resize"); expect(r.position).toBeCloseTo(0.5, 5); @@ -102,10 +117,9 @@ describe("resolveKeyframeRetime — single keyframe (both first and last)", () = it("resizes left before the start", () => { const r = resolveKeyframeRetime({ - ...WINDOW, + ...LEFT_BOUNDARY_DROP, keyframes: lone, draggedTweenPct: 100, - dropAbsTime: 0.5, }); expect(r.kind).toBe("resize"); expect(r.position).toBeCloseTo(0.5, 5); @@ -127,14 +141,16 @@ describe("resolveKeyframeRetime — guards", () => { ).toBe("noop"); }); - it("no-ops a boundary drop on a flat tween (nothing to remap)", () => { - expect( - resolveKeyframeRetime({ - ...WINDOW, - keyframes: [], - draggedTweenPct: 100, - dropAbsTime: 8, - }).kind, - ).toBe("noop"); + it("extends a flat tween when its synthesized end diamond moves right", () => { + const r = resolveKeyframeRetime({ + ...WINDOW, + keyframes: [], + draggedTweenPct: 100, + dropAbsTime: 8, + }); + expect(r.kind).toBe("resize"); + expect(r.position).toBe(2); + expect(r.duration).toBe(6); + expect(r.pctRemap).toEqual([]); }); }); diff --git a/packages/studio/src/components/editor/keyframeRetime.ts b/packages/studio/src/components/editor/keyframeRetime.ts index 4c0a9c1031..477cae600f 100644 --- a/packages/studio/src/components/editor/keyframeRetime.ts +++ b/packages/studio/src/components/editor/keyframeRetime.ts @@ -56,6 +56,29 @@ const round3 = (n: number) => Math.round(n * 1000) / 1000; const round1 = (n: number) => Math.round(n * 10) / 10; // 0.1% precision const clamp = (n: number, lo: number, hi: number) => Math.max(lo, Math.min(hi, n)); +/** Resolve timing for a flat tween's synthesized start/end diamond. */ +function resolveFlatTweenBoundaryRetime(opts: { + keyframeCount: number; + draggedTweenPct: number; + tweenStart: number; + tweenEnd: number; + dropAbsTime: number; +}): KeyframeRetimeResult | null { + const { keyframeCount, draggedTweenPct, tweenStart, tweenEnd, dropAbsTime } = opts; + if (keyframeCount > 0 || (draggedTweenPct !== 0 && draggedTweenPct !== 100)) return null; + const draggedTime = draggedTweenPct === 0 ? tweenStart : tweenEnd; + if (Math.abs(dropAbsTime - draggedTime) <= EPSILON_TIME) return { kind: "noop" }; + const newStart = draggedTweenPct === 0 ? dropAbsTime : tweenStart; + const newEnd = draggedTweenPct === 100 ? dropAbsTime : tweenEnd; + if (newEnd <= newStart + EPSILON_TIME) return { kind: "noop" }; + return { + kind: "resize", + position: round3(newStart), + duration: round3(newEnd - newStart), + pctRemap: [], + }; +} + /** * Decide move vs resize for a dragged keyframe and, for resize, return the new * tween window + remapped keyframes. @@ -76,15 +99,30 @@ export function resolveKeyframeRetime(opts: { if (tweenDuration <= 0) return { kind: "noop" }; const tweenEnd = tweenStart + tweenDuration; - // Within the tween window → plain move (re-key the tween-%). This branch never - // touches the keyframes array, so it still works for synthesized flat tweens. + // A flat tween's synthesized diamonds are its real start/end boundaries — + // there is no authored keyframe node to re-key. Moving either boundary must + // therefore resize the tween window while keeping the opposite endpoint at + // its absolute time. Returning `resize` with an empty remap lets the caller + // update the flat tween's position/duration instead of sending a keyframe + // mutation that would silently no-op. + const flatBoundary = resolveFlatTweenBoundaryRetime({ + keyframeCount: keyframes.length, + draggedTweenPct, + tweenStart, + tweenEnd, + dropAbsTime, + }); + if (flatBoundary) return flatBoundary; + + // Within the tween window → plain move (re-key the tween-%). if (dropAbsTime >= tweenStart - EPSILON_TIME && dropAbsTime <= tweenEnd + EPSILON_TIME) { const toTweenPct = clamp(((dropAbsTime - tweenStart) / tweenDuration) * 100, 0, 100); if (Math.abs(toTweenPct - draggedTweenPct) < NOOP_EPSILON_PCT) return { kind: "noop" }; return { kind: "move", toTweenPct }; } - // Boundary resize needs the real keyframes to remap; a flat tween has none here. + // Boundary resize needs the real keyframes to remap. Flat start/end boundaries + // were handled above. if (keyframes.length === 0) return { kind: "noop" }; const newStart = Math.min(dropAbsTime, tweenStart); diff --git a/packages/studio/src/components/nle/useTimelineEditCallbacks.test.ts b/packages/studio/src/components/nle/useTimelineEditCallbacks.test.ts new file mode 100644 index 0000000000..f8dbc70d42 --- /dev/null +++ b/packages/studio/src/components/nle/useTimelineEditCallbacks.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, it } from "vitest"; +import { resolveTimelineKeyframeTarget } from "./useTimelineEditCallbacks"; + +const FLAT_ANIMATION = { + id: "#title-fromTo-2900", + propertyGroup: null, +}; + +describe("resolveTimelineKeyframeTarget", () => { + it("resolves a synthesized diamond when one mixed flat tween is selected", () => { + expect( + resolveTimelineKeyframeTarget( + 8.75, + [{ percentage: 8.75, tweenPercentage: 100 }], + [FLAT_ANIMATION], + ), + ).toEqual({ animId: "#title-fromTo-2900", tweenPct: 100 }); + }); + + it("keeps multiple ungrouped flat tweens unresolved instead of guessing", () => { + expect( + resolveTimelineKeyframeTarget( + 8.75, + [{ percentage: 8.75, tweenPercentage: 100 }], + [FLAT_ANIMATION, { id: "#title-to-2900", propertyGroup: null }], + ), + ).toBeNull(); + }); + + it("keeps multiple flat tweens in the same property group unresolved", () => { + expect( + resolveTimelineKeyframeTarget( + 50, + [{ percentage: 50, tweenPercentage: 100, propertyGroup: "position" }], + [ + { id: "position-a", propertyGroup: "position" }, + { id: "position-b", propertyGroup: "position" }, + ], + ), + ).toBeNull(); + }); + + it("prefers an explicit property-group match", () => { + expect( + resolveTimelineKeyframeTarget( + 50, + [{ percentage: 50, tweenPercentage: 25, propertyGroup: "position" }], + [ + { id: "opacity", propertyGroup: "opacity" }, + { id: "position", propertyGroup: "position" }, + ], + ), + ).toEqual({ animId: "position", tweenPct: 25 }); + }); +}); diff --git a/packages/studio/src/components/nle/useTimelineEditCallbacks.ts b/packages/studio/src/components/nle/useTimelineEditCallbacks.ts index 7413aa18a4..a90028083f 100644 --- a/packages/studio/src/components/nle/useTimelineEditCallbacks.ts +++ b/packages/studio/src/components/nle/useTimelineEditCallbacks.ts @@ -36,6 +36,46 @@ export interface TimelineEditCallbackDeps { handleRazorSplitAll: (splitTime: number) => Promise | void; } +interface TimelineKeyframeTargetAnimation { + id: string; + propertyGroup?: string | null; + keyframes?: unknown; +} + +interface TimelineCachedKeyframe { + percentage: number; + tweenPercentage?: number; + propertyGroup?: string; +} + +/** + * Resolve a rendered timeline diamond back to the animation that authored it. + * Flat tweens use synthesized diamonds, so a mixed flat tween may have neither + * a property group nor real keyframes. It is safe to fall back only when that + * flat tween is the selection's sole animation; multiple candidates remain + * unresolved rather than retiming an arbitrary tween. + */ +export function resolveTimelineKeyframeTarget( + pct: number, + keyframes: ReadonlyArray, + animations: ReadonlyArray, +): { animId: string; tweenPct: number } | null { + const kf = keyframes.find((item) => Math.abs(item.percentage - pct) < 0.2); + const group = kf?.propertyGroup; + const groupedCandidates = group + ? animations.filter((animation) => animation.propertyGroup === group) + : []; + const groupedKeyframed = groupedCandidates.find((animation) => animation.keyframes); + const soleGroupedFlat = + groupedCandidates.length === 1 && !groupedCandidates[0]?.keyframes + ? groupedCandidates[0] + : undefined; + const keyframed = animations.find((animation) => animation.keyframes); + const soleFlat = animations.length === 1 && !animations[0]?.keyframes ? animations[0] : undefined; + const animation = groupedKeyframed ?? soleGroupedFlat ?? keyframed ?? soleFlat; + return animation ? { animId: animation.id, tweenPct: kf?.tweenPercentage ?? pct } : null; +} + /** * Builds the timeline edit callback bag (move/resize/split/razor plus the * keyframe-diamond callbacks) provided to `` via TimelineEditProvider. @@ -76,12 +116,7 @@ export function useTimelineEditCallbacks({ // fallow-ignore-next-line complexity (pct: number): { animId: string; tweenPct: number } | null => { const cached = usePlayerStore.getState().keyframeCache.get(domEditSelection?.id ?? ""); - const kf = cached?.keyframes.find((k) => Math.abs(k.percentage - pct) < 0.2); - const group = kf?.propertyGroup; - const anim = - (group ? selectedGsapAnimations.find((a) => a.propertyGroup === group) : undefined) ?? - selectedGsapAnimations.find((a) => a.keyframes); - return anim ? { animId: anim.id, tweenPct: kf?.tweenPercentage ?? pct } : null; + return resolveTimelineKeyframeTarget(pct, cached?.keyframes ?? [], selectedGsapAnimations); }, [domEditSelection?.id, selectedGsapAnimations], ); @@ -154,12 +189,19 @@ export function useTimelineEditCallbacks({ decision.position != null && decision.duration != null ) { - handleGsapResizeKeyframedTween( - target.animId, - decision.position, - decision.duration, - decision.pctRemap, - ); + if (anim.keyframes) { + handleGsapResizeKeyframedTween( + target.animId, + decision.position, + decision.duration, + decision.pctRemap, + ); + } else { + handleGsapUpdateMeta(target.animId, { + position: decision.position, + duration: decision.duration, + }); + } } }, onChangeKeyframeEase: (_elId: string, _pct: number, ease: string) => { From 270179d94b3ca3b82f61655b354855187f0d210c Mon Sep 17 00:00:00 2001 From: ukimsanov Date: Mon, 20 Jul 2026 19:26:10 -0700 Subject: [PATCH 2/2] fix(studio): harden flat keyframe retiming --- .../components/editor/keyframeRetime.test.ts | 23 +++++++++++++ .../src/components/editor/keyframeRetime.ts | 14 +++++--- .../nle/useTimelineEditCallbacks.test.ts | 32 ++++++++++++++++++- .../nle/useTimelineEditCallbacks.ts | 26 +++++++-------- 4 files changed, 76 insertions(+), 19 deletions(-) diff --git a/packages/studio/src/components/editor/keyframeRetime.test.ts b/packages/studio/src/components/editor/keyframeRetime.test.ts index 5c1d9eb085..876c5129a5 100644 --- a/packages/studio/src/components/editor/keyframeRetime.test.ts +++ b/packages/studio/src/components/editor/keyframeRetime.test.ts @@ -58,6 +58,29 @@ describe("resolveKeyframeRetime — move (within the tween window)", () => { expect(r.duration).toBe(3); expect(r.pctRemap).toEqual([]); }); + + it("no-ops an unexpected interior percentage on a flat tween", () => { + expect( + resolveKeyframeRetime({ + ...WINDOW, + keyframes: [], + draggedTweenPct: 50, + dropAbsTime: 5, + }).kind, + ).toBe("noop"); + }); + + it("no-ops instead of rounding a sub-10ms flat tween to zero", () => { + expect( + resolveKeyframeRetime({ + tweenStart: 2, + tweenDuration: 4, + keyframes: [], + draggedTweenPct: 100, + dropAbsTime: 2.0004, + }).kind, + ).toBe("noop"); + }); }); describe("resolveKeyframeRetime — resize (past the tween boundary)", () => { diff --git a/packages/studio/src/components/editor/keyframeRetime.ts b/packages/studio/src/components/editor/keyframeRetime.ts index 477cae600f..f235b418ed 100644 --- a/packages/studio/src/components/editor/keyframeRetime.ts +++ b/packages/studio/src/components/editor/keyframeRetime.ts @@ -51,6 +51,8 @@ export interface KeyframeRetimeResult { const NOOP_EPSILON_PCT = 0.1; /** Slack (seconds) for the within-tween boundary test. */ const EPSILON_TIME = 1e-4; +/** Smallest authored tween window; avoids sub-millisecond/round-to-zero durations. */ +const MIN_TWEEN_DURATION = 0.01; const round3 = (n: number) => Math.round(n * 1000) / 1000; const round1 = (n: number) => Math.round(n * 10) / 10; // 0.1% precision @@ -65,16 +67,20 @@ function resolveFlatTweenBoundaryRetime(opts: { dropAbsTime: number; }): KeyframeRetimeResult | null { const { keyframeCount, draggedTweenPct, tweenStart, tweenEnd, dropAbsTime } = opts; - if (keyframeCount > 0 || (draggedTweenPct !== 0 && draggedTweenPct !== 100)) return null; + if (keyframeCount > 0) return null; + // Flat tweens have only synthesized boundary diamonds. Never delegate an + // unexpected interior percentage to the authored-keyframe move path. + if (draggedTweenPct !== 0 && draggedTweenPct !== 100) return { kind: "noop" }; const draggedTime = draggedTweenPct === 0 ? tweenStart : tweenEnd; if (Math.abs(dropAbsTime - draggedTime) <= EPSILON_TIME) return { kind: "noop" }; const newStart = draggedTweenPct === 0 ? dropAbsTime : tweenStart; const newEnd = draggedTweenPct === 100 ? dropAbsTime : tweenEnd; - if (newEnd <= newStart + EPSILON_TIME) return { kind: "noop" }; + const newDuration = newEnd - newStart; + if (newDuration < MIN_TWEEN_DURATION) return { kind: "noop" }; return { kind: "resize", position: round3(newStart), - duration: round3(newEnd - newStart), + duration: round3(newDuration), pctRemap: [], }; } @@ -127,7 +133,7 @@ export function resolveKeyframeRetime(opts: { const newStart = Math.min(dropAbsTime, tweenStart); const newEnd = Math.max(dropAbsTime, tweenEnd); - const newDuration = Math.max(0.01, newEnd - newStart); + const newDuration = Math.max(MIN_TWEEN_DURATION, newEnd - newStart); // The dragged keyframe is the one whose tween-% is closest to draggedTweenPct. let draggedIdx = 0; diff --git a/packages/studio/src/components/nle/useTimelineEditCallbacks.test.ts b/packages/studio/src/components/nle/useTimelineEditCallbacks.test.ts index f8dbc70d42..662234daa0 100644 --- a/packages/studio/src/components/nle/useTimelineEditCallbacks.test.ts +++ b/packages/studio/src/components/nle/useTimelineEditCallbacks.test.ts @@ -40,7 +40,37 @@ describe("resolveTimelineKeyframeTarget", () => { ).toBeNull(); }); - it("prefers an explicit property-group match", () => { + it("keeps a keyframed and flat tween in the same property group unresolved", () => { + expect( + resolveTimelineKeyframeTarget( + 50, + [{ percentage: 50, tweenPercentage: 25, propertyGroup: "position" }], + [ + { id: "position-keyframed", propertyGroup: "position", keyframes: {} }, + { id: "position-flat", propertyGroup: "position" }, + ], + ), + ).toBeNull(); + }); + + it("keeps multiple ungrouped keyframed tweens unresolved", () => { + expect( + resolveTimelineKeyframeTarget( + 50, + [{ percentage: 50, tweenPercentage: 25 }], + [ + { id: "ungrouped-a", keyframes: {} }, + { id: "ungrouped-b", keyframes: {} }, + ], + ), + ).toBeNull(); + }); + + it("does not infer an animation when the rendered diamond misses the cache", () => { + expect(resolveTimelineKeyframeTarget(60, [], [FLAT_ANIMATION])).toBeNull(); + }); + + it("resolves the sole candidate in an explicit property group", () => { expect( resolveTimelineKeyframeTarget( 50, diff --git a/packages/studio/src/components/nle/useTimelineEditCallbacks.ts b/packages/studio/src/components/nle/useTimelineEditCallbacks.ts index a90028083f..76122eb47c 100644 --- a/packages/studio/src/components/nle/useTimelineEditCallbacks.ts +++ b/packages/studio/src/components/nle/useTimelineEditCallbacks.ts @@ -51,9 +51,10 @@ interface TimelineCachedKeyframe { /** * Resolve a rendered timeline diamond back to the animation that authored it. * Flat tweens use synthesized diamonds, so a mixed flat tween may have neither - * a property group nor real keyframes. It is safe to fall back only when that - * flat tween is the selection's sole animation; multiple candidates remain - * unresolved rather than retiming an arbitrary tween. + * a property group nor real keyframes. The cache currently carries a property + * group, not an animation id, so resolution is safe only when that group has a + * single candidate. Ambiguous candidates remain unresolved rather than + * retiming an arbitrary tween. */ export function resolveTimelineKeyframeTarget( pct: number, @@ -61,19 +62,13 @@ export function resolveTimelineKeyframeTarget( animations: ReadonlyArray, ): { animId: string; tweenPct: number } | null { const kf = keyframes.find((item) => Math.abs(item.percentage - pct) < 0.2); + if (!kf) return null; const group = kf?.propertyGroup; - const groupedCandidates = group + const candidates = group ? animations.filter((animation) => animation.propertyGroup === group) - : []; - const groupedKeyframed = groupedCandidates.find((animation) => animation.keyframes); - const soleGroupedFlat = - groupedCandidates.length === 1 && !groupedCandidates[0]?.keyframes - ? groupedCandidates[0] - : undefined; - const keyframed = animations.find((animation) => animation.keyframes); - const soleFlat = animations.length === 1 && !animations[0]?.keyframes ? animations[0] : undefined; - const animation = groupedKeyframed ?? soleGroupedFlat ?? keyframed ?? soleFlat; - return animation ? { animId: animation.id, tweenPct: kf?.tweenPercentage ?? pct } : null; + : animations.filter((animation) => !animation.propertyGroup); + const animation = candidates.length === 1 ? candidates[0] : undefined; + return animation ? { animId: animation.id, tweenPct: kf.tweenPercentage ?? pct } : null; } /** @@ -197,6 +192,9 @@ export function useTimelineEditCallbacks({ decision.pctRemap, ); } else { + // resize-keyframed-tween requires an authored `keyframes` AST node + // and intentionally no-ops for a flat tween. Update its real tween + // window through the metadata writer (and SDK cutover path) instead. handleGsapUpdateMeta(target.animId, { position: decision.position, duration: decision.duration,