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.

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

fix: prevent matching of urls nested inside tags #651

Merged
merged 2 commits into from
Feb 19, 2024

Conversation

aswin-s
Copy link
Contributor

@aswin-s aswin-s commented Feb 16, 2024

This PR improves the performance of URL parsing regular expression by preventing matching of urls nested inside html tags like <code>, <pre> etc. This was causing performance issues on mobile devices. The PR adds an early return in regex matching by excluding links inside tags and there by avoiding the need to validate the string for possible TLDs which is more than 10K characters long.

Fixed Issues

$ GH_LINK Expensify/App#34324

Details: Expensify/App#34324 (comment)

Tests

  1. What unit/integration tests cover your change? What autoQA tests cover your change?
    New test cases were added to verify that urls nested inside tags are not matched.

  2. What tests did you perform that validates your changed worked?
    Tested with Expensify App to make sure that url parsing is working as expected.

Follow steps below:

  1. Update package json to use this branch for expensify-commons
"expensify-common": "git+ssh://git@github.com/Expensify/expensify-common.git#8239f30ea92dc493f582813b9684558a0af386fd",
  1. Install packages via npm i
  2. Start the app
  3. Verify that url parsing in chat threads are working as expected.

QA

  1. What does QA need to do to validate your changes?
    Same as tests
  2. What areas to they need to test for regressions?
    This PR shouldn't introduce any regression related to URL validation and parsing within Expensify app.

@aswin-s aswin-s marked this pull request as ready for review February 16, 2024 07:17
@aswin-s aswin-s requested a review from a team as a code owner February 16, 2024 07:17
@melvin-bot melvin-bot bot requested review from techievivek and removed request for a team February 16, 2024 07:18
@AndrewGable AndrewGable requested review from AndrewGable and removed request for techievivek February 19, 2024 17:33
@AndrewGable AndrewGable merged commit 83ae619 into Expensify:main Feb 19, 2024
5 checks passed
aswin-s added a commit to aswin-s/expensify-common that referenced this pull request Feb 29, 2024
@aswin-s aswin-s mentioned this pull request Feb 29, 2024
MonilBhavsar added a commit that referenced this pull request Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants