fix (batch 1): isNewManualExpenseFlowEnabled beta flag regressions#96717
fix (batch 1): isNewManualExpenseFlowEnabled beta flag regressions#96717thelullabyy wants to merge 8 commits into
Conversation
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.
|
|
@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] |
thelullabyy
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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: 1516076ba6
ℹ️ 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".
Reviewer Checklist
Screenshots/VideosiOS: Native
MacOS: Chrome / Safari
|
| const policyIdReal = getIOURequestPolicyID(transaction, reportReal ?? transactionReport ?? participantReport); | ||
| // The self-DM a submissions-disabled workspace flow is seeded onto carries the placeholder '_FAKE_' policy; | ||
| // don't let that shadow the selected workspace chat's real policy, or this page's categories never load. See #96576. | ||
| const policyIdReal = getIOURequestPolicyID(transaction, reportHasRealPolicy(reportReal) ? reportReal : (transactionReport ?? participantReport ?? reportReal)); |
There was a problem hiding this comment.
🟢 Divergent fallback chains for the same reportHasRealPolicy guard
The helper is applied with a slightly different fallback in each file:
IOURequestStepCategory.tsx:77->reportHasRealPolicy(reportReal) ? reportReal : (transactionReport ?? participantReport ?? reportReal)IOURequestStepCategoryCreate.tsx:64/IOURequestStepConfirmation.tsx:170->... : (participantReport ?? reportReal)IOURequestStepTag.tsx:68/IOURequestStepTaxRatePage.tsx:64->... : participantReport
Each variant matches that file's pre-existing fallback set, so this is not a regression. But five subtly different expressions for one conceptual rule ("prefer a report with a real policy, else the participant report") is a maintenance hazard: the next person fixing this area has to diff five call sites to know they are equivalent.
Why it matters: reviewer/maintainer cognitive load, and the risk that a future edit "fixes" one site and forgets the others.
Suggestion (optional): consider a small resolver that encapsulates the preference order, e.g. pickReportForPolicy(reportReal, transactionReport, participantReport), returning the first with a real policy.
ℹ️ Not blocking.
| // On iOS, presenting this picker as a native <Modal> while it is embedded inside the create-expense RHP | ||
| // (itself a modal presentation) deadlocks the main thread - opening the picker and then interacting with the | ||
| // confirmation freezes the whole app (#96609 / #96550) | ||
| if (Platform.OS === 'ios') { |
There was a problem hiding this comment.
Agree with the AI reviewer here:
🟢 iOS overlay uses an inline zIndex literal and direct Platform fork
Two small style-guide points:
- The inline
{zIndex: 10}object is a magic number and a fresh literal on each render. Since this is a full-screen sibling that already paints on top by JSX order, verify thezIndexis actually needed; if it is, prefer a named style/StyleUtils value so the stacking intent is documented and reused. - Forking render output on
Platform.OS === 'ios'inline works, but the established pattern for platform-divergent rendering is generally a.ios.tsxsplit. Inline is acceptable here given how small the fork is; flagging only for consistency. Since the fork is driven by a constant, there is no hooks-ordering risk (bothuseLocalizeanduseThemeStylesrun before any early return), which is correct.
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
✅ Completed checklist and review, found
|
Checking... |
|
@ikevin127 It is ready for second round review |
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
The latest fix works, just a refactoring request since I noticed we're now double-going against the "platform-specific code MUST be placed in dedicated files and folders ... reject any PR that attempts to put platform-specific code anywhere else" rule 👇
| {isNewManualExpenseFlowEnabled && isParticipantPickerVisible && ( | ||
| {/* On iOS the picker is a plain overlay we mount only when visible; on web/Android it is a self-animating | ||
| react-native-modal that must stay mounted while hidden so it can animate out and fire onModalHide. */} | ||
| {isNewManualExpenseFlowEnabled && (getPlatform() !== CONST.PLATFORM.IOS || isParticipantPickerVisible) && ( |
There was a problem hiding this comment.
🟠 Two inline platform checks now, both should move into dedicated platform files (incl. src/components/ParticipantPicker.tsx, the if (Platform.OS === 'ios') block)
First off, the fix itself works, the iOS overlay dodges the nested-modal deadlock, and the follow-up commit correctly restored the web/Android Modal animation. No complaints on the behavior 🟢
The concern is structural. As of the latest commit there are now two inline platform branches, so this goes against CROSS-PLATFORM.md twice over:
ParticipantPicker.tsx->if (Platform.OS === 'ios')picks overlay vsModal.IOURequestStepConfirmation.tsx:978->getPlatform() !== CONST.PLATFORM.IOSgates the mount.
The doc is explicit: "platform-specific code MUST be placed in dedicated files and folders ... reject any PR that attempts to put platform-specific code anywhere else." A comment explains the intent but doesn't grant the exception, that's reserved for a sign-off from multiple internal engineers.
And the second check is really a symptom of the first: because the picker forks on platform internally, the parent had to add its own platform gate to keep the hidden Modal mounted on web. That's platform awareness leaking up into the business logic, which is exactly what this rule exists to stop.
Both checks disappear if we push the split into platform files:
src/components/ParticipantPicker/
BaseParticipantPicker.tsx // shared ScreenWrapper + MoneyRequestParticipantsSelector body
index.tsx // web/Android: RIGHT_DOCKED Modal, stays mounted while hidden, animates via isVisible
index.ios.tsx // iOS: absolute-fill View overlay, returns null when !isVisible
Then the parent goes back to a plain render with zero platform awareness:
{isNewManualExpenseFlowEnabled && (
<ParticipantPicker ... isVisible={isParticipantPickerVisible} ... />
)}Mount-while-hidden vs mount-when-visible becomes each platform file's own business, and the confirmation step no longer needs to know what platform it's running on. If we'd rather keep the inline version, can we get an explicit sign-off from another engineer on the issue per the doc, instead of leaving it in a comment ?
Anchor on the if (Platform.OS === 'ios', ParticipantPicker.tsx (line 116) as the root cause; the IOURequestStepConfirmation.tsx:978 gate can just get a one-line reply pointing back here.
▎ Reviewed with AI assistance; the call and wording are mine.
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required since we're fixing regressions.
3 participants
Explanation of Change
Fixed Issues
$ #93854
$ #96550
$ #96609
$ #96576
$ #96668
$ #96578
PROPOSAL:
Tests
Verify that these bugs are not reproducible when new manual expense flow beta flag is enabled
#96550
#96609
#96576
#96668
#96578
Offline tests
QA Steps
Verify that these bugs are not reproducible when new manual expense flow beta flag is enabled
#96550
#96609
#96576
#96668
#96578
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
issue-96609.mov
issue-96550.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
issue-96576.mov
issue-96668.mov
issue-96578.mov