[$250] Key Navigation - Pressing CTRL+K in attachment and closing search, leads to different chat

[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: 9.0.25-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/4891152&group_by=cases:section_id&group_order=asc&group_id=229066
Issue reported by: Applause - Internal Team

Action Performed:

1. Open the staging.new.expensify website
2. Open any chat in which an attachment was sent
3. Click the attachment to open preview modal
4. Press CTRL+K to open search page
5. Dismiss the opened search page
6. Verify if you are redirected to the correct chat

Expected Result:

When the user opens the preview modal of an attachment in any chat, presses CTRL+K and closes the search page, the website should return to the same chat where the user was in

Actual Result:

After the user opens the preview modal of an attachment in any chat, presses CTRL+K and closes the search page, is redirected to a different 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

Bug6584145_1724765287823.CTRL_K.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01212d967034d02032
• Upwork Job ID: 1829648537564304374
• Last Price Increase: 2024-09-20
• Automatic offers:
• DylanDylann | Reviewer | 104123548
• FitseTLT | Contributor | 104123550

Issue Owner

Current Issue Owner: @DylanDylann

[melvin-bot]

Triggered auto assignment to @kevinksullivan (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lanitochka17]

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

[FitseTLT]

Edited by proposal-police: This proposal was edited at 2024-08-27 20:45:56 UTC.

Proposal

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

Key Navigation - Pressing CTRL+K in attachment and closing search, leads to different chat

What is the root cause of that problem?

When press the shortcut key Modal.close will be called here

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

Line 308 in e32430c

Modal.close(Session.checkIfActionIsAllowed(() => Navigation.navigate(ROUTES.CHAT_FINDER)));

This will call the modal onClose here

App/src/libs/actions/Modal.ts

Lines 29 to 37 in e32430c

	if (closeModals.length === 0) {
	return;
	}
	if (onModalClose) {
	closeModals[closeModals.length - 1](isNavigate);
	return;
	}
	closeModals[closeModals.length - 1]();
	}

the onModalClose in this case is this

App/src/pages/home/report/ReportAttachments.tsx

Lines 41 to 44 in e32430c

	onModalClose={() => {
	Navigation.dismissModal();
	// This enables Composer refocus when the attachments modal is closed by the browser navigation
	ComposerFocusManager.setReadyToFocus();

Which dismissModal and it will pop the report attachment modal route but this onModalClose will be called again via onModalDidClose (because the modal itself calls the onModalDidClose on its onModalHide)

App/src/libs/actions/Modal.ts

Lines 52 to 57 in e32430c

	function onModalDidClose() {
	if (!onModalClose) {
	return;
	}
	if (closeModals.length) {
	closeTop();

so another pop on the route will navigate the user to another report

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

The closeTop here

App/src/libs/actions/Modal.ts

Line 57 in e32430c

closeTop();

was intended to chain close multiple modals so that the onClose of the next modal will be called on the onModalDidClose of the current modal but we haven't poped the last onClose after calling it in closeTop
So in closeTop here, we need to pop the last closeModals after it is called becasue there is no need to call the onClose of the topmost modal again after it is called (because calling it by itself means we have closed the modal) here

App/src/libs/actions/Modal.ts

Lines 28 to 37 in e32430c

	function closeTop() {
	if (closeModals.length === 0) {
	return;
	}
	if (onModalClose) {
	closeModals[closeModals.length - 1](isNavigate);
	return;
	}
	closeModals[closeModals.length - 1]();
	}

    if (onModalClose) {
        closeModals[closeModals.length - 1](isNavigate);
        closeModals.pop();
        return;
    }
    closeModals[closeModals.length - 1]();
    closeModals.pop();

What alternative solutions did you explore? (Optional)

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01212d967034d02032

[melvin-bot]

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

[melvin-bot]

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

[DylanDylann]

@FitseTLT Could you reproduce this issue anymore?

Screen.Recording.2024-09-04.at.12.42.50.mov

[FitseTLT]

@DylanDylann The reason you are not reproducing it for report attachment is because of this pr which tried to solve the same problem with wrong RCA and wrong solution. So instead of solving the issue from the root cause they applied a partial fix to report attachment so the problem still exists for other modals such as receipt attachment (Transaction Receipt), here is a video:
the page navigates back from transaction details page to the iou report page when CTRL+K, so I suggest we revert that fix and apply the right fix that solves problem from the root cause.

2024-09-04.14-43-22.mp4

[melvin-bot]

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

[melvin-bot]

@kevinksullivan, @DylanDylann Huh... This is 4 days overdue. Who can take care of this?

[DylanDylann]

On my list today

[DylanDylann]

@kevinksullivan I think this should be handled in #46388 I added a comment to the original issue. We can close this one

[FitseTLT]

@DylanDylann the issue is payedout/closed and way past its regression period, how is that suppose to be handled there while I have found the correct root cause and solution here.

CC @kevinksullivan

[melvin-bot]

@kevinksullivan @DylanDylann this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[thienlnam]

@dominictb / @allgandalf Can you comment on the proposal? Ideally, if this can resolve all cases - we should undo the fix and have this fixed universally

[CortneyOfstad]

@DylanDylann where are we at with this?

[melvin-bot]

@CortneyOfstad, @DylanDylann Eep! 4 days overdue now. Issues have feelings too...

[DylanDylann]

@dominictb @allgandalf Bump on this comment: #48113 (comment)

[allgandalf]

Sorry, I mostly rely on K2 and do not open mail often, so missed this pin.

@DylanDylann I am a little confused by that comment? can you brief about what the issue is here? thanks

[DylanDylann]

@allgandalf Here: #48113 (comment) Please help to check

[allgandalf]

I have notified the contributor to take a look at this one on slack

[DylanDylann]

@allgandalf @dominictb Thanks for your response. Hmm, I think we will handle the new bug that @FitseTLT mentioned here separately. It is hard to say this is a regression from the previous PR because this bug is in another place and isn't known at that time. The previous issue also be closed 2 weeks ago.

I will review @FitseTLT's proposal and handle it as a new issue

[melvin-bot]

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

[melvin-bot]

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

[DylanDylann]

@dominictb Would you like to post your proposal to fix this bug globally on this issue? It looks promising

[DylanDylann]

@FitseTLT The same bug still happen when reloading page

Screen.Recording.2024-09-24.at.16.40.04.mov

[FitseTLT]

@FitseTLT The same bug still happen when reloading page

Screen.Recording.2024-09-24.at.16.40.04.mov

@DylanDylann What you found here is a totally different bug that is not occuring because onMOdalHide is being called twice as the main bug here but it is caused by Navigation.goBack being called without fallbackRoute because we haven't passed shouldEnforceFallback true. We have options to solve this

1. We can use dismissModal with report id here
   

   App/src/pages/TransactionReceiptPage.tsx

   Line 68 in a99cf94

   Navigation.goBack(ROUTES.REPORT_WITH_ID.getRoute(report?.reportID ?? '-1'));

                Navigation.dismissModal(report?.reportID ?? '-1');

Because dismissModal correctly handles the case of transaction receipt page here ( and also dismissModal is the most appropriate as we are dismissing a modal)

App/src/libs/Navigation/dismissModalWithReport.ts

Line 42 in a99cf94

case SCREENS.TRANSACTION_RECEIPT:

2. We can also pass shouldEnforceFallback to goBack

                Navigation.goBack(ROUTES.REPORT_WITH_ID.getRoute(report?.reportID ?? '-1'), true);

[melvin-bot]

@CortneyOfstad @DylanDylann this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[DylanDylann]

Let's go with @FitseTLT's proposal. I the PR phase, please include the change to address this bug

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

📣 @DylanDylann 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @FitseTLT 🎉 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 📖

[DylanDylann]

@CortneyOfstad This is ready for payment. The PR is deployed to production on 1/10

[CortneyOfstad]

Payment Summary

@DylanDylann — paid $250 via Upwork
@FitseTLT — pid $250 via Upwork