Conversation
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
@rinej Completed testing in iPhone 15 pro - 18.6.2, iPhone 17 Pro Max, iOS 26.3 and iPhone 16 Pro, No issues observed |
|
@ikevin127 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] |
melvin-bot
bot
requested review from
trjExpensify
and removed request for
a team
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.
|
|
@ikevin127 are you able to review this one? |
|
Yes, will do! |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-hybrid.mp4Android: mWeb Chromeandroid-mweb.mp4iOS: HybridAppios-hybrid.MP4iOS: mWeb Safariios-mweb.MP4 |
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
(3) Code-review related comments to be addressed before merging. Moving forward to manual testing ♻️
| RNFetchBlob.fs | ||
| .stat(filePath) | ||
| .then(({size}) => { | ||
| resolve({ | ||
| uri, | ||
| width: 0, | ||
| height: 0, | ||
| size, | ||
| type: options.type || 'image/jpeg', | ||
| name: options.name || 'fileName.jpg', | ||
| }); | ||
| }) | ||
| .catch(reject); |
There was a problem hiding this comment.
🔴 CRITICAL - Layout Breakage: Returning 0 for Image Dimensions
Issue: When the fallback path is triggered, we are mocking the file response and hardcoding width: 0, height: 0.
Why it matters: In React Native, UI components dependent on dynamic aspect-ratio layout (like image bubbles in chat or attachments) often compute spacing via aspectRatio: width / height. Passing a 0 will explicitly cause NaN (divide-by-zero) propagation, which notoriously causes Invariant Violation crashes in React Native, or results in a completely invisible 0x0 area in the UI.
Bug Example: If a 48MP manipulation fails, the chat screen processes a file with 0x0 size. The chat renders a collapsed attachment, or the app throws a fatal layout exception.
Suggested Fix: You must extract the actual image dimensions of the original image before resolving the fallback.
import {Image} from 'react-native';| RNFetchBlob.fs | |
| .stat(filePath) | |
| .then(({size}) => { | |
| resolve({ | |
| uri, | |
| width: 0, | |
| height: 0, | |
| size, | |
| type: options.type || 'image/jpeg', | |
| name: options.name || 'fileName.jpg', | |
| }); | |
| }) | |
| .catch(reject); | |
| // Retrieve actual size to prevent divide-by-zero layout crashes | |
| Image.getSize(uri, (width, height) => { | |
| RNFetchBlob.fs.stat(filePath).then(({size}) => { | |
| resolve({ | |
| uri, | |
| width, | |
| height, | |
| size, | |
| type: options.type || 'image/jpeg', | |
| name: options.name || 'fileName.jpg', | |
| }); | |
| }).catch(reject); | |
| }, (getSizeError) => { | |
| // If we can't even get dimensions, we must reject | |
| reject(getSizeError); | |
| }); |
| }) | ||
| .catch(reject); | ||
| }) | ||
| .catch((error) => { |
There was a problem hiding this comment.
🟠 Cross-Platform Compatibility & Android Side-Effects - Unintended Fallback on Android
Issue: As noted in the issue affected devices and PR description - this logic fix targets an issue existent on iOS mWeb / Native. However, we modified .native.ts files, which executes on both Android and iOS.
Why it matters: If an Android device fails to manipulate an image (e.g., encountering an OutOfMemoryError typical of lower-end Android devices loading large bit-maps), this .catch() block will now swallow the error and pass the massive original image forward.
Uploading heavily uncompressed local images on Android will cause immediate "Network Request Failed" errors due to payload size limits, or further OOM crashes in the network layer.
Suggested Fix: Isolate this fallback logic platform-specifically, using platform specific files as per our guidelines. Pseudocode:
import {Platform} from 'react-native';
// Inside the catch block...
.catch((error) => {
if (Platform.OS === 'android') {
Log.warn('Image manipulation failed on Android', {error: error instanceof Error ? error.message : String(error)});
// Allow Android to fail fast and maintain its current lifecycle
return reject(error);
}
// Proceed with iOS fallback logic...
})
src/hooks/useFilesValidation.tsx
Outdated
| @@ -147,11 +147,12 @@ function useFilesValidation(onFilesValidated: (files: FileObject[], dataTransfer | |||
| }; | |||
|
|
|||
| const convertHeicImageToJpegPromise = (file: FileObject): Promise<FileObject> => { | |||
There was a problem hiding this comment.
🟡 Type Consistency & Clean Code Principles - Function Contract Violation
Issue: The function is named convertHeicImageToJpegPromise, but the modified .catch block now resolves resolve(originalFile).
Why this matters: The caller explicitly expects a JPEG if the Promise resolves. By resolving a .heic file, you permit HEIC files into the app feed pipeline. If Expensify Web users open this chat in Google Chrome, the HEIC attachment will appear completely broken because most web browsers do not natively support decoding HEIC.
Suggested Fix: If we accept that HEICs might be sent un-converted, you must rename the function to something that reflects reality, like ensureProcessableImagePromise.
ℹ️ Additionally, communicate with the team whether the web Client / Backend actually parses non-converted HEICs. Failing the conversion and showing an alert format might actually be better than polluting the timeline with an unreadable web attachment.
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
✅ Completed PR Reviewer Checklist, manual testing on real device looks good. Other than the ☝️ 3 comments / concerns found during code-review, LGTM 🟢
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #81702 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Yikes, that's a nasty bug in a core flow! 👍
|
@ikevin127 thanks for the review! |
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
Thanks for applying the changes ✅
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Noting one thing as a follow up but thank you, I will move it ahead so this is fixed asap
| if (Platform.OS !== 'ios') { | ||
| reject(error); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Could you split this up to ios and android files to remove this check?
There was a problem hiding this comment.
should I create new PR with that quick fix?
|
🚧 @mountiny 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/mountiny in version: 9.3.40-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.40-0 🚀
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.41-4 🚀
|
6 participants
Explanation of Change
Fixed context loss during HEIC image processing by adding error handling and fallback conversion. When HEIC conversion fails, the image is processed using a fallback method, preventing crashes and improving logging for debugging.
Based on the previous Draft PR -> #83675
Fixed Issues
$ #81702
PROPOSAL:
Tests
Offline tests
QA Steps
Prerequisites:
Test steps:
Additional test for picking from browser:
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