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. |
| <ReceiptPreviews | ||
| {isMobileWeb ? ( | ||
| <MobileWebCameraView | ||
| receiptScan={receiptScan} |
There was a problem hiding this comment.
I don't really like the way we pass everything from useReceiptScan hook to the component here. Can we pass the individual prop that we needed? Don't include the one that we don't need.
…pScan-clean-up-p3
|
@bernhardoj Please take a look. |
|
@samranahm you’ve got conflicts |
…pScan-clean-up-p3 # Conflicts: # src/pages/iou/request/step/IOURequestStepScan/index.native.tsx # src/pages/iou/request/step/IOURequestStepScan/index.tsx
|
@samranahm I did some tidying up and updating the test. You can apply the patch here. I'm testing now the app... |
|
The drag and drop area is stuck. dnd.stuck.mp4Other platforms work fine so far. NOTE: I can't test adding the receipt image in iOS because of this issue Screenshots/VideosAndroid: HybridAppandroid.mp4Android: mWeb Chromeandroid.mweb.mp4iOS: HybridAppiOS: mWeb Safariios.mweb.mp4MacOS: Chrome / Safari |
Oh, this actually happens on cc: @roryabraham |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.mp4Android: mWeb Chromeandroid.mweb.mp4iOS: HybridAppiOS: mWeb Safariios.mweb.mp4MacOS: Chrome / Safariweb.mp4 |
|
Resolving merge conflicts. |
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. |


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