From 69020699dfb595194c8bf85117525045439bcdb2 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Tue, 28 Jul 2026 04:47:25 +0200 Subject: [PATCH] fix(studio): seek ruler clicks to the pressed position and round retime percentages A ruler press with no pointer movement settled the playhead at t=0 instead of the clicked time. handlePointerUp replays pendingClientXRef, which only the pointermove path wrote, so a plain click fell back to the ref's initial 0 and overwrote the correct pointerdown seek. Seed the ref on pointerdown. The keyframe retime move branch also returned the raw quotient while the resize branch rounded to 3dp, so values like 74.81203007518799% landed in the user's source and churned the diff on every drag. Round at the point of computation so the no-op test and the written value agree. --- .../components/editor/keyframeRetime.test.ts | 42 ++++++ .../src/components/editor/keyframeRetime.ts | 5 +- .../components/useTimelineRangeSelection.ts | 5 + .../useTimelineRangeSelectionScrub.test.tsx | 127 ++++++++++++++++++ 4 files changed, 178 insertions(+), 1 deletion(-) create mode 100644 packages/studio/src/player/components/useTimelineRangeSelectionScrub.test.tsx diff --git a/packages/studio/src/components/editor/keyframeRetime.test.ts b/packages/studio/src/components/editor/keyframeRetime.test.ts index 4047d5c77..81264f0ab 100644 --- a/packages/studio/src/components/editor/keyframeRetime.test.ts +++ b/packages/studio/src/components/editor/keyframeRetime.test.ts @@ -180,3 +180,45 @@ describe("resolveKeyframeRetime — guards", () => { expect(r.pctRemap).toEqual([]); }); }); + +describe("resolveKeyframeRetime — move percentages are rounded like the resize path", () => { + // The move branch used to return the raw quotient, so `74.81203007518799%` + // landed in the user's source and churned the diff on every drag. + const decimals = (n: number): number => String(n).split(".")[1]?.length ?? 0; + + it("rounds a repeating quotient to 3dp", () => { + const r = resolveKeyframeRetime({ + tweenStart: 2, + tweenDuration: 3, + keyframes: KEYFRAMES, + draggedTweenPct: 0, + dropAbsTime: 3, // (3-2)/3 = 33.333333333333336% + }); + expect(r.kind).toBe("move"); + expect(r.toTweenPct).toBe(33.333); + }); + + it("never emits more than 3 decimal places", () => { + for (const tweenDuration of [3, 7, 9, 11, 133]) { + const r = resolveKeyframeRetime({ + tweenStart: 2, + tweenDuration, + keyframes: KEYFRAMES, + draggedTweenPct: 0, + dropAbsTime: 3, + }); + expect(r.kind).toBe("move"); + expect(decimals(r.toTweenPct ?? 0)).toBeLessThanOrEqual(3); + } + }); + + it("leaves an already-short percentage untouched", () => { + const r = resolveKeyframeRetime({ + ...WINDOW, + keyframes: KEYFRAMES, + draggedTweenPct: 0, + dropAbsTime: 3, // (3-2)/4 = exactly 25% + }); + expect(r.toTweenPct).toBe(25); + }); +}); diff --git a/packages/studio/src/components/editor/keyframeRetime.ts b/packages/studio/src/components/editor/keyframeRetime.ts index f7fa1d134..c722e6abb 100644 --- a/packages/studio/src/components/editor/keyframeRetime.ts +++ b/packages/studio/src/components/editor/keyframeRetime.ts @@ -121,7 +121,10 @@ export function resolveKeyframeRetime(opts: { // Within the tween window → plain move (re-key the tween-%). if (dropAbsTime >= tweenStart - EPSILON_TIME && dropAbsTime <= tweenEnd + EPSILON_TIME) { - const toTweenPct = clamp(((dropAbsTime - tweenStart) / tweenDuration) * 100, 0, 100); + // Round here, not at the return: the no-op test below and the value written + // to source must be the same number. The resize branch already rounds, so + // this keeps both write paths at the authored 3dp precision. + const toTweenPct = round3(clamp(((dropAbsTime - tweenStart) / tweenDuration) * 100, 0, 100)); if (Math.abs(toTweenPct - draggedTweenPct) < NOOP_EPSILON_PCT) return { kind: "noop" }; return { kind: "move", toTweenPct }; } diff --git a/packages/studio/src/player/components/useTimelineRangeSelection.ts b/packages/studio/src/player/components/useTimelineRangeSelection.ts index 1a98e251c..c4cd93046 100644 --- a/packages/studio/src/player/components/useTimelineRangeSelection.ts +++ b/packages/studio/src/player/components/useTimelineRangeSelection.ts @@ -268,6 +268,11 @@ export function useTimelineRangeSelection({ if (!point || !scrollRect || isTimelineRulerPress(e.clientY, scrollRect.top)) { isDragging.current = true; setIsScrubbing(true); + // Seed the pending coordinate so a press with no pointermove still + // replays THIS x on pointerup. `updateScrubDrag` is the only other + // writer, so without this a plain click settles on the ref's initial + // 0 and clamps the playhead back to t=0. + pendingClientXRef.current = e.clientX; seekFromX(e.clientX); return; } diff --git a/packages/studio/src/player/components/useTimelineRangeSelectionScrub.test.tsx b/packages/studio/src/player/components/useTimelineRangeSelectionScrub.test.tsx new file mode 100644 index 000000000..61c93c6a4 --- /dev/null +++ b/packages/studio/src/player/components/useTimelineRangeSelectionScrub.test.tsx @@ -0,0 +1,127 @@ +// @vitest-environment happy-dom + +// Regression guard for the ruler-click seek. `handlePointerUp` replays +// `pendingClientXRef`, which only `updateScrubDrag` (pointermove) used to write, +// so a press with no move settled on the ref's initial 0 and clamped the +// playhead to t=0 — silently discarding the correct pointerdown seek. +import React, { act } from "react"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { mountReactHarness } from "../../hooks/domSelectionTestHarness"; +import { useTimelineRangeSelection } from "./useTimelineRangeSelection"; + +(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +/** Scroll container top; a press within RULER_H of this counts as a ruler press. */ +const SCROLL_TOP = 100; +/** Comfortably inside the ruler band (RULER_H is 28 at time of writing). */ +const RULER_Y = SCROLL_TOP + 4; + +type Handlers = ReturnType; + +function makeScrollEl(): HTMLDivElement { + const el = document.createElement("div"); + el.getBoundingClientRect = () => + ({ top: SCROLL_TOP, left: 0, width: 1000, height: 400 }) as DOMRect; + return el; +} + +function makePointerEvent(clientX: number, clientY: number): React.PointerEvent { + const currentTarget = document.createElement("div"); + currentTarget.setPointerCapture = vi.fn(); + return { + button: 0, + shiftKey: false, + metaKey: false, + ctrlKey: false, + pointerId: 1, + clientX, + clientY, + target: document.createElement("div"), + currentTarget, + } as unknown as React.PointerEvent; +} + +const roots: Array<{ unmount: () => void }> = []; + +afterEach(() => { + for (const root of roots.splice(0)) act(() => root.unmount()); +}); + +function setup(): { handlers: () => Handlers; seekFromX: ReturnType } { + const seekFromX = vi.fn(); + const scrollEl = makeScrollEl(); + let latest: Handlers | null = null; + + // Hoisted so they survive re-renders. The hook mutates `isDragging.current` + // across the press, and a fresh object per render would reset it to false. + const scrollRef = { current: scrollEl }; + const ppsRef = { current: 25 }; + const dragScrollRaf = { current: 0 }; + const isDragging = { current: false }; + const elementsRef = { current: [] }; + const trackOrderRef = { current: [] }; + const rowHeightsRef = { current: [] }; + + function Probe(): null { + latest = useTimelineRangeSelection({ + scrollRef, + ppsRef, + effectiveDuration: 30, + pps: 25, + seekFromX, + autoScrollDuringDrag: vi.fn(), + dragScrollRaf, + isDragging, + setShowPopover: vi.fn(), + elementsRef, + trackOrderRef, + rowHeightsRef, + contentOrigin: 0, + }); + return null; + } + + roots.push(mountReactHarness()); + return { + handlers: () => { + if (!latest) throw new Error("hook did not render"); + return latest; + }, + seekFromX, + }; +} + +describe("useTimelineRangeSelection — ruler press seek", () => { + it("replays the pressed x on pointerup when the pointer never moved", () => { + const { handlers, seekFromX } = setup(); + + act(() => handlers().handlePointerDown(makePointerEvent(1000, RULER_Y))); + act(() => handlers().handlePointerUp()); + + // Both the press and the settle must land on the SAME x. The bug settled on + // 0, so the LAST call is the one that decides where the playhead ends up. + expect(seekFromX).toHaveBeenCalledWith(1000); + expect(seekFromX).not.toHaveBeenCalledWith(0); + expect(seekFromX.mock.calls.at(-1)).toEqual([1000]); + }); + + it("does not fall back to x=0 for a press nearer the left edge either", () => { + const { handlers, seekFromX } = setup(); + + act(() => handlers().handlePointerDown(makePointerEvent(400, RULER_Y))); + act(() => handlers().handlePointerUp()); + + expect(seekFromX.mock.calls.at(-1)).toEqual([400]); + }); + + it("settles on the final pointermove x when the pointer did move", () => { + const { handlers, seekFromX } = setup(); + + act(() => handlers().handlePointerDown(makePointerEvent(700, RULER_Y))); + act(() => handlers().handlePointerMove(makePointerEvent(760, RULER_Y))); + act(() => handlers().handlePointerUp()); + + // The move must win over the seeded press coordinate. + expect(seekFromX.mock.calls.at(-1)).toEqual([760]); + }); +});