Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HOLD for payment 2024-04-03 [$500] Workspace - 3 dot menu in WS listing/WS chat stops working after viewing WS avatar #37609

Closed
5 of 6 tasks
lanitochka17 opened this issue Mar 1, 2024 · 43 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Mar 1, 2024

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?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6398331_1709303779903.RPReplay_Final1709297090.mp4
Bug6398331_1709303779912.WEB.mp4

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
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 1, 2024
Copy link

melvin-bot bot commented Mar 1, 2024

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

@lanitochka17
Copy link
Author

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

@lanitochka17
Copy link
Author

@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
Copy link
Contributor

Krishna2323 commented Mar 2, 2024

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);.

useEffect(
() => () => {
// 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
[],
);

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:

isVisibleRef.current = isVisible;

    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

modal_onhide_issue.mp4

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
Copy link
Contributor

bernhardoj commented Mar 2, 2024

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.

const isBehindModal = modal?.willAlertModalBecomeVisible && !modal?.isPopover && !shouldOverlay;

The condition that always fails is modal?.willAlertModalBecomeVisible. When we open the WS avatar page, modal?.willAlertModalBecomeVisible will be true,

useEffect(() => {
isVisibleRef.current = isVisible;
let removeOnCloseListener: () => void;
if (isVisible) {
Modal.willAlertModalBecomeVisible(true, type === CONST.MODAL.MODAL_TYPE.POPOVER || type === CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED);

but when we close the WS avatar page, modal?.willAlertModalBecomeVisible is never updated to false because hideModal is never called.

const hideModal = useCallback(
(callHideCallback = true) => {
Modal.willAlertModalBecomeVisible(false);

hideModal will either be called when the avatar modal unmounts,

useEffect(
() => () => {
// 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
[],
);

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

or when onModalHide is called.

onModalHide={hideModal}

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,

const closeModal = useCallback(() => {
setIsModalOpen(false);
if (typeof onModalClose === 'function') {
onModalClose();
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [onModalClose]);

and in onModalClose, we close the WS avatar page.

onModalClose={() => {
Navigation.goBack(ROUTES.REPORT_WITH_ID_DETAILS.getRoute(report?.reportID ?? ''));
}}

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.

if (isVisible) {
Modal.willAlertModalBecomeVisible(true, type === CONST.MODAL.MODAL_TYPE.POPOVER || type === CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED);
// To handle closing any modal already visible when this modal is mounted, i.e. PopoverReportActionContextMenu
removeOnCloseListener = Modal.setCloseModal(onClose);
}

And we set it to false when hideModal is called.

const hideModal = useCallback(
(callHideCallback = true) => {
Modal.willAlertModalBecomeVisible(false);

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.

const closeModal = useCallback(() => {
setIsModalOpen(false);
if (typeof onModalClose === 'function') {
onModalClose();
}

@rayane-d
Copy link
Contributor

rayane-d commented Mar 3, 2024

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 #1699 , and after that we added the listeners in #2050 , but when we added willAlertModalBecomeVisible for modal in Onyx in #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)

        } 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 melvin-bot bot added the Overdue label Mar 3, 2024
Copy link

melvin-bot bot commented Mar 4, 2024

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

Copy link

melvin-bot bot commented Mar 5, 2024

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

@abekkala
Copy link
Contributor

abekkala commented Mar 5, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Mar 5, 2024
@abekkala abekkala added External Added to denote the issue can be worked on by a contributor Overdue labels Mar 5, 2024
@melvin-bot melvin-bot bot changed the title Workspace - 3 dot menu in WS listing/WS chat stops working after viewing WS avatar [$500] Workspace - 3 dot menu in WS listing/WS chat stops working after viewing WS avatar Mar 5, 2024
Copy link

melvin-bot bot commented Mar 5, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 5, 2024
Copy link

melvin-bot bot commented Mar 5, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Mar 5, 2024
@kmbcook
Copy link
Contributor

kmbcook commented Mar 6, 2024

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.

useEffect(() => {
isVisibleRef.current = isVisible;
let removeOnCloseListener: () => void;
if (isVisible) {
Modal.willAlertModalBecomeVisible(true, type === CONST.MODAL.MODAL_TYPE.POPOVER || type === CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED);
// To handle closing any modal already visible when this modal is mounted, i.e. PopoverReportActionContextMenu
removeOnCloseListener = Modal.setCloseModal(onClose);
}
return () => {
if (!removeOnCloseListener) {
return;
}
removeOnCloseListener();
};
}, [isVisible, wasVisible, onClose, type]);

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.

return () => {
if (!removeOnCloseListener) {
return;
}
removeOnCloseListener();
};

@abekkala
Copy link
Contributor

abekkala commented Mar 6, 2024

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

@sarahRosannaBusch
Copy link

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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?

@allroundexperts
Copy link
Contributor

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
Copy link
Contributor

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

@bernhardoj
Copy link
Contributor

@allroundexperts bump

@kmbcook
Copy link
Contributor

kmbcook commented Mar 12, 2024

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
Copy link
Contributor

@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
Copy link
Contributor

@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 #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.

useEffect(
() => () => {
// Only trigger onClose and setModalVisibility if the modal is unmounting while visible.
if (!isVisibleRef.current) {
return;
}
hideModal(true);
},

@Krishna2323
Copy link
Contributor

I agree with @bernhardoj, we aren't solving the root cause here. hideModal(true) should be called when page unmounts and it will handle everything.

useEffect(
() => () => {
// Only trigger onClose and setModalVisibility if the modal is unmounting while visible.
if (!isVisibleRef.current) {
return;
}
hideModal(true);
},

const hideModal = useCallback(
(callHideCallback = true) => {
Modal.willAlertModalBecomeVisible(false);
if (shouldSetModalVisibility) {
Modal.setModalVisibility(false);
}
if (callHideCallback) {
onModalHide();
}
Modal.onModalDidClose();
if (!fullscreen) {
ComposerFocusManager.setReadyToFocus();
}
},
[shouldSetModalVisibility, onModalHide, fullscreen],
);

@allroundexperts
Copy link
Contributor

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.

@bernhardoj Are you sure that removing this will not cause any regressions?

@bernhardoj
Copy link
Contributor

@allroundexperts after some testing, I found that replacing onModalClose with onModalHide caused a regression.

  1. Open profile avatar
  2. Press the browser/device back button
  3. There will be 2 navigation back happen, one from the browser/native, one from onModalHide

Now I know why we use onModalClose because it will never be called when we navigate using the browser/device back button.

So, we can't use the solution of replacing onModalClose with onModalHide, but we still need a solution that makes sure hideModal is called instead of only Modal.willAlertModalBecomeVisible.

@allroundexperts
Copy link
Contributor

So, we can't use the solution of replacing onModalClose with onModalHide, but we still need a solution that makes sure hideModal is called instead of only Modal.willAlertModalBecomeVisible.

For now, I still feel like the chosen proposal is the best one.

@Krishna2323
Copy link
Contributor

@allroundexperts, what do you think about my proposal? It works as expected.

@bernhardoj
Copy link
Contributor

@allroundexperts I have updated my proposal's main solution to make sure hideModal is called. If it still doesn't convince you, can you ask to reopen #37634 and I can move my proposal there?

@abekkala abekkala removed their assignment Mar 22, 2024
@abekkala abekkala added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Mar 22, 2024
Copy link

melvin-bot bot commented Mar 22, 2024

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Mar 22, 2024
@abekkala
Copy link
Contributor

@isabelastisser I'm ooo until April 08 - I'll be taking back any issues still Open once I return


CURRENT STATUS:

@allroundexperts has chosen a proposal by @rayane-djouah. A PR has already been merged: #38098 which was just deployed to staging. Once it has deployed to prod this issue should update with a payment date.

@abekkala abekkala self-assigned this Mar 22, 2024
@isabelastisser
Copy link
Contributor

No updates.

@rayane-d
Copy link
Contributor

@isabelastisser PR was deployed to production #38098 (comment) on Mar, 27.
This should be [HOLD for payment 2024-04-03]

@isabelastisser isabelastisser changed the title [$500] Workspace - 3 dot menu in WS listing/WS chat stops working after viewing WS avatar HOLD for payment 2024-04-03 [$500] Workspace - 3 dot menu in WS listing/WS chat stops working after viewing WS avatar Apr 2, 2024
@isabelastisser
Copy link
Contributor

Payment summary:

Fix: @rayane-djouah $500 - paid in Upwork
PR: Review @allroundexperts $500 - pending payment in newdot.

@isabelastisser
Copy link
Contributor

Payment summary:

PENDING:

PR: Review @allroundexperts $500 - pending payment in newdot.

@JmillsExpensify
Copy link

$500 approved for @allroundexperts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests