Decompose scan pr3 variants router#89293
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
melvin-bot
Bot
requested review from
a team,
bernhardoj and
joekaufmanexpensify
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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| if (!isMultiScanEnabled) { | ||
| removeDraftTransactionsByIDs(Object.keys(draftTransactionIDs ?? {}), true); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The processReceipts function body is nearly identical across ScanFromReport, ScanGlobalCreate, and ScanSkipConfirmation. All three share the same pattern: early return on empty files, conditionally calling removeDraftTransactionsByIDs, then calling buildReceiptFiles with the same parameter shape. This duplicated logic increases maintenance overhead — a bug fix or change must be applied in three places.
Extract the shared receipt-processing logic into a reusable utility or hook, e.g.:
// utils/processReceiptFiles.ts
function processReceiptFiles({files, isMultiScanEnabled, draftTransactionIDs, ...buildParams}) {
if (files.length === 0) return [];
if (!isMultiScanEnabled) {
removeDraftTransactionsByIDs(Object.keys(draftTransactionIDs ?? {}), true);
}
return buildReceiptFiles(buildParams);
}Then each variant calls this shared function and only adds its own navigation/submission logic.
Reviewed at: aa8ed07 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
I think we can move the logic to buildReceiptFiles instead of creating another util.
Also, we check for isStartingScan before calling removeDraftTransactionsByIDs.
App/src/pages/iou/request/step/IOURequestStepScan/hooks/useReceiptScan.ts
Lines 177 to 179 in cab0b7b
| currentUserEmail: currentUserPersonalDetails.login, | ||
| backToReport, | ||
| shouldGenerateTransactionThreadReport: false, | ||
| isASAPSubmitBetaEnabled, |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The string 'USD' is a magic value used as a currency fallback. The codebase already defines CONST.CURRENCY.USD for this purpose.
Replace the magic string with the named constant:
currency: transaction?.currency ?? CONST.CURRENCY.USD,Reviewed at: aa8ed07 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const firstReceiptFile = files.at(0); | ||
|
|
||
| // Split bill flow | ||
| if (iouType === CONST.IOU.TYPE.SPLIT && firstReceiptFile) { |
There was a problem hiding this comment.
❌ PERF-15 (docs)
This useEffect calls getLocationPermission().then(...) which triggers getCurrentPosition and setUserLocation. The dependency array [transaction?.amount, iouType] means this effect can re-fire. If the deps change while the previous async chain is still in flight, the stale callback will still execute clearUserLocation() and setUserLocation(), potentially overwriting a more recent result.
Add an ignore flag to discard stale responses:
useEffect(() => {
let ignore = false;
const gpsRequired = transaction?.amount === 0 && iouType !== CONST.IOU.TYPE.SPLIT;
if (!gpsRequired) {
return;
}
getLocationPermission().then((status) => {
if (ignore || (status !== RESULTS.GRANTED && status !== RESULTS.LIMITED)) {
return;
}
clearUserLocation();
getCurrentPosition(
(successData) => {
if (!ignore) {
setUserLocation({longitude: successData.coords.longitude, latitude: successData.coords.latitude});
}
},
() => {},
);
});
return () => { ignore = true; };
}, [transaction?.amount, iouType]);Reviewed at: aa8ed07 | 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: aa8ed07830
ℹ️ 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".
| startScanProcessSpan(isMultiScanEnabled); | ||
| navigateGlobalCreate(receiptFiles); |
There was a problem hiding this comment.
When isMultiScanEnabled is true, this branch still navigates immediately after every capture, so the scan screen exits after the first shutter press instead of letting users collect multiple camera shots and submit them together. In practice this regresses multi-scan behavior on mobile/native for the create flow (the same eager navigation pattern is also present in the other scan variants), while only gallery multi-select continues to support batching.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| if (!isMultiScanEnabled) { | ||
| removeDraftTransactionsByIDs(Object.keys(draftTransactionIDs ?? {}), true); |
There was a problem hiding this comment.
validTransactionDraftIDsSelector already returns a string[] of transaction IDs, but wrapping it with Object.keys(...) converts it to array indexes ("0", "1", ...). As a result removeDraftTransactionsByIDs() removes the wrong Onyx keys and leaves real draft transactions behind, which can leak stale drafts/receipts into subsequent scan flows.
Useful? React with 👍 / 👎.
| const [conciergeReportID] = useOnyx(ONYXKEYS.CONCIERGE_REPORT_ID); | ||
| const [allTransactionDrafts] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_DRAFT, {selector: validTransactionDraftsSelector}); | ||
| const [draftTransactionIDs] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_DRAFT, {selector: validTransactionDraftIDsSelector}); | ||
| const [shouldStartLocationPermissionFlow] = useOnyx(ONYXKEYS.NVP_LAST_LOCATION_PERMISSION_PROMPT); |
There was a problem hiding this comment.
This reads NVP_LAST_LOCATION_PERMISSION_PROMPT directly and later treats it as a boolean gate, but the stored value is a timestamp string, not a derived shouldPrompt flag. After a user has ever been prompted, the non-empty timestamp remains truthy and the permission modal will be shown on every eligible scan instead of respecting the intended cooldown threshold.
Useful? React with 👍 / 👎.
…cation permission selector
|
No product review needed |
|
🚧 @Julesssss has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.mp4Android: mWeb Chromeandroid.mweb.mp4iOS: HybridAppiOS: mWeb Safariios.mweb.mp4MacOS: Chrome / Safariweb.mp4 |
|
BUG: can't replace receipt from confirmation page Untitled.mp4 |
|
Added the fix, the ScanEditReceipt was always writing the new receipt to COLLECTION.TRANSACTION and calling the replaceReceipt API, for multi-scan creates draft transactions. So we were updating the wrong Onyx key, and the confirmation page showed the old image. SO now for drafts → we update the draft locally as well. For saved transactions -> we keep the existing replace-receipt API path. ReplaceFix.mp4Please check now |
| // Drafts (e.g. multi-scan transactions on the confirmation page) live in TRANSACTION_DRAFT and have no | ||
| // backend record yet, so we just update the draft locally. Saved transactions need the replace-receipt API call. | ||
| if (isDraftTransaction) { | ||
| setMoneyRequestReceipt(transactionID, source, file.name ?? '', true, file.type); |
There was a problem hiding this comment.
The bug is fixed, but checking the draft transaction could lead to another bug. If the transaction draft data exists, then it'll assume we are updating a draft transaction, even though we are replacing a created expense.
I think we should check the action instead. We render ScanEditReceipt either because backTo exists or action is edit.
If the action is edit, then we are replacing a created expense receipt.
bernhardoj
left a comment
bernhardoj
left a comment
There was a problem hiding this comment.
LGTM!
There is 1 small comment left, but NAB.
|
this is on you now, @Julesssss |
I will add that change in the last - follow up cleanup PR I resolved all new conflicts, waiting for final review |
|
Not sure what's going on with |
|
🚧 @cristipaval 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/cristipaval in version: 9.3.84-0 🚀
Bundle Size Analysis (Sentry): |
|
I reviewed the changes in this PR. This is a purely internal code refactoring that decomposes the monolithic No user-facing behavior is changed — all four scan flows (Edit Receipt, From Report, Global Create, Skip Confirmation) work identically before and after this change. No new UI elements, labels, or workflows were introduced. No help site documentation changes are required. |
8 participants
Explanation of Change
This is PR 3a and 3b combined of 5 in the IOURequestStepScan decomposition series (see #85583 for the full split plan).
PR 1 (#87083) and PR 2 (#87883) are merged.
The monolithic IOURequestStepScan had ~27 Onyx subscriptions shared across all scan flows, causing unnecessary re-renders and camera jank.
This PR decomposes it into a thin router (ScanRouter) that selects one of 4 focused variant components (ScanEditReceipt, ScanFromReport, ScanGlobalCreate, ScanSkipConfirmation), each subscribing only to the Onyx data it needs.
Shared logic is extracted into 6 small utility files (receipt file building, file source abstraction, telemetry span helper, file readability check, scan capture hook, GPS permission gate).
The old platform-split files (index.native.tsx, DesktopWebUploadView, MobileWebCameraView) are deleted - platform differences are now handled by the Camera component from PR 2.
Fixed Issues
$ #89383
$ #89384
PROPOSAL:
Tests
Flow 1: Edit Receipt
Flow 2: From Report
Flow 3: Global Create (FAB)
Flow 4: Skip Confirmation (Quick Action)
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
Scan Receipt:
https://github.com/user-attachments/assets/4689e74b-61b0-4a70-960b-ae797b685e49
Replace:
android-replace.mp4
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Scan Receipt:
https://github.com/user-attachments/assets/09c7a784-02dd-49a4-9e1a-e5a2f49dd9b8
Replace:
https://github.com/user-attachments/assets/f1a2c22c-781b-44fd-a62e-609587746f83