Skip to content

feat(studio): add timeline keyframe retiming interactions #2783

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
miguel-heygen wants to merge 3 commits into
base: codex/studio-timeline-b-variable-layout-v2
Choose a base branch
from
Open

feat(studio): add timeline keyframe retiming interactions #2783

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b3ce4dd
feat(studio): add timeline keyframe retiming interactions
miguel-heygen Jul 20, 2026
2c3ed8f
fix(studio): let Escape cancel a keyframe retime and throttle its pre…
miguel-heygen Jul 25, 2026
32427d9
refactor(studio): guard the diamond connector's previous keyframe
miguel-heygen Jul 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions packages/studio/src/components/editor/keyframeRetime.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// Boundary cases share an arrange/assert shape on purpose: each case states its
// own window, drag, and expected remap so a failure reads without cross-referencing.
// fallow-ignore-file code-duplication
import { describe, expect, it } from "vitest";
import { resolveKeyframeRetime, type RetimeKeyframe } from "./keyframeRetime";

Expand Down Expand Up @@ -94,12 +97,12 @@ describe("resolveKeyframeRetime — resize (past the tween boundary)", () => {
expect(r.kind).toBe("resize");
expect(r.position).toBeCloseTo(2, 5); // start unchanged
expect(r.duration).toBeCloseTo(6, 5); // 8 - 2
// abs 2/4/8 over the new [2,8] window → 0 / 33.3 / 100. pctRemap carries each
// abs 2/4/8 over the new [2,8] window → 0 / 33.333 / 100. pctRemap carries each
// existing keyframe's old→new tween-%; the commit re-keys in place (value +
// ease + _auto preserved by round-tripping the source node, not re-emitted here).
expect(r.pctRemap).toEqual([
{ from: 0, to: 0 },
{ from: 50, to: 33.3 },
{ from: 50, to: 33.333 },
{ from: 100, to: 100 },
]);
});
Expand All @@ -113,10 +116,10 @@ describe("resolveKeyframeRetime — resize (past the tween boundary)", () => {
expect(r.kind).toBe("resize");
expect(r.position).toBeCloseTo(0.5, 5);
expect(r.duration).toBeCloseTo(5.5, 5); // 6 - 0.5
// abs 0.5/4/6 over [0.5,6] → 0 / 63.6 / 100.
// abs 0.5/4/6 over [0.5,6] → 0 / 63.636 / 100.
expect(r.pctRemap).toEqual([
{ from: 0, to: 0 },
{ from: 50, to: 63.6 },
{ from: 50, to: 63.636 },
{ from: 100, to: 100 },
]);
});
Expand Down
3 changes: 1 addition & 2 deletions packages/studio/src/components/editor/keyframeRetime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ const EPSILON_TIME = 1e-4;
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
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. */
Expand Down Expand Up @@ -153,7 +152,7 @@ export function resolveKeyframeRetime(opts: {
const pctRemap: KeyframePctRemap[] = keyframes.map((kf, i) => {
const absTime =
i === draggedIdx ? dropAbsTime : tweenStart + (kf.percentage / 100) * tweenDuration;
return { from: kf.percentage, to: round1(((absTime - newStart) / newDuration) * 100) };
return { from: kf.percentage, to: round3(((absTime - newStart) / newDuration) * 100) };
});

return {
Expand Down
9 changes: 6 additions & 3 deletions packages/studio/src/components/nle/useTimelineEditCallbacks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,13 +152,13 @@ export function useTimelineEditCallbacks({
// resizes the tween — position/duration grow so the dragged keyframe lands at
// the drop while every other keyframe keeps its absolute time (value+ease too).
// fallow-ignore-next-line complexity
onMoveKeyframe: (_elId: string, fromClipPct: number, toClipPct: number) => {
onMoveKeyframe: async (_elId: string, fromClipPct: number, toClipPct: number) => {
const target = resolveKeyframeTarget(fromClipPct);
const sel = domEditSelection;
if (!target || !sel) return;
if (!target || !sel) return false;
const anim = selectedGsapAnimations.find((a) => a.id === target.animId);
const tweenStart = anim ? resolveTweenStart(anim) : null;
if (!anim || tweenStart === null) return;
if (!anim || tweenStart === null) return false;

@james-russo-rames-d-jusso james-russo-rames-d-jusso Jul 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Promise committed signal is cosmetic — mutation is fire-and-forget

if (decision.kind === "move" && decision.toTweenPct != null) {
  handleGsapMoveKeyframe(target.animId, target.tweenPct, decision.toTweenPct);
} else if (...) {
  ...
    handleGsapResizeKeyframedTween(...);
  } else {
    handleGsapUpdateMeta(...);
  }
} else {
  return false;
}
return true;

The entire PR feature (pendingRetimeRef + clearPending on !committed) rests on onMoveKeyframe returning a Promise reflecting whether the mutation actually persisted. But handleGsapMoveKeyframe, handleGsapResizeKeyframedTween, handleGsapUpdateMeta in useGsapSelectionHandlers.ts return void (line 342: void moveKeyframe(sel, animId, fromPercentage, toPercentage);), and moveKeyframe/resizeKeyframedTween in useGsapKeyframeOps.ts use void commitMutation(...).catch(error => trackGsapSaveFailure(...)) — fire-and-forget with a catch that only fires telemetry. onMoveKeyframe: async always resolves to true immediately after firing, regardless of whether server commit rejects, selection is stale, schema validator refuses, etc. The .then((committed) => if (!committed) clearPending()) in TimelineClipDiamonds is effectively dead code for anything except the two synchronous early-returns in useTimelineEditCallbacks itself.

Fix: Await the underlying handler in a variant that returns its commitMutation promise (e.g. moveKeyframeAsync on the actions context returning Promise) and thread it through handleGsapMoveKeyframe → useTimelineEditCallbacks. Or explicitly document that committed only reflects preflight validity — but then rename it so consumers stop treating it as a persist signal.

— Review by Rames D Jusso

const tweenDuration = anim.duration ?? resolveTweenDuration(anim);
const sourceFile = sel.sourceFile || activeCompPath || "index.html";
const { elements, domClipChildren } = usePlayerStore.getState();
Expand Down Expand Up @@ -200,7 +200,10 @@ export function useTimelineEditCallbacks({
duration: decision.duration,
});
}
} else {
return false;
}
return true;
},
onChangeKeyframeEase: (_elId: string, _pct: number, ease: string) => {
for (const anim of selectedGsapAnimations) {
Expand Down
34 changes: 30 additions & 4 deletions packages/studio/src/player/components/KeyframeDiamondContextMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,32 @@ export interface KeyframeDiamondContextMenuState {
elementId: string;
percentage: number;
tweenPercentage?: number;
propertyGroup?: string;
animationId?: string;
currentEase?: string;
}

interface KeyframeDiamondContextMenuProps {
state: KeyframeDiamondContextMenuState;
onClose: () => void;
onDelete: (elementId: string, percentage: number) => void;
onDelete: (
elementId: string,
percentage: number,
propertyGroup?: string,
tweenPercentage?: number,
animationId?: string,
) => void;
onDeleteAll: (elementId: string) => void;
onChangeEase?: (elementId: string, percentage: number, ease: string) => void;
onCopyProperties?: (elementId: string, percentage: number) => void;
/** Retime the keyframe to the current playhead, preserving its value + ease. */
onMoveToPlayhead?: (elementId: string, fromPercentage: number) => void;
onMoveToPlayhead?: (
elementId: string,
fromPercentage: number,
propertyGroup?: string,
tweenPercentage?: number,
animationId?: string,
) => void;
}

export const KeyframeDiamondContextMenu = memo(function KeyframeDiamondContextMenu({
Expand Down Expand Up @@ -51,7 +65,13 @@ export const KeyframeDiamondContextMenu = memo(function KeyframeDiamondContextMe
// 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.
onMoveToPlayhead(state.elementId, state.percentage);
onMoveToPlayhead(
state.elementId,
state.percentage,
state.propertyGroup,
state.tweenPercentage,
state.animationId,
);
onClose();
}}
>
Expand All @@ -64,7 +84,13 @@ export const KeyframeDiamondContextMenu = memo(function KeyframeDiamondContextMe
type="button"
className="w-full flex items-center gap-2 px-3 py-1.5 text-xs text-red-400 hover:bg-neutral-800 cursor-pointer text-left"
onClick={() => {
onDelete(state.elementId, state.percentage);
onDelete(
state.elementId,
state.percentage,
state.propertyGroup,
state.tweenPercentage,
state.animationId,
);
onClose();
}}
>
Expand Down
Loading
Loading