Limit fallback image rendering and add priority to reduce carousel lo…#82282
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
src/components/Image/types.ts
Outdated
| /** Priorities for completing loads. If more than one load is queued at a time, | ||
| * the load with the higher priority will be started first. | ||
| * Maps to SDWebImageHighPriority (iOS) and Glide.Priority.IMMEDIATE (Android). */ | ||
| priority?: 'low' | 'normal' | 'high' | null; |
There was a problem hiding this comment.
Maybe we can add a const with these values, so it's easier to use?
There was a problem hiding this comment.
Thank you, very good comment, I've updated my PR according to your suggestions
src/components/Lightbox/index.tsx
Outdated
| // Limits fallback image rendering to only a few pages around the active page. | ||
| // This prevents distant carousel items from queuing unnecessary image downloads, | ||
| // which would starve the active image of network bandwidth. | ||
| const isFallbackInRange = useMemo(() => { |
There was a problem hiding this comment.
Lightbox is already compiled with react compiler. Maybe we don't need useMemo here?
There was a problem hiding this comment.
You're right, thanks
src/components/Lightbox/index.tsx
Outdated
| [onScaleChangedContext, onScaleChangedProp], | ||
| ); | ||
|
|
||
| const imagePriority = useMemo(() => { |
There was a problem hiding this comment.
Same. Is useMemo necessary here as its already using React compiler?
There was a problem hiding this comment.
You're right, thanks
There was a problem hiding this comment.
I suppose this is still WIP, right? I'm seeing several errors
There was a problem hiding this comment.
Yes, but today I'm going to fully publish ready to review PR
There was a problem hiding this comment.
PR is ready for review :)
|
@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] |
tests/unit/LightboxTest.tsx
Outdated
| }); | ||
| } | ||
| return { | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line comment here lacks a justification explaining why the rule is disabled. While __esModule: true is a common pattern in Jest mocks that requires the naming convention rule to be disabled, please add a brief comment explaining the reason.
// eslint-disable-next-line @typescript-eslint/naming-convention -- __esModule is required by Jest to properly mock ES modules with default exports
__esModule: true,Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| return { | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| __esModule: true, | ||
| default: ({children}: {children: React.ReactNode}) => MockReact.createElement(View, {testID: 'multi-gesture-canvas'}, children), |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line comment here lacks a justification explaining why the rule is disabled. Please add a brief comment explaining the reason.
// eslint-disable-next-line @typescript-eslint/naming-convention -- __esModule is required by Jest to properly mock ES modules with default exports
__esModule: true,Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
No product review needed |
|
Waiting for @abzokhattab to review and test |
There was a problem hiding this comment.
Looks like we are missing tests for the NORMAL priority
There was a problem hiding this comment.
Thanks, added them in the latest commit
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-02-17.at.00.36.06.movScreen.Recording.2026-02-17.at.00.38.04.movAndroid: mWeb ChromeScreen.Recording.2026-02-17.at.00.42.03.moviOS: HybridAppUntitled.movScreen.Recording.2026-02-17.at.00.16.03.movScreen.Recording.2026-02-17.at.00.16.20.moviOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-02-17.at.00.19.38.movUntitled.mov |
|
Nice Improvement @szymonzalarski98 i added a small comment on the tests but the manual testing looks good to me ... i uploaded 30 big images ranging from 5mg to 10mg each and the loading time improved |
|
🚧 @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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/cristipaval in version: 9.3.21-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.21-4 🚀
|
|
🚀 Deployed to staging by https://github.com/cristipaval in version: 9.3.22-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.22-4 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.22-4 🚀
|
Explanation of Change
Previously, Lightbox rendered a fallback
for every carousel page outside the lightbox window. In a 50-image carousel, this meant ~47 unnecessary image downloads queued simultaneously, starving the active image of network bandwidth (SDWebImage processes downloads FIFO with 3 concurrent slots).
This PR adds two fixes:
Bounded fallback window — limits fallback image rendering to ±2 pages around the active page (FALLBACK_OFFSET), reducing total mounted
components from ~50 to ~5.
Image loading priority — passes expo-image priority prop so the active image downloads first (high), adjacent lightbox pages next (normal), and fallback images last (low).
Fixed Issues
$ #81250
PROPOSAL:
Proposal: Eliminate Bandwidth Starvation in Attachment Carousel Loading
Background
The Expensify mobile application uses a Lightbox component to display full-screen expense receipts and attachments in a horizontally swipeable carousel. To enable smooth swiping between images, the carousel renders the current attachment as well as several adjacent pages.
Problem
When a user opens an attachment carousel in a chat page, if the application simultaneously initiates multiple attachments downloads for all adjacent carousel items, then the active attachment's download speed is throttled by network contention, resulting in a several-second delay before the user can read the attachment.
Solution
We will implement intelligent image loading prioritization and bandwidth management to ensure the active attachment always has the highest throughput.
Introduce Image Priority: Add a priority prop to the base Image component that accepts 'low', 'normal', or 'high'.Limit Fallback Render Range: Introduce a range constraint (FALLBACK_OFFSET = 2) that prevents Lightbox from rendering its fallback
component for pages more than two positions away from the user's current view. The Lightbox component itself remains mounted on all pages, but distant ones simply do not render an Image — and therefore do not initiate any network requests.Dynamic Priority Assignment:Assign High priority to the active page.Assign Normal priority to pages within the lightbox window (±1 on iOS, ±0 on Android).Assign Low priority to pages within the fallback range (±2) but outside the lightbox window.Pages outside the fallback range render no Image component at all.Conditional Rendering: Update the Lightbox logic to strictly enforce that distant carousel items do not mount their Image components until they move within the permitted look-ahead range, using both a synchronous useMemo guard and an asynchronous state flag for defense-in-depth during fast swipe sequences.
Tests
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
Android: mWeb Chrome
iOS: Native
images.1.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
Images-web.1.mp4