From e317f1fbe300aafbc37641ea0619b42f25de29b1 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Mon, 27 Jul 2026 17:36:34 +0200 Subject: [PATCH] refactor(studio): double-cast test fixtures and split three dense functions CONTRIBUTING.md allows `as unknown as T` with a justification, not a bare `as T`; the gsapShared fixtures only carry the fields under test. The fallow complexity gate flagged three functions on this branch. Each is split at its natural seam rather than suppressed: the auto-expand scan moves out of the effect, the four repeated attribute guards in nodeMatchesManifestClip collapse into one table-driven check, and the segment-% interpolation moves out of onPathDown. --- .../components/editor/MotionPathOverlay.tsx | 24 ++++++++++----- packages/studio/src/hooks/gsapShared.test.ts | 12 ++++---- .../components/useAutoExpandKeyframedClips.ts | 30 ++++++++++++------- .../src/player/lib/timelineElementHelpers.ts | 23 +++++++------- 4 files changed, 55 insertions(+), 34 deletions(-) diff --git a/packages/studio/src/components/editor/MotionPathOverlay.tsx b/packages/studio/src/components/editor/MotionPathOverlay.tsx index 5d29fab68..664140429 100644 --- a/packages/studio/src/components/editor/MotionPathOverlay.tsx +++ b/packages/studio/src/components/editor/MotionPathOverlay.tsx @@ -45,6 +45,19 @@ type DragState = { ref: MotionNodeRef; }; +/** + * Tween-% for a stop inserted at fraction `t` along the segment between two + * nodes. null when either end is not a keyframe (arc waypoints carry no %). + */ +function interpolatedKeyframePct( + a: MotionNodeRef | undefined, + b: MotionNodeRef | undefined, + t: number, +): number | null { + if (a?.type !== "keyframe" || b?.type !== "keyframe") return null; + return Math.round((a.pct + (b.pct - a.pct) * t) * 1000) / 1000; +} + const NODE_PX = 6; // node radius in screen pixels (kept constant across zoom) // Click-vs-drag cutoff in SCREEN pixels. Below this the pointer-up is a click // (select the keyframe); at or above it the gesture commits a move. Screen-space @@ -402,13 +415,10 @@ export const MotionPathOverlay = memo(function MotionPathOverlay({ void commitAddWaypoint(animId, np.segIndex + 1, x, y, commitMutation); } else { // Linear keyframe path: interpolate the new stop's tween-% from the two - // keyframes bounding the clicked segment (np.t = fraction along it), then - // insert it. Lands ON the current line, so the dot doesn't jump — drag it - // after to bend the path. - const a = abs[np.segIndex]?.ref; - const b = abs[np.segIndex + 1]?.ref; - if (a?.type !== "keyframe" || b?.type !== "keyframe") return; - const pct = Math.round((a.pct + (b.pct - a.pct) * np.t) * 1000) / 1000; + // keyframes bounding the clicked segment, then insert it. Lands ON the + // current line, so the dot doesn't jump — drag it after to bend the path. + const pct = interpolatedKeyframePct(abs[np.segIndex]?.ref, abs[np.segIndex + 1]?.ref, np.t); + if (pct === null) return; e.stopPropagation(); void commitAddKeyframe(animId, pct, x, y, commitMutation); } diff --git a/packages/studio/src/hooks/gsapShared.test.ts b/packages/studio/src/hooks/gsapShared.test.ts index 0458d091b..604b93b69 100644 --- a/packages/studio/src/hooks/gsapShared.test.ts +++ b/packages/studio/src/hooks/gsapShared.test.ts @@ -11,17 +11,19 @@ import { toClipPercentage, } from "./gsapShared"; +// Fixtures carry only the fields the function under test reads; the double-cast +// is the documented way to stand in for the full runtime shape (CONTRIBUTING.md). +const tween = (duration: number | undefined) => ({ duration }) as unknown as GsapAnimation; + describe("resolveEditableTweenDuration", () => { - const selection = { dataAttributes: { duration: "16.26" } } as DomEditSelection; + const selection = { dataAttributes: { duration: "16.26" } } as unknown as DomEditSelection; it("uses the owning clip duration when the tween omits an outer duration", () => { - expect(resolveEditableTweenDuration({ duration: undefined } as GsapAnimation, selection)).toBe( - 16.26, - ); + expect(resolveEditableTweenDuration(tween(undefined), selection)).toBe(16.26); }); it("keeps an explicitly-authored tween duration", () => { - expect(resolveEditableTweenDuration({ duration: 4 } as GsapAnimation, selection)).toBe(4); + expect(resolveEditableTweenDuration(tween(4), selection)).toBe(4); }); }); diff --git a/packages/studio/src/player/components/useAutoExpandKeyframedClips.ts b/packages/studio/src/player/components/useAutoExpandKeyframedClips.ts index 33b0731af..c5ba437e3 100644 --- a/packages/studio/src/player/components/useAutoExpandKeyframedClips.ts +++ b/packages/studio/src/player/components/useAutoExpandKeyframedClips.ts @@ -11,6 +11,24 @@ import { animationContributesLane } from "./TimelinePropertyLanes"; * — tracked per-clip so a later user collapse sticks and never bounces back open * (and clips added later still auto-expand). */ +/** + * Prunes clips that left the source, then returns the ones that newly contribute + * a lane. The prune matters because the set is otherwise append-only: a clip + * deleted and reinserted under the same id (undo, paste) would be remembered as + * already-expanded and never auto-expand again. + */ +function freshLaneClips(gsapAnimations: Map, clips: Set) { + for (const key of clips) { + if (!gsapAnimations.has(key)) clips.delete(key); + } + const fresh: string[] = []; + for (const [key, animations] of gsapAnimations) { + if (clips.has(key)) continue; + if (animations.some(animationContributesLane)) fresh.push(key); + } + return fresh; +} + export function useAutoExpandKeyframedClips(gsapAnimations: Map): void { const expandClips = usePlayerStore((s) => s.expandClips); const projectId = useStudioShellContextOptional()?.projectId ?? null; @@ -24,17 +42,7 @@ export function useAutoExpandKeyframedClips(gsapAnimations: Map number]> = [ + ["data-start", (clip) => clip.start], + ["data-duration", (clip) => clip.duration], + ["data-track-index", (clip) => clip.track], +]; + function nodeMatchesManifestClip(node: Element, clip: ClipManifestClip): boolean { const tagName = clip.tagName?.toLowerCase(); if (tagName && node.tagName.toLowerCase() !== tagName) return false; - - const start = Number.parseFloat(node.getAttribute("data-start") ?? ""); - if (Number.isFinite(start) && !numbersNearlyEqual(start, clip.start)) return false; - - const duration = Number.parseFloat(node.getAttribute("data-duration") ?? ""); - if (Number.isFinite(duration) && !numbersNearlyEqual(duration, clip.duration)) return false; - - const track = Number.parseInt(node.getAttribute("data-track-index") ?? "", 10); - if (Number.isFinite(track) && track !== clip.track) return false; - - return true; + // An attribute only constrains the match when it parses to a finite number: + // missing or garbled reads as "unknown", not "mismatch". + return MANIFEST_CLIP_ATTRS.every(([attr, expected]) => { + const actual = Number.parseFloat(node.getAttribute(attr) ?? ""); + return !Number.isFinite(actual) || numbersNearlyEqual(actual, expected(clip)); + }); } function findTimelineDomNode(doc: Document, id: string): Element | null {