[PAID] [$250] Chat - When opening a PDF and clicking 'Ctrl + K,' the search opens but the page scrolls to the next

[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: 9.0.13-3
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
**Email or phone of affected tester (no customers):**betlihemasfaw14@gmail.com
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to any chat and click the plus sign
2. Select "Attachment" and send the Pdf attachment
3. Open the attachment and click 'Ctrl + K
   4, click on 'esc' to close

Expected Result:

When we open the PDF and click 'Ctrl + K,' the search should open, and the page should not scroll to the next page

Actual Result:

When we open the PDF and click 'Ctrl + K,' the search opens, but the page scrolls to the next page

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

Bug6550529_1721740884041.Screen_Recording_2024-07-23_at_6.19.05_AM.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b20e5826b61d01de
• Upwork Job ID: 1820952988321413899
• Last Price Increase: 2024-08-13
• Automatic offers:
• allgandalf | Reviewer | 103557309
• dominictb | Contributor | 103557313

Issue Owner

Current Issue Owner: @

[melvin-bot]

Triggered auto assignment to @strepanier03 (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]

We think that this bug might be related to #vip-vsp

[lanitochka17]

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

[dominictb]

Proposal

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

Reproduction step:

• Upload a pdf in a chat (chat 1) and then open another chat (chat 2)
• Open chat 1 again using LHN
• Open the pdf attachment
• Click Cmd + K

Instead of closing the pdf attachment and show the chat 1 in the background while the chat search left modal open, it shows the chat 2 in the background instead

What is the root cause of that problem?

In here, when calling Modal.close, we are actually calling the Modal's onClose callback more than once

• The first time is in here, we called closeTop from Modal.close() function directly
• The second time is in here, the react-native-modal library will trigger the onModalHide, and in that function we also call Modal.onModalDidClose, which in turn calls closeTop accordingly
• The third time is in when the modal is being unmounted, and like the second time, it triggers Modal.closeTop. However, at this point in time, we have already removed the onClose listener here, hence in here the callback will be triggered, i.e: opening the chat search page.

So, this function will be triggered twice, and because the navigation is not an synchronous operation, in here, we can see that during both triggers on dismissModal function, the lastRoute is still the chat attachment modal, however, we are dispatching pop action twice, hence we can see that the navigation will look like this Chat attachment modal > Chat 1 report page > Chat 2 report page > Chat search page modal

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

The idea is to make sure that the onClose callback of each modal is idempotent, i.e: If we have already triggered the callback, triggering it again shouldn't cause any other side effects. We can choose to fix it for just ReportAttachments or fix it globally.

To fix it for ReportAttachments, simply add a ref const modalDismissed = useRef(false) , and in here lock the action Navigation.dismissModal

if(!modalDismissed.current) {
      Navigation.dismissModal();
      modalDismissed.current = true;
 }

To fix it globally for all modal onClose callbacks (in case those callbacks are not idempotent in principle), we can keep track the number of trigger for each onClose callbacks in here, and prevent the callback to be called again if it has been called before

const closeModals: Array<[(isNavigating?: boolean) => void, number]> = [];

function setCloseModal(onClose: () => void) {
    if (!closeModals.includes(onClose)) {
        closeModals.push([onClose, 0]); // initially, the number of execution is 0
    }
    return () => {
        const index = closeModals.findIndex([func, counter] => func === onClose);
        if (index === -1) {
            return;
        }
        closeModals.splice(index, 1);
    };
}

function closeTop() {
    if (closeModals.length === 0) {
        return;
    }
    const [onClose, executionCounter] = closeModals[closeModals.length - 1]
    if(executionCounter > 0) { // prevent multiple triggers
        return;
    }
    closeModals[closeModals.length - 1][1] += 1

    if (onModalClose) {
        onClose(isNavigate)
        return;
    }
   onClose()
}

What alternative solutions did you explore? (Optional)

[melvin-bot]

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

[melvin-bot]

@strepanier03 Still overdue 6 days?! Let's take care of this!

[strepanier03]

Testing now.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

@strepanier03 @thesahindia 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!

[melvin-bot]

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

[melvin-bot]

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

[allgandalf]

Taking this over from @thesahindia , @strepanier03 can you please assign this to me (slack)

[allgandalf]

@dominictb 's proposal LGTM, they RCA is crystal clear and solution seems to fix this issue.

I would stick to only fixing this with ReportAttachments though, as in general shortcuts have a lot of technical aspects and the way they are coded is considering a low-level overview, so maybe an internal engineer can create another issue where we can investigate and solve this issue globally (This is my opinion though, if internal engineer differs with my thought then we can try solving it in this issue).

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[allgandalf]

@strepanier03 can you assign me for this issue please

[melvin-bot]

📣 @allgandalf 🎉 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]

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

[allgandalf]

@dominictb , when can the PR be ready?

[dominictb]

PR is ready.

[allgandalf]

Left a comment on that, can you please check once you find time, thanks ;))

[allgandalf]

@dominictb , left some review for you, please check that once you find time, thanks :)

[allgandalf]

PR is on staging ♻️

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[allgandalf]

^ is a false alarm, The regression isn't from our PR imo

[strepanier03]

Updated title since it looks like automation might have failed on this one.

[strepanier03]

Payment Summary

• $250 - @allgandalf via Upwork
• $250 - @dominictb via Upwork

Both contracts paid and closed.

[strepanier03]

Dang, thought today was the 6th lol, I'm all turned around.

[allgandalf]

haha, no worries, happens on the best of our days

[DylanDylann]

@dominictb @allgandalf We have the same issue with this one. And a contributor complains about @dominictb's solution. I see this is the 100% same issue so please help to check the similar bug that is mentioned here

cc @thienlnam

[dominictb]

In my last proposal, I did propose a general solution as alternative (which is sorta equivalent of the other proposal in other issue), but Gandalf decide to use the primary solution in his review.