diff --git a/packages/studio/src/App.tsx b/packages/studio/src/App.tsx index 7bdaafd5b..7921f28e9 100644 --- a/packages/studio/src/App.tsx +++ b/packages/studio/src/App.tsx @@ -157,6 +157,9 @@ export function StudioApp() { pendingTimelineEditPathRef, }); const invalidateGsapCacheRef = useRef<() => void>(() => {}); + // Stable identity — what the ref indirection is for. An inline arrow re-created + // the memoized timeline handlers (it is in their deps) on every render. + const invalidateGsapCache = useCallback(() => invalidateGsapCacheRef.current(), []); const timelineEditing = useTimelineEditing({ projectId, activeCompPath, @@ -174,7 +177,7 @@ export function StudioApp() { sdkSession: editFlowSdkSession, publishSdkSession: sdkHandle.publish, forceReloadSdkSession: sdkHandle.forceReload, - invalidateGsapCache: () => invalidateGsapCacheRef.current(), + invalidateGsapCache, handleDomZIndexReorderCommitRef, }); const handleTimelineElementsMove: TimelineMoveEditsHandler = useCallback( diff --git a/packages/studio/src/components/TimelineToolbar.tsx b/packages/studio/src/components/TimelineToolbar.tsx index 3b42013b8..c745f087d 100644 --- a/packages/studio/src/components/TimelineToolbar.tsx +++ b/packages/studio/src/components/TimelineToolbar.tsx @@ -5,7 +5,7 @@ import { isPlayheadWithinTween, type EnableKeyframesSession, } from "../hooks/useEnableKeyframes"; -import { computeElementPercentage } from "../hooks/gsapShared"; +import { computeElementPercentage, KEYFRAME_PCT_MATCH } from "../hooks/gsapShared"; import { useKeyframeKeyboard } from "../hooks/useKeyframeKeyboard"; import { getNextTimelineZoomPercent, @@ -54,8 +54,8 @@ function isMotionPathEndpoint(animation: GsapAnimation | undefined, percentage: if (!animation?.keyframes) return false; const keyframes = animation.keyframes.keyframes; return ( - Math.abs((keyframes[0]?.percentage ?? -Infinity) - percentage) <= 1 || - Math.abs((keyframes.at(-1)?.percentage ?? Infinity) - percentage) <= 1 + Math.abs((keyframes[0]?.percentage ?? -Infinity) - percentage) <= KEYFRAME_PCT_MATCH || + Math.abs((keyframes.at(-1)?.percentage ?? Infinity) - percentage) <= KEYFRAME_PCT_MATCH ); } @@ -80,7 +80,7 @@ function resolveKeyframeToggleState( const percentage = computeElementPercentage(currentTime, session.domEditSelection, animation); const pathEndpoint = isMotionPathEndpoint(arcAnimation, percentage); const active = animation.keyframes.keyframes.some( - (keyframe) => Math.abs(keyframe.percentage - percentage) <= 1, + (keyframe) => Math.abs(keyframe.percentage - percentage) <= KEYFRAME_PCT_MATCH, ); return { state: pathEndpoint ? "none" : active ? "active" : "inactive", diff --git a/packages/studio/src/hooks/gsapDragPositionCommit.ts b/packages/studio/src/hooks/gsapDragPositionCommit.ts index 9ccf93503..28c437710 100644 --- a/packages/studio/src/hooks/gsapDragPositionCommit.ts +++ b/packages/studio/src/hooks/gsapDragPositionCommit.ts @@ -2,7 +2,7 @@ import type { GsapAnimation } from "@hyperframes/core/gsap-parser"; import type { DomEditSelection } from "../components/editor/domEditingTypes"; import { usePlayerStore } from "../player/store/playerStore"; import { resolveTweenStart, resolveTweenDuration } from "../utils/globalTimeCompiler"; -import { resolveEditableTweenDuration } from "./gsapShared"; +import { KEYFRAME_PCT_MATCH, resolveEditableTweenDuration } from "./gsapShared"; import { roundTo3 } from "../utils/rounding"; import { computeDraggedGsapPosition } from "./draggedGsapPosition"; import { @@ -12,17 +12,27 @@ import { materializeIfDynamic, } from "./gsapDragCommit"; +/** + * The tween's keyframes with one inserted at `percentage`. Any existing keyframe + * within {@link KEYFRAME_PCT_MATCH} of the insert is REPLACED, not kept: the + * server takes a replace-with-keyframes list verbatim, so an append-only build + * could hand it two keyframes a fraction of a percent apart. The invariant lives + * here rather than in each caller's own pre-check, which is how the two callers + * ended up with different tolerances in the first place. + */ export function buildTemporalArcKeyframes( anim: GsapAnimation, percentage: number, properties: Record, ) { return [ - ...(anim.keyframes?.keyframes ?? []).map((keyframe) => ({ - percentage: keyframe.percentage, - properties: { ...keyframe.properties }, - ...(keyframe.ease ? { ease: keyframe.ease } : {}), - })), + ...(anim.keyframes?.keyframes ?? []) + .filter((keyframe) => Math.abs(keyframe.percentage - percentage) > KEYFRAME_PCT_MATCH) + .map((keyframe) => ({ + percentage: keyframe.percentage, + properties: { ...keyframe.properties }, + ...(keyframe.ease ? { ease: keyframe.ease } : {}), + })), { percentage, properties }, ].sort((a, b) => a.percentage - b.percentage); } @@ -279,7 +289,12 @@ export async function commitGsapPositionFromDrag( const { activeKeyframePct, setActiveKeyframePct } = usePlayerStore.getState(); const pct = activeKeyframePct ?? computeCurrentPercentage(selection, anim); const keyframes = anim.keyframes?.keyframes ?? []; - const pointIndex = keyframes.findIndex((kf) => Math.abs(kf.percentage - pct) < 0.05); + // Same tolerance as applyArcKeyframeAtPlayhead and isMotionPathEndpoint. A + // tighter one here meant a drag that landed a fraction of a percent off an + // authored waypoint skipped the update-point branch and appended instead. + const pointIndex = keyframes.findIndex( + (kf) => Math.abs(kf.percentage - pct) <= KEYFRAME_PCT_MATCH, + ); if (pointIndex >= 0) { await callbacks.commitMutation( selection, diff --git a/packages/studio/src/hooks/gsapShared.ts b/packages/studio/src/hooks/gsapShared.ts index c77ddd645..00c79ceda 100644 --- a/packages/studio/src/hooks/gsapShared.ts +++ b/packages/studio/src/hooks/gsapShared.ts @@ -70,6 +70,15 @@ export function isInstantHold(animation: GsapAnimation): boolean { // `CSS.escape`, it needs no browser global (this runs in node tests too). const SAFE_HASH_ID = /^-?[A-Za-z_][\w-]*$/; +/** + * How close (in tween-%) a playhead has to be to count as sitting ON an existing + * keyframe. Every "is there already a keyframe here?" test shares this: with two + * different tolerances in play, one path decided "no keyframe here, append one" + * while another decided "yes, edit that one", and a drag near a waypoint left two + * keyframes a fraction of a percent apart. + */ +export const KEYFRAME_PCT_MATCH = 1; + export function idSelector(id: string): string { // A `#id` selector is only valid for a CSS identifier. IDs that start with a // digit (e.g. "01-hook-hero-word") make `document.querySelector("#01-...")` and diff --git a/packages/studio/src/hooks/keyframeCacheAstLoad.ts b/packages/studio/src/hooks/keyframeCacheAstLoad.ts index 19eaacdcf..bca5f4b23 100644 --- a/packages/studio/src/hooks/keyframeCacheAstLoad.ts +++ b/packages/studio/src/hooks/keyframeCacheAstLoad.ts @@ -11,18 +11,13 @@ import { elementCacheKeys, writeGsapAnimationsForElement, } from "./gsapKeyframeCacheHelpers"; -import { toClipKeyframes } from "./gsapShared"; +import { idFromSelector, toClipKeyframes } from "./gsapShared"; import { deduplicateKeyframes, isStaticPositionHold, synthesizeFlatTweenKeyframes, } from "./gsapTweenSynth"; -function extractIdFromSelector(selector: string): string | null { - const match = selector.match(/^#([\w-]+)/); - return match ? match[1] : null; -} - /** * Resolve a tween's target selector to the ids of the element(s) it animates. * A bare `#id` resolves directly; anything else (a class like `.dot`, a group @@ -36,10 +31,13 @@ export function resolveSelectorElementIds( selector: string, doc: Document | null | undefined, ): string[] { - const bareId = selector.match(/^#([\w-]+)$/); - if (bareId) return [bareId[1]]; + // A whole-selector id match (either shape) addresses exactly one element. + const bareId = /^(#[\w-]+|\[id="(?:\\.|[^"\\])*"\])$/.test(selector) + ? idFromSelector(selector) + : null; + if (bareId) return [bareId]; if (!doc) { - const lead = extractIdFromSelector(selector); + const lead = idFromSelector(selector); return lead ? [lead] : []; } const ids = new Set(); @@ -51,7 +49,7 @@ export function resolveSelectorElementIds( if (el.id) ids.add(el.id); } } catch { - const lead = extractIdFromSelector(sel); + const lead = idFromSelector(sel); if (lead) ids.add(lead); } } diff --git a/packages/studio/src/hooks/useEnableKeyframes.ts b/packages/studio/src/hooks/useEnableKeyframes.ts index 16299b2bd..38c8f5008 100644 --- a/packages/studio/src/hooks/useEnableKeyframes.ts +++ b/packages/studio/src/hooks/useEnableKeyframes.ts @@ -15,6 +15,7 @@ import { fetchParsedAnimations, getAnimationsForElement } from "./useGsapTweenCa import { selectorFromSelection, computeElementPercentage, + KEYFRAME_PCT_MATCH, isInstantHold, resolveEditableTweenDuration, } from "./gsapShared"; @@ -302,7 +303,9 @@ async function applyKeyframeAtPlayhead( } const pct = start === null ? computeElementPercentage(t, sel) : absoluteToPercentage(t, start, duration); - const existing = kfAnim.keyframes?.keyframes.find((k) => Math.abs(k.percentage - pct) <= 1); + const existing = kfAnim.keyframes?.keyframes.find( + (k) => Math.abs(k.percentage - pct) <= KEYFRAME_PCT_MATCH, + ); if (existing) { session.handleGsapRemoveKeyframe(kfAnim.id, existing.percentage); return; @@ -406,7 +409,7 @@ export async function applyArcKeyframeAtPlayhead( const nodes = arcAnim.keyframes?.keyframes ?? []; const playheadPercentage = absoluteToPercentage(t, start, duration); const timedNodeIndex = nodes.findIndex( - (node) => Math.abs(node.percentage - playheadPercentage) <= 1, + (node) => Math.abs(node.percentage - playheadPercentage) <= KEYFRAME_PCT_MATCH, ); if (timedNodeIndex !== -1) { if (timedNodeIndex > 0 && timedNodeIndex < nodes.length - 1) { diff --git a/packages/studio/src/hooks/useGsapKeyframeOps.ts b/packages/studio/src/hooks/useGsapKeyframeOps.ts index b2a9ee3ba..fe105e644 100644 --- a/packages/studio/src/hooks/useGsapKeyframeOps.ts +++ b/packages/studio/src/hooks/useGsapKeyframeOps.ts @@ -15,6 +15,7 @@ import { } from "../utils/sdkCutover"; import type { KeyframeCacheEntry } from "../player/store/playerStore"; import { commitKeyframeAtTimeImpl } from "./gsapKeyframeCommit"; +import { idFromSelector } from "./gsapShared"; import { clearKeyframeCacheForElement, readKeyframeSnapshot, @@ -338,7 +339,7 @@ export function useGsapKeyframeOps({ // remove-all-keyframes collapses the tween to a static hold and the commit // path doesn't return parsed animations, so the keyframe cache is never // refreshed — clear it here so the timeline diamonds disappear immediately. - const elementId = selection.id ?? selection.selector?.match(/^#([\w-]+)/)?.[1] ?? null; + const elementId = selection.id ?? idFromSelector(selection.selector); if (elementId) clearKeyframeCacheForElement(targetPath, elementId); if (sdkSession && sdkDeps) { const handled = await sdkGsapRemoveAllKeyframesPersist( diff --git a/packages/studio/src/player/components/TimelineClipDiamonds.tsx b/packages/studio/src/player/components/TimelineClipDiamonds.tsx index 8509db281..b7e2c6391 100644 --- a/packages/studio/src/player/components/TimelineClipDiamonds.tsx +++ b/packages/studio/src/player/components/TimelineClipDiamonds.tsx @@ -58,17 +58,29 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({ // Pending retime destination (clip + tween %) per keyframe key, so a rapid // second drag composes from where the first move left the keyframe (whose // cache entry has not rebuilt yet) instead of the stale rendered value. - const pendingRetimeRef = useRef(new Map()); + const pendingRetimeRef = useRef | null>(null); + // Lazy: `useRef(new Map())` allocates a Map on every render and throws all but + // the first away, once per mounted lane. + pendingRetimeRef.current ??= new Map(); + const pendingRetimes = pendingRetimeRef.current; useEffect(() => { - // Clear a pending entry once the authoritative cache reflects a keyframe at - // ~its destination. Match by tolerance, not equality: cache writers round - // clip %s, so an exact check would leak an entry after every successful retime. - for (const [key, pending] of pendingRetimeRef.current) { - if (keyframesData.keyframes.some((k) => Math.abs(k.percentage - pending.clipPct) < 0.2)) { - pendingRetimeRef.current.delete(key); - } + // Clear a pending entry once the authoritative cache reflects THAT keyframe + // at ~its destination. Match by tolerance, not equality: cache writers round + // clip %s, so an exact check would leak an entry after every successful + // retime. Match by identity too: a bare "some keyframe is near that %" test + // cleared the entry whenever an unrelated sibling happened to sit there, + // which is easy to hit on an evenly spaced row. + const pendingEntries = pendingRetimeRef.current; + if (!pendingEntries) return; + for (const [key, pending] of pendingEntries) { + const settled = keyframesData.keyframes.some( + (k) => + timelineKeyframeSelectionKey(elementId, keyframeTarget(k)) === key && + Math.abs(k.percentage - pending.clipPct) < 0.2, + ); + if (settled) pendingEntries.delete(key); } - }, [keyframesData.keyframes]); + }, [keyframesData.keyframes, elementId]); // Visual-only preview of the dragged diamond's clip-% — no runtime/GSAP hold // (that optimistic hold was the #1763 flake). The atomic move-keyframe commit // on drop re-keys the diamond from source. @@ -190,9 +202,19 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({ const target = keyframeTarget(kf); const kfKey = timelineKeyframeSelectionKey(elementId, target); // Clamp against this keyframe's own tween, not the whole merged row. - const siblingRow = siblingRowOf(kf); - const siblingClipPcts = siblingRow.map((k) => k.percentage); - const siblingIndex = siblingRow.indexOf(kf); + // Compose each sibling's pending destination in first: clamping against + // cached positions while the dragged keyframe reads its pending one let + // a second drag cross a neighbour that had already moved past it. + const siblingRow = siblingRowOf(kf) + .map((k) => ({ + keyframe: k, + clipPct: + pendingRetimes.get(timelineKeyframeSelectionKey(elementId, keyframeTarget(k))) + ?.clipPct ?? k.percentage, + })) + .sort((a, b) => a.clipPct - b.clipPct); + const siblingClipPcts = siblingRow.map((s) => s.clipPct); + const siblingIndex = siblingRow.findIndex((s) => s.keyframe === kf); // While dragging this diamond, render it at the live preview clip-%. const renderPct = preview?.kfKey === kfKey ? preview.clipPct : kf.percentage; // Center the marker's non-overlapping hit region ON its keyframe %, so @@ -216,7 +238,7 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({ startX: e.clientX, lastX: e.clientX, index: siblingIndex, - fromClipPct: pendingRetimeRef.current.get(kfKey)?.clipPct ?? kf.percentage, + fromClipPct: pendingRetimes.get(kfKey)?.clipPct ?? kf.percentage, moved: false, }; } @@ -309,7 +331,7 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({ // For a rapid second retime the diamond still renders the stale cache // position, so identify the FROM keyframe by the pending (already-moved) // position; the mutation locates the source keyframe by this identity. - const pendingBefore = pendingRetimeRef.current.get(kfKey); + const pendingBefore = pendingRetimes.get(kfKey); const fromTarget = pendingBefore ? { ...target, @@ -318,10 +340,10 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({ } : target; const pending = { clipPct: res.toClipPct, tweenPct: newTweenPct }; - pendingRetimeRef.current.set(kfKey, pending); + pendingRetimes.set(kfKey, pending); const clearPending = () => { - if (pendingRetimeRef.current.get(kfKey) === pending) { - pendingRetimeRef.current.delete(kfKey); + if (pendingRetimes.get(kfKey) === pending) { + pendingRetimes.delete(kfKey); } }; // A rejected drop (the destination time is already occupied) snaps diff --git a/packages/studio/src/player/components/timelineKeyframeIdentity.test.ts b/packages/studio/src/player/components/timelineKeyframeIdentity.test.ts index 5b774715a..78ea65a13 100644 --- a/packages/studio/src/player/components/timelineKeyframeIdentity.test.ts +++ b/packages/studio/src/player/components/timelineKeyframeIdentity.test.ts @@ -32,6 +32,13 @@ describe("timeline keyframe selection identity", () => { expect(timelineKeyframeTargetFromSelectionKey("comp#a", key)).toBeNull(); }); + // Timeline.tsx still writes the collapsed form for clip-lane shift-clicks, and + // an element id can itself contain a colon — the split has to be the LAST one. + it("splits the collapsed key at the last colon so a colon-bearing id survives", () => { + expect(timelineKeyframeTargetFromSelectionKey("a:b", "a:b:40")).toEqual({ percentage: 40 }); + expect(timelineKeyframeTargetFromSelectionKey("a", "a:b:40")).toBeNull(); + }); + it("retains the collapsed key fallback and rejects malformed percentages", () => { expect(timelineKeyframeTargetFromSelectionKey("comp#a", "comp#a:30")).toEqual({ percentage: 30, diff --git a/packages/studio/src/player/components/timelineLayout.test.ts b/packages/studio/src/player/components/timelineLayout.test.ts index 63f0c97fe..4cd2fa14d 100644 --- a/packages/studio/src/player/components/timelineLayout.test.ts +++ b/packages/studio/src/player/components/timelineLayout.test.ts @@ -1,5 +1,8 @@ import { describe, it, expect } from "vitest"; import { + CLIP_Y, + INSERT_BOUNDARY_BAND, + getTimelineInsertBoundaryBand, RULER_H, TRACK_H, LANE_H, @@ -225,6 +228,7 @@ describe("getTimelineScrubTime", () => { clientX: 500, viewportLeft: 0, scrollLeft: 0, + contentOrigin: GUTTER + TRACKS_LEFT_PAD, pixelsPerSecond: 0, duration: 10, }), @@ -232,3 +236,25 @@ describe("getTimelineScrubTime", () => { expect(at(origin + 250, Number.NaN)).toBe(0); }); }); + +// The only production hook keeping resolveInsertRow's band aligned with the +// rendered clip inset once rows can be taller than TRACK_H. Pinned directly so a +// change to CLIP_Y or the invalid-height fallback can't silently drift it. +describe("getTimelineInsertBoundaryBand", () => { + it("matches the fixed band for a plain track row", () => { + expect(getTimelineInsertBoundaryBand(TRACK_H)).toBe(INSERT_BOUNDARY_BAND); + expect(getTimelineInsertBoundaryBand(TRACK_H)).toBe(CLIP_Y / TRACK_H); + }); + + it("shrinks as the row grows, so the band stays CLIP_Y pixels tall", () => { + const expanded = TRACK_H + 2 * LANE_H; + expect(getTimelineInsertBoundaryBand(expanded)).toBeCloseTo(CLIP_Y / expanded, 10); + expect(getTimelineInsertBoundaryBand(expanded)).toBeLessThan(INSERT_BOUNDARY_BAND); + }); + + it("falls back to the plain-track band for a height that is not usable", () => { + for (const height of [0, -10, Number.NaN]) { + expect(getTimelineInsertBoundaryBand(height)).toBe(INSERT_BOUNDARY_BAND); + } + }); +}); diff --git a/packages/studio/src/player/components/timelineLayout.ts b/packages/studio/src/player/components/timelineLayout.ts index 4db33c01c..0ac3cb856 100644 --- a/packages/studio/src/player/components/timelineLayout.ts +++ b/packages/studio/src/player/components/timelineLayout.ts @@ -104,6 +104,10 @@ function getTimelineRowOffset(row: number, rowHeights: readonly number[]): numbe const offsets = getTimelineRowOffsets(rowHeights); if (row <= 0) return row * getTimelineRowHeight(0, rowHeights); if (row >= rowHeights.length) { + // Deliberately TRACK_H, not the last row's height: rows past the end do not + // exist yet, and a row created by dropping there starts unexpanded. The + // pre-first-row branch above uses row 0's concrete height instead because + // that row DOES exist — the pointer is in the top pad above a real lane. return (offsets[rowHeights.length] ?? 0) + (row - rowHeights.length) * TRACK_H; } const wholeRow = Math.floor(row); diff --git a/packages/studio/src/player/components/useAutoExpandKeyframedClips.ts b/packages/studio/src/player/components/useAutoExpandKeyframedClips.ts index 1ff3e1d91..33b0731af 100644 --- a/packages/studio/src/player/components/useAutoExpandKeyframedClips.ts +++ b/packages/studio/src/player/components/useAutoExpandKeyframedClips.ts @@ -24,6 +24,12 @@ export function useAutoExpandKeyframedClips(gsapAnimations: Map