-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix(studio): harden keyframe editing semantics #2689
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5dd41e9
1e99115
5816e70
a9bdade
7f86018
dd77b83
50b6822
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,10 @@ | |
|
|
||
| import React, { act } from "react"; | ||
| import { createRoot } from "react-dom/client"; | ||
| import { afterEach, describe, expect, it } from "vitest"; | ||
| import { afterEach, describe, expect, it, vi } from "vitest"; | ||
| import type { GsapAnimation } from "@hyperframes/core/gsap-parser"; | ||
| import { usePlayerStore } from "../player/store/playerStore"; | ||
| import { makeSelection } from "../hooks/domSelectionTestHarness"; | ||
| import { TimelineToolbar } from "./TimelineToolbar"; | ||
|
|
||
| (globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; | ||
|
|
@@ -13,12 +15,14 @@ afterEach(() => { | |
| usePlayerStore.setState({ autoKeyframeEnabled: true }); | ||
| }); | ||
|
|
||
| function renderToolbar() { | ||
| function renderToolbar( | ||
| domEditSession?: React.ComponentProps<typeof TimelineToolbar>["domEditSession"], | ||
| ) { | ||
| const host = document.createElement("div"); | ||
| document.body.append(host); | ||
| const root = createRoot(host); | ||
| act(() => { | ||
| root.render(<TimelineToolbar />); | ||
| root.render(<TimelineToolbar domEditSession={domEditSession} />); | ||
| }); | ||
| return { host, root }; | ||
| } | ||
|
|
@@ -54,3 +58,44 @@ describe("TimelineToolbar β auto-keyframe toggle (#1808)", () => { | |
| act(() => root.unmount()); | ||
| }); | ||
| }); | ||
| describe("TimelineToolbar β motion path endpoints", () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π’ TimelineToolbar keyboard/button parity: only the 'endpoint disabled' path asserted; 4 new motion-path label branches untested const button = host.querySelector<HTMLButtonElement>(
'button[aria-label="Motion path endpoint"]',
);
expect(button?.disabled).toBe(true);The diff introduces five distinct tooltip/aria-label branches ('Motion path endpoints cannot be removed', 'Extend motion path to playhead (K)', 'Remove waypoint from motion path (K)', 'Add waypoint to motion path (K)', plus the pre-existing non-motion-path branches). The only new test asserts the endpoint case. If a future refactor swaps the arcAnimation-vs-keyframedAnimation resolution order, the wrong label ships silently. Fix: Add render assertions for each of the four new label branches. |
||
| it("does not advertise a destructive keyframe toggle for a required endpoint", () => { | ||
| usePlayerStore.setState({ currentTime: 10 }); | ||
| const animation: GsapAnimation = { | ||
| id: "#el-to-0-position", | ||
| targetSelector: "#el", | ||
| method: "to", | ||
| position: 0, | ||
| duration: 10, | ||
| properties: {}, | ||
| keyframes: { | ||
| format: "object-array", | ||
| keyframes: [ | ||
| { percentage: 0, properties: { x: 0, y: 0 } }, | ||
| { percentage: 100, properties: { x: 100, y: 0 } }, | ||
| ], | ||
| }, | ||
| arcPath: { | ||
| enabled: true, | ||
| autoRotate: false, | ||
| segments: [{ curviness: 1 }], | ||
| }, | ||
| }; | ||
| const element = document.createElement("div"); | ||
| element.id = "el"; | ||
| const session = { | ||
| domEditSelection: makeSelection("Element", element), | ||
| selectedGsapAnimations: [animation], | ||
| handleGsapAddAnimation: vi.fn(), | ||
| handleGsapConvertToKeyframes: vi.fn(), | ||
| handleGsapRemoveKeyframe: vi.fn(), | ||
| } satisfies NonNullable<React.ComponentProps<typeof TimelineToolbar>["domEditSession"]>; | ||
|
|
||
| const { host, root } = renderToolbar(session); | ||
| const button = host.querySelector<HTMLButtonElement>( | ||
| 'button[aria-label="Motion path endpoint"]', | ||
| ); | ||
| expect(button?.disabled).toBe(true); | ||
| act(() => root.unmount()); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π’ Server-side resolveReplacementEaseEach re-parses the entire script on every request that omits easeEach
Called from both
executeGsapMutationAcorn(:1520) andexecuteGsapMutationRecast(:1891). During a bulk keyframe edit sequence over an arc with N stops, each request re-parses the entire GSAP script AST just to look up sourceeaseEach. Correctness fine but parse cost scales with N. The new client callers (useEnableKeyframes/useGsapKeyframeOps) sendease: 'none'only β do NOT send easeEach β so this fallback path fires for every real motion-path temporal-keyframe write.Fix: Client-side: derive easeEach from the local anim record before the fetch, or pass through gsapAnimations lookup. Or server-side: cache the parse across a single request batch.
β Review by Rames D Jusso