[HOLD for payment 2023-08-28] [$1000] Chat - Email is pasted twice in the composer when copied from user profile

[lanitochka17]

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 staging.new.expensify.com
2. Open profile page for any user in chat
3. Click Copy to clipboard icon next to the email
4. Paste the email in the composer

Expected Result:

Email is pasted once in the composer

Actual Result:

Email is pasted twice in the composer

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• Windows / Chrome
• MacOS / Desktop

Version Number: 1.3.55-1

Reproducible in staging?: Yes

Reproducible in production?: No

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

Bug6168183_bandicam_2023-08-17_22-20-02-511.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f8779e48a7b14b9d
• Upwork Job ID: 1692204576618807296
• Last Price Increase: 2023-08-17
• Automatic offers:
• situchan | Reviewer | 26181155
• daordonez11 | Contributor | 26181158

[OSBotify]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @bondydaa (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[situchan]

This was first reported by me - https://expensify.slack.com/archives/C049HHMV9SM/p1692127516386599

[daordonez11]

This error was introduced in this PR #23359

Proposal

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

Double paste in composer after copying email prom profile

What is the root cause of that problem?

First of all, the error only happens when the profile is opened (copy email) and then closed because it triggers the handler of navigation focus.

Then there are 2 different paste handlers at the same time, one associated to the whole document and one associated to the input

Hence handlePaste is called 2 times

App/src/components/Composer/index.js

Lines 355 to 366 in 2e1c4e7

	useEffect(() => {
	// we need to handle listeners on navigation focus/blur as Composer is not unmounting
	// when navigating away to different report
	const unsubscribeFocus = navigation.addListener('focus', () => document.addEventListener('paste', handlePaste));
	const unsubscribeBlur = navigation.addListener('blur', () => document.removeEventListener('paste', handlePaste));
	if (_.isFunction(forwardedRef)) {
	forwardedRef(textInput.current);
	}
	if (textInput.current) {
	textInput.current.addEventListener('paste', handlePaste);

PS: There is another error reading the code. If textInput.current does not exist, the first time the Composer is shown the pasted text will not go through handlePaste

Update after @situchan comment we should handle it as the class component had it before where paste was handled as a document event with document.addEventListener instead of using textInput.current

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

Both handlers should be handled the same way, either both are associated with the document to ensure the handler is removed when navigation blur is called or there is a validation to handle each case.

So instead of using textInput.current for paste we can use document for handling the paste event always

Also in the return method of the effect we should do: textInput.current.removeEventListener('paste', handlePaste); or include the same validation to remove the respective handler.

In pseudocode something like if we want to handle both:

const unsubscribeFocus = navigation.addListener('focus', () => {
            if (textInput.current) {
                textInput.current.addEventListener('paste', handlePaste);
            }
            else {
                document.addEventListener('paste', handlePaste)
            }
        });
        const unsubscribeBlur = navigation.addListener('blur', () => {
            if (textInput.current) {
                textInput.current.removeEventListener('paste', handlePaste);
            }
            else {
                document.removeEventListener('paste', handlePaste)
            }
        });

What alternative solutions did you explore? (Optional)

Validate if handlePAste is already called in some way and avoid multicall or something like throttle in the same method but too complex for a simple task

cc @mountiny

[StevenKKC]

Proposal

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

Email is pasted twice in the composer when copied from user profile.

What is the root cause of that problem?

In Composer component, we add two paste event listener.

App/src/components/Composer/index.js

Line 358 in e89ea9d

const unsubscribeFocus = navigation.addListener('focus', () => document.addEventListener('paste', handlePaste));

App/src/components/Composer/index.js

Line 366 in e89ea9d

textInput.current.addEventListener('paste', handlePaste);

So the text is pasted twice in the composer.

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

We should remove one paste event listener.
Since adding event listener to textInput.current is redundant, we can remove this line.

    if (textInput.current) {
-       textInput.current.addEventListener('paste', handlePaste);

What alternative solutions did you explore? (Optional)

None.

[mountiny]

@daordonez11 Are you bale to create a PR for this with a fix now? Its a deploy blocker so urgency is required.

@bondydaa would you like to let the contributor fix this?

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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]

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

[melvin-bot]

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

[daordonez11]

I can definitely create a PR and work on it right now @mountiny @bondydaa

[mountiny]

@bondydaa up to you

[getusha]

Proposal

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

Text is pasted twice when text is copied from user profile.

What is the root cause of that problem?

When navigation is focused it will add a 'paste' event listener, which makes the component have two event listeners at once triggering paste twice.

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

Prior to adding the event listener to 'textInput', initiate a call to document.removeEventListener('paste', handlePaste) in order to remove the previous event listener from the document.

What alternative solutions did you explore? (Optional)

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

• Fix for double paste in composer after function component mig #25419
• Revert hotfix: Move again to avoid listener still active #25424

If no regressions arise, payment will be issued on 2023-08-28. 🎊

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:

• @daordonez11 requires payment offer (Contributor)
• @situchan requires payment offer (Reviewer)

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:

• The PR that introduced the bug has been identified. Link to the PR: Migrate Composer. web #23359
• 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: Migrate Composer. web #23359 (comment)
• 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.
• 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.(https://github.com/Expensify/App/blob/main/contributingGuides/REGRESSION_TEST_BEST_PRACTICES.md) steps to ensure the same bug will not reach production again.
• [@twisterdotcom / @MitchExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/313469

[b4s36t4]

Do I get any reporting bonus #25409 (comment) for this one?

[MitchExpensify]

The issue in question here was reported by our internal testers. Does what @b4s36t4 commented on here look like a distinct issue to what was reporting here originally to you @situchan ?

[situchan]

The issue in question here was reported by our internal testers. Does what @b4s36t4 commented on here look like a distinct issue to what was reporting here originally to you @situchan ?

I think he's eligible though it's not a formal bug report in slack.

[MitchExpensify]

Payment summary:

Reporting: Applause N/A
C: @daordonez11 $1500 (Upwork)
C+: @situchan $1500 (Upwork)

[MitchExpensify]

Paid and contracts ended

[MitchExpensify]

Bump on BZ steps @situchan

[melvin-bot]

@bondydaa, @twisterdotcom, @daordonez11, @MitchExpensify, @situchan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[situchan]

• The PR that introduced the bug has been identified. Link to the PR: Migrate Composer. web #23359
• 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: Migrate Composer. web #23359 (comment)
• 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.
• 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 Proposal:

1. Open any chat
2. Open any RHP page (i.e. Settings page) and then close
3. Copy any text
4. Paste into the composer
5. Verify that text is not pasted twice

@MitchExpensify I am also eligible for reporting bonus as I reported before this GH is created - #25409 (comment)

[MitchExpensify]

I am also eligible for reporting bonus as I reported before this GH is created - #25409 (comment)

Paid reporting bonus!

[MitchExpensify]

Added new test request - Thanks everyone!