Popover onClose runs in the loop after closing the popover

[maxiadlovskii]

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.13.4, 7.13.5

What package has an issue?

@mantine/core

What framework do you use?

create-react-app (CRA)

In which browsers you can reproduce the issue?

Chrome

Describe the bug

In case the onClose callback depends on some state value and then changes it, then, after first closing of the popover, on close callback runs in the loop.
I attached a link to codesandbox. Just open and close popover and you will see how updates count. When I open Popover again, it stops counting. a

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

https://codesandbox.io/p/sandbox/mantine-popover-bug-forked-dst5wp?file=%2Fsrc%2FApp.tsx

Possible fix

In my opinion, somewhere in the code, you run onClose when the callback is updating because of some dependencies state update

Self-service

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

[maxiadlovskii]

In my example in codesandbox, you even can press the Count button, when the popover is closed. That would run in the loop an onClose. That means that if from any place of the application, I change some of the onClose dependency, it triggers onClose call

Screen.Recording.2024-11-12.at.15.48.51.mov

[minosss]

In your example, the onClose and onOpen will be changed in every re-render.

So it will be called in a loop.

// use-popover.ts
useDidUpdate(() => {
    if (!_opened) {
      options.onClose?.();
    } else {
      options.onOpen?.();
    }
}, [_opened, options.onClose, options.onOpen]);

Cache the callbacks

<Popover
  onClose={useCallback(() => {  setCount((c) => c+1); }, [])}
  onOpen={useCallback(() => {}, [])}
/>

[maxiadlovskii]

In your example, the onClose and onOpen will be changed in every re-render.

So it will be called in a loop.

// use-popover.ts
useDidUpdate(() => {
    if (!_opened) {
      options.onClose?.();
    } else {
      options.onOpen?.();
    }
}, [_opened, options.onClose, options.onOpen]);

Cache the callbacks

<Popover
  onClose={useCallback(() => {  setCount((c) => c+1); }, [])}
  onOpen={useCallback(() => {}, [])}
/>

My callback onClose use useCallback. From your code example

// use-popover.ts
useDidUpdate(() => {
    if (!_opened) {
      options.onClose?.();
    } else {
      options.onOpen?.();
    }
}, [_opened, options.onClose, options.onOpen]);

I see that mantine run onClose even when the callback is updated. That is not right. Because onClose might have different dependencies, which might be changed independently to popover from other places of application. Update of onClose callback shouldn't run it.
You should run onClose only when _opened updating to false.

[minosss]

If you are talking about trigger logic, it might be so, it depends on the author.

My choice is to manage the state within the current component as much as possible. If the callback function needs to use some state, I would use ref instead of modifying the callback function.

[rtivital]

Fixed in 7.14.1