[HOLD for payment 2023-09-20] [$1000] Showing the report action menu for two messages at the same time

[kavimuru]

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. Navigate to chat
2. Send any message(e.g. test 1), send another message(e.g. test 2)
3. Hover over the first sent message (in this example, test 1)
4. Now press on the composer and then press the up key so that the last message will show edit composer(in this case, test 2), and then hover over test 1 message with your cursor
5. Now edit the message and press enter and observe that both messages are showing the action menu

Expected Result:

The action menu should be displayed on the last focused message, not on the two messages at the same time

Actual Result:

The action menu is showing for two messages at the same time

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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: 1.2.94-0
Reproducible in staging?: Need reproduction
Reproducible in production?: Need reproduction
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

Two-actionmenu-bug.mov

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017c1c2a84205bafde
• Upwork Job ID: 1661084222111625216
• Last Price Increase: 2023-08-31
• Automatic offers:
• wildan-m | Contributor | 26513246
• jayeshmangwani | Reporter | 26513247

[MelvinBot]

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

[MelvinBot]

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

[dylanexpensify]

Reproduced, but updated OP to be a bit clearer of repro steps

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~0169a9bfc76777e15d

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[jjcoffee]

Proposal

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

The mini report action context menu is sometimes displaying multiple times when we only want to display it once.

What is the root cause of that problem?

When we initiate the edit, if we move our mouse inside the ReportActionItem, the onMouseEnter event is fired, which sets isHovered to true. However, when we press enter to save the edit, isHovered is not updated via any event (e.g. onMouseLeave or onBlur), even though the mouse "jumps" to be hovering over the message above. The message the mouse jumps to does trigger onMouseEnter, which results in both being highlighted (as isHovered is true for both).

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

The most straight forward way to solve this is to trigger a blur event on the TextInput (via this.textInput.blur()), to update isHovered when we're handling pressing Enter to save the message here:

App/src/pages/home/report/ReportActionItemMessageEdit.js

Lines 232 to 244 in 2831da1

	triggerSaveOrCancel(e) {
	// Do not trigger actions for mobileWeb or native clients that have the keyboard open because for those devices, we want the return key to insert newlines rather than submit the form
	if (!e || this.props.isSmallScreenWidth || this.props.isKeyboardShown) {
	return;
	}
	if (e.key === 'Enter' && !e.shiftKey) {
	e.preventDefault();
	this.publishDraft();
	} else if (e.key === 'Escape') {
	e.preventDefault();
	this.deleteDraft();
	}
	}

The same happens if you press Esc so we'd want to handle that there too.

context-menu-issue-2023-04-05_21.08.43.mp4

What alternative solutions did you explore? (Optional)

Another way to implement the same solution above is to call setIsHovered directly, by passing it along with the hovered state here:

App/src/components/Hoverable/index.js

Lines 107 to 111 in 6f6cf65

	{ // If this.props.children is a function, call it to provide the hover state to the children.
	_.isFunction(this.props.children)
	? this.props.children(this.state.isHovered)
	: this.props.children
	}

Then we can call it in the same place(s) as the main solution above.

[Pujan92]

Seems this was solved earlier by @s77rt here and issue might be occurring again after this PR

[s77rt]

@Pujan92 The linked issue is slightly different than this one. The linked PR seems unrelated too.

[Santhosh-Sellavel]

Seems this was solved earlier by @s77rt here and issue might be occurring again after this PR

@Pujan92 Why do you think that breaks can you explain?

[Pujan92]

Sorry, my bad. Based on the title I just mapped it to an earlier issue. but yes this issue is slightly different.

[Santhosh-Sellavel]

Cool, there is no rush, In the future please check twice before you mention PR that caused a regression or include any specifics about the changes that were made which caused a regression, thanks!

[parasharrajat]

Heads Up! We are looking to remove onBlur logic from Hoverable #16052. I am yet to confirm this but chances are evolving.

[Santhosh-Sellavel]

@parasharrajat Should we keep this on hold?

[Santhosh-Sellavel]

bump @parasharrajat

[parasharrajat]

I will update you soon on this. Thanks.

[wildan-m]

Based on my calculations
I was officially assigned 6 Sep, 2023 1:48 AM GMT+7
And the last update from C and C+ is 6 Sept, 2023 7:02 AM GMT+7

It took 5 hours, and 14 minutes, If there is no regression, I believe it should be eligible for an urgency bonus

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

• Resolve stuck hover due to the mouse leave event not triggered. #26690

If no regressions arise, payment will be issued on 2023-09-20. 🎊

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:

• @wildan-m requires payment offer (Contributor)
• @parasharrajat does not require payment (Eligible for Manual Requests)
• @jayeshmangwani 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

[melvin-bot]

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:

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

[dylanexpensify]

@arosiclair mind chiming in here - was the urgency not hit due to internal freeze? It looks that way, but confirming

[parasharrajat]

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:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: New Change
• [@parasharrajat] 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: N/A
• [@parasharrajat] 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
• [@parasharrajat] Determine if we should create a regression test for this bug. Yes
• [@parasharrajat] 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.

Regression Test Steps

Remove all reactions for Web/Desktop

1. Go to any reports that have many messages.
2. Scroll up at least three times.
3. Add reactions to any message.
4. Remove all reactions.
5. Verify that only the message being hovered has a highlighted background.

task thread for Web/Desktop

1. Go to any report.
2. Click (➕)menu on composer > Actions > Assign task.
3. Create a task.
4. In the task thread's main details, check the checkbox beside the task title.
5. Uncheck it
6. Verify the hover style moves to the currently hovered element.

Do you agree 👍 or 👎 ?

[arosiclair]

@arosiclair mind chiming in here - was the urgency not hit due to internal freeze? It looks that way, but confirming

Yeah that's correct. I approved the PR about a day after they were hired but held on the merge freeze.

[wildan-m]

ping @dylanexpensify

[wildan-m]

@dylanexpensify, @arosiclair, seems there is no regression here. can we process the UpWork contract?

[dylanexpensify]

Sorry - on it!

[dylanexpensify]

Payment summary:

• Reporter: @jayeshmangwani $250
• Contributor: @wildan-m $1,500
• Contributor+: @parasharrajat $1,500

Upwork Job! Please apply!

[dylanexpensify]

@jayeshmangwani please accept the offer sent to you! 🙇‍♂️

[dylanexpensify]

@wildan-m paid!

[jayeshmangwani]

@dylanexpensify Offer accepted

[dylanexpensify]

done!

[parasharrajat]

I forgot to mention earlier, that this issue was reopened and reporting bonus was already paid out to @jayeshmangwani #16923 (comment).

we reopened this issue after #18555 so @dhanashree-sawant should be eligible for reporting bonus here. I can see that we didn't pay that reporting bonus on #18555 to her.

[parasharrajat]

It might be better to pay that reporting bonus on #18555 to keep the state clean and skip the reporting bonus for this issue. @dylanexpensify

[jayeshmangwani]

@dylanexpensify Sorry for the confusion here, @parasharrajat is right and I was already paid a few months back for this issue but I somehow forgot about that, Rajat thanks for clearing things

[dylanexpensify]

Hello! Thanks for bringing to my attention! @jayeshmangwani we'll leave you with the bonus we paid out - this is on us! @dhanashree-sawant let me get you paid out for the reporting amount in the other issue (confirmed here)!

[parasharrajat]

Payment requested as per #16923 (comment)

[JmillsExpensify]

$1,500 payment approved for @parasharrajat based on BZ summary.