Inproper usage of refs throughout the code base

[Bastian]

Dependencies check up

• I have verified that I use latest version of all @mantine/* packages

What version of @mantine/* packages do you have in package.json?

7.16.0

What package has an issue?

@mantine/hooks

What framework do you use?

Vite

In which browsers you can reproduce the issue?

All

Describe the bug

While working on #7404, I noticed that there are many places that depend on ref values inside useEffect hooks. Sometimes the ref value is included in the dependency array (for example in the useHover hook [1]) and sometimes it is used inside a useEffect hook but not declared as a dependency at all (for example in the useMove hook [2]).

Both are problematic and are a source of bugs. Updates to a ref value do not trigger re-renders and thus the code of the useEffect hook will never be executed in some scenarios.

Here are two sandbox examples, illustrating the bug:

• useHover: https://codesandbox.io/p/sandbox/mantine-react-template-forked-x745wn
• useMove: https://codesandbox.io/p/sandbox/mantine-react-template-forked-rz84sj

The useHover one does not work after clicking the "Show Div" button, but a triggering a re-rendering "fixes" it, because the value of ref.current is included in the dependency array:

chrome_9D4PmPSNAm.mp4

The useMove one never recovers, even after re-renders, because ref.current is not included in the dependency array:

chrome_FNBs4qzPem.mp4

These are just two examples, but there are many others, and since these hooks are also used in the Mantine components, they are most likely affected as well.

[1] ref.current in dependency array of useHover

mantine/packages/@mantine/hooks/src/use-hover/use-hover.ts

Lines 9 to 21 in 85f6f0a

	useEffect(() => {
	if (ref.current) {
	ref.current.addEventListener('mouseenter', onMouseEnter);
	ref.current.addEventListener('mouseleave', onMouseLeave);
	return () => {
	ref.current?.removeEventListener('mouseenter', onMouseEnter);
	ref.current?.removeEventListener('mouseleave', onMouseLeave);
	};
	}
	return undefined;
	}, [ref.current]);

[2] ref.current used in useEffect hook but not included in dependency array

mantine/packages/@mantine/hooks/src/use-move/use-move.ts

Lines 115 to 124 in 85f6f0a

	ref.current?.addEventListener('mousedown', onMouseDown);
	ref.current?.addEventListener('touchstart', onTouchStart, { passive: false });
	return () => {
	if (ref.current) {
	ref.current.removeEventListener('mousedown', onMouseDown);
	ref.current.removeEventListener('touchstart', onTouchStart);
	}
	};
	}, [dir, onChange]);

If possible, include a link to a codesandbox with a minimal reproduction

No response

Possible fix

Refs should not be used in these cases. Instead the html element should be stored inside a React state which triggers a re-render when the value changes.

import { useCallback, useEffect, useState } from 'react';


export function useHover<T extends HTMLElement = any>() {
  const [hovered, setHovered] = useState(false);
  const [node, setNode] = useState<T>();
  const onMouseEnter = useCallback(() => setHovered(true), []);
  const onMouseLeave = useCallback(() => setHovered(false), []);

  useEffect(() => {
    if (node) {
      node.addEventListener('mouseenter', onMouseEnter);
      node.addEventListener('mouseleave', onMouseLeave);

      return () => {
        node?.removeEventListener('mouseenter', onMouseEnter);
        node?.removeEventListener('mouseleave', onMouseLeave);
      };
    }

    return undefined;
  }, [node]);

  return { ref: setNode, hovered };
}

The usage for the user remains the same, but strictly speaking this is a breaking change.

Self-service

• I would be willing to implement a fix for this issue

[Bastian]

@rtivital My pull request #7404 did not fix this bug, this is a different and bigger problem. Can you please reopen this issue?

[DanielSegal1]

Would like to have that assigned to me
Already fixed useHover and moving to next hooks right now
thanks!