[$500] Android - Chat - Trying to paste onyx data of #applause.expensifail.com chat app freezes

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 9.9.16 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Action Performed:

1. Launch app
2. Long press #applause.expensifail.com and select copy onyx data
3. Open a report and try to paste it

Expected Result:

Trying to paste onyx data of #applause.expensifail.com chat app must not freeze

Actual Result:

Trying to paste onyx data of #applause.expensifail.com chat app freezes and shows not responding pop up box

Workaround:

Unknown

Platforms:

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

• [x] Android: Native
• [ ] Android: mWeb Chrome
• [ ] iOS: Native
• [ ] iOS: mWeb Safari
• [ ] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/29bd8bfb-e1bb-4040-9050-a5d04c8696a9

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d242239866ebb6cc
• Upwork Job ID: 1822052495061109151
• Last Price Increase: 2024-08-24
• Automatic offers:
• c3024 | Reviewer | 103915587
• dominictb | Contributor | 103915591

Issue Owner

Current Issue Owner: @c3024

[melvin-bot[bot]]

Triggered auto assignment to @kevinksullivan (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lanitochka17]

We think that this bug might be related to #vip-vsp

[lanitochka17]

@kevinksullivan FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

We think that this bug might be related to

wave-control

[melvin-bot[bot]]

@kevinksullivan Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot[bot]]

@kevinksullivan Huh... This is 4 days overdue. Who can take care of this?

[kevinksullivan]

I don't know where this fits roadmap wise, since it's just general chat troubleshooting.

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~01d242239866ebb6cc

[melvin-bot[bot]]

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

[kevinksullivan]

will just put external on it for now to see if it is a quick fix.

[c3024]

@lanitochka17

This works well with chats on my account. I guess this bug happens because a lot of data is pasted at once.

Could you paste the data in a notepad and share the text file?

Does pasting this data work well on other platforms?

[lanitochka17]

@c3024 вставка.txt Issue reproducible on mWeb/Safari, mWeb/Chrome, IOS Not reproducible in Web

[c3024]

Waiting for proposals

[melvin-bot[bot]]

@kevinksullivan, @c3024 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[c3024]

Waiting for proposals!

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@kevinksullivan @c3024 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot[bot]]

@kevinksullivan, @c3024 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[c3024]

Waiting for proposals!

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@kevinksullivan, @c3024 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot[bot]]

Upwork job price has been updated to $500

[kevinksullivan]

Ah I see this is not reproducible as of late so I am going to close it.

[dominictb]

Proposal

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

Trying to paste onyx data of #applause.expensifail.com chat app freezes and shows not responding pop up box.

Note: Please use the QA text file here

What is the root cause of that problem?

The text file contains 400k characters, and this will basically overload our ExpensiMark parser. Which means, any parsing logic using ExpensiMark regarding this length input text will result in a freeze.

There's a comment here in the codebase that explains the performance issue, and also suggesting a potential solution: https://github.com/Expensify/App/blob/b1882434a26e24a159322962597f07251d8365ea/src/libs/ReportUtils.ts#L4005

We can try to swap markdown text input with normal TextInput in here and disable the useHtmlPaste in here and the app won't freeze anymore

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

The idea is that we should avoid parsing long text using ExpensiMark. To update it for both native and web platform, we need to follow these steps:

0. Define a upper limit, i.e: If we won't parse the text if the text length is larger than that. It could a bit more than 10000, as we won't allow to send any text of more than 10k characters to the BE anyway.

1. In the app: We should fix the useHtmlPaste

• In here, skip the parsing if the text length is larger than XXX

• In the web platform, the app also freezes in case of long text paste due to document.execCommand('insertText'), so we should replace insertByCommand here with

(textInputRef.current as unknown as HTMLInputElement)?.setRangeText(text, 0, 0, 'end')
textInputHTMLElement?.dispatchEvent(new Event('input', {bubbles: true})); // this is to trigger the onChange event, which sync the pasted value with the text input state

this will make sure the app won't freeze because of large text paste

2. In the react-native-live-markdown, we should prevent parsing long text in the parser. To make it flexible (as this is an external library), we can introduce a prop maxParsableTextLength: number, and we should prevent the text parsing for all platforms (web, ios, android)

2.1, For web:

• In here, add a condition to set the ranges = [] if text length is over the limit

2.2, For native platform:

• Add the maxParsableTextLength definition here

2.2.1, For iOS, we can support the new prop the same way as markdownStyle in here and in here. The maxParsableTextLength will be set in the RCTMarkdownUtils the same way as markdownStyle here, and in here, we should skip parsing and let the output to be empty array in case the text length is larger than max length.

2.2.3, For Android, we approach with same way with iOS: Define the new prop in here (also support the abstract method in oldarch in here, and in here, set the output to empty array json string [] if the text length is larger than pre-defined limit

What alternative solutions did you explore? (Optional)

We can impose a hard limit in react-native-live-markdown parser itself but I don't think that's a wise choice since the same defined limit will be hard-coded in 2 different places (1 in the app, another in the react-native-live-markdown library), and also since this is an external library, whether parsing/not parsing long text should depend on library user's choice (even though we could flag a warning about the performance issue of the expensify-common with long text)

[dominictb]

@kevinksullivan this issue is still reproducible on latest main as well as in staging with this file here, so can we re-open the issue?

[dominictb]

gentle bump @kevinksullivan @c3024 for re-opening the issue!

[c3024]

@kevinksullivan , this is still reproducible. Please reopen this issue. @dominictb posted a viable solution.

[dominictb]

@kevinksullivan bump for re-opening the issue.

[dominictb]

@kevinksullivan @c3024 bump for re-opening the issue.

[Julesssss]

As mentioned above, this is still reproducible.

@c3024 could you please review this proposal, thanks.

[c3024]

I agree with the RCA. I think the primary solution in the proposal outlined by @dominictb will fix this issue, along with allowing the users of the react-native-live-markdown library to decide the max parsable length. We can work on further specifics in the PR.

🎀 👀 🎀 C+ Reviewed

[melvin-bot[bot]]

Current assignee @Julesssss is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot[bot]]

📣 @c3024 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[melvin-bot[bot]]

📣 @dominictb 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[kevinksullivan]

Looping in another BZ member as I'll be OOO

[melvin-bot[bot]]

Triggered auto assignment to @sonialiap (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[tomekzaw]

I'm not sure if skipping Markdown formatting is the right fix (it's definitely poor user performance).

Instead, can we focus on improving the performance instead of ignoring the problem?

[Julesssss]

Instead, can we focus on improving the performance instead of ignoring the problem?

Are you able to share an alternative proposal?

[tomekzaw]

Are you able to share an alternative proposal?

Not really, just wanted to suggest a different approach to this very problem, not a concrete solution.

As for the current proposal, I don't think maxParsableTextLength is a good solution as well. Not only this is a very specific condition (text length) but also moves the responsibility of implementing the logic to the live-markdown library. How about exposing a boolean prop enableFormatting (or formattingEnabled) from live-markdown library (with a default value of true) and moving the condition logic to E/App? This is more extendable, e.g. it will allow us to disable Markdown formatting based on the complexity of the text (e.g. how many syntax elements are there) instead of solely on text length.

[Julesssss]

The ideas of using a lib prop, and moving responsibility of the cutoff point to the client are both good suggestions.

@tomekzaw are you happy for @dominictb to merge those into his solution?

[dominictb]

https://github.com/Expensify/App/issues/46766#issuecomment-2352408316. I'm good with this approach. Will update my work accordingly.

[tomekzaw]

@Julesssss Sure, sounds like a plan.

[tomekzaw]

@dominictb As for the prop name, maybe let's go with enableMarkdownFormatting? What do you think?

[dominictb]

That's fine by me @tomekzaw.

[melvin-bot[bot]]

@Julesssss, @sonialiap, @kevinksullivan, @c3024, @dominictb Huh... This is 4 days overdue. Who can take care of this?

[dominictb]

Still working on the PR.

[dominictb]

@tomekzaw sorry for asking, but if we use enableMarkdownFormatting: boolean, does that mean whenever there's a text change event:

• Do not parse the markdown format immediately (which is what we are doing right now)
• enableMarkdownFormatting will be calculated and the markdown will (possibly, based on the flag) be parsed in the next re-render phase

This basically means the current implementation of markdown input component will be changed dramatically. Do you think it's OK to do that? The point is we need to stop the markdown formatting immediately after the text change event, hence this dilemma

[tomekzaw]

@dominictb I'm not sure if I understand the dillema correctly. Specifically:

does that mean whenever there's a text change event

What is the actual question?

enableMarkdownFormatting will be calculated

enableMarkdownFormatting is a prop that is passed from outside, it will not be calculated

This basically means the current implementation of markdown input component will be changed dramatically. Do you think it's OK to do that?

Will it indeed?

For web implementation, it's just a matter of using enableMarkdownFormatting so that globalThis.parseExpensiMarkToRanges is not called and simply return [] instead.

For native implementation, it's just a matter of passing enableMarkdownFormatting prop to MarkdownTextInputDecoratorView and doing the same thing but on native side.

The point is we need to stop the markdown formatting immediately after the text change event, hence this dilemma

What's the actual dillema?

If you want, I can implement enableMarkdownFormatting prop in react-native-live-markdown myself and you will just use it in your app to E/App.

[dominictb]

@tomekzaw sorry it should be clearer from me. Let me give you an example:

1. Let's say the current input text length is 5k
2. The user paste another text of 5k length appending to the current text content

Let's see what happen in MarkdownInput component (web), given that enableMarkdownFormatting={value > 5100}

• After (1), enableMarkdownFormatting=true, and text is parsed accordingly
• After (2), this will be triggered, which in turn lead to the text being parsed here, and this function will be called, as at this point, enableMarkdownFormatting is still true.

What I intend in my original proposal is that, I want this function not to be called immediately when the text change, but this seems unachievable with your idea (I think?).

Correct me if I'm wrong. Thank you for your help!

[tomekzaw]

@dominictb Thanks for your response, I see the problem now.

So if the value of enableMarkdownFormatting is calculated based on value state, then it would still format message if the text was pasted since value is not updated at this point yet.

The workaround in this case would be to pass a function, instead of a boolean, to enableMarkdownFormatting. This function would be invoked each time we need to check if we should parse the current text (no matter if it's from React state or from event handlers like onTextChanged). So the API would be like:

enableMarkdownFormatting: (value: string) => boolean;

and the usage would be:

enableMarkdownFormatting={(value) => value > 5100}

However, this would be problematic on Android and iOS since regular JS functions can't be run on the UI thread unless they are marked as worklets. Note that we're actually integrating worklets to live-markdown in https://github.com/Expensify/react-native-live-markdown/pull/439. This would mean that we would need two separate worklets now:

enableMarkdownFormatting: (value: string) => boolean;
parser: (value: string) => MarkdownRange[]

So, why don't we merge this logic into parser worklet? We can just return [] at the beginning of the parser worklet if some condition is met (e.g. value.length > 5100).

Finally, https://github.com/Expensify/react-native-live-markdown/pull/439 is not merged yet so the classic counterpart would be to just add the following change in parseExpensiMarkToRanges in parser/index.ts?

function parseExpensiMarkToRanges(markdown: string): MarkdownRange[] {
  if (markdown.length > 5000) {
    return []; // skip formatting for long messages
  }
  // rest of existing code
}

@dominictb What do you think?