[HOLD for payment 2023-10-02] [$500] DEV - Web - Console error when user try to drag and drop attachment

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Go to any chat
2. Drag any attachment and hover on header of chat

Expected Result:

It should not show any console error

Actual Result:

Console error appears (Uncaught TypeError: Cannot read properties of null (reading 'hideCreateMenu') )

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: Dev 1.3.67-0
Reproducible in staging?: n
Reproducible in production?: n
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-11.at.12.08.00.PM.mov

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016eba922aef37e232
• Upwork Job ID: 1702001522393657344
• Last Price Increase: 2023-09-13
• Automatic offers:
• 0xmiroslav | Reviewer | 26722904
• ishpaul777 | Contributor | 26722905
• gadhiyamanan | Reporter | 26722906

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~016eba922aef37e232

[melvin-bot]

Triggered auto assignment to @johncschuster (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 - @0xmiroslav (External)

[ishpaul777]

Proposal

Problem

DEV - Web - Console error when user try to drag and drop attachment

Root Cause

We are initalizing the popover modal ref with null, and dont have a condition to return if popoverModal.current is null when adding/removing dragover listner resulting in a console error when popovermodal ref is null

Changes

modify SidebarScreen to only execute hideCreateMenu if popoverModal.current

    const onHideMenu = useCallback(() => {
        console.log('Hiding create menu');
        if (popoverModal.current) {
            popoverModal.current.hideCreateMenu();
        }
    }, []);


    /**
     * Method create event listener
     */
    const createDragoverListener = () => {
        document.addEventListener('dragover', onHideMenu);
    };

    /**
     * Method remove event listener.
     */
    const removeDragoverListener = () => {
        document.removeEventListener('dragover', onHideMenu);
    };

[0xmiros]

@ishpaul777 are you able to reproduce in latest main?

[ishpaul777]

@0xmiroslav just pulled main and checked issue is not reproducable.

[0xmiros]

@johncschuster we can close this

[ishpaul777]

@0xmiroslav I think the issue here is even though we want to remove the eventlistner when removeDragoverListener is executed the listner never removed because we are passing anonymous while adding and removing listner.

Add a console.log in addEventListner, you will realize dragover listner is not removerd when FAB menu is closed.

document.addEventListener('dragover', () => {
            console.log('dragover')
            popoverModal.current.hideCreateMenu()
        });

more info - https://stackoverflow.com/questions/10444077/javascript-removeeventlistener-not-working

[0xmiros]

@ishpaul777 I am not convinced about your analysis. How does it affect the app?

[ishpaul777]

The removeDragoverListener is meant to be executed to remove the listner isn't it ?

[0xmiros]

The removeDragoverListener is meant to be executed to remove the listner isn't it ?

right. can you please share app video which demonstrates your concern? Console log is also fine.

[ishpaul777]

Screen.Recording.2023-09-13.at.11.50.09.PM.mov

[0xmiros]

ok good catch.
I will let the engineer decide if that should be fixed here as it doesn't affect production app.
IMO, it's worth fixing for performance and optimization.
🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[srikarparsi]

Hi! Yes I do think think this should be fixed as well. Assigning you to the issue @ishpaul777

[melvin-bot]

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

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

[melvin-bot]

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

Offer link
Upwork job

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @ishpaul777 got assigned: 2023-09-18 00:52:55 Z
• when the PR got merged: 2023-09-21 02:54:23 UTC
• days elapsed: 3

On to the next one 🚀

[0xmiros]

Hope this would be eligible for efficiency bonus as C+ approved PR in 2 days 10 hrs and there were no changes requested.
cc: @srikarparsi

[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.3.73-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix remove dragover listner when popover hides #27649

If no regressions arise, payment will be issued on 2023-10-02. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @0xmiroslav requires payment offer (Reviewer)
• @ishpaul777 requires payment offer (Contributor)
• @gadhiyamanan requires payment offer (Reporter)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[ishpaul777]

Hey! @johncschuster! gentle bump for payment! Thanks

[0xmiros]

This was just code optimization, originally console error. So we can skip BZ checklist here.

[johncschuster]

Payment has been issued to everyone! Thanks!

[ishpaul777]

Hey @johncschuster,thank for sending payment, I think the PR is eligible for bonus as #27375 (comment)

cc @srikarparsi

[ishpaul777]

gentle bump @johncschuster on above ^, thanks!

[sonialiap]

Reopening to make sure that @johncschuster sees the Q about bonus

[johncschuster]

Urgency bonus has been issued to @0xmiroslav and @ishpaul777