From 6b58678d94795d70a33870aabc4e1af9ef70549d Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Tue, 28 Jul 2026 00:37:31 +0200 Subject: [PATCH] fix(studio): drop dead keyframe menu wiring and close the R1 review items The diamond context menu still declared `onChangeEase` and `onCopyProperties` props, and `TimelineOverlays` still threaded `onChangeKeyframeEase` plus a `keyframeCache` it never read. Nothing on any timeline branch calls them, so they are removed along with the `onChangeKeyframeEase` callback implementation. Also from review: - `deleteSelectedKeyframes` only falls back to the sole keyframed animation when there is exactly one. A collapsed selection key carries no animation id, so taking the first of several deleted an arbitrary tween's keyframe. - The duration-less retime test asserts the real 87.601% instead of `expect.any(Number)`, so a wrong timing basis fails it. - `Timeline` wires the keyframe handlers' `onSelectSegment` through to the diamonds; it was built and then dropped, so segment ease selection never fired. - The flat text section arms auto-focus in state rather than reading and clearing a ref during render, which Strict Mode's double render swallowed. --- .../editor/propertyPanelFlatTextSection.tsx | 21 ++++++++++--------- .../nle/useTimelineEditCallbacks.test.tsx | 6 +++++- .../nle/useTimelineEditCallbacks.ts | 14 ------------- .../src/contexts/TimelineEditContext.tsx | 1 - .../src/hooks/deleteSelectedKeyframes.ts | 5 ++++- .../components/KeyframeDiamondContextMenu.tsx | 2 -- .../studio/src/player/components/Timeline.tsx | 6 ++---- .../src/player/components/TimelineLanes.tsx | 4 ++++ .../player/components/TimelineOverlays.tsx | 14 +------------ .../player/components/timelineCallbacks.ts | 1 - 10 files changed, 27 insertions(+), 47 deletions(-) diff --git a/packages/studio/src/components/editor/propertyPanelFlatTextSection.tsx b/packages/studio/src/components/editor/propertyPanelFlatTextSection.tsx index a85670abe..954aaec9b 100644 --- a/packages/studio/src/components/editor/propertyPanelFlatTextSection.tsx +++ b/packages/studio/src/components/editor/propertyPanelFlatTextSection.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from "react"; +import { useEffect, useState } from "react"; import { useTrackDesignInput } from "../../contexts/DesignPanelInputContext"; import { Plus, X } from "../../icons/SystemIcons"; import { isTextEditableSelection, type DomEditSelection } from "./domEditing"; @@ -257,10 +257,12 @@ export function FlatTextSection({ const [activeFieldKey, setActiveFieldKey] = useState( element.textFields[0]?.key ?? null, ); - // Armed by the add handler, read and cleared by the render that first shows - // the new field. A ref rather than state: the marker only has to survive one - // render, and clearing it afterwards would be a state-syncing effect. - const autoFocusFieldKeyRef = useRef(null); + // Armed by the add handler so the newly added field mounts focused. State, not + // a ref cleared during render: Strict Mode renders twice, so the first pass + // would eat the marker and the second would mount the field unfocused. Nothing + // clears it on read either — `autoFocus` is a mount-only DOM prop and the + // editor is keyed on the field, so it can only fire once per added field. + const [autoFocusFieldKey, setAutoFocusFieldKey] = useState(null); useEffect(() => { const nextFields = element.textFields; @@ -275,8 +277,7 @@ export function FlatTextSection({ const activeField = textFields.find((field) => field.key === activeFieldKey) ?? textFields[0]; if (!activeField) return null; - const autoFocusActiveField = autoFocusFieldKeyRef.current === activeField.key; - if (autoFocusActiveField) autoFocusFieldKeyRef.current = null; + const autoFocusActiveField = autoFocusFieldKey === activeField.key; if (textFields.length > 1) { return ( @@ -286,13 +287,13 @@ export function FlatTextSection({ activeFieldKey={activeField.key} styles={styles} onSelect={(fieldKey) => { - autoFocusFieldKeyRef.current = null; + setAutoFocusFieldKey(null); setActiveFieldKey(fieldKey); }} onAdd={() => void Promise.resolve(onAddTextField(activeField.key)).then((nextKey) => { if (!nextKey) return; - autoFocusFieldKeyRef.current = nextKey; + setAutoFocusFieldKey(nextKey); setActiveFieldKey(nextKey); }) } @@ -330,7 +331,7 @@ export function FlatTextSection({ track("button", "Add text field"); void Promise.resolve(onAddTextField(activeField.key)).then((nextKey) => { if (!nextKey) return; - autoFocusFieldKeyRef.current = nextKey; + setAutoFocusFieldKey(nextKey); setActiveFieldKey(nextKey); }); }} diff --git a/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx b/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx index 84ca79a70..78aeb1227 100644 --- a/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx +++ b/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx @@ -512,10 +512,14 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => { ), ).resolves.toBe(true); + // The whole point of the clip basis: the drop lands at 10.94 + 0.40 * 16.26 = + // 17.444s, and the duration-less tween borrows the clip's 16.26s window from + // its 3.2s start, so 17.444 - 3.2 over 16.26 is 87.601%. Any other basis (a + // zero-length tween, or the clip's own 0-100 %) produces a different number. expect(mocks.actions.handleGsapMoveKeyframe).toHaveBeenCalledWith( durationless.id, 50, - expect.any(Number), + expect.closeTo(87.601, 3), mocks.selection, ); expect(mocks.actions.handleGsapResizeKeyframedTween).not.toHaveBeenCalled(); diff --git a/packages/studio/src/components/nle/useTimelineEditCallbacks.ts b/packages/studio/src/components/nle/useTimelineEditCallbacks.ts index f20fa36b3..3173efcfc 100644 --- a/packages/studio/src/components/nle/useTimelineEditCallbacks.ts +++ b/packages/studio/src/components/nle/useTimelineEditCallbacks.ts @@ -301,20 +301,6 @@ export function useTimelineEditCallbacks({ } return Promise.resolve(false); }, - onChangeKeyframeEase: (elId: string, _pct: number, ease: string) => { - // The edited element's own animations + selection, not the selection's: - // an ease change on a non-selected lane otherwise rewrote whichever - // element happened to be selected, in whichever file it lives. - const animations = resolveElementAnimations(elId); - const element = usePlayerStore.getState().elements.find((el) => (el.key ?? el.id) === elId); - if (!element) return; - void buildDomSelectionForTimelineElement(element).then((selection) => { - if (!selection) return; - for (const anim of animations) { - if (anim.keyframes) handleGsapUpdateMeta(anim.id, { ease }, selection); - } - }); - }, // fallow-ignore-next-line complexity onToggleKeyframeAtPlayhead: (el: TimelineElement) => { const currentTime = usePlayerStore.getState().currentTime; diff --git a/packages/studio/src/contexts/TimelineEditContext.tsx b/packages/studio/src/contexts/TimelineEditContext.tsx index be7f4e9ad..ead14fa77 100644 --- a/packages/studio/src/contexts/TimelineEditContext.tsx +++ b/packages/studio/src/contexts/TimelineEditContext.tsx @@ -40,7 +40,6 @@ export function TimelineEditProvider({ value.onRazorSplitAll, value.onDeleteKeyframe, value.onDeleteAllKeyframes, - value.onChangeKeyframeEase, value.onMoveKeyframeToPlayhead, value.onMoveKeyframe, value.onToggleKeyframeAtPlayhead, diff --git a/packages/studio/src/hooks/deleteSelectedKeyframes.ts b/packages/studio/src/hooks/deleteSelectedKeyframes.ts index 79342aa04..8a0e4e4d2 100644 --- a/packages/studio/src/hooks/deleteSelectedKeyframes.ts +++ b/packages/studio/src/hooks/deleteSelectedKeyframes.ts @@ -20,7 +20,10 @@ export function deleteSelectedKeyframes(session: { const { selectedKeyframes, selectedElementId } = usePlayerStore.getState(); if (!selectedElementId) return; const keyframedAnimations = session.selectedGsapAnimations.filter((anim) => anim.keyframes); - const fallbackAnimation = keyframedAnimations[0]; + // A collapsed selection key (an ungrouped animation) carries no animation id, + // so it only resolves when there is exactly one keyframed animation it could + // mean. Taking the first of several deletes an arbitrary tween's keyframe. + const fallbackAnimation = keyframedAnimations.length === 1 ? keyframedAnimations[0] : undefined; const animationsById = new Map(keyframedAnimations.map((animation) => [animation.id, animation])); const removals = new Map(); for (const key of selectedKeyframes) { diff --git a/packages/studio/src/player/components/KeyframeDiamondContextMenu.tsx b/packages/studio/src/player/components/KeyframeDiamondContextMenu.tsx index 9dc70df9d..6e1b94ff6 100644 --- a/packages/studio/src/player/components/KeyframeDiamondContextMenu.tsx +++ b/packages/studio/src/player/components/KeyframeDiamondContextMenu.tsx @@ -21,8 +21,6 @@ interface KeyframeDiamondContextMenuProps { onClose: () => void; onDelete: (elementId: string, target: TimelineKeyframeTarget) => void; onDeleteAll: (element: TimelineElement) => 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?: (element: TimelineElement, target: TimelineKeyframeTarget) => void; } diff --git a/packages/studio/src/player/components/Timeline.tsx b/packages/studio/src/player/components/Timeline.tsx index 38bef2479..dad0c6f36 100644 --- a/packages/studio/src/player/components/Timeline.tsx +++ b/packages/studio/src/player/components/Timeline.tsx @@ -75,7 +75,6 @@ export const Timeline = memo(function Timeline({ onRazorSplitAll, onDeleteKeyframe, onDeleteAllKeyframes, - onChangeKeyframeEase, onMoveKeyframeToPlayhead, onMoveKeyframe, } = useResolvedTimelineEditCallbacks({ @@ -263,7 +262,7 @@ export const Timeline = memo(function Timeline({ const selectedKeyframes = usePlayerStore((s) => s.selectedKeyframes); const toggleSelectedKeyframe = usePlayerStore((s) => s.toggleSelectedKeyframe); - const { onClickKeyframe, onShiftClickKeyframe, onContextMenuKeyframe } = + const { onClickKeyframe, onSelectSegment, onShiftClickKeyframe, onContextMenuKeyframe } = useTimelineKeyframeHandlers({ expandedElements, keyframeCache, @@ -509,6 +508,7 @@ export const Timeline = memo(function Timeline({ currentTime={currentTime} beatAnalysis={adjustedBeatAnalysis} onClickKeyframe={onClickKeyframe} + onSelectSegment={onSelectSegment} onShiftClickKeyframe={onShiftClickKeyframe} onMoveKeyframe={onMoveKeyframe} onContextMenuKeyframe={onContextMenuKeyframe} @@ -547,9 +547,7 @@ export const Timeline = memo(function Timeline({ setKfContextMenu={setKfContextMenu} onDeleteKeyframe={onDeleteKeyframe} onDeleteAllKeyframes={onDeleteAllKeyframes} - onChangeKeyframeEase={onChangeKeyframeEase} onMoveKeyframeToPlayhead={onMoveKeyframeToPlayhead} - keyframeCache={keyframeCache} clipContextMenu={clipContextMenu} setClipContextMenu={setClipContextMenu} currentTime={currentTime} diff --git a/packages/studio/src/player/components/TimelineLanes.tsx b/packages/studio/src/player/components/TimelineLanes.tsx index 4ee3afc53..eb9775872 100644 --- a/packages/studio/src/player/components/TimelineLanes.tsx +++ b/packages/studio/src/player/components/TimelineLanes.tsx @@ -74,6 +74,8 @@ export interface TimelineLaneBaseProps { currentTime: number; onClickKeyframe?: (element: TimelineElement, keyframe: TimelineKeyframeTarget) => void; onShiftClickKeyframe?: (elementId: string, keyframe: TimelineKeyframeTarget) => void; + /** Click on the segment BETWEEN two diamonds: selects it to edit its ease. */ + onSelectSegment?: (elementId: string, keyframe: TimelineKeyframeTarget) => void; onContextMenuKeyframe?: ( e: React.MouseEvent, elementId: string, @@ -141,6 +143,7 @@ export function TimelineLanes({ currentTime, onClickKeyframe, onShiftClickKeyframe, + onSelectSegment, onContextMenuKeyframe, onMoveKeyframe, onContextMenuClip, @@ -482,6 +485,7 @@ export function TimelineLanes({ onClickKeyframe?.(previewElement, keyframe) } onShiftClickKeyframe={onShiftClickKeyframe} + onSelectSegment={onSelectSegment} onContextMenuKeyframe={onContextMenuKeyframe} onMoveKeyframe={onMoveKeyframe} suppressClickRef={suppressClickRef} diff --git a/packages/studio/src/player/components/TimelineOverlays.tsx b/packages/studio/src/player/components/TimelineOverlays.tsx index cfc2527b4..b8d4417b8 100644 --- a/packages/studio/src/player/components/TimelineOverlays.tsx +++ b/packages/studio/src/player/components/TimelineOverlays.tsx @@ -1,4 +1,4 @@ -import type { KeyframeCacheEntry, TimelineElement } from "../store/playerStore"; +import type { TimelineElement } from "../store/playerStore"; import type { TimelineTheme } from "./timelineTheme"; import type { TimelineRangeSelection } from "./timelineEditing"; import type { TimelineEditCallbacks } from "./timelineCallbacks"; @@ -38,9 +38,7 @@ interface TimelineOverlaysProps { setKfContextMenu: (value: KeyframeDiamondContextMenuState | null) => void; onDeleteKeyframe: TimelineEditCallbacks["onDeleteKeyframe"]; onDeleteAllKeyframes: TimelineEditCallbacks["onDeleteAllKeyframes"]; - onChangeKeyframeEase: TimelineEditCallbacks["onChangeKeyframeEase"]; onMoveKeyframeToPlayhead: TimelineEditCallbacks["onMoveKeyframeToPlayhead"]; - keyframeCache: Map; clipContextMenu: ClipContextMenuState | null; setClipContextMenu: (value: ClipContextMenuState | null) => void; currentTime: number; @@ -68,9 +66,7 @@ export function TimelineOverlays({ setKfContextMenu, onDeleteKeyframe, onDeleteAllKeyframes, - onChangeKeyframeEase, onMoveKeyframeToPlayhead, - keyframeCache, clipContextMenu, setClipContextMenu, currentTime, @@ -108,19 +104,11 @@ export function TimelineOverlays({ onClose={() => setKfContextMenu(null)} onDelete={(elId, target) => onDeleteKeyframe?.(elId, target)} onDeleteAll={(elId) => onDeleteAllKeyframes?.(elId)} - onChangeEase={(elId, pct, ease) => onChangeKeyframeEase?.(elId, pct, ease)} onMoveToPlayhead={ onMoveKeyframeToPlayhead ? (element, target) => onMoveKeyframeToPlayhead(element, target) : undefined } - onCopyProperties={(elId, pct) => { - const kfData = keyframeCache.get(elId); - const kf = kfData?.keyframes.find((k) => k.percentage === pct); - if (kf) { - void navigator.clipboard.writeText(JSON.stringify(kf.properties, null, 2)); - } - }} /> )} diff --git a/packages/studio/src/player/components/timelineCallbacks.ts b/packages/studio/src/player/components/timelineCallbacks.ts index 969cc0ab3..e4e8db081 100644 --- a/packages/studio/src/player/components/timelineCallbacks.ts +++ b/packages/studio/src/player/components/timelineCallbacks.ts @@ -74,7 +74,6 @@ export interface TimelineEditCallbacks { onRazorSplitAll?: (splitTime: number) => Promise | void; onDeleteKeyframe?: (elementId: string, keyframe: TimelineKeyframeTarget) => void; onDeleteAllKeyframes?: (element: TimelineElement) => void; - onChangeKeyframeEase?: (elementId: string, percentage: number, ease: string) => void; onMoveKeyframeToPlayhead?: (element: TimelineElement, keyframe: TimelineKeyframeTarget) => void; /** Drag-to-retime: `keyframe` identifies the dragged keyframe (its percentage * is clip-relative), `toClipPercentage` is the neighbour-clamped drop. */