Skip to content

fix: optimise url matching regex#666

Merged
stitesExpensify merged 1 commit into
Expensify:mainfrom
aswin-s:fix/34324
Mar 19, 2024
Merged

fix: optimise url matching regex#666
stitesExpensify merged 1 commit into
Expensify:mainfrom
aswin-s:fix/34324

Conversation

@aswin-s

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

Copy link
Copy Markdown
Contributor

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

ntdiary commented Mar 19, 2024

Copy link
Copy Markdown

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

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