Skip to content

Commit 1631cdd

Browse files
refactor: resolve multi-select strategy by file extension instead of 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.
1 parent b65b34b commit 1631cdd

6 files changed

Lines changed: 125 additions & 19 deletions

File tree

src/components/AttachmentPicker/index.native.tsx

Lines changed: 7 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import {keepLocalCopy, pick, types} from '@react-native-documents/picker';
33
import {Str} from 'expensify-common';
44
import {ImageManipulator, SaveFormat} from 'expo-image-manipulator';
55
import React, {useCallback, useMemo, useRef, useState} from 'react';
6-
import {Alert, Platform, View} from 'react-native';
6+
import {Alert, View} from 'react-native';
77
import RNFetchBlob from 'react-native-blob-util';
88
import {launchImageLibrary} from 'react-native-image-picker';
99
import type {Asset, Callback, CameraOptions, ImageLibraryOptions, ImagePickerResponse} from 'react-native-image-picker';
@@ -26,6 +26,7 @@ import type {TranslationPaths} from '@src/languages/types';
2626
import type {FileObject, ImagePickerResponse as FileResponse} from '@src/types/utils/Attachment';
2727
import type IconAsset from '@src/types/utils/IconAsset';
2828
import launchCamera from './launchCamera/launchCamera';
29+
import processAssets from './processAssets';
2930
import type AttachmentPickerProps from './types';
3031

3132
const EXTENSION_TO_NATIVE_TYPE: Record<string, string> = {
@@ -259,24 +260,11 @@ function AttachmentPicker({
259260
}
260261
};
261262

262-
// Each HEIC asset is decoded to a full native bitmap during conversion. On iOS HybridApp the
263-
// process shares a single jetsam budget with OldDot, so converting the whole selection at once can
264-
// allocate N large bitmaps simultaneously and OOM. There we process assets one at a time to keep at
265-
// most one heavy decode alive; other platforms keep the original concurrent behavior (per #93846).
266-
const processAssets = async () => {
267-
if (Platform.OS === 'ios') {
268-
for (const asset of assets) {
269-
// eslint-disable-next-line no-await-in-loop
270-
await processAsset(asset);
271-
}
272-
} else {
273-
await Promise.all(assets.map(processAsset));
274-
}
275-
276-
resolve(processedAssets.length > 0 ? processedAssets : undefined);
277-
};
278-
279-
processAssets().catch(reject);
263+
// `processAssets` is resolved by file extension: sequential on iOS to avoid OOMing on
264+
// concurrent HEIC decodes (see processAssets/index.ios.ts), concurrent elsewhere (per #93846).
265+
processAssets(assets, processAsset)
266+
.then(() => resolve(processedAssets.length > 0 ? processedAssets : undefined))
267+
.catch(reject);
280268
});
281269
}),
282270
[fileLimit, showGeneralAlert, translate, type],
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
/**
2+
* Processes the picked assets concurrently. This is the default strategy on every platform except iOS
3+
* (see sequential.ts), where decoding the whole selection at once can exhaust the native memory budget.
4+
*/
5+
function processAssets<T>(assets: T[], processAsset: (asset: T) => Promise<void>): Promise<void> {
6+
return Promise.all(assets.map(processAsset)).then(() => undefined);
7+
}
8+
9+
export default processAssets;
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
// iOS strategy: convert the multi-select sequentially so at most one heavy HEIC decode is alive at a time.
2+
import processAssets from './sequential';
3+
4+
export default processAssets;
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
// Default (non-iOS) strategy: convert the whole multi-select concurrently. iOS overrides this with
2+
// sequential processing via index.ios.ts to avoid OOMing on simultaneous HEIC decodes.
3+
import processAssets from './concurrent';
4+
5+
export default processAssets;
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
/**
2+
* Processes the picked assets one at a time. Each HEIC asset decodes to a full native bitmap during
3+
* conversion, and on iOS HybridApp the process shares a single jetsam memory budget with the resident OldDot,
4+
* so converting the whole selection at once can allocate N large bitmaps simultaneously and OOM (Sentry
5+
* APP-63S). Sequencing keeps at most one heavy decode alive at a time.
6+
*/
7+
async function processAssets<T>(assets: T[], processAsset: (asset: T) => Promise<void>): Promise<void> {
8+
for (const asset of assets) {
9+
// The sequencing is the whole point here: await each decode before starting the next.
10+
// eslint-disable-next-line no-await-in-loop
11+
await processAsset(asset);
12+
}
13+
}
14+
15+
export default processAssets;
Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
import processAssetsConcurrent from '@components/AttachmentPicker/processAssets/concurrent';
2+
import processAssetsSequential from '@components/AttachmentPicker/processAssets/sequential';
3+
4+
/** Lets every already-scheduled microtask run, so in-flight `processAsset` calls reach their parked state. */
5+
async function flushMicrotasks() {
6+
for (let i = 0; i < 10; i++) {
7+
await Promise.resolve();
8+
}
9+
}
10+
11+
function createDeferred() {
12+
let resolve: () => void = () => {};
13+
const promise = new Promise<void>((res) => {
14+
resolve = res;
15+
});
16+
return {promise, resolve};
17+
}
18+
19+
/**
20+
* Tracks how many `processAsset` calls are in flight at once. Every call parks on a shared gate, so the peak
21+
* concurrency is observable before the gate is released: a concurrent strategy reaches `items.length`, a
22+
* sequential one never exceeds 1.
23+
*/
24+
function createConcurrencyTracker() {
25+
const gate = createDeferred();
26+
let running = 0;
27+
let maxRunning = 0;
28+
const processAsset = async (): Promise<void> => {
29+
running += 1;
30+
maxRunning = Math.max(maxRunning, running);
31+
await gate.promise;
32+
running -= 1;
33+
};
34+
return {processAsset, release: gate.resolve, getMaxRunning: () => maxRunning};
35+
}
36+
37+
describe('AttachmentPicker processAssets', () => {
38+
const items = ['a', 'b', 'c'];
39+
40+
describe('default (concurrent)', () => {
41+
it('runs every asset at the same time', async () => {
42+
const tracker = createConcurrencyTracker();
43+
44+
const done = processAssetsConcurrent(items, tracker.processAsset);
45+
await flushMicrotasks();
46+
expect(tracker.getMaxRunning()).toBe(items.length);
47+
48+
tracker.release();
49+
await done;
50+
});
51+
52+
it('processes every asset', async () => {
53+
const processed: string[] = [];
54+
55+
await processAssetsConcurrent(items, async (item: string) => {
56+
processed.push(item);
57+
});
58+
59+
expect(processed).toEqual(items);
60+
});
61+
});
62+
63+
describe('iOS (sequential)', () => {
64+
it('runs at most one asset at a time', async () => {
65+
const tracker = createConcurrencyTracker();
66+
67+
const done = processAssetsSequential(items, tracker.processAsset);
68+
await flushMicrotasks();
69+
expect(tracker.getMaxRunning()).toBe(1);
70+
71+
tracker.release();
72+
await done;
73+
});
74+
75+
it('processes the assets in order', async () => {
76+
const processed: string[] = [];
77+
78+
await processAssetsSequential(items, async (item: string) => {
79+
processed.push(item);
80+
});
81+
82+
expect(processed).toEqual(items);
83+
});
84+
});
85+
});

0 commit comments

Comments
 (0)