Repository navigation
Conversation
…ConfirmationList # Conflicts: # src/components/MoneyRequestConfirmationList.tsx
|
@codex review |
![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: 168d14194b
ℹ️ 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".
|
a few hrs |
|
@situchan any news? |
|
Looks good so far |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios1.mp4ios2.mp4iOS: mWeb SafariMacOS: Chrome / Safariweb1.movweb2.movweb3.movweb4.mov |
|
Please fix conflict |
|
🤖 Review: the split is faithful. I diffed all six variants and the extracted hooks against the old 715-line monolith and found no dispatch, prop, or side-effect regression. Three things are worth your attention before merge — one test gap and two behavior changes that aren't refactors. 1. The distance footer's 3-way branch lost its only test coverageThe deleted The branch also got easier to break in the move: 2.
|
…ConfirmationList # Conflicts: # src/components/MoneyRequestConfirmationList/DistanceRequestController.tsx
|
🤖 Code quality review. The mechanical hygiene here is genuinely good — zero 1. The context bag un-memoizes the four controllers — a perf regression vs
|
situchan
left a comment
situchan
left a comment
There was a problem hiding this comment.
I performed full regression test in create expense / track expense / send invoice / pay / split / share flow.
No issues found
|
I would leave Melvin's feedback for a follow-up.
Yes, but the controllers are light, they read a few contexts and their effects only re-run when deps change. The heaviest part is
The controllers render before the layout on purpose, so their effects run before the footer fields effects. Moving them into the layout means we can change it and it should be tested. And the third point is better to do after the first two. |
|
🤖 Web test steps verification results: I found no confirmed regressions. Six of seven web flows passed: workspace Manual, manual Distance, Track in self DM, Submit to workspace, Pay someone, and Time. The one failure is the short-viewport scroll check. It is most likely a test-harness artifact, because the same scroll command also failed on unrelated screens (the onboarding feature list and the workspace More features page). If it's cheap, you could check the Manual confirmation page at about 1280×577 on
Code regression passNo merge regressions. Across the five merges from No behavior regressions. I checked remounts, effect order, callsite props, recomputed values, and values the variants now omit. The double-submit guard depends on the page's One behavior change, probably harmless. On the start page, the Manual tab embeds the confirmation list, and tabs stay mounted. If you switch Manual → Time → Manual (or to Distance in a split), the shared draft keeps its ID but changes type. The dispatcher then picks a different variant, so the list remounts. On
Both changes improve on |
|
@VickyStash can you please create issue for the follow ups if you dont plan to handle it here so we dont forget (you can use melvin) |
|
@MelvinBot Can you create a one follow-up issue for points 1, 2, 3 from this comment #101198 (comment). |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Large diff but looks like a fine refactoring
| const routeError = Object.values(transaction?.errorFields?.route ?? {}).at(0); | ||
| const isTypeSplit = iouType === CONST.IOU.TYPE.SPLIT; | ||
| const shouldShowReadOnlySplits = isPolicyExpenseChat || isReadOnly || isScanRequest; | ||
| // Both the validation gate and the clear gate below key off this, so it is computed once here rather than |
There was a problem hiding this comment.
We should be having empty line before comments unless its start of a block
| // Both the validation gate and the clear gate below key off this, so it is computed once here rather than | |
| // Both the validation gate and the clear gate below key off this, so it is computed once here rather than |
|
I created [Refactor] MoneyRequestConfirmationList follow-up: stabilize controller props, dedupe variant shell, single-path context. It's a sub-issue of [Refactor] Split The issue covers points 1–3, in that order. It includes your two caveats: the controllers must keep rendering before the layout, and point 3 is easier after points 1 and 2. I didn't find any existing issue for this work. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.95-0 🚀
|
|
No help site changes are required for this PR, so I did not create a docs PR. This PR is an internal refactor for performance. It splits What I checked
Since users see no change, the existing articles under |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.4.95-4 🚀
|
5 participants
Explanation of Change
Next step of splitting
IOURequestStepConfirmation.Step 1 (#100041) forked
MoneyRequestConfirmationListFooterper expense type behind a footer dispatcher. This PR does the same one level up:MoneyRequestConfirmationList.tsxbecomes anindex.tsxdispatcher plus six variants, and the footer dispatcher is deleted because each list variant now renders its own footer directly. The old double dispatch (list type → footer type) collapses into one.Perf measurements
PerDiem/Time wins most
Time
Per Diem
Manual
Fixed Issues
$ #99449
PROPOSAL: N/A
Tests
Every expense confirmation path goes through this component, so the matrix below is the test. For each row, open the confirmation step, verify the fields the type shows on
mainare present and no others, edit one field, and submit. Verify the submitted expense matches what was confirmed.Create, from global + or from a workspace chat
Track expense (self DM)
Edit
DynamicSplitBillDetailsPage). Verify the read-only list, the receipt, and editing amounts when the split is editable.SubmitDetailsPage). Verify the receipt shows and submits.Offline tests
Same, as in Tests section
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same, as in Tests section
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
Manual
manual.mp4
Scan / Multi-scan
scan.mp4
Distance, map
distance_map.mp4
Distance, manual
distance_manual.mp4
Distance, odometer
distance_odometr.mp4
Per diem
perdiem.mp4
Time
time.mp4
Split
spllit_scan.mp4
split_manual.mp4
Invoice
invoice.mp4
Pay
pay.mp4
Submit expense from self dm
submit_wk.mp4
Submit Per Diem from self dm
submit_perdiem.mp4
Split details
view_split.mp4
Share
share.mov