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

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

Open
1 of 6 tasks
lanitochka17 opened this issue Aug 3, 2024 · 74 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Aug 3, 2024

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: 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?

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6560837_1722668432456.Screenrecorder-2024-08-03-12-23-52-882_compress_1.mp4

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 OwnerCurrent Issue Owner: @c3024
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 3, 2024
Copy link

melvin-bot bot commented Aug 3, 2024

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
Copy link
Author

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

@lanitochka17
Copy link
Author

@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
Copy link
Author

We think that this bug might be related to
#wave-control

@melvin-bot melvin-bot bot added the Overdue label Aug 5, 2024
Copy link

melvin-bot bot commented Aug 6, 2024

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

Copy link

melvin-bot bot commented Aug 8, 2024

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

@kevinksullivan
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label Aug 9, 2024
@kevinksullivan kevinksullivan added External Added to denote the issue can be worked on by a contributor Overdue labels Aug 9, 2024
@melvin-bot melvin-bot bot changed the title Android - Chat - Trying to paste onyx data of #applause.expensifail.com chat app freezes [$250] Android - Chat - Trying to paste onyx data of #applause.expensifail.com chat app freezes Aug 9, 2024
Copy link

melvin-bot bot commented Aug 9, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 9, 2024
Copy link

melvin-bot bot commented Aug 9, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Aug 9, 2024
@kevinksullivan
Copy link
Contributor

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

@c3024
Copy link
Contributor

c3024 commented Aug 10, 2024

@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
Copy link
Author

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

@melvin-bot melvin-bot bot added the Overdue label Aug 12, 2024
@c3024
Copy link
Contributor

c3024 commented Aug 12, 2024

Waiting for proposals

@melvin-bot melvin-bot bot removed the Overdue label Aug 12, 2024
Copy link

melvin-bot bot commented Aug 15, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Aug 15, 2024
@c3024
Copy link
Contributor

c3024 commented Aug 16, 2024

Waiting for proposals!

@melvin-bot melvin-bot bot removed the Overdue label Aug 16, 2024
@dominictb
Copy link
Contributor

Expensify/react-native-live-markdown#504 @tomekzaw live MD PR is ready for review.

Copy link

melvin-bot bot commented Sep 25, 2024

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

@Julesssss
Copy link
Contributor

PR raised

Copy link

melvin-bot bot commented Oct 3, 2024

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

Copy link

melvin-bot bot commented Oct 7, 2024

@Julesssss, @sonialiap, @c3024, @dominictb Still overdue 6 days?! Let's take care of this!

@dominictb
Copy link
Contributor

Expensify/react-native-live-markdown#505 is merged. Will open PR on E/App by today.

@dominictb
Copy link
Contributor

#49228 solution updated here. Will update the evidence and open the PR later.

@dominictb
Copy link
Contributor

dominictb commented Oct 9, 2024

Update: The solution works on Web and Desktop, on iOS (mWeb/native) somehow it's really slow (my simulator) - it seems like coming from the react-native-live-markdown (checked this on the react-native-live-markdown sample app and it's really slow as well. This never happens before when I worked on the library PR). I'll need to check on Android to confirm this, and will update here once I have more info.

Copy link

melvin-bot bot commented Oct 9, 2024

@Julesssss, @sonialiap, @c3024, @dominictb 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

@c3024
Copy link
Contributor

c3024 commented Oct 9, 2024

App PR is about to be finalized, Melvin!

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Oct 10, 2024
@dominictb
Copy link
Contributor

@tomekzaw sorry for disturbing you, but after applying the maxLength fix we still got freeze on iOS mWeb and iOS native. After tinkering around the example app in the react-native-live-markdown repo it seems to me that max text length that the parser could parse without getting freeze on iOS is around 3000 characters, not 10000 characters. So:

  • Have we ever had any official benchmark of the parser performance with large text in various platforms & machine (iOS, Android, Desktop,...)?
  • Actually the parser performs significantly worse on simulator(iPhone/iPad) Safari and native, but it works pretty well (with 10000-length text) in MacOS Safari/Chrome or even Android (Chrome/native). I came across this issue Speed up ExpensiMark on Hermes react-native-live-markdown#105, does it related to this situation?

Thanks a lot! cc @c3024

@dominictb
Copy link
Contributor

@tomekzaw actually, what I observe further: We are re-parsing the entire input on every keystroke, and that's expensive operation given that the current input text is long (around 5000 characters). I tested on MacOS Chrome and couldn't even fast typing around that length.

@tomekzaw
Copy link
Contributor

tomekzaw commented Oct 22, 2024

@dominictb That's true, there are some performance issues when typing long messages. Not sure if it's caused by ExpensiMark.js algorithm or platform-specific formatting implmementation. One way would be to re-apply spans only if any change is detected. This something we'd like to address sometime in the future.

@dominictb
Copy link
Contributor

@tomekzaw @Julesssss @c3024 I'm a bit in a dilemma of the final solution here. The current solution is to limit

The situation is like this: on Desktop Chrome/Safari, it's a bit laggy when we fast typing, and on Android/iOS (web/native), the app lags but not freezes (by definition the issue is solved), but the experience is not smooth.

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-11-01.at.16.40.32.mp4

Ideally, we should address #46766 (comment). One workaround is to set another limit on live markdown's parser to less than 5000 (or even less). This basically means: if you paste the text length > 5000, the parser doesn't parse, i.e the app will be smooth. But if the text length is 4900-ish, the app will lag a bit as now the parser will parse the whole text.

Please advise!

@Julesssss
Copy link
Contributor

Julesssss commented Nov 1, 2024

Hey @dominictb, yeah it's tough because it seems there's no magic number at which performance is stable on all platforms.

I liked the original proposal and I would be happy to do something like this:

  • Complete the original plan, using a reduced limit of 5000 (or perhaps lower)
  • Then decide if we want to request improvements to react-native-live-markdown/Expensimark

By the way I notice the live-markdown is being reverted back to 0.1.164 (while my fix is introduced in 0.1.168

@dominictb it looks like release version is back up to 0.1.179

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Nov 4, 2024
Copy link

melvin-bot bot commented Nov 4, 2024

This issue has not been updated in over 15 days. @Julesssss, @sonialiap, @c3024, @dominictb eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@dominictb
Copy link
Contributor

@dominictb it looks like release version is back up to 0.1.179

Thanks @Julesssss I have noted that and updated my branch

Complete the original plan, using a reduced limit of 5000 (or perhaps lower)

@tomekzaw given that the Expensify/react-native-live-markdown#439 is nowhere near to merged soon, could I update the parser in the react-native-live-markdown directly, i.e: do not attempt to parse if the text length is less than 5000? I don't think the maxLength could be lower than 10000?

@tomekzaw
Copy link
Contributor

tomekzaw commented Nov 6, 2024

@dominictb

given that the Expensify/react-native-live-markdown#439 is nowhere near to merged soon

Here's the most recent update on #439: #50949 (comment)

could I update the parser in the react-native-live-markdown directly

Okay, let's do it.

@Julesssss
Copy link
Contributor

Sounds good. @c3024 are you happy with the plan?

@c3024
Copy link
Contributor

c3024 commented Nov 6, 2024

Yes, that sounds good!

@dominictb
Copy link
Contributor

PR: Expensify/react-native-live-markdown#542

@tomekzaw
Copy link
Contributor

PR Expensify/react-native-live-markdown#542 has been merged.

@dominictb
Copy link
Contributor

on my way to update the the E/App PR now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

7 participants