Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
I have a few concerns with the new components. FileUpload is almost identical to DesktopWebUploadView, while CameraCapture is almost identical to MobileWebCameraView. However, there are a few differences between the old and new components that I think are worth considering.
In FileUpload, we don't have the We always show blink, even though we only show it when multi-scan is enabled. (maybe expected) PDFValidationComponent is removed from both files. (maybe expected too because we can put it in the parents) ReceiptPreviews is removed in CameraCapture. etc. From what I understand, we want to create a more focused component (Variant) to reduce unnecessary onyx subscription, which we currently subscribe to many data in For example, showBlink and blinkStyle for mobile view. |
|
Thanks for review! These are all intentional and will make more sense once PR 3a/3b land. Existing hooks Screen wrappers That it the main idea - Also here is the full plan for the split, for reference -> #85583 (comment) Do you see any capture/upload logic that should remain inside |
melvin-bot
Bot
requested review from
bernhardoj and
trjExpensify
and removed request for
a team
|
@bernhardoj 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] |
|
@bernhardoj also please note, I am going be OOO for a week, but someone from the team will jump to answers all concerns and move it forward |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return; | ||
| } | ||
| queryCameraPermission() | ||
| .then((state) => { |
There was a problem hiding this comment.
❌ PERF-15 (docs)
This useEffect performs async work via queryCameraPermission().then(...) which calls setCameraPermissionState and setIsQueriedPermissionState, but has no cleanup mechanism to discard stale responses. When isTabActive toggles rapidly (e.g., quick tab switches), a slow earlier promise can resolve after a faster later one and overwrite the correct permission state.
Add an ignore flag to prevent stale responses from updating state:
useEffect(() => {
if (!isTabActive) {
return;
}
let ignore = false;
queryCameraPermission()
.then((state) => {
if (ignore) return;
setCameraPermissionState(state);
if (state === 'granted') {
requestCameraPermission();
}
})
.catch(() => {
if (ignore) return;
setCameraPermissionState('denied');
})
.finally(() => {
if (ignore) return;
setIsQueriedPermissionState(true);
});
return () => { ignore = true; };
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [isTabActive]);Reviewed at: 81d47b7 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| fill={isFlashLightOn ? theme.white : theme.icon} | ||
| /> | ||
| </PressableWithFeedback> | ||
| </View> |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
Hardcoded icon sizes 16 and 32 are used throughout this file (lines 362, 395, 426, 442) instead of the named constants already defined in the codebase: variables.iconSizeSmall (16) and variables.iconSizeMenuItem (32).
Replace the magic numbers with the existing named constants:
import variables from '@styles/variables';
// Instead of height={16} width={16}
height={variables.iconSizeSmall}
width={variables.iconSizeSmall}
// Instead of height={32} width={32}
height={variables.iconSizeMenuItem}
width={variables.iconSizeMenuItem}Reviewed at: 81d47b7 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| onPress={() => setFlash((prevFlash) => !prevFlash)} | ||
| > | ||
| <Icon | ||
| height={16} |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
Same as in CameraCapture.tsx: hardcoded icon sizes 16 and 32 are used throughout this file (lines ~394, ~429, ~461, ~477) instead of the named constants variables.iconSizeSmall (16) and variables.iconSizeMenuItem (32).
Replace with the existing named constants:
import variables from '@styles/variables';
height={variables.iconSizeSmall}
width={variables.iconSizeSmall}
height={variables.iconSizeMenuItem}
width={variables.iconSizeMenuItem}Reviewed at: 81d47b7 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
We previously used variables.iconSizeSmall here
| <View | ||
| style={[styles.uploadFileViewTextContainer, styles.userSelectNone]} | ||
| // eslint-disable-next-line react/jsx-props-no-spreading, react-hooks/refs | ||
| {...panResponderRef.current.panHandlers} |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line react/jsx-props-no-spreading, react-hooks/refs comment lacks a justification explaining why these rules are disabled.
Add a comment explaining the reason, for example:
{/* PanResponder handlers must be spread onto the View for gesture recognition */}
{/* eslint-disable-next-line react/jsx-props-no-spreading, react-hooks/refs */}
{...panResponderRef.current.panHandlers}Reviewed at: 81d47b7 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| * Camera — web entry point. | ||
| * On mobile browsers renders a camera viewfinder (CameraCapture). | ||
| * On desktop browsers renders a drag-and-drop / file-picker upload area (FileUpload). | ||
| */ |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line react/jsx-props-no-spreading comments at lines 11 and 19 lack justification explaining why the rule is disabled.
Add a comment explaining the reason, for example:
{/* Props are forwarded as-is to the platform-specific variant */}
{/* eslint-disable-next-line react/jsx-props-no-spreading */}
{...props}Reviewed at: 81d47b7 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81d47b7aa4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * Multi-scan is a camera-based mobile interaction; desktop uses file picker with shouldAcceptMultipleFiles instead. | ||
| */ | ||
| function MultiScanGate({children}: MultiScanProviderProps) { | ||
| if (isMobile()) { |
There was a problem hiding this comment.
MultiScanGate currently uses isMobile() to decide whether to mount MultiScanProvider, but in native bundles @libs/Browser/index.ts returns false for isMobile(). That means native screens will bypass the provider and useMultiScanState()/useMultiScanActions() fall back to defaults, so multi-scan never becomes available once this component is wired in. Please switch this gate to a platform check that includes native (not only mobile web).
Useful? React with 👍 / 👎.
| const [dismissedProductTrainingResult] = useOnyx(ONYXKEYS.NVP_DISMISSED_PRODUCT_TRAINING); | ||
| const [draftTransactionIDs] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_DRAFT, {selector: validTransactionDraftIDsSelector}); | ||
|
|
||
| const canUseMultiScan = iouType !== CONST.IOU.TYPE.SPLIT; |
There was a problem hiding this comment.
canUseMultiScan now only checks iouType !== SPLIT, but existing scan logic (hooks/useMobileReceiptScan.ts) also requires the CREATE/start-scan context. With this condition, STEP_SCAN edit/replace flows can incorrectly show the multi-scan toggle, and toggling it calls removeTransactionReceipt/removeDraftTransactionsByIDs, which can clear draft receipt state unexpectedly during edit flows. Preserve the previous start-flow guard when deriving canUseMultiScan.
Useful? React with 👍 / 👎.
| setDeviceConstraints(defaultConstraints); | ||
| return; | ||
| } | ||
| navigator.mediaDevices.enumerateDevices().then((devices) => { |
There was a problem hiding this comment.
In requestCameraPermission, enumerateDevices() is called without a rejection handler. If the browser rejects enumeration (which can happen on some permission/device states), deviceConstraints is never set and the UI can stay in the loading state (cameraPermissionState === 'granted' while constraints remain empty). Add a .catch fallback to defaultConstraints as done in the existing useWebCamera implementation.
Useful? React with 👍 / 👎.
I think we should keep using both |
| return ( | ||
| <View | ||
| style={styles.flex1} | ||
| onLayout={() => onLayout?.()} |
There was a problem hiding this comment.
I think we can just do
| onLayout={() => onLayout?.()} | |
| onLayout={onLayout} |
| > | ||
| <View style={[styles.flex1]}> | ||
| {cameraPermissionStatus !== RESULTS.GRANTED && ( | ||
| <View style={[styles.cameraView, styles.permissionView, styles.userSelectNone]}> |
There was a problem hiding this comment.
In main, we have a conditional style for landscape mode.
| > | ||
| <Icon | ||
| height={32} | ||
| width={32} |
There was a problem hiding this comment.
Let's use variables.iconSizeMenuItem here
| > | ||
| <Icon | ||
| height={32} | ||
| width={32} |
There was a problem hiding this comment.
Let's use variables.iconSizeMenuItem here
| > | ||
| <Icon | ||
| height={32} | ||
| width={32} |
There was a problem hiding this comment.
Let's use variables.iconSizeMenuItem here
| <View style={[styles.flexRow, styles.justifyContentAround, styles.alignItemsCenter, styles.pv3]}> | ||
| <AttachmentPicker | ||
| onOpenPicker={() => { | ||
| setIsAttachmentPickerActive(true); |
There was a problem hiding this comment.
Looks like we removed setIsLoaderVisible from here. I see that this loader context is used for attachment-related features. I think at least we should add a callback here so each variant can call setIsLoaderVisible (it'll be duplicated code though).
| </View> | ||
| <DragAndDropConsumer onDrop={handleDrop}> | ||
| <DropZoneUI | ||
| icon={lazyIcons.SmartScan} |
There was a problem hiding this comment.
What's the plan for the "replace" case here?
Maybe we can pass the icon and dropTitle as props?
| style={[styles.flex1, styles.chooseFilesView(false)]} | ||
| > | ||
| <View style={[styles.flex1, styles.alignItemsCenter, styles.justifyContentCenter]}> | ||
| {!isDraggingOver && ( |
There was a problem hiding this comment.
We previously also used isDraggingOverWrapper from the StepScreenDragAndDropWrapper.
So, in prod, we get the isDraggingOver state from 2 providers.
1 from IOURequestStartPage, and 1 from the StepScreenDragAndDropWrapper. In this PR, we only get it from IOURequestStartPage.
|
|
||
| return ( | ||
| <View | ||
| onLayout={() => onLayout?.()} |
There was a problem hiding this comment.
| onLayout={() => onLayout?.()} | |
| onLayout={onLayout} |
| size={CONST.ACTIVITY_INDICATOR_SIZE.LARGE} | ||
| style={[styles.flex1]} | ||
| color={theme.textSupporting} | ||
| reasonAttributes={{context: 'CameraCapture', cameraPermissionState, videoConstraintsReady: !isEmptyObject(videoConstraints)}} |
There was a problem hiding this comment.
Looks like the last attribute is different than what we had before (I think we should use both though)
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
3 participants
Explanation of Change
This is PR 2 of 5 in the
IOURequestStepScandecomposition series (see #85571 for the full split plan).PR 1 (#87083) and PR 2 can land in parallel - they have no shared files
On the PR we extract a unified Camera component with platform variants and MultiScan infrastructure. All new files - no existing code is modified (except adding the ScanRoute type to types.ts).
New files:
These components are not yet consumed - they will be wired up in PR 3
Fixed Issues
$ #85583 (comment)
PROPOSAL:
Tests
Offline tests
QA Steps
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