From b155ed46b63989e6e77e2186e6ed1bfc184fc200 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 9 Jul 2026 12:58:00 -0700 Subject: [PATCH] fix(studio): reconcile keyframe-seek timing basis between flat Layout and Motion groups Whole-plan coherence review (Plan 3a Layout + Plan 3b Motion) found that Layout's keyframe gutter and Motion's Timing row independently derived an element's start/duration and disagreed whenever an element had animations but no explicit data-duration: Motion correctly inferred the range from the element's GSAP tweens, while Layout's keyframe gutter fell back to a naive `duration ?? 1`, so clicking a keyframe percentage in Layout could seek to a different absolute time than what Motion's Timing row displayed. Extract deriveElementTiming (propertyPanelFlatTimingDerivation.ts) as the single shared basis both paths now consume: FlatTimingRow (Motion) and PropertyPanelFlat's own elStart/elDuration (Layout's keyframe gutter and 3D Transform block). PropertyPanelFlat now recomputes this basis itself from its own element/gsapAnimations props instead of trusting the parent's naive value, so PropertyPanel.tsx (and its legacy non-flat panel) is untouched. Co-Authored-By: Claude Sonnet 5 --- .../components/editor/PropertyPanel.test.tsx | 96 +++++++++++++++++++ .../components/editor/PropertyPanelFlat.tsx | 15 ++- .../editor/propertyPanelFlatMotionSection.tsx | 26 +---- .../propertyPanelFlatTimingDerivation.test.ts | 71 ++++++++++++++ .../propertyPanelFlatTimingDerivation.ts | 59 ++++++++++++ 5 files changed, 241 insertions(+), 26 deletions(-) create mode 100644 packages/studio/src/components/editor/propertyPanelFlatTimingDerivation.test.ts create mode 100644 packages/studio/src/components/editor/propertyPanelFlatTimingDerivation.ts diff --git a/packages/studio/src/components/editor/PropertyPanel.test.tsx b/packages/studio/src/components/editor/PropertyPanel.test.tsx index a86621c76..27c1febc8 100644 --- a/packages/studio/src/components/editor/PropertyPanel.test.tsx +++ b/packages/studio/src/components/editor/PropertyPanel.test.tsx @@ -143,6 +143,40 @@ function animatedElement() { }; } +// Inferred-timing fixture (whole-plan coherence fix): NO explicit data-start +// or data-duration — sections.timing must turn on via animationCount (fed +// from gsapAnimations.length), not an authored attribute, so both the Motion +// Timing row and the Layout keyframe gutter are forced to infer the range +// from the element's own GSAP tween instead of reading it off an attribute. +function inferredMotionElement() { + return { + ...baseElement(), + id: "inferred-anim", + selector: "#inferred-anim", + label: "Inferred Anim", + }; +} + +// A single "to" tween running from t=2 to t=5 (position 2, duration 3), with +// keyframes on "x" at 0/50/100% — enough to drive both FlatTimingRow's +// inference and the Layout "x" row's keyframe-seek gutter. +const INFERRED_TIMING_ANIMATION = { + id: "a1", + targetSelector: "#inferred-anim", + method: "to", + position: 2, + duration: 3, + properties: { x: 100 }, + keyframes: { + format: "percentage", + keyframes: [ + { percentage: 0, properties: { x: 0 } }, + { percentage: 50, properties: { x: 50 } }, + { percentage: 100, properties: { x: 100 } }, + ], + }, +} as never; + async function renderPanel( flatEnabled: boolean, elementOverride: ReturnType = baseElement(), @@ -418,3 +452,65 @@ describe("PropertyPanel — Motion group (Plan 3b)", () => { RENDER_TIMEOUT_MS, ); }); + +// Whole-plan coherence fix: Layout's keyframe-seek basis and Motion's Timing +// row basis must agree on the same start/duration for an element that has +// animations but no explicit data-duration — before the fix, Layout fell back +// to a naive `duration ?? 1` while Motion correctly inferred the range from +// the tween (position 2, duration 3 -> start 2 / duration 3 / end 5). +describe("PropertyPanel — flat Layout/Motion timing agreement (whole-plan coherence fix)", () => { + it( + "Motion's Timing row shows the inferred start/end/duration for an element with animations but no explicit duration", + async () => { + const { host, root } = await renderPanel(true, inferredMotionElement(), { + gsapAnimations: [INFERRED_TIMING_ANIMATION], + }); + openFlatGroup(host, "Motion"); + const motionGroup = host.querySelector('[data-flat-group-open="true"]'); + if (!motionGroup) throw new Error("expected the Motion group to be open"); + expect(motionGroup.textContent).toContain("Inferred"); + const inputs = motionGroup.querySelectorAll("input"); + // FlatTimingRow renders Start, End, Duration in that order. + expect(inputs[0]?.value).toBe("2.00s"); + expect(inputs[1]?.value).toBe("5.00s"); + expect(inputs[2]?.value).toBe("3.00s"); + act(() => root.unmount()); + }, + RENDER_TIMEOUT_MS, + ); + + it( + "Layout's X-row keyframe gutter seeks to the SAME absolute time Motion's Timing row shows as the midpoint (50% of an inferred 2s-5s range = 3.5s)", + async () => { + const onSeekToTime = vi.fn(); + const { host, root } = await renderPanel(true, inferredMotionElement(), { + gsapAnimations: [INFERRED_TIMING_ANIMATION], + onSeekToTime, + }); + openFlatGroup(host, "Layout"); + const layoutGroup = host.querySelector('[data-flat-group-open="true"]'); + if (!layoutGroup) throw new Error("expected the Layout group to be open"); + + const xRow = Array.from(layoutGroup.querySelectorAll(".group")).find( + (el) => el.querySelector("span")?.textContent === "X", + ); + if (!xRow) throw new Error("expected an X row"); + const gutter = xRow.querySelector('[data-flat-kf-gutter="true"]'); + if (!gutter) throw new Error("expected a keyframe gutter on the X row"); + // The diamond button always carries a `title`; the two plain arrow + // buttons don't. At currentPct=0 with keyframes at 0/50/100%, the prev + // arrow is disabled (no earlier keyframe) and the next arrow seeks to + // the 50% keyframe — exactly the case the coherence bug affected. + const nextArrow = Array.from(gutter.querySelectorAll("button")).find( + (b) => !b.title && !b.disabled, + ); + if (!nextArrow) throw new Error("expected an enabled next-keyframe arrow button"); + act(() => nextArrow.dispatchEvent(new MouseEvent("click", { bubbles: true }))); + + // Same basis as the Timing row: start 2 + 50% * duration 3 = 3.5. + expect(onSeekToTime).toHaveBeenCalledWith(3.5); + act(() => root.unmount()); + }, + RENDER_TIMEOUT_MS, + ); +}); diff --git a/packages/studio/src/components/editor/PropertyPanelFlat.tsx b/packages/studio/src/components/editor/PropertyPanelFlat.tsx index b1d57d097..6c3ee483a 100644 --- a/packages/studio/src/components/editor/PropertyPanelFlat.tsx +++ b/packages/studio/src/components/editor/PropertyPanelFlat.tsx @@ -11,6 +11,7 @@ import { FlatTextSection } from "./propertyPanelFlatTextSection"; import { FlatStyleSection } from "./propertyPanelFlatStyleSections"; import { FlatLayoutSection } from "./propertyPanelFlatLayoutSection"; import { FlatMotionSection } from "./propertyPanelFlatMotionSection"; +import { deriveElementTiming } from "./propertyPanelFlatTimingDerivation"; import { createGsapLivePreview } from "./gsapLivePreview"; import { formatTextFieldPreview, StyleSections } from "./propertyPanelSections"; import { STUDIO_GSAP_PANEL_ENABLED } from "./manualEditingAvailability"; @@ -96,8 +97,12 @@ export function PropertyPanelFlat({ currentPct, animIdForProp, gsapRuntimeValues, - elStart, - elDuration, + // Renamed: PropertyPanel.tsx still computes/passes these for its own legacy + // (non-flat) panel, but the flat path recomputes its own basis below via + // deriveElementTiming so it agrees with Motion's Timing row — ignore the + // parent's naive `elDuration ?? 1` fallback. + elStart: _elStart, + elDuration: _elDuration, onCommitAnimatedProperty, onCommitAnimatedProperties, onSeekToTime, @@ -222,6 +227,12 @@ export function PropertyPanelFlat({ setPinnedGroupIds((current) => current.includes(groupId) ? current.filter((id) => id !== groupId) : [...current, groupId], ); + // Basis for the Layout keyframe gutter (X/Y/W/H/Angle + 3D Transform) — + // must agree with Motion's Timing row (FlatTimingRow), which infers the + // range from animations when there's no explicit data-duration. Computed + // here (not threaded from PropertyPanel) both to keep that file under its + // 600-LOC gate and because element/gsapAnimations are already in scope. + const { start: elStart, duration: elDuration } = deriveElementTiming(element, gsapAnimations); // Trivial percentage→time seek, derived here rather than threaded from // PropertyPanel (keeps that file under its 600-LOC gate). const seekFromKfPct = (pct: number) => onSeekToTime?.(elStart + (pct / 100) * elDuration); diff --git a/packages/studio/src/components/editor/propertyPanelFlatMotionSection.tsx b/packages/studio/src/components/editor/propertyPanelFlatMotionSection.tsx index 01fde1ffe..0eb914760 100644 --- a/packages/studio/src/components/editor/propertyPanelFlatMotionSection.tsx +++ b/packages/studio/src/components/editor/propertyPanelFlatMotionSection.tsx @@ -7,21 +7,7 @@ import { CommitField } from "./propertyPanelPrimitives"; import { AnimationCard } from "./AnimationCard"; import { ADD_METHODS, ADD_METHOD_LABELS, METHOD_TOOLTIPS } from "./gsapAnimationConstants"; import type { GsapAnimationEditCallbacks } from "./gsapAnimationCallbacks"; - -function deriveTimingFromAnimations( - animations: GsapAnimation[], -): { start: number; duration: number } | null { - let lo = Infinity; - let hi = -Infinity; - for (const a of animations) { - const s = a.resolvedStart ?? (typeof a.position === "number" ? a.position : 0); - const d = a.duration ?? 0; - lo = Math.min(lo, s); - hi = Math.max(hi, s + d); - } - if (!Number.isFinite(lo) || !Number.isFinite(hi) || hi <= lo) return null; - return { start: lo, duration: hi - lo }; -} +import { deriveElementTiming } from "./propertyPanelFlatTimingDerivation"; export function FlatTimingRow({ element, @@ -32,15 +18,7 @@ export function FlatTimingRow({ animations?: GsapAnimation[]; onSetAttribute: (attr: string, value: string) => void | Promise; }) { - const explicitStart = Number.parseFloat(element.dataAttributes.start ?? "0") || 0; - const explicitDuration = - Number.parseFloat( - element.dataAttributes.duration ?? element.dataAttributes["hf-authored-duration"] ?? "0", - ) || 0; - - const derived = explicitDuration > 0 ? null : deriveTimingFromAnimations(animations); - const start = derived ? derived.start : explicitStart; - const duration = derived ? derived.duration : explicitDuration; + const { start, duration, inferred: derived } = deriveElementTiming(element, animations); const end = start + duration; const commitStart = (nextValue: string) => { diff --git a/packages/studio/src/components/editor/propertyPanelFlatTimingDerivation.test.ts b/packages/studio/src/components/editor/propertyPanelFlatTimingDerivation.test.ts new file mode 100644 index 000000000..910934f80 --- /dev/null +++ b/packages/studio/src/components/editor/propertyPanelFlatTimingDerivation.test.ts @@ -0,0 +1,71 @@ +import { describe, expect, it } from "vitest"; +import { deriveElementTiming } from "./propertyPanelFlatTimingDerivation"; +import type { DomEditSelection } from "./domEditingTypes"; +import type { GsapAnimation } from "@hyperframes/core/gsap-parser"; + +function withDataAttributes( + dataAttributes: Record, +): Pick { + return { dataAttributes }; +} + +describe("deriveElementTiming", () => { + it("uses the explicit data-start/data-duration attributes when duration is authored", () => { + const result = deriveElementTiming(withDataAttributes({ start: "8", duration: "4" })); + expect(result).toEqual({ start: 8, duration: 4, inferred: false }); + }); + + it("infers start/duration from animations when there is no explicit data-duration", () => { + const animations = [{ position: 2, duration: 3 } as unknown as GsapAnimation]; + const result = deriveElementTiming( + withDataAttributes({ start: "0", duration: "0" }), + animations, + ); + expect(result).toEqual({ start: 2, duration: 3, inferred: true }); + }); + + it("spans the earliest tween start to the latest tween end across multiple animations", () => { + const animations = [ + { position: 1, duration: 2 } as unknown as GsapAnimation, // 1 -> 3 + { position: 2, duration: 4 } as unknown as GsapAnimation, // 2 -> 6 + ]; + const result = deriveElementTiming(withDataAttributes({}), animations); + expect(result).toEqual({ start: 1, duration: 5, inferred: true }); + }); + + it("prefers an explicit data-duration over inference even when animations exist", () => { + const animations = [{ position: 2, duration: 3 } as unknown as GsapAnimation]; + const result = deriveElementTiming( + withDataAttributes({ start: "0", duration: "10" }), + animations, + ); + expect(result).toEqual({ start: 0, duration: 10, inferred: false }); + }); + + it("falls back to hf-authored-duration when data-duration is absent", () => { + const result = deriveElementTiming( + withDataAttributes({ start: "1", "hf-authored-duration": "6" }), + ); + expect(result).toEqual({ start: 1, duration: 6, inferred: false }); + }); + + it("returns a zero-duration, non-inferred result with no attributes and no animations", () => { + const result = deriveElementTiming(withDataAttributes({})); + expect(result).toEqual({ start: 0, duration: 0, inferred: false }); + }); + + // This is the exact bug from the whole-plan coherence review: Layout's + // keyframe-seek basis must land on the same absolute time that Motion's + // Timing row displays as the element's midpoint. + it("agrees with a keyframe-percentage seek: 50% lands on the same midpoint the Timing row would show", () => { + const animations = [{ position: 2, duration: 3 } as unknown as GsapAnimation]; + const timing = deriveElementTiming( + withDataAttributes({ start: "0", duration: "0" }), + animations, + ); + const seekTimeAt50Pct = timing.start + (50 / 100) * timing.duration; + const timingRowMidpoint = timing.start + timing.duration / 2; + expect(seekTimeAt50Pct).toBe(timingRowMidpoint); + expect(seekTimeAt50Pct).toBe(3.5); + }); +}); diff --git a/packages/studio/src/components/editor/propertyPanelFlatTimingDerivation.ts b/packages/studio/src/components/editor/propertyPanelFlatTimingDerivation.ts new file mode 100644 index 000000000..a4cb30121 --- /dev/null +++ b/packages/studio/src/components/editor/propertyPanelFlatTimingDerivation.ts @@ -0,0 +1,59 @@ +import type { GsapAnimation } from "@hyperframes/core/gsap-parser"; +import type { DomEditSelection } from "./domEditingTypes"; + +/** + * The single source of truth for an element's clip start/duration in the flat + * inspector. Both the Motion group's Timing row (`FlatTimingRow`) and the + * Layout group's keyframe gutter (fed via `elStart`/`elDuration` from + * `PropertyPanel.tsx` through `PropertyPanelFlat.tsx`) must derive this the + * same way — otherwise a keyframe-percentage seek in Layout lands on a + * different absolute time than the range Motion displays for the same + * element (found by the Plan 3a+3b whole-plan coherence review). + * + * Precedence: an explicit `data-duration` (or `data-hf-authored-duration`) + * wins outright. Only when neither is present do we infer the range from the + * element's own GSAP tweens (earliest tween start → latest tween end). + * + * Scoped to the FLAT inspector only — the legacy (non-flat) panel keeps its + * own, unrelated `elStart`/`elDuration ?? 1` computation in `PropertyPanel.tsx` + * untouched. + */ +export interface ElementTiming { + start: number; + duration: number; + /** True when duration/start came from `deriveTimingFromAnimations`, not an authored attribute. */ + inferred: boolean; +} + +function deriveTimingFromAnimations( + animations: GsapAnimation[], +): { start: number; duration: number } | null { + let lo = Infinity; + let hi = -Infinity; + for (const a of animations) { + const s = a.resolvedStart ?? (typeof a.position === "number" ? a.position : 0); + const d = a.duration ?? 0; + lo = Math.min(lo, s); + hi = Math.max(hi, s + d); + } + if (!Number.isFinite(lo) || !Number.isFinite(hi) || hi <= lo) return null; + return { start: lo, duration: hi - lo }; +} + +export function deriveElementTiming( + element: Pick, + animations: GsapAnimation[] = [], +): ElementTiming { + const explicitStart = Number.parseFloat(element.dataAttributes.start ?? "0") || 0; + const explicitDuration = + Number.parseFloat( + element.dataAttributes.duration ?? element.dataAttributes["hf-authored-duration"] ?? "0", + ) || 0; + + const derived = explicitDuration > 0 ? null : deriveTimingFromAnimations(animations); + return { + start: derived ? derived.start : explicitStart, + duration: derived ? derived.duration : explicitDuration, + inferred: derived !== null, + }; +}