Skip to content

Commit

Permalink
[@mantine/hooks] Fix potential dangerous access of ref value in useEf…
Browse files Browse the repository at this point in the history
…fect 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.
  • Loading branch information
Bastian authored Jan 26, 2025
1 parent 85f6f0a commit c2920f1
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ export function useEventListener<K extends keyof HTMLElementEventMap, T extends
const ref = useRef<T>(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]);
Expand Down
12 changes: 7 additions & 5 deletions packages/@mantine/hooks/src/use-focus-within/use-focus-within.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,15 @@ export function useFocusWithin<T extends HTMLElement = any>({
};

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);
};
}

Expand Down
12 changes: 7 additions & 5 deletions packages/@mantine/hooks/src/use-hover/use-hover.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@ export function useHover<T extends HTMLElement = any>() {
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);
};
}

Expand Down
18 changes: 10 additions & 8 deletions packages/@mantine/hooks/src/use-move/use-move.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,15 @@ export function useMove<T extends HTMLElement = any>(
}, []);

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);
Expand Down Expand Up @@ -112,13 +114,13 @@ export function useMove<T extends HTMLElement = any>(
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]);
Expand Down
18 changes: 10 additions & 8 deletions packages/@mantine/hooks/src/use-radial-move/use-radial-move.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,12 @@ export function useRadialMove<T extends HTMLElement = any>(
}, []);

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);
Expand Down Expand Up @@ -122,13 +124,13 @@ export function useRadialMove<T extends HTMLElement = any>(
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]);
Expand Down

0 comments on commit c2920f1

Please sign in to comment.