regen-network / rnd-dev-team

RND Dev Team issue management (DEPRECATED)
0 stars 0 forks source link

Issues with uploading large images for user profile #1683

Closed erikalogie closed 1 year ago

erikalogie commented 1 year ago

Describe the bug From Sari: "Hey guys! I believe I found an issue with updating user profiles. It seems like the size of the photo plays a role. If I attempt to upload a larger photo, it will either crash before allowing me size and crop the photo, or it will stall after I click apply after cropping my photo and then crash. Then a generic error message appears in an orange bar at the top. After a few failed tries, I sized my photo down from 2K+ pixels in width to 1200 and it saved without any issue. I was able to recreate this twice with two different user profiles on separate browsers."

To Reproduce

  1. Go to user profile edit form
  2. Add a large image (wider than 2k pixels)
  3. Expect saving to fail

Expected behavior Saving large images should work as expected.

Screenshots

Desktop (please complete the following information):

Smartphone (please complete the following information):

Additional context

worked.jpeg

did not work.jpeg____

For Admin Use

blushi commented 1 year ago

Hey team! Please add your planning poker estimate with Zenhub @flagrede @wgwz

wgwz commented 1 year ago

In separate issue, I found that POST-ing large images to the registry-server /files endpoint works fine. https://github.com/regen-network/registry-server/issues/327#issuecomment-1544261460

So it seems like the issue with POST-ing large images, only exists from the client-side code in the app.

flagrede commented 1 year ago

@wgwz that's good to know thanks for sharing that.

wgwz commented 1 year ago

@flagrede no problem, that said, the way that the client-side code is interacting with the registry-server, does result in an exception being raised in the registry-server. so somehow whatever negotiation is taking place between client-side and the server is going a little funky. i don't have any other ideas why that could be at the moment.

flagrede commented 1 year ago

I was unable to reproduce this issue so far in all env (local/staging/prod). I even try to throttle my connection speed to 3G but same result.

wgwz commented 1 year ago

Hey @flagrede i just took a look at this again in staging and i was able to repro. I'm going to attach the file I was using. Here's a loom as well (not really too much in here but it shows it): https://www.loom.com/share/aa1b933171db41fba976fa2d28d6d3c7

Here's the file I was using: https://github.com/regen-network/regen-registry/assets/10120306/3af854e3-4dad-4c82-8012-babeeb5730ed

wgwz commented 1 year ago

Ah ha! Ok, so I just found something new. It is an issue in the registry-server! I figured out how to reproduce it (TL;DR i was not using a large enough file). When I tested the /files endpoint with 100MiB file, it fails the same way as what we see in the client-side!

This is good news, because it means a code change just in the registry-server and we can easily reproduce now. My best guess for the fix right now is that we should switch over to the newer AWS SDK for s3. Which I've begun using here, as an example: https://github.com/regen-network/registry-server/blob/dev/server/__tests__/utils.ts#L259

I think I'll take this task and work on it now that I've isolated the issue.

wgwz commented 1 year ago

One thing I wanted to note @flagrede is that in the profile image cropping tool, while I was doing some debugging, I realized that after cropping, even though you are selecting a subset of the image, that cropping operation can actually result in a larger output image file than the original file that was input. This was a surprise to me! I wanted to mention it to you in case it's something we can optimize, or at least just make you aware of it (if you weren't already).

flagrede commented 1 year ago

@wgwz thanks for pointing that out. The current cropping logic is quite complex and hard to modify but from what I saw it creates a new canvas to produce the new image so there must be something wrong in the computation here if we end up with a bigger resolution. I would advise refactoring it entirely if we stick with client-side cropping to solve this issue and make it easier to modify. /cc @blushi