From c2920f1f8b992e15688a1d52991d778d56bacbe2 Mon Sep 17 00:00:00 2001 From: Bastian Oppermann Date: Sun, 26 Jan 2025 07:29:34 +0100 Subject: [PATCH] [@mantine/hooks] Fix potential dangerous access of ref value in useEffect cleanup (#7404) Directly accessing the value of a ref in a `useEffect` cleanup function can lead to problems, as refs "live" outside of React's rendering cycle and it can therefore happen that the value of the ref changes between the first execution and the cleanup. --- .../use-event-listener/use-event-listener.ts | 8 +++++--- .../src/use-focus-within/use-focus-within.ts | 12 +++++++----- .../@mantine/hooks/src/use-hover/use-hover.ts | 12 +++++++----- .../@mantine/hooks/src/use-move/use-move.ts | 18 ++++++++++-------- .../src/use-radial-move/use-radial-move.ts | 18 ++++++++++-------- 5 files changed, 39 insertions(+), 29 deletions(-) diff --git a/packages/@mantine/hooks/src/use-event-listener/use-event-listener.ts b/packages/@mantine/hooks/src/use-event-listener/use-event-listener.ts index 482bba9268..42dcf093dc 100644 --- a/packages/@mantine/hooks/src/use-event-listener/use-event-listener.ts +++ b/packages/@mantine/hooks/src/use-event-listener/use-event-listener.ts @@ -8,9 +8,11 @@ export function useEventListener(null); useEffect(() => { - if (ref.current) { - ref.current.addEventListener(type, listener as any, options); - return () => ref.current?.removeEventListener(type, listener as any, options); + const node = ref.current; + + if (node) { + node.addEventListener(type, listener as any, options); + return () => node?.removeEventListener(type, listener as any, options); } return undefined; }, [listener, options]); diff --git a/packages/@mantine/hooks/src/use-focus-within/use-focus-within.ts b/packages/@mantine/hooks/src/use-focus-within/use-focus-within.ts index c26b8e66ba..f2caea39f2 100644 --- a/packages/@mantine/hooks/src/use-focus-within/use-focus-within.ts +++ b/packages/@mantine/hooks/src/use-focus-within/use-focus-within.ts @@ -41,13 +41,15 @@ export function useFocusWithin({ }; useEffect(() => { - if (ref.current) { - ref.current.addEventListener('focusin', handleFocusIn); - ref.current.addEventListener('focusout', handleFocusOut); + const node = ref.current; + + if (node) { + node.addEventListener('focusin', handleFocusIn); + node.addEventListener('focusout', handleFocusOut); return () => { - ref.current?.removeEventListener('focusin', handleFocusIn); - ref.current?.removeEventListener('focusout', handleFocusOut); + node?.removeEventListener('focusin', handleFocusIn); + node?.removeEventListener('focusout', handleFocusOut); }; } diff --git a/packages/@mantine/hooks/src/use-hover/use-hover.ts b/packages/@mantine/hooks/src/use-hover/use-hover.ts index fc487f62d5..f23bc4047c 100644 --- a/packages/@mantine/hooks/src/use-hover/use-hover.ts +++ b/packages/@mantine/hooks/src/use-hover/use-hover.ts @@ -7,13 +7,15 @@ export function useHover() { const onMouseLeave = useCallback(() => setHovered(false), []); useEffect(() => { - if (ref.current) { - ref.current.addEventListener('mouseenter', onMouseEnter); - ref.current.addEventListener('mouseleave', onMouseLeave); + const node = ref.current; + + if (node) { + node.addEventListener('mouseenter', onMouseEnter); + node.addEventListener('mouseleave', onMouseLeave); return () => { - ref.current?.removeEventListener('mouseenter', onMouseEnter); - ref.current?.removeEventListener('mouseleave', onMouseLeave); + node?.removeEventListener('mouseenter', onMouseEnter); + node?.removeEventListener('mouseleave', onMouseLeave); }; } diff --git a/packages/@mantine/hooks/src/use-move/use-move.ts b/packages/@mantine/hooks/src/use-move/use-move.ts index d6da69372d..164e31bc8e 100644 --- a/packages/@mantine/hooks/src/use-move/use-move.ts +++ b/packages/@mantine/hooks/src/use-move/use-move.ts @@ -34,13 +34,15 @@ export function useMove( }, []); useEffect(() => { + const node = ref.current; + const onScrub = ({ x, y }: UseMovePosition) => { cancelAnimationFrame(frame.current); frame.current = requestAnimationFrame(() => { - if (mounted.current && ref.current) { - ref.current.style.userSelect = 'none'; - const rect = ref.current.getBoundingClientRect(); + if (mounted.current && node) { + node.style.userSelect = 'none'; + const rect = node.getBoundingClientRect(); if (rect.width && rect.height) { const _x = clamp((x - rect.left) / rect.width, 0, 1); @@ -112,13 +114,13 @@ export function useMove( onScrub({ x: event.changedTouches[0].clientX, y: event.changedTouches[0].clientY }); }; - ref.current?.addEventListener('mousedown', onMouseDown); - ref.current?.addEventListener('touchstart', onTouchStart, { passive: false }); + node?.addEventListener('mousedown', onMouseDown); + node?.addEventListener('touchstart', onTouchStart, { passive: false }); return () => { - if (ref.current) { - ref.current.removeEventListener('mousedown', onMouseDown); - ref.current.removeEventListener('touchstart', onTouchStart); + if (node) { + node.removeEventListener('mousedown', onMouseDown); + node.removeEventListener('touchstart', onTouchStart); } }; }, [dir, onChange]); diff --git a/packages/@mantine/hooks/src/use-radial-move/use-radial-move.ts b/packages/@mantine/hooks/src/use-radial-move/use-radial-move.ts index c80e10ee24..b94b8dc195 100644 --- a/packages/@mantine/hooks/src/use-radial-move/use-radial-move.ts +++ b/packages/@mantine/hooks/src/use-radial-move/use-radial-move.ts @@ -63,10 +63,12 @@ export function useRadialMove( }, []); useEffect(() => { + const node = ref.current; + const update = (event: MouseEvent, done = false) => { - if (ref.current) { - ref.current.style.userSelect = 'none'; - const deg = getAngle([event.clientX, event.clientY], ref.current); + if (node) { + node.style.userSelect = 'none'; + const deg = getAngle([event.clientX, event.clientY], node); const newValue = normalizeRadialValue(deg, step || 1); onChange(newValue); @@ -122,13 +124,13 @@ export function useRadialMove( update(event.touches[0] as any); }; - ref.current?.addEventListener('mousedown', onMouseDown); - ref.current?.addEventListener('touchstart', handleTouchStart, { passive: false }); + node?.addEventListener('mousedown', onMouseDown); + node?.addEventListener('touchstart', handleTouchStart, { passive: false }); return () => { - if (ref.current) { - ref.current.removeEventListener('mousedown', onMouseDown); - ref.current.removeEventListener('touchstart', handleTouchStart); + if (node) { + node.removeEventListener('mousedown', onMouseDown); + node.removeEventListener('touchstart', handleTouchStart); } }; }, [onChange]);