refactor: IOURequestStepScan clean-up, phase 3: Consolidate isMobile() and add useDragAndDropSupport#83380
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors IOURequestStepScan to centralize mobile/desktop behavior checks and introduce a platform-specific useDragAndDropSupport helper for drag-and-drop availability.
Changes:
- Added
useDragAndDropSupportwith web + native implementations. - Replaced repeated
isMobile()calls with a cached boolean inIOURequestStepScan. - Updated layout logic to use the new drag-and-drop capability flag in places where desktop-only styling is applied.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/pages/iou/request/step/IOURequestStepScan/useDragAndDropSupport/index.ts | Adds web implementation for drag-and-drop support detection. |
| src/pages/iou/request/step/IOURequestStepScan/useDragAndDropSupport/index.native.ts | Adds native implementation (always false) for drag-and-drop support detection. |
| src/pages/iou/request/step/IOURequestStepScan/index.tsx | Uses useDragAndDropSupport() and consolidates isMobile() calls to drive UI/layout decisions. |
Comments suppressed due to low confidence (4)
src/pages/iou/request/step/IOURequestStepScan/index.tsx:600
styles.chooseFilesView(!canUseDragAndDrop)ties the choose-files padding variant to drag-and-drop capability rather than layout width. SincecanUseDragAndDropis a feature flag (desktop web) and not a responsive breakpoint, this changes behavior from the priorisSmallScreenWidth-based styling and can cause incorrect padding calculations. Consider restoring the responsive boolean (e.g., viauseResponsiveLayout()/shouldUseNarrowLayout) for thechooseFilesView(...)argument, and keepcanUseDragAndDroponly for gating drag-and-drop-specific UI.
const chooseFilesPaddingVertical = Number(styles.chooseFilesView(!canUseDragAndDrop).paddingVertical);
src/pages/iou/request/step/IOURequestStepScan/index.tsx:662
- This condition/argument combination is self-contradictory: when
canUseDragAndDropis true,!canUseDragAndDropis always false, so this will always applystyles.chooseFilesView(false)and never the other variant. If the intent is to select a narrow vs wide layout, pass the responsive boolean (as before) intochooseFilesView(...)instead of!canUseDragAndDrop.
style={[styles.flex1, canUseDragAndDrop && styles.chooseFilesView(!canUseDragAndDrop)]}
src/pages/iou/request/step/IOURequestStepScan/index.tsx:65
isMobileWebis derived fromisMobile()but the comment states it's intended to cover both mobile web and native apps. Either rename the variable to reflect the broader meaning (ifisMobile()is cross-platform), or update the comment/logic so the name and behavior match (e.g., keepisMobileWebonly for web and use a separate native/mobile flag where needed).
const isMobileWeb = isMobile();
src/pages/iou/request/step/IOURequestStepScan/index.tsx:601
isMobileWebis derived fromisMobile()but the comment states it's intended to cover both mobile web and native apps. Either rename the variable to reflect the broader meaning (ifisMobile()is cross-platform), or update the comment/logic so the name and behavior match (e.g., keepisMobileWebonly for web and use a separate native/mobile flag where needed).
// We use isMobileWeb here to explicitly hide the alternative methods component on both mobile web and native apps
const chooseFilesPaddingVertical = Number(styles.chooseFilesView(!canUseDragAndDrop).paddingVertical);
const shouldHideAlternativeMethods = isMobileWeb || alternativeMethodsHeight + desktopUploadViewHeight + chooseFilesPaddingVertical * 2 > containerHeight;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@trjExpensify no product considerations here |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| // eslint-disable-next-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth | ||
| const {isSmallScreenWidth} = useResponsiveLayout(); | ||
| const isMobileWeb = isMobile(); | ||
| const canUseDragAndDrop = useDragAndDropSupport(); |
There was a problem hiding this comment.
Hmm, since this hook doesn't use any React APIs or trigger re-renders, it might be better implemented as a utility function rather than a hook.
Also, since we use this on index.tsx file only, we can just simply use isMobileWeb here.
const canUseDragAndDrop = !isMobileWeb;
cc: @roryabraham
There was a problem hiding this comment.
If we refactor this further as I commented here, then we probably can just remove this.
There was a problem hiding this comment.
I agree, we don't really need a hook here.
| screenshotQuality={0} | ||
| /> | ||
| {canUseMultiScan && isMobile() ? ( | ||
| {canUseMultiScan && isMobileWeb ? ( |
There was a problem hiding this comment.
We can remove isMobileWeb condition from mobileCameraView since we only shows mobileCameraView if isMobileWeb is true.
yes this work but I think having separate component for |
|
@samranahm I think since it's a web component only, it's better to put it in I think if we have the |
|
I don't feel strongly, but this is one case where I think the code could be made cleaner/clearer if we subdivide |
|
Though I agree |
Okay, let's separate it into 2 files, but name the mobile web one |
|
@codex review. |
This comment was marked as outdated.
This comment was marked as outdated.
IOURequestStepScan clean-up, phase 3: Consolidate isMobile() and add useDragAndDropSupportIOURequestStepScan clean-up, phase 3: Consolidate isMobile() and add useDragAndDropSupport
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
@bernhardoj up to you. I trust you to decide whichever you think most appropriate. Go ahead and create a new issue if you want |
|
I reported it in Slack. @samranahm another conflict 😬 |
No problem 😄 Happy to resolve it. |
…pScan-clean-up-p3
|
The test is unrelated. It's a flaky one. |
|
@roryabraham All yours. |
|
Sorry @samranahm, conflicts |
|
Resolving merge conflicts. |
…pScan-clean-up-p3
roryabraham
left a comment
There was a problem hiding this comment.
After making the other suggested changes, the MobileWebCameraView compiles with React Compiler, and you can remove all manual memoization.
Suggested diff
diff --git a/Mobile-Expensify b/Mobile-Expensify
index 56b3c41ffcc..ff7dc596e57 160000
--- a/Mobile-Expensify
+++ b/Mobile-Expensify
@@ -1 +1 @@
-Subproject commit 56b3c41ffcc17e76ee067daf27e9309d0eb2fcd9
+Subproject commit ff7dc596e57ba85cb0a7365da514ab733f4aca56
diff --git a/src/pages/iou/request/step/IOURequestStepScan/components/MobileWebCameraView.tsx b/src/pages/iou/request/step/IOURequestStepScan/components/MobileWebCameraView.tsx
index a20acca1985..e859d1771c8 100644
--- a/src/pages/iou/request/step/IOURequestStepScan/components/MobileWebCameraView.tsx
+++ b/src/pages/iou/request/step/IOURequestStepScan/components/MobileWebCameraView.tsx
@@ -1,5 +1,5 @@
import {useIsFocused} from '@react-navigation/native';
-import React, {useCallback, useEffect, useReducer, useRef, useState} from 'react';
+import React, {useEffect, useReducer, useRef, useState} from 'react';
import type {LayoutRectangle} from 'react-native';
import {StyleSheet, View} from 'react-native';
import type {OnyxEntry} from 'react-native-onyx';
@@ -36,6 +36,39 @@ import {isEmptyObject} from '@src/types/utils/EmptyObject';
import NavigationAwareCamera from './NavigationAwareCamera/WebCamera';
import ReceiptPreviews from './ReceiptPreviews';
+/**
+ * Eagerly query camera permission state at module load time so the result
+ * is available synchronously (or near-synchronously) when the component mounts.
+ * This avoids a loading-spinner flash when permission is already granted.
+ */
+let cachedPermissionState: PermissionState | undefined;
+if (typeof navigator !== 'undefined' && navigator.permissions) {
+ navigator.permissions
+ .query({name: 'camera'})
+ .then((status) => {
+ cachedPermissionState = status.state;
+ status.addEventListener('change', () => {
+ cachedPermissionState = status.state;
+ });
+ })
+ .catch(() => {
+ cachedPermissionState = 'denied';
+ });
+}
+
+function queryCameraPermission(): Promise<PermissionState> {
+ if (cachedPermissionState !== undefined) {
+ return Promise.resolve(cachedPermissionState);
+ }
+ if (typeof navigator === 'undefined' || !navigator.permissions) {
+ return Promise.resolve('denied' as PermissionState);
+ }
+ return navigator.permissions
+ .query({name: 'camera'})
+ .then((status) => status.state)
+ .catch((): PermissionState => 'denied');
+}
+
type MobileWebCameraViewProps = {
initialTransaction: OnyxEntry<Transaction>;
initialTransactionID: string;
@@ -101,11 +134,12 @@ function MobileWebCameraView({
const lazyIllustrations = useMemoizedLazyIllustrations(['MultiScan', 'Hand', 'Shutter']);
const lazyIcons = useMemoizedLazyExpensifyIcons(['Bolt', 'Gallery', 'ReceiptMultiple', 'boltSlash']);
const isTabActive = useIsFocused();
- const [cameraPermissionState, setCameraPermissionState] = useState<PermissionState | undefined>('prompt');
+ const [cameraPermissionState, setCameraPermissionState] = useState<PermissionState | undefined>(() => cachedPermissionState ?? 'prompt');
const [isFlashLightOn, toggleFlashlight] = useReducer((state: boolean) => !state, false);
const [isTorchAvailable, setIsTorchAvailable] = useState(false);
- const [isQueriedPermissionState, setIsQueriedPermissionState] = useState(false);
- const [videoConstraints, setVideoConstraints] = useState<MediaTrackConstraints>();
+ const [isQueriedPermissionState, setIsQueriedPermissionState] = useState(() => cachedPermissionState !== undefined);
+ const [deviceConstraints, setDeviceConstraints] = useState<MediaTrackConstraints>();
+ const videoConstraints = isTabActive ? deviceConstraints : undefined;
const cameraRef = useRef<Webcam>(null);
const trackRef = useRef<MediaStreamTrack | null>(null);
const viewfinderLayout = useRef<LayoutRectangle>(null);
@@ -115,7 +149,7 @@ function MobileWebCameraView({
* On phones that have ultra-wide lens, react-webcam uses ultra-wide by default.
* The last deviceId is of regular lens camera.
*/
- const requestCameraPermission = useCallback(() => {
+ const requestCameraPermission = () => {
const defaultConstraints = {facingMode: {exact: 'environment'}};
navigator.mediaDevices
.getUserMedia({video: {facingMode: {exact: 'environment'}, zoom: {ideal: 1}}})
@@ -135,12 +169,12 @@ function MobileWebCameraView({
}
}
if (deviceId) {
- setVideoConstraints({deviceId});
+ setDeviceConstraints({deviceId});
return;
}
}
if (!navigator.mediaDevices.enumerateDevices) {
- setVideoConstraints(defaultConstraints);
+ setDeviceConstraints(defaultConstraints);
return;
}
navigator.mediaDevices.enumerateDevices().then((devices) => {
@@ -153,30 +187,26 @@ function MobileWebCameraView({
}
}
if (!lastBackDeviceId) {
- setVideoConstraints(defaultConstraints);
+ setDeviceConstraints(defaultConstraints);
return;
}
- setVideoConstraints({deviceId: lastBackDeviceId});
+ setDeviceConstraints({deviceId: lastBackDeviceId});
});
})
.catch(() => {
- setVideoConstraints(defaultConstraints);
+ setDeviceConstraints(defaultConstraints);
setCameraPermissionState('denied');
});
- }, []);
+ };
useEffect(() => {
if (!isTabActive) {
- setVideoConstraints(undefined);
return;
}
- navigator.permissions
- .query({
- name: 'camera',
- })
- .then((permissionState) => {
- setCameraPermissionState(permissionState.state);
- if (permissionState.state === 'granted') {
+ queryCameraPermission()
+ .then((state) => {
+ setCameraPermissionState(state);
+ if (state === 'granted') {
requestCameraPermission();
}
})
@@ -186,9 +216,8 @@ function MobileWebCameraView({
.finally(() => {
setIsQueriedPermissionState(true);
});
- // We only want to get the camera permission status when the component is mounted
- // eslint-disable-next-line react-hooks/exhaustive-deps
- }, [isTabActive]);
+ // We want to refresh camera permission status when the tab is focused because the user could have changed permissions while the tab was backgrounded.
+ }, [isTabActive, requestCameraPermission]);
useEffect(
() => () => {
@@ -214,46 +243,43 @@ function MobileWebCameraView({
setIsTorchAvailable('torch' in capabilities && !!capabilities.torch);
};
- const clearTorchConstraints = useCallback(() => {
+ const clearTorchConstraints = () => {
if (!trackRef.current) {
return;
}
trackRef.current.applyConstraints({
advanced: [{torch: false}],
});
- }, []);
+ };
- const onCapture = useCallback(
- (file: FileObject, filename: string, source: string) => {
- const transaction =
- isMultiScanEnabled && initialTransaction?.receipt?.source
- ? buildOptimisticTransactionAndCreateDraft({
- initialTransaction,
- currentUserPersonalDetails,
- reportID,
- })
- : initialTransaction;
- const transactionID = transaction?.transactionID ?? initialTransactionID;
- const newReceiptFiles = [...receiptFiles, {file, source, transactionID}];
+ const onCapture = (file: FileObject, filename: string, source: string) => {
+ const transaction =
+ isMultiScanEnabled && initialTransaction?.receipt?.source
+ ? buildOptimisticTransactionAndCreateDraft({
+ initialTransaction,
+ currentUserPersonalDetails,
+ reportID,
+ })
+ : initialTransaction;
+ const transactionID = transaction?.transactionID ?? initialTransactionID;
+ const newReceiptFiles = [...receiptFiles, {file, source, transactionID}];
- setMoneyRequestReceipt(transactionID, source, filename, !isEditing, file.type);
- setReceiptFiles(newReceiptFiles);
+ setMoneyRequestReceipt(transactionID, source, filename, !isEditing, file.type);
+ setReceiptFiles(newReceiptFiles);
- if (isMultiScanEnabled) {
- return;
- }
+ if (isMultiScanEnabled) {
+ return;
+ }
- if (isEditing) {
- updateScanAndNavigate(file, source);
- return;
- }
+ if (isEditing) {
+ updateScanAndNavigate(file, source);
+ return;
+ }
- submitReceipts(newReceiptFiles);
- },
- [isMultiScanEnabled, initialTransaction, currentUserPersonalDetails, reportID, initialTransactionID, receiptFiles, isEditing, submitReceipts, setReceiptFiles, updateScanAndNavigate],
- );
+ submitReceipts(newReceiptFiles);
+ };
- const getScreenshot = useCallback(() => {
+ const getScreenshot = () => {
if (!cameraRef.current) {
requestCameraPermission();
return;
@@ -298,9 +324,9 @@ function MobileWebCameraView({
endSpan(CONST.TELEMETRY.SPAN_RECEIPT_CAPTURE);
onCapture(file, filename, source);
});
- }, [isMultiScanEnabled, showBlink, requestCameraPermission, onCapture]);
+ };
- const capturePhoto = useCallback(() => {
+ const capturePhoto = () => {
if (trackRef.current && isFlashLightOn) {
trackRef.current
.applyConstraints({
@@ -316,7 +342,7 @@ function MobileWebCameraView({
}
getScreenshot();
- }, [isFlashLightOn, getScreenshot, clearTorchConstraints]);
+ };
return (
<StepScreenWrapper
| setVideoConstraints(undefined); | ||
| return; | ||
| } | ||
| navigator.permissions |
There was a problem hiding this comment.
NAB/suggestion: query camera permission once on module load to pre-warm the permissions and use them in initial state on the first render
| setIsQueriedPermissionState(true); | ||
| }); | ||
| // We only want to get the camera permission status when the component is mounted | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
This suppression is making React Compiler skip this component. The simple fix is to just add requestCameraPermission to the dependency array. Because it's wrapped in useCallback(() => {}, []), it's referentially stable and won't trigger additional executions of this effect.
|
|
||
| useEffect(() => { | ||
| if (!isTabActive) { | ||
| setVideoConstraints(undefined); |
There was a problem hiding this comment.
Looks like this is the only place where setVideoConstraints is set to undefined. We can potentially eliminate an unnecessary re-render by splitting videoConstraints into a separate state variable maybe called deviceConstraints or something. Then in render, derive it to undefined if !isTabActive. This prevents the extra setState call in the effect and fixes the lint warning.
|
Oh, and also it looks like if you make the changes I suggested, you can drop |
Sure thing to do. |
|
|
|
@roryabraham Please take a look. |
| .then((permissionState) => { | ||
| setCameraPermissionState(permissionState.state); | ||
| if (permissionState.state === 'granted') { | ||
| queryCameraPermission() |
There was a problem hiding this comment.
A few comments from me about this:
- Even after we have the cached state, we still doing the state update here
- We add an event listener at the module level, which never being cleaned up
cc: @roryabraham
There was a problem hiding this comment.
Both good comments.
Even after we have the cached state, we still doing the state update here
Yes, I think it's ok to have setState in a useEffect if it's being used to sync React with a external systems like browser APIs.
We add an event listener at the module level, which never being cleaned up
Good point, but I think because it's module level it's ok - if the component is remounted we will just keep the one existing subscription. I believe module import is internally memoized by most (all?) JS runtimes, so the import is idempotent.
So I think this is ok.
|
🚧 @roryabraham 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! 🧪🧪
|


Explanation of Change
Fixed Issues
$ #79929
PROPOSAL: N/A
Tests
"Upload receipts or drag and drop them here".
Offline tests
QA Steps
Same as test
// TODO: These must be filled out, or the issue title must include "[No QA]."
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari