From 5e7b209fb895fe793960ac31f3d0b0c3cce39305 Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Wed, 30 Oct 2024 20:45:24 +0545 Subject: [PATCH 01/13] debounced nav --- .../src/components/Modal/ModalNavigation.tsx | 99 +++++++++++++++++-- .../state/src/hooks/useExpandSample.ts | 14 +-- 2 files changed, 97 insertions(+), 16 deletions(-) diff --git a/app/packages/core/src/components/Modal/ModalNavigation.tsx b/app/packages/core/src/components/Modal/ModalNavigation.tsx index d993eb2002..d35baacba3 100644 --- a/app/packages/core/src/components/Modal/ModalNavigation.tsx +++ b/app/packages/core/src/components/Modal/ModalNavigation.tsx @@ -3,7 +3,7 @@ import { LookerArrowRightIcon, } from "@fiftyone/components"; import * as fos from "@fiftyone/state"; -import React, { useCallback, useRef } from "react"; +import React, { useCallback, useEffect, useRef } from "react"; import { useRecoilValue, useRecoilValueLoadable } from "recoil"; import styled from "styled-components"; @@ -63,17 +63,92 @@ 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); + const nextTimeoutRef = useRef(null); + const accumulatedNextOffsetRef = useRef(0); + + const previousTimeoutRef = useRef(null); + const accumulatedPreviousOffsetRef = useRef(0); + + const modalRef = useRef(modal); + + modalRef.current = modal; + + const navigateNext = useCallback(() => { + if (!modalRef.current?.hasNext) { + return; + } + + if (!nextTimeoutRef.current) { + // First click: navigate immediately + onNavigate(); + navigation?.next(1).then(setModal); + accumulatedNextOffsetRef.current = 0; + console.log(">!>Immediate next execution"); + } else { + // Subsequent clicks: accumulate offset + accumulatedNextOffsetRef.current += 1; + console.log(">!>Debouncing next"); + } + + // Reset debounce timer + if (nextTimeoutRef.current) { + clearTimeout(nextTimeoutRef.current); + } + + nextTimeoutRef.current = setTimeout(() => { + if (accumulatedNextOffsetRef.current > 0) { + onNavigate(); + navigation?.next(accumulatedNextOffsetRef.current).then(setModal); + accumulatedNextOffsetRef.current = 0; + } + nextTimeoutRef.current = null; + }, 200); }, [navigation, onNavigate, setModal]); - const navigatePrevious = useCallback(async () => { - onNavigate(); - const result = await navigation?.previous(); - setModal(result); - }, [onNavigate, navigation, setModal]); + const navigatePrevious = useCallback(() => { + if (!modalRef.current?.hasPrevious) { + return; + } + + if (!previousTimeoutRef.current) { + // First click: navigate immediately + onNavigate(); + navigation?.previous(1).then(setModal); + accumulatedPreviousOffsetRef.current = 0; + console.log(">!>Immediate previous execution"); + } else { + // Subsequent clicks: accumulate offset + accumulatedPreviousOffsetRef.current += 1; + console.log(">!>Debouncing previous"); + } + + // Reset debounce timer + if (previousTimeoutRef.current) { + clearTimeout(previousTimeoutRef.current); + } + + previousTimeoutRef.current = setTimeout(() => { + if (accumulatedPreviousOffsetRef.current > 0) { + onNavigate(); + navigation + ?.previous(accumulatedPreviousOffsetRef.current) + .then(setModal); + accumulatedPreviousOffsetRef.current = 0; + } + previousTimeoutRef.current = null; + }, 200); + }, [navigation, onNavigate, setModal]); + + useEffect(() => { + return () => { + if (nextTimeoutRef.current) { + clearTimeout(nextTimeoutRef.current); + } + if (previousTimeoutRef.current) { + clearTimeout(previousTimeoutRef.current); + } + }; + }, []); const keyboardHandler = useCallback( (e: KeyboardEvent) => { @@ -122,6 +197,10 @@ const ModalNavigation = ({ onNavigate }: { onNavigate: () => void }) => { onClick={navigateNext} > +
oi
+ {accumulatedNextOffsetRef.current > 0 && ( +
{accumulatedNextOffsetRef.current}
+ )} )} diff --git a/app/packages/state/src/hooks/useExpandSample.ts b/app/packages/state/src/hooks/useExpandSample.ts index b80bb0c4d6..97426efba3 100644 --- a/app/packages/state/src/hooks/useExpandSample.ts +++ b/app/packages/state/src/hooks/useExpandSample.ts @@ -8,6 +8,7 @@ import * as atoms from "../recoil/atoms"; import * as groupAtoms from "../recoil/groups"; import useSetExpandedSample from "./useSetExpandedSample"; import useSetModalState from "./useSetModalState"; +import { useCallback } from "react"; export type Sample = Exclude< Exclude< @@ -22,6 +23,7 @@ export type Sample = Exclude< export default (store: WeakMap) => { const setExpandedSample = useSetExpandedSample(); const setModalState = useSetModalState(); + return useRecoilCallback( ({ snapshot, set }) => async ({ @@ -64,20 +66,20 @@ export default (store: WeakMap) => { return { id: id.description, groupId }; }; - const next = async () => { - const result = await iter(cursor.next(1)); + const next = async (skip: number) => { + const result = await iter(cursor.next(skip)); return { - hasNext: Boolean(await cursor.next(1, true)), + hasNext: Boolean(await cursor.next(skip, true)), hasPrevious: true, ...result, }; }; - const previous = async () => { - const result = await iter(cursor.next(-1)); + const previous = async (skip: number) => { + const result = await iter(cursor.next(-1 * skip)); return { hasNext: true, - hasPrevious: Boolean(await cursor.next(-1, true)), + hasPrevious: Boolean(await cursor.next(-1 * skip, true)), ...result, }; }; From 94ba70526b44f7e82e8da4d1361946f9ad8f445f Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Wed, 30 Oct 2024 21:15:31 +0545 Subject: [PATCH 02/13] break things up and add unit tests --- .../src/components/Modal/ModalNavigation.tsx | 119 +++--------- .../Modal/debouncedNavigator.test.ts | 181 ++++++++++++++++++ .../components/Modal/debouncedNavigator.ts | 70 +++++++ 3 files changed, 282 insertions(+), 88 deletions(-) create mode 100644 app/packages/core/src/components/Modal/debouncedNavigator.test.ts create mode 100644 app/packages/core/src/components/Modal/debouncedNavigator.ts diff --git a/app/packages/core/src/components/Modal/ModalNavigation.tsx b/app/packages/core/src/components/Modal/ModalNavigation.tsx index d35baacba3..7631a37009 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, useEffect, 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,92 +64,38 @@ const ModalNavigation = ({ onNavigate }: { onNavigate: () => void }) => { const modal = useRecoilValue(fos.modalSelector); const navigation = useRecoilValue(fos.modalNavigation); - const nextTimeoutRef = useRef(null); - const accumulatedNextOffsetRef = useRef(0); - - const previousTimeoutRef = useRef(null); - const accumulatedPreviousOffsetRef = useRef(0); - const modalRef = useRef(modal); modalRef.current = modal; - const navigateNext = useCallback(() => { - if (!modalRef.current?.hasNext) { - return; - } - - if (!nextTimeoutRef.current) { - // First click: navigate immediately - onNavigate(); - navigation?.next(1).then(setModal); - accumulatedNextOffsetRef.current = 0; - console.log(">!>Immediate next execution"); - } else { - // Subsequent clicks: accumulate offset - accumulatedNextOffsetRef.current += 1; - console.log(">!>Debouncing next"); - } - - // Reset debounce timer - if (nextTimeoutRef.current) { - clearTimeout(nextTimeoutRef.current); - } - - nextTimeoutRef.current = setTimeout(() => { - if (accumulatedNextOffsetRef.current > 0) { - onNavigate(); - navigation?.next(accumulatedNextOffsetRef.current).then(setModal); - accumulatedNextOffsetRef.current = 0; - } - nextTimeoutRef.current = null; - }, 200); - }, [navigation, onNavigate, setModal]); - - const navigatePrevious = useCallback(() => { - if (!modalRef.current?.hasPrevious) { - return; - } - - if (!previousTimeoutRef.current) { - // First click: navigate immediately - onNavigate(); - navigation?.previous(1).then(setModal); - accumulatedPreviousOffsetRef.current = 0; - console.log(">!>Immediate previous execution"); - } else { - // Subsequent clicks: accumulate offset - accumulatedPreviousOffsetRef.current += 1; - console.log(">!>Debouncing previous"); - } - - // Reset debounce timer - if (previousTimeoutRef.current) { - clearTimeout(previousTimeoutRef.current); - } - - previousTimeoutRef.current = setTimeout(() => { - if (accumulatedPreviousOffsetRef.current > 0) { - onNavigate(); - navigation - ?.previous(accumulatedPreviousOffsetRef.current) - .then(setModal); - accumulatedPreviousOffsetRef.current = 0; - } - previousTimeoutRef.current = null; - }, 200); - }, [navigation, onNavigate, setModal]); + const nextNavigator = useMemo( + () => + createDebouncedNavigator({ + isNavigationIllegalWhen: () => modalRef.current?.hasNext === false, + navigateFn: (offset) => navigation?.next(offset).then(setModal), + onNavigationStart: onNavigate, + debounceTime: 200, + }), + [navigation, onNavigate, setModal] + ); + + const previousNavigator = useMemo( + () => + createDebouncedNavigator({ + isNavigationIllegalWhen: () => modalRef.current?.hasPrevious === false, + navigateFn: (offset) => navigation?.previous(offset).then(setModal), + onNavigationStart: onNavigate, + debounceTime: 200, + }), + [navigation, onNavigate, setModal] + ); useEffect(() => { return () => { - if (nextTimeoutRef.current) { - clearTimeout(nextTimeoutRef.current); - } - if (previousTimeoutRef.current) { - clearTimeout(previousTimeoutRef.current); - } + nextNavigator.cleanup(); + previousNavigator.cleanup(); }; - }, []); + }, [nextNavigator, previousNavigator]); const keyboardHandler = useCallback( (e: KeyboardEvent) => { @@ -164,12 +111,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); @@ -184,7 +131,7 @@ const ModalNavigation = ({ onNavigate }: { onNavigate: () => void }) => { @@ -194,13 +141,9 @@ const ModalNavigation = ({ onNavigate }: { onNavigate: () => void }) => { $isRight $isSidebarVisible={isSidebarVisible} $sidebarWidth={sidebarwidth} - onClick={navigateNext} + onClick={nextNavigator.navigate} > -
oi
- {accumulatedNextOffsetRef.current > 0 && ( -
{accumulatedNextOffsetRef.current}
- )} )} 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, + }; +} From 65d0ff6f171c42246663f7b773e1ee3410b82781 Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Wed, 30 Oct 2024 22:13:42 +0545 Subject: [PATCH 03/13] fix json panel and help panel ref integrity --- app/packages/core/src/components/Modal/hooks.ts | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) 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, From c21cb2b03833512d350ea47a64fb067d4b5a8978 Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Wed, 30 Oct 2024 22:14:07 +0545 Subject: [PATCH 04/13] add typing for offset --- app/packages/state/src/hooks/useExpandSample.ts | 13 ++++++------- app/packages/state/src/recoil/modal.ts | 4 ++-- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/app/packages/state/src/hooks/useExpandSample.ts b/app/packages/state/src/hooks/useExpandSample.ts index 97426efba3..77431333c0 100644 --- a/app/packages/state/src/hooks/useExpandSample.ts +++ b/app/packages/state/src/hooks/useExpandSample.ts @@ -8,7 +8,6 @@ import * as atoms from "../recoil/atoms"; import * as groupAtoms from "../recoil/groups"; import useSetExpandedSample from "./useSetExpandedSample"; import useSetModalState from "./useSetModalState"; -import { useCallback } from "react"; export type Sample = Exclude< Exclude< @@ -66,20 +65,20 @@ export default (store: WeakMap) => { return { id: id.description, groupId }; }; - const next = async (skip: number) => { - const result = await iter(cursor.next(skip)); + const next = async (offset?: number) => { + const result = await iter(cursor.next(offset ?? 1)); return { - hasNext: Boolean(await cursor.next(skip, true)), + hasNext: Boolean(await cursor.next(offset ?? 1, true)), hasPrevious: true, ...result, }; }; - const previous = async (skip: number) => { - const result = await iter(cursor.next(-1 * skip)); + const previous = async (offset: number) => { + const result = await iter(cursor.next(-1 * (offset ?? 1))); return { hasNext: true, - hasPrevious: Boolean(await cursor.next(-1 * skip, 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({ From da97121ac9e638cbb13e317bdd20a4c662dbaac0 Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Wed, 30 Oct 2024 22:14:34 +0545 Subject: [PATCH 05/13] change debounce time to 150 --- app/packages/core/src/components/Modal/ModalNavigation.tsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/packages/core/src/components/Modal/ModalNavigation.tsx b/app/packages/core/src/components/Modal/ModalNavigation.tsx index 7631a37009..4b5dc4459b 100644 --- a/app/packages/core/src/components/Modal/ModalNavigation.tsx +++ b/app/packages/core/src/components/Modal/ModalNavigation.tsx @@ -68,13 +68,15 @@ const ModalNavigation = ({ onNavigate }: { onNavigate: () => void }) => { 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: 200, + debounceTime: 150, }), [navigation, onNavigate, setModal] ); @@ -85,7 +87,7 @@ const ModalNavigation = ({ onNavigate }: { onNavigate: () => void }) => { isNavigationIllegalWhen: () => modalRef.current?.hasPrevious === false, navigateFn: (offset) => navigation?.previous(offset).then(setModal), onNavigationStart: onNavigate, - debounceTime: 200, + debounceTime: 150, }), [navigation, onNavigate, setModal] ); From ec42f0dc61dc0aeb3a71138415a00ce6357d0df4 Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Thu, 31 Oct 2024 00:11:47 +0545 Subject: [PATCH 06/13] more typing and cleanup --- .../src/components/Modal/debouncedNavigator.ts | 2 +- app/packages/core/src/components/Modal/hooks.ts | 4 ++-- app/packages/state/src/hooks/useExpandSample.ts | 16 +++++++++++----- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/app/packages/core/src/components/Modal/debouncedNavigator.ts b/app/packages/core/src/components/Modal/debouncedNavigator.ts index 70dfa4524a..136795bcc5 100644 --- a/app/packages/core/src/components/Modal/debouncedNavigator.ts +++ b/app/packages/core/src/components/Modal/debouncedNavigator.ts @@ -1,6 +1,6 @@ interface DebouncedNavigatorOptions { isNavigationIllegalWhen: () => boolean; - navigateFn: (offset: number) => Promise | void; + navigateFn: (offset: number) => Promise; onNavigationStart: () => void; debounceTime?: number; } diff --git a/app/packages/core/src/components/Modal/hooks.ts b/app/packages/core/src/components/Modal/hooks.ts index 13c8105cbe..ac61249265 100644 --- a/app/packages/core/src/components/Modal/hooks.ts +++ b/app/packages/core/src/components/Modal/hooks.ts @@ -10,8 +10,8 @@ export const useLookerHelpers = () => { // todo: jsonPanel and helpPanel are not referentially stable // so use refs here - const jsonPanelRef = useRef(jsonPanel); - const helpPanelRef = useRef(helpPanel); + const jsonPanelRef = useRef(jsonPanel); + const helpPanelRef = useRef(helpPanel); jsonPanelRef.current = jsonPanel; helpPanelRef.current = helpPanel; diff --git a/app/packages/state/src/hooks/useExpandSample.ts b/app/packages/state/src/hooks/useExpandSample.ts index 77431333c0..414a9587d1 100644 --- a/app/packages/state/src/hooks/useExpandSample.ts +++ b/app/packages/state/src/hooks/useExpandSample.ts @@ -65,20 +65,26 @@ export default (store: WeakMap) => { return { id: id.description, groupId }; }; - const next = async (offset?: number) => { - const result = await iter(cursor.next(offset ?? 1)); + const next = async (offset: number = 1) => { + const nextId = await cursor.next(offset); + const nextCheckId = await cursor.next(offset, true); + + const result = await iter(Promise.resolve(nextId)); return { - hasNext: Boolean(await cursor.next(offset ?? 1, true)), + hasNext: Boolean(nextCheckId), hasPrevious: true, ...result, }; }; const previous = async (offset: number) => { - const result = await iter(cursor.next(-1 * (offset ?? 1))); + const prevId = await cursor.next(-1 * offset); + const prevCheckId = await cursor.next(-1 * offset, true); + + const result = await iter(Promise.resolve(prevId)); return { hasNext: true, - hasPrevious: Boolean(await cursor.next(-1 * (offset ?? 1), true)), + hasPrevious: Boolean(prevCheckId), ...result, }; }; From 20c1c628cd70be284ead1eb5c0e42715b43ea5dd Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Thu, 31 Oct 2024 01:08:49 +0545 Subject: [PATCH 07/13] fix e2e --- .../app/src/components/AnalyticsConsent.tsx | 14 ++++++++++---- e2e-pw/src/oss/constants/index.ts | 1 + e2e-pw/src/oss/fixtures/loader.ts | 18 ++++++++++++++++++ e2e-pw/src/oss/poms/selector.ts | 2 -- .../src/oss/specs/plugins/histograms.spec.ts | 12 +++++------- 5 files changed, 34 insertions(+), 13 deletions(-) diff --git a/app/packages/app/src/components/AnalyticsConsent.tsx b/app/packages/app/src/components/AnalyticsConsent.tsx index a5370a9f31..34ed4d3d25 100644 --- a/app/packages/app/src/components/AnalyticsConsent.tsx +++ b/app/packages/app/src/components/AnalyticsConsent.tsx @@ -85,17 +85,23 @@ export default function AnalyticsConsent({ Help us improve FiftyOne - We use cookies to understand how FiftyOne is used and improve the product. - You can help us by enabling anonymous analytics. + We use cookies to understand how FiftyOne is used and improve the + product. You can help us by enabling anonymous analytics. - + Disable - + diff --git a/e2e-pw/src/oss/constants/index.ts b/e2e-pw/src/oss/constants/index.ts index f1dd4a3808..f070b5bf7c 100644 --- a/e2e-pw/src/oss/constants/index.ts +++ b/e2e-pw/src/oss/constants/index.ts @@ -2,6 +2,7 @@ import { Duration } from "src/oss/utils"; // time to wait for fiftyone app to load export const DEFAULT_APP_LOAD_TIMEOUT = Duration.Minutes(2); +export const POPUP_DISMISS_TIMEOUT = Duration.Seconds(5); export const DEFAULT_APP_PORT = 8787; export const DEFAULT_APP_HOSTNAME = "0.0.0.0"; diff --git a/e2e-pw/src/oss/fixtures/loader.ts b/e2e-pw/src/oss/fixtures/loader.ts index b794922ef8..5a3f7002ea 100644 --- a/e2e-pw/src/oss/fixtures/loader.ts +++ b/e2e-pw/src/oss/fixtures/loader.ts @@ -8,6 +8,7 @@ import { import { PythonRunner } from "src/shared/python-runner/python-runner"; import kill from "tree-kill"; import waitOn from "wait-on"; +import { POPUP_DISMISS_TIMEOUT } from "../constants"; import { Duration } from "../utils"; type WebServerProcessConfig = { @@ -188,5 +189,22 @@ export class OssLoader extends AbstractFiftyoneLoader { {}, { timeout: Duration.Seconds(10) } ); + + // close all pop-ups (cookies, new feature annoucement, etc.) + try { + await page + .getByTestId("btn-dismiss-query-performance-toast") + .click({ timeout: POPUP_DISMISS_TIMEOUT }); + } catch { + console.log("No query performance toast to dismiss"); + } + + try { + await page + .getByTestId("btn-disable-cookies") + .click({ timeout: POPUP_DISMISS_TIMEOUT }); + } catch { + console.log("No cookies button to disable"); + } } } diff --git a/e2e-pw/src/oss/poms/selector.ts b/e2e-pw/src/oss/poms/selector.ts index 71dbb7c1d8..0e81cbb96a 100644 --- a/e2e-pw/src/oss/poms/selector.ts +++ b/e2e-pw/src/oss/poms/selector.ts @@ -60,7 +60,5 @@ class SelectorAsserter { ) ).toBeVisible(); } - - await this.selectorPom.closeResults(); } } diff --git a/e2e-pw/src/oss/specs/plugins/histograms.spec.ts b/e2e-pw/src/oss/specs/plugins/histograms.spec.ts index 28c6a619b2..2fa715e8eb 100644 --- a/e2e-pw/src/oss/specs/plugins/histograms.spec.ts +++ b/e2e-pw/src/oss/specs/plugins/histograms.spec.ts @@ -75,15 +75,13 @@ test("histograms panel", async ({ histogram, panel }) => { "str", "tags", ]); - await expect(await histogram.locator).toHaveScreenshot("bool-histogram.png", { + await expect(histogram.locator).toHaveScreenshot("bool-histogram.png", { animations: "allow", }); + await histogram.selector.closeResults(); await histogram.selectField("float"); - await expect(await histogram.locator).toHaveScreenshot( - "float-histogram.png", - { - animations: "allow", - } - ); + await expect(histogram.locator).toHaveScreenshot("float-histogram.png", { + animations: "allow", + }); }); From 42086ac7b38400f6def6885447c1efa7b272809e Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Thu, 31 Oct 2024 01:23:41 +0545 Subject: [PATCH 08/13] make e2e required again --- .github/workflows/pr.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index bee364d1c9..9e66fa3da4 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -47,7 +47,7 @@ jobs: all-tests: runs-on: ubuntu-latest - needs: [build, lint, test] + needs: [build, lint, test, e2e] if: always() steps: - run: sh -c ${{ From fe27f764a2e7ae5363915d25f7e070280ebb7274 Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Thu, 31 Oct 2024 01:59:22 +0545 Subject: [PATCH 09/13] no need to block until canvas-loaded event in grid modal e2e --- e2e-pw/src/oss/specs/regression-tests/media-field.spec.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/e2e-pw/src/oss/specs/regression-tests/media-field.spec.ts b/e2e-pw/src/oss/specs/regression-tests/media-field.spec.ts index 9d54cd2ee1..bda2c4c94c 100644 --- a/e2e-pw/src/oss/specs/regression-tests/media-field.spec.ts +++ b/e2e-pw/src/oss/specs/regression-tests/media-field.spec.ts @@ -64,13 +64,8 @@ test.beforeAll(async ({ fiftyoneLoader }) => { dataset.save()`); }); -test("grid media field", async ({ eventUtils, fiftyoneLoader, grid, page }) => { +test("grid media field", async ({ fiftyoneLoader, grid, page }) => { await fiftyoneLoader.waitUntilGridVisible(page, datasetName); - const wait = eventUtils.getEventReceivedPromiseForPredicate( - "canvas-loaded", - ({ detail }) => detail.sampleFilepath === "/tmp/empty.png" - ); - await wait; await expect(grid.getNthLooker(0)).toHaveScreenshot("grid-media-field.png"); }); From 8f20094eded2cc2412db204e62c903aaed8a0841 Mon Sep 17 00:00:00 2001 From: Benjamin Kane Date: Thu, 31 Oct 2024 10:50:24 -0400 Subject: [PATCH 10/13] Add modal routing optimization (#5014) * remove modal navigation from router transactions * cache gathered paths * e2e updates * update selection test * lint, add comments --- app/packages/app/src/routing/RouterContext.ts | 211 +++++++++++------- app/packages/app/src/routing/matchPath.ts | 2 +- .../app/src/useEvents/useSetGroupSlice.ts | 6 +- .../app/src/useWriters/onSetGroupSlice.ts | 6 +- .../core/src/components/Grid/useRefreshers.ts | 9 +- .../state/src/recoil/pathData/counts.ts | 17 +- .../state/src/recoil/sidebarExpanded.ts | 14 -- e2e-pw/src/oss/poms/grid/index.ts | 8 +- e2e-pw/src/oss/poms/modal/index.ts | 14 +- e2e-pw/src/oss/specs/selection.spec.ts | 2 +- 10 files changed, 167 insertions(+), 122 deletions(-) diff --git a/app/packages/app/src/routing/RouterContext.ts b/app/packages/app/src/routing/RouterContext.ts index e684acaf8d..92a2114e16 100644 --- a/app/packages/app/src/routing/RouterContext.ts +++ b/app/packages/app/src/routing/RouterContext.ts @@ -69,6 +69,8 @@ export const createRouter = ( ): Router => { const history = isNotebook() ? createMemoryHistory() : createBrowserHistory(); + const getEntryResource = makeGetEntryResource(); + let currentEntryResource: Resource>; let nextCurrentEntryResource: Resource>; @@ -79,17 +81,27 @@ export const createRouter = ( >(); const update = (location: FiftyOneLocation, action?: Action) => { - requestAnimationFrame(() => { - for (const [_, [__, onPending]] of subscribers) onPending?.(); - }); currentEntryResource.load().then(({ cleanup }) => { - nextCurrentEntryResource = getEntryResource( - environment, - routes, - location as FiftyOneLocation, - false, - handleError - ); + try { + nextCurrentEntryResource = getEntryResource({ + environment, + routes, + location: location as FiftyOneLocation, + hard: false, + handleError, + }); + } catch (e) { + if (e instanceof Resource) { + // skip the page change if a resource is thrown + return; + } + + throw e; + } + + requestAnimationFrame(() => { + for (const [_, [__, onPending]] of subscribers) onPending?.(); + }); const loadingResource = nextCurrentEntryResource; loadingResource.load().then((entry) => { @@ -135,13 +147,13 @@ export const createRouter = ( load(hard = false) { const runUpdate = !currentEntryResource || hard; if (!currentEntryResource || hard) { - currentEntryResource = getEntryResource( + currentEntryResource = getEntryResource({ environment, - routes, - history.location as FiftyOneLocation, hard, - handleError - ); + handleError, + location: history.location as FiftyOneLocation, + routes, + }); } runUpdate && update(history.location as FiftyOneLocation); return currentEntryResource.load(); @@ -171,79 +183,114 @@ export const createRouter = ( }; }; -const getEntryResource = ( - environment: Environment, - routes: RouteDefinition[], - location: FiftyOneLocation, - hard = false, - handleError?: (error: unknown) => void -): Resource> => { - let route: RouteDefinition; - let matchResult: MatchPathResult | undefined = undefined; - for (let index = 0; index < routes.length; index++) { - route = routes[index]; - const match = matchPath( - location.pathname, - route, - location.search, - location.state - ); - - if (match) { - matchResult = match; - break; +const SKIP_EVENTS = new Set(["modal", "slice"]); + +const makeGetEntryResource = () => { + let currentLocation: FiftyOneLocation; + let currentResource: Resource>; + + const isReusable = (location: FiftyOneLocation) => { + if (currentLocation) { + return ( + SKIP_EVENTS.has(location.state.event || "") || + SKIP_EVENTS.has(currentLocation?.state.event || "") + ); } - } - if (matchResult == null) { - throw new NotFoundError({ path: location.pathname }); - } + return false; + }; - const fetchPolicy = hard ? "network-only" : "store-or-network"; + const getEntryResource = ({ + environment, + handleError, + hard = false, + location, + routes, + }: { + current?: FiftyOneLocation; + environment: Environment; + routes: RouteDefinition[]; + location: FiftyOneLocation; + hard: boolean; + handleError?: (error: unknown) => void; + }): Resource> => { + if (isReusable(location)) { + // throw the current resource (page) if it can be reused + throw currentResource; + } - return new Resource(() => { - return Promise.all([route.component.load(), route.query.load()]).then( - ([component, concreteRequest]) => { - const preloadedQuery = loadQuery( - environment, - concreteRequest, - matchResult.variables || {}, - { - fetchPolicy, - } - ); - - let resolveEntry: (entry: Entry) => void; - const promise = new Promise>((resolve) => { - resolveEntry = resolve; - }); - const subscription = fetchQuery( - environment, - concreteRequest, - matchResult.variables || {}, - { fetchPolicy } - ).subscribe({ - next: (data) => { - const { state: _, ...rest } = location; - resolveEntry({ - state: matchResult.variables as LocationState, - ...rest, - component, - data, - concreteRequest, - preloadedQuery, - cleanup: () => { - subscription?.unsubscribe(); - }, - }); - }, - error: (error) => handleError?.(error), - }); + let route: RouteDefinition; + let matchResult: MatchPathResult | undefined = undefined; + for (let index = 0; index < routes.length; index++) { + route = routes[index]; + const match = matchPath( + location.pathname, + route, + location.search, + location.state + ); - return promise; + if (match) { + matchResult = match; + break; } - ); - }); + } + + if (matchResult == null) { + throw new NotFoundError({ path: location.pathname }); + } + + const fetchPolicy = hard ? "network-only" : "store-or-network"; + + currentLocation = location; + currentResource = new Resource(() => { + return Promise.all([route.component.load(), route.query.load()]).then( + ([component, concreteRequest]) => { + const preloadedQuery = loadQuery( + environment, + concreteRequest, + matchResult.variables || {}, + { + fetchPolicy, + } + ); + + let resolveEntry: (entry: Entry) => void; + const promise = new Promise>((resolve) => { + resolveEntry = resolve; + }); + const subscription = fetchQuery( + environment, + concreteRequest, + matchResult.variables || {}, + { fetchPolicy } + ).subscribe({ + next: (data) => { + const { state: _, ...rest } = location; + resolveEntry({ + state: matchResult.variables as LocationState, + ...rest, + component, + data, + concreteRequest, + preloadedQuery, + cleanup: () => { + subscription?.unsubscribe(); + }, + }); + }, + error: (error) => handleError?.(error), + }); + + return promise; + } + ); + }); + + return currentResource; + }; + + return getEntryResource; }; export const RouterContext = React.createContext< diff --git a/app/packages/app/src/routing/matchPath.ts b/app/packages/app/src/routing/matchPath.ts index b8c7533359..1223647081 100644 --- a/app/packages/app/src/routing/matchPath.ts +++ b/app/packages/app/src/routing/matchPath.ts @@ -10,7 +10,7 @@ const compilePath = (path: string) => }); export type LocationState = { - event?: "modal"; + event?: "modal" | "slice"; fieldVisibility?: State.FieldVisibilityStage; groupSlice?: string; modalSelector?: ModalSelector; diff --git a/app/packages/app/src/useEvents/useSetGroupSlice.ts b/app/packages/app/src/useEvents/useSetGroupSlice.ts index 9ef152cea5..a58ba62754 100644 --- a/app/packages/app/src/useEvents/useSetGroupSlice.ts +++ b/app/packages/app/src/useEvents/useSetGroupSlice.ts @@ -15,7 +15,11 @@ const useSetGroupSlice: EventHandlerHook = ({ router, session }) => { session.current.sessionGroupSlice = slice; }); - router.push(pathname, { ...router.location.state, groupSlice: slice }); + router.push(pathname, { + ...router.location.state, + event: "slice", + groupSlice: slice, + }); }, [router, session] ); diff --git a/app/packages/app/src/useWriters/onSetGroupSlice.ts b/app/packages/app/src/useWriters/onSetGroupSlice.ts index f2a58d37ba..4045cac469 100644 --- a/app/packages/app/src/useWriters/onSetGroupSlice.ts +++ b/app/packages/app/src/useWriters/onSetGroupSlice.ts @@ -13,7 +13,11 @@ const onSetGroupSlice: RegisteredWriter<"sessionGroupSlice"> = const pathname = router.history.location.pathname + string; - router.push(pathname, { ...router.location.state, groupSlice: slice }); + router.push(pathname, { + ...router.location.state, + event: "slice", + groupSlice: slice, + }); if (env().VITE_NO_STATE) return; commitMutation(environment, { diff --git a/app/packages/core/src/components/Grid/useRefreshers.ts b/app/packages/core/src/components/Grid/useRefreshers.ts index f811d36b1a..10922ab09f 100644 --- a/app/packages/core/src/components/Grid/useRefreshers.ts +++ b/app/packages/core/src/components/Grid/useRefreshers.ts @@ -63,13 +63,8 @@ export default function useRefreshers() { useEffect( () => - subscribe(({ event }, { reset }, previous) => { - if ( - event === "fieldVisibility" || - event === "modal" || - previous?.event === "modal" - ) - return; + subscribe(({ event }, { reset }) => { + if (event === "fieldVisibility") return; // if not a modal page change, reset the grid location reset(gridAt); diff --git a/app/packages/state/src/recoil/pathData/counts.ts b/app/packages/state/src/recoil/pathData/counts.ts index 6059375fcd..1de0e8d8eb 100644 --- a/app/packages/state/src/recoil/pathData/counts.ts +++ b/app/packages/state/src/recoil/pathData/counts.ts @@ -146,6 +146,21 @@ export const counts = selectorFamily({ }, }); +export const gatheredPaths = selectorFamily({ + key: "gatheredPaths", + get: + ({ + embeddedDocType, + ftype, + }: { + embeddedDocType?: string | string[]; + ftype: string | string[]; + }) => + ({ get }) => { + return [...new Set(gatherPaths(get, ftype, embeddedDocType))]; + }, +}); + export const cumulativeCounts = selectorFamily< { [key: string]: number }, { @@ -160,7 +175,7 @@ export const cumulativeCounts = selectorFamily< get: ({ extended, path: key, modal, ftype, embeddedDocType }) => ({ get }) => { - return [...new Set(gatherPaths(get, ftype, embeddedDocType))].reduce( + return get(gatheredPaths({ ftype, embeddedDocType })).reduce( (result, path) => { const data = get(counts({ extended, modal, path: `${path}.${key}` })); for (const value in data) { diff --git a/app/packages/state/src/recoil/sidebarExpanded.ts b/app/packages/state/src/recoil/sidebarExpanded.ts index 2c83c5f8cd..2e3044e9d3 100644 --- a/app/packages/state/src/recoil/sidebarExpanded.ts +++ b/app/packages/state/src/recoil/sidebarExpanded.ts @@ -1,4 +1,3 @@ -import { subscribe } from "@fiftyone/relay"; import { DefaultValue, atom, atomFamily, selectorFamily } from "recoil"; export const sidebarExpandedStore = atomFamily< @@ -7,12 +6,6 @@ export const sidebarExpandedStore = atomFamily< >({ key: "sidebarExpandedStore", default: {}, - effects: [ - ({ node }) => - subscribe(({ event }, { reset }, previous) => { - event !== "modal" && previous?.event !== "modal" && reset(node); - }), - ], }); export const sidebarExpanded = selectorFamily< @@ -36,13 +29,6 @@ export const sidebarExpanded = selectorFamily< export const granularSidebarExpandedStore = atom<{ [key: string]: boolean }>({ key: "granularSidebarExpandedStore", default: {}, - effects: [ - ({ node }) => - subscribe( - ({ event }, { set }, previous) => - event !== "modal" && previous?.event !== "modal" && set(node, {}) - ), - ], }); export const granularSidebarExpanded = selectorFamily({ diff --git a/e2e-pw/src/oss/poms/grid/index.ts b/e2e-pw/src/oss/poms/grid/index.ts index daa42cd6f0..b05ab5e870 100644 --- a/e2e-pw/src/oss/poms/grid/index.ts +++ b/e2e-pw/src/oss/poms/grid/index.ts @@ -1,4 +1,4 @@ -import { expect, Locator, Page } from "src/oss/fixtures"; +import { Locator, Page, expect } from "src/oss/fixtures"; import { EventUtils } from "src/shared/event-utils"; import { GridActionsRowPom } from "../action-row/grid-actions-row"; import { GridSliceSelectorPom } from "../action-row/grid-slice-selector"; @@ -52,9 +52,7 @@ export class GridPom { } async openNthSample(n: number) { - await this.url.pageChange(() => - this.getNthLooker(n).click({ position: { x: 10, y: 80 } }) - ); + await this.getNthLooker(n).click({ position: { x: 10, y: 80 } }); } async openFirstSample() { @@ -80,7 +78,7 @@ export class GridPom { } async selectSlice(slice: string) { - await this.url.pageChange(() => this.sliceSelector.selectSlice(slice)); + await this.sliceSelector.selectSlice(slice); } /** diff --git a/e2e-pw/src/oss/poms/modal/index.ts b/e2e-pw/src/oss/poms/modal/index.ts index 176856107f..33a8e58cdc 100644 --- a/e2e-pw/src/oss/poms/modal/index.ts +++ b/e2e-pw/src/oss/poms/modal/index.ts @@ -1,4 +1,4 @@ -import { expect, Locator, Page } from "src/oss/fixtures"; +import { Locator, Page, expect } from "src/oss/fixtures"; import { EventUtils } from "src/shared/event-utils"; import { Duration } from "../../utils"; import { ModalTaggerPom } from "../action-row/tagger/modal-tagger"; @@ -118,11 +118,9 @@ export class ModalPom { ) { const currentSampleId = await this.sidebar.getSampleId(); - await this.url.pageChange(() => - this.locator - .getByTestId(`nav-${direction === "forward" ? "right" : "left"}-button`) - .click() - ); + await this.locator + .getByTestId(`nav-${direction === "forward" ? "right" : "left"}-button`) + .click(); // wait for sample id to change await this.page.waitForFunction((currentSampleId) => { @@ -219,9 +217,7 @@ export class ModalPom { async close() { // close by clicking outside of modal - await this.url.pageChange(() => - this.page.click("body", { position: { x: 0, y: 0 } }) - ); + await this.page.click("body", { position: { x: 0, y: 0 } }); } async navigateNextSample(allowErrorInfo = false) { diff --git a/e2e-pw/src/oss/specs/selection.spec.ts b/e2e-pw/src/oss/specs/selection.spec.ts index 604b0c015b..858ab29d3f 100644 --- a/e2e-pw/src/oss/specs/selection.spec.ts +++ b/e2e-pw/src/oss/specs/selection.spec.ts @@ -95,7 +95,7 @@ extensionDatasetNamePairs.forEach(([extension, datasetName]) => { await grid.openNthSample(1); await modal.assert.verifySelectionCount(1); - await grid.url.back(); + await modal.close(); await modal.assert.isClosed(); await grid.assert.isSelectionCountEqualTo(1); }); From 1ecde84d19efa5a00d1900e2f15d85a2c9a23f99 Mon Sep 17 00:00:00 2001 From: Benjamin Kane Date: Thu, 31 Oct 2024 10:50:45 -0400 Subject: [PATCH 11/13] freeze tensorflow dep (#5016) Co-authored-by: Br2850 --- requirements/github.txt | 2 +- requirements/test.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/requirements/github.txt b/requirements/github.txt index 64ae132326..cc56602780 100644 --- a/requirements/github.txt +++ b/requirements/github.txt @@ -4,7 +4,7 @@ numpy<2 pydicom>=2.2.0 shapely>=1.7.1 -tensorflow +tensorflow==2.17.0 tensorflow-datasets torch torchvision diff --git a/requirements/test.txt b/requirements/test.txt index 9173566d9c..5e619a4ee7 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -7,5 +7,5 @@ pytest-cov==4.0.0 pytest-mock==3.10.0 pytest-asyncio shapely -tensorflow +tensorflow==2.17.0 twine>=3 From 1a9c5d57e425fade23d5be43136fbf3802530461 Mon Sep 17 00:00:00 2001 From: Sashank Aryal <66688606+sashankaryal@users.noreply.github.com> Date: Thu, 31 Oct 2024 20:36:12 +0545 Subject: [PATCH 12/13] don't throw in spotlight.destroy() (#5017) * assert lastVisibleItem before computedHidden * console.error instead of throw when spotlight destroy called and spotlight is not attached --- .../components/src/components/AdaptiveMenu/index.tsx | 6 ++++-- app/packages/spotlight/src/index.ts | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/packages/components/src/components/AdaptiveMenu/index.tsx b/app/packages/components/src/components/AdaptiveMenu/index.tsx index 13b6f03077..3d86f7bbe1 100644 --- a/app/packages/components/src/components/AdaptiveMenu/index.tsx +++ b/app/packages/components/src/components/AdaptiveMenu/index.tsx @@ -53,8 +53,10 @@ export default function AdaptiveMenu(props: AdaptiveMenuPropsType) { if (!containerElem) return; hideOverflowingNodes(containerElem, (_: number, lastVisibleItemId) => { const lastVisibleItem = itemsById[lastVisibleItemId]; - const computedHidden = items.length - lastVisibleItem.index - 1; - setHidden(computedHidden); + if (lastVisibleItem?.index) { + const computedHidden = items.length - lastVisibleItem.index - 1; + setHidden(computedHidden); + } }); } diff --git a/app/packages/spotlight/src/index.ts b/app/packages/spotlight/src/index.ts index 689d69e8b1..081f181459 100644 --- a/app/packages/spotlight/src/index.ts +++ b/app/packages/spotlight/src/index.ts @@ -104,7 +104,8 @@ export default class Spotlight extends EventTarget { destroy(): void { if (!this.attached) { - throw new Error("spotlight is not attached"); + console.error("spotlight is not attached"); + return; } this.#backward?.remove(); From 5922bcdc28c8c80f30cba61a3d5fceb97c150009 Mon Sep 17 00:00:00 2001 From: Sashank Aryal <66688606+sashankaryal@users.noreply.github.com> Date: Thu, 31 Oct 2024 20:37:45 +0545 Subject: [PATCH 13/13] fix video regression + e2e (#5020) * add missing await * optional chaining for signal controller --- app/packages/looker/src/lookers/abstract.ts | 4 ++-- .../looker/src/lookers/imavid/ima-vid-frame-samples.ts | 4 ++-- app/packages/looker/src/util.ts | 6 +++--- .../group-video/default-video-slice-group.spec.ts | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/packages/looker/src/lookers/abstract.ts b/app/packages/looker/src/lookers/abstract.ts index 8c253e07bc..c290716c8c 100644 --- a/app/packages/looker/src/lookers/abstract.ts +++ b/app/packages/looker/src/lookers/abstract.ts @@ -397,12 +397,12 @@ export abstract class AbstractLooker< const argsWithSignal: AddEventListenerOptions = typeof optionsOrUseCapture === "boolean" ? { - signal: this.abortController.signal, + signal: this.abortController?.signal, capture: optionsOrUseCapture, } : { ...(optionsOrUseCapture ?? {}), - signal: this.abortController.signal, + signal: this.abortController?.signal, }; this.eventTarget.addEventListener(eventType, handler, argsWithSignal); } diff --git a/app/packages/looker/src/lookers/imavid/ima-vid-frame-samples.ts b/app/packages/looker/src/lookers/imavid/ima-vid-frame-samples.ts index df3fc41e8d..2e92b8f45d 100644 --- a/app/packages/looker/src/lookers/imavid/ima-vid-frame-samples.ts +++ b/app/packages/looker/src/lookers/imavid/ima-vid-frame-samples.ts @@ -88,7 +88,7 @@ export class ImaVidFrameSamples { sample.image = image; resolve(sampleId); }, - { signal: this.abortController.signal } + { signal: this.abortController?.signal } ); image.addEventListener( @@ -105,7 +105,7 @@ export class ImaVidFrameSamples { // setting src should trigger the load event image.src = BASE64_BLACK_IMAGE; }, - { signal: this.abortController.signal } + { signal: this.abortController?.signal } ); image.src = source; diff --git a/app/packages/looker/src/util.ts b/app/packages/looker/src/util.ts index 0209ec3a13..db64ba40ee 100644 --- a/app/packages/looker/src/util.ts +++ b/app/packages/looker/src/util.ts @@ -462,7 +462,7 @@ export const createWorker = ( (error) => { dispatchEvent("error", error); }, - { signal: abortController.signal } + { signal: abortController?.signal } ); worker.addEventListener( @@ -475,7 +475,7 @@ export const createWorker = ( dispatchEvent("error", new ErrorEvent("error", { error })); } }, - { signal: abortController.signal } + { signal: abortController?.signal } ); worker.postMessage({ @@ -496,7 +496,7 @@ export const createWorker = ( listeners[method].forEach((callback) => callback(worker, args)); }, - { signal: abortController.signal } + { signal: abortController?.signal } ); return worker; }; diff --git a/e2e-pw/src/oss/specs/regression-tests/group-video/default-video-slice-group.spec.ts b/e2e-pw/src/oss/specs/regression-tests/group-video/default-video-slice-group.spec.ts index 685514818f..d0b31470c8 100644 --- a/e2e-pw/src/oss/specs/regression-tests/group-video/default-video-slice-group.spec.ts +++ b/e2e-pw/src/oss/specs/regression-tests/group-video/default-video-slice-group.spec.ts @@ -71,7 +71,7 @@ test.describe("default video slice group", () => { }); test.beforeEach(async ({ page, fiftyoneLoader }) => { - fiftyoneLoader.waitUntilGridVisible(page, datasetName); + await fiftyoneLoader.waitUntilGridVisible(page, datasetName); }); test("video as default slice renders", async ({ grid, modal }) => {