Attachments carousel loading optimizations#83185
Conversation
and re-introduce carousel loading optimizations
|
@abzokhattab 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] |
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.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfdc21b735
ℹ️ 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".
|
|
||
| const foundPage = state.pagerItems.findIndex((item) => item.attachmentID === attachmentID); | ||
| const identifier = attachmentID ?? uri; | ||
| const foundPage = state.pagerItems.findIndex((item) => (item.attachmentID ?? item.source) === identifier); |
There was a problem hiding this comment.
Guard missing pager matches before setting page index
findIndex can return -1 here when the current attachment is not present in pagerItems (for example on web carousels where the context only provides a single source entry). In that case this component treats the visible item as inactive, and isImageLoaded short-circuits to true, so the loading indicator is skipped and users can see a blank frame until the image finishes loading. Please handle the -1 case explicitly (e.g., fallback to activePage/0) before deriving active/loading state.
Useful? React with 👍 / 👎.
…en attachment is not found, and add corresponding unit test for this behavior
…ymonzalarski/attachments/images-load-extremely-slow-when-viewing-in-the-app-fix
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
reviewing it today |
…s/images-load-extremely-slow-when-viewing-in-the-app-fix
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-02-25.at.02.06.26.movScreen.Recording.2026-02-25.at.02.08.07.movScreen.Recording.2026-02-25.at.02.10.01.movScreen.Recording.2026-02-25.at.02.11.30.movAndroid: mWeb ChromeScreen.Recording.2026-02-25.at.02.15.01.moviOS: HybridAppScreen.Recording.2026-02-25.at.01.29.41.movScreen.Recording.2026-02-25.at.01.57.38.movScreen.Recording.2026-02-25.at.01.59.25.moviOS: mWeb SafariScreen.Recording.2026-02-25.at.02.01.47.movMacOS: Chrome / SafariScreen.Recording.2026-02-25.at.01.20.53.movScreen.Recording.2026-02-25.at.01.22.15.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. |
|
🚧 @cristipaval 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/cristipaval in version: 9.3.26-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.3.26-8 🚀
|
PR #82282 was reverted because it exposed a pre-existing bug:
convertFileToAttachment() did not set attachmentID, causing
every Lightbox to resolve to page = 0 via findIndex. With
FALLBACK_OFFSET=2, pages beyond ±2 of the incorrect page 0
showed blank on Android (NUMBER_OF_CONCURRENT_LIGHTBOXES=1).
Explanation of Change
file.uri (root cause fix)
attachmentID is undefined (defense-in-depth)
rendering to ±2 pages
expo-image loading order
Image types
and priority assignment
Fixed Issues
$ #81250
PROPOSAL:
Tests
Prerequisites: Use a chat room with 30+ image attachments.
Each scenario below should be tested twice — once after a
hard refresh of the app (to ensure no cached images give
false positives), and once without a refresh (normal warm
state).
Scenario 1: Sequential swipe from first to last and back
the last one
excessive loading time)
first one
Scenario 2: Opening from the middle and swiping in both
directions
conversation
each one displays
verify each one displays
displays
Scenario 3: Opening the last image
4-5+ seconds)
correctly
Scenario 4: Zoom and gestures
Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
recording_images_android_fix.mov
Android: mWeb Chrome
iOS: Native
recording_images_ios_fix.mov
iOS: mWeb Safari
MacOS: Chrome / Safari