[PAID] [LOW] [$500] Web - Chat - Console error shows up when opening emoji picker and then clicking on send button #32743

lanitochka17 · 2023-12-08T19:55:10Z

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

Go to staging.new.expensify.com
Go to any chat
Click on the emoji picker
Click Send button

Expected Result:

No console error

Actual Result:

Console error shows up - Uncaught TypeError: Cannot read properties of null (reading 'onCloseCallback')

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

Bug6306170_1702063410475.20231209_014314__1_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~017a72f5508e45e17d
Upwork Job ID: 1733213660108169216
Last Price Increase: 2024-01-05
Automatic offers:

hoangzinh | Reviewer | 28128112
mkhutornyi | Contributor | 28128113

melvin-bot · 2023-12-08T19:55:14Z

Job added to Upwork: https://www.upwork.com/jobs/~017a72f5508e45e17d

melvin-bot · 2023-12-08T19:55:14Z

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

melvin-bot · 2023-12-08T19:55:16Z

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot · 2023-12-08T19:55:19Z

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

happy-devs · 2023-12-08T20:00:02Z

Proposal

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

An unescaped method is showing as an console error.

What is the root cause of that problem?

The onCloseCallback method mentioned in the error is in the activePopoverRef.current ref, which may not have it present at the time of being used. This is a quirk of the language where even referencing an endpoint is not safe if it is null, and must be accompanied by an extra clause within the if statement.

App/src/components/PopoverProvider/index.tsx

Lines 15 to 26 in d4fb27a

    
           const closePopover = React.useCallback((anchorRef?: React.RefObject<HTMLElement>) => { 
        
               if (!activePopoverRef.current || (anchorRef && anchorRef !== activePopoverRef.current.anchorRef)) { 
        
                   return; 
        
               } 
        
               activePopoverRef.current.close(); 
        
               if (activePopoverRef.current.onCloseCallback) { 
        
                   activePopoverRef.current.onCloseCallback(); 
        
               } 
        
               activePopoverRef.current = null; 
        
               setIsOpen(false); 
        
           }, []);

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

use optional chaining within the if statement to remove or escape the method on current to prevent it from being invoked unless present:

if (activePopoverRef.current?.onCloseCallback) {
            activePopoverRef.current.onCloseCallback();
        }

Some reading material on the issue I've read in the past: https://stackoverflow.com/questions/62603498/optional-chaining-cause-unexpected-result-when-used-in-if-statement

it-education-md · 2023-12-08T20:03:08Z

I can't reproduce

ZhenjaHorbach · 2023-12-08T20:06:50Z

Proposal

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

Web - Chat - Console error shows up when opening emoji picker and then clicking on send button

What is the root cause of that problem?

The main problem is that we are accessing an empty ref (activePopoverRef.current === null )

And when we try to check this value activePopoverRef.current.onCloseCallback we get error

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

We can update this code like

    const closePopover = React.useCallback((anchorRef?: React.RefObject<HTMLElement>) => {
        if (!activePopoverRef.current || (anchorRef && anchorRef !== activePopoverRef.current.anchorRef)) {
            return;
        }

        const onCloseCallback = lodashGet(activePopoverRef, 'current.onCloseCallback', () => {});
        onCloseCallback();
        activePopoverRef.current.close();
        activePopoverRef.current = null;
        setIsOpen(false);
    }, []);

App/src/components/PopoverProvider/index.tsx

Lines 15 to 26 in d4fb27a

    
           const closePopover = React.useCallback((anchorRef?: React.RefObject<HTMLElement>) => { 
        
               if (!activePopoverRef.current || (anchorRef && anchorRef !== activePopoverRef.current.anchorRef)) { 
        
                   return; 
        
               } 
        
               activePopoverRef.current.close(); 
        
               if (activePopoverRef.current.onCloseCallback) { 
        
                   activePopoverRef.current.onCloseCallback(); 
        
               } 
        
               activePopoverRef.current = null; 
        
               setIsOpen(false); 
        
           }, []);

What alternative solutions did you explore? (Optional)

NA

strepanier03 · 2023-12-08T20:17:25Z

I can reproduce but I'm not sure of the priority of this. I am raising this internally and will follow up.

gijoe0295 · 2023-12-08T20:54:20Z

Proposal

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

Console error shows up when opening emoji picker and then clicking on send button.

What is the root cause of that problem?

Short answer

The ref here is null at the time it is invoked.

Long answer

But if that's the case, why didn't the above line activePopoverRef.current.close() fail?

When the popover opens, we bind the callback onClose aka hideEmojiPicker in EmojiPicker component to the close function of the popover ref:

App/src/components/PopoverWithoutOverlay/index.js

Lines 76 to 80 in a9cdc0b

    
           onOpen({ 
        
               ref: props.withoutOverlayRef, 
        
               close: props.onClose, 
        
               anchorRef: props.anchorRef, 
        
           });

Later when we press Esc, closePopover would be invoked here in which we should care about these lines:

App/src/components/PopoverProvider/index.tsx

Lines 20 to 24 in 142212f

    
           activePopoverRef.current.close(); 
        
           if (activePopoverRef.current.onCloseCallback) { 
        
               activePopoverRef.current.onCloseCallback(); 
        
           } 
        
           activePopoverRef.current = null;

First it runs the ref's close (line 20) and indeed invoke hideEmojiPicker (mentioned above). Later the isVisible prop is changed to false thus invoke the ref's close again here. Note that the previous close call has not finished yet at this moment.
The second invoke of close happens straightforward, without any problem and consequently set the ref to null (line 24)
Now the stack returns to the first close call (mentioned in 1) and continues from line 21. Since the ref was null from 2, it would throw error here.

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

Use optional chaining here:

if (activePopoverRef.current?.onCloseCallback) {
    activePopoverRef.current.onCloseCallback();
}

We should not move the onCloseCallback above close because onCloseCallback is supposed to run after close and that's the purpose of callback functions.

What alternative solutions did you explore? (Optional)

NA

mkhutornyi · 2023-12-08T20:56:35Z

Proposal

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

Web - Chat - Console error shows up when opening emoji picker and then clicking on send button

What is the root cause of that problem?

App/src/components/PopoverProvider/index.tsx

Lines 15 to 26 in d4fb27a

    
           const closePopover = React.useCallback((anchorRef?: React.RefObject<HTMLElement>) => { 
        
               if (!activePopoverRef.current || (anchorRef && anchorRef !== activePopoverRef.current.anchorRef)) { 
        
                   return; 
        
               } 
        
               activePopoverRef.current.close(); 
        
               if (activePopoverRef.current.onCloseCallback) { 
        
                   activePopoverRef.current.onCloseCallback(); 
        
               } 
        
               activePopoverRef.current = null; 
        
               setIsOpen(false); 
        
           }, []);

At first glance, there's no reason for activePopoverRef.current.onCloseCallback being called as already early returns on this line when activePopoverRef.current is null:

App/src/components/PopoverProvider/index.tsx

Line 16 in d4fb27a

    
           if (!activePopoverRef.current || (anchorRef && anchorRef !== activePopoverRef.current.anchorRef)) {

But the issue is in this line:

App/src/components/PopoverProvider/index.tsx

Line 20 in d4fb27a

activePopoverRef.current.close();

This close() function resets reference to null

This is the main root cause

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

change call order like this:

 if (activePopoverRef.current.onCloseCallback) { 
     activePopoverRef.current.onCloseCallback(); 
 } 
 activePopoverRef.current.close();

What alternative solutions did you explore? (Optional)

remove all onOpenCallback, onCloseCallback usages throughout the app as they're unused

hoangzinh · 2023-12-09T16:16:46Z

I can reproduce but I'm not sure of the priority of this. I am raising this internally and will follow up.

@strepanier03 let's us know if we want to fix this issue.

strepanier03 · 2023-12-11T18:23:34Z

I will update as soon as I have an internal consensus, thanks!

Pujan92 · 2023-12-11T18:25:15Z

Proposal

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

Console error for closing the emoji picker

What is the root cause of that problem?

Earlier in this PR we have set the onCloseCallback, onOpenCallback to fix the popover stuck issue. But now it has been removed in the other PR to maintain a list of modals closeModal methods.
closePopover method calls the close method of the activePopoverRef which resets the ref and isVisible state change of Popover also calls the close method again and in that execution activePopoverRef is null which throws the console error.

App/src/components/PopoverProvider/index.tsx

Lines 15 to 23 in 298d9d2

    
           const closePopover = React.useCallback((anchorRef?: React.RefObject<HTMLElement>) => { 
        
               if (!activePopoverRef.current || (anchorRef && anchorRef !== activePopoverRef.current.anchorRef)) { 
        
                   return; 
        
               } 
        
               activePopoverRef.current.close(); 
        
               if (activePopoverRef.current.onCloseCallback) { 
        
                   activePopoverRef.current.onCloseCallback(); 
        
               }

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

We can get rid of the onOpenCallback, onCloseCallback fields as it is not needed now as pointed out above due to this PR changes.

melvin-bot · 2023-12-15T11:08:52Z

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

melvin-bot · 2023-12-15T16:00:19Z

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

melvin-bot · 2023-12-19T11:11:53Z

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

strepanier03 · 2023-12-19T21:10:21Z

All right, we would like to move forward and fix this error.

strepanier03 · 2023-12-19T22:34:53Z

Discussing here still.

hoangzinh · 2023-12-20T09:32:58Z

@strepanier03 does it mean I can start to review existing proposals or still wait for final call?

strepanier03 · 2023-12-21T22:24:04Z

@hoangzinh - Please pause until next week. I'll update this on the 28th when I'm back in the office.

melvin-bot · 2023-12-22T11:02:55Z

@hoangzinh @strepanier03 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 · 2024-01-29T07:35:13Z

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

melvin-bot · 2024-01-29T07:35:37Z

📣 @hoangzinh 🎉 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 · 2024-01-29T07:35:42Z

📣 @mkhutornyi 🎉 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 · 2024-02-07T09:42:16Z

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

melvin-bot · 2024-02-07T09:42:20Z

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

remove unused popover ref callbacks #35402

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

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

@hoangzinh requires payment automatic offer (Reviewer)
@mkhutornyi requires payment automatic offer (Contributor)

melvin-bot · 2024-02-07T09:42:21Z

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:

[@hoangzinh] The PR that introduced the bug has been identified. Link to the PR:
[@hoangzinh] 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:
[@hoangzinh] 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:
[@hoangzinh] Determine if we should create a regression test for this bug.
[@hoangzinh] 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.
[@strepanier03] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

strepanier03 · 2024-02-13T19:05:22Z

Looks like we're good until tomorrow. @hoangzinh feel free to finish the checklist and I'll handle the reg test.

strepanier03 · 2024-02-14T19:00:07Z

@mkhutornyi - I've paid you via Upwork and closed the contract, thanks for your help resolving this GH!

@hoangzinh - I'll check later today for the checklist and move forward when it's done.

hoangzinh · 2024-02-15T10:58:17Z

BugZero Checklist:

The PR that introduced the bug has been identified. Link to the PR: fix: 29378 Distance - Search is open in background on distance image preview #30556
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: https://github.com/Expensify/App/pull/30556/files#r1490822312
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: N/A
Determine if we should create a regression test for this bug: ⛔ it's just a clean-up of unused code PR, no new logic is added.

strepanier03 · 2024-02-15T21:46:50Z

Got it, thanks @hoangzinh I'll finish this up now.

strepanier03 · 2024-02-15T21:50:31Z

Okay, paid and contract ended. Thanks again for helping to get this finished @hoangzinh and @mkhutornyi 🙌

lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 8, 2023

melvin-bot bot changed the title ~~Web - Chat - Console error shows up when opening emoji picker and then clicking on send button~~ [$500] Web - Chat - Console error shows up when opening emoji picker and then clicking on send button Dec 8, 2023

melvin-bot bot assigned strepanier03 Dec 8, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 8, 2023

melvin-bot bot assigned hoangzinh Dec 8, 2023

melvin-bot bot added the Overdue label Dec 14, 2023

melvin-bot bot removed the Overdue label Dec 19, 2023

MonilBhavsar assigned mkhutornyi and unassigned mkhutornyi Jan 29, 2024

melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 29, 2024

mkhutornyi mentioned this issue Jan 30, 2024

remove unused popover ref callbacks #35402

Merged

60 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jan 30, 2024

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Feb 7, 2024

melvin-bot bot changed the title ~~[LOW] [$500] Web - Chat - Console error shows up when opening emoji picker and then clicking on send button~~ [HOLD for payment 2024-02-14] [LOW] [$500] Web - Chat - Console error shows up when opening emoji picker and then clicking on send button Feb 7, 2024

melvin-bot bot removed the Reviewing Has a PR in review label Feb 7, 2024

melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Feb 13, 2024

melvin-bot bot removed the Overdue label Feb 13, 2024

hoangzinh mentioned this issue Feb 15, 2024

fix: 29378 Distance - Search is open in background on distance image preview #30556

Merged

58 tasks

strepanier03 closed this as completed Feb 15, 2024

strepanier03 changed the title ~~[HOLD for payment 2024-02-14] [LOW] [$500] Web - Chat - Console error shows up when opening emoji picker and then clicking on send button~~ [PAID] [LOW] [$500] Web - Chat - Console error shows up when opening emoji picker and then clicking on send button Feb 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PAID] [LOW] [$500] Web - Chat - Console error shows up when opening emoji picker and then clicking on send button #32743

[PAID] [LOW] [$500] Web - Chat - Console error shows up when opening emoji picker and then clicking on send button #32743

lanitochka17 commented Dec 8, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Dec 8, 2023

melvin-bot bot commented Dec 8, 2023

melvin-bot bot commented Dec 8, 2023

melvin-bot bot commented Dec 8, 2023

happy-devs commented Dec 8, 2023 •

edited

Loading

it-education-md commented Dec 8, 2023

ZhenjaHorbach commented Dec 8, 2023 •

edited

Loading

strepanier03 commented Dec 8, 2023 •

edited

Loading

gijoe0295 commented Dec 8, 2023 •

edited

Loading

mkhutornyi commented Dec 8, 2023

hoangzinh commented Dec 9, 2023

strepanier03 commented Dec 11, 2023

Pujan92 commented Dec 11, 2023

melvin-bot bot commented Dec 15, 2023

melvin-bot bot commented Dec 15, 2023

melvin-bot bot commented Dec 19, 2023

strepanier03 commented Dec 19, 2023

strepanier03 commented Dec 19, 2023

hoangzinh commented Dec 20, 2023

strepanier03 commented Dec 21, 2023

melvin-bot bot commented Dec 22, 2023

melvin-bot bot commented Jan 29, 2024

melvin-bot bot commented Jan 29, 2024

melvin-bot bot commented Jan 29, 2024

melvin-bot bot commented Feb 7, 2024

melvin-bot bot commented Feb 7, 2024

melvin-bot bot commented Feb 7, 2024

strepanier03 commented Feb 13, 2024

strepanier03 commented Feb 14, 2024

hoangzinh commented Feb 15, 2024

strepanier03 commented Feb 15, 2024

strepanier03 commented Feb 15, 2024

[PAID] [LOW] [$500] Web - Chat - Console error shows up when opening emoji picker and then clicking on send button #32743

[PAID] [LOW] [$500] Web - Chat - Console error shows up when opening emoji picker and then clicking on send button #32743

Comments

lanitochka17 commented Dec 8, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Dec 8, 2023

melvin-bot bot commented Dec 8, 2023

melvin-bot bot commented Dec 8, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Dec 8, 2023

happy-devs commented Dec 8, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

it-education-md commented Dec 8, 2023

ZhenjaHorbach commented Dec 8, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

strepanier03 commented Dec 8, 2023 • edited Loading

gijoe0295 commented Dec 8, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

Short answer

Long answer

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

What alternative solutions did you explore? (Optional)

mkhutornyi commented Dec 8, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

hoangzinh commented Dec 9, 2023

strepanier03 commented Dec 11, 2023

Pujan92 commented Dec 11, 2023

Proposal

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

What is the root cause of that problem?

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

melvin-bot bot commented Dec 15, 2023

melvin-bot bot commented Dec 15, 2023

melvin-bot bot commented Dec 19, 2023

strepanier03 commented Dec 19, 2023

strepanier03 commented Dec 19, 2023

hoangzinh commented Dec 20, 2023

strepanier03 commented Dec 21, 2023

melvin-bot bot commented Dec 22, 2023

melvin-bot bot commented Jan 29, 2024

melvin-bot bot commented Jan 29, 2024

melvin-bot bot commented Jan 29, 2024

melvin-bot bot commented Feb 7, 2024

melvin-bot bot commented Feb 7, 2024

melvin-bot bot commented Feb 7, 2024

strepanier03 commented Feb 13, 2024

strepanier03 commented Feb 14, 2024

hoangzinh commented Feb 15, 2024

BugZero Checklist:

strepanier03 commented Feb 15, 2024

strepanier03 commented Feb 15, 2024

lanitochka17 commented Dec 8, 2023 •

edited by melvin-bot bot

Loading

happy-devs commented Dec 8, 2023 •

edited

Loading

ZhenjaHorbach commented Dec 8, 2023 •

edited

Loading

strepanier03 commented Dec 8, 2023 •

edited

Loading

gijoe0295 commented Dec 8, 2023 •

edited

Loading