Repository navigation
Keep the preview uri when resizing large image attachments on web - #102852
Conversation
Resized attachments over 24 MiB lost their blob uri, so the attachment preview had no source and showed an endless spinner. Co-authored-by: Qichen Zhu <QichenZhu@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
|
🤖 Web test steps verification results: I reached a DM chat and opened the Add attachment menu, but the web test tool can't select files in the browser's file picker. No image over 24 MiB could be attached, so the resize and preview fix (steps 2–4) wasn't tested, and the tool can't read the JS console.
|
| const file = new File(['content'], 'image.jpeg', {type: 'image/jpeg'}); | ||
| file.uri = 'blob:http://localhost/image'; | ||
|
|
||
| // When the file is cloned |
There was a problem hiding this comment.
@MelvinBot, please remove comments that don't add value like this.
There was a problem hiding this comment.
Done in d7af11b. I removed the 5 comments in the new tests that only restated the next line: both "When the file is cloned" comments, "When the image is resized", and the native test's Given/Then comments.
I kept the comments that explain why, for example that previews read uri to show the image. FileUtilsTest.ts passes (44/44) and ESLint is clean.
Co-authored-by: Qichen Zhu <QichenZhu@users.noreply.github.com>
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-native.movAndroid: mWeb Chromeandroid-web.moviOS: HybridAppios-native.moviOS: mWeb Safariios-web.movMacOS: Chrome / Safarimac-web.mov |
QichenZhu
left a comment
There was a problem hiding this comment.
LGTM.
MelvinBot doesn't provide test steps. My suggestion is:
Precondition: Have an image larger than 24 MiB in size and less than 50 megapixels in resolution.
Steps:
- Navigate to a DM.
- Click + > Add attachment.
- Select the image and upload it.
- Verify that the file uploads successfully. The test fails if the screen loads indefinitely.
|
@MelvinBot mark this PR ready for review |
|
I marked this PR ready for review. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@QichenZhu Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
I already reviewed. |
|
@grgia, this is ready for your second review. |
|
🚧 grgia has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/grgia in version: 9.5.6-0 🚀
|
|
No help site update needed. This bug fix makes large image previews on web load as users expect instead of spinning forever, so no article sentence is now wrong. |
|
Any QA steps here? |
|
@IuliiaHerets QA steps:
Thanks! |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.5.6-6 🚀
Bundle Size Analysis (Sentry): |





Explanation of Change
On web, images over 24 MiB are resized before the attachment preview opens.
resizeImageIfNeededpasses the resized file throughcreateFile, whose web branch clones it withnew File(...). TheFileconstructor doesn't copy the customuriproperty, so the resized file lost its bloburi. The preview then rendered an image with an empty source, which never firesonLoad, so the spinner never went away.This PR fixes the root cause in
createFile: the web branch now copiesurionto the clonedFile, matching the native branch, which already keepsuri. Every other step that creates a web attachment setsuri, socreateFilewas the only place that dropped it. Fixing it there also covers the other caller ofresizeImageIfNeeded(the "add additional receipt" flow) and the workspace-avatar callers inPolicy.ts, where the extra property is harmless.I also added unit tests for
createFile(web and native) and forresizeImageIfNeededon web with an image over the max size. The new web tests fail without the fix and pass with it.Fixed Issues
$ #102470
PROPOSAL: #102470 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Offline tests
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari