fix (batch 4): isNewManualExpenseFlowEnabled beta flag regressions - #98635
fix (batch 4): isNewManualExpenseFlowEnabled beta flag regressions#98635thelullabyy wants to merge 5 commits into
Conversation
|
@codex review |
|
@ikevin127 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] |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
| @@ -243,7 +245,7 @@ function IOURequestStartPage({ | |||
| // (wrong fields, and the "Confirm page shows per diem" bug). Wait for the reset so the manual confirmation | |||
| // mounts once against the rebuilt manual draft. | |||
| // The header and tab bar remain visible above this loader, so per UI-1 use ActivityIndicator (users can still go back) instead of FullScreenLoadingIndicator. | |||
| manualTabContent = ( | |||
| manualContent = ( | |||
| <View style={[styles.flex1, styles.fullScreenLoading]}> | |||
| <ActivityIndicator | |||
| testID="manualTabPendingReset" | |||
| @@ -252,7 +254,7 @@ function IOURequestStartPage({ | |||
| </View> | |||
| ); | |||
| } else { | |||
| manualTabContent = ( | |||
| manualContent = ( | |||
There was a problem hiding this comment.
🟠 src/pages/iou/request/IOURequestStartPage.tsx:228-256: the scan / per-diem loader branch is now reachable in the PAY flow, which has no tab-switch reset to ever clear it
const shouldEmbedConfirmation = isNewManualExpenseFlowEnabled && (shouldUseTab || iouType === CONST.IOU.TYPE.PAY);
let manualContent: React.ReactNode;
if (!shouldEmbedConfirmation) {
...
} else if (isScanRequest(transaction) || isPerDiemRequest(transaction)) {
// renders an ActivityIndicator foreverThe loader branch's own comment states its premise: "When switching from the Scan or Per diem tab, the shared draft is briefly still a scan/per-diem request until the tab-switch reset rebuilds it as manual." That reset is resetIOUTypeIfChanged, and it is wired in exactly one place, onTabSelected on line 298, inside the shouldUseTab ? (...) branch. The PAY flow renders no tabs, so nothing ever calls it.
That means for PAY the branch has no exit. If transaction.iouRequestType is scan or perDiem when the page mounts, the user sits on a spinner until they press back.
Reachability is narrow but real: the draft lives under the shared CONST.IOU.OPTIMISTIC_TRANSACTION_ID key, and startMoneyRequest only removes it when the caller passes draftTransactionIDs (clearMoneyRequest -> getRemoveDraftTransactionsByIDsData(draftTransactionIDs); an undefined list is a no-op).
QuickActionNavigation.ts:62 passes a variable that can be undefined. So: leave a Scan or Per diem draft in a chat, invoke "Pay someone" in that same chat, and the pay page can mount against the stale draft. The file already acknowledges this class of staleness with its latchedDraftStaleness guard, but that guard only feeds transactionRequestType, while this branch reads the raw transaction.
Suggested fix (one word, and it restores the branch to exactly the case its comment describes):
} else if (shouldUseTab && (isScanRequest(transaction) || isPerDiemRequest(transaction))) {Bug if not addressed: a user taps "Pay someone" and gets a permanent loading spinner with a working back button and nothing else. No error, no console warning, and it only reproduces after an unrelated scan or per diem in the same chat, which makes it nearly untriageable from a bug report.
| } | ||
|
|
||
| if (!trimmedMerchant) { | ||
| const isUntypedPlaceholder = !transaction?.isMerchantSet && isInvalidMerchantValue(trimmedMerchant); |
There was a problem hiding this comment.
🟡 src/components/MoneyRequestConfirmationList/hooks/useFormErrorManagement.ts:175: this predicate already exists verbatim in MerchantField.tsx, and the two must not drift
New:
const isUntypedPlaceholder = !transaction?.isMerchantSet && isInvalidMerchantValue(trimmedMerchant);Existing, src/components/MoneyRequestConfirmationList/sections/MerchantField.tsx:62:
const displayMerchantValue = !merchantState?.isMerchantSet && isInvalidMerchantValue(merchantValue) ? '' : merchantValue;Same rule, expressed twice: "a placeholder the user never typed counts as empty." One decides what the field shows, the other decides whether it is valid.
If they ever diverge you get the worst possible state: a field that renders empty but blocks confirmation with "please enter a valid merchant", or the reverse.
Suggested fix, extract next to isInvalidMerchantValue in src/libs/ValidationUtils.ts and use it in both places:
/** A merchant the user never typed: the flow seeded the "Expense" / "(none)" placeholder, so it should be treated as empty. */
function isUntypedPlaceholderMerchant(isMerchantSet: boolean | undefined, merchant: string): boolean {
return !isMerchantSet && isInvalidMerchantValue(merchant);
}I verified the blast radius is otherwise contained: isMerchantFieldValid has only two consumers, the clear-error effect at :207 and useConfirmationValidation.ts:226,232, and the widened branch only flips outcomes where useConfirmationValidation already declines to raise an error (else if (transaction?.isMerchantSet && !isMerchantFieldValid)).
So the fix itself is correct and minimal, including for server-set scan merchants of (none) where isMerchantSet is never written.
| let manualTabContent: React.ReactNode; | ||
| if (!isNewManualExpenseFlowEnabled) { | ||
| manualTabContent = ( | ||
| const shouldEmbedConfirmation = isNewManualExpenseFlowEnabled && (shouldUseTab || iouType === CONST.IOU.TYPE.PAY); |
There was a problem hiding this comment.
🟡 The pay quick action's SKIP_CONFIRMATION fast path is now dead, and I want to make sure that is a decision rather than a side effect
This line plus the last hunk in this file combine to stop IOURequestStepAmount from ever mounting in the pay flow:
- here,
shouldEmbedConfirmationistrueforiouType === PAYonce the beta is on - in the last hunk, the non-tab branch (the branch PAY renders through) stopped hardcoding the amount page and now renders
{manualContent}, which for PAY is<IOURequestStepConfirmation shouldHideHeader />
IOURequestStepAmount is the only screen in this flow that reads the flag, so the whole chain below goes unreachable from the quick action:
QuickActionNavigation.ts:61-62:CONST.QUICK_ACTIONS.SEND_MONEYcallsstartMoneyRequest(CONST.IOU.TYPE.PAY, reportID, draftTransactionIDs, undefined, true, ...), where the 5th arg isskipConfirmation, soSKIP_CONFIRMATION{OPTIMISTIC_TRANSACTION_ID}is written astruerequestTypeisundefined, sostartMoneyRequesthits itsdefault:case and navigates toROUTES.MONEY_REQUEST_CREATE, this pageIOURequestStepAmount.tsx:96is the only reader of that key,:153-159turns it intoshouldSkipConfirmation,:161-162callsusePreMountDestination(...),:243passes it intosubmitAmountIOUAmountSubmission.ts:449-453:if (shouldSkipConfirmation) { if (iouType === PAY || SEND) { submitSkipConfirmationPayment(...); return; } }
So the flag is still written and never read: the quick action used to go amount page then pay immediately, and now it always lands on the details page.
To be clear on scope, this is only the SEND_MONEY quick action (the green FAB button when the last action was paying someone). The + > Pay user menu path (AttachmentPickerWithMenuItems.tsx:283) passes no skipConfirmation, so it always showed the confirmation and is unaffected.
I think this is intended: the embedded confirmation carries the amount inline, so there is no separate step left to skip, and the same thing already happens to the SUBMIT quick actions under the beta. If so, all I want is a note here so the next person does not go hunting for the missing skip path:
| const shouldEmbedConfirmation = isNewManualExpenseFlowEnabled && (shouldUseTab || iouType === CONST.IOU.TYPE.PAY); | |
| // The pay quick action writes SKIP_CONFIRMATION, but IOURequestStepAmount is its only reader and no longer mounts | |
| // for PAY: the embedded confirmation carries the amount inline, so there is no separate step left to skip. | |
| const shouldEmbedConfirmation = isNewManualExpenseFlowEnabled && (shouldUseTab || iouType === CONST.IOU.TYPE.PAY); |
Could you also confirm on device that the pay quick action still feels like a quick action? That is the one path with no test coverage in this PR, and if it turns out users do lose a step there, it is a silent UX regression QA will not catch because the flow still completes.
| let manualTabContent: React.ReactNode; | ||
| if (!isNewManualExpenseFlowEnabled) { | ||
| manualTabContent = ( | ||
| const shouldEmbedConfirmation = isNewManualExpenseFlowEnabled && (shouldUseTab || iouType === CONST.IOU.TYPE.PAY); |
There was a problem hiding this comment.
🟢 src/pages/iou/request/IOURequestStartPage.tsx:228: iouType === CONST.IOU.TYPE.PAY leaves INVOICE as the only real exclusion, and it is worth saying why
shouldUseTab excludes SEND, PAY and INVOICE. SEND cannot actually reach this page: src/libs/Navigation/types.ts:2063+ types every money-request route param as Exclude<IOUType, typeof CONST.IOU.TYPE.REQUEST | typeof CONST.IOU.TYPE.SEND>. So in practice the new condition means "everything except invoice".
Two things:
- Why is INVOICE left on the amount-first flow? The exact double-step from Expense - Amount page is still displayed before landing on new flow when paying someone. #96598 exists there too (FAB -> Send invoice -> amount -> Next -> details), and Invoice - Invoice can be sent without date, which creates invoice room with infinite loading #96579 in this same PR is an invoice bug on that details page. If it is deliberate (invoices need the amount step to pick the sender workspace first) a one-line comment would prevent someone "finishing the job" later and regressing it.
- Since the intent is "embed unless invoice", consider expressing that directly so the condition does not read as an arbitrary allowlist:
// The invoice flow keeps the amount page as its landing step; every other type embeds the confirmation.
const shouldEmbedConfirmation = isNewManualExpenseFlowEnabled && iouType !== CONST.IOU.TYPE.INVOICE;Only adopt that form if INVOICE really is the sole exclusion. If you prefer to keep SEND guarded defensively, the current shape is fine, just add the comment.
| function mockMissingInvoicingDetails() { | ||
| const policyMock = jest.requireMock<{hasInvoicingDetails: typeof hasInvoicingDetails}>('@userActions/Policy/Policy'); | ||
| (policyMock.hasInvoicingDetails as jest.Mock).mockReturnValueOnce(false); | ||
| } |
There was a problem hiding this comment.
🟢 tests/unit/hooks/confirmAction.test.ts:17-21: The new helper keeps a cast that jest.mocked removes (no type casting allowed)
Centralising the three copies is the right call. Since you are already touching it, drop the as jest.Mock:
function mockMissingInvoicingDetails() {
jest.mocked(hasInvoicingDetails).mockReturnValueOnce(false);
}jest.mocked preserves the real signature, so if hasInvoicingDetails ever takes a second argument or returns a non-boolean, this fails at compile time instead of silently mocking the wrong shape.
as jest.Mock erases that.
| ...placeholderMerchantParams, | ||
| transaction: createMock<OnyxTypes.Transaction>({ | ||
| transactionID: 'txn1', | ||
| amount: 100, | ||
| merchant: CONST.TRANSACTION.DEFAULT_MERCHANT, | ||
| isMerchantSet: true, |
There was a problem hiding this comment.
🟢 tests/unit/hooks/useFormErrorManagement.test.tsx:184-203: async test with no await, and a fixture that fights itself
The #96593 test is declared async but never awaits anything (rerender and act are both sync here). That trips @typescript-eslint/require-await in the repo's config, so drop the keyword.
Separately, the middle test spreads placeholderMerchantParams and then overrides its main field:
...placeholderMerchantParams,
transaction: createMock<OnyxTypes.Transaction>({... isMerchantSet: true ...}),The fixture's whole point is isMerchantSet: false, so spreading it and immediately replacing it makes the reader diff two transaction literals to find the one bit that matters. Clearer as an explicit override of just that bit:
transaction: {...placeholderMerchantParams.transaction, isMerchantSet: true},
| * Seeds the beta, the manual tab selection and a draft transaction of the given request type, then renders the page. | ||
| */ | ||
| async function renderStartPageWithDraftType(iouRequestType: IOURequestType) { | ||
| async function renderStartPageWithDraftType(iouRequestType: IOURequestType, iouType: IOUType = CONST.IOU.TYPE.SUBMIT, isNewManualExpenseFlowEnabled = true) { |
There was a problem hiding this comment.
🟢 tests/ui/IOURequestStartPageManualTabTest.tsx:93: three positional params, two of which are opaque at the call site
async function renderStartPageWithDraftType(iouRequestType: IOURequestType, iouType: IOUType = CONST.IOU.TYPE.SUBMIT, isNewManualExpenseFlowEnabled = true)renderStartPageWithDraftType(CONST.IOU.REQUEST_TYPE.MANUAL, CONST.IOU.TYPE.PAY, false) reads as "manual, pay, false". A single options object keeps the call sites self-describing as this grows:
async function renderStartPage({iouRequestType, iouType = CONST.IOU.TYPE.SUBMIT, isNewManualExpenseFlowEnabled = true}: RenderStartPageOptions)Also worth one more assertion while you are in here: INVOICE keeps the amount page. That is the behavior most likely to be changed by accident later (see the 🟢 finding above), and it costs one test:
it('keeps the amount page as the landing page for the invoice flow', async () => {
await renderStartPageWithDraftType(CONST.IOU.REQUEST_TYPE.MANUAL, CONST.IOU.TYPE.INVOICE);
expect(screen.getByTestId(AMOUNT_TEST_ID)).toBeOnTheScreen();
});The new IOURequestStepAmount mock is correct, by the way: default and the named IOURequestStepAmountWithTransactionOnly both match the real module's exports (IOURequestStepAmount.tsx:332-333).
Reviewer Checklist
Screenshots/Videos96579.mov96593.mov96598.mov |
|
@thelullabyy Completed the PR Reviewer Checklist, the 3 issue's tests passed. I dropped (7) code comments, some concerning extended scope that could cause regressions and some code quality-related. Please tag me once addressed, I'll take another look and approve if all good ✅ |
|
Hey, I will recheck and fix your comments once GH issue is resolved. Currently, I couldn't pull code to my local... |
|
@ikevin127 It is ready for review again |
2 participants
Explanation of Change
Fixed Issues
$ #93854
$ #96598
$ #96593
$ #96579
PROPOSAL:
Tests
Verify these bugs are no longer reproducible
$ #96598
$ #96593
$ #96579
Offline tests
QA Steps
Verify these bugs are no longer reproducible
$ #96598
$ #96593
$ #96579
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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
96598.mov
96593.mov
96579.mov