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: optimise url matching regex #666

Merged
merged 1 commit into from
Mar 19, 2024
Merged

Conversation

aswin-s
Copy link
Contributor

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

This PR improves the performance of URL parsing regular expression by preventing catastrophic backtracking . This was causing performance issues on mobile devices. The PR adds an early return in regex matching by excluding text that doesn't match a url pattern and there by avoiding the need to validate the string for possible TLDs which is more than 10K characters long.

This is a reattempt to fix the issue, which had caused a regression earlier. #657

Fixed Issues

$ Expensify/App#34324

Proposal: 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#4dc751027f70dcfdd3528990c8fed79cd479a05a",
  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.

@ntdiary
Copy link

ntdiary commented Mar 19, 2024

Works well. :)

Note: What we want to address here is the catastrophic backtracking problem in regular expressions, while still ensuring that the app can correctly parsing normal URLs.

test.mp4

@stitesExpensify stitesExpensify merged commit aed3f80 into Expensify:main Mar 19, 2024
5 checks passed
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.

3 participants