feat: remove isNewManualExpenseFlowEnabled beta flag#96228
Conversation
|
@ZhenjaHorbach 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] |
|
Hey @ZhenjaHorbach, sorry but I think @ikevin127 will be reviewer of this PR. We worked on this but got reverted here #94134 |
![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: 09e08eaa33
ℹ️ 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".
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-06-21.at.18.17.32.mov |
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
Rework Verification — #96228 vs reverted #94134 (issue #93854)
What happened with the original PR
#94134 (identical title, same issue #93854) was merged 2026-06-22, deployed to staging as 9.4.17-0, and immediately generated ~18 deploy blockers, then was reverted. The rework #96228 is functionally the same flag removal. The re-land strategy is the standard one: each blocker bug was fixed on main while the beta flag still shielded production, and #96228 now flips the flag to expose those already-fixed paths. All 18 blockers are now CLOSED or Awaiting Payment, which means their fixes were merged and QA'd. So the rework is only safe if (a) every fix is actually in main, and (b) the flag flip exposes the fixed path and doesn't reintroduce old code.
⚠️ The one structural difference between the two PRs
The reverted #94134 modified shouldEnableNegative in src/libs/ReportUtils.ts (dropped the isNewManualExpenseFlow param and applied the P2P-recipient negative exclusion to all callers). #96228 does NOT touch ReportUtils.ts — it hardcodes true only at the AmountField call site (AmountField.tsx:122) and leaves every other caller (including the submission path IOUAmountSubmission.ts:209) on the old default-false behavior.
This turns out to be OK, because the negative-amount blocker is now fixed by a different mechanism that lives in main and is exposed by this PR:
IOURequestStepConfirmation.tsx:994→shouldExcludeP2P={(transaction?.amount ?? 0) < 0}on the now-unconditionally-renderedParticipantPicker- which flows to
ParticipantSearchResults.tsx:191/242→includeP2P/includeUserToInvite: ... && !shouldExcludeP2P - plus the
isValidMoneyRequestAmount(iouAmount, iouType, true, isP2P)guard inuseConfirmationValidation.ts
So the negative case is guarded at the picker/validation layer rather than in shouldEnableNegative. Recommend the author still sanity-check that there's no path to a negative P2P submission that bypasses the picker exclusion, since the shouldEnableNegative divergence means the two PRs are not behaviorally identical there.
Blocker-by-blocker verification
✅ Verified fixed in the exposed code path (found in current code / this diff):
| # | Blocker | Fix confirmed |
|---|---|---|
| 94261 | Negative amount submittable to user from self DM | shouldExcludeP2P={amount<0} wired through ParticipantPicker → ParticipantSearchResults (includeP2P/includeUserToInvite gated) — verified end-to-end |
| 94244 | Distance expense created when date removed | shouldShowConfirmationDate returns true for isDistanceRequest, and useConfirmationValidation now unconditionally blocks on shouldShowDate && !isReadOnly && isCreatedMissing(transaction) |
| 94266 | "This field is required" shown twice | useFormErrorManagement dedups common.error.fieldRequired/invalidMerchant inline (in diff) |
| 94264 | Tax decimal removed → replaced with 0 | TaxFields.tsx numeric-compare useEffect ("must not be overwritten while typing") now unconditional (in diff) |
| 94246 | Clicking outside RHP doesn't close creation RHP | onBackdropPress={() => Navigation.dismissModal()} on the now-unconditional ParticipantPicker (in diff) |
| 94263 | Invoice reselect same user → wrong RHP | useReportSelectionActions simplified to Navigation.goBack(backTo) (in diff) |
| 94233 / 94273 | Autofocus / fields unfocus after each input | AmountField.tsx autofocus useEffect (deferred focus past picker-close) present in diff |
🔶 FOR PR AUTHOR: Cannot verify statically — the residual re-revert risk (requires explicit re-test on this build):
| # | Blocker | Why it needs manual re-test |
|---|---|---|
| 94289 (OPEN) | Distance expense to a new/invited user shows error | Depends on invite-participant handling (users with no reportID/accountID); not verifiable by reading web code |
| 94282 (OPEN, Overdue) | Submissions disabled → expense created in wrong report with error | Depends on useDefaultParticipants auto-report logic (shouldAutoReport/getPolicyExpenseChat), which this PR modifies — highest-risk of the set |
| 94255 | Android: cursor before negative sign, number disappears | Native NumberWithSymbolForm behavior — Android only |
| 94258 | Android: unable to enter multi-line description | Native DescriptionField behavior — Android only |
| 94274 | Android & iOS: description field not visible when focused | Native scroll-into-view — device only |
| 94262 | Keyboard closes after entering number, report opens in background | Native keyboard/nav timing |
| 94259 | Android: returns to workspace chat after QAB flow | Native navigation |
| 94271 | iOS: returns to "Spend" briefly after closing confirm page | iOS navigation timing |
| 94234 | Referral RHP only opens after returning from participant list | Navigation timing |
| 94260 | Android: referral banner behind navigation bar | Native layout / safe-area |
Verdict
The rework is structurally sound and most blockers are provably addressed in the exposed code paths — this is a real re-land (fixes on main + flag flip), not a blind re-merge of the reverted change. But two things keep it from being a clean "safe to merge":
- The flag flip re-exposes all 18 blocker scenarios at once. ~7 are verified in code; the remaining ~11 (all the native-platform and navigation-timing ones, plus the two still-open feature bugs 94282 and 94289) can only be confirmed by re-running those exact repro steps on this build, on Android native, iOS native, and mWeb. Given the original had zero screenshots filled in and was reverted for exactly these, I'd block merge on a completed all-platform QA pass against the full blocker list — not "Same as tests."
- 94282 is the single highest risk: it's still OPEN + Overdue (seems a fix was shipped since payment is due), this issue is about the
useDefaultParticipantsauto-report path that this PR directly edits, and it's a "doesn't exist in prod" new-feature bug — exactly the kind of thing that slips through unit tests and re-triggers a deploy blocker.
Recommendation to the author: in the PR's Tests/QA sections, replace the generic 4-step script with the explicit repro steps from #94282, #94289, #94261, #94244 and the Android-native ones (#94255, #94258, #94274), and attach the all-platform videos. That's the concrete difference between this landing and getting reverted a second time.
cc @thelullabyy
|
@ikevin127 I uploaded recordings for regressions, kindly check again |
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM - Tests well
cc @puneetlath coming from the issue, if you could assign me as I was reviewing on the previously now reverted PR, then if all good to you, we can merge.
3 participants
Explanation of Change
Fixed Issues
$ #93854
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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
Screen.Recording.2026-07-17.at.18.53.35.mov
Screen.Recording.2026-07-17.at.18.54.33.mov
Screen.Recording.2026-07-17.at.18.55.23.mov
Android: mWeb Chrome
iOS: Native
Screen.Recording.2026-07-17.at.18.49.29.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-06-21.at.15.43.27.mov
Screen.Recording.2026-07-17.at.18.42.39.mov
Screen.Recording.2026-07-17.at.18.43.43.mov
Screen.Recording.2026-07-17.at.18.44.31.mov