diff --git a/packages/cli/src/server/fileWatcher.test.ts b/packages/cli/src/server/fileWatcher.test.ts index 62412fcb9..036a4058a 100644 --- a/packages/cli/src/server/fileWatcher.test.ts +++ b/packages/cli/src/server/fileWatcher.test.ts @@ -1,6 +1,14 @@ -import { describe, expect, it } from "vitest"; +import { EventEmitter } from "node:events"; +import { describe, expect, it, vi } from "vitest"; -import { shouldWatchProjectFile } from "./fileWatcher.js"; +const mockWatcher = new EventEmitter() as EventEmitter & { close: () => void }; +mockWatcher.close = vi.fn(); + +vi.mock("node:fs", () => ({ + watch: vi.fn(() => mockWatcher), +})); + +const { shouldWatchProjectFile, createProjectWatcher } = await import("./fileWatcher.js"); describe("shouldWatchProjectFile", () => { it("watches files that can affect the project signature", () => { @@ -17,3 +25,15 @@ describe("shouldWatchProjectFile", () => { expect(shouldWatchProjectFile(".hyperframes/cache.json")).toBe(false); }); }); + +describe("createProjectWatcher", () => { + // Regression: fs.watch can fail asynchronously (e.g. EMFILE from exhausted + // OS watch handles) via an 'error' event, not a thrown exception. An + // EventEmitter 'error' with no listener crashes the whole process — this + // must degrade gracefully instead, per the sibling synchronous-failure path. + it("does not crash the process when the underlying watcher emits 'error'", () => { + createProjectWatcher("/fake/project/dir"); + expect(() => mockWatcher.emit("error", new Error("EMFILE"))).not.toThrow(); + expect(mockWatcher.close).toHaveBeenCalled(); + }); +}); diff --git a/packages/cli/src/server/fileWatcher.ts b/packages/cli/src/server/fileWatcher.ts index 050f009ea..91a8b2354 100644 --- a/packages/cli/src/server/fileWatcher.ts +++ b/packages/cli/src/server/fileWatcher.ts @@ -47,6 +47,15 @@ export function createProjectWatcher(projectDir: string): ProjectWatcher { } }, DEBOUNCE_MS); }); + // fs.watch can fail asynchronously too (e.g. EMFILE from exhausted OS watch + // handles) — that surfaces as an 'error' event, not a thrown exception. An + // EventEmitter 'error' with no listener crashes the whole process, so this + // listener is required for the same "degrade gracefully" the catch below + // already promises for the synchronous failure mode. + watcher.on("error", () => { + watcher?.close(); + watcher = null; + }); } catch { // fs.watch may fail on some platforms — degrade gracefully (no auto-refresh) } diff --git a/packages/core/src/runtime/init.test.ts b/packages/core/src/runtime/init.test.ts index 2e73d4e79..1a35e922a 100644 --- a/packages/core/src/runtime/init.test.ts +++ b/packages/core/src/runtime/init.test.ts @@ -550,6 +550,56 @@ describe("initSandboxRuntimeModular", () => { expect(video.currentTime).toBe(9); }); + // Regression (#1838): a video authoring its OWN data-start (the normal case + // for a timed clip the studio positions on a track) took a fast literal- + // value path that skipped adding the host composition's start offset — + // unlike the no-own-data-start case above, which already went through + // resolveStartForElement and got the offset for free. The video played + // from the ROOT timeline's time instead of holding until its parent scene + // began, desyncing from the correctly-offset GSAP overlay in the same scene. + it("offsets a nested video's own data-start by its host composition's start", () => { + const root = document.createElement("div"); + root.setAttribute("data-composition-id", "main"); + root.setAttribute("data-root", "true"); + root.setAttribute("data-width", "1920"); + root.setAttribute("data-height", "1080"); + document.body.appendChild(root); + + const child = document.createElement("div"); + child.setAttribute("data-composition-id", "scene-2"); + child.setAttribute("data-start", "20"); + child.setAttribute("data-duration", "16"); + root.appendChild(child); + + const video = document.createElement("video"); + // Authored relative to scene-2's own local timeline, not the root's. + video.setAttribute("data-start", "0"); + video.setAttribute("data-duration", "16"); + child.appendChild(video); + Object.defineProperty(video, "duration", { value: 20, writable: true, configurable: true }); + Object.defineProperty(video, "paused", { value: true, writable: true, configurable: true }); + Object.defineProperty(video, "readyState", { value: 4, writable: true, configurable: true }); + Object.defineProperty(video, "currentTime", { value: 0, writable: true, configurable: true }); + video.load = () => {}; + video.play = () => Promise.resolve(); + + window.__timelines = { + main: createMockTimeline(40), + "scene-2": createMockTimeline(16), + }; + + initSandboxRuntimeModular(); + + const player = window.__player; + expect(player).toBeDefined(); + + // Root t=25 is 5s into scene-2 (which starts at root t=20) — the video + // must be 5s into its own local playback, not 25s (root time). + player?.seek(25); + + expect(video.currentTime).toBe(5); + }); + it("updates visibility for timed elements inside nested compositions", () => { const root = document.createElement("div"); root.setAttribute("data-composition-id", "main"); diff --git a/packages/core/src/runtime/init.ts b/packages/core/src/runtime/init.ts index 8fedc633b..c4eda6590 100644 --- a/packages/core/src/runtime/init.ts +++ b/packages/core/src/runtime/init.ts @@ -496,7 +496,16 @@ export function initSandboxRuntimeModular(): void { const resolveMediaStartSeconds = (element: Element, fallback = 0): number => { if (!element.hasAttribute("data-hf-auto-start") && element.hasAttribute("data-start")) { - return Math.max(0, Number(element.getAttribute("data-start") ?? 0) || 0); + // `data-start` is authored relative to the media element's OWN sub- + // composition, not the root timeline — `fallback` carries the host + // composition's resolved absolute start (see syncMediaForCurrentState's + // inheritedStart), so it must be added, not discarded. Skipping it made + // a nested video play from root t=0 instead of holding until its + // parent scene began (issue #1838) — resolveStartForElement's own + // absolute-expression branch already adds this same host offset, this + // fast literal-value path just didn't. + const own = Math.max(0, Number(element.getAttribute("data-start") ?? 0) || 0); + return own + fallback; } return resolveStartForElement(element, fallback); }; diff --git a/packages/parsers/src/gsapParser.ts b/packages/parsers/src/gsapParser.ts index 1aa5b269f..4272ec060 100644 --- a/packages/parsers/src/gsapParser.ts +++ b/packages/parsers/src/gsapParser.ts @@ -2338,7 +2338,11 @@ export function moveKeyframeInScript( ): string { const loc = locateAnimationWithFallback(script, animationId); if (!loc) return script; - const kfNode = findKeyframesObjectNode(loc.target.call.varsArg); + // Array-form keyframes can't host an arbitrary destination percentage — + // normalize to object form in place first (mirrors addKeyframeToScript). + const kfNode = + findKeyframesObjectNode(loc.target.call.varsArg) ?? + convertArrayKeyframesToObjectNode(loc.target.call.varsArg); if (!kfNode) return script; const match = findKeyframePropByPct(kfNode, fromPercentage); @@ -2395,7 +2399,11 @@ export function resizeKeyframedTweenInScript( ): string { const loc = locateAnimationWithFallback(script, animationId); if (!loc) return script; - const kfNode = findKeyframesObjectNode(loc.target.call.varsArg); + // Array-form keyframes can't host an arbitrary re-keyed percentage — + // normalize to object form in place first (mirrors addKeyframeToScript). + const kfNode = + findKeyframesObjectNode(loc.target.call.varsArg) ?? + convertArrayKeyframesToObjectNode(loc.target.call.varsArg); if (!kfNode) return script; const seen = new Set(); @@ -2595,7 +2603,13 @@ export function convertToKeyframesInScript( export function removeAllKeyframesFromScript(script: string, animationId: string): string { let loc = locateAnimationWithFallback(script, animationId); if (!loc) return script; - const kfNode = findKeyframesObjectNode(loc.target.call.varsArg); + // Array-form keyframes have no percentage-keyed props for + // filterPercentageProps to read — normalize to object form first (mirrors + // addKeyframeToScript/moveKeyframeInScript), otherwise this silently no-ops + // on every array-form tween. + const kfNode = + findKeyframesObjectNode(loc.target.call.varsArg) ?? + convertArrayKeyframesToObjectNode(loc.target.call.varsArg); if (!kfNode) return script; const kfEntries = filterPercentageProps(kfNode) diff --git a/packages/parsers/src/gsapWriter.parity.test.ts b/packages/parsers/src/gsapWriter.parity.test.ts index 2a066e799..6f1e4478d 100644 --- a/packages/parsers/src/gsapWriter.parity.test.ts +++ b/packages/parsers/src/gsapWriter.parity.test.ts @@ -1216,6 +1216,34 @@ describe("parity: moveKeyframeInScript (recast vs acorn)", () => { }); }); +// Regression: array-form `keyframes: [...]` has no explicit percentages, so +// locateWithKeyframes/findKeyframesObjectNode (which only match the object +// form) resolved to nothing and the move silently no-op'd — Studio's "Move to +// Playhead" and drag-to-retime did nothing on any array-authored tween. Both +// writers now normalize array → object form first (mirrors addKeyframeToScript). +describe("moveKeyframeInScript: array-form keyframes (recast + acorn parity)", () => { + for (const [label, move] of [ + ["acorn", moveKeyframeAcorn], + ["recast", moveKeyframeRecast], + ] as const) { + it(`${label}: normalizes the array then retimes the moved keyframe`, () => { + const id = acornId(KF_ADD_ARRAY_SCRIPT); + const out = move(KF_ADD_ARRAY_SCRIPT, id, 50, 75); + expect(out).not.toBe(KF_ADD_ARRAY_SCRIPT); + const kfs = shapeOf(out).keyframes?.keyframes ?? []; + expect(kfs.map((k) => k.percentage)).toEqual([0, 75, 100]); + expect(kfs.find((k) => k.percentage === 75)!.properties).toEqual({ x: 50, y: 80 }); + }); + } + + it("parity: both writers reparse to the same model", () => { + const id = acornId(KF_ADD_ARRAY_SCRIPT); + expect(modelOf(moveKeyframeAcorn(KF_ADD_ARRAY_SCRIPT, id, 50, 75))).toEqual( + modelOf(moveKeyframeRecast(KF_ADD_ARRAY_SCRIPT, id, 50, 75)), + ); + }); +}); + // ── resizeKeyframedTweenInScript (boundary drag: re-key + grow window) ──────── // Boundary drag-to-retime grows/shifts the tween window and RE-KEYS keyframes in // place. Unlike replace-with-keyframes (array rebuild), it must preserve author @@ -1272,6 +1300,67 @@ describe("resizeKeyframedTweenInScript: preserves author intent (acorn + recast) }); }); +// Regression: same array-form gap as moveKeyframeInScript above — boundary +// drag-to-retime re-keys existing keyframes to arbitrary percentages, which an +// array can't host. Both writers now normalize array → object form first. +const RESIZE_ARRAY_REMAP = [ + { from: 0, to: 0 }, + { from: 50, to: 25 }, + { from: 100, to: 100 }, +]; + +describe("resizeKeyframedTweenInScript: array-form keyframes (recast + acorn parity)", () => { + for (const [label, resize] of [ + ["acorn", resizeKeyframedTweenAcorn], + ["recast", resizeKeyframedTweenRecast], + ] as const) { + it(`${label}: normalizes the array then re-keys to the remapped percentages`, () => { + const id = acornId(KF_ADD_ARRAY_SCRIPT); + const out = resize(KF_ADD_ARRAY_SCRIPT, id, 0.2, 2, RESIZE_ARRAY_REMAP); + expect(out).not.toBe(KF_ADD_ARRAY_SCRIPT); + const kfs = shapeOf(out).keyframes?.keyframes ?? []; + expect(kfs.map((k) => k.percentage)).toEqual([0, 25, 100]); + expect(kfs.find((k) => k.percentage === 25)!.properties).toEqual({ x: 50, y: 80 }); + }); + } + + it("parity: both writers reparse to the same model", () => { + const id = acornId(KF_ADD_ARRAY_SCRIPT); + expect( + modelOf(resizeKeyframedTweenAcorn(KF_ADD_ARRAY_SCRIPT, id, 0.2, 2, RESIZE_ARRAY_REMAP)), + ).toEqual( + modelOf(resizeKeyframedTweenRecast(KF_ADD_ARRAY_SCRIPT, id, 0.2, 2, RESIZE_ARRAY_REMAP)), + ); + }); +}); + +describe("removeAllKeyframesFromScript: array-form keyframes (recast + acorn parity)", () => { + // Regression: the recast writer required an object-form `keyframes` node + // before doing anything, so array-form tweens silently no-op'd — the studio + // clears its keyframe cache optimistically on delete-all, so the diamonds + // vanished from the UI while the script kept every keyframe untouched. + for (const [label, removeAll] of [ + ["acorn", removeAllAcorn], + ["recast", removeAllRecast], + ] as const) { + it(`${label}: normalizes the array then collapses to the last keyframe`, () => { + const id = acornId(KF_ADD_ARRAY_SCRIPT); + const out = removeAll(KF_ADD_ARRAY_SCRIPT, id); + expect(out).not.toBe(KF_ADD_ARRAY_SCRIPT); + const shape = shapeOf(out); + expect(shape.keyframes).toBeUndefined(); + expect(shape.properties).toEqual({ x: 100, y: 0 }); + }); + } + + it("parity: both writers reparse to the same model", () => { + const id = acornId(KF_ADD_ARRAY_SCRIPT); + expect(modelOf(removeAllAcorn(KF_ADD_ARRAY_SCRIPT, id))).toEqual( + modelOf(removeAllRecast(KF_ADD_ARRAY_SCRIPT, id)), + ); + }); +}); + // ── addAnimationWithKeyframesToScript parity (recast vs acorn) ─────────────── // WS-3.C add path: both writers insert a new keyframed tl.to() call. The // inserted statement's authored model (selector, keyframes, duration, ease, diff --git a/packages/parsers/src/gsapWriterAcorn.ts b/packages/parsers/src/gsapWriterAcorn.ts index a6a9d40bc..224f8522e 100644 --- a/packages/parsers/src/gsapWriterAcorn.ts +++ b/packages/parsers/src/gsapWriterAcorn.ts @@ -1209,17 +1209,20 @@ export function moveKeyframeInScript( fromPercentage: number, toPercentage: number, ): string { - const located = locateWithKeyframes(script, animationId); + // ensureKeyframesNode (not locateWithKeyframes) so array-form `keyframes: [...]` + // tweens normalize to percentage-object form first — locateWithKeyframes alone + // bails on ArrayExpression, silently no-op'ing every move on an array-form tween. + const located = ensureKeyframesNode(script, animationId); if (!located) return script; - const { kfNode } = located; + const { script: src, kfNode } = located; const match = findKfPropByPct(kfNode, fromPercentage); - if (!match) return script; + if (!match) return src; // No-op ONLY for a negligible move (matches the drag's NOOP_EPSILON). The old // `collision.prop === match.prop` guard dropped EVERY sub-PCT_TOLERANCE (2%) // retime, because findKfPropByPct resolves the destination back onto the // from-keyframe — so a deliberate 1% drag committed nothing. - if (Math.abs(fromPercentage - toPercentage) < MOVE_NOOP_EPSILON_PCT) return script; + if (Math.abs(fromPercentage - toPercentage) < MOVE_NOOP_EPSILON_PCT) return src; // A destination keyframe is only a real collision (overwrite) when it's a // DIFFERENT keyframe; resolving back onto the from-keyframe is not. const dest = findKfPropByPct(kfNode, toPercentage); @@ -1234,15 +1237,15 @@ export function moveKeyframeInScript( if (collision && prop === collision.prop) continue; const pct = percentageFromKey(propKeyName(prop) ?? ""); if (Number.isNaN(pct)) continue; - entries.push({ pct, record: valueNodeToRecord(prop.value, script) }); + entries.push({ pct, record: valueNodeToRecord(prop.value, src) }); } - entries.push({ pct: toPercentage, record: valueNodeToRecord(match.prop.value, script) }); + entries.push({ pct: toPercentage, record: valueNodeToRecord(match.prop.value, src) }); entries.sort((a, b) => a.pct - b.pct); const body = entries .map((e) => `${JSON.stringify(`${e.pct}%`)}: ${recordToCode(e.record)}`) .join(", "); - const ms = new MagicString(script); + const ms = new MagicString(src); ms.overwrite(kfNode.start, kfNode.end, `{ ${body} }`); return ms.toString(); } @@ -1266,9 +1269,11 @@ export function resizeKeyframedTweenInScript( newDuration: number, pctRemap: ReadonlyArray<{ from: number; to: number }>, ): string { - const located = locateWithKeyframes(script, animationId); + // ensureKeyframesNode (not locateWithKeyframes) so array-form `keyframes: [...]` + // tweens normalize to percentage-object form first — see moveKeyframeInScript. + const located = ensureKeyframesNode(script, animationId); if (!located) return script; - const { target, kfNode } = located; + const { script: src, target, kfNode } = located; // Resolve every re-key against the ORIGINAL AST first (offsets stay stable), // then splice — distinct key nodes, so the overwrites never overlap. A Set @@ -1282,7 +1287,7 @@ export function resizeKeyframedTweenInScript( edits.push({ keyNode: match.prop.key, to }); } - const ms = new MagicString(script); + const ms = new MagicString(src); for (const { keyNode, to } of edits) { ms.overwrite(keyNode.start, keyNode.end, JSON.stringify(`${to}%`)); } diff --git a/packages/studio/src/hooks/gsapTweenSynth.test.ts b/packages/studio/src/hooks/gsapTweenSynth.test.ts new file mode 100644 index 000000000..3d8d77122 --- /dev/null +++ b/packages/studio/src/hooks/gsapTweenSynth.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, it } from "vitest"; +import type { GsapAnimation } from "@hyperframes/core/gsap-parser"; +import { synthesizeFlatTweenKeyframes } from "./gsapTweenSynth"; + +function anim(overrides: Partial): GsapAnimation { + return { + id: "a1", + targetSelector: "#title", + method: "to", + position: 0, + properties: {}, + ...overrides, + }; +} + +describe("synthesizeFlatTweenKeyframes", () => { + it("returns null for a set() static hold", () => { + expect(synthesizeFlatTweenKeyframes(anim({ method: "set", properties: { x: 5 } }))).toBeNull(); + }); + + // Regression: removeAllKeyframesFromScript collapses a keyframed tween to + // `tl.to(..., { duration: 0, immediateRender: true })` — a static hold with + // the same "not an animation" semantics as set(), but a different method + // string. Before this fix, only position-only (x/y) collapses were treated + // as holds elsewhere; a collapse to scale/opacity (or anything else) still + // synthesized a phantom keyframe diamond after "Delete All Keyframes". + it("returns null for a to() collapsed to a zero-duration immediateRender hold", () => { + const collapsed = anim({ + method: "to", + duration: 0, + // Both parsers encode a literal `immediateRender: true` as this raw + // source string, not a boolean — see gsapParser.ts/gsapParserAcorn.ts. + extras: { immediateRender: "__raw:true" }, + properties: { scale: 1, opacity: 1 }, + }); + expect(synthesizeFlatTweenKeyframes(collapsed)).toBeNull(); + }); + + it("still synthesizes keyframes for a genuine animated to() tween", () => { + const out = synthesizeFlatTweenKeyframes( + anim({ method: "to", duration: 1, properties: { opacity: 1 } }), + ); + expect(out).not.toBeNull(); + expect(out?.keyframes.map((k) => k.percentage)).toEqual([0, 100]); + }); + + it("still synthesizes keyframes for a duration:0 tween that isn't an immediateRender hold", () => { + // duration:0 alone isn't enough — only paired with immediateRender does it + // mean "this is a static hold, not an animation". + const out = synthesizeFlatTweenKeyframes( + anim({ method: "to", duration: 0, properties: { opacity: 1 } }), + ); + expect(out).not.toBeNull(); + }); +}); diff --git a/packages/studio/src/hooks/gsapTweenSynth.ts b/packages/studio/src/hooks/gsapTweenSynth.ts index 3256286ac..edb849f28 100644 --- a/packages/studio/src/hooks/gsapTweenSynth.ts +++ b/packages/studio/src/hooks/gsapTweenSynth.ts @@ -23,12 +23,18 @@ export function deduplicateKeyframes( // fallow-ignore-next-line complexity export function synthesizeFlatTweenKeyframes(anim: GsapAnimation): GsapKeyframesData | null { - if (anim.method === "set") { - // A `set` is a STATIC HOLD — a value applied at one point, not an animated - // keyframe. It must NOT synthesize a keyframe, or the timeline + panel show a - // phantom diamond for a value that doesn't animate. This holds for a base - // `gsap.set` (off-timeline) AND an on-timeline `tl.set`, and aligns the AST - // path with the runtime scan, which already skips every zero-duration set. + // Both parsers store extras as raw source text (`__raw:${code}`) so + // non-editable config like `stagger: {...}` survives verbatim — a literal + // `immediateRender: true` prints as exactly this string, not a boolean. + const hasImmediateRenderHold = anim.extras?.immediateRender === "__raw:true"; + if (anim.method === "set" || (anim.duration === 0 && hasImmediateRenderHold)) { + // A `set` — or a `to()`/`from()` collapsed to a zero-duration + // immediateRender hold (what removeAllKeyframesFromScript collapses a + // keyframed tween to) — is a STATIC HOLD: a value applied at one point, + // not an animated keyframe. It must NOT synthesize a keyframe, or the + // timeline + panel show a phantom diamond for a value that doesn't + // animate. This aligns the AST path with the runtime scan, which already + // skips every zero-duration set. return null; } const toProps = anim.properties; diff --git a/packages/studio/src/player/components/TimelineCanvas.tsx b/packages/studio/src/player/components/TimelineCanvas.tsx index 2215a1f0c..d1ba71f15 100644 --- a/packages/studio/src/player/components/TimelineCanvas.tsx +++ b/packages/studio/src/player/components/TimelineCanvas.tsx @@ -446,6 +446,7 @@ export const TimelineCanvas = memo(function TimelineCanvas({ onShiftClickKeyframe={onShiftClickKeyframe} onContextMenuKeyframe={onContextMenuKeyframe} onMoveKeyframe={onMoveKeyframe} + suppressClickRef={suppressClickRef} /> )} diff --git a/packages/studio/src/player/components/TimelineClipDiamonds.test.tsx b/packages/studio/src/player/components/TimelineClipDiamonds.test.tsx index c3d05defa..254e5d608 100644 --- a/packages/studio/src/player/components/TimelineClipDiamonds.test.tsx +++ b/packages/studio/src/player/components/TimelineClipDiamonds.test.tsx @@ -70,4 +70,146 @@ describe("TimelineClipDiamonds", () => { expect(onClickKeyframe).not.toHaveBeenCalled(); act(() => root.unmount()); }); + + // Regression: once the clip is selected, canDrag arms on every diamond + // press. A real click's few px of mouse/trackpad jitter then resolves (via + // the neighbour clamp) back onto ~the same position — "noop", not "move" — + // which fell through neither branch and silently did nothing: no + // selection, no retime. It must still count as the click it was. + it("treats a drag-armed press that resolves to a no-op move as a click", () => { + const onClickKeyframe = vi.fn(); + const onMoveKeyframe = vi.fn(); + const host = document.createElement("div"); + document.body.append(host); + const root = createRoot(host); + act(() => { + root.render( + , + ); + }); + const diamond = host.querySelector('button[title="50%"]'); + expect(diamond).not.toBeNull(); + + act(() => { + diamond!.dispatchEvent( + pointerEvent("pointerdown", { bubbles: true, button: 0, clientX: 100 }), + ); + // 4px of travel at a 5000px clip width is ~0.08 clip-% — above the drag + // threshold (so resolveKeyframeDrag doesn't short-circuit to "click" + // itself) but below the no-op epsilon once neighbour-clamped. + diamond!.dispatchEvent(pointerEvent("pointerup", { bubbles: true, button: 0, clientX: 104 })); + }); + + expect(onClickKeyframe).toHaveBeenCalledWith(50); + expect(onMoveKeyframe).not.toHaveBeenCalled(); + act(() => root.unmount()); + }); + + // Regression: a genuine retime (drag far enough to actually move the + // keyframe) committed the move but never selected/parked on the result — + // the diamond it was just dragged looked exactly like one nothing happened + // to. Select it at its NEW position too. + it("selects the keyframe at its new position after a real drag-retime", () => { + const onClickKeyframe = vi.fn(); + const onMoveKeyframe = vi.fn(); + const host = document.createElement("div"); + document.body.append(host); + const root = createRoot(host); + act(() => { + root.render( + , + ); + }); + const diamond = host.querySelector('button[title="50%"]'); + expect(diamond).not.toBeNull(); + + act(() => { + diamond!.dispatchEvent( + pointerEvent("pointerdown", { bubbles: true, button: 0, clientX: 100 }), + ); + // 4px at a 200px clip width is 2 clip-% — well past the no-op epsilon, + // a real retime. + diamond!.dispatchEvent(pointerEvent("pointerup", { bubbles: true, button: 0, clientX: 104 })); + }); + + expect(onMoveKeyframe).toHaveBeenCalledWith("clip-1", 50, 52); + expect(onClickKeyframe).toHaveBeenCalledWith(52); + act(() => root.unmount()); + }); + + // Regression: onClickKeyframe's state updates can re-render the diamond + // button out from under the gesture before the browser auto-synthesizes the + // "click" event that follows a button's pointerdown+pointerup. That orphaned + // click then bubbles to the ancestor clip's onClick, which toggles selection + // off whenever the clip is already selected — the state a diamond click + // always happens in — so every keyframe click immediately deselected its + // own clip. suppressClickRef lets that ancestor ignore the stray click. + it("arms suppressClickRef synchronously on a keyframe click", () => { + const suppressClickRef = { current: false }; + const host = document.createElement("div"); + document.body.append(host); + const root = createRoot(host); + act(() => { + root.render( + , + ); + }); + const diamond = host.querySelector('button[title="50%"]'); + expect(diamond).not.toBeNull(); + + act(() => { + diamond!.dispatchEvent(pointerEvent("pointerup", { bubbles: true, button: 0 })); + }); + + expect(suppressClickRef.current).toBe(true); + act(() => root.unmount()); + }); }); diff --git a/packages/studio/src/player/components/TimelineClipDiamonds.tsx b/packages/studio/src/player/components/TimelineClipDiamonds.tsx index febc4fa53..129104dfb 100644 --- a/packages/studio/src/player/components/TimelineClipDiamonds.tsx +++ b/packages/studio/src/player/components/TimelineClipDiamonds.tsx @@ -45,6 +45,10 @@ interface TimelineClipDiamondsProps { fromClipPercentage: number, toClipPercentage: number, ) => void; + /** Set while resolving a diamond press so the ancestor clip's onClick (which + * toggles selection off when already selected) ignores the native "click" + * the browser auto-synthesizes after this button's pointerdown+pointerup. */ + suppressClickRef?: React.RefObject; } const DIAMOND_RATIO = 0.8; @@ -76,6 +80,7 @@ export const TimelineClipDiamonds = memo(function TimelineClipDiamonds({ onShiftClickKeyframe, onContextMenuKeyframe, onMoveKeyframe, + suppressClickRef, }: TimelineClipDiamondsProps) { // Hooks must run before the early return below. const dragRef = useRef(null); @@ -83,6 +88,21 @@ export const TimelineClipDiamonds = memo(function TimelineClipDiamonds({ // (that optimistic hold was the #1763 flake). The atomic move-keyframe commit // on drop re-keys the diamond from source. const [preview, setPreview] = useState<{ kfKey: string; clipPct: number } | null>(null); + // The button element can re-render (reposition/unmount) synchronously from + // the state updates onClickKeyframe/onMoveKeyframe trigger, before the + // browser gets to auto-synthesize the "click" event that normally follows + // pointerdown+pointerup on a button. That orphaned click then fires on + // whatever ancestor is still there — the clip wrapper — whose own onClick + // toggles selection off when the clip is already selected (the state a + // diamond click always happens in). Suppressing it here is the same fix + // already used for clip drag/resize in useTimelineClipDrag.ts. + const suppressNextClick = () => { + if (!suppressClickRef) return; + suppressClickRef.current = true; + requestAnimationFrame(() => { + suppressClickRef.current = false; + }); + }; if (clipWidthPx < 20) return null; @@ -102,7 +122,19 @@ export const TimelineClipDiamonds = memo(function TimelineClipDiamonds({ const canDrag = isSelected && !!onMoveKeyframe; return ( -
+
{sorted.map((kf, i) => { if (i === 0) return null; const prev = sorted[i - 1]!; @@ -179,6 +211,7 @@ export const TimelineClipDiamonds = memo(function TimelineClipDiamonds({ // No drag armed (canDrag false / non-primary press) → treat as a click. if (!d || d.kfKey !== kfKey) { if (e.button !== 0) return; + suppressNextClick(); if (e.shiftKey) onShiftClickKeyframe?.(elementId, kf.percentage); else onClickKeyframe?.(kf.percentage); return; @@ -187,6 +220,7 @@ export const TimelineClipDiamonds = memo(function TimelineClipDiamonds({ dragRef.current = null; setPreview(null); e.currentTarget.releasePointerCapture?.(e.pointerId); + suppressNextClick(); const res = resolveKeyframeDrag({ pointerDownX: d.startX, pointerUpX: e.clientX, @@ -195,11 +229,20 @@ export const TimelineClipDiamonds = memo(function TimelineClipDiamonds({ draggedIndex: i, sortedClipPcts, }); - if (res.kind === "click") { + if (res.kind === "click" || res.kind === "noop") { + // "noop" is a press with enough pointer jitter to arm a drag (canDrag + // is on for every diamond once the clip is selected) that resolved + // back onto ~the same position — no real retime, so treat it as the + // click it was. Otherwise a normal click with a few px of mouse/ + // trackpad drift silently does nothing: no selection, no move. if (e.shiftKey) onShiftClickKeyframe?.(elementId, kf.percentage); else onClickKeyframe?.(kf.percentage); } else if (res.kind === "move" && res.toClipPct != null) { onMoveKeyframe?.(elementId, d.fromClipPct, res.toClipPct); + // A retime still targeted this exact diamond — park/select it at its + // new position, same as a plain click, or a drag that actually moved + // something looks identical to one that silently did nothing. + onClickKeyframe?.(res.toClipPct); } };