Migrate BaseImage web component to expo-image#83325
Conversation
|
|
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Note: While testing I've found side issue: #78859 (comment), should be fixed in this PR |
|
@shubham1206agra @mountiny One of you needs to 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 561676171c
ℹ️ 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".
| useEffect(() => { | ||
| // expo-image doesn't support onLoadStart on web, so we call it manually when the source changes, matching react-native-web's Image behavior | ||
| onLoadStart?.(); | ||
| }, [source, onLoadStart]); |
There was a problem hiding this comment.
Stop retriggering onLoadStart on every render
Calling onLoadStart from a useEffect that depends on both source and onLoadStart causes it to fire on every rerender when callers pass non-memoized handlers (for example ImageView passes onLoadStart={imageLoadingStart} in src/components/ImageView/index.tsx, where imageLoadingStart is recreated each render). On web this can repeatedly enqueue state updates (setImageSize({width: 0, height: 0})) and lead to render thrashing or an update loop even when the image URL has not changed, which is a behavior regression from the prior event-driven react-native image implementation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think that previously the same kind of logic was used in the Image from react-native-web, it also called onLoadStart from a useEffect that depends on both source and onLoadStart:
https://github.com/necolas/react-native-web/blob/a9de220ba9e65bdea540fb5322ffb1da2b0bf442/packages/react-native-web/src/exports/Image/index.js#L278-L322
Just maybe I need to add a check for the source to be not a falsy value.
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required on this one.
|
Whoops mean to comment not approve, though plenty more review to go in any case. |
|
@shubham1206agra can you please prioritize this review? |
|
🚧 @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.
|
@mountiny, is there any way we can send the HIEC image in the App without doing any conversions? |
@shubham1206agra We are doing conversions to support HEIC images display on the web I beleive. |
|
HEIC is a long long discussion in the App |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
@shubham1206agra also asked in slack, but do you have capacity to review and complete the checklist tomorrow? |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work? |
|
@shubham1206agra Will do the checklist tomorrow |
Reviewer Checklist
Screenshots/VideosAndroid: mWeb ChromeScreen.Recording.2026-03-02.at.7.22.22.PM.moviOS: mWeb SafariScreen.Recording.2026-03-02.at.7.17.14.PM.movMacOS: Chrome / SafariScreen.Recording.2026-03-02.at.7.12.04.PM.mov |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 9.3.28-0 🚀
|
|
This PR failing because of the issue 1.mp4 |
|
Deploy Blocker #83982 was identified to be related to this PR. |
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 9.3.30-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.30-3 🚀
|
Explanation of Change
Migrate web
BaseImagefrom react-native Image to expo-image+003+image-header-support.patch) since expo-image handles HTTP headers nativelyrecyclingKeyprop instead of the React key for image recyclingFixed Issues
$ #33725
#9402
PROPOSAL: N/A
Tests
3.1 Open any attachment, see it loads correctly
Offline tests
Same, as in the Tests section
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same, as in the Tests section
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.mp4
Android: mWeb Chrome
android_web.mp4
iOS: Native
ios.mov
iOS: mWeb Safari
ios_web.mov
MacOS: Chrome / Safari
web.mp4