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

[$250] iOS - App crashes when pasting large text to the composer and backgrounding the app #51059

Closed
1 of 8 tasks
IuliiaHerets opened this issue Oct 17, 2024 · 57 comments
Closed
1 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Oct 17, 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.0.50-5
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y
Issue was found when executing this PR: #50487
Issue reported by: Applause Internal Team

Action Performed:

  1. Navigate to https://www.lipsum.com/
  2. Generate text with 10 000 words
  3. Copy the generated text and the title and quotes under in at the top of the page
  4. Open the app
  5. Log in with a Gmail account
  6. Open any 1:1 DM
  7. Long tap inside the composer
  8. Tap on "Paste"
  9. Background the app
  10. Foreground the app
  11. Try to interact with the app
  12. Repeat steps 9-11 until it crashes

Expected Result:

If the pasted text is longer than 10,000 characters, we should trim the text to 10,000 characters.

We can use a similar pattern to how we indicate the character limit in the Workspace name input field (but it would say Character limit reached (10,000/10,000) instead, or something similar)

Actual Result:

When pasting large text to the composer and backgrounding the app a few times, we attempt to paste the full text string, and the app crashes.

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6638032_1729189609126.Pmqw0322.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021859632203774875578
  • Upwork Job ID: 1859632203774875578
  • Last Price Increase: 2024-12-05
  • Automatic offers:
    • wildan-m | Contributor | 105221106
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 17, 2024
Copy link

melvin-bot bot commented Oct 17, 2024

Triggered auto assignment to @sakluger (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.

@IuliiaHerets
Copy link
Author

@sakluger 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

@sakluger
Copy link
Contributor

sakluger commented Oct 17, 2024

Is this a separate bug from #48888? Or is it a regression from that issue's PR?

If we don't allow users to send messages over 10k characters, why would we allow them to paste over 10k characters in the composer? Why don't we just cut off everything over 10k?

@sakluger
Copy link
Contributor

cc @anmurali @jasperhuangg @Pujan92 curious for your thoughts on the expected behavior here since you all were involved in #48888.

Copy link

melvin-bot bot commented Oct 21, 2024

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

@sakluger
Copy link
Contributor

I posted to Slack (https://expensify.slack.com/archives/C05LX9D6E07/p1729615084337139) asking for feedback on the expected behavior.

@melvin-bot melvin-bot bot removed the Overdue label Oct 22, 2024
@Pujan92
Copy link
Contributor

Pujan92 commented Oct 22, 2024

@sakluger I can't reproduce the crash. Also, this isn't a regression from the other issue.

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-10-22.at.22.33.43.mp4

@sakluger
Copy link
Contributor

This feels like a major edge case, especially given that @Pujan92 can't reproduce at all.

I'm going to close the issue for now. @IuliiaHerets - if you're still able to reproduce it, feel free to reopen. At that point, if we can consistently reproduce, then we'll likely change the behavior to auto-clip the text to 10k max characters.

@lanitochka17
Copy link

Issue is still reproducible on the latest build 9.0.63-3

Crash.long.text.mp4

@lanitochka17 lanitochka17 reopened this Nov 18, 2024
@sakluger sakluger moved this from Done to LOW in [#whatsnext] #quality Nov 19, 2024
@sakluger
Copy link
Contributor

@lanitochka17 thanks for sharing the new video. Just to confirm, did you use the exact same reproduction steps, or did you modify the steps in any way?

@sakluger sakluger changed the title iOS - It crashes when pasting large text to the composer and backgrounding the app iOS - App crashes when pasting large text to the composer and backgrounding the app Nov 21, 2024
@sakluger
Copy link
Contributor

I updated the expected behavior in the OP to indicate that we should trim to 10,000 characters if the user tries pasting more than that.

@sakluger sakluger added the External Added to denote the issue can be worked on by a contributor label Nov 21, 2024
@melvin-bot melvin-bot bot changed the title iOS - App crashes when pasting large text to the composer and backgrounding the app [$250] iOS - App crashes when pasting large text to the composer and backgrounding the app Nov 21, 2024
Copy link

melvin-bot bot commented Nov 21, 2024

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

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

melvin-bot bot commented Nov 21, 2024

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

@wildan-m
Copy link
Contributor

wildan-m commented Dec 12, 2024

@sakluger @techievivek @allroundexperts what is the expectation if the composer already have text? we should trim combined text, right? not only the text from clipboard.

I couldn't find a solution without blinking in native, will this blinking acceptable?

Kapture.2024-12-12.at.16.19.40.mp4

Another thing to consider is -- we would trim the text value before markdown parsing right?
I'm asking this because each character / pattern will have character length that different with what actually visible.

For instance new line will have 6 character
Emoji can have more than two character
site url can have meta character more than 20 character, because it will be wrapped by anchor tag <a href ....

If we trim parsed value, then the performance might still slow because we'll need to parse it before we trim, also we can't predict if the trimmed text broke the parsed tag or not

i.e.

https://www.google.com

might be parsed as

<a href ="https://www.google.com" />

what if the trimmed result stop at <a href = ? I think that would create unexpected result.

If we agree with my suggestion (to trim text before parsed), then the length will not exactly 10.000 it can be more than it especially if the pasted text contain special pattern (emoji, new line, link, etc)

@sakluger
Copy link
Contributor

what is the expectation if the composer already have text? we should trim combined text, right? not only the text from clipboard.

Yes, we should check the combined total and trim to 10k total characters.

I couldn't find a solution without blinking in native, will this blinking acceptable?

I didn't really notice the blinking, I think it's fine. @techievivek what do you think?

Another thing to consider is -- we would trim the text value before markdown parsing right?

I'll defer to @techievivek and @allroundexperts for this question.

@melvin-bot melvin-bot bot added the Overdue label Dec 16, 2024
Copy link

melvin-bot bot commented Dec 16, 2024

@sakluger, @wildan-m, @allroundexperts, @techievivek Whoops! This issue is 2 days overdue. Let's get this updated quick!

@allroundexperts
Copy link
Contributor

Another thing to consider is -- we would trim the text value before markdown parsing right?

I personally think that we shouldn't trim it, but @techievivek will have the final call.

@melvin-bot melvin-bot bot removed the Overdue label Dec 16, 2024
@sakluger
Copy link
Contributor

Sounds good, let's see what @techievivek says about trimming before parsing.

@techievivek
Copy link
Contributor

Yes, we should check the combined total and trim to 10k total characters.

+1️⃣

I didn't really notice the blinking, I think it's fine. @techievivek what do you think?

Yeah, it is acceptable for now.

I personally think that we shouldn't trim it, but @techievivek will have the final call.

I suggest trimming it before the markdown parsing to avoid the edge cases you mentioned in your comment. This will help prevent issues like broken links or similar problems. @allroundexperts, do you have any thoughts on why we shouldn't be trimming before? IMO trimming beforehand seems like a safer option.

@allroundexperts
Copy link
Contributor

allroundexperts commented Dec 18, 2024

@allroundexperts, do you have any thoughts on why we shouldn't be trimming before? IMO trimming beforehand seems like a safer option.

It just appears a little weird to me that the character limit count won't change when you'd press the space bar.

@melvin-bot melvin-bot bot added the Overdue label Dec 23, 2024
@techievivek
Copy link
Contributor

Aaah, ok, but I think that's acceptable than handling those edge cases mentioned above.

@melvin-bot melvin-bot bot removed the Overdue label Dec 23, 2024
@techievivek
Copy link
Contributor

@wildan-m I hope you are clear now with the expectations so you can get started with the PR.

Copy link

melvin-bot bot commented Dec 27, 2024

@sakluger, @wildan-m, @allroundexperts, @techievivek Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Dec 27, 2024
@wildan-m
Copy link
Contributor

Limited availability, will continue the PR tomorrow

@melvin-bot melvin-bot bot removed the Overdue label Dec 27, 2024
@wildan-m
Copy link
Contributor

I think this PR #49228 has already resolve the same thing for this issue.

@sakluger sakluger added the retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause label Dec 30, 2024
@melvin-bot melvin-bot bot added the Overdue label Dec 30, 2024
@sakluger
Copy link
Contributor

Good find! That PR has been merged for two weeks, so I'll ask QA for a retest to see if this bug is resolved.

Copy link

melvin-bot bot commented Dec 31, 2024

@sakluger, @wildan-m, @allroundexperts, @techievivek Whoops! This issue is 2 days overdue. Let's get this updated quick!

Copy link

melvin-bot bot commented Jan 2, 2025

@sakluger, @wildan-m, @allroundexperts, @techievivek Eep! 4 days overdue now. Issues have feelings too...

@sakluger
Copy link
Contributor

sakluger commented Jan 2, 2025

I asked for the retest here: https://expensify.slack.com/archives/C9YU7BX5M/p1735839601691729

@mvtglobally
Copy link

Issue is not reproducible

0-02-01-91e21b572656558f6e2bc4dbff02176716bd247045ad7f3b71e8e53e7e0b7790_277892e65c8b7f2.mp4

@sakluger
Copy link
Contributor

sakluger commented Jan 2, 2025

Nice, sounds like this is fixed! I'm a little unclear on how much work was completed for this issue before we discovered that another PR fixed it already. @allroundexperts is any payment due for this issue?

@melvin-bot melvin-bot bot removed the Overdue label Jan 2, 2025
@sakluger
Copy link
Contributor

sakluger commented Jan 3, 2025

I am pretty sure no payment is due, so I'm going to close the issue. @allroundexperts - if you disagree, please raise in Slack (since I may not see comments on this GH issue after it is closed).

@sakluger sakluger closed this as completed Jan 3, 2025
@github-project-automation github-project-automation bot moved this from LOW to Done in [#whatsnext] #quality Jan 3, 2025
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. Daily KSv2 External Added to denote the issue can be worked on by a contributor retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause
Projects
Development

No branches or pull requests

8 participants