refactor(packages): move slider throttle from commit to change (#1219)

packages/core/src/core/ui/time-slider/tests/time-slider-core.test.ts

@@ -42,7 +42,7 @@ describe('TimeSliderCore', () => {
         value: 0,
         min: 0,
         max: 100,
-        commitThrottle: 100,
+        changeThrottle: 100,
       });
     });
   });

packages/core/src/core/ui/time-slider/time-slider-core.ts

@@ -13,8 +13,8 @@ export interface TimeSliderProps extends SliderProps {
   min?: number | undefined;
   /** @internal Derived from `duration` — not user-settable. */
   max?: number | undefined;
-  /** Trailing-edge throttle (ms) for seek requests during drag. */
-  commitThrottle?: number | undefined;
+  /** Leading+trailing throttle (ms) for `onValueChange` during drag. */
+  changeThrottle?: number | undefined;
 }
 
 export interface TimeSliderState extends SliderState, Pick<MediaTimeState, 'currentTime' | 'duration' | 'seeking'> {
@@ -27,7 +27,7 @@ export class TimeSliderCore extends SliderCore {
   static override readonly defaultProps: NonNullableObject<TimeSliderProps> = {
     ...SliderCore.defaultProps,
     label: 'Seek',
-    commitThrottle: 100,
+    changeThrottle: 100,
   };
 
   #props = { ...TimeSliderCore.defaultProps };

packages/core/src/dom/ui/slider.ts

@@ -25,14 +25,17 @@ export interface SliderOptions {
   getLargeStepPercent: () => number;
 
   /**
-   * Trailing-edge throttle (ms) for `onValueCommit` during drag. When `> 0`,
-   * `onValueCommit` fires periodically while dragging, then a final unthrottled
-   * commit fires on pointer release. `0` (default) disables — commits only on release.
+   * Leading+trailing throttle (ms) for `onValueChange` during drag. When
+   * `> 0`, `onValueChange` fires immediately on the first drag move (leading
+   * edge), then at most once per window during subsequent moves. `0` (default)
+   * disables throttling — `onValueChange` fires on every pointermove.
    */
-  commitThrottle?: number | undefined;
+  changeThrottle?: number | undefined;
   /** Adjust a raw 0–100 percent for thumb alignment. Enables `adjustForAlignment()`. */
   adjustPercent?: ((rawPercent: number, thumbSize: number, trackSize: number) => number) | undefined;
+  /** Fires continuously as the value changes (every pointermove during drag, keyboard steps). */
   onValueChange?: ((percent: number) => void) | undefined;
+  /** Fires once when the user commits the value (pointer release, keyboard step). */
   onValueCommit?: ((percent: number) => void) | undefined;
   onDragStart?: (() => void) | undefined;
   onDragEnd?: (() => void) | undefined;
@@ -86,16 +89,29 @@ export function createSlider(options: SliderOptions): SliderApi {
   });
 
   const abort = new AbortController();
-  const commitThrottleMs = options.commitThrottle ?? 0;
+  const changeThrottleMs = options.changeThrottle ?? 0;
 
   let isDragging = false,
     moveCount = 0,
     cachedRTL = false,
     cachedRect: DOMRect | null = null,
-    capturedPointerId: number | null = null;
+    capturedPointerId: number | null = null,
+    lastDragPercent = 0,
+    committedOnRelease = false;
 
-  const throttledCommit =
-    commitThrottleMs > 0 ? throttle((percent: number) => options.onValueCommit?.(percent), commitThrottleMs) : null;
+  const throttledChange =
+    changeThrottleMs > 0
+      ? throttle((percent: number) => options.onValueChange?.(percent), changeThrottleMs, { leading: true })
+      : null;
+
+  /** Fire `onValueChange` — throttled during drag when `changeThrottle > 0`. */
+  function fireChange(percent: number, duringDrag: boolean): void {
+    if (duringDrag && throttledChange) {
+      throttledChange(percent);
+    } else {
+      options.onValueChange?.(percent);
+    }
+  }
 
   function releaseCapture(): void {
     if (isNull(capturedPointerId)) return;
@@ -114,16 +130,22 @@ export function createSlider(options: SliderOptions): SliderApi {
     if (!isDragging) {
       input.patch({ pointing: false });
     } else {
+      // Fire a final commit if pointerup didn't already handle it.
+      if (!committedOnRelease) {
+        options.onValueCommit?.(lastDragPercent);
+      }
+
       isDragging = false;
       input.patch({ dragging: false, pointing: false });
       options.onDragEnd?.();
     }
 
+    committedOnRelease = false;
     cleanup();
   }
 
   function cleanup() {
-    throttledCommit?.cancel();
+    throttledChange?.cancel();
     capturedPointerId = null;
     cachedRect = null;
   }
@@ -144,6 +166,7 @@ export function createSlider(options: SliderOptions): SliderApi {
       cachedRect = el.getBoundingClientRect();
       cachedRTL = options.isRTL();
       moveCount = 0;
+      committedOnRelease = false;
 
       releaseCapture();
       capturedPointerId = event.pointerId;
@@ -151,6 +174,7 @@ export function createSlider(options: SliderOptions): SliderApi {
 
       const percent = getPercentFromPointerEvent(event, cachedRect, options.getOrientation(), cachedRTL);
 
+      lastDragPercent = percent;
       input.patch({ pointing: true, pointerPercent: percent, dragPercent: percent });
       options.onValueChange?.(percent);
 
@@ -178,14 +202,14 @@ export function createSlider(options: SliderOptions): SliderApi {
 
         if (!isDragging && moveCount >= DRAG_THRESHOLD) {
           isDragging = true;
+          lastDragPercent = percent;
           input.patch({ dragging: true, dragPercent: percent, pointerPercent: percent });
           options.onDragStart?.();
-          options.onValueChange?.(percent);
-          throttledCommit?.(percent);
+          fireChange(percent, true);
         } else if (isDragging) {
+          lastDragPercent = percent;
           input.patch({ dragPercent: percent, pointerPercent: percent });
-          options.onValueChange?.(percent);
-          throttledCommit?.(percent);
+          fireChange(percent, true);
         } else {
           // Below drag threshold — update hover preview only.
           input.patch({ pointerPercent: percent });
@@ -207,9 +231,11 @@ export function createSlider(options: SliderOptions): SliderApi {
 
       const percent = getPercentFromPointerEvent(event, cachedRect!, options.getOrientation(), cachedRTL);
 
-      // Cancel pending throttled commit before the final unthrottled one.
-      throttledCommit?.cancel();
+      // Cancel any pending throttled change before the final unthrottled pair.
+      throttledChange?.cancel();
+      options.onValueChange?.(percent);
       options.onValueCommit?.(percent);
+      committedOnRelease = true;
     },
 
     onPointerLeave() {

packages/core/src/dom/ui/tests/slider.test.ts

@@ -873,31 +873,11 @@ describe('createSlider', () => {
     });
   });
 
-  describe('commitThrottle', () => {
-    it('does not fire onValueCommit during drag when commitThrottle is 0', () => {
+  describe('commit semantics', () => {
+    it('does not fire onValueCommit during drag', () => {
       const onValueCommit = vi.fn();
       const el = createMockElement({ left: 0, width: 200 });
-      const slider = createSlider(createOptions({ getElement: () => el, onValueCommit, commitThrottle: 0 }));
-
-      slider.rootProps.onPointerDown(pointerEvent({ clientX: 50 }));
-      onValueCommit.mockClear();
-
-      // Pass drag threshold
-      firePointerMove(slider, { clientX: 60 });
-      firePointerMove(slider, { clientX: 80 });
-      firePointerMove(slider, { clientX: 100 });
-
-      expect(onValueCommit).not.toHaveBeenCalled();
-
-      slider.destroy();
-    });
-
-    it('fires throttled onValueCommit during drag when commitThrottle > 0', () => {
-      vi.useFakeTimers();
-
-      const onValueCommit = vi.fn();
-      const el = createMockElement({ left: 0, width: 200 });
-      const slider = createSlider(createOptions({ getElement: () => el, onValueCommit, commitThrottle: 100 }));
+      const slider = createSlider(createOptions({ getElement: () => el, onValueCommit }));
 
       slider.rootProps.onPointerDown(pointerEvent({ clientX: 50 }));
       onValueCommit.mockClear();
@@ -906,103 +886,211 @@ describe('createSlider', () => {
       firePointerMove(slider, { clientX: 60 });
       firePointerMove(slider, { clientX: 80 });
       firePointerMove(slider, { clientX: 100 });
-      firePointerMove(slider, { clientX: 120 });
 
-      // Not yet — throttle hasn't fired
+      // Commit must not fire during drag — only on release.
       expect(onValueCommit).not.toHaveBeenCalled();
 
-      // Advance timer past throttle
-      vi.advanceTimersByTime(100);
+      slider.destroy();
+    });
+
+    it('fires onValueChange on pointerup before onValueCommit', () => {
+      const order: string[] = [];
+      const onValueChange = vi.fn(() => order.push('change'));
+      const onValueCommit = vi.fn(() => order.push('commit'));
+      const el = createMockElement({ left: 0, width: 200 });
+      const slider = createSlider(createOptions({ getElement: () => el, onValueChange, onValueCommit }));
+
+      slider.rootProps.onPointerDown(pointerEvent({ clientX: 50 }));
+      firePointerMove(slider, { clientX: 60 });
+      firePointerMove(slider, { clientX: 80 });
+
+      onValueChange.mockClear();
+      onValueCommit.mockClear();
+      order.length = 0;
+
+      firePointerUp(slider, { clientX: 100 });
+
+      expect(onValueChange).toHaveBeenCalledWith(50);
+      expect(onValueCommit).toHaveBeenCalledWith(50);
+      expect(order).toEqual(['change', 'commit']);
+
+      slider.destroy();
+    });
+
+    it('fires onValueCommit on stale drag exit with last drag percent', () => {
+      const onValueCommit = vi.fn();
+      const el = createMockElement({ left: 0, width: 200 });
+      const slider = createSlider(createOptions({ getElement: () => el, onValueCommit }));
+
+      slider.rootProps.onPointerDown(pointerEvent({ clientX: 50 }));
+      firePointerMove(slider, { clientX: 60 });
+      firePointerMove(slider, { clientX: 80 });
+      onValueCommit.mockClear();
+
+      // Stale drag: buttons = 0, mouse pointer
+      firePointerMove(slider, { clientX: 100, buttons: 0, pointerType: 'mouse' });
 
       expect(onValueCommit).toHaveBeenCalledOnce();
-      // Should have the latest drag percent (120/200 = 60%)
-      expect(onValueCommit).toHaveBeenCalledWith(60);
+      // Last drag percent was 40% (80/200) — the stale move doesn't update it.
+      expect(onValueCommit).toHaveBeenCalledWith(40);
+
+      slider.destroy();
+    });
+
+    it('fires onValueCommit on lostpointercapture without pointerup', () => {
+      const onValueCommit = vi.fn();
+      const el = createMockElement({ left: 0, width: 200 });
+      const slider = createSlider(createOptions({ getElement: () => el, onValueCommit }));
+
+      slider.rootProps.onPointerDown(pointerEvent({ clientX: 50 }));
+      firePointerMove(slider, { clientX: 60 });
+      firePointerMove(slider, { clientX: 80 });
+      onValueCommit.mockClear();
+
+      // Lost capture without pointerup (e.g., tab switch, pointercancel).
+      fireLostPointerCapture(slider);
+
+      expect(onValueCommit).toHaveBeenCalledOnce();
+      expect(onValueCommit).toHaveBeenCalledWith(40);
+
+      slider.destroy();
+    });
+
+    it('does not double-commit on pointerup followed by lostpointercapture', () => {
+      const onValueCommit = vi.fn();
+      const el = createMockElement({ left: 0, width: 200 });
+      const slider = createSlider(createOptions({ getElement: () => el, onValueCommit }));
+
+      slider.rootProps.onPointerDown(pointerEvent({ clientX: 50 }));
+      firePointerMove(slider, { clientX: 60 });
+      firePointerMove(slider, { clientX: 80 });
+      onValueCommit.mockClear();
+
+      // Normal flow: pointerup commits, then lostpointercapture cleans up.
+      firePointerUp(slider, { clientX: 100 });
+      fireLostPointerCapture(slider);
+
+      // Only one commit from pointerup.
+      expect(onValueCommit).toHaveBeenCalledOnce();
+      expect(onValueCommit).toHaveBeenCalledWith(50);
+
+      slider.destroy();
+    });
+
+    it('does not fire onValueCommit on lostpointercapture without drag', () => {
+      const onValueCommit = vi.fn();
+      const el = createMockElement({ left: 0, width: 200 });
+      const slider = createSlider(createOptions({ getElement: () => el, onValueCommit }));
+
+      slider.rootProps.onPointerDown(pointerEvent({ clientX: 50 }));
+      onValueCommit.mockClear();
+
+      // Lost capture before drag threshold was reached.
+      fireLostPointerCapture(slider);
+
+      expect(onValueCommit).not.toHaveBeenCalled();
+
+      slider.destroy();
+    });
+  });
+
+  describe('changeThrottle', () => {
+    it('fires onValueChange immediately on first drag move (leading edge)', () => {
+      vi.useFakeTimers();
+
+      const onValueChange = vi.fn();
+      const el = createMockElement({ left: 0, width: 200 });
+      const slider = createSlider(createOptions({ getElement: () => el, onValueChange, changeThrottle: 100 }));
+
+      slider.rootProps.onPointerDown(pointerEvent({ clientX: 50 }));
+      onValueChange.mockClear();
+
+      // Pass drag threshold — first drag move fires immediately (leading edge).
+      firePointerMove(slider, { clientX: 60 });
+      firePointerMove(slider, { clientX: 80 });
+
+      expect(onValueChange).toHaveBeenCalledOnce();
+      expect(onValueChange).toHaveBeenCalledWith(40);
 
       slider.destroy();
       vi.useRealTimers();
     });
 
-    it('uses trailing-edge: batches rapid moves into one commit', () => {
+    it('coalesces rapid moves during cooldown to trailing edge', () => {
       vi.useFakeTimers();
 
-      const onValueCommit = vi.fn();
+      const onValueChange = vi.fn();
       const el = createMockElement({ left: 0, width: 200 });
-      const slider = createSlider(createOptions({ getElement: () => el, onValueCommit, commitThrottle: 100 }));
+      const slider = createSlider(createOptions({ getElement: () => el, onValueChange, changeThrottle: 100 }));
 
       slider.rootProps.onPointerDown(pointerEvent({ clientX: 50 }));
-      onValueCommit.mockClear();
+      onValueChange.mockClear();
 
-      // Pass threshold
+      // Pass threshold — leading fires.
       firePointerMove(slider, { clientX: 60 });
       firePointerMove(slider, { clientX: 80 });
+      expect(onValueChange).toHaveBeenCalledOnce();
 
-      // Multiple rapid moves during drag
+      // More moves during cooldown.
       firePointerMove(slider, { clientX: 100 });
       firePointerMove(slider, { clientX: 120 });
       firePointerMove(slider, { clientX: 140 });
 
-      vi.advanceTimersByTime(100);
+      // Still only the leading call.
+      expect(onValueChange).toHaveBeenCalledOnce();
 
-      // Only one commit with the latest value (140/200 = 70%)
-      expect(onValueCommit).toHaveBeenCalledOnce();
-      expect(onValueCommit).toHaveBeenCalledWith(70);
+      // Trailing fires on cooldown expiry with latest value (140/200 = 70%).
+      vi.advanceTimersByTime(100);
+      expect(onValueChange).toHaveBeenCalledTimes(2);
+      expect(onValueChange).toHaveBeenLastCalledWith(70);
 
       slider.destroy();
       vi.useRealTimers();
     });
 
-    it('fires another throttled commit after the first one completes', () => {
-      vi.useFakeTimers();
-
-      const onValueCommit = vi.fn();
+    it('does not throttle onValueChange when changeThrottle is 0', () => {
+      const onValueChange = vi.fn();
       const el = createMockElement({ left: 0, width: 200 });
-      const slider = createSlider(createOptions({ getElement: () => el, onValueCommit, commitThrottle: 100 }));
+      const slider = createSlider(createOptions({ getElement: () => el, onValueChange, changeThrottle: 0 }));
 
       slider.rootProps.onPointerDown(pointerEvent({ clientX: 50 }));
-      onValueCommit.mockClear();
+      onValueChange.mockClear();
 
-      // Pass threshold and drag
+      // Every drag move fires immediately.
       firePointerMove(slider, { clientX: 60 });
       firePointerMove(slider, { clientX: 80 });
-      vi.advanceTimersByTime(100);
-      expect(onValueCommit).toHaveBeenCalledOnce();
+      firePointerMove(slider, { clientX: 100 });
 
-      // Continue dragging — should schedule another throttle
-      firePointerMove(slider, { clientX: 140 });
-      vi.advanceTimersByTime(100);
-
-      expect(onValueCommit).toHaveBeenCalledTimes(2);
-      expect(onValueCommit).toHaveBeenLastCalledWith(70);
+      expect(onValueChange).toHaveBeenCalledTimes(2);
 
       slider.destroy();
-      vi.useRealTimers();
     });
 
-    it('cancels throttle and fires final unthrottled commit on pointerup', () => {
+    it('cancels throttled change and fires unthrottled on pointerup', () => {
       vi.useFakeTimers();
 
-      const onValueCommit = vi.fn();
+      const onValueChange = vi.fn();
       const el = createMockElement({ left: 0, width: 200 });
-      const slider = createSlider(createOptions({ getElement: () => el, onValueCommit, commitThrottle: 100 }));
+      const slider = createSlider(createOptions({ getElement: () => el, onValueChange, changeThrottle: 100 }));
 
       slider.rootProps.onPointerDown(pointerEvent({ clientX: 50 }));
-      onValueCommit.mockClear();
+      onValueChange.mockClear();
 
-      // Pass threshold and drag
+      // Pass threshold (leading fires) and more moves.
       firePointerMove(slider, { clientX: 60 });
       firePointerMove(slider, { clientX: 80 });
       firePointerMove(slider, { clientX: 120 });
 
-      // Release before throttle fires
+      // Release before trailing fires.
       firePointerUp(slider, { clientX: 150 });
 
-      // Final commit with release position (150/200 = 75%)
-      expect(onValueCommit).toHaveBeenCalledOnce();
-      expect(onValueCommit).toHaveBeenCalledWith(75);
+      // Leading (threshold) + unthrottled pointerup. The coalesced moves were cancelled.
+      expect(onValueChange).toHaveBeenCalledTimes(2);
+      expect(onValueChange).toHaveBeenLastCalledWith(75);
 
-      // Advancing timer should NOT fire a stale throttled commit
+      // Advancing timer should NOT fire a stale trailing change.
       vi.advanceTimersByTime(200);
-      expect(onValueCommit).toHaveBeenCalledOnce();
+      expect(onValueChange).toHaveBeenCalledTimes(2);
 
       slider.destroy();
       vi.useRealTimers();
@@ -1011,85 +1099,56 @@ describe('createSlider', () => {
     it('cancels throttle on destroy', () => {
       vi.useFakeTimers();
 
-      const onValueCommit = vi.fn();
+      const onValueChange = vi.fn();
       const el = createMockElement({ left: 0, width: 200 });
-      const slider = createSlider(createOptions({ getElement: () => el, onValueCommit, commitThrottle: 100 }));
+      const slider = createSlider(createOptions({ getElement: () => el, onValueChange, changeThrottle: 100 }));
 
       slider.rootProps.onPointerDown(pointerEvent({ clientX: 50 }));
-      onValueCommit.mockClear();
+      onValueChange.mockClear();
 
-      // Pass threshold
+      // Pass threshold (leading fires).
       firePointerMove(slider, { clientX: 60 });
       firePointerMove(slider, { clientX: 80 });
+      // More moves pending.
+      firePointerMove(slider, { clientX: 120 });
 
       slider.destroy();
 
-      // Advancing timer should NOT fire
+      // Advancing timer should NOT fire the pending trailing change.
       vi.advanceTimersByTime(200);
-      expect(onValueCommit).not.toHaveBeenCalled();
+      expect(onValueChange).toHaveBeenCalledOnce();
 
       vi.useRealTimers();
     });
 
-    it('cancels throttle on lostpointercapture', () => {
-      vi.useFakeTimers();
-
-      const onValueCommit = vi.fn();
-      const el = createMockElement({ left: 0, width: 200 });
-      const slider = createSlider(createOptions({ getElement: () => el, onValueCommit, commitThrottle: 100 }));
-
-      slider.rootProps.onPointerDown(pointerEvent({ clientX: 50 }));
-      onValueCommit.mockClear();
-
-      // Pass threshold
-      firePointerMove(slider, { clientX: 60 });
-      firePointerMove(slider, { clientX: 80 });
-
-      fireLostPointerCapture(slider);
-
-      // Advancing timer should NOT fire
-      vi.advanceTimersByTime(200);
-      expect(onValueCommit).not.toHaveBeenCalled();
-
-      slider.destroy();
-      vi.useRealTimers();
-    });
-
-    it('does not throttle keyboard commits', () => {
-      const onValueCommit = vi.fn();
-      const slider = createSlider(createOptions({ getPercent: () => 50, onValueCommit, commitThrottle: 100 }));
+    it('does not throttle keyboard changes', () => {
+      const onValueChange = vi.fn();
+      const slider = createSlider(createOptions({ getPercent: () => 50, onValueChange, changeThrottle: 100 }));
 
       slider.thumbProps.onKeyDown(keyboardEvent('ArrowRight'));
 
-      // Keyboard commits fire immediately, not throttled
-      expect(onValueCommit).toHaveBeenCalledOnce();
-      expect(onValueCommit).toHaveBeenCalledWith(51);
+      expect(onValueChange).toHaveBeenCalledOnce();
+      expect(onValueChange).toHaveBeenCalledWith(51);
 
       slider.destroy();
     });
 
-    it('defaults commitThrottle to 0 when not provided', () => {
-      vi.useFakeTimers();
-
-      const onValueCommit = vi.fn();
+    it('defaults changeThrottle to 0 when not provided', () => {
+      const onValueChange = vi.fn();
       const el = createMockElement({ left: 0, width: 200 });
-      // No commitThrottle option — should default to 0 (disabled)
-      const slider = createSlider(createOptions({ getElement: () => el, onValueCommit }));
+      const slider = createSlider(createOptions({ getElement: () => el, onValueChange }));
 
       slider.rootProps.onPointerDown(pointerEvent({ clientX: 50 }));
-      onValueCommit.mockClear();
+      onValueChange.mockClear();
 
+      // Every drag move fires immediately (no throttle).
       firePointerMove(slider, { clientX: 60 });
       firePointerMove(slider, { clientX: 80 });
       firePointerMove(slider, { clientX: 100 });
 
-      vi.advanceTimersByTime(200);
-
-      // No throttled commits — default is disabled
-      expect(onValueCommit).not.toHaveBeenCalled();
+      expect(onValueChange).toHaveBeenCalledTimes(2);
 
       slider.destroy();
-      vi.useRealTimers();
     });
   });
 });