Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Strikethrough link is not parsed correctly on link with URL that contains tilde #28616

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Strikethrough link is not parsed correctly on link with URL that contains tilde #28616

merged 1 commit into from
Oct 4, 2023

Conversation

ZhenjaHorbach
Copy link
Contributor

@ZhenjaHorbach ZhenjaHorbach commented Oct 2, 2023 •
edited
Loading

Details

Strikethrough link isn't parsed correctly on link with URL that contains ~

Fixed Issues

$ #27329
PROPOSAL: #27329 (comment)

Tests

  1. Open a chat
  2. Send this message ~[Example Link](https://example.com/?example=~ex)~ (URL includes ~)
  3. The text must be crossed out

Input: ~[Example Link](https://example.com/?example=~ex)~
Output: Example Link

  • Verify that no errors appear in the JS console

Offline tests

  1. Open a chat
  2. Send this message ~[Example Link](https://example.com/?example=~ex)~ (URL includes ~)
  3. The text must be crossed out

Input: ~[Example Link](https://example.com/?example=~ex)~
Output: Example Link

QA Steps

  1. Open a chat
  2. Send this message ~[Example Link](https://example.com/?example=~ex)~ (URL includes ~)
  3. The text must be crossed out

Input: ~[Example Link](https://example.com/?example=~ex)~
Output: Example Link

  • 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:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • 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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • 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
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • 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 grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is 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
    • If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
    • 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 code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • 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 the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • 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.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Screen.Recording.2023-10-02.at.16.24.12.mov
Mobile Web - Chrome
Screen.Recording.2023-10-02.at.16.24.12.mov
Mobile Web - Safari
Screen.Recording.2023-10-02.at.16.24.12.mov
Desktop
Screen.Recording.2023-10-02.at.16.24.12.mov
iOS
Screen.Recording.2023-10-02.at.16.24.12.mov
Android
Screen.Recording.2023-10-02.at.16.24.12.mov

@ZhenjaHorbach ZhenjaHorbach requested a review from a team as a code owner October 2, 2023 14:25
@melvin-bot melvin-bot bot requested review from robertKozik and removed request for a team October 2, 2023 14:25
@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

@robertKozik Please 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]

@ZhenjaHorbach ZhenjaHorbach changed the title Fix Strikethrough link is not parsed correctly on link with URL that contains tilde Strikethrough link is not parsed correctly on link with URL that contains tilde Oct 2, 2023
@ZhenjaHorbach
Copy link
Contributor Author

@robertKozik
Hello )
PR is ready )

@ZhenjaHorbach
Copy link
Contributor Author

@robertKozik
Hello )
Sorry for reminding you))
PR is ready )

@robertKozik
Copy link
Contributor

Sorry, I'll look into it soon(ish)

@robertKozik
Copy link
Contributor

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:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • 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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • 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 grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is 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 code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • 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 the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • 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.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
web.mov
Mobile Web - Chrome
android.-.ios.-.web.mov
Mobile Web - Safari
android.-.ios.-.web.mov
Desktop
desktop.mov
iOS
ios.-.android.-.native.mov
Android
ios.-.android.-.native.mov

robertKozik
robertKozik approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@robertKozik robertKozik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@melvin-bot melvin-bot bot requested a review from Beamanator October 4, 2023 11:12
Beamanator
Beamanator approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Beamanator Beamanator merged commit 28b30fc into Expensify:main Oct 4, 2023
21 of 76 checks passed
@OSBotify
Copy link
Contributor

OSBotify commented Oct 4, 2023

✋ 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 github-actions bot added the DeployBlockerCash This issue or pull request should block deployment label Oct 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Name Duration
App start TTI 1311.280 ms → 1496.825 ms (+185.545 ms, +14.1%) 🔴
App start runJsBundle 915.156 ms → 1039.233 ms (+124.077 ms, +13.6%) 🔴
Show details
Name Duration
App start TTI Baseline
Mean: 1311.280 ms
Stdev: 50.466 ms (3.8%)
Runs: 1194.042092999909 1242.9619720000774 1244.3108879998326 1253.1152119999751 1260.2774979998358 1261.0445050001144 1261.0654279999435 1262.1406379998662 1280.7973039997742 1289.311226000078 1289.4720910000615 1292.988444000017 1300.1223809998482 1306.0994099997915 1310.3375619999133 1315.4836389999837 1315.7088819998316 1325.5567160001956 1328.6578179998323 1329.0219319998287 1331.4544770000502 1339.5045810001902 1341.855295999907 1341.9146590000018 1341.9504060000181 1358.2135299998336 1362.279281000141 1364.721071000211 1366.4864610000513 1394.1138869998977 1444.6616949997842

Current
Mean: 1496.825 ms
Stdev: 38.545 ms (2.6%)
Runs: 1404.2853069999255 1435.8591240001842 1445.2910810001194 1450.24647800019 1454.4807620001957 1460.1208999999799 1464.4024299997836 1465.2535129999742 1465.9210999999195 1471.8233260000125 1474.7257909998298 1478.7365419999696 1489.6514329998754 1489.8106459998526 1492.557618000079 1499.7006080001593 1502.2866739998572 1504.130774000194 1505.2403480000794 1505.5102360001765 1511.5451159998775 1513.2445820001885 1519.2385550001636 1522.340352000203 1524.3507860000245 1530.2655710000545 1546.8304070001468 1547.0831289999187 1553.6852699997835 1555.5681710001081 1555.8283029999584 1558.381903000176
App start runJsBundle Baseline
Mean: 915.156 ms
Stdev: 41.326 ms (4.5%)
Runs: 827 856 856 872 873 878 881 881 886 889 891 892 901 908 908 910 917 921 924 930 931 931 935 940 940 946 947 957 963 983 997 1014

Current
Mean: 1039.233 ms
Stdev: 28.257 ms (2.7%)
Runs: 1000 1001 1002 1005 1006 1008 1012 1021 1022 1023 1025 1028 1033 1037 1037 1037 1037 1038 1047 1048 1049 1053 1054 1056 1060 1068 1071 1091 1100 1108

Meaningless Changes To Duration

Show entries
Name Duration
Open Search Page TTI 668.249 ms → 693.317 ms (+25.068 ms, +3.8%)
App start nativeLaunch 23.469 ms → 25.484 ms (+2.015 ms, +8.6%)
App start regularAppStart 0.020 ms → 0.019 ms (-0.001 ms, -3.9%)
Show details
Name Duration
Open Search Page TTI Baseline
Mean: 668.249 ms
Stdev: 29.356 ms (4.4%)
Runs: 612.9733480000868 618.4118650001474 624.3870450002141 626.8862310000695 636.662639000453 637.8805750003085 645.4134519998915 648.1597499996424 654.4819340002723 656.3327229996212 657.238078000024 661.0252689998597 661.3097329996526 661.656739000231 666.5670169997029 667.3639329997823 668.5127770002 669.8519700001925 676.4558109999634 676.7520350003615 677.809936999809 682.4802250000648 683.0841069999151 686.0586749999784 687.3419600003399 692.9329840000719 698.0407310002483 703.3796800002456 714.8493659999222 725.5927729997784 735.8277190001681

Current
Mean: 693.317 ms
Stdev: 28.623 ms (4.1%)
Runs: 641.4432379999198 648.2784429998137 658.5200600000098 660.2983800000511 663.6138520003296 667.4951989999972 667.7345389998518 670.2392580001615 671.9966230001301 673.4997560000047 675.8858230002224 680.5571699999273 680.6278490000404 680.6549490001053 684.227498000022 685.2362060002051 692.3387460000813 697.0189619995654 697.0837409999222 699.1553960000165 702.3021240001544 702.7440190003254 703.3785400004126 704.746907999739 708.6402999996208 708.8619389999658 718.4428309998475 718.481037999969 724.6104740002193 734.2097169999033 743.0041910000145 752.5048830001615 761.6417240002193
App start nativeLaunch Baseline
Mean: 23.469 ms
Stdev: 2.883 ms (12.3%)
Runs: 18 19 20 20 20 20 21 21 21 21 22 23 23 23 23 23 23 24 24 24 24 25 25 25 26 26 27 27 27 28 29 29

Current
Mean: 25.484 ms
Stdev: 2.108 ms (8.3%)
Runs: 23 23 23 23 23 24 24 24 24 24 24 24 25 25 25 25 25 25 25 26 26 26 26 27 27 27 28 29 29 30 31
App start regularAppStart Baseline
Mean: 0.020 ms
Stdev: 0.001 ms (7.1%)
Runs: 0.016600999981164932 0.017293000128120184 0.01733399974182248 0.01806599972769618 0.018066000193357468 0.018106999807059765 0.01831099996343255 0.018635999877005816 0.018919999711215496 0.019001999869942665 0.01912499964237213 0.0194089999422431 0.019816000014543533 0.01985699962824583 0.019897999707609415 0.019978999625891447 0.020018999930471182 0.020142000168561935 0.02022299962118268 0.020345000084489584 0.02038600016385317 0.02046799985691905 0.020506999921053648 0.020589000079780817 0.020629999693483114 0.0206300001591444 0.020711000077426434 0.020913999993354082 0.02103699976578355 0.022419999819248915 0.02290799980983138

Current
Mean: 0.019 ms
Stdev: 0.001 ms (2.8%)
Runs: 0.0181470001116395 0.01822900027036667 0.01823000004515052 0.018309999722987413 0.018433000426739454 0.018472999799996614 0.018473000265657902 0.018514000345021486 0.018554999958723783 0.018596000038087368 0.018717000260949135 0.01879799971356988 0.018798999954015017 0.018798999954015017 0.019003000110387802 0.01904299994930625 0.019043000414967537 0.019082999788224697 0.019084000028669834 0.01912399986758828 0.019164999946951866 0.019164999946951866 0.019166000187397003 0.019367999862879515 0.019694000016897917 0.019733999855816364 0.0197759997099638 0.020345000084489584

@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2023

@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker.

@OSBotify OSBotify mentioned this pull request Oct 5, 2023
Closed
57 tasks
@OSBotify
Copy link
Contributor

OSBotify commented Oct 5, 2023

🚀 Deployed to staging by https://github.com/Beamanator in version: 1.3.78-0 🚀

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

@melvin-bot melvin-bot bot mentioned this pull request Oct 6, 2023
Closed
6 tasks
@OSBotify OSBotify mentioned this pull request Oct 6, 2023
Closed
@OSBotify
Copy link
Contributor

OSBotify commented Oct 6, 2023

🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.78-4 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Oct 6, 2023

🚀 Deployed to staging by https://github.com/Beamanator in version: 1.3.79-0 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Oct 9, 2023

🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Oct 9, 2023

🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 cancelled 🔪
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

OSBotify commented Oct 9, 2023

🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀

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

@MelvinBot MelvinBot mentioned this pull request May 7, 2024
Open
50 tasks
@melvin-bot melvin-bot bot mentioned this pull request May 7, 2024
Closed
@pavelbe4solutions pavelbe4solutions mentioned this pull request May 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Beamanator Beamanator Beamanator approved these changes

@robertKozik robertKozik robertKozik approved these changes

Assignees
No one assigned
Labels
DeployBlockerCash This issue or pull request should block deployment
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ZhenjaHorbach @robertKozik @OSBotify @Beamanator