Autolink when edititng comments except wehn explicit link removal

[markarmanus]

Details

The issue is when we edit a comment that has links we dont autolink them so that you can click on them. This is because we purposely removed autolnking in this PR #9090
The reason we removed it was when a user would delete the link from a comment it would re genrate it again.

The solution is this PR is to always generate a link unles a user explicitly removes it while editing. This requires comapring the original comment and after deleting comment, detecting which links if any the user removed explicitly and delete them.

Fixed Issues

$ #13221
$ #13221 (comment)

Tests

Same as QA

• Verify that no errors appear in the JS console

Offline tests

No specefic difference on offline. should behave exactly the same as online.

QA Steps

1. Write a new message to any chat.
   (e.g from `` to hello)
2. edit the message and add a link
   (e.g from hello to hello www.google.com)
3. confirm the link is generated and can be clicked on.
4. edit message again now add more normal text
   (e.g hello [www.google.com](https://www.google.com) to. hello [www.google.com](https://www.google.com) bye)
5. confirm the link is still there
6. edit message again and remove the link part and the []
   (e.g from hello [www.google.com](www.google.com) bye to hello www.google.com bye)
7. confirm the link is removed
8. edit the message again and any text
   (e.g from hello www.google.com bye to hello www.google.com bye newText)
9. confirm the link is generated again.
10. delete only the () part of the link
    (e.g from hello [www.google.com](https://www.google.com) bye newText to hello [www.google.com] newText bye)
11. confirm the link is removed but the [] are still there. (i.e comment should now be hello [www.google.com] newText bye
12. add a second link to the message
    (e.g from hello [www.google.com] bye newText to hello [www.google.com] bye newText facebook.com)
13. confirm two links are generated one for google and one for facebook. and the google one is in []
14. now delete the () part of the facebook link
    (e.g from helle [[www.google.com](https://www.google.com)] bye newText [facebook.com](https://www.facebook.com) to helle [[www.google.com](https://www.google.com)] bye newText facebook.com
15. confirm only the facebook link is deleted.

Note: if the comment has the same link multiple times it could cause odd behaviour but its an edge case so should be okay.

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • iOS / native
  • Android / native
  • iOS / Safari
  • Android / Chrome
  • MacOS / Chrome
  • MacOS / Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• I verified that if a function's arguments changed that all usages have also been updated correctly
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.
  off every checkbox in the PR author checklist, including those that don't apply to this PR.

Desktop,Android and IOS

Screen.Recording.2022-12-13.at.12.35.32.AM.mp4

Screenshots/Videos

Web

Screen.Recording.2022-12-15.at.2.45.01.PM.mp4

Mobile Web - Chrome

Screen.Recording.2022-12-15.at.4.06.17.PM.mov

Mobile Web - Safari

Screen.Recording.2022-12-15.at.4.10.13.PM.mov

Desktop

Please refer to video above with all Desktop, Android and IOS

iOS

Please refer to video above with all Desktop, Android and IOS

Android

Please refer to video above with all Desktop, Android and IOS

[melvin-bot]

@cead22 @eVoloshchak One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

[eVoloshchak]

The code looks good and works well, there are a couple of comments and stylistic issues that need to be addressed

@markarmanus, as per this discussion, could you please add tests for all the cases you tested in the video?

[markarmanus]

@eVoloshchak all comments addressed and extended the testing to include everything in the video.

[eVoloshchak]

@eVoloshchak all comments addressed and extended the testing to include everything in the video.

Thanks!
I think @cead22 was talking about the unit tests

[cead22]

Yes, please add unit tests for all the cases you tested manually 🙇

[markarmanus]

@cead22 @eVoloshchak Added unit tests. Also for some reason re requesting the review from @eVoloshchak removed @cead22. apologies for that. i dont seem to have premissions to add you again.

[markarmanus]

@cead22 @eVoloshchak any updates ?

[eVoloshchak]

Reviewing in a couple of hours

[markarmanus]

@eVoloshchak @cead22 Sorry to be buggy, but i only have sometime to work on this today and i dont want to miss the 3 days bonus :D would really appreciate if you guys can find the time for it soon. Again apologies

[cead22]

Sorry, but there's a few items left to polish this PR

[markarmanus]

@cead22 Done and no worries at all, thats the entire point of PR reviews. to make sure the code quality is up to snuff. You have a good eye.

[eVoloshchak]

@markarmanus, there are a couple of issues left with the PR description

1. Change the PR title to be more informative, it should reflect what has been done/fixed
2. Upload a separate video for each platform, following the PR template
3. PR Author Checklist is failing, I think you just need to copy a new one from here

[eVoloshchak]

Reviewer Checklist

• I have verified the author checklist is complete (all boxes are checked off).
• I verified the correct issue is linked in the ### Fixed Issues section above
• I verified testing steps are clear and they cover the changes made in this PR
  • I verified the steps for local testing are in the Tests section
  • I verified the steps for Staging and/or Production testing are in the QA steps section
  • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
• I checked that screenshots or videos are included for tests on all platforms
• I included screenshots or videos for tests on all platforms
• I verified tests pass on all platforms & I tested again on:
  • iOS / native
  • Android / native
  • iOS / Safari
  • Android / Chrome
  • MacOS / Chrome
  • MacOS / Desktop
• If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
• I verified proper code patterns were followed (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I verified that this PR follows the guidelines as stated in the Review Guidelines
• I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Screen.Recording.2022-12-15.at.11.18.32.mov

Not able to test due to an issue with the back end, using a Desktop video instead

Mobile Web - Chrome

Screen.Recording.2022-12-15.at.11.18.32.mov

Not able to test due to an issue with the back end, using a Desktop video instead

Mobile Web - Safari

Screen.Recording.2022-12-15.at.11.18.32.mov

Not able to test due to an issue with the back end, using a Desktop video instead

Desktop

Screen.Recording.2022-12-15.at.11.18.32.mov

iOS

Screen.Recording.2022-12-15.at.11.15.07.mov

Android

Screen_Recording_20221215-112215_New.Expensify.mp4

-- add screenshots or videos here -->

[cead22]

@cead22 Done and no worries at all, thats the entire point of PR reviews. to make sure the code quality is up to snuff. You have a good eye.

Thanks for understanding

[markarmanus]

@AndrewGable Also no need to apologize always open to improve the code.

[markarmanus]

cc: @eVoloshchak @cead22

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[github-actions]

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
TTI	787.761 ms → 820.864 ms (+33.102 ms, +4.2%)
runJsBundle	185.968 ms → 187.233 ms (+1.266 ms, +0.7%)
nativeLaunch	10.129 ms → 10.594 ms (+0.465 ms, +4.6%)
regularAppStart	0.016 ms → 0.014 ms (-0.002 ms, -10.1%)

Show details

Name	Duration
TTI	Baseline Mean: 787.761 ms Stdev: 39.088 ms (5.0%) Runs: 737.7522340007126 741.1830490007997 745.6529820002615 749.0772720016539 752.4258259981871 753.5808000005782 755.9827740006149 758.8155900016427 761.2115149982274 761.6394039988518 762.759472001344 765.9158999994397 767.3703549988568 769.7797710001469 770.0257360003889 786.0559450015426 789.2824199981987 789.658707998693 791.6496590003371 793.4442900009453 795.413504999131 796.2184639982879 803.9495179988444 811.0301310010254 817.8686849996448 836.9137879982591 852.5440489985049 861.4352440014482 867.3746519982815 886.8266890011728 Current Mean: 820.864 ms Stdev: 52.273 ms (6.4%) Runs: 739.2278859987855 768.3503310009837 770.3983099982142 771.3267190009356 771.5155409984291 772.224305998534 778.1988920010626 779.214958999306 783.110011998564 783.4225759990513 783.7271559983492 784.589811000973 795.0935500003397 804.5152899995446 804.6776689998806 811.2334939986467 813.5079139992595 814.6920420005918 815.3123290017247 823.7906140014529 826.000029001385 836.4664990007877 848.6284330002964 862.2920570001006 865.817279998213 873.5885020010173 882.8974509984255 885.1661729998887 907.5501019991934 922.1723809987307 968.0632440000772
runJsBundle	Baseline Mean: 185.968 ms Stdev: 19.909 ms (10.7%) Runs: 157 162 163 165 165 169 169 172 172 174 175 175 177 179 182 182 182 183 185 187 191 191 191 200 200 208 213 215 217 222 242 Current Mean: 187.233 ms Stdev: 21.129 ms (11.3%) Runs: 157 162 165 166 166 167 170 172 173 175 177 178 179 181 182 183 183 184 185 187 191 194 199 202 209 215 219 227 229 240
nativeLaunch	Baseline Mean: 10.129 ms Stdev: 1.408 ms (13.9%) Runs: 8 9 9 9 9 9 9 9 9 9 9 9 10 10 10 10 10 10 10 10 10 10 10 11 11 11 11 13 13 13 14 Current Mean: 10.594 ms Stdev: 1.730 ms (16.3%) Runs: 8 8 8 8 9 9 9 9 9 10 10 10 10 10 10 10 11 11 11 11 11 11 11 12 12 12 12 12 13 13 14 15
regularAppStart	Baseline Mean: 0.016 ms Stdev: 0.001 ms (6.4%) Runs: 0.013712998479604721 0.014281999319791794 0.014364000409841537 0.014403998851776123 0.014729999005794525 0.014730002731084824 0.014812000095844269 0.014974001795053482 0.015096001327037811 0.015177998691797256 0.015258997678756714 0.015339996665716171 0.01534000039100647 0.015420999377965927 0.015421997755765915 0.01550300046801567 0.015828002244234085 0.01586899906396866 0.015990998595952988 0.016072001308202744 0.016113001853227615 0.016154002398252487 0.016316000372171402 0.016439002007246017 0.016479000449180603 0.016520000994205475 0.016683001071214676 0.016805000603199005 0.016885999590158463 0.01741499826312065 0.0185139998793602 Current Mean: 0.014 ms Stdev: 0.001 ms (5.3%) Runs: 0.012816999107599258 0.013020999729633331 0.013062000274658203 0.01314299926161766 0.013386998325586319 0.01342800259590149 0.013468999415636063 0.013509001582860947 0.013672001659870148 0.013833999633789062 0.013915996998548508 0.01395600289106369 0.01395699754357338 0.013996999710798264 0.014038000255823135 0.014119002968072891 0.014201000332832336 0.014364000409841537 0.014688998460769653 0.014810997992753983 0.01493300125002861 0.014933999627828598 0.015055999159812927 0.015136998146772385 0.015177000313997269 0.015217997133731842 0.015380997210741043

[OSBotify]

🚀 Deployed to staging by @cead22 in version: 1.2.42-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[OSBotify]

🚀 Deployed to production by @yuwenmemon in version: 1.2.42-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[s77rt]

This PR caused a regression here #16571.
We used to trim draft messages, convert to html and send to the server.
With this PR the process is a little different:

1. Trim draft messages
2. Convert to html
3. Convert to markdown
4. Convert to html
5. Send to the server

In step 3, we need to trim the message again as that step undo the trim of step 1

[fedirjh]

Hey there! There is a regression from this PR in #18514. When we capitalize the domain of the edited URL, the URL gets removed. Please note that domain names are case-insensitive according to RFC 4343.

[aimane-chnaif]

Coming from #18911:
I understand it's difficult to find edge cases but this regex didn't handle such cases like [[test]](test.com)

[aimane-chnaif]

For consistency, we're now using markdown link regex exported from expensify-common.

[parasharrajat]

There was a case where resaving the edited message without changing the content will parse the link again #29225. Instead, it should not auto-link when it is removed explicitly and the content is not changed.