[$500] Web - Navigating to Search by keyboard shortcut when the "Update Workspace currency to USD" Modal is open Does not close the modal

[kbecciv]

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

---

Action Performed:

1. Open any workspace in which currency is not default to USD
2. Click Threedot then "Delete Workspace",Confirm Modal will be visible,
3. Press CMD+K/CMD+SHIFT+K, note here before navigating the Modal closes
4. Click bank account, "Update Workspace currency to USD" Modal will be visible.
5. Press CMD+K/CMD+SHIFT+K.

Expected Result:

Modal should close before navigation.

Actual Result:

Modal Remains open and app navigates to search.

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.68.12
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):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2023-09-09.at.5.52.35.PM.mov

Recording.4419.mp4

Expensify/Expensify Issue URL:
Issue reported by: @ishpaul777
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694263008212129

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0152ef93375eec0d1f
• Upwork Job ID: 1701659303722033152
• Last Price Increase: 2023-09-12

[melvin-bot]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0152ef93375eec0d1f

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Triggered auto assignment to @strepanier03 (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[ishpaul777]

Proposal

Problem Statement

When user is Navigated to search using keyboard shortcuts, the attachement Modal is not closed and navigation gets broken.

Root Cause

The root cause is when keyboard shortcut is triggered we call Modal.close(() => callback), here modal.close is dependent on the onClose prop which we pass in Modal component. But in this case we have 2 Modals in stack, so variable we are using to store onClose prop is getting overridden by second ConfirmModal onClose prop (which is null if modal not visble)

Solution

Conditionally rendering the props of the ConfirmModal in WorkspaceInitialPage based on which modal is visible and having only one modal in the stack would solve the issue.

                     <ConfirmModal
                        title={(isDeleteModalOpen ? props.translate('workspace.common.delete') : null )|| (isCurrencyModalOpen ? props.translate('workspace.bankAccount.updateToUSD') : null)}
                        isVisible={isDeleteModalOpen || isCurrencyModalOpen}
                        onConfirm={isDeleteModalOpen ? confirmDeleteAndHideModal : confirmCurrencyChangeAndHideModal}
                        onCancel={isDeleteModalOpen ? () => setIsDeleteModalOpen(false) : () => setIsCurrencyModalOpen(false)}
                        prompt={props.translate('workspace.common.deleteConfirmation')}
                        confirmText={isDeleteModalOpen ? props.translate('workspace.common.delete') : props.translate('workspace.bankAccount.updateToUSD')}
                        cancelText={props.translate('common.cancel')}
                        danger
                     />

Alternative Solution

As this problem can be solved by conditionally rendering the ConfirmModal. There is a downside to the approach we are following as explained in root cause, we can only have one modal in Stack at a time. otherwise keyboard shortcuts fails
A better approach would be to use a Array to store the onClose, and setOnClose function will push the onClose function to the array along with a ModalID. When unmounting the modal will remove the onClose function from the array. This way we can have multiple modals in stack and onClose will be called in the order of modals in stack.

Pseudo Code:

• Modal Mounted and visible setOnClose({ onClose, modalID })
• setOnClose will push the onClose function to the array along with a ModalID
• Modal Unmounted removeOnClose(modalID)
• Modal.close will call all of the closeModal Functions in array

[tienifr]

Proposal

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

Web - Navigating to Search by keyboard shortcut when the "Update Workspace currency to USD" Modal is open Does not close the modal

What is the root cause of that problem?

We have 2 modals in WorkspaceInitialPage

App/src/pages/workspace/WorkspaceInitialPage.js

Lines 266 to 276 in 2bf7bf4

	<ConfirmModal
	title={props.translate('workspace.bankAccount.workspaceCurrency')}
	isVisible={isCurrencyModalOpen}
	onConfirm={confirmCurrencyChangeAndHideModal}
	onCancel={() => setIsCurrencyModalOpen(false)}
	prompt={props.translate('workspace.bankAccount.updateCurrencyPrompt')}
	confirmText={props.translate('workspace.bankAccount.updateToUSD')}
	cancelText={props.translate('common.cancel')}
	danger
	/>
	<ConfirmModal

When isCurrencyModalOpen is true, both 2 onCancel callback are re-created, that causes this hook triggers

App/src/components/Modal/BaseModal.js

Lines 77 to 82 in 2bf7bf4

	useEffect(() => {
	Modal.willAlertModalBecomeVisible(isVisible);
	// To handle closing any modal already visible when this modal is mounted, i.e. PopoverReportActionContextMenu
	Modal.setCloseModal(isVisible ? onClose : null);
	}, [isVisible, onClose]);

• CurrencyModal is re-rendered -> we set closeModal to setIsCurrencyModalOpen function (isVisible = true)
• DeleteModal is re-rendered -> we set closeModal to null (isVisible = false)

When users enter CMD+K/CMD+SHIFT+K, we already have the logic to close the modal

App/src/libs/Navigation/AppNavigator/AuthScreens.js

Line 185 in 2bf7bf4

Modal.close(() => {

but at that time, the closeModal is null -> we can't close the currencyModal

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

The problem here is because the modal re-renders unexpectedly. When isCurrencyModalOpen get changed, we should not re-create the onClose callback

In order to archive that, we should wrap 2 onClose callbacks into useCallback hook

    const onCloseCurrencyModal = useCallback(() => setIsCurrencyModalOpen(false),[])
    const onCloseDeleteModal = useCallback(() => setIsDeleteModalOpen(false),[])

Result

Screen.Recording.2023-09-13.at.14.46.58.mov

[situchan]

@tienifr's proposal looks good to me.
🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[marcaaron]

Hmm I am really unsure sure about this proposal. It seems like there is something wrong with the implementation of the modal component itself.

First, why is it overwriting the callback? That seems like a strange hack to me.

@tienifr Can you try to put the problem into your own words now that you've investigated it? I don't think repeating the title of the issue is sufficient. It feels like there is some kind of deeper design flaw related to modals if this bug can easily be introduced again in the future.

[marcaaron]

@situchan please find a suitable proposal and tag me back in when this is ready for review.

[tienifr]

@marcaaron I think we don't have any flaws in the modal implementation

There's only one opened modal at a time.

Here's the normal flow:

1. isCurrencyModalOpen and isDeleteModalOpen are false -> there's no opened modal -> onClose is null
2. isCurrencyModalOpen is set to true -> onClose is set to () => setIsCurrencyModalOpen(false)
3. If we want to open delete modal, we have to close the currency modal first (by clicking outside) -> isCurrencyModalOpen is set to false -> onClose is null
4. Then isDeleteModalOpen is set to true -> onClose is set to () => setIsDeleteModalOpen(false)

So we can easily realize that we can not face with race condition here

But in step 2, when isCurrencyModalOpen is set to true, onCancel callback of DeleteModal is also re-created to the new one

App/src/pages/workspace/WorkspaceInitialPage.js

Line 280 in 2bf7bf4

onCancel={() => setIsDeleteModalOpen(false)}

So that triggers this hook

App/src/components/Modal/BaseModal.js

Lines 77 to 82 in 2bf7bf4

	useEffect(() => {
	Modal.willAlertModalBecomeVisible(isVisible);
	// To handle closing any modal already visible when this modal is mounted, i.e. PopoverReportActionContextMenu
	Modal.setCloseModal(isVisible ? onClose : null);
	}, [isVisible, onClose]);

because isVisible of DeleteModal is false -> onClose is set to null

Note: We just face the problem in the component that has more than 1 modal, so I think the best way to fix it is wrap onCancel into useCallback hook

Please let me know if there're some unclear points. Thanks

[tienifr]

@marcaaron Do you agree with this #27277 (comment)?

[marcaaron]

Honestly, it's all kind of unclear.

So we can easily realize that we can not face with race condition here

I'm not realizing it. Can you walk us through it? You are describing the different states in the code. But not really describing why it works the way that it does. It particular, I find it strange that we are losing the callback and why useCallback() fixes it.

[tienifr]

@marcaaron I can't reproduce, it's fixed by other PR, so we can close this one

[situchan]

Not reproducible really. @tienifr do you know which PR fixed this?

[tienifr]

@situchan This one #27639

[strepanier03]

We good to close this one out then?

[situchan]

We good to close this one out then?

yes

[strepanier03]

Gotcha, thanks for confirming. Closing now.