fix tooltip race condition (#5030)

* fix tooltip race condition * linting --------- Co-authored-by: Benjamin Kane <[email protected]>
voxel51 · Nov 2, 2024 · e54c7a7 · e54c7a7
1 parent d586c4c
commit e54c7a7
Show file tree

Hide file tree

Showing 5 changed files with 64 additions and 72 deletions.
diff --git a/app/packages/core/src/components/Modal/Modal.tsx b/app/packages/core/src/components/Modal/Modal.tsx
@@ -1,17 +1,17 @@
 import { HelpPanel, JSONPanel } from "@fiftyone/components";
 import { OPERATOR_PROMPT_AREAS, OperatorPromptArea } from "@fiftyone/operators";
 import * as fos from "@fiftyone/state";
-import React, { useCallback, useEffect, useMemo, useRef } from "react";
+import React, { useCallback, useMemo, useRef } from "react";
 import ReactDOM from "react-dom";
 import { useRecoilCallback, useRecoilValue } from "recoil";
 import styled from "styled-components";
 import { ModalActionsRow } from "../Actions";
 import Sidebar from "../Sidebar";
-import { useLookerHelpers } from "./hooks";
-import { modalContext } from "./modal-context";
 import ModalNavigation from "./ModalNavigation";
 import { ModalSpace } from "./ModalSpace";
 import { TooltipInfo } from "./TooltipInfo";
+import { useLookerHelpers, useTooltipEventHandler } from "./hooks";
+import { modalContext } from "./modal-context";
 import { useModalSidebarRenderEntry } from "./use-sidebar-render-entry";
 
 const ModalWrapper = styled.div`
@@ -156,9 +156,9 @@ const Modal = () => {
           if (activeLookerRef.current) {
             // we handle close logic in modal + other places
             return;
-          } else {
-            await modalCloseHandler();
           }
+
+          await modalCloseHandler();
         }
       },
     []
@@ -168,7 +168,7 @@ const Modal = () => {
 
   const isFullScreen = useRecoilValue(fos.fullscreen);
 
-  const { onNavigate } = useLookerHelpers();
+  const { closePanels } = useLookerHelpers();
 
   const screenParams = useMemo(() => {
     return isFullScreen
@@ -178,14 +178,21 @@ const Modal = () => {
 
   const activeLookerRef = useRef<fos.Lookers>();
 
-  // this is so that other components can add event listeners to the active looker
-  const onLookerSetSubscribers = useRef<((looker: fos.Lookers) => void)[]>([]);
+  const addTooltipEventHandler = useTooltipEventHandler();
+  const removeTooltipEventHanlderRef = useRef<ReturnType<
+    typeof addTooltipEventHandler
+  > | null>(null);
 
-  const onLookerSet = useCallback((looker: fos.Lookers) => {
-    onLookerSetSubscribers.current.forEach((sub) => sub(looker));
+  const onLookerSet = useCallback(
+    (looker: fos.Lookers) => {
+      looker.addEventListener("close", modalCloseHandler);
 
-    looker.addEventListener("close", modalCloseHandler);
-  }, []);
+      // remove previous event listener
+      removeTooltipEventHanlderRef.current?.();
+      removeTooltipEventHanlderRef.current = addTooltipEventHandler(looker);
+    },
+    [modalCloseHandler, addTooltipEventHandler]
+  );
 
   const setActiveLookerRef = useCallback(
     (looker: fos.Lookers) => {
@@ -200,7 +207,6 @@ const Modal = () => {
       value={{
         activeLookerRef,
         setActiveLookerRef,
-        onLookerSetSubscribers,
       }}
     >
       <ModalWrapper
@@ -212,7 +218,7 @@ const Modal = () => {
         <TooltipInfo />
         <ModalContainer style={{ ...screenParams }}>
           <OperatorPromptArea area={OPERATOR_PROMPT_AREAS.DRAWER_LEFT} />
-          <ModalNavigation onNavigate={onNavigate} />
+          <ModalNavigation closePanels={closePanels} />
           <SpacesContainer>
             <ModalSpace />
           </SpacesContainer>

diff --git a/app/packages/core/src/components/Modal/ModalNavigation.tsx b/app/packages/core/src/components/Modal/ModalNavigation.tsx
@@ -44,7 +44,7 @@ const Arrow = styled.span<{
   }
 `;
 
-const ModalNavigation = ({ onNavigate }: { onNavigate: () => void }) => {
+const ModalNavigation = ({ closePanels }: { closePanels: () => void }) => {
   const showModalNavigationControls = useRecoilValue(
     fos.showModalNavigationControls
   );
@@ -75,21 +75,21 @@ const ModalNavigation = ({ onNavigate }: { onNavigate: () => void }) => {
       createDebouncedNavigator({
         isNavigationIllegalWhen: () => modalRef.current?.hasNext === false,
         navigateFn: (offset) => navigation?.next(offset).then(setModal),
-        onNavigationStart: onNavigate,
+        onNavigationStart: closePanels,
         debounceTime: 150,
       }),
-    [navigation, onNavigate, setModal]
+    [navigation, closePanels, setModal]
   );
 
   const previousNavigator = useMemo(
     () =>
       createDebouncedNavigator({
         isNavigationIllegalWhen: () => modalRef.current?.hasPrevious === false,
         navigateFn: (offset) => navigation?.previous(offset).then(setModal),
-        onNavigationStart: onNavigate,
+        onNavigationStart: closePanels,
         debounceTime: 150,
       }),
-    [navigation, onNavigate, setModal]
+    [navigation, closePanels, setModal]
   );
 
   useEffect(() => {

diff --git a/app/packages/core/src/components/Modal/ModalSamplePlugin.tsx b/app/packages/core/src/components/Modal/ModalSamplePlugin.tsx
@@ -1,10 +1,9 @@
 import { ErrorBoundary } from "@fiftyone/components";
 import * as fos from "@fiftyone/state";
-import React, { Suspense, useCallback, useEffect, useMemo } from "react";
-import { useRecoilCallback, useRecoilValue, useSetRecoilState } from "recoil";
+import React, { Suspense, useEffect } from "react";
+import { useRecoilValue, useSetRecoilState } from "recoil";
 import styled from "styled-components";
 import Group from "./Group";
-import { useModalContext } from "./hooks";
 import { Sample2D } from "./Sample2D";
 import { Sample3d } from "./Sample3d";
 
@@ -23,56 +22,9 @@ export const ModalSample = React.memo(() => {
   const isGroup = useRecoilValue(fos.isGroup);
   const is3D = useRecoilValue(fos.is3DDataset);
 
-  const tooltip = fos.useTooltip();
   const setIsTooltipLocked = useSetRecoilState(fos.isTooltipLocked);
   const setTooltipDetail = useSetRecoilState(fos.tooltipDetail);
 
-  const tooltipEventHandler = useRecoilCallback(
-    ({ snapshot, set }) =>
-      (e) => {
-        const isTooltipLocked = snapshot
-          .getLoadable(fos.isTooltipLocked)
-          .getValue();
-
-        if (e.detail) {
-          set(fos.tooltipDetail, e.detail);
-          if (!isTooltipLocked && e.detail?.coordinates) {
-            tooltip.setCoords(e.detail.coordinates);
-          }
-        } else if (!isTooltipLocked) {
-          set(fos.tooltipDetail, null);
-        }
-      },
-    [tooltip]
-  );
-
-  const { activeLookerRef, onLookerSetSubscribers } = useModalContext();
-
-  const addTooltipEventListener = useMemo(() => {
-    return (looker: fos.Lookers) => {
-      looker.addEventListener("tooltip", tooltipEventHandler);
-    };
-  }, []);
-
-  useEffect(() => {
-    onLookerSetSubscribers.current.push(addTooltipEventListener);
-
-    return () => {
-      activeLookerRef?.current?.removeEventListener(
-        "tooltip",
-        tooltipEventHandler
-      );
-      onLookerSetSubscribers.current = onLookerSetSubscribers.current.filter(
-        (fn) => fn !== addTooltipEventListener
-      );
-    };
-  }, [
-    activeLookerRef,
-    addTooltipEventListener,
-    onLookerSetSubscribers,
-    tooltipEventHandler,
-  ]);
-
   useEffect(() => {
     // reset tooltip state when modal is closed
     setIsTooltipLocked(false);

diff --git a/app/packages/core/src/components/Modal/hooks.ts b/app/packages/core/src/components/Modal/hooks.ts
@@ -16,15 +16,15 @@ export const useLookerHelpers = () => {
   jsonPanelRef.current = jsonPanel;
   helpPanelRef.current = helpPanel;
 
-  const onNavigate = useCallback(() => {
+  const closePanels = useCallback(() => {
     jsonPanelRef.current?.close();
     helpPanelRef.current?.close();
   }, []);
 
   return {
     jsonPanel,
     helpPanel,
-    onNavigate,
+    closePanels,
   };
 };
 
@@ -78,3 +78,38 @@ export const useModalContext = () => {
 
   return ctx;
 };
+
+export const useTooltipEventHandler = () => {
+  const tooltip = fos.useTooltip();
+
+  const tooltipEventHandler = useRecoilCallback(
+    ({ snapshot, set }) =>
+      (e) => {
+        const isTooltipLocked = snapshot
+          .getLoadable(fos.isTooltipLocked)
+          .getValue();
+
+        if (e.detail) {
+          set(fos.tooltipDetail, e.detail);
+          if (!isTooltipLocked && e.detail?.coordinates) {
+            tooltip.setCoords(e.detail.coordinates);
+          }
+        } else if (!isTooltipLocked) {
+          set(fos.tooltipDetail, null);
+        }
+      },
+    [tooltip]
+  );
+
+  return useCallback(
+    (looker: fos.Lookers) => {
+      looker.removeEventListener("tooltip", tooltipEventHandler);
+      looker.addEventListener("tooltip", tooltipEventHandler);
+
+      return () => {
+        looker.removeEventListener("tooltip", tooltipEventHandler);
+      };
+    },
+    [tooltipEventHandler]
+  );
+};
diff --git a/app/packages/core/src/components/Modal/modal-context.ts b/app/packages/core/src/components/Modal/modal-context.ts
@@ -4,7 +4,6 @@ import React, { createContext } from "react";
 interface ModalContextT {
   activeLookerRef: React.MutableRefObject<Lookers | undefined>;
   setActiveLookerRef: (looker: Lookers) => void;
-  onLookerSetSubscribers: React.MutableRefObject<((looker: Lookers) => void)[]>;
 }
 
 export const modalContext = createContext<ModalContextT | undefined>(undefined);