Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
2.99k stars 2.5k forks source link

[$500] No feedback and delay when uploading a large attachment #38478

Open m-natarajan opened 2 months ago

m-natarajan commented 2 months ago

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: Reproducible in staging?: needs reproduction Reproducible in production?: needs reproduction 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: @neil-marcellini Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1710524006372329

Action Performed:

  1. Use iOS on a slow network connection
  2. Go to any chat
  3. Click the plus button
  4. Add an attachment
  5. Select a video larger than the current limit of 24mb

Expected Result:

"Attachment too large" popup shows immediately

Actual Result:

Nothing happens for quite a while, then you receive a popover explaining that the attachment is too large

Workaround:

unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/38435837/1eb76771-48a3-4f74-8e04-2833e052d922

https://github.com/Expensify/App/assets/26260477/aa4196a1-9d32-4706-94d6-b7480efaed94

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01398c6a10dbc0b325
  • Upwork Job ID: 1770333731300958208
  • Last Price Increase: 2024-03-27
  • Automatic offers:
    • hoangzinh | Reviewer | 0
    • ShridharGoel | Contributor | 0
melvin-bot[bot] commented 2 months ago

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

ShridharGoel commented 2 months ago

Proposal

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

When uploading a large attachment there is a delay on iOS.

What is the root cause of that problem?

Image picker library is taking time to fetch the result on IOS.

To test it out, I had added logs to check time inside imageLibrary method in the native.ts file of the library.

The time difference was coming between call to launchImageLibrary and getting the result.

Github issue for some discussion on the same: https://github.com/react-native-image-picker/react-native-image-picker/issues/2117

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

Add assetRepresentationMode as current.

const imagePickerOptions = {
    includeBase64: false,
    saveToPhotos: false,
    selectionLimit: 1,
    includeExtra: false,
+  assetRepresentationMode: 'current'
};
melvin-bot[bot] commented 2 months ago

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

Christinadobrzyn commented 2 months ago

I can reproduce this. Here's the GH about increasing the size from 24 MB - https://github.com/Expensify/App/issues/32702

I think this can be external

melvin-bot[bot] commented 2 months ago

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

ShridharGoel commented 2 months ago

Proposal

Updated

ShridharGoel commented 1 month ago

@hoangzinh Kindly have a look at my proposal when possible.

hoangzinh commented 1 month ago

Hi @ShridharGoel I don't think you have a correct RCA. When I put console log in that isValidFile function, it already had size value, so it just read from the object data. Screenshot 2024-03-21 at 21 35 17

ShridharGoel commented 1 month ago

@hoangzinh If you are using a Simulator, are you able to replicate the issue?

hoangzinh commented 1 month ago

ah nope I can't. Is it only reproducible in a real iphone?

ShridharGoel commented 1 month ago

@hoangzinh It seems like that. I've analysed the issue more - it's related to the image picker taking time. I'll update my proposal.

ShridharGoel commented 1 month ago

Proposal

Updated

hoangzinh commented 1 month ago

@Christinadobrzyn @ShridharGoel what is the file size you used to test? I used a video file with 128MB but it shows up file size error almost immediately

ShridharGoel commented 1 month ago

@hoangzinh Do you have access to a physical iPhone to check?

hoangzinh commented 1 month ago

Yes I do @ShridharGoel

ShridharGoel commented 1 month ago

@hoangzinh Can you check with a 500 MB video maybe?

@situchan Do you have an iPhone? If possible, can you check if you are able to replicate this?

situchan commented 1 month ago

@ShridharGoel I can test on my iPhone. Can you attach or share link to video file you tested?

ShridharGoel commented 1 month ago

@situchan Can you try this? Link

situchan commented 1 month ago

I am seeing "Attachment too large" modal immediately

Christinadobrzyn commented 1 month ago

testing this again, I am also seeing the "attachment too large" prompt immediately. So I think this is resolved but I'm just double-checking with @neil-marcellini to make sure this is what is expected.

https://github.com/Expensify/App/assets/51066321/768cc2a8-1c6c-439c-bb4b-681232866077

neil-marcellini commented 1 month ago

@Christinadobrzyn that looks like a slightly different flow you tested. I'm consistently able to reproduce with this video, regardless of network speed. I'm on iOS 17.3.1, New Expensify v1.4.50-5

https://github.com/Expensify/App/assets/26260477/aa4196a1-9d32-4706-94d6-b7480efaed94

ShridharGoel commented 1 month ago

@hoangzinh Issue can be seen every time using the above video. Also, I've checked that my proposal fixes it, can you have a look?

Christinadobrzyn commented 1 month ago

Thanks @neil-marcellini! I can also reproduce with the above video - I added it to the OP so we can use it for testing.

hoangzinh commented 1 month ago

@ShridharGoel sure it's on my list today. I will post updates soon

hoangzinh commented 1 month ago

Hi @ShridharGoel with this video https://github.com/Expensify/App/issues/38478#issuecomment-2017471404. How long does it take to show the modal on your phone?

ShridharGoel commented 1 month ago

@hoangzinh 15-20 seconds approx (Using iOS 17.2 simulator)

https://github.com/Expensify/App/assets/35566748/53dc448b-458e-4c8f-bfa9-30d35ec57102

ShridharGoel commented 1 month ago

It becomes almost instant after adding assetRepresentationMode: 'current'.

https://github.com/Expensify/App/assets/35566748/d7d01753-1389-42f1-b2a8-fffbea8007e8

hoangzinh commented 1 month ago

Thanks @ShridharGoel I can reproduce this bug now. One of the key step is we have to select from Gallery instead of document

hoangzinh commented 1 month ago

A pending attachment view is immediately inserted into the text input box where the cursor is. Then if the attachment is too large the pending view is replaced with an attachment error view/icon and a dismissible error message. Also, the attachment size limit should be way larger. I know we are already working on this.

Hi @Christinadobrzyn can we change the expected behavior for this issue? I mean can we change it to the "Attachment too large" modal shows up immediately?

Christinadobrzyn commented 1 month ago

Yep! Updated the Expected Result in the OP @hoangzinh!

hoangzinh commented 1 month ago

Thanks @Christinadobrzyn

hoangzinh commented 1 month ago

@ShridharGoel's proposal https://github.com/Expensify/App/issues/38478#issuecomment-2004567438 looks good to me.

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @tylerkaraszewski, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] commented 1 month ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

Christinadobrzyn commented 1 month ago

Just a heads up that I'm going to be ooo on Friday and Monday for Easter. I'll check in on this on Tuesday when I'm back.

ShridharGoel commented 1 month ago

Can this be assigned to me so that I can open a PR ?

hoangzinh commented 1 month ago

@ShridharGoel we would wait @tylerkaraszewski for final review

melvin-bot[bot] commented 1 month ago

πŸ“£ @hoangzinh πŸŽ‰ 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] commented 1 month ago

πŸ“£ @ShridharGoel πŸŽ‰ 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 πŸ“–

melvin-bot[bot] commented 1 month ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

AndrewGable commented 1 month ago

FYI the PR was reverted for regression/deploy blocker linked above. Please make sure to address the bug in a future PR.

hoangzinh commented 3 weeks ago

cc @ShridharGoel let's work on this issue again.

ShridharGoel commented 3 weeks ago

Sure, will check this again. We made changes to imagePickerOptions, it shouldn't have affected other attachment types.

Christinadobrzyn commented 2 weeks ago

hi @hoangzinh and @ShridharGoel - can you provide an update on this?

ShridharGoel commented 2 weeks ago

Will check this over the weekend.

On Fri, May 3, 2024, 3:43 PM Christina Dobrzynski @.***> wrote:

hi @hoangzinh https://github.com/hoangzinh and @ShridharGoel https://github.com/ShridharGoel - can you provide an update on this?

β€” Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/38478#issuecomment-2092708609, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIPLJHB5ORTPIKFIXHQLTD3ZANPM3AVCNFSM6AAAAABE3ZUSZ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJSG4YDQNRQHE . You are receiving this because you were mentioned.Message ID: @.***>

ShridharGoel commented 2 weeks ago

@AndrewGable @hoangzinh This change doesn't seem to cause any issue with any attachment types. I've verified this on iOS native and uploading text files and PDFs was working with the changes in #39334.

@AndrewGable Was it happening for you before the revert?

AndrewGable commented 1 week ago

As noted here: https://github.com/Expensify/App/issues/40492#issue-2251359045

It was not happening on production but it was on staging, it didn't happen after the revert.

ShridharGoel commented 1 week ago

@AndrewGable Was it happening on staging for you?

AndrewGable commented 1 week ago

Yes it was before the revert

ShridharGoel commented 1 week ago

Is it possible that something else also got reverted when you tested after reverting these changes? Asking because on adding the exact code on main, it's working fine for me.

@hoangzinh Can you also test?