From ba8df2661c4af74897c734ad6376a0b1dc3fd5e2 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Sat, 25 Jul 2026 23:09:13 +0200 Subject: [PATCH] fix(studio): resolve keyframe fallbacks against the clicked element Three review follow-ups on the editor-callback consolidation. The keyframe-target resolve now takes the clicked element's key and reads that element's keyframe cache. The diamond context menu and move-to-playhead pass no explicit target, so they fell through to the cache of whatever element happened to be selected: opening the menu on a non-selected element's diamond resolved against the wrong keyframes. PropertyPanelFlat opens the Motion group by adjusting state during render instead of in an effect, so the AnimationCard mounts on the same commit the focus request arrives on rather than a frame later. Both animation sections pass a module-level focus consumer instead of a fresh inline arrow, so AnimationCard's focus effect stops re-running on every parent render. --- .../editor/GsapAnimationSection.tsx | 4 +- .../components/editor/PropertyPanelFlat.tsx | 26 +++++---- .../editor/gsapAnimationCallbacks.ts | 10 ++++ .../editor/propertyPanelFlatMotionSection.tsx | 4 +- .../nle/useTimelineEditCallbacks.test.tsx | 53 +++++++++++++++++++ .../nle/useTimelineEditCallbacks.ts | 21 ++++++-- 6 files changed, 99 insertions(+), 19 deletions(-) diff --git a/packages/studio/src/components/editor/GsapAnimationSection.tsx b/packages/studio/src/components/editor/GsapAnimationSection.tsx index 1fac3972e..2ced7b6a8 100644 --- a/packages/studio/src/components/editor/GsapAnimationSection.tsx +++ b/packages/studio/src/components/editor/GsapAnimationSection.tsx @@ -6,6 +6,7 @@ import { AnimationCard } from "./AnimationCard"; import { type GsapAnimationEditCallbacks, withTrackedGsapAnimationCallbacks, + clearFocusedEaseSegment, } from "./gsapAnimationCallbacks"; import { useTrackDesignInput } from "../../contexts/DesignPanelInputContext"; import { usePlayerStore } from "../../player"; @@ -29,7 +30,6 @@ export const GsapAnimationSection = memo(function GsapAnimationSection({ const [addMenuOpen, setAddMenuOpen] = useState(false); const trackedCallbacks = withTrackedGsapAnimationCallbacks(callbacks, track); const focusedEaseSegment = usePlayerStore((s) => s.focusedEaseSegment); - const setFocusedEaseSegment = usePlayerStore((s) => s.setFocusedEaseSegment); return (
}> @@ -57,7 +57,7 @@ export const GsapAnimationSection = memo(function GsapAnimationSection({ focusedSegment={ focusedEaseSegment?.animationId === anim.id ? focusedEaseSegment : null } - onFocusSegmentConsumed={() => setFocusedEaseSegment(null)} + onFocusSegmentConsumed={clearFocusedEaseSegment} /> ))} diff --git a/packages/studio/src/components/editor/PropertyPanelFlat.tsx b/packages/studio/src/components/editor/PropertyPanelFlat.tsx index 86900c126..894713523 100644 --- a/packages/studio/src/components/editor/PropertyPanelFlat.tsx +++ b/packages/studio/src/components/editor/PropertyPanelFlat.tsx @@ -249,18 +249,22 @@ export function PropertyPanelFlat({ // force the Motion group open so its AnimationCard (which only mounts while // the group is expanded) can consume the focus and reveal the ease editor. const focusedEaseSegment = usePlayerStore((s) => s.focusedEaseSegment); - // Identity of the element THIS panel actually renders (not the store's - // selectedElementId, which flips synchronously on selection while the panel - // still renders the previous element during async DOM-selection resolution): - // a stale panel would otherwise consume a focus request meant for its - // successor when both share a class-selector animation id. + // The element THIS panel renders, not the store's selectedElementId: that + // flips synchronously while the panel still renders its predecessor, so a + // stale panel would consume a request meant for its successor whenever the + // two share a class-selector animation id. const renderedElementId = `${element.sourceFile}#${element.id}`; - useEffect(() => { - if (!focusedEaseSegment || focusedEaseSegment.elementId !== renderedElementId) return; - if (gsapAnimations.some((a) => a.id === focusedEaseSegment.animationId)) { - setOpenGroupId("motion"); - } - }, [focusedEaseSegment, gsapAnimations, renderedElementId]); + // Adjusted during render (not an effect) so the card mounts on the same + // commit the request lands on. Keyed on request identity: a group the user + // closes afterwards stays closed. + const [consumedFocus, setConsumedFocus] = useState(focusedEaseSegment); + if (focusedEaseSegment !== consumedFocus) { + setConsumedFocus(focusedEaseSegment); + const focusesThisPanel = + focusedEaseSegment?.elementId === renderedElementId && + gsapAnimations.some((a) => a.id === focusedEaseSegment.animationId); + if (focusesThisPanel) setOpenGroupId("motion"); + } const [justToggledIds, setJustToggledIds] = useState([]); const justToggledTimeoutRef = useRef | null>(null); diff --git a/packages/studio/src/components/editor/gsapAnimationCallbacks.ts b/packages/studio/src/components/editor/gsapAnimationCallbacks.ts index d35cf4479..bc5d78356 100644 --- a/packages/studio/src/components/editor/gsapAnimationCallbacks.ts +++ b/packages/studio/src/components/editor/gsapAnimationCallbacks.ts @@ -1,4 +1,5 @@ import type { ArcPathSegment } from "@hyperframes/parsers/gsap-parser"; +import { usePlayerStore } from "../../player"; /** * Edit callbacks shared by GsapAnimationSection and each AnimationCard it @@ -155,3 +156,12 @@ export function withTrackedGsapAnimationCallbacks( : undefined, }; } + +/** + * Stable consumer for the store's one-shot ease-focus request. Module-level on + * purpose: an inline arrow in the section components is a dep of AnimationCard's + * focus effect, so a fresh identity each render re-runs that effect every render. + */ +export function clearFocusedEaseSegment(): void { + usePlayerStore.getState().setFocusedEaseSegment(null); +} diff --git a/packages/studio/src/components/editor/propertyPanelFlatMotionSection.tsx b/packages/studio/src/components/editor/propertyPanelFlatMotionSection.tsx index c8edf195b..4823a995a 100644 --- a/packages/studio/src/components/editor/propertyPanelFlatMotionSection.tsx +++ b/packages/studio/src/components/editor/propertyPanelFlatMotionSection.tsx @@ -9,6 +9,7 @@ import { AnimationCard } from "./AnimationCard"; import { type GsapAnimationEditCallbacks, withTrackedGsapAnimationCallbacks, + clearFocusedEaseSegment, } from "./gsapAnimationCallbacks"; import { deriveElementTiming } from "./propertyPanelFlatTimingDerivation"; import { usePlayerStore } from "../../player"; @@ -138,7 +139,6 @@ export function FlatMotionSection({ const [addMenuOpen, setAddMenuOpen] = useState(false); const trackedCallbacks = withTrackedGsapAnimationCallbacks(callbacks, track); const focusedEaseSegment = usePlayerStore((s) => s.focusedEaseSegment); - const setFocusedEaseSegment = usePlayerStore((s) => s.setFocusedEaseSegment); // Only consume a focus request aimed at the element THIS panel renders (not // the store's selectedElementId, which flips synchronously during async // selection resolution), so a shared class-selector animation id can't open @@ -182,7 +182,7 @@ export function FlatMotionSection({ defaultExpanded={index === 0} flat focusedSegment={focusedHere?.animationId === anim.id ? focusedHere : null} - onFocusSegmentConsumed={() => setFocusedEaseSegment(null)} + onFocusSegmentConsumed={clearFocusedEaseSegment} /> ))} { view.unmount(); }); + // The diamond context menu opens on whatever diamond was clicked, which need + // not belong to the selected element, and it passes no explicit target — so + // the resolve falls back to the cache. Reading the SELECTED element's cache + // there resolves against the wrong element's keyframes. + it("resolves a cache fallback against the clicked element, not the selected one", () => { + const circle: TimelineElement = { + ...element, + id: "circle", + key: "scenes/main.html#circle", + domId: "circle", + }; + usePlayerStore.setState({ + elements: [element, circle], + gsapAnimations: new Map([["scenes/main.html#circle", [otherKeyframedAnimation]]]), + keyframeCache: new Map([ + // Decoy at the same clip-% under the selected element's key. + [ + "box", + { + format: "percentage", + keyframes: [{ percentage: 100, tweenPercentage: 100, properties: { x: 420 } }], + }, + ], + [ + "scenes/main.html#circle", + { + format: "percentage", + keyframes: [ + { + percentage: 100, + tweenPercentage: 100, + propertyGroup: "position", + animationId: otherKeyframedAnimation.id, + properties: { x: 420 }, + }, + ], + }, + ], + ]), + }); + const view = renderCallbacks(); + + act(() => { + view.callbacks.onMoveKeyframeToPlayhead?.("scenes/main.html#circle", 100); + }); + + expect(mocks.actions.handleGsapMoveKeyframeToPlayhead).toHaveBeenCalledWith( + otherKeyframedAnimation.id, + 100, + ); + view.unmount(); + }); + it("keeps selected-element flat boundary deletion on the animation delete path", () => { const view = renderCallbacks(); diff --git a/packages/studio/src/components/nle/useTimelineEditCallbacks.ts b/packages/studio/src/components/nle/useTimelineEditCallbacks.ts index c800528ae..e72d4bc7b 100644 --- a/packages/studio/src/components/nle/useTimelineEditCallbacks.ts +++ b/packages/studio/src/components/nle/useTimelineEditCallbacks.ts @@ -143,12 +143,18 @@ export function useTimelineEditCallbacks({ tweenPercentage?: number, animationId?: string, animations: GsapAnimation[] = selectedGsapAnimations, + elementKey?: string, ): { animId: string; tweenPct: number } | null => { const explicitTarget = propertyGroup !== undefined || tweenPercentage !== undefined || animationId !== undefined ? [{ percentage: pct, propertyGroup, tweenPercentage, animationId }] : undefined; - const cached = usePlayerStore.getState().keyframeCache.get(domEditSelection?.id ?? ""); + // The clicked element's own cache when the caller knows it: the diamond + // context menu can open on an element that is not the selected one, and + // reading the selection's cache there resolves against the wrong element. + const cached = usePlayerStore + .getState() + .keyframeCache.get(elementKey ?? domEditSelection?.id ?? ""); return resolveTimelineKeyframeTarget( pct, explicitTarget ?? cached?.keyframes ?? [], @@ -198,7 +204,7 @@ export function useTimelineEditCallbacks({ }, onDeleteKeyframe: (elId, pct, group, tweenPct, animationId) => { const animations = resolveElementAnimations(elId); - const target = resolveKeyframeTarget(pct, group, tweenPct, animationId, animations); + const target = resolveKeyframeTarget(pct, group, tweenPct, animationId, animations, elId); if (!target) return; const element = usePlayerStore.getState().elements.find((el) => (el.key ?? el.id) === elId); if (!element) { @@ -213,8 +219,15 @@ export function useTimelineEditCallbacks({ }); }, // Retime the keyframe to the playhead, preserving its value + ease. - onMoveKeyframeToPlayhead: (_elId, pct, group, tweenPct, animationId) => { - const target = resolveKeyframeTarget(pct, group, tweenPct, animationId); + onMoveKeyframeToPlayhead: (elId, pct, group, tweenPct, animationId) => { + const target = resolveKeyframeTarget( + pct, + group, + tweenPct, + animationId, + resolveElementAnimations(elId), + elId, + ); if (target) handleGsapMoveKeyframeToPlayhead(target.animId, target.tweenPct); }, // Drag-to-retime. The diamond reports clip-%s; resolveKeyframeTarget gives