[HOLD for payment 2024-06-06] [$500] Web - CTRL + K does not open Search while user avatar preview opened

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com 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: https://expensify.testrail.io/index.php?/tests/view/4355697
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Log in to New Expensify
2. Navigate to any chat
3. Click on user name in chat history
4. Click on user avatar in RHP
5. Press CTRL/CMD + K on keyboard
6. Verify Search is opened
7. Click on FAB > Start chat

Expected Result:

Avatar preview should close, and left-hand Search page should open when CTRL + K pressed

Actual Result:

Avatar preview closes, but the search pages does not open until user opens any RHP e.g. Start chat

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

Bug6398697_1709321287617.2024-03-01_20-18-43.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0165d96894b5f57808
• Upwork Job ID: 1770923450252546048
• Last Price Increase: 2024-03-28
• Automatic offers:
• tienifr | Contributor | 0

Issue Owner

Current Issue Owner: @allroundexperts / @slafortune

[melvin-bot]

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

[lanitochka17]

@slafortune 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

[lanitochka17]

We think that this bug might be related to #wave8
CC @zanyrenney

[slafortune]

I'm not able to recreate this, are you able to @lanitochka17 ?

[slafortune]

Closing - not able to recreate

[allroundexperts]

@slafortune This is still reproducible. @bernhardoj has a proposal that might fix it. Can you please re-open this?

[slafortune]

You bet - Thanks @allroundexperts

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0165d96894b5f57808

[melvin-bot]

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

[bernhardoj]

Proposal

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

Can't open the search page using the keyboard shortcut while on a workspace avatar page.

What is the root cause of that problem?

When we press the search shortcut, the navigation code is wrapped with Modal.close.

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

Lines 231 to 235 in 7e4a438

	const unsubscribeSearchShortcut = KeyboardShortcut.subscribe(
	searchShortcutConfig.shortcutKey,
	() => {
	Modal.close(Session.checkIfActionIsAllowed(() => Navigation.navigate(ROUTES.SEARCH)));
	},

Modal.close will call the callback immediately if there is no modal visible (closeModals.length === 0), otherwise, it will store the callback to the "pending" close (onModalClose), close the visible modal, and wait until the modal is hidden.

App/src/libs/actions/Modal.ts

Lines 42 to 50 in 7e4a438

	function close(onModalCloseCallback: () => void, isNavigating = true) {
	if (closeModals.length === 0) {
	onModalCloseCallback();
	return;
	}
	onModalClose = onModalCloseCallback;
	isNavigate = isNavigating;
	closeTop();
	}

In our case, Modal.close will close the user avatar modal first. When a modal is hidden, it is supposed to call onModalDidClose which will call the pending callback and execute the navigation to the search page.

App/src/libs/actions/Modal.ts

Lines 52 to 63 in 7e4a438

	function onModalDidClose() {
	if (!onModalClose) {
	return;
	}
	if (closeModals.length) {
	closeTop();
	return;
	}
	onModalClose();
	onModalClose = null;
	isNavigate = undefined;
	}

onModalDidClose itself is called in the Modal hideModal function, but hideModal is never called.

App/src/components/Modal/BaseModal.tsx

Lines 63 to 72 in 575d0ff

	const hideModal = useCallback(
	(callHideCallback = true) => {
	Modal.willAlertModalBecomeVisible(false);
	if (shouldSetModalVisibility) {
	Modal.setModalVisibility(false);
	}
	if (callHideCallback) {
	onModalHide();
	}
	Modal.onModalDidClose();

hideModal will either be called when the avatar modal unmounts,

App/src/components/Modal/BaseModal.tsx

Lines 96 to 106 in 22cb01c

	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
	[],
	);

or when onModalHide is called.

App/src/components/Modal/BaseModal.tsx

Line 187 in 22cb01c

onModalHide={hideModal}

When we call Modal.close, it will call the AttachmentModal closeModal which sets the visibility to false and calls onModalClose.

App/src/components/AttachmentModal.tsx

Lines 391 to 396 in edfb3b1

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

And for the avatar modal, we navigate back in onModalClose.

App/src/pages/settings/Profile/ProfileAvatar.tsx

Lines 42 to 44 in edfb3b1

	onModalClose={() => {
	Navigation.goBack();
	}}

So, onModalHide didn't get a chance to be called because the modal is unmounted earlier before the modal is completely hidden.

The hideModal also failed to be called here because the visible state is already false.

App/src/components/Modal/BaseModal.tsx

Lines 97 to 107 in 575d0ff

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

App/src/components/Modal/BaseModal.tsx

Lines 83 to 87 in ddf9e7a

	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.

App/src/components/Modal/BaseModal.tsx

Lines 63 to 65 in ddf9e7a

	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.

[tienifr]

Proposal

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

Avatar preview closes, but the search pages does not open until user opens any RHP e.g. Start chat

What is the root cause of that problem?

When we try to open the search modal by Cmd K.

The isVisibleRef will be set to false here

And then the attachment modal is unmounted, calling this, at this point the attachment modal was not completely closed (onModalHide here was not called), but the isVisibleRef.current here is already false as explained above, so the hideModal is not called. Then the hideModal won't call here too because the Modal was already unmounted.

So it's not able to trigger the hide modal callback properly and the search page could not be shown.

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

The logic to set the isVisibleRef here is wrong when isVisible is changed, it doesn't mean the modal is visible/not visible. The modal open/close animation takes time to run and the modal won't be in an open/closed state until the animation finishes. That's also indicated by the use of onModalWillShow, onModalShow, isVisible is true only means the modal will show, not that it's visible. This leads to the attachment modal thinking that the modal is actually hidden, while actually it was not (onModalHide didn't call), leading to this issue.

So we should:

1. Remove this faulty isVisibleRef set logic here
2. Set isVisibleRef.current to true when the modal was actually opened, in onModalShow here

isVisibleRef.current = true;

3. Set isVisibleRef.current to true when the modal was actually closed, in onModalHide here

What alternative solutions did you explore? (Optional)

We can make an upstream fix to react-native-modal to make sure onModalHide is called if the modal was unmounted while the modal is still visible. This makes sense to me because it feels strange that onModalHide is not triggered by react-native-modal when the modal was actually hidden (via unmounting)

[thesahindia]

@allroundexperts, seems like you already have some knowledge about this issue. Wanna take the C+ role here?

[slafortune]

Checking with @allroundexperts here

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[dukenv0307]

@slafortune I'd be happy to take over this issue as C+, if @allroundexperts is not available/or low on capacity

[allroundexperts]

I can take a look. Can you please assign this to me?

[puneetlath]

When you say it's under review, what do you mean @slafortune? Is it not on hold anymore?

[slafortune]

Whoops! I should have updated the title - correct! Not on hold and there is a PR being reviewed.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.77-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: modal does not open while another is open #42506

If no regressions arise, payment will be issued on 2024-06-06. 🎊

For reference, here are some details about the assignees on this issue:

• @allroundexperts requires payment through NewDot Manual Requests
• @tienifr requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@allroundexperts] The PR that introduced the bug has been identified. Link to the PR:
• [@allroundexperts] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@allroundexperts] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@allroundexperts] Determine if we should create a regression test for this bug.
• [@allroundexperts] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@slafortune] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[slafortune]

@tienifr Paid $500 via upworks

[slafortune]

@allroundexperts can you please complete the checklist?

[melvin-bot]

@slafortune, @allroundexperts, @aldo-expensify, @tienifr Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[aldo-expensify]

Friendly bump @allroundexperts

[allroundexperts]

Checklist

1. This was an issue on the upstream repo. As such, there isn't any culprit PR.
2. N/A
3. N/A
4. A regression test would be helpful here. The steps given in the OP look good enough to me.

[slafortune]

1. Log in to New Expensify
2. Navigate to any chat
3. Click on user name in chat history
4. Click on user avatar in RHP
5. Press CTRL/CMD + K on keyboard
6. Verify Search is opened
7. Click on FAB > Start chat

[slafortune]

@allroundexperts C+ is due $500 via NewDot

[JmillsExpensify]

$500 approved for @allroundexperts