Skip to content

Commit

Permalink
Fix potential dangerous access of ref value in useEffect cleanup
Browse files Browse the repository at this point in the history
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 committed Jan 23, 2025
1 parent 85f6f0a commit e68aaeb
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 e68aaeb

Please sign in to comment.