Fix iOS receipt multi-select OOM: skip decoding hidden previews + sequence conversions#94976
Conversation
When several images are picked at once, the AttachmentPicker fires every HEIC conversion concurrently, so N full-resolution bitmaps can decode simultaneously. On iOS HybridApp, where the process shares a single jetsam budget with OldDot, that concurrent spike contributes to the std::bad_alloc OOM (Sentry APP-63S). Process picked assets one at a time on iOS so at most one heavy decode is alive; other platforms keep the original concurrent behavior. This is the sequencing half of the Expensify#93846 fix, split out from the decode cap so its memory impact can be validated and adopted independently.
melvin-bot
Bot
requested review from
a team,
QichenZhu and
flaviadefaria
and removed request for
a team
|
@QichenZhu 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // process shares a single jetsam budget with OldDot, so converting the whole selection at once can | ||
| // allocate N large bitmaps simultaneously and OOM. There we process assets one at a time to keep at | ||
| // most one heavy decode alive; other platforms keep the original concurrent behavior (per #93846). | ||
| const processAssets = async () => { |
There was a problem hiding this comment.
❌ CONSISTENCY-1 (docs)
This change adds a runtime Platform.OS === 'ios' branch inside the component to give iOS sequential processing and other platforms concurrent processing. Mixing platform-specific control flow inside a shared component is exactly the pattern CONSISTENCY-1 discourages — it increases branching complexity and platform-specific bug risk, and the .native.tsx/.web.tsx exception does not cover an iOS-vs-Android split done via a runtime check. There is currently no index.ios.tsx/index.android.tsx in this folder, so the divergence is being handled with an in-component check rather than file extensions.
Prefer separating the platform-specific behavior through file extensions. For example, extract the asset-processing strategy into a platform-resolved helper:
// processAssets.ios.ts — sequential to keep at most one heavy HEIC decode alive
export default async function processAssets(assets: Asset[], processAsset: (a: Asset) => Promise<void>) {
for (const asset of assets) {
// eslint-disable-next-line no-await-in-loop -- decode one at a time to avoid OOM on iOS HybridApp
await processAsset(asset);
}
}
// processAssets.ts (default/android) — concurrent
export default async function processAssets(assets: Asset[], processAsset: (a: Asset) => Promise<void>) {
await Promise.all(assets.map(processAsset));
}Then call the resolved processAssets(assets, processAsset) in index.native.tsx without the Platform.OS check, letting the bundler pick the correct implementation per platform.
Reviewed at: b65b34b | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Good call. Refactored to extension-based resolution: the concurrent/sequential strategy now lives in a platform-resolved processAssets helper (concurrent.ts default, sequential.ts on iOS via index.ios.ts), so the component just calls processAssets(assets, processAsset) with no Platform.OS branch. Added unit tests for both strategies.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
…a runtime check Addresses the CONSISTENCY-1 review note: rather than branching on Platform.OS === 'ios' inside the component, extract the concurrent/sequential strategy into a platform-resolved processAssets helper (concurrent.ts default, sequential.ts on iOS via index.ios.ts). The component just calls processAssets(assets, processAsset) and lets the bundler pick the implementation. Adds unit tests covering both strategies.
dilshodmackbook-sketch
force-pushed
the
dilshod/fix-93846-sequence-multiselect
branch
from
fe5da76 to
1631cdd
Compare
|
@codex 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.
…equence-multiselect # Conflicts: # src/components/AttachmentPicker/index.native.tsx
ReceiptPreviews is mounted whenever multi-scan is available (canUseMultiScan && onMultiScanSubmit), not gated on isMultiScanEnabled. When multi-scan is off the strip only animates its height/width to 0, but the FlatList still mounts an item per draft receipt and renders a full <Image>, decoding every draft receipt into native memory while nothing is visible. On iOS a large multi-select (e.g. 30 HEIC receipts) decodes them all offscreen and can spike native memory into an OOM (Expensify#93846). Render only the placeholder until multi-scan is enabled so hidden receipts are never decoded, and pass isMultiScanEnabled as extraData so cells re-render when the flag flips. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dilshodmackbook-sketch
changed the title
|
Folded the ReceiptPreviews fix into this PR so #94976 now covers both spikes from #93846: hidden previews no longer decode when multi-scan is off, and the iOS conversions are sequenced. Updated the description and test steps to the Scan-flow repro (30 HEIC). Planning to close #94969 in favor of this. |
|
Profiled the documented 30-HEIC Scan repro with Xcode's Memory report (before/after videos in the PR body). Peak drops 6.71 GB -> 3.42 GB (~49%). The sequencing removes the bulk (the concurrent 30-decode spike), and the ReceiptPreviews placeholder trims the remaining offscreen decodes. This is a standalone dev build on the simulator so the absolute numbers are inflated, but the relative delta holds and it lines up with the two spike sources we discussed. |
|
@QichenZhu friendly bump on this one whenever you get a chance. Everything's ready on my end: both spikes from #93846 are addressed, the 30-HEIC Scan repro is profiled (peak 6.71 GB -> 3.42 GB), and CI is green. Happy to walk through anything if it helps the review. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
@dilshodmackbook-sketch good progress! This PR works for low traffic accounts, but for high traffic accounts, the app still crashes after you tap Create expenses. after.movCould you dig further? A likely culprit is |
|
Thanks @QichenZhu, great lead — I dug into For each receipt, Agreed on keeping it a separate follow-up PR so it can be reverted independently. I'll confirm what |
|
Quick scoping question before I open the follow-up: the high traffic crash turned out to be a separate root cause from the decode spike this PR fixes — it's the |
|
@dilshodmackbook-sketch, it is the same issue, just with multiple causes. |
|
@mountiny, could you trigger an ad-hoc build again? Also, what do you think of this plan?
|
|
Sounds good, I'll keep it under #93846 and move the One thing I confirmed while digging: |
|
Follow-up is up: #95663. It strips the large policy collections ( |
|
🚧 mountiny 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! 🧪🧪
|
|
Update: I retested today and saw no major difference before and after this fix, even though it used to work. Let me check if recent changes on main affected things. iPhone 17 - Low-traffic account - Before fix - No crashbefore-iphone-17-low.moviPhone 17 - Low-traffic account - After fix - No crashafter-iphone-17-low.moviPad 9 - Low-traffic account - Before fix - Crashbefore-ipad-9-low.moviPad 9 - Low-traffic account - After fix - Crashafter-ipad-9-low.mov |
3 participants
Explanation of Change
This is the fix for the iOS receipt multi-select OOM (
std::bad_alloc, SentryAPP-63S). Profiling the documented repro (30 HEIC receipts through the Scan flow) shows the peak comes from two independent memory spikes, so this PR addresses both:1. Hidden receipt previews decode every draft receipt (dominant spike).
ReceiptPreviewsis mounted whenever multi-scan is available (canUseMultiScan && onMultiScanSubmit), not gated onisMultiScanEnabled. When multi-scan is off the strip only animates its height/width to0, but theFlatListstill mounts one item per draft receipt and renders a full<Image>, decoding every draft receipt into native memory while nothing is visible. On a large multi-select (e.g. 30 HEIC) all of them decode offscreen at once.Fix: render the placeholder (not the
<Image>) until multi-scan is enabled, so hidden receipts are never decoded.isMultiScanEnabledis passed asextraDataso the cells re-render when the flag flips.2. Concurrent HEIC→JPEG conversions at pick time.
When several images are multi-selected,
AttachmentPickerfires every HEIC→JPEG conversion concurrently — the loop kicks off eachverifyFileFormat(...).then(renderAsync())without awaiting the previous one, so N large photos decode into native memory at the same time.Fix: on iOS, process picked assets one at a time so at most one heavy decode is alive at any moment. Other platforms keep the original concurrent behavior. The strategy is resolved by file extension (see
processAssets/index.ios.ts).Together these keep peak native memory bounded through the multi-image scan flow, where previously a large batch could OOM on iOS.
Memory profiling (iOS, documented 30-HEIC Scan repro, Xcode Memory report):
main)Peak memory drops 6.71 GB → 3.42 GB (~49%). Sequencing removes the bulk (the concurrent 30-decode spike); the
ReceiptPreviewsplaceholder trims the remaining offscreen decodes. Numbers are from a standalone dev build on the simulator, so absolute values are inflated, but the relative delta holds. Before/after videos are in the iOS: Native section below.Fixed Issues
$ #93846
PROPOSAL: #93846 (comment)
Tests
Precondition: download the HEIC image from #68778 (comment), duplicate it into 30 copies, and import them into iOS Photos.
Offline tests
QA Steps
Precondition: import ~30 large HEIC photos into iOS Photos.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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
video_2026-07-09_18-43-59.mp4
Android: mWeb Chrome
Screen.Recording.2026-07-09.at.16.19.55.mov
iOS: Native
30-HEIC Scan repro, Xcode Memory report next to the app (standalone dev build on the simulator).
Before (
main) — peak 6.71 GB:before-6.71GB.mp4
After (this PR) — peak 3.42 GB:
after-3.42GB.mp4
iOS: mWeb Safari
Screen.Recording.2026-07-09.at.16.22.19.mov
MacOS: Chrome / Safari
Screen.Recording.2026-07-09.at.15.50.57.mp4
Screen.Recording.2026-07-09.at.16.11.37.mp4