[HOLD for payment 2024-09-07][$250] Chat - Messages stuck highlighted when opening context menus consecutively #45524

lanitochka17 · 2024-07-16T20:35:09Z

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.7-4
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
Issue reported by: Applause - Internal Team

Action Performed:

Log in to New Expensify
Navigate to any chat with existing messages
Right click on a message
Right click on another message
Click anywhere to close context menu

Expected Result:

The second clicked message should not be highlighted

Actual Result:

The second clicked message remains highlighted

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6544401_1721161832467.2024-07-16_16-45-49.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~010fad7b2cd94a8560
Upwork Job ID: 1818656780802952530
Last Price Increase: 2024-08-07

Issue Owner

Current Issue Owner: @anmurali

melvin-bot · 2024-07-16T20:35:17Z

Triggered auto assignment to @anmurali (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 · 2024-07-16T20:35:48Z

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

lanitochka17 · 2024-07-16T20:36:05Z

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

nyomanjyotisa · 2024-07-17T06:51:42Z

Proposal

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

Messages stuck highlighted when opening context menus consecutively

What is the root cause of that problem?

App/src/pages/home/report/ContextMenu/ReportActionContextMenu.ts

Lines 128 to 133 in 9c509a0

    
           if (contextMenuRef.current.instanceID) { 
        
               hideContextMenu(); 
        
               contextMenuRef.current.runAndResetOnPopoverHide(); 
        
           } 
        
           contextMenuRef.current.showContextMenu(

if there is an already open context menu, we don't wait until the hideContextMenu done before executing showContextMenu

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

make showContextMenu and hideContextMenu functions async and add await to the hideContextMenu() call here

RESULT

New-Expensify.19.mp4

What alternative solutions did you explore? (Optional)

melvin-bot · 2024-07-22T18:38:57Z

@anmurali Eep! 4 days overdue now. Issues have feelings too...

bernhardoj · 2024-07-24T08:46:36Z

Proposal

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

Message background is stuck when open context menu while other context menu is showing.

What is the root cause of that problem?

When we open a context menu, we pass a show and hide callback to the popover.

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

Lines 357 to 358 in 1084706

    
           () => setIsContextMenuActive(true), 
        
           toggleContextMenuFromActiveReportAction,

Show callback will set the context menu active to true, while the hide callback will toggle the context menu and set it based on the context menu visibility.

When we open the first context menu and then open the second one, the hide callback of the first and second context menus is called. The hide callback is cleared when called.

Why the hide callback is called twice?

When we open a context menu, if an existing context menu is found, it will hide it first.

App/src/pages/home/report/ContextMenu/ReportActionContextMenu.ts

Lines 126 to 133 in 1084706

    
           // If there is an already open context menu, close it first before opening 
        
           // a new one. 
        
           if (contextMenuRef.current.instanceID) { 
        
               hideContextMenu(); 
        
               contextMenuRef.current.runAndResetOnPopoverHide(); 
        
           } 
        
           contextMenuRef.current.showContextMenu(

hideContextMenu() will set the context menu visibility to false while runAndResetOnPopoverHide() triggers the hide callback (1st hide callback).

Because we set the visibility to false, the context menu popover onModalHide is triggered, calling another runAndResetOnPopoverHide().

Open the first context menu, hide callback is set to the first context menu hide callback (callback = 1st)
Open the 2nd context menu.

The first context menu hide callback is called (callback = empty) (hideContextMenu and runAndResetOnPopoverHide are called)
The second context menu hide callback is set (callback = 2nd)
The second context menu hide callback is called (callback = empty) (runAndResetOnPopoverHide is called by onModalHide)

Close the 2nd context menu, no hide callback is called, the highlight stays

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

If we look at the popover without overlay initial implementation, they face this problem before and that's why they added the code to close the current context menu before opening a new one in this commit.

App/src/pages/home/report/ContextMenu/ReportActionContextMenu.ts

Lines 126 to 133 in 1084706

    
           // If there is an already open context menu, close it first before opening 
        
           // a new one. 
        
           if (contextMenuRef.current.instanceID) { 
        
               hideContextMenu(); 
        
               contextMenuRef.current.runAndResetOnPopoverHide(); 
        
           } 
        
           contextMenuRef.current.showContextMenu(

But if you notice in the commit, we only call runAndResetOnPopoverHide to trigger the hide callback, which means we never set the context menu visibility to false (which also means the context menu never closes, just repositioned to a new position of the 2nd context menu).

But in this resolve conflict commit, they added hideContextMenu. I believe it's because to allow opening the 2nd context menu on the same message without any problem.

Why does the double hide callback happen now? I'm pretty sure it's because the component has gone through several changes. For example, if you move the setting visibility to true here

App/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.tsx

Line 216 in 1084706

setIsPopoverVisible(true);

to outside the promise, then the double hide callback won't happen since setIsPopoverVisible(false) and setIsPopoverVisible(true) are called almost right after the other, so the visibility state stays true. But in the current situation, the component has enough time to re-render the component with visibility of false, which triggers the extra runAndResetOnPopoverHide.

There solution is to wait until the current context menu is completely hidden before opening a new one. hideContextMenu second param supports a hide callback which we can use to show the second context menu after the 1st one is hidden.

if (contextMenuRef.current.instanceID) {
    hideContextMenu(undefined, // show context menu);
    // no need runAndResetOnPopoverHide
    return;
}
// show context menu immediately

Also, there is another bug where the popover instance ID is never cleared, so contextMenuRef.current.instanceID is always true. To fix that, we need to clear it in runAndResetOnPopoverHide.

App/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.tsx

Lines 241 to 245 in 5005f59

    
           const runAndResetOnPopoverHide = () => { 
        
               reportIDRef.current = '-1'; 
        
               reportActionIDRef.current = '-1'; 
        
               originalReportIDRef.current = '-1';

setInstanceID('');

melvin-bot · 2024-07-24T18:21:24Z

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

melvin-bot · 2024-07-26T18:21:11Z

@anmurali 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

melvin-bot · 2024-07-30T18:00:24Z

@anmurali this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot · 2024-07-30T18:15:28Z

@anmurali 12 days overdue. Walking. Toward. The. Light...

melvin-bot · 2024-07-31T14:35:42Z

Job added to Upwork: https://www.upwork.com/jobs/~010fad7b2cd94a8560

melvin-bot · 2024-07-31T14:35:47Z

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

hungvu193 · 2024-08-01T06:18:03Z

Will be on my review list this tomorrow

hungvu193 · 2024-08-02T02:32:26Z

@nyomanjyotisa I don't think we allow using async/await at the moment. Please checkout the contributing guide here

hungvu193 · 2024-08-02T02:44:05Z

@bernhardoj Can you provide a little bit more about the code snippet that you mentioned here? TIA

There solution is to wait until the current context menu is completely hidden before opening a new one. hideContextMenu second param supports a hide callback which we can use to show the second context menu after the 1st one is hidden.
if (contextMenuRef.current.instanceID) {
    hideContextMenu(undefined, // show context menu);
    // no need runAndResetOnPopoverHide
    return;
}
// show context menu immediately

bernhardoj · 2024-08-02T02:54:36Z

"// show context menu" is the showContextMenu code.

App/src/pages/home/report/ContextMenu/ReportActionContextMenu.ts

Lines 133 to 152 in 5005f59

    
           contextMenuRef.current.showContextMenu( 
        
               type, 
        
               event, 
        
               selection, 
        
               contextMenuAnchor, 
        
               reportID, 
        
               reportActionID, 
        
               originalReportID, 
        
               draftMessage, 
        
               onShow, 
        
               onHide, 
        
               isArchivedRoom, 
        
               isChronosReport, 
        
               isPinnedChat, 
        
               isUnreadChat, 
        
               disabledActions, 
        
               shouldCloseOnTarget, 
        
               setIsEmojiPickerActive, 
        
               isOverflowMenu, 
        
           );

So, it will look like this

if (contextMenuRef.current.instanceID) {
    hideContextMenu(undefined, () => contextMenuRef.current.showContextMenu(...));
    return;
}
contextMenuRef.current.showContextMenu(...);

hungvu193 · 2024-08-02T02:59:56Z

I see. I don't think it works, I still can reproduce this issue.

Screen.Recording.2024-08-02.at.09.59.14.mov

tsa321 · 2024-08-02T03:07:45Z

Edited by proposal-police: This proposal was edited at 2024-08-03 02:42:43 UTC.

Proposal

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

Chat messages stuck highlighted when opening context menus consecutively

What is the root cause of that problem?

App/src/components/PopoverWithoutOverlay/index.tsx

Lines 50 to 62 in 5005f59

    
           if (isVisible) { 
        
               onModalShow(); 
        
               onOpen?.({ 
        
                   ref: withoutOverlayRef, 
        
                   close: onClose, 
        
                   anchorRef, 
        
               }); 
        
               removeOnClose = Modal.setCloseModal(onClose); 
        
           } else { 
        
               onModalHide(); 
        
               close(anchorRef); 
        
               Modal.onModalDidClose(); 
        
           }

There is race codition when calling onModalHide for previous popoverWithouOverlay. So when on second right click, the popoverWithoutOverlay will call onModalHide using onModalHide fucntion of newer popoverWithoutOverlay instead of previous popoverWithoutOverly.

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

Actually we can store the instanceID in PopoverProvider. We can pass the instanceID to PopoverProvider in onOpen here:

App/src/components/PopoverWithoutOverlay/index.tsx

Lines 52 to 55 in 459759d

    
           onOpen?.({ 
        
               ref: withoutOverlayRef, 
        
               close: onClose, 
        
               anchorRef,

add parameter instanceID. Then in onOpen of the PopoverProvider we add:

instanceIDRef.current = popoverParams.instanceID;

then add getContextInstanceID function in PopoverProvider:

const getContextInstaceID = () => {
     return instanceIDRef.current;
};

then in isVisible false of PopoverWithoutOverly we call onModalHide(getContextInstaceID());
Also need to adjust the OnPopoverHide ref to be an object with instanceID as key and onHide as value. then adjust the code accordingly.

The complete code is in testbranch.
Or maybe we can store onHide funciton there (need to test).

github-actions · 2024-08-02T03:08:00Z

true

tsa321 · 2024-08-02T03:17:18Z

@hungvu193 Here is the test branch / code changes.

Result:

macos-web-d.mp4

bernhardoj · 2024-08-02T04:41:37Z

@hungvu193 I found that the instanceID is never cleared when hiding the popover, so contextMenuRef.current.instanceID is always true even though there is no active popover. So, when we try to open the popover again, it tries to hide it, but nothing to hide, so the hide callback is never called.

App/src/pages/home/report/ContextMenu/ReportActionContextMenu.ts

Lines 128 to 129 in 5005f59

    
           if (contextMenuRef.current.instanceID) { 
        
               hideContextMenu();

I have updated my proposal to fix that too.

melvin-bot · 2024-08-29T05:56:45Z

⚠️ 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.

tsa321 · 2024-08-29T08:40:25Z

ignore this, my bad. Should have set the USE_REACT_STRICT_MODE to false before testing.

hungvu193 · 2024-09-08T14:12:39Z

@anmurali The second attempt was deployed to prod 8 days ago. This issue is now ready for payment

anmurali · 2024-09-19T16:51:24Z

I think this issue behooves a regression test?

hungvu193 · 2024-09-20T07:04:07Z

Regression Test:

Open any chat that has multiple messages (or send multiple messages on the chat)
Right-click on one message.
Right-click on another message While the first context menu is open.
Repeat it multiple times
Verify the highlight of the message disappears once the context menu of the message is hidden
Close the context menu
Right-click on a message
Verify the context menu is shown and the message is highlighted

Do we 👍 or 👎 ?

hungvu193 · 2024-10-01T03:52:44Z

@anmurali Can you please handle the payment summary here? Ty 😄

roryabraham · 2024-10-04T23:48:24Z

This was deployed to prod on 2024-08-30 so is definitely ready for payment. Marking it as such

melvin-bot · 2024-10-14T18:07:29Z

@anmurali, @hungvu193, @roryabraham, @bernhardoj Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot · 2024-10-15T18:01:38Z

@anmurali, @hungvu193, @roryabraham, @bernhardoj Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

anmurali · 2024-10-16T02:08:28Z

@hungvu193 is paid
Sent @bernhardoj an offer here

bernhardoj · 2024-10-16T07:45:52Z

@anmurali I've requested in ND.

JmillsExpensify · 2024-10-21T09:12:29Z

Can I get a payment summary with the amount for @bernhardoj?

melvin-bot · 2024-10-21T18:07:34Z

@anmurali, @hungvu193, @roryabraham, @bernhardoj Huh... This is 4 days overdue. Who can take care of this?

anmurali · 2024-10-22T20:03:52Z

Payment summary

@hungvu193 paid $250 on Upwork
@bernhardoj approved $250 via New Dot

JmillsExpensify · 2024-10-24T17:02:59Z

$250 approved for @bernhardoj

lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 16, 2024

melvin-bot bot assigned anmurali Jul 16, 2024

melvin-bot bot added the Overdue label Jul 19, 2024

muttmuure added the External Added to denote the issue can be worked on by a contributor label Jul 31, 2024

melvin-bot bot changed the title ~~Chat - Messages stuck highlighted when opening context menus consecutively~~ [$250] Chat - Messages stuck highlighted when opening context menus consecutively Jul 31, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 31, 2024

melvin-bot bot assigned hungvu193 Jul 31, 2024

melvin-bot bot removed the Overdue label Jul 31, 2024

roryabraham added Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review labels Oct 4, 2024

roryabraham changed the title ~~[$250] Chat - Messages stuck highlighted when opening context menus consecutively~~ [PAY][$250] Chat - Messages stuck highlighted when opening context menus consecutively Oct 4, 2024

melvin-bot bot added the Overdue label Oct 9, 2024

luacmartins changed the title ~~[PAY][$250] Chat - Messages stuck highlighted when opening context menus consecutively~~ [HOLD for payment 2024-09-07][$250] Chat - Messages stuck highlighted when opening context menus consecutively Oct 10, 2024

luacmartins added Daily KSv2 and removed Weekly KSv2 labels Oct 10, 2024

melvin-bot bot added Daily KSv2 Overdue and removed Overdue Daily KSv2 labels Oct 10, 2024

melvin-bot bot removed the Overdue label Oct 16, 2024

melvin-bot bot added the Overdue label Oct 18, 2024

melvin-bot bot removed the Overdue label Oct 22, 2024

anmurali closed this as completed Oct 22, 2024

[HOLD for payment 2024-09-07][$250] Chat - Messages stuck highlighted when opening context menus consecutively #45524

[HOLD for payment 2024-09-07][$250] Chat - Messages stuck highlighted when opening context menus consecutively #45524

Comments

lanitochka17 commented Jul 16, 2024 • edited by roryabraham Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Jul 16, 2024

lanitochka17 commented Jul 16, 2024

lanitochka17 commented Jul 16, 2024

nyomanjyotisa commented Jul 17, 2024

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)

melvin-bot bot commented Jul 22, 2024

bernhardoj commented Jul 24, 2024 • 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?

melvin-bot bot commented Jul 24, 2024

melvin-bot bot commented Jul 26, 2024

melvin-bot bot commented Jul 30, 2024

melvin-bot bot commented Jul 30, 2024

melvin-bot bot commented Jul 31, 2024

melvin-bot bot commented Jul 31, 2024

hungvu193 commented Aug 1, 2024

hungvu193 commented Aug 2, 2024

hungvu193 commented Aug 2, 2024

bernhardoj commented Aug 2, 2024

hungvu193 commented Aug 2, 2024

tsa321 commented Aug 2, 2024 • 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?

github-actions bot commented Aug 2, 2024

tsa321 commented Aug 2, 2024 • edited Loading

bernhardoj commented Aug 2, 2024

melvin-bot bot commented Aug 29, 2024

tsa321 commented Aug 29, 2024

hungvu193 commented Sep 8, 2024

anmurali commented Sep 19, 2024

hungvu193 commented Sep 20, 2024

hungvu193 commented Oct 1, 2024

roryabraham commented Oct 4, 2024

melvin-bot bot commented Oct 14, 2024

melvin-bot bot commented Oct 15, 2024

anmurali commented Oct 16, 2024

bernhardoj commented Oct 16, 2024

JmillsExpensify commented Oct 21, 2024

melvin-bot bot commented Oct 21, 2024

anmurali commented Oct 22, 2024 • edited by JmillsExpensify Loading

JmillsExpensify commented Oct 24, 2024

lanitochka17 commented Jul 16, 2024 •

edited by roryabraham

Loading

bernhardoj commented Jul 24, 2024 •

edited

Loading

tsa321 commented Aug 2, 2024 •

edited

Loading

tsa321 commented Aug 2, 2024 •

edited

Loading

anmurali commented Oct 22, 2024 •

edited by JmillsExpensify

Loading