refactor: extract common selection mode logic into shared hooks#92518
Conversation
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
…e payment select Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ 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". |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
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.
|
melvin-bot
Bot
requested review from
JmillsExpensify and
situchan
and removed request for
a team
|
I'll add the recordings later today. |
![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: 3460dabf33
ℹ️ 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".
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
…obile Signed-off-by: krishna2323 <belivethatkg@gmail.com>
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
Test cases look good to me, though no product review required.
|
Recordings added. @situchan could you please start reviewing? |
|
@situchan friendly bump |
|
On my list today |
|
Is this still on your list? |
|
oh sorry @Krishna2323 please fix conflict |
| policyID: moneyRequestReport?.policyID, | ||
| onPress: (params: PaymentActionParams) => confirmPaymentRef.current(params), | ||
| shouldHidePaymentOptions, | ||
| shouldShowApproveButton: false, |
There was a problem hiding this comment.
🤖 Behavior change to confirm (mobile): the shared hook hardcodes shouldShowApproveButton: false / shouldDisableApproveButton: false, but the old mobile useSelectionModeReportActions passed real values:
const shouldShowApproveButton = canApproveIOU(report, policy, reportMetadata, currentUserAccountID, transactions) && !hasOnlyPendingTransactions;
const shouldDisableApproveButton = shouldShowApproveButton && !isAllowedToApproveExpenseReport(report);usePaymentOptions uses these to append (or, when pay is hidden, return only) an Approve entry inside the Pay sub‑menu. With both pinned to false, that Approve item no longer appears in the mobile Pay sub‑menu.
In practice this looks harmless — mobile already renders a dedicated top‑level Approve action (hasApproveAction), and the desktop dropdown always passed false here, so this aligns mobile with desktop and removes a duplicate. Flagging only to confirm the removal of the sub‑menu Approve entry is intentional and that there's no report state where the sub‑menu was the only path to Approve.
|
🤖 Code review — clean refactor, no high-confidence bugs. I traced the extraction of selection-mode/dropdown money-report logic into the new shared hooks ( I left one inline comment on a concrete behavior change worth an explicit confirm, plus two low-confidence notes below. Behavior change to confirm (inline on Low-confidence notes (no action needed if intentional):
No blocking issues from my side. |
^ |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@situchan conflicts resolved |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
…ode payment Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@situchan could you please review this with priority? It's been 3 weeks since the PR was opened, and the longer it sits, the higher the chance of regressions since this is a refactor. Thanks! |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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". |
|
🤖 Re-review (post conflict-resolution) — still a clean, behavior-preserving extraction. I re-traced the shared hooks against
Payment, hold-menu, pay-invoice-vs-pay-request, KYC routing, delegate/locked-account guards, and search refresh are all byte-equivalent to the originals. Two things still worth an explicit confirm before merge: 1. Mobile submit/approve guards now include pending-delete transactions (medium confidence — likely intentional)
This matches the canonical desktop 2. Approve option dropped from the mobile pay sub-menu — already tracked in the open inline thread on Note: test coverage gap (no action strictly required)
No blocking issues from my side — both items above are "confirm it's intentional," not "this is broken." |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.movios2.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
🤖 Regression hunt — 2 behavior changes worth confirming, no new breakage found. I ran an adversarial sweep across every changed file (desktop dropdown −248 lines, mobile hook −381 lines, toolbar plumbing, and the new shared hooks), comparing each moved path against 1. Mobile submit/approve guards now include pending-delete transactions 2. Approve option dropped from the mobile pay sub-menu Specifically verified as NOT regressed (the traps I checked)
Net: a clean refactor. The only open questions are the two confirms above — neither is a hard break. |
|
🤖 Re-review — still a clean refactor. No new findings; CI green, Codex 👍. I re-read the full diff against the current head ( Only two intentional behavior changes remain open to confirm (both flagged previously, neither a hard break):
Both align mobile to the existing desktop behavior, so they look deliberate — just resolve the open inline thread to confirm intent. One non-blocking observation: on mobile, Net: no blocking issues from my side. The reviewer ( |
|
Codex Review: Didn't find any major issues. Hooray! 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". |
|
@Krishna2323 can you add a PR description that explains what the PR is doing and why. Thanks! |
|
🚧 puneetlath has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.4.25-0 🚀
|
|
@Krishna2323 This PR is failing in test 9 because of a regression issue #95026 
The issue is reproducible in: iOS Bug7195925_1782859559884.App_Freeze.mp4 |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.4.25-2 🚀
Bundle Size Analysis (Sentry): |
7 participants
Explanation of Change
Fixed Issues
$ #72502
PROPOSAL:
Tests
Prerequisites: Have a workspace with at least 2-3 expense reports containing multiple transactions. Some reports should have held expenses, some should be ready for approval/payment.
1. Mobile - Submit flow (selection mode)
2. Mobile - Submit with pending RTER
3. Mobile - Approve flow (selection mode)
4. Mobile - Approve with held expenses
5. Mobile - Pay flow (selection mode)
6. Mobile - Pay with held expenses
7. Mobile - Pay invoice
8. Mobile - Delegate access restriction
9. Mobile - Locked account
10. Desktop - Submit flow (selection mode)
11. Desktop - Approve flow (selection mode)
12. Desktop - Pay flow (selection mode)
13. Desktop - Pay with held expenses
14. Desktop - Workspace policy payment option
15. Regression - Search refresh after payment
Offline tests
QA Steps
Same as tests
Verify that no errors appear in the JS console
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
andorid_hybrid.mp4
Android: mWeb Chrome
android_mWeb.mp4
iOS: Native
ios_hybrid.mp4
iOS: mWeb Safari
ios_mWeb.mp4
MacOS: Chrome / Safari
web_chrome.mp4