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

lanitochka17 commented 3 months ago

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:

Go to any chat and click the plus sign
Select "Attachment" and send the Pdf attachment
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
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/93819efe-d977-448e-955a-7182c256610c

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[bot] commented 3 months ago

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 commented 3 months ago

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

lanitochka17 commented 3 months ago

@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 commented 3 months ago

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[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

strepanier03 commented 3 months ago

Testing now.

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

@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[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

allgandalf commented 3 months ago

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

allgandalf commented 3 months ago

@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[bot] commented 3 months ago

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

allgandalf commented 3 months ago

@strepanier03 can you assign me for this issue please

melvin-bot[bot] commented 3 months ago

Current assignee @allgandalf is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 3 months ago

📣 @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[bot] commented 3 months ago

📣 @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 commented 3 months ago

@dominictb , when can the PR be ready?

dominictb commented 3 months ago

PR is ready.

allgandalf commented 3 months ago

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

allgandalf commented 2 months ago

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

allgandalf commented 2 months ago

PR is on staging ♻️

melvin-bot[bot] commented 2 months ago

⚠️ 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 commented 2 months ago

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

strepanier03 commented 2 months ago

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

strepanier03 commented 2 months ago

Payment Summary

[x] $250 - @allgandalf via Upwork
[x] $250 - @dominictb via Upwork

Both contracts paid and closed.

strepanier03 commented 2 months ago

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

allgandalf commented 2 months ago

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

DylanDylann commented 2 months ago

@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 commented 2 months ago

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.

Expensify / App