[HOLD for payment 2024-04-01] [$250] Android - App lags after adding code block in task description

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.24-0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause internal team
Slack conversation:

Action Performed:

1. Open the app and log in
2. Tap the FAB > Assign Task
3. Enter title
4. Add the code block in the task description:
   if (files.length > 0) {
   // Prevent the default so we do not post the file name into the text box
   event.preventDefault(); this.props.onPasteFile(event.clipboardData.files[0]);
   return;
   if (files.length > 0) {
   // Prevent the default so we do not post the file name into the text box
   event.preventDefault() this.props.onPasteFile(event.clipboardData.files[0]);
   return;
5. Proceed to the next screen and finish creating the task

Expected Result:

Able to create a task without significant delay

Actual Result:

The app lags after pasting the code block in the description field

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6338420_1704939519196.video_2024-01-10_21-17-46.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~013b44c7d5e9e29617
• Upwork Job ID: 1745374242762850304
• Last Price Increase: 2024-03-15
• Automatic offers:
• ntdiary | Reviewer | 0
• aswin-s | Contributor | 0

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~013b44c7d5e9e29617

[melvin-bot]

Triggered auto assignment to @garrettmknight (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ntdiary (External)

[garrettmknight]

Waiting on proposals

[sfurqan2]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The app lags when code is entered in description textbox

What is the root cause of that problem?

From what I have investigated so far, I think default value in the component is parsing the text entered into the textbox. When we enter the code above, I think the parser takes longer time to render which causes the lag.

App/src/pages/tasks/NewTaskDetailsPage.js

Line 116 in 8e2fb67

defaultValue={parser.htmlToMarkdown(parser.replace(taskDescription))}

What changes do you think we should make in order to solve the problem?

I think we should remove parser from default value attribute in text input and move it to where "task description" state default value is set.

What alternative solutions did you explore? (Optional)

NA

[aswin-s]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Adding code block in task description makes the page laggy.

What is the root cause of that problem?

PR #29144 modified the task description input logic to support mark down. Task description is parsed to mark down before saving and vice versa when setting the default value to input.

However the parsing logic which uses the method parser.replace() is computationally intensive.

CPU Profiling above shows modifyTextForUrlLinks, which gets invoked by parser.replace taking around 6 seconds to execute when transitioning from New Task to Confirm task page.

There are 3 locations in Task creation where parser.replace is used.

App/src/pages/tasks/NewTaskDetailsPage.js

Line 116 in 8ba2782

defaultValue={parser.htmlToMarkdown(parser.replace(taskDescription))}

App/src/pages/tasks/NewTaskDescriptionPage.js

Line 72 in 8ba2782

defaultValue={parser.htmlToMarkdown(parser.replace(props.task.description))}

App/src/pages/tasks/TaskDescriptionPage.js

Line 120 in 8ba2782

defaultValue={parser.htmlToMarkdown((props.report && parser.replace(props.report.description)) || '')}

Note that defaultValue prop is directly assigned a function call instead of a memoized value. This causes the computationally intensive method to be invoked whenever the page state changes. This in turn causes the JS bridge to get blocked and animations to lag.

Also the useEffect here is dependent on props.task.

App/src/pages/tasks/NewTaskDetailsPage.js

Lines 46 to 49 in d4c28a4

	useEffect(() => {
	setTaskTitle(props.task.title);
	setTaskDescription(parser.htmlToMarkdown(parser.replace(props.task.description || '')));
	}, [props.task]);

When navigating from NewTaskDetailPage to NewTaskPage(ConfirmTask) screen , the first screen is not unmounted by ReactNavigation. It keeps the page in memory for navigating back. This causes the useEffect to get executed when task object is updated in Onyx. Which also invokes the parser.replace method.

So overall a computationally intensive method gets unnecessarily executed multiple times causing the app to lag.

What changes do you think we should make in order to solve the problem?

Update URL_WEBSITE_REGEX in expensify-commons with below regex

https://github.com/Expensify/expensify-common/blob/6ae333c38ca5909245c6310db1dc66728d62d0ed/lib/Url.js#L5

const URL_WEBSITE_REGEX = `${URL_PROTOCOL_REGEX}?((?:www\\.)?[a-z0-9](?=(?<label>[-a-z0-9]*[a-z0-9]))\\k<label>?\\.)+(?:${TLD_REGEX})(?:\\:${ALLOWED_PORTS}|\\b|(?=_))(?!@(?:[a-z\\d-]+\\.)+[a-z]{2,})`;

This would prevent catastrophic backtracking issue with TLD regex.

Then memoize the defaultValue prop using React.useMemo, so that parser.replace gets executed only when description changes.

const defaultValue = useMemo(() => parser.htmlToMarkdown(parser.replace(taskDescription)), [taskDescription]);

Also the useEffect should get executed only when the screen is focused. Update the logic to prevent unwanted execution offscreen.

const isFocused = useIsFocused();

useEffect(() => {
  // Update state only if screen is focused
  if (isFocused) {        
    setTaskTitle(props.task.title);
    setTaskDescription(parser.htmlToMarkdown(parser.replace(props.task.description || '')));
  }
}, [props.task.title, props.task.description, isFocused]);

Result

Here is the CPU profile after applying the fix. Overall time spent in parser.replace reduced to 2s from 6.3s.

What alternative solutions did you explore? (Optional)

The other area to focus on is actually optimising the parser.replace logic in ExpensiMark library. However it is in Expensify-Common repository and possibly shared by multiple apps.

[garrettmknight]

@ntdiary looks like we've got some proposals for this one. Can you take a look when you get a chance?

[ntdiary]

Ah, sorry for the delay, I missed this issue, will review soon.

[ntdiary]

Thank you all for the proposals. Although the related regular expressions are very complex, I still believe that optimizing them is the fundamental solution. Even with the use of useMemo, I feel that a 2s delay is not ideal for the user. :)

https://github.com/Expensify/expensify-common/blob/b682bb4078caaffa2f54ab8c35dd76178e322e11/lib/ExpensiMark.js#L183

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[ntdiary]

Inline code is a valid input, and we support markdown syntax not only on this page, but also on other pages. So we should optimize the execution efficiency of the relevant regular expressions.

[sfurqan2]

Hey @ntdiary, I was of the assumption that we only show inline code on page load, as I did not see other components parsing the updated description text. Please let me know if that's not the correct behavior.

[ntdiary]

Not only the description text, but also every case that calls parser.replace may experience this delay. :)

[sfurqan2]

I agree then, we must think of some way to optimize this regex. :)

[aswin-s]

@ntdiary The real culprit is the huge tlds regex that's being used in MARKDOWN_URL_REGEX. To test the theory I replaced the TLD_REGEX const with a much simpler regex which simply validates if the string is a valid tlds instead of explicitly matching specific TLDs.

const TLD_REGEX='(?:[a-zA-Z]{2,63}|xn--[a-zA-Z0-9-]{4,59})';

And here are the results for a single execution of parser.replace(). With original TLD_REGEX execution time is approximately 1s where as for simpler regex it is 10ms. Also it is important to note that the execution time is directly proportional to the input length. If I copy over the same test input 4 times, execution time with original regex becomes 4 seconds. That's 1s for around 350 characters. Which is quite huge!

Before	After

So it boils down to the decision whether we can trim down the specific TLD names with in the TLD_REGEX.

[aswin-s]

@aswin-s, it seems I misplaced the ? character. This new regex should be able to match single-character cases 😂

-((?:www\\.)?[a-z0-9](?=(?<label>[-a-z0-9]*[a-z0-9]))\\k<label>?\\.)+(?:${TLD_REGEX})
+((?:www\\.)?[a-z0-9](?=(?<label>[-a-z0-9]*[a-z0-9])?)\\k<label>\\.)+(?:${TLD_REGEX})

@ntdiary This seems to be working. I'll raise a new PR shortly.

[garrettmknight]

@aswin-s how's that PR coming along?

[melvin-bot]

Upwork job price has been updated to $250

[garrettmknight]

Dropping price for regression

[aswin-s]

Raised PR Expensify/expensify-common#666

[garrettmknight]

PR is merged, awaiting deploy to test.

[garrettmknight]

@AndrewGable for expensify-common changes, when do we expect to be able to test them? Is it immediately?

[AndrewGable]

There needs to be an App PR to use the latest changes of expensify-common, then we can test

[ntdiary]

PR #37814 has bumped expensify-common version, but QA is ongoing, so we can test in the next QA stage. :)

[garrettmknight]

Great - I'll test again on Monday!

[garrettmknight]

Tested and confirmed it's much faster. Setting for payment.

[garrettmknight]

Payment Summary:

• Contributor: @aswin-s $250 paid via Upwork
• Reviewer: @ntdiary $250 (link)

[garrettmknight]

All paid, closing.