Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix tooltip race condition #5030

Merged
merged 2 commits into from
Nov 2, 2024
Merged

fix tooltip race condition #5030

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
02fc70b
fix tooltip race condition
sashankaryal Nov 1, 2024
706c53e
linting
benjaminpkane Nov 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 20 additions & 14 deletions app/packages/core/src/components/Modal/Modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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`
Expand Down Expand Up @@ -156,9 +156,9 @@ const Modal = () => {
if (activeLookerRef.current) {
// we handle close logic in modal + other places
return;
} else {
await modalCloseHandler();
}

await modalCloseHandler();
}
},
[]
Expand All @@ -168,7 +168,7 @@ const Modal = () => {

const isFullScreen = useRecoilValue(fos.fullscreen);

const { onNavigate } = useLookerHelpers();
const { closePanels } = useLookerHelpers();

const screenParams = useMemo(() => {
return isFullScreen
Expand All @@ -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) => {
Expand All @@ -200,7 +207,6 @@ const Modal = () => {
value={{
activeLookerRef,
setActiveLookerRef,
onLookerSetSubscribers,
}}
>
<ModalWrapper
Expand All @@ -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>
Expand Down
10 changes: 5 additions & 5 deletions app/packages/core/src/components/Modal/ModalNavigation.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ const Arrow = styled.span<{
}
`;

const ModalNavigation = ({ onNavigate }: { onNavigate: () => void }) => {
const ModalNavigation = ({ closePanels }: { closePanels: () => void }) => {
const showModalNavigationControls = useRecoilValue(
fos.showModalNavigationControls
);
Expand Down Expand Up @@ -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(() => {
Expand Down
52 changes: 2 additions & 50 deletions app/packages/core/src/components/Modal/ModalSamplePlugin.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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";

Expand All @@ -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);
Expand Down
39 changes: 37 additions & 2 deletions app/packages/core/src/components/Modal/hooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
};

Expand Down Expand Up @@ -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]
);
Comment on lines +104 to +114
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider optimizing event listener cleanup pattern

The current implementation removes and adds the event listener on every call. This could be simplified to avoid unnecessary DOM operations.

Consider this alternative implementation:

  return useCallback(
    (looker: fos.Lookers) => {
-     looker.removeEventListener("tooltip", tooltipEventHandler);
      looker.addEventListener("tooltip", tooltipEventHandler);

      return () => {
        looker.removeEventListener("tooltip", tooltipEventHandler);
      };
    },
    [tooltipEventHandler]
  );

This change:

  • Removes the redundant initial removeEventListener call
  • Still maintains proper cleanup through the returned cleanup function
  • Reduces unnecessary DOM operations
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return useCallback(
(looker: fos.Lookers) => {
looker.removeEventListener("tooltip", tooltipEventHandler);
looker.addEventListener("tooltip", tooltipEventHandler);
return () => {
looker.removeEventListener("tooltip", tooltipEventHandler);
};
},
[tooltipEventHandler]
);
return useCallback(
(looker: fos.Lookers) => {
looker.addEventListener("tooltip", tooltipEventHandler);
return () => {
looker.removeEventListener("tooltip", tooltipEventHandler);
};
},
[tooltipEventHandler]
);

};
1 change: 0 additions & 1 deletion app/packages/core/src/components/Modal/modal-context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Loading