[HOLD PR #42892][$250] Text pasted from Slack won't send or disappears in NewDot.

[m-natarajan]

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.61-5 Reproducible in staging?: y 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 Expensify/Expensify Issue URL: Issue reported by: @mallenexpensify Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1712695994757859

Action Performed:

Scenario 1:

1. Copy text from from Slack
2. Paste text in compose box
3. Tap return

Scenario 2:

1. Copy text from from Slack
2. Type > in compose box
3. Paste text after > (and a space after)
4. Tap return

Expected Result:

Message is sent

Actual Result:

Scenario 1: Message stays in compose box and won't send. Cursor stays at the beginning of the message Scenario 2: Only the > part of the message sends.

Workaround:

unknown

Platforms:

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

• [ ] Android: Native
• [ ] Android: mWeb Chrome
• [ ] iOS: Native
• [ ] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/38435837/e5d2ba57-5336-40ce-9aa7-6800505cc0e8

https://github.com/Expensify/App/assets/38435837/384d0321-b113-4ea0-8537-3fb1990be135

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01892e3b8e5931a268
• Upwork Job ID: 1778891800225918976
• Last Price Increase: 2024-04-12
• Automatic offers:
• jayeshmangwani | Reviewer | 0
• kpadmanabhan | Contributor | 0

[melvin-bot[bot]]

Triggered auto assignment to @slafortune (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.

[melvin-bot[bot]]

Triggered auto assignment to @roryabraham (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[github-actions[bot]]

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

[mountiny]

@roryabraham been discussing here with @thienlnam that this is specific for Slack it seems so we should not have to hold deploy for this bug

[brkdavis]

@m-natarajan can you pls paste the unformatted content of the slack message? initial suspicion is > symbol is treated with a special character when you copy to clipboard.

[melvin-bot[bot]]

📣 @brkdavis! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:

   Contributor details
   Your Expensify account email: <REPLACE EMAIL HERE>
   Upwork Profile Link: <REPLACE LINK HERE>

[Julesssss]

As discussed here in Slack, we will not mark https://github.com/Expensify/App/issues/39971 as a blocker. It's only reproducable when triple clicking before copy/pasting and is part of a new feature.

[mallenexpensify]

Also happens when trying to copy/paste from https://github.com/Expensify/App/blob/main/contributingGuides/HOW_TO_BECOME_A_CONTRIBUTOR_PLUS.md

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~01892e3b8e5931a268

[roryabraham]

making this external

[melvin-bot[bot]]

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

[brkdavis]

Proposal

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

Text pasted from many editors including TextEdit or Notepad with a triple click select and copy and then paste to the NewDot report composer is not enabling "Send" button.

What is the root cause of that problem?

When you triple click and copy a text to clipboard, it brings along hidden CR LF characters along with it. After pasting cursor goes to position zero. NOT WORKING STRING

However, if you copy a line from start to end by drag and select and copy, the CR LF characters do not come along with it. After pasting, cursor goes to end of string. WORKING STRING In this case the Send button is enabled.

Drag and drop of multi-line text works fine as long as you copy till end of line and NOT include newline. WORKING STRING

Multi-line that include trailing CR LF is not working as well. Note that in this case cursor is in position 0.

NOT WORKING STRING

NOT WORKING STRING results in following to be TRUE in ReportActionComposer.tsx.

  var isSendDisabled = isCommentEmpty || isBlockedFromConcierge || !!disabled || hasExceededMaxCommentLength;

This is because isCommentEmpty returns TRUE. ONYX update in ComposerWithSuggestions.tsx for NOT WORKING STRING is not fired through onChangeText event and hence draft report is not saved to ONYX. This is because useHtmlPaste hook handles paste action, but captures cursor position with startIndex as 0 and endIndex as 0 due to additional CR LF at the end of string.

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

There are 2 ways to handle this issue. 1. Before pasting, remove the trailing carriage return and line feed in useHtmlPaste --> index.ts and paste the text without these, so that ONYX update can be fired as expected.

var handlePastePlainText = useCallback(function (event) {
    . . . 
    plainText = plainText.replace(/^\s+|\s+$/g, '');
    if (plainText) {
      paste(plainText);
    }
  }, [paste]);

2. Fix the index issue. Make sure that start and end index are calculated correctly in useHtmlPaste --> index.ts by insertAtCaret() so that the paste is handled correctly and subsequent events are fired accordingly.

2. Remove trailing Carriage Return and Line Feed inside useHtmlPaste/index.ts inside function insertAtCaret(). This fix alone is enough to make it work.

   const insertAtCaret = (target: HTMLElement, text: string) => {
      . . . 
       text = text.replace(/^\s+|\s+$/g, '');  // Remove only trailing Carriage Return and Line Feed characters 
       const node = document.createTextNode(text);
       range.insertNode(node);
       . . . 

Below are working screenshots for multiline entries

• text before removing CR LF

• text after removing CR LF

• UI after dispatching input event and onChangeText of ComposerWithSuggestions.tsx is executed and "Send" is enabled

What alternative solutions did you explore? (Optional)

After pasting a NON WORKING STRING, move to end of string and programmatically add a white space at end of string. This will enable the "Send" button.

[jayeshmangwani]

Reviewing Proposal

[roitman-g]

Proposal

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

Every time the user pastes anything with the new line in the end the cursor is incorrectly put in the beginning of the text, so if you were to triple click something copy and paste as well as copying and pasting anything with a new line this issue appears.

What is the root cause of that problem?

The root problem is that in "insertAtCaret" function in useHTMLPaste hook the selection range start and end index is determined incorrectly in cases there is a new line in the end of the insertion.

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

I suggest using document.execCommand('insertHTML', false, text); instead of implementing it by ourselves (insertAtCaret). Method execCommand is being deprecated, however it is already being used in insertByCommand function as well as "copy" functionality and it does not seem like there is a valid alternative right now.

What alternative solutions did you explore? (Optional)

N/A

[jayeshmangwani]

@brkdavis Your first solution will not work, as in this case when we paste, and the text includes "Carriage Return" and "Line Feed" at the end of the string, At that time plainText will be undefined in the handlePastePlainText function.

[jayeshmangwani]

@roitman-g We can not replace insertAtCaret with document.execCommand altogether, as it was added intentionally to handle the other custom functionalities in the app; instead, can we fix in theinsertAtCaret function that is causing this issue ?

[roitman-g]

@jayeshmangwani do you mean that dispatching a 'paste' and 'input' event is necessary? If that is the case

we can replace only these lines in insertAtCaret with execCommand.

        const range = selection.getRangeAt(0);
        range.deleteContents();
        const node = document.createTextNode(text);
        range.insertNode(node);

        // Move caret to the end of the newly inserted text node.
        range.setStart(node, node.length);
        range.setEnd(node, node.length);
        selection.removeAllRanges();
        selection.addRange(range);

If not, could you say which kind of functionalities so I can test? I tried pasting all kind of things and it seems like besides the end of the line edge case, this piece of code works the same as execCommand('insertHTML, .....).

Also, there is issue which probably a duplicate, they have the same root cause.

[suneox]

Proposal

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

Text pasted from Slack won't send or disappears in NewDot.

What is the root cause of that problem?

When paste text will execute moveCursor then setCursorPosition but the next textNodes[i + 1] is undefined so the logic below will not execute so we have this error

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

The @expensify/react-native-live-markdown has handled a lot of logic so to avoid regression we just need to check textNodes[i + 1] exits before executing function range.setStart

    textNodes[i + 1] && range.setStart(textNodes[i + 1] as Node, 0);

POC apply this @expensify+react-native-live-markdown+0.1.47.patch to verify

https://github.com/Expensify/App/assets/11959869/79de1e76-98db-400f-b2d0-1038a401e666

What alternative solutions did you explore? (Optional)

Or we can update the condition

-  if (prevChar === '\n' && prevTextLength !== undefined && prevTextLength < textCharacters.length) {
+  if (prevChar === '\n' && !!prevTextLength && prevTextLength < textCharacters.length) {

[jayeshmangwani]

@roitman-g The Purpose of this logic was this PR https://github.com/Expensify/App/pull/39177. Now, if you want to verify that your suggested changes will not recreate the issue mentioned in the PR, then you can follow the Tests section of PR after applying your suggested changes; I am copying the Tests from the PR below.

1. Copy following text:

https://expensify.com
# Lorem ipsum
> Hello world
`foo`
@here

@someone@swmansion.com

2. Enter any chat
3. Paste copied text from step 1 into main composer
4. Verify if cursors is positioned properly, it should be at the end of the pasted text
5. Verify if there are no extra new lines. Especially verify if the empty line is inside the text from step 1, hasn't been duplicated

[suneox]

Proposal updated

Add alternative solutions

[roitman-g]

@jayeshmangwani this case works the same for me with execCommand('insertHTML', false, text). The result I get when pasting it:

[brkdavis]

Proposal

Updated

@jayeshmangwani : please check the updated proposal. Only 1 line added inside insertAtCaret() to remove trailing CR LF. Please check point 2 and working code and screenshots with the fix.

[jayeshmangwani]

@suneox Your proposal is just preventing the error from happening, and it does not fix the root cause as, in your attached video(at 1:05 to 1:17) after pasting the text cursor position is wrong

One suggestion: Please always write down your understanding of the issue in the Please re-state the problem that we are trying to solve in this issue Section of the proposal template; there is no need to copy/paste the GH title.

[jayeshmangwani]

@brkdavis's Updated proposal looks good to me

@brkdavis Just one suggestion(question): rather than removing all the whitespace from the beginning and end of a string, can we remove the carriage return and line feed characters only from the start and end like this /^[\r\n]+|[\r\n]+$/g ?

🎀 👀 🎀 C+ reviewed

[melvin-bot[bot]]

Current assignee @roryabraham is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[roitman-g]

there is also a bug worth mentioning though probably not related to the paste functionality - if you copy whatever is in the "copy the following text" manually (so not with copy icon button and it is copied as a code block) and paste it - you cannot send the message and chat box is not expanded.

@roitman-g The Purpose of this logic was this PR #39177. Now, if you want to verify that your suggested changes will not recreate the issue mentioned in the PR, then you can follow the Tests section of PR after applying your suggested changes; I am copying the Tests from the PR below.

1. Copy following text:

https://expensify.com
# Lorem ipsum
> Hello world
`foo`
@here

@someone@swmansion.com

2. Enter any chat
3. Paste copied text from step 1 into main composer
4. Verify if cursors is positioned properly, it should be at the end of the pasted text
5. Verify if there are no extra new lines. Especially verify if the empty line is inside the text from step 1, hasn't been duplicated

[slafortune]

@roryabraham I will be out until 4/22, looks like the ball is in your court at the moment. Not adding another BZ since this looks like it's in a good place.

[brkdavis]

@brkdavis's Updated proposal looks good to me

@brkdavis Just one suggestion(question): rather than removing all the whitespace from the beginning and end of a string, can we remove the carriage return and line feed characters only from the start and end like this /^[\r\n]+|[\r\n]+$/g ?

🎀 👀 🎀 C+ reviewed

Sure. I will test with removal of trailing CR LF alone and proceed if looking good. @jayeshmangwani / @roryabraham : shall I proceed with a PR? but I still do not see an offer yet on upwork?

[jayeshmangwani]

@brkdavis do not proceed with PR, we still need approval from @roryabraham , and then you will get an upwork offer then you can start with the PR

[melvin-bot[bot]]

📣 @jayeshmangwani 🎉 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[bot]]

📣 @brkdavis You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

[roryabraham]

@brkdavis your proposal looks good with the caveat that I would like to see some automated tests added to the PR to cover this functionality. @jayeshmangwani we should also make sure we add manual regression tests to cover this as well.

Thanks, looking forward to the PR!

[jayeshmangwani]

@brkdavis As you are a new contributor and its your first assigned issue according to our contributing guidelines New contributors are limited to working on one job at a time, please do not post proposal on any other issue until this issue got completed(PR hit production) and if you have already submitted proposal on any other issue and if your proposal selected then please let the c+ know that your first PR is in review.

[brkdavis]

Contributor details Your Expensify account email: brkdavis890@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01063aad97ce0e22c6

@roryabraham / @jayeshmangwani : can you please help me link to my correct upwork account?

[melvin-bot[bot]]

⚠️ Unable to store contributor details.

[jayeshmangwani]

@brkdavis You have linked your account with the below details; what's the issue?

Your Expensify account email: brkdavis890@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01cef163ba3c7ed327?mp_source=share

[brkdavis]

@jayeshmangwani : melvin-bot was not picking my upwork profile. I was getting error like above. I think my upwork profile is linked to another email of mine. I created a dummy upwork profile to confirm this and then that got picked up correctly. Unfortunately, now my account is linked to this dummy upwork profile. I would like to use this upwork profile: https://www.upwork.com/freelancers/~01cef163ba3c7ed327?mp_source=share How can this be corrected now?

[jayeshmangwani]

@brkdavis I don't know how to update the Upwork profile in melvin-bot; you can raise the issue on Slack in #expensify-open-source

[brkdavis]

@jayeshmangwani : I will do that. Or is re-assigning this issue to another account of mine faster? https://github.com/kpadmanabhan

[kpadmanabhan]

@jayeshmangwani : I will do that. Or is re-assigning this issue to another account of mine faster? https://github.com/kpadmanabhan

Can you please re-assign this issue to this account of mine?

[kpadmanabhan]

Contributor details Your Expensify account email: krispv@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01063aad97ce0e22c6

[melvin-bot[bot]]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[jayeshmangwani]

@roryabraham We have been getting some strange issues here, @brkdavis is having some issues with storing their Upwork profile data in melvin-bot. They have another Github and an Upwork account; that account is linked correctly. Should we assign it to their new GH account @kpadmanabhan so we can move forward with PR?

[melvin-bot[bot]]

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

[jayeshmangwani]

@kpadmanabhan What's your ETA for PR?

[kpadmanabhan]

@jayeshmangwani Code changes are good. Working on the unit test strategy for the change.

@jayeshmangwani / @roryabraham : Does writing a test for the string.replace(...) add value? I dont think it does. Hence working on following strategies for unit tests / integration tests

• for useHtmlPaste hook. But this cannot cover if the send button will be enabled or not. This has to be checked in manual test or e2e automated test. Did you mean e2e test when you said unit tests? Facing challenges with heavy consumption of UI components (window and DOM) inside the hooks.
• for composer. This will cover paste and send button by mocking other components around it.

Or do you mean test the newly written regular expression alone? Either by mocking UI components or by extracting string sanitization into a different function and test that alone?

Please let me know your thoughts.

[roryabraham]

@kpadmanabhan thanks for putting some thought into an automated testing strategy. I think that tests in useHtmlPaste covering the removal or carriage returns might be sufficient, and including tests for composer that covers paste and send button would be above-and-beyond (would love to see it). Tests for useHtmlPaste can probably be classified as unit tests, while tests for composer might be more accurately classified as "ui tests", probably relying on react-native-testing-library for mocked renders.

We do not currently have any functional e2e tests, so we shouldn't try to establish a new pattern by adding them here.

[kpadmanabhan]

Thanks @roryabraham.

FYI. Concept for ui tests for composer, as I was trying out these. We can use or extend the below outside the scope of this issue.

        const pasted = jest.fn(() => null);
        const changed = jest.fn(() => null);

        render(<AnimatedMarkdownTextInputRef data-testid="composer-input" />);

        const composerElement = screen.queryByTestId('composer-input');
        useHtmlPaste(composerElement, handlePaste, true);

        const pasteFromClipboard = createEvent.paste(composerElement, {
            clipboardData: {
              getData: () => '123456',
            },
          });

          fireEvent(composerElement, pasteFromClipboard);

          expect(pasted).toHaveBeenCalled();
          expect(pasted).toHaveBeenCalledWith('123456');

We use testing-library/react, but this will need some changes to the component as well to support testability.

[roryabraham]

We use testing-library/react

Let's use testing-library/react-native ?

[melvin-bot[bot]]

@slafortune @jayeshmangwani @roryabraham @kpadmanabhan 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!