From 675cbe194df4ebf0cda3fc488ec0088b9c576e11 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Mon, 27 Jul 2026 15:46:59 +0200 Subject: [PATCH] fix(studio): retime flat tween boundaries and target the clicked keyframe's own tween Two more regressions the QA triage attributed to this stack. Dragging a flat tween's boundary diamond did nothing: the handler bailed on `!anim.keyframes` even though resolveKeyframeRetime already resolves that case to a position/duration resize. The empty remap now dispatches update-meta, which moves the window without rewriting the authored flat tween into keyframes form (what the keyframed-resize writer would do as a side effect). The property panel's keyframe gutter guessed one animation per property group, so clicking a keyframe authored by a sibling tween on a merged row named the wrong tween and the writer silently removed nothing. The diamond now reports the clicked keyframe's own animationId, with the group guess kept as the fallback for cache rows that carry no identity. --- .../components/editor/KeyframeNavigation.tsx | 12 +++- .../editor/KeyframeNavigationDiamond.test.tsx | 59 +++++++++++++++++++ .../src/components/editor/PropertyPanel.tsx | 20 +++++-- .../editor/propertyPanelFlatLayoutSection.tsx | 4 +- .../nle/useTimelineEditCallbacks.test.tsx | 11 +++- .../nle/useTimelineEditCallbacks.ts | 17 ++++-- 6 files changed, 108 insertions(+), 15 deletions(-) create mode 100644 packages/studio/src/components/editor/KeyframeNavigationDiamond.test.tsx diff --git a/packages/studio/src/components/editor/KeyframeNavigation.tsx b/packages/studio/src/components/editor/KeyframeNavigation.tsx index b8016f1a6..219e08d92 100644 --- a/packages/studio/src/components/editor/KeyframeNavigation.tsx +++ b/packages/studio/src/components/editor/KeyframeNavigation.tsx @@ -11,12 +11,15 @@ interface KeyframeNavigationProps { tweenPercentage?: number; properties: Record; ease?: string; + /** The tween that authored this keyframe, when the cache knows it. */ + animationId?: string; }> | null; /** Current playhead percentage within the element's lifetime (0-100) */ currentPercentage: number; onSeek: (percentage: number) => void; onAddKeyframe: (percentage: number) => void; - onRemoveKeyframe: (percentage: number) => void; + /** `animationId` is the clicked keyframe's OWN tween; see handleDiamondClick. */ + onRemoveKeyframe: (percentage: number, animationId?: string) => void; onConvertToKeyframes: () => void; } @@ -152,7 +155,12 @@ export const KeyframeNavigation = memo(function KeyframeNavigation({ if (diamondState === "ghost") { onConvertToKeyframes(); } else if (diamondState === "active" && atCurrent) { - onRemoveKeyframe(atCurrent.tweenPercentage ?? atCurrent.percentage); + // Report the keyframe's OWN tween. A merged gutter row shows the keyframes + // of every tween in the property group, so the caller's "the group's + // animation" guess names the wrong tween whenever the clicked keyframe + // belongs to a sibling — and the writer then finds no keyframe at that + // percentage and silently does nothing. + onRemoveKeyframe(atCurrent.tweenPercentage ?? atCurrent.percentage, atCurrent.animationId); } else { onAddKeyframe(clipToTweenPercentage(propertyKeyframes, currentPercentage)); } diff --git a/packages/studio/src/components/editor/KeyframeNavigationDiamond.test.tsx b/packages/studio/src/components/editor/KeyframeNavigationDiamond.test.tsx new file mode 100644 index 000000000..3cf0ddcd1 --- /dev/null +++ b/packages/studio/src/components/editor/KeyframeNavigationDiamond.test.tsx @@ -0,0 +1,59 @@ +// @vitest-environment happy-dom + +import { act } from "react"; +import { beforeAll, describe, expect, it, vi } from "vitest"; +import { installReactActEnvironment, mountReactHarness } from "../../hooks/domSelectionTestHarness"; +import { KeyframeNavigation } from "./KeyframeNavigation"; + +beforeAll(installReactActEnvironment); + +/** + * Regression: a merged gutter row shows the keyframes of EVERY tween in the + * property group. The panel guesses "the group's animation" for the write, so a + * click on a keyframe authored by a sibling tween named the wrong animation and + * the writer silently found nothing to remove. The diamond now reports the + * clicked keyframe's own animationId. + */ +const MERGED_ROW = [ + { percentage: 0, tweenPercentage: 0, properties: { x: 0 }, animationId: "delta-to-500-position" }, + { + percentage: 50, + tweenPercentage: 25, + properties: { x: 120 }, + animationId: "delta-to-4000-position", + }, +]; + +function clickDiamond(currentPercentage: number, onRemoveKeyframe: (...args: never[]) => void) { + const root = mountReactHarness( + , + ); + const diamond = document.querySelector('[title="Remove x keyframe"]'); + expect(diamond).not.toBeNull(); + act(() => { + diamond?.click(); + }); + act(() => root.unmount()); +} + +describe("KeyframeNavigation diamond", () => { + it("reports the clicked keyframe's own animation on a merged row", () => { + const onRemoveKeyframe = vi.fn(); + clickDiamond(50, onRemoveKeyframe); + expect(onRemoveKeyframe).toHaveBeenCalledWith(25, "delta-to-4000-position"); + }); + + it("still reports the first tween's keyframe as its own", () => { + const onRemoveKeyframe = vi.fn(); + clickDiamond(0, onRemoveKeyframe); + expect(onRemoveKeyframe).toHaveBeenCalledWith(0, "delta-to-500-position"); + }); +}); diff --git a/packages/studio/src/components/editor/PropertyPanel.tsx b/packages/studio/src/components/editor/PropertyPanel.tsx index 237d84269..0c9905e80 100644 --- a/packages/studio/src/components/editor/PropertyPanel.tsx +++ b/packages/studio/src/components/editor/PropertyPanel.tsx @@ -408,7 +408,9 @@ export const PropertyPanel = memo(function PropertyPanel(props: PropertyPanelPro onCommitAnimatedProperty && void onCommitAnimatedProperty(element, "x", displayX) } - onRemoveKeyframe={(pct) => onRemoveKeyframe?.(animIdForProp("x"), pct)} + onRemoveKeyframe={(pct, animationId) => + onRemoveKeyframe?.(animationId ?? animIdForProp("x"), pct) + } onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("x"))} /> )} @@ -433,7 +435,9 @@ export const PropertyPanel = memo(function PropertyPanel(props: PropertyPanelPro onCommitAnimatedProperty && void onCommitAnimatedProperty(element, "y", displayY) } - onRemoveKeyframe={(pct) => onRemoveKeyframe?.(animIdForProp("y"), pct)} + onRemoveKeyframe={(pct, animationId) => + onRemoveKeyframe?.(animationId ?? animIdForProp("y"), pct) + } onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("y"))} /> )} @@ -458,7 +462,9 @@ export const PropertyPanel = memo(function PropertyPanel(props: PropertyPanelPro onCommitAnimatedProperty && void onCommitAnimatedProperty(element, "width", displayW) } - onRemoveKeyframe={(pct) => onRemoveKeyframe?.(animIdForProp("width"), pct)} + onRemoveKeyframe={(pct, animationId) => + onRemoveKeyframe?.(animationId ?? animIdForProp("width"), pct) + } onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("width"))} /> )} @@ -483,7 +489,9 @@ export const PropertyPanel = memo(function PropertyPanel(props: PropertyPanelPro onCommitAnimatedProperty && void onCommitAnimatedProperty(element, "height", displayH) } - onRemoveKeyframe={(pct) => onRemoveKeyframe?.(animIdForProp("height"), pct)} + onRemoveKeyframe={(pct, animationId) => + onRemoveKeyframe?.(animationId ?? animIdForProp("height"), pct) + } onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("height"))} /> )} @@ -507,7 +515,9 @@ export const PropertyPanel = memo(function PropertyPanel(props: PropertyPanelPro onCommitAnimatedProperty && void onCommitAnimatedProperty(element, "rotation", displayR) } - onRemoveKeyframe={(pct) => onRemoveKeyframe?.(animIdForProp("rotation"), pct)} + onRemoveKeyframe={(pct, animationId) => + onRemoveKeyframe?.(animationId ?? animIdForProp("rotation"), pct) + } onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("rotation"))} /> )} diff --git a/packages/studio/src/components/editor/propertyPanelFlatLayoutSection.tsx b/packages/studio/src/components/editor/propertyPanelFlatLayoutSection.tsx index da217814f..a9c256a00 100644 --- a/packages/studio/src/components/editor/propertyPanelFlatLayoutSection.tsx +++ b/packages/studio/src/components/editor/propertyPanelFlatLayoutSection.tsx @@ -83,10 +83,10 @@ function KeyframeGutter({ track("button", `Add ${property} keyframe`); void onCommitAnimatedProperty(element, property, displayValue); }} - onRemoveKeyframe={(pct) => { + onRemoveKeyframe={(pct, animationId) => { if (!onRemoveKeyframe) return; track("button", `Remove ${property} keyframe`); - onRemoveKeyframe(animIdForProp(property), pct); + onRemoveKeyframe(animationId ?? animIdForProp(property), pct); }} onConvertToKeyframes={() => { if (!onConvertToKeyframes) return; diff --git a/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx b/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx index 1589f7061..cbb35e90b 100644 --- a/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx +++ b/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx @@ -184,7 +184,7 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => { view.unmount(); }); - it("settles false for a boundary drag while the tween is still flat", async () => { + it("retimes a flat tween's boundary through update-meta, not the keyframe writer", async () => { const view = renderCallbacks(); await expect( @@ -198,9 +198,16 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => { }, 25, ), - ).resolves.toBe(false); + ).resolves.toBe(true); + // The start boundary moved to 0.25s; the end stays put, so the window is 0.75s. + expect(mocks.actions.handleGsapUpdateMeta).toHaveBeenCalledWith( + flatAnimation.id, + { position: 0.25, duration: 0.75 }, + mocks.selection, + ); expect(mocks.actions.handleGsapMoveKeyframe).not.toHaveBeenCalled(); + // resize-keyframed-tween would convert the flat tween to keyframes form. expect(mocks.actions.handleGsapResizeKeyframedTween).not.toHaveBeenCalled(); view.unmount(); }); diff --git a/packages/studio/src/components/nle/useTimelineEditCallbacks.ts b/packages/studio/src/components/nle/useTimelineEditCallbacks.ts index 050b37339..7ca47e0bf 100644 --- a/packages/studio/src/components/nle/useTimelineEditCallbacks.ts +++ b/packages/studio/src/components/nle/useTimelineEditCallbacks.ts @@ -256,10 +256,6 @@ export function useTimelineEditCallbacks({ const anim = animations.find((a) => a.id === target.animId); const tweenStart = anim ? resolveTweenStart(anim) : null; if (!anim || tweenStart === null) return Promise.resolve(false); - // Synthesized flat endpoints are clip boundaries, not authored keyframes. - // Boundary-to-clip resize wiring is intentionally deferred; ignore the - // drag rather than dispatching a free keyframe move that cannot be written. - if (!anim.keyframes) return Promise.resolve(false); const sourceFile = sel.sourceFile || activeCompPath || "index.html"; const { elements, domClipChildren } = usePlayerStore.getState(); const { elStart, elDuration } = resolveClipTimingBasis( @@ -285,6 +281,19 @@ export function useTimelineEditCallbacks({ decision.position != null && decision.duration != null ) { + // An empty remap means a FLAT tween's synthesized boundary: there is no + // keyframe node to re-key, only the window to move. Sending it through + // the keyframed-resize writer would rewrite the authored flat tween into + // keyframes form as a side effect of a pure position/duration change, so + // dispatch update-meta and leave the tween as the author wrote it. + if (decision.pctRemap.length === 0) { + handleGsapUpdateMeta( + target.animId, + { position: decision.position, duration: decision.duration }, + sel, + ); + return true; + } return handleGsapResizeKeyframedTween( target.animId, decision.position,