Split the confirmation footer into a dispatcher and per-type variants pt1 - #99578
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 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". |
|
@claude review |
|
@MelvinBot what do you think about the changes? |
|
Overall this is a clean, well-scoped execution of step 1 of the plan — the dispatcher + per-type variant split reads exactly as described in #99449, the JSDoc on the new modules is genuinely helpful, and the render-prop seam ( Routing correctly mirrors the old runtime guards ✅ — the dispatcher conditions are the same predicates the sections used to self-check, so the behavior should be preserved:
Dropping Watch items:
|
|
@claude review |
|
@codex review |
|
@MelvinBot review |
|
Codex Review: Didn't find any major issues. Bravo. 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-reviewed at The two structural assumptions the variants now bake in are correct ✅ Both
Smoke test (web) — DefaultFooter path ✅ The dispatcher routes manual / scan / distance / split / invoice to
Per-diem and time weren't exercised — the test account had no per-diem/time-enabled workspace — so those two variants rest on the code-trace above plus your recorded screenshots. Non-blocking watch items (unchanged from last pass)
Nothing blocking from me — the refactor is sound and the fallback path is verified. LGTM once the per-diem coverage note is addressed (or consciously deferred). |
|
No product review needed. |
|
@situchan lets get this one reviewed 🙌 thank you! |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safaritest1.movtest2.mov |
|
No functional regressions found at The one thing I'd fixThe props→context wiring is now untested, and it's the easiest thing to get silently wrong. Before this PR the footer owned Net effect: swapping The whole per-diem branch is also 0% covered. Measured with
Time is fully covered because Regression audit — clean, with the reasoningEvery combination routes identically (verified two ways)
Web smoke test — pass
Per-diem and time therefore rest on the code trace above plus your recorded videos — which is the second reason the per-diem unit test matters. Evidence (4)Manual expense confirmation screen
Distance expense confirmation with map route, Distance and Rate fields
Split expense confirmation with Paid by / Participants sections
Split expense submitted successfully
Two non-blocking notes
Nothing blocking. The refactor is sound — I'd just add the per-diem test before merge, since it's the one path with neither automated nor manual verification in this run. |
|
Applied NABs! |
mountiny
left a comment
There was a problem hiding this comment.
Cool, thanks for starting on this
|
🚧 mountiny 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/mountiny in version: 9.4.66-0 🚀
|
|
🤖 No help site changes are required for this PR, so I did not create a docs PR. This is an internal component refactor. It changes how the confirmation footer code is organized, not what anyone sees or does in the app. Why I concluded no docs update is neededWhat the diff actually changes. All 19 files are under No user-facing copy changed. The diff touches no No behavior changed. The PR is explicitly scoped as a refactor with no intended behavior change, and the structure preserves that: The articles that cover these flows stay accurate. The help site documents per diem and time expenses from the user's point of view — for example Per One thing to flag for your own review, not for docs: because this refactor is only pt1, @VickyStash — you asked me to close with a note pointing you at the linked help site PR. Since no docs changes were needed, there is no docs PR to link or mark |




Explanation of Change
Refactor with no behavior change.
MoneyRequestConfirmationListFooterwas one component serving every expense type, selecting behavior at runtime through boolean flags passed down from the list. This PR turns it into a folder with a dispatcher and per-type variants, and migrates the first two types (PerDiem and Time).Updates:
The footer becomes a folder.
MoneyRequestConfirmationListFooter.tsxsplits intoindex.tsxfor dispatch,variants/DefaultFooter.tsxfor the previous body, andtypes.tsfor the shared props type.PerDiemFooteris the first variant. It renders the per-diem subrate fields and the shared field list and nothing else, because per-diem shows no receipt on the confirmation page, is never a distance or scan request, and never enters compact mode.TimeFooteris the second variant.TimeDetailsFieldsrenders Amount, Description and the hours/rate fields with no conditionals.ConfirmationFieldListstops renderingTransactionDetailsFields. It takes arenderTransactionDetailsFieldscallback and invokes it in the same position, so render order does not change, and each variant supplies the details component its own type needs.ConfirmationFieldsProvidermoves up toMoneyRequestConfirmationList.MoneyRequestConfirmationListFooterPropsdrops from 28 props to 13.The confirmation mount span gains a request-type attribute.
High level of the new structure that is going to be: comment
Fixed Issues
$ #99449
PROPOSAL: N/A
Tests
This is a refactor with no intended behavior change, so testing is regression-focused: every expense type must render and submit exactly as before.
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 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
Per-diem
per-diem.mp4
Time
time.mp4
Manual
manual.mp4
Scan
scan.mp4
Distance
distance.mp4
Split
split.pay.mp4
Invoice
invoice.mp4
Submit
submit.mp4