From cbdd61d102047c9c7f625e92afee4ab216255d98 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Sun, 26 Jul 2026 01:36:04 +0200 Subject: [PATCH] refactor(studio): give every keyframe diamond one identity The collapsed clip row dropped a keyframe's property group and animation id before handing it to a callback, so the same keyframe hashed to a different selection key than the expanded property lane did. Selecting a diamond in one view left it unselected in the other, and retime/delete on the collapsed row lost the animation id they use to pick between two animations that collide at one percentage. Diamonds now always carry their full identity, the collapsed shim just curries the element id, and Timeline reuses useTimelineKeyframeHandlers instead of its own inline copy of the same three handlers. Neighbour geometry moves into one marker record per diamond, which drops the index-lookup non-null assertions the connector pass needed. --- .../studio/src/player/components/Timeline.tsx | 48 +++------ .../components/TimelineClipDiamonds.test.tsx | 53 ++++++++- .../components/TimelineClipDiamonds.tsx | 101 ++++++++++-------- .../src/player/components/TimelineLanes.tsx | 14 ++- .../components/TimelinePropertyLanes.test.tsx | 5 +- 5 files changed, 136 insertions(+), 85 deletions(-) diff --git a/packages/studio/src/player/components/Timeline.tsx b/packages/studio/src/player/components/Timeline.tsx index 037b0bac1..38bef2479 100644 --- a/packages/studio/src/player/components/Timeline.tsx +++ b/packages/studio/src/player/components/Timeline.tsx @@ -28,6 +28,7 @@ import { useResolvedTimelineEditCallbacks } from "./useResolvedTimelineEditCallb import type { TimelineProps } from "./TimelineTypes"; import { useTrackGapMenu } from "./useTrackGapMenu"; import { useTimelineGapHighlights } from "./useTimelineGapHighlights"; +import { useTimelineKeyframeHandlers } from "./useTimelineKeyframeHandlers"; import { useStudioPlaybackContextOptional } from "../../contexts/StudioContext"; // Re-export pure utilities so existing imports from "./Timeline" still resolve. @@ -262,6 +263,17 @@ export const Timeline = memo(function Timeline({ const selectedKeyframes = usePlayerStore((s) => s.selectedKeyframes); const toggleSelectedKeyframe = usePlayerStore((s) => s.toggleSelectedKeyframe); + const { onClickKeyframe, onShiftClickKeyframe, onContextMenuKeyframe } = + useTimelineKeyframeHandlers({ + expandedElements, + keyframeCache, + onSelectElement, + onSeek, + setSelectedElementId, + setKfContextMenu, + toggleSelectedKeyframe, + }); + const selectedElement = useMemo( () => expandedElements.find((element) => (element.key ?? element.id) === selectedElementId) ?? null, @@ -496,40 +508,10 @@ export const Timeline = memo(function Timeline({ selectedKeyframes={selectedKeyframes} currentTime={currentTime} beatAnalysis={adjustedBeatAnalysis} - onClickKeyframe={(el, pct) => { - usePlayerStore.getState().clearSelectedKeyframes(); - const elKey = el.key ?? el.id; - setSelectedElementId(elKey); - onSelectElement?.(el); - // Select the clicked diamond (matches shift-click); cleared above so this single-selects. - toggleSelectedKeyframe(`${elKey}:${pct}`); - const absTime = el.start + (pct / 100) * el.duration; - onSeek?.(absTime); - const kfData = keyframeCache?.get(elKey); - const kf = kfData?.keyframes.find((k) => Math.abs(k.percentage - pct) < 0.5); - usePlayerStore.getState().setActiveKeyframePct(kf?.tweenPercentage ?? null); - }} - onShiftClickKeyframe={(elId, pct) => { - toggleSelectedKeyframe(`${elId}:${pct}`); - }} + onClickKeyframe={onClickKeyframe} + onShiftClickKeyframe={onShiftClickKeyframe} onMoveKeyframe={onMoveKeyframe} - onContextMenuKeyframe={(e, elId, pct) => { - const el = expandedElements.find((x) => (x.key ?? x.id) === elId); - if (!el) return; - setSelectedElementId(elId); - onSelectElement?.(el); - const kfData = keyframeCache.get(elId); - const kf = kfData?.keyframes.find((k) => Math.abs(k.percentage - pct) < 0.2); - setKfContextMenu({ - x: e.clientX + 4, - y: e.clientY + 2, - element: el, - elementId: elId, - percentage: pct, - tweenPercentage: kf?.tweenPercentage, - currentEase: kf?.ease ?? kfData?.ease, - }); - }} + onContextMenuKeyframe={onContextMenuKeyframe} onContextMenuClip={(e, el) => { e.preventDefault(); setSelectedElementId(el.key ?? el.id); diff --git a/packages/studio/src/player/components/TimelineClipDiamonds.test.tsx b/packages/studio/src/player/components/TimelineClipDiamonds.test.tsx index 2a41e7841..85804deb1 100644 --- a/packages/studio/src/player/components/TimelineClipDiamonds.test.tsx +++ b/packages/studio/src/player/components/TimelineClipDiamonds.test.tsx @@ -4,6 +4,7 @@ import React, { act } from "react"; import { createRoot } from "react-dom/client"; import { afterEach, describe, expect, it, vi } from "vitest"; import { TimelineClipDiamonds, TimelineDiamondLane } from "./TimelineClipDiamonds"; +import { timelineKeyframeSelectionKey } from "./timelineKeyframeIdentity"; (globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; @@ -90,7 +91,57 @@ describe("TimelineClipDiamonds", () => { diamond!.dispatchEvent(pointerEvent("pointerup", { bubbles: true, button: 0 })); }); - expect(onClickKeyframe).toHaveBeenCalledWith(50); + expect(onClickKeyframe).toHaveBeenCalledWith( + "clip-1", + expect.objectContaining({ percentage: 50 }), + ); + act(() => root.unmount()); + }); + + // The collapsed clip row and the expanded property lanes read the same cache, + // so a keyframe that carries a property group has to hash to the same key in + // both — otherwise collapsing a track silently drops the selection. + it("keys a grouped keyframe the same way collapsed as expanded", () => { + const groupedKeyframe = { + percentage: 50, + tweenPercentage: 25, + propertyGroup: "position", + animationId: "anim-1", + properties: { x: 100 }, + }; + const sharedKey = timelineKeyframeSelectionKey("clip-1", groupedKeyframe); + const host = document.createElement("div"); + document.body.append(host); + const root = createRoot(host); + const onClickKeyframe = vi.fn(); + act(() => { + root.render( + , + ); + }); + const diamond = host.querySelector('button[title="50%"]'); + // Highlighted from the shared key alone (the playhead is off-clip here). + expect(diamond?.querySelector("path:last-child")?.getAttribute("fill")).toBe("#4ba3d2"); + + act(() => { + diamond?.dispatchEvent(pointerEvent("pointerup", { bubbles: true, button: 0 })); + }); + expect(onClickKeyframe).toHaveBeenCalledWith("clip-1", { + percentage: 50, + tweenPercentage: 25, + propertyGroup: "position", + animationId: "anim-1", + }); act(() => root.unmount()); }); diff --git a/packages/studio/src/player/components/TimelineClipDiamonds.tsx b/packages/studio/src/player/components/TimelineClipDiamonds.tsx index 10c58dfcf..cf3c516a1 100644 --- a/packages/studio/src/player/components/TimelineClipDiamonds.tsx +++ b/packages/studio/src/player/components/TimelineClipDiamonds.tsx @@ -44,9 +44,13 @@ interface TimelineClipDiamondsProps { currentPercentage: number; elementId: string; selectedKeyframes: ReadonlySet; - onClickKeyframe?: (percentage: number) => void; - onShiftClickKeyframe?: (elementId: string, percentage: number) => void; - onContextMenuKeyframe?: (e: React.MouseEvent, elementId: string, percentage: number) => void; + onClickKeyframe?: (elementId: string, keyframe: TimelineKeyframeTarget) => void; + onShiftClickKeyframe?: (elementId: string, keyframe: TimelineKeyframeTarget) => void; + onContextMenuKeyframe?: ( + e: React.MouseEvent, + elementId: string, + keyframe: TimelineKeyframeTarget, + ) => void; /** Drag-to-retime: move a keyframe to a new time, preserving its value + ease. * `keyframe` identifies the dragged keyframe (clip-relative percentage plus * whatever animation identity the row carries); `toClipPercentage` is the @@ -105,18 +109,21 @@ type DragState = { cancelled?: boolean; }; -function keyframeTarget( - keyframe: TimelineDiamondKeyframe, - groupAware: boolean, -): TimelineKeyframeTarget { - return groupAware - ? { - percentage: keyframe.percentage, - tweenPercentage: keyframe.tweenPercentage, - propertyGroup: keyframe.propertyGroup, - animationId: keyframe.animationId, - } - : { percentage: keyframe.percentage }; +/** + * The full identity of a diamond, used by every callback and by the selection + * key. Collapsed clip rows and expanded property lanes read the same cache, so + * they must hash a shared keyframe to the same key: dropping the group here for + * the collapsed row would leave a diamond selected in one view and unselected in + * the other, and would strip the animation id the retime/delete mutations use to + * pick between two animations that collide at one percentage. + */ +function keyframeTarget(keyframe: TimelineDiamondKeyframe): TimelineKeyframeTarget { + return { + percentage: keyframe.percentage, + tweenPercentage: keyframe.tweenPercentage, + propertyGroup: keyframe.propertyGroup, + animationId: keyframe.animationId, + }; } export const TimelineDiamondLane = memo(function TimelineDiamondLane({ @@ -216,16 +223,21 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({ // Clip-%s of the sorted keyframes — the neighbour clamp (preview + drop) needs // the whole row to bound the dragged diamond between its immediate siblings. const sortedClipPcts = sorted.map((k) => k.percentage); - const sortedCenterXs = sorted.map((keyframe) => - Math.max(0, Math.min(clipWidthPx, (keyframe.percentage / 100) * clipWidthPx)), - ); - const markerMetrics = sortedCenterXs.map((centerX, index) => { - const previousGap = index > 0 ? centerX - sortedCenterXs[index - 1]! : Infinity; - const nextGap = - index < sortedCenterXs.length - 1 ? sortedCenterXs[index + 1]! - centerX : Infinity; + const centerXOf = (percentage: number) => + Math.max(0, Math.min(clipWidthPx, (percentage / 100) * clipWidthPx)); + // One record per diamond, carrying its own geometry, so the connector and + // button passes below read neighbours as values instead of index lookups. + const markers = sorted.map((keyframe, index) => { + const centerX = centerXOf(keyframe.percentage); + const previous = sorted[index - 1]; + const next = sorted[index + 1]; + const previousGap = previous ? centerX - centerXOf(previous.percentage) : Infinity; + const nextGap = next ? centerXOf(next.percentage) - centerX : Infinity; const nearestGap = Math.max(1, Math.min(previousGap, nextGap)); const hitWidth = Math.min(diamondSize, nearestGap); return { + keyframe, + centerX, hitWidth, visualSize: hitWidth === diamondSize ? diamondSize : Math.max(2, hitWidth - 2), }; @@ -248,25 +260,24 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({ pointerEvents: "none", }} > - {sorted.map((kf, i) => { - if (i === 0) return null; - const prev = sorted[i - 1]!; - const x1 = sortedCenterXs[i - 1]!; - const x2 = sortedCenterXs[i]!; + {markers.map((marker, i) => { + const previous = markers[i - 1]; + if (!previous) return null; + const kf = marker.keyframe; + const x1 = previous.centerX; + const x2 = marker.centerX; if (x2 - x1 < 1) return null; - const connectorLeft = x1 + markerMetrics[i - 1]!.visualSize / 2; - const connectorWidth = - x2 - x1 - markerMetrics[i - 1]!.visualSize / 2 - markerMetrics[i]!.visualSize / 2; - // Group-aware target for the ease button: the segment ease is - // per-keyframe (each keyframe carries its own animationId/tweenPercentage). - // On a merged inline row the button is hidden where the segment is - // ambiguous (two source animations collide at this % with different - // eases; see easeAmbiguous) or the keyframe has no source animation id - // (runtime-scanned) so there is no tween to target. - const target = keyframeTarget(kf, true); + const connectorLeft = x1 + previous.visualSize / 2; + const connectorWidth = x2 - x1 - previous.visualSize / 2 - marker.visualSize / 2; + // The ease button targets one segment, so it needs the keyframe's own + // animationId/tweenPercentage. On a merged inline row the button is + // hidden where the segment is ambiguous (two source animations collide + // at this % with different eases; see easeAmbiguous) or the keyframe has + // no source animation id (runtime-scanned) so there is no tween to target. + const target = keyframeTarget(kf); const ease = kf.ease ?? globalEase; return ( - +
{ - const target = keyframeTarget(kf, groupAware); + {markers.map((marker, i) => { + const kf = marker.keyframe; + const target = keyframeTarget(kf); const kfKey = timelineKeyframeSelectionKey(elementId, target); // While dragging this diamond, render it at the live preview clip-%. const renderPct = preview?.kfKey === kfKey ? preview.clipPct : kf.percentage; @@ -344,7 +356,6 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({ // The 0% diamond's left half lands in the reserved left gutter (the // content origin is inset past the label column, Figma-style) so it stays // fully visible instead of being clipped by the sticky label column. - const marker = markerMetrics[i]!; const leftPx = (renderPct / 100) * clipWidthPx - marker.hitWidth / 2; const isKfSelected = selectedKeyframes.has(kfKey); const atPlayhead = isSelected && Math.abs(kf.percentage - currentPercentage) < 0.5; @@ -565,12 +576,10 @@ export const TimelineClipDiamonds = memo(function TimelineClipDiamonds( props.onClickKeyframe?.(target.percentage)} - onShiftClickKeyframe={(target) => - props.onShiftClickKeyframe?.(props.elementId, target.percentage) - } + onClickKeyframe={(target) => props.onClickKeyframe?.(props.elementId, target)} + onShiftClickKeyframe={(target) => props.onShiftClickKeyframe?.(props.elementId, target)} onContextMenuKeyframe={(e, target) => - props.onContextMenuKeyframe?.(e, props.elementId, target.percentage) + props.onContextMenuKeyframe?.(e, props.elementId, target) } onMoveKeyframe={ props.onMoveKeyframe diff --git a/packages/studio/src/player/components/TimelineLanes.tsx b/packages/studio/src/player/components/TimelineLanes.tsx index 8437bb64e..4ee3afc53 100644 --- a/packages/studio/src/player/components/TimelineLanes.tsx +++ b/packages/studio/src/player/components/TimelineLanes.tsx @@ -72,9 +72,13 @@ export interface TimelineLaneBaseProps { keyframeCache?: Map; selectedKeyframes: Set; currentTime: number; - onClickKeyframe?: (element: TimelineElement, percentage: number) => void; - onShiftClickKeyframe?: (elementId: string, percentage: number) => void; - onContextMenuKeyframe?: (e: React.MouseEvent, elementId: string, percentage: number) => void; + onClickKeyframe?: (element: TimelineElement, keyframe: TimelineKeyframeTarget) => void; + onShiftClickKeyframe?: (elementId: string, keyframe: TimelineKeyframeTarget) => void; + onContextMenuKeyframe?: ( + e: React.MouseEvent, + elementId: string, + keyframe: TimelineKeyframeTarget, + ) => void; onMoveKeyframe?: ( elementId: string, keyframe: TimelineKeyframeTarget, @@ -474,7 +478,9 @@ export function TimelineLanes({ } elementId={elementKey} selectedKeyframes={selectedKeyframes} - onClickKeyframe={(pct) => onClickKeyframe?.(previewElement, pct)} + onClickKeyframe={(_elId, keyframe) => + onClickKeyframe?.(previewElement, keyframe) + } onShiftClickKeyframe={onShiftClickKeyframe} onContextMenuKeyframe={onContextMenuKeyframe} onMoveKeyframe={onMoveKeyframe} diff --git a/packages/studio/src/player/components/TimelinePropertyLanes.test.tsx b/packages/studio/src/player/components/TimelinePropertyLanes.test.tsx index 200f25d8c..387d84567 100644 --- a/packages/studio/src/player/components/TimelinePropertyLanes.test.tsx +++ b/packages/studio/src/player/components/TimelinePropertyLanes.test.tsx @@ -424,7 +424,10 @@ describe("TimelinePropertyLanes", () => { act(() => { diamonds[1]?.dispatchEvent(new MouseEvent("pointerup", { bubbles: true, button: 0 })); }); - expect(onClickKeyframe).toHaveBeenCalledWith(50); + expect(onClickKeyframe).toHaveBeenCalledWith( + "clip-1", + expect.objectContaining({ percentage: 50 }), + ); act(() => root.unmount()); }); });