[$500] Workspace - 3 dot menu in WS listing/WS chat stops working after viewing WS avatar

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.46-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): [email protected] Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

1. Navigate to staging.new.expensify.com
2. Click Settings icon at bottom >> Workspaces
3. Tap on 3 dot menu for any WS
4. Select admin or announce room
5. Tap on header
6. Tap on WS avatar to view larger preview
7. Tap on 3 dot menu in WS chat (Note : 3 dot not working)
8. Go back to WS listing page
9. Tap on 3 dot menu for any WS

Expected Result:

3 dot menu in WS listing page (to access delete, admin & announce rooms) & in WS chat (to select unpin/pin option) should work after viewing WS avatar

Actual Result:

3 dot menu in WS listing/WS chat stops working after viewing WS avatar

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [x] Android: Native
• [x] Android: mWeb Chrome
• [x] iOS: Native
• [x] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/99be7269-fea2-4b4f-8003-9057c9d76444

https://github.com/Expensify/App/assets/78819774/4aea41a0-b0aa-4786-8bca-a945a58ccf95

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d9482d637d6fa5bd
• Upwork Job ID: 1765134061012324352
• Last Price Increase: 2024-03-05
• Automatic offers:
• rayane-djouah | Contributor | 0

[melvin-bot[bot]]

Triggered auto assignment to @abekkala (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[lanitochka17]

We think that this bug might be related to #wave6 CC @greg-schroeder

[lanitochka17]

@abekkala FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[Krishna2323]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Workspace - 3 dot menu in WS listing/WS chat stops working after viewing WS avatar

What is the root cause of that problem?

isVisibleRef.current isn't properly synchronized with the component's unmounting process. This led to the cleanup function, responsible for hiding the modal during unmounting, potentially not being triggered when the modal was still visible. The hideModal is responsible for calling Modal.setModalVisibility(false);. https://github.com/Expensify/App/blob/af68510ad375bc99679908ce26dafead3a8b3dc1/src/components/Modal/BaseModal.tsx#L96-L106

What changes do you think we should make in order to solve the problem?

We should set isVisibleRef.current = isVisible; inside the useEffect which is responsible for hiding the modal during unmounting. By doing this we ensure that the isVisibleRef is in sync with isVisible. Or we can directly use isVisible and remove the isVisibleRef.

Remove the line below: https://github.com/Expensify/App/blob/af68510ad375bc99679908ce26dafead3a8b3dc1/src/components/Modal/BaseModal.tsx#L80

    useEffect(
        () => {
            isVisibleRef.current = isVisible;

            return () => {
                // Only trigger onClose and setModalVisibility if the modal is unmounting while visible.
                if (!isVisibleRef.current) {
                    return;
                }
                hideModal(true);
            };
        },
        // eslint-disable-next-line react-hooks/exhaustive-deps
        [],
    );

Both solutions works as expected

https://github.com/Expensify/App/assets/85894871/1c1a6378-9f8b-4d0a-9621-82f03124919b

Alternatively

Remove isVisibleRef and directly use isVisible.

    useEffect(
        () => () => {
            // Only trigger onClose and setModalVisibility if the modal is unmounting while visible.
            if (!isVisible) {
                return;
            }
            hideModal(true);
        },
        // eslint-disable-next-line react-hooks/exhaustive-deps
        [],
    );

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Can't open the three-dot menu after opening a workspace avatar.

What is the root cause of that problem?

The three-dot menu won't show when it's behind a modal. https://github.com/Expensify/App/blob/22cb01c968a53407f7a9d864bfba30f41366f6a0/src/components/ThreeDotsMenu/index.tsx#L86

The condition that always fails is modal?.willAlertModalBecomeVisible. When we open the WS avatar page, modal?.willAlertModalBecomeVisible will be true, https://github.com/Expensify/App/blob/22cb01c968a53407f7a9d864bfba30f41366f6a0/src/components/Modal/BaseModal.tsx#L79-L83

but when we close the WS avatar page, modal?.willAlertModalBecomeVisible is never updated to false because hideModal is never called. https://github.com/Expensify/App/blob/22cb01c968a53407f7a9d864bfba30f41366f6a0/src/components/Modal/BaseModal.tsx#L62-L64

hideModal will either be called when the avatar modal unmounts, https://github.com/Expensify/App/blob/22cb01c968a53407f7a9d864bfba30f41366f6a0/src/components/Modal/BaseModal.tsx#L96-L106

but at the time the modal is unmounted, the visible state is already updated to false,

or when onModalHide is called. https://github.com/Expensify/App/blob/22cb01c968a53407f7a9d864bfba30f41366f6a0/src/components/Modal/BaseModal.tsx#L187

The first one already works as expected, but the second one is the issue. The reason it didn't get called is that when we press the close button of the WS avatar modal, it will update the visible state to false and call onModalClose, https://github.com/Expensify/App/blob/22cb01c968a53407f7a9d864bfba30f41366f6a0/src/components/AttachmentModal.tsx#L388-L396

and in onModalClose, we close the WS avatar page. https://github.com/Expensify/App/blob/22cb01c968a53407f7a9d864bfba30f41366f6a0/src/pages/ReportAvatar.tsx#L33-L35

So, the onModalHide didn't get a chance to be called because we closed the page too early.

What changes do you think we should make in order to solve the problem?

We need to make sure the hideModal is called if the Modal is unmounted and hideModal is never called yet. To achieve that, we can create a new ref called shouldCallHideModalOnUnmount.

const shouldCallHideModalOnUnmount = useRef(false);
...
// unmount code
if (!shouldCallHideModalOnUnmount.current) {
    return;
}
hideModal(true);

We set it to true when the modal becomes visible. https://github.com/Expensify/App/blob/ddf9e7ae25fcc9b0a2516ed341947ad38130b36f/src/components/Modal/BaseModal.tsx#L83-L87

And we set it to false when hideModal is called. https://github.com/Expensify/App/blob/ddf9e7ae25fcc9b0a2516ed341947ad38130b36f/src/components/Modal/BaseModal.tsx#L63-L65

This will make sure the hideModal is called in a case where the Modal is previously visible and unmounted, but doesn't have the chance to call hideModal.

What alternative solutions did you explore? (Optional)

Or we can switch the order of setting the visible state and calling onModalClose. https://github.com/Expensify/App/blob/22cb01c968a53407f7a9d864bfba30f41366f6a0/src/components/AttachmentModal.tsx#L388-L394

[rayane-d]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Can't open the three-dot menu after opening a workspace/profile/report avatar.

What is the root cause of that problem?

• In order for ThreeDotsMenu's PopoverMenu to show, isVisible prop here should become true => isBehindModal should be false here => willAlertModalBecomeVisible should be false.

• The root cause of the bug is that when we open the workspace/profile/report avatar modal, willAlertModalBecomeVisible will be true in Onyx, but when we close the avatar alert modal, willAlertModalBecomeVisible is not reset to false.

Explanation:

1. when we click on the close button, closeModal function is called in onCloseButtonPress of HeaderWithBackButton
2. when we execute closeModal function we set the isModalOpen state to false here and call the onModalClose function here
3. isModalOpen state is passed to isVisible prop of Modal here therefore, when we execute closeModal function, we set modal Visibility to false
4. we execute onModalClose in closeModal, in our case we will perform navigation. therefore BaseModal will unmount and we navigate before that onModalHide will be called here
5. hideModal function here is responsible for changing willAlertModalBecomeVisible to false here
6. hideModal will either be called when the base modal unmounts here or when we hide the modal by clicking outside
7. in our case hideModal is not called when the baseModal unmount because we return early here when isVisible is false.
8. when one of the workspace/profile/report avatar screens here is completely unmounted we set the modal visibility in Onyx to false here but willAlertModalBecomeVisible still true. this is why isBehindModal is true.

the bug also occurs when we click outside the modal (onBackButtonPress is triggered and call handleBackdropPress here which seéonClose => perform navigation =>)

we introduced isVisible for modal in Onyx in https://github.com/Expensify/App/pull/1699 , and after that we added the listeners in https://github.com/Expensify/App/pull/2050 , but when we added willAlertModalBecomeVisible for modal in Onyx in https://github.com/Expensify/App/pull/5889 we didn't add Modal.willAlertModalBecomeVisible(false); call in here.

What changes do you think we should make in order to solve the problem?

To fix this problem from root and prevent future similar bugs when using navigation inside onModalClose prop instead of onModalHide, we should add Modal.willAlertModalBecomeVisible(false); call in here.

What alternative solutions did you explore? (Optional)

• here we can add :

        } else {
            Modal.willAlertModalBecomeVisible(false);
        }

to call it when we change visibility in here. this is happening before navigation => unmount.

• we can use onModalHide instead of onModalClose in the workspace/profile/report avatar modal, but this doesn't solve the issue completely and we can reintroduce the bug if we use navigation inside onModalClose in the future.

• switching the order of setting the visible state and calling onModalClose here, or setting isVisibleRef.current = isVisible; inside the useEffect which is responsible for hiding the modal during unmounting or directly using isVisible in it, may reintroduce the bug fixed here.

[melvin-bot[bot]]

@abekkala Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot[bot]]

@abekkala Whoops! This issue is 2 days overdue. Let's get this updated quick!

[abekkala]

@lanitochka17 hmm, yeah, I feel that the 3 dot menu should always work

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~01d9482d637d6fa5bd

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @allroundexperts (External)

[kmbcook]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Workspace - 3 dot menu in WS listing/WS chat stops working after viewing WS avatar

What is the root cause of that problem?

BaseModal sets willAlertModalBecomeVisible to true, but fails to set it back to false after the modal is closed. After the modal closes there is no reason for willAlertModalBecomeVisible to be true, since the modal will not become visible because it has closed. https://github.com/Expensify/App/blob/44fc9e1121cc5e214ba1fa24c5d3aad97216666b/src/components/Modal/BaseModal.tsx#L79-L94

What changes do you think we should make in order to solve the problem?

Set willAlertModalBecomeVisible to false when BaseModal closes. Do this by calling Modal.willAlertModalBecomeVisible(false) in the clean-up function which is called when the modal closes, this function. https://github.com/Expensify/App/blob/44fc9e1121cc5e214ba1fa24c5d3aad97216666b/src/components/Modal/BaseModal.tsx#L88-L93

[abekkala]

@allroundexperts when you have a moment can you review the proposals that have come in so far?

[sarahRosannaBusch]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The Three Dots Menu stops working after viewing WS Avatar in a preview modal.

What is the root cause of that problem?

After viewing the avatar in the modal then clicking the ThreeDotsMenu, its hidePopoverMenu() is called immediately after showPopoverMenu() so the popover is never visible. This is happening because the modal used to view the avatar is not tracking it's state properly, and the Three Dot Menu is being automatically closed when it thinks the modal is open.

What changes do you think we should make in order to solve the problem?

The Three Dots Menu should not be dependent on the state of the modal.

In src/components/ThreeDotsMenu/index.tsx the isBehindModal variable and useEffect() function should be removed entirely as they are not needed. Without them, the onPress event is the only thing determining when to show/hide the popover menu, so it is always correct. (I've tested on all platforms to confirm this is true.)

What alternative solutions did you explore? (Optional)

The modal state bug, as others have already proposed, should also be fixed - but maybe as a separate issue?

[allroundexperts]

In src/components/ThreeDotsMenu/index.tsx the isBehindModal variable and useEffect() function should be removed entirely as they are not needed. Without them, the onPress event is the only thing determining when to show/hide the popover menu, so it is always correct. (I've tested on all platforms to confirm this is true.)

We occasionally open the modals by keyboard shortcuts as well. This would fail in that case.

[sarahRosannaBusch]

We occasionally open the modals by keyboard shortcuts as well. This would fail in that case.

My solution still works for this case, because the popover menu is closed on blur (not by a close button). So anytime the user does anything it is already closing properly. I've tried this using the shortcuts to open the RHP and LHP, but I'm not actually seeing a shortcut to open a modal. Am I missing something?

[allroundexperts]

My solution still works for this case, because the popover menu is closed on blur (not by a close button). So anytime the user does anything it is already closing properly. I've tried this using the shortcuts to open the RHP and LHP, but I'm not actually seeing a shortcut to open a modal. Am I missing something?

No, I just tested it and it seems to work. However, can you verify if applying your fix won't bring back https://github.com/Expensify/App/issues/33541?

[allroundexperts]

Thanks for your proposal @kmbcook. I see that your solution is similar to what @rayane-djouah proposed earlier. You're suggesting to apply the same fix at a different place. Is there any reason for it? How is it any better from applying it at the screen level?

[allroundexperts]

I'm leaning towards @rayane-djouah's proposal since it fixes the problem at its root. The other proposals seem to be more of a patch work on the original problem. That being said, I'm open to a discussion if anyone disagrees.

🎀 👀 🎀 C+ reviewed

[melvin-bot[bot]]

Triggered auto assignment to @Gonals, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[bernhardoj]

@allroundexperts the issue we have here is that the hideModal of BaseModal isn't called at all, not just the Modal.willAlertModalBecomeVisible. If it fixes the root cause, it should also solve this issue which has the same root cause.

willAlertModalBecomeVisible also doesn't seem related to RHP.

[sarahRosannaBusch]

My solution still works for this case, because the popover menu is closed on blur (not by a close button). So anytime the user does anything it is already closing properly. I've tried this using the shortcuts to open the RHP and LHP, but I'm not actually seeing a shortcut to open a modal. Am I missing something?

No, I just tested it and it seems to work. However, can you verify if applying your fix won't bring back #33541?

Oh, yes I was able to reproduce that issue on Android with my "fix". Sorry, I should have looked for that.

[Krishna2323]

@allroundexperts, any thoughts on my proposal?

[abekkala]

@Gonals do you agree with @allroundexperts chosen proposal?

[melvin-bot[bot]]

📣 @rayane-djouah 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[bernhardoj]

@allroundexperts any thoughts on my disagreement here?

[bernhardoj]

@allroundexperts bump

[kmbcook]

Thanks for your proposal @kmbcook. I see that your solution is similar to what @rayane-djouah proposed earlier. You're suggesting to apply the same fix at a different place. Is there any reason for it? How is it any better from applying it at the screen level?

I actually didn't read that earlier proposal carefully. Looks like it is better.

[allroundexperts]

@bernhardoj I went through your proposal again and I still think that @rayane-djouah's proposal is better. In my opinion, we should not rely on hideModal to be called in order to fully close the modal. If someone dismisses the modal by navigation, willAlertModalBecomeVisible should become false automatically. With your approach, we would have to call onModalHide explicitly in other pages as well just like you mentioned in your proposal. Even if we fix those pages now, I am pretty sure that this issue would return as new modals are added. A better way to do this in my opinion would be to detach this dependency on onModalHide call.

I hope that clarified the reasoning for selecting @rayane-djouah's proposal.

[bernhardoj]

@allroundexperts I'm still unconvinced about setting the willAlertModalBecomeVisible when a page is removed from navigation. Even if we do it, we will still need to replace onModalClose with onModalHide to fix this issue https://github.com/Expensify/App/issues/37634 as they have the same root cause.

I think it's just about the attachment modal lifecycle. The onModalClose is called when the user request to close the modal. If we immediately do a navigation, then onModalHide will never be called. Calling Modal.willAlertModalBecomeVisible when a page is removed doesn't solve the root cause of the issue.

Even if we fix those pages now, I am pretty sure that this issue would return as new modals are added.

We can just remove the onModalClose if that's confusing as it's only being used in report, WS, profile avatar, and transaction receipt page to navigate away.

Or if we still want to keep the onModalClose, then we need to change the ref here to isHideModalCalled.current and call hideModal when it's not yet called in case the animation is still ongoing. https://github.com/Expensify/App/blob/c915e7f026eddc710dd01a38abc3afc9334002ae/src/components/Modal/BaseModal.tsx#L96-L103