feat: Add thumbnail support for receipts and improve image handling#84919
Conversation
src/pages/iou/request/step/IOURequestStepScan/cropImageToAspectRatio.ts
Outdated
Show resolved
Hide resolved
src/pages/iou/request/step/IOURequestStepScan/cropImageToAspectRatio.ts
Outdated
Show resolved
Hide resolved
| * Used on native to avoid decoding the full 12MP camera photo on the confirmation page. | ||
| */ | ||
| function generateThumbnail(sourceUri: string, maxWidth = 512): Promise<string | undefined> { | ||
| return ImageManipulator.manipulate(sourceUri) |
There was a problem hiding this comment.
We definitely should check this on Android and iOS both. I think I saw on my crappy Galaxy S8 that the image preview on confirmation page takes about 1 second to load without any resizing.
But, also I think we can control the overall file size by tweaking the vision camera settings e.g. if we use takeSnapshot() instead of takePhoto() then maybe this thumbnail generation is not needed? Or the tradeoff is less? Hard to say! Hacking over here.
…to pregenerate-thumbnails-for-the-confirm-screen
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
…ath. This change improves performance by decoupling thumbnail generation from the main receipt processing flow, ensuring that the UI remains responsive during photo capture. Updated logic for setting receipt files and handling editing state accordingly.
src/pages/iou/request/step/IOURequestStepScan/cropImageToAspectRatio.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c74437146
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
PR doesn’t need product input as a performance PR. Unassigning and unsubscribing myself. |
…to pregenerate-thumbnails-for-the-confirm-screen
|
@kubabutkiewicz can you resolve the comments about inconsistency? Do you think we could add some unit tests here? Thanks. |
…to pregenerate-thumbnails-for-the-confirm-screen
This comment has been minimized.
This comment has been minimized.
|
@brunovjk @Julesssss |
|
Maybe we can create some better UX here, like instead of showing blank preview, show some blured template image and replace it with the generated thumbnail when its ready |
Yeah even though the perf improvement is good, the flash isn't great. Perhaps a slight fade in animation or placeholder. |
…to pregenerate-thumbnails-for-the-confirm-screen
…mation screen. Update `useLocalReceiptThumbnail` to manage thumbnail caching and references, preventing source swaps. Adjust `MoneyRequestConfirmationListFooter` to utilize cached thumbnails effectively. Ensure seamless integration in `IOURequestStepScan` for enhanced user experience.
|
@brunovjk @Julesssss I investigated it a bit and I found the solution which wont cause flickering nor the need of introduction of loader/animation |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp84919_android_native.movAndroid: mWeb Chrome84919_web_native.moviOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariN/A |
|
@Julesssss can you help me test it on iOS? I don't have a physical iOS device, and I couldn't unlock the camera in the simulator. Thanks a lot. |
|
🚧 @Julesssss has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
Yeah I can do this tomorrow |
|
Ah @kubabutkiewicz could you merge main tomorrow and resolve conflicts too, thanks |
…to pregenerate-thumbnails-for-the-confirm-screen
|
@Julesssss on it! |
…width to 256px for faster decoding. Implement prefetching of thumbnails to eliminate latency during display. Update IOURequestStepScan to handle thumbnail generation and navigation more efficiently.
…to pregenerate-thumbnails-for-the-confirm-screen
…to pregenerate-thumbnails-for-the-confirm-screen
Oh no another conflict, sorry! |
…to pregenerate-thumbnails-for-the-confirm-screen
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
iOS tested well for me. I noticed one issue that I raised as a separate bug. |
|
🚧 @Julesssss has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
Explanation of Change
Fixed Issues
$#83503
PROPOSAL:
Tests
Offline tests
N/A
QA Steps
Same as above
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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