[@mantine/core]Menu: Removing code that may cause dead loops

[xshuxin]

I have already raised the issue and hope it can be helpful~
The following is a description of the problem：
When Menu uses the toggle method provided internally by useDisclosure to control unfolding and collapsing, it can lead to dependency rendering exceeding the maximum update depth and falling into a dead loop. The following is the error message from the console: "Maximum update depth exceeded. This can happen when a component calls setState inside useEffect, but useEffect either does not have a dependency array, or one of the dependency changes on every render.

[xshuxin]

onChange() should not be called when the Menu is open, but should be called when Menu. targetis clicked, so I attempted to fix the data flow interaction form to avoid dead loops.

#4029

[cyantree]

Thanks for the PR.
However I think that's not sufficient.

IMHO there are three usecases that should be checked:

• Uncontrolled use
• Controlled use where Menu.Target will be used for toggling the state
• Controlled use where some other component will be used for toggling the state

I've prepared an example for these:
https://codesandbox.io/s/fast-wave-lfz8d3?file=/src/App.tsx

I think the following should happen:

Uncontrolled

When the menu is closed and the user clicks to open it

• the menu opens
• onOpen() will be called
• onChange(true) will be called

When the menu is opened and the user clicks to close it

• the menu closes
• onClose() will be called
• onChange(false) will be called

Controlled with Menu.Target

When opened={false} and the user clicks the trigger (or something else) to open it

• the menu does not automatically open
• onOpen() will be called
• onChange(true) will be called
• opened={true} will be set from the outside
• the menu opens
• neither onOpen() nor onChange() will be called again

When opened={true} and the user clicks the trigger (or something else) to close it

• the menu does not automatically close
• onClose() will be called
• onChange(false) will be called
• the menu closes
• neither onOpen() nor onChange() will be called again

Controlled with external component

When opened gets set to true

• the menu opens
• neither onOpen() nor onChange() will be called

When opened gets set to false

• the menu closes
• neither onOpen() nor onChange() will be called

---

Currently (before and after your PR) these cases do not work consistently and also onChange() sometimes gets triggered too often.

Also this needs to be tested for trigger='hover' where hovering the target opens/closes the menu.

[xshuxin]

@cyantree You're absolutely right! I have indeed overlooked many scenarios. I just updated the code and tested it again. The scenarios you mentioned are basically covered. I have relatively shallow experience and this is the first time I have mentioned PR. I feel that you are very experienced in this area. Thank you very much for your suggestion! If possible, do you have time to help review the code? Or propose a better solution?

[cyantree]

Thanks @xshuxin for taking the time and initiative!

Sure, I'm happy to help reviewing your code.

However at first maybe let's check back with @rtivital whether he agrees with the proposed concept. I'm just a contributor as you and therefore not the one making the decisions.

If he agrees have a look at the unit tests in Menu.test.tsx. They should cover all described scenarios, succeed with your changes and (at least partially) fail with the pre-PR code.

For testing the menu tests I'd change jest.config.js as the following to only run the relevant test:

testMatch: ['**/Menu.test.tsx'],

I don't know if there's an easier way to achieve this?

After finalizing the tests please revert this change before committing and run the whole test suite again.

[xshuxin]

@cyantree

Thank you very much!
Based on your proposal, I conducted a single module automation test on Menu locally and found that it passed successfully. At the same time, the entire automation test seems to have also passed.
I'm very sorry, I don't have a better way to conduct a single module self-test more easily.
Compared to modifying configuration files, it seems more convenient to run commands directly~

npx jest <path-to-file>
npx jest --watch

I am excited and hope to successfully propose my first PR!

[rtivital]

Hi @xshuxin, @cyantree proposal is correct, please let me know when your code is ready for review

[ikapta]

@xshuxin @cyantree @rtivital
I think this issue is occured by this PR #3716 ，Popover component In Controlled status, the onClose will called twice, and use-popover.ts listened the opened status，But Menu passed useDisclousure toggle() to Popover, so dead cycle happened.

Should attention in Popover, there had an intentional logic in Popover component:
if uncontrolled, the PopoverTarget will bind an onClick event to call onClose and setOpen;
if Controlled, here is no event at target, user should controlled by himself.

// This code in PopoverTarget component
...(!ctx.controlled ? { onClick: ctx.onToggle } : null),

And the website doc will has an mislead, onChange only called in closeOnClickOutside:

So I have some small update thought:
1、If follow convention, and no changes here , just revert that pr, and user controlled onChange\onClose by himself.
2、Otherwise split Menu logic from close and open, no mixed it to Popover onClose\onOpen.
3、Remove !ctx.controlled ? onClick : null logic in PopoverTarget, then onClose and onChange can be called in unControlled, there will no influence, but should do more test, For Controlled mode user can just use onChange , or custom onClick fn by himself.

[xshuxin]

Hi @xshuxin, @cyantree proposal is correct, please let me know when your code is ready for revie

@cyantree @rtivital
I'm very sorry, I just saw this message.I am ready for the code to be reviewed~

[cyantree]

Thanks @xshuxin , also for the Jest hint. :)
Can you extend the tests to cover all cases described in #4030 (comment) ?
That allows avoiding regressions.
As described earlier these tests should (at least partially) fail with the old code (because it has it flaws) but succeed with the new code.

@ikapta Thanks for the insight. Yes, maybe that's also relevant here. Extending the menu tests helps getting and keeping this right.

[xshuxin]

Thanks @cyantree @ikapta insightful insights. :)
I have extended the testing and successfully covered all situations described in #4030(comment) to avoid regression.

Referring to the reason for the bug explained by @ikapta , onClose and onOpen will be called in the controlled state of Popover, causing Menu to trigger a second call.However, Menu managed the calling logic for onClose and onOpen on its own, so I removed them...
Secondly, after removing these two methods, I only changed the clickOutside to not properly call the onClose method. However, due to its controlled component nature, it still calls the 'onChange' method. Therefore, I added onClose to the onChange method to ensure that everything still works properly.

My change is basically the same as the previous call logic, except that the clickOutside call close/open logic is put into onChange.

It seems that the extension test you mentioned is inconsistent with the old logic. Could you please help me check if my understanding is incorrect.

[cyantree]

Unfortunately I can't find any new tests. They would really help finding the missing pieces. Do you need help with that? It's ok if they are not perfect or fail but it would be a good next step in covering all usecases.

It's correct that the cases I described won't match the old logic. That's because I think programmatic state changes shouldn't trigger these on*() callbacks and the old logic does that.

[cyantree]

Here are some tests for the described scenarios with trigger="click".
And they fail with the old code and pass with your changes. Great!
Feel free to rework or adjust them.

So what's missing are tests for the mode trigger="hover".

  it('correctly calls callbacks when opening and closing the uncontrolled menu via target click', async () => {
    const spyOnOpen = jest.fn();
    const spyOnClose = jest.fn();
    const spyOnChange = jest.fn();

    render(<TestContainer onChange={spyOnChange} onOpen={spyOnOpen} onClose={spyOnClose} />);

    expectClosed();

    await userEvent.click(getControl());

    expectOpened();
    expect(spyOnOpen).toHaveBeenCalledTimes(1);
    expect(spyOnClose).not.toHaveBeenCalled();
    expect(spyOnChange).toHaveBeenCalledTimes(1);
    expect(spyOnChange).toHaveBeenLastCalledWith(true);

    spyOnOpen.mockReset();
    spyOnClose.mockReset();
    spyOnChange.mockReset();
    await userEvent.click(getControl());

    expectClosed();
    expect(spyOnOpen).not.toHaveBeenCalled();
    expect(spyOnClose).toHaveBeenCalledTimes(1);
    expect(spyOnChange).toHaveBeenCalledTimes(1);
    expect(spyOnChange).toHaveBeenLastCalledWith(false);
  });

  it('correctly calls callbacks when opening and closing the controlled menu only via prop', async () => {
    const spyOnOpen = jest.fn();
    const spyOnClose = jest.fn();
    const spyOnChange = jest.fn();

    const ControlledMenu = (props: MenuProps) => <TestContainer onChange={spyOnChange} onOpen={spyOnOpen} onClose={spyOnClose} {...props} />;

    const rendering = render(<ControlledMenu opened={false} />);

    expectClosed();
    expect(spyOnOpen).not.toHaveBeenCalled();
    expect(spyOnClose).not.toHaveBeenCalled();
    expect(spyOnChange).not.toHaveBeenCalled();

    rendering.rerender(<ControlledMenu opened />);

    // Changing state via prop shouldn't call on* methods
    expectOpened();
    expect(spyOnOpen).not.toHaveBeenCalled();
    expect(spyOnClose).not.toHaveBeenCalled();
    expect(spyOnChange).not.toHaveBeenCalled();

    rendering.rerender(<ControlledMenu opened={false} />);

    // Changing state via prop shouldn't call on* methods
    expectClosed();
    expect(spyOnOpen).not.toHaveBeenCalled();
    expect(spyOnClose).not.toHaveBeenCalled();
    expect(spyOnChange).not.toHaveBeenCalled();
  });

  it('correctly calls callbacks when opening and closing the controlled menu via target click', async () => {
    const spyOnOpen = jest.fn();
    const spyOnClose = jest.fn();
    const spyOnChange = jest.fn();

    const ControlledMenu = (props: MenuProps) => <TestContainer onChange={spyOnChange} onOpen={spyOnOpen} onClose={spyOnClose} {...props} />;

    const rendering = render(<ControlledMenu opened={false} />);
    await userEvent.click(getControl());

    expectClosed();
    expect(spyOnOpen).toHaveBeenCalledTimes(1);
    expect(spyOnClose).not.toHaveBeenCalled();
    expect(spyOnChange).toHaveBeenCalledTimes(1);
    expect(spyOnChange).toHaveBeenLastCalledWith(true);

    spyOnOpen.mockReset();
    spyOnClose.mockReset();
    spyOnChange.mockReset();
    rendering.rerender(<ControlledMenu opened />);

    expectOpened();
    await userEvent.click(getControl());

    expectOpened();
    expect(spyOnOpen).not.toHaveBeenCalled();
    expect(spyOnClose).toHaveBeenCalledTimes(1);
    expect(spyOnChange).toHaveBeenCalledTimes(1);
    expect(spyOnChange).toHaveBeenLastCalledWith(false);

    rendering.rerender(<ControlledMenu opened={false} />);

    expectClosed();
  });

[cyantree]

FYI: I'll be unable to work on this for the next few days.

[xshuxin]

I'm sorry, I've been too busy these past few days.

I think I misunderstood your point earlier. I only conducted testing locally and did not extend Jest

Thanks @cyantree . I have carefully read some of the test code you provided for the scenario described by trigger="click"! They're great!

But I encountered some minor issues and found that they couldn't match through the Prettier format. It seems that there is a problem with these few lines of code.

  const ControlledMenu = (props: MenuProps) => <TestContainer
      onChange={spyOnChange}
      onOpen={spyOnOpen}
      onClose={spyOnClose}
      {...props}
    />;

    const rendering = render(<ControlledMenu opened={false} />);

Aaccording to my humble opinion, regarding the trigger="hover" scenario, it seems that the original jest is sufficient to describe the corresponding scenario. Do you think so?

[rtivital]

Thanks!

[rtivital]

I'll have to revert it in 6.0.9 – https://codesandbox.io/s/nifty-curran-fy1nlu?file=/src/App.tsx
menus are opening up when clicking anywhere on the screen, instead of when the Target is clicked

[xshuxin]

Secondly, after removing these two methods, I only changed the clickOutside to not properly call the onClose method. However, due to its controlled component nature, it still calls the 'onChange' method. Therefore, I added onClose to the onChange method to ensure that everything still works properly.

@rtivital I'm very sorry, I missed out on this question. Please let me try to fix it.