From 262590e50740fcbcfbe51498932e4774a49aa377 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Mon, 4 Sep 2023 23:29:19 -0400 Subject: [PATCH] perf: Improve performance of rendering context menus --- app/components/ContextMenu/index.tsx | 143 ++++++++++-------- app/hooks/useDebouncedCallback.ts | 28 ---- app/hooks/useThrottledCallback.ts | 40 +++++ app/hooks/useWindowSize.ts | 27 ++-- .../AddPeopleToCollection.tsx | 14 +- 5 files changed, 145 insertions(+), 107 deletions(-) delete mode 100644 app/hooks/useDebouncedCallback.ts create mode 100644 app/hooks/useThrottledCallback.ts diff --git a/app/components/ContextMenu/index.tsx b/app/components/ContextMenu/index.tsx index c883118e0..ac78a027d 100644 --- a/app/components/ContextMenu/index.tsx +++ b/app/components/ContextMenu/index.tsx @@ -57,11 +57,6 @@ const ContextMenu: React.FC = ({ ...rest }: Props) => { const previousVisible = usePrevious(rest.visible); - const maxHeight = useMenuHeight({ - visible: rest.visible, - elementRef: rest.unstable_disclosureRef, - }); - const backgroundRef = React.useRef(null); const { ui } = useStores(); const { t } = useTranslation(); const { setIsMenuOpen } = useMenuContext(); @@ -99,21 +94,6 @@ const ContextMenu: React.FC = ({ t, ]); - // We must manually manage scroll lock for iOS support so that the scrollable - // element can be passed into body-scroll-lock. See: - // https://github.com/ariakit/ariakit/issues/469 - React.useEffect(() => { - const scrollElement = backgroundRef.current; - if (rest.visible && scrollElement && !isSubMenu) { - disableBodyScroll(scrollElement, { - reserveScrollBarGap: true, - }); - } - return () => { - scrollElement && !isSubMenu && enableBodyScroll(scrollElement); - }; - }, [isSubMenu, rest.visible]); - // Perf win – don't render anything until the menu has been opened if (!rest.visible && !previousVisible) { return null; @@ -124,51 +104,94 @@ const ContextMenu: React.FC = ({ return ( <> - {(props) => { - // kind of hacky, but this is an effective way of telling which way - // the menu will _actually_ be placed when taking into account screen - // positioning. - const topAnchor = props.style?.top === "0"; - // @ts-expect-error ts-migrate(2339) FIXME: Property 'placement' does not exist on type 'Extra... Remove this comment to see the full error message - const rightAnchor = props.placement === "bottom-end"; - - return ( - <> - {isMobile && ( - { - ev.preventDefault(); - ev.stopPropagation(); - rest.hide?.(); - }} - /> - )} - - - {rest.visible || rest.animating ? children : null} - - - - ); - }} + {(props) => ( + + {children} + + )} ); }; +type InnerContextMenuProps = MenuStateReturn & { + isSubMenu: boolean; + menuProps: { style?: React.CSSProperties; placement: string }; + children: React.ReactNode; +}; + +/** + * Inner context menu allows deferring expensive window measurement hooks etc + * until the menu is actually opened. + */ +const InnerContextMenu = (props: InnerContextMenuProps) => { + const { menuProps } = props; + // kind of hacky, but this is an effective way of telling which way + // the menu will _actually_ be placed when taking into account screen + // positioning. + const topAnchor = menuProps.style?.top === "0"; + const rightAnchor = menuProps.placement === "bottom-end"; + const backgroundRef = React.useRef(null); + const isMobile = useMobile(); + + const maxHeight = useMenuHeight({ + visible: props.visible, + elementRef: props.unstable_disclosureRef, + }); + + // We must manually manage scroll lock for iOS support so that the scrollable + // element can be passed into body-scroll-lock. See: + // https://github.com/ariakit/ariakit/issues/469 + React.useEffect(() => { + const scrollElement = backgroundRef.current; + if (props.visible && scrollElement && !props.isSubMenu) { + disableBodyScroll(scrollElement, { + reserveScrollBarGap: true, + }); + } + return () => { + scrollElement && !props.isSubMenu && enableBodyScroll(scrollElement); + }; + }, [props.isSubMenu, props.visible]); + + return ( + <> + {isMobile && ( + { + ev.preventDefault(); + ev.stopPropagation(); + props.hide?.(); + }} + /> + )} + + + {props.visible || props.animating ? props.children : null} + + + + ); +}; + export default ContextMenu; export const Backdrop = styled.div` diff --git a/app/hooks/useDebouncedCallback.ts b/app/hooks/useDebouncedCallback.ts deleted file mode 100644 index ddd6a5161..000000000 --- a/app/hooks/useDebouncedCallback.ts +++ /dev/null @@ -1,28 +0,0 @@ -import * as React from "react"; - -export default function useDebouncedCallback( - callback: (...params: T[]) => unknown, - wait: number -) { - // track args & timeout handle between calls - const argsRef = React.useRef(); - const timeout = React.useRef>(); - - function cleanup() { - if (timeout.current) { - clearTimeout(timeout.current); - } - } - - // make sure our timeout gets cleared if consuming component gets unmounted - React.useEffect(() => cleanup, []); - return function (...args: T[]) { - argsRef.current = args; - cleanup(); - timeout.current = setTimeout(() => { - if (argsRef.current) { - callback(...argsRef.current); - } - }, wait); - }; -} diff --git a/app/hooks/useThrottledCallback.ts b/app/hooks/useThrottledCallback.ts new file mode 100644 index 000000000..6e7b6e90c --- /dev/null +++ b/app/hooks/useThrottledCallback.ts @@ -0,0 +1,40 @@ +import throttle from "lodash/throttle"; +import * as React from "react"; +import useUnmount from "./useUnmount"; + +interface ThrottleSettings { + leading?: boolean | undefined; + trailing?: boolean | undefined; +} + +const defaultOptions: ThrottleSettings = { + leading: false, + trailing: true, +}; + +/** + * A hook that returns a throttled callback function. + * + * @param fn The function to throttle + * @param wait The time in ms to wait before calling the function + * @param dependencies The dependencies to watch for changes + * @param options The throttle options + */ +export default function useThrottledCallback any>( + fn: T, + wait = 250, + dependencies: React.DependencyList = [], + options: ThrottleSettings = defaultOptions +) { + const handler = React.useMemo( + () => throttle(fn, wait, options), + // eslint-disable-next-line react-hooks/exhaustive-deps + dependencies + ); + + useUnmount(() => { + handler.cancel(); + }); + + return handler; +} diff --git a/app/hooks/useWindowSize.ts b/app/hooks/useWindowSize.ts index 9bb4e7248..01c4715a4 100644 --- a/app/hooks/useWindowSize.ts +++ b/app/hooks/useWindowSize.ts @@ -1,6 +1,6 @@ import * as React from "react"; -import useDebouncedCallback from "./useDebouncedCallback"; import useEventListener from "./useEventListener"; +import useThrottledCallback from "./useThrottledCallback"; /** * A debounced hook that listens to the window resize event and returns the @@ -14,22 +14,25 @@ export default function useWindowSize() { height: window.innerHeight, }); - const handleResize = useDebouncedCallback(() => { - if ( - window.innerWidth !== windowSize.width || - window.innerHeight !== windowSize.height - ) { - setWindowSize({ - width: window.innerWidth, - height: window.innerHeight, - }); - } + const handleResize = useThrottledCallback(() => { + setWindowSize((state) => { + if ( + window.innerWidth === state.width && + window.innerHeight === state.height + ) { + return state; + } + + return { width: window.innerWidth, height: window.innerHeight }; + }); }, 100); useEventListener("resize", handleResize); // Call handler right away so state gets updated with initial window size - handleResize(); + React.useEffect(() => { + handleResize(); + }, [handleResize]); return windowSize; } diff --git a/app/scenes/CollectionPermissions/AddPeopleToCollection.tsx b/app/scenes/CollectionPermissions/AddPeopleToCollection.tsx index 2387fd1b2..9f6a4f4a3 100644 --- a/app/scenes/CollectionPermissions/AddPeopleToCollection.tsx +++ b/app/scenes/CollectionPermissions/AddPeopleToCollection.tsx @@ -13,8 +13,8 @@ import PaginatedList from "~/components/PaginatedList"; import Text from "~/components/Text"; import useBoolean from "~/hooks/useBoolean"; import useCurrentTeam from "~/hooks/useCurrentTeam"; -import useDebouncedCallback from "~/hooks/useDebouncedCallback"; import useStores from "~/hooks/useStores"; +import useThrottledCallback from "~/hooks/useThrottledCallback"; import useToasts from "~/hooks/useToasts"; import MemberListItem from "./components/MemberListItem"; @@ -31,12 +31,7 @@ function AddPeopleToCollection({ collection }: Props) { useBoolean(); const [query, setQuery] = React.useState(""); - const handleFilter = (ev: React.ChangeEvent) => { - setQuery(ev.target.value); - debouncedFetch(ev.target.value); - }; - - const debouncedFetch = useDebouncedCallback( + const debouncedFetch = useThrottledCallback( (query) => users.fetchPage({ query, @@ -44,6 +39,11 @@ function AddPeopleToCollection({ collection }: Props) { 250 ); + const handleFilter = (ev: React.ChangeEvent) => { + setQuery(ev.target.value); + void debouncedFetch(ev.target.value); + }; + const handleAddUser = async (user: User) => { try { await memberships.create({