debounce modal navigation

app/packages/core/src/components/Modal/ModalNavigation.tsx

@@ -3,9 +3,10 @@ import {
   LookerArrowRightIcon,
 } from "@fiftyone/components";
 import * as fos from "@fiftyone/state";
-import React, { useCallback, useRef } from "react";
+import React, { useCallback, useEffect, useMemo, useRef } from "react";
 import { useRecoilValue, useRecoilValueLoadable } from "recoil";
 import styled from "styled-components";
+import { createDebouncedNavigator } from "./debouncedNavigator";
 
 const Arrow = styled.span<{
   $isRight?: boolean;
@@ -63,17 +64,40 @@ const ModalNavigation = ({ onNavigate }: { onNavigate: () => void }) => {
   const modal = useRecoilValue(fos.modalSelector);
   const navigation = useRecoilValue(fos.modalNavigation);
 
-  const navigateNext = useCallback(async () => {
-    onNavigate();
-    const result = await navigation?.next();
-    setModal(result);
-  }, [navigation, onNavigate, setModal]);
+  const modalRef = useRef(modal);
 
-  const navigatePrevious = useCallback(async () => {
-    onNavigate();
-    const result = await navigation?.previous();
-    setModal(result);
-  }, [onNavigate, navigation, setModal]);
+  modalRef.current = modal;
+
+  // important: make sure all dependencies of the navigators are referentially stable,
+  // or else the debouncing mechanism won't work
+  const nextNavigator = useMemo(
+    () =>
+      createDebouncedNavigator({
+        isNavigationIllegalWhen: () => modalRef.current?.hasNext === false,
+        navigateFn: (offset) => navigation?.next(offset).then(setModal),
+        onNavigationStart: onNavigate,
+        debounceTime: 150,
+      }),
+    [navigation, onNavigate, setModal]
+  );
+
+  const previousNavigator = useMemo(
+    () =>
+      createDebouncedNavigator({
+        isNavigationIllegalWhen: () => modalRef.current?.hasPrevious === false,
+        navigateFn: (offset) => navigation?.previous(offset).then(setModal),
+        onNavigationStart: onNavigate,
+        debounceTime: 150,
+      }),
+    [navigation, onNavigate, setModal]
+  );
+
+  useEffect(() => {
+    return () => {
+      nextNavigator.cleanup();
+      previousNavigator.cleanup();
+    };
+  }, [nextNavigator, previousNavigator]);
 
   const keyboardHandler = useCallback(
     (e: KeyboardEvent) => {
@@ -89,12 +113,12 @@ const ModalNavigation = ({ onNavigate }: { onNavigate: () => void }) => {
       }
 
       if (e.key === "ArrowLeft") {
-        navigatePrevious();
+        previousNavigator.navigate();
       } else if (e.key === "ArrowRight") {
-        navigateNext();
+        nextNavigator.navigate();
       }
     },
-    [navigateNext, navigatePrevious]
+    [nextNavigator, previousNavigator]
   );
 
   fos.useEventHandler(document, "keyup", keyboardHandler);
@@ -109,7 +133,7 @@ const ModalNavigation = ({ onNavigate }: { onNavigate: () => void }) => {
         <Arrow
           $isSidebarVisible={isSidebarVisible}
           $sidebarWidth={sidebarwidth}
-          onClick={navigatePrevious}
+          onClick={previousNavigator.navigate}
         >
           <LookerArrowLeftIcon data-cy="nav-left-button" />
         </Arrow>
@@ -119,7 +143,7 @@ const ModalNavigation = ({ onNavigate }: { onNavigate: () => void }) => {
           $isRight
           $isSidebarVisible={isSidebarVisible}
           $sidebarWidth={sidebarwidth}
-          onClick={navigateNext}
+          onClick={nextNavigator.navigate}
         >
           <LookerArrowRightIcon data-cy="nav-right-button" />
         </Arrow>

app/packages/core/src/components/Modal/debouncedNavigator.test.ts

@@ -0,0 +1,181 @@
+import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
+import { createDebouncedNavigator } from "./debouncedNavigator";
+
+describe("createDebouncedNavigator", () => {
+  let navigateFn: ReturnType<typeof vi.fn>;
+  let onNavigationStart: ReturnType<typeof vi.fn>;
+  let isNavigationIllegalWhen: ReturnType<typeof vi.fn>;
+  let debouncedNavigator: ReturnType<typeof createDebouncedNavigator>;
+
+  beforeEach(() => {
+    vi.useFakeTimers();
+    navigateFn = vi.fn();
+    onNavigationStart = vi.fn();
+    isNavigationIllegalWhen = vi.fn().mockReturnValue(false);
+    debouncedNavigator = createDebouncedNavigator({
+      isNavigationIllegalWhen,
+      navigateFn,
+      onNavigationStart,
+      debounceTime: 100,
+    });
+  });
+
+  afterEach(() => {
+    vi.clearAllTimers();
+    vi.restoreAllMocks();
+  });
+
+  it("should navigate immediately on the first call", () => {
+    debouncedNavigator.navigate();
+
+    expect(isNavigationIllegalWhen).toHaveBeenCalled();
+    expect(onNavigationStart).toHaveBeenCalledTimes(1);
+    expect(navigateFn).toHaveBeenCalledWith(1);
+  });
+
+  it("should debounce subsequent calls and accumulate offset", () => {
+    // immediate call
+    debouncedNavigator.navigate();
+    // accumulated
+    debouncedNavigator.navigate();
+    // accumulated
+    debouncedNavigator.navigate();
+
+    // only the first call
+    expect(onNavigationStart).toHaveBeenCalledTimes(1);
+
+    // advance time less than debounceTime
+    vi.advanceTimersByTime(50);
+    // another accumulated call
+    debouncedNavigator.navigate();
+
+    // advance time to trigger debounce after the last navigate
+    // need to advance full debounceTime after last call
+    vi.advanceTimersByTime(100);
+
+    // first immediate call + after debounce
+    expect(onNavigationStart).toHaveBeenCalledTimes(2);
+    // immediate call
+    expect(navigateFn).toHaveBeenCalledWith(1);
+    // accumulated calls
+    expect(navigateFn).toHaveBeenCalledWith(3);
+  });
+
+  it("should reset after debounce period", () => {
+    // immediate call
+    debouncedNavigator.navigate();
+    // accumulated
+    debouncedNavigator.navigate();
+
+    vi.advanceTimersByTime(100);
+
+    // next navigate call should be immediate again
+    debouncedNavigator.navigate();
+
+    expect(onNavigationStart).toHaveBeenCalledTimes(3);
+    expect(navigateFn).toHaveBeenNthCalledWith(1, 1);
+    // accumulated offset
+    expect(navigateFn).toHaveBeenNthCalledWith(2, 1);
+    expect(navigateFn).toHaveBeenNthCalledWith(3, 1);
+  });
+
+  it("should not navigate when isNavigationIllegalWhen returns true", () => {
+    isNavigationIllegalWhen.mockReturnValueOnce(true);
+
+    debouncedNavigator.navigate();
+
+    expect(isNavigationIllegalWhen).toHaveBeenCalled();
+    expect(onNavigationStart).not.toHaveBeenCalled();
+    expect(navigateFn).not.toHaveBeenCalled();
+  });
+
+  it("should cancel pending navigation when cleanup is called", () => {
+    // immediate call
+    debouncedNavigator.navigate();
+    // accumulated
+    debouncedNavigator.navigate();
+    debouncedNavigator.cleanup();
+
+    vi.advanceTimersByTime(200);
+
+    // only the immediate call
+    expect(onNavigationStart).toHaveBeenCalledTimes(1);
+    expect(navigateFn).toHaveBeenCalledTimes(1);
+    expect(navigateFn).toHaveBeenCalledWith(1);
+  });
+
+  it("should clear timeout when isNavigationIllegalWhen returns true during debounce", () => {
+    // immediate call
+    debouncedNavigator.navigate();
+    // accumulated
+    debouncedNavigator.navigate();
+
+    isNavigationIllegalWhen.mockReturnValue(true);
+    // should not accumulate further
+    debouncedNavigator.navigate();
+
+    vi.advanceTimersByTime(100);
+
+    // only the initial navigation
+    expect(onNavigationStart).toHaveBeenCalledTimes(1); //
+    // only immediate call
+    expect(navigateFn).toHaveBeenCalledTimes(1);
+    expect(navigateFn).toHaveBeenCalledWith(1);
+
+    // reset mock to allow navigation
+    isNavigationIllegalWhen.mockReturnValue(false);
+
+    // should navigate immediately
+    debouncedNavigator.navigate();
+
+    expect(onNavigationStart).toHaveBeenCalledTimes(2);
+    expect(navigateFn).toHaveBeenCalledTimes(2);
+    expect(navigateFn).toHaveBeenCalledWith(1);
+  });
+
+  it("should handle multiple sequences correctly", () => {
+    // first sequence
+    // immediate
+    debouncedNavigator.navigate();
+    // accumulated
+    debouncedNavigator.navigate();
+    debouncedNavigator.navigate();
+
+    vi.advanceTimersByTime(100);
+
+    expect(onNavigationStart).toHaveBeenCalledTimes(2);
+    expect(navigateFn).toHaveBeenNthCalledWith(1, 1);
+    expect(navigateFn).toHaveBeenNthCalledWith(2, 2);
+
+    // second sequence
+    // immediate call
+    debouncedNavigator.navigate();
+    // accumulated
+    debouncedNavigator.navigate();
+
+    vi.advanceTimersByTime(100);
+
+    expect(onNavigationStart).toHaveBeenCalledTimes(4);
+    expect(navigateFn).toHaveBeenNthCalledWith(3, 1);
+    expect(navigateFn).toHaveBeenNthCalledWith(4, 1);
+  });
+
+  it("should reset accumulatedOffset when isNavigationIllegalWhen returns true", () => {
+    // immediate call
+    debouncedNavigator.navigate();
+    // accumulated
+    debouncedNavigator.navigate();
+
+    isNavigationIllegalWhen.mockReturnValueOnce(true);
+
+    // should not accumulate further
+    debouncedNavigator.navigate();
+
+    vi.advanceTimersByTime(100);
+
+    // only the immediate call
+    expect(onNavigationStart).toHaveBeenCalledTimes(1);
+    expect(navigateFn).toHaveBeenCalledTimes(1);
+    expect(navigateFn).toHaveBeenCalledWith(1);
+  });
+});

app/packages/core/src/components/Modal/debouncedNavigator.ts

@@ -0,0 +1,70 @@
+interface DebouncedNavigatorOptions {
+  isNavigationIllegalWhen: () => boolean;
+  navigateFn: (offset: number) => Promise<void> | void;
+  onNavigationStart: () => void;
+  debounceTime?: number;
+}
+
+export function createDebouncedNavigator({
+  isNavigationIllegalWhen,
+  navigateFn,
+  onNavigationStart,
+  debounceTime = 100,
+}: DebouncedNavigatorOptions) {
+  let timeout: ReturnType<typeof setTimeout> | null = null;
+  let accumulatedOffset = 0;
+  let isFirstCall = true;
+
+  const cleanup = () => {
+    if (timeout) {
+      clearTimeout(timeout);
+      timeout = null;
+    }
+    accumulatedOffset = 0;
+    isFirstCall = true;
+  };
+
+  const navigate = () => {
+    if (isNavigationIllegalWhen()) {
+      if (timeout) {
+        clearTimeout(timeout);
+        timeout = null;
+      }
+      // Reset state variables
+      isFirstCall = true;
+      accumulatedOffset = 0;
+      return;
+    }
+
+    if (isFirstCall) {
+      // first invocation: navigate immediately
+      onNavigationStart();
+      navigateFn(1);
+      accumulatedOffset = 0;
+      isFirstCall = false;
+    } else {
+      // subsequently, accumulate offset
+      accumulatedOffset += 1;
+    }
+
+    // reset debounce timer
+    if (timeout) {
+      clearTimeout(timeout);
+    }
+
+    timeout = setTimeout(() => {
+      if (accumulatedOffset > 0) {
+        onNavigationStart();
+        navigateFn(accumulatedOffset);
+        accumulatedOffset = 0;
+      }
+      timeout = null;
+      isFirstCall = true;
+    }, debounceTime);
+  };
+
+  return {
+    navigate,
+    cleanup,
+  };
+}

app/packages/core/src/components/Modal/hooks.ts

@@ -1,16 +1,25 @@
 import * as fos from "@fiftyone/state";
 import { useHelpPanel, useJSONPanel } from "@fiftyone/state";
-import { useCallback, useContext } from "react";
+import { useCallback, useContext, useRef } from "react";
 import { useRecoilCallback } from "recoil";
 import { modalContext } from "./modal-context";
 
 export const useLookerHelpers = () => {
   const jsonPanel = useJSONPanel();
   const helpPanel = useHelpPanel();
+
+  // todo: jsonPanel and helpPanel are not referentially stable
+  // so use refs here
+  const jsonPanelRef = useRef(jsonPanel);
+  const helpPanelRef = useRef(helpPanel);
+
+  jsonPanelRef.current = jsonPanel;
+  helpPanelRef.current = helpPanel;
+
   const onNavigate = useCallback(() => {
-    jsonPanel.close();
-    helpPanel.close();
-  }, [helpPanel, jsonPanel]);
+    jsonPanelRef.current?.close();
+    helpPanelRef.current?.close();
+  }, []);
 
   return {
     jsonPanel,