refactor: Move startSplitBill navigation handling into the UI - #100032
refactor: Move startSplitBill navigation handling into the UI#100032TaduJR wants to merge 1 commit into
Conversation
|
@linhvovan29546 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] |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edf94e6995
ℹ️ 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".
| taxAmount: transactionTaxAmount, | ||
| taxValue: transactionTaxValue, | ||
| shouldPlaySound: index === transactions.length - 1, | ||
| optimisticSplitChatReportID, |
There was a problem hiding this comment.
Preserve earlier scans when the shared chat write is deferred
When multiple receipts are submitted while a SEARCH or DISMISS_MODAL deferred-write channel is active, every loop iteration reuses this ID before the first startSplitBill optimistic write has executed. Each call therefore concludes that the chat is new and prepares Onyx.METHOD.SET updates for the same report-actions key; when the deferred callbacks run, a later scan replaces the earlier scan's created and IOU actions, leaving the first split invisible while offline and also queueing multiple create-chat requests for the same report. The shared chat must be established once before subsequent split payloads are built, or subsequent writes must merge into it rather than independently treating it as new.
Useful? React with 👍 / 👎.
Explanation of Change
This is part of the ongoing work to move navigation out of our actions and into the UI, so an action only writes data and the screen decides where to send the user.
Before this change,
startSplitBillcreated the split and also navigated to the split chat. This PR takes the navigation out of the action. The two screens that start a scan split (the confirmation step and the scan skip-confirmation) now handle it themselves. Each screen works out the destination chat before calling the action and passes it in, so it navigates to the exact chat the split was created in. When several receipts are split at once, they now share one chat and the user is taken there a single time after the last one.startSplitBillno longer returns a value either, since only tests were reading it.Fixed Issues
$ #84631
PROPOSAL: #84631 (comment)
Tests
Test 1: Split one scanned receipt with one person (direct message)
Test 2: Split several scanned receipts at once
Test 3: Split a scanned receipt with a group
Test 4: Split with a brand-new group of people
Test 5: Quick-action "Split receipt" (skips the details screen)
Test 6: Offline behavior
Offline tests
Same as tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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