diff --git a/app/packages/core/src/components/Modal/ModalNavigation.tsx b/app/packages/core/src/components/Modal/ModalNavigation.tsx index d993eb2002..4b5dc4459b 100644 --- a/app/packages/core/src/components/Modal/ModalNavigation.tsx +++ b/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 }) => { @@ -119,7 +143,7 @@ const ModalNavigation = ({ onNavigate }: { onNavigate: () => void }) => { $isRight $isSidebarVisible={isSidebarVisible} $sidebarWidth={sidebarwidth} - onClick={navigateNext} + onClick={nextNavigator.navigate} > diff --git a/app/packages/core/src/components/Modal/debouncedNavigator.test.ts b/app/packages/core/src/components/Modal/debouncedNavigator.test.ts new file mode 100644 index 0000000000..91062edfc1 --- /dev/null +++ b/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; + let onNavigationStart: ReturnType; + let isNavigationIllegalWhen: ReturnType; + let debouncedNavigator: ReturnType; + + 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); + }); +}); diff --git a/app/packages/core/src/components/Modal/debouncedNavigator.ts b/app/packages/core/src/components/Modal/debouncedNavigator.ts new file mode 100644 index 0000000000..70dfa4524a --- /dev/null +++ b/app/packages/core/src/components/Modal/debouncedNavigator.ts @@ -0,0 +1,70 @@ +interface DebouncedNavigatorOptions { + isNavigationIllegalWhen: () => boolean; + navigateFn: (offset: number) => Promise | void; + onNavigationStart: () => void; + debounceTime?: number; +} + +export function createDebouncedNavigator({ + isNavigationIllegalWhen, + navigateFn, + onNavigationStart, + debounceTime = 100, +}: DebouncedNavigatorOptions) { + let timeout: ReturnType | 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, + }; +} diff --git a/app/packages/core/src/components/Modal/hooks.ts b/app/packages/core/src/components/Modal/hooks.ts index 2cdb6d6310..13c8105cbe 100644 --- a/app/packages/core/src/components/Modal/hooks.ts +++ b/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, diff --git a/app/packages/state/src/hooks/useExpandSample.ts b/app/packages/state/src/hooks/useExpandSample.ts index b80bb0c4d6..77431333c0 100644 --- a/app/packages/state/src/hooks/useExpandSample.ts +++ b/app/packages/state/src/hooks/useExpandSample.ts @@ -22,6 +22,7 @@ export type Sample = Exclude< export default (store: WeakMap) => { const setExpandedSample = useSetExpandedSample(); const setModalState = useSetModalState(); + return useRecoilCallback( ({ snapshot, set }) => async ({ @@ -64,20 +65,20 @@ export default (store: WeakMap) => { return { id: id.description, groupId }; }; - const next = async () => { - const result = await iter(cursor.next(1)); + const next = async (offset?: number) => { + const result = await iter(cursor.next(offset ?? 1)); return { - hasNext: Boolean(await cursor.next(1, true)), + hasNext: Boolean(await cursor.next(offset ?? 1, true)), hasPrevious: true, ...result, }; }; - const previous = async () => { - const result = await iter(cursor.next(-1)); + const previous = async (offset: number) => { + const result = await iter(cursor.next(-1 * (offset ?? 1))); return { hasNext: true, - hasPrevious: Boolean(await cursor.next(-1, true)), + hasPrevious: Boolean(await cursor.next(-1 * (offset ?? 1), true)), ...result, }; }; diff --git a/app/packages/state/src/recoil/modal.ts b/app/packages/state/src/recoil/modal.ts index 52baab87c2..3c30927f5b 100644 --- a/app/packages/state/src/recoil/modal.ts +++ b/app/packages/state/src/recoil/modal.ts @@ -119,8 +119,8 @@ export const isModalActive = selector({ }); export type ModalNavigation = { - next: () => Promise; - previous: () => Promise; + next: (offset?: number) => Promise; + previous: (offset?: number) => Promise; }; export const modalNavigation = atom({