refactor: add ConfirmationFieldsProvider and grouped types#90325
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
OlimpiaZurek
force-pushed
the
provider-confirmation-fields
branch
from
51d0632 to
6b3b4a9
Compare
![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: 6b3b4a98f5
ℹ️ 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".
| const [draftTransaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${transactionID}`); | ||
| const [existingTransaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`); | ||
| const transaction = draftTransaction ?? existingTransaction; |
There was a problem hiding this comment.
When editing an existing split bill, SplitBillDetailsPage passes the SPLIT_TRANSACTION_DRAFT transaction into this footer and inline edits are written back to that same collection, but this refetch only considers TRANSACTION_DRAFT and TRANSACTION. As soon as a split edit draft exists, useFooterDerivedFlags and tag visibility are computed from the stale original transaction while the leaf fields use useTransactionSelector() and read the split draft, so derived values such as currency/tag/tax visibility can disagree with what the user just edited (for example tax formatting staying on the old currency after changing the split amount currency).
Useful? React with 👍 / 👎.
OlimpiaZurek
force-pushed
the
provider-confirmation-fields
branch
from
2add245 to
2db4381
Compare
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ 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". |
melvin-bot
Bot
requested review from
JmillsExpensify and
ShridharGoel
and removed request for
a team
|
@ShridharGoel 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| isPolicyExpenseChat = false, | ||
| children, | ||
| }: ProviderProps) { | ||
| const value = useMemo( |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler is enabled in this codebase and automatically memoizes values based on their dependencies. The useMemo wrapping the context value object is redundant if this file compiles successfully with React Compiler. The compiler will detect that the object depends on the listed props and cache it automatically.
Note: I was unable to run the React Compiler compliance check to verify this file compiles. If it does not compile, this useMemo is appropriate. If it does compile, remove the manual memoization:
const value = {
transactionID,
reportID,
reportActionID,
action,
iouType,
policyID,
isReadOnly,
didConfirm,
isEditingSplitBill,
isNewManualExpenseFlowEnabled,
isPolicyExpenseChat,
};
return <ConfirmationFieldsContext.Provider value={value}>{children}</ConfirmationFieldsContext.Provider>;Reviewed at: 2db4381 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const transaction = isEditingSplitBill ? (splitDraftTransaction ?? existingTransaction) : (draftTransaction ?? existingTransaction); | ||
| const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`); | ||
| const [policyTags] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_TAGS}${policyID}`); | ||
| const policyTagLists = useMemo(() => getTagLists(policyTags), [policyTags]); |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler is enabled in this codebase and automatically memoizes derived values. The useMemo wrapping getTagLists(policyTags) is redundant if this file compiles with React Compiler -- the compiler will detect that policyTagLists depends only on policyTags and cache the result automatically.
Note: I was unable to run the React Compiler compliance check to verify this file compiles. If it does not compile, this useMemo is appropriate. If it does compile, remove the manual memoization:
const policyTagLists = getTagLists(policyTags);Reviewed at: 2db4381 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const icons = useMemoizedLazyExpensifyIcons(['Sparkles', 'DownArrow']); | ||
| const {action, iouType, transactionID, policyID, isReadOnly, isPolicyExpenseChat, isEditingSplitBill} = useConfirmationFields(); | ||
|
|
||
| const [splitDraftTransaction] = useOnyx(`${ONYXKEYS.COLLECTION.SPLIT_TRANSACTION_DRAFT}${transactionID}`); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The transaction resolution logic here (subscribing to SPLIT_TRANSACTION_DRAFT, TRANSACTION_DRAFT, and TRANSACTION, then selecting based on isEditingSplitBill) duplicates the same pattern already implemented in useTransactionSelector.ts. That hook exists specifically for this purpose and was updated in this same PR to read isEditingSplitBill from context.
Consider reusing useTransactionSelector with an identity selector, or extracting a useResolvedTransaction variant that returns the full transaction object:
// Option A: use identity selector
const transaction = useTransactionSelector(transactionID, (t) => t);
// Option B: extract a dedicated hook (e.g., useResolvedTransaction)
const transaction = useResolvedTransaction(transactionID);This keeps the Onyx key selection logic in a single location.
Reviewed at: 2db4381 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
I went with the inline subscriptions deliberately. Unlike the leaves that pass a narrow selector to useTransactionSelector (e.g. amountSlice,merchantState), this consumer needs the whole transaction for derived flags and tag visibility. Using useTransactionSelector with an identity selector(t) => t would force deepEqual walks across the full Transaction on every Onyx write, vs. the cheap reference checks the no-selector subscriptions do here. If we see another consumer needing the same pattern, I’d probably extract a dedicated hook like useResolvedTransaction() around the no-selector path. With only a single caller right now, though, keeping it inline felt simpler.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2db43816be
ℹ️ 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".
| const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`); | ||
| const [policyTags] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_TAGS}${policyID}`); | ||
| const policyTagLists = useMemo(() => getTagLists(policyTags), [policyTags]); |
There was a problem hiding this comment.
The footer now derives policy/policyTags from policyID in ConfirmationFieldsContext, while submit validation in MoneyRequestConfirmationList still uses usePolicyCategoriesForConfirmation(policyID) and usePolicyTagsForConfirmation(policyID) from the parent prop. In track-expense flows where the effective policy can diverge from that prop (for example after workspace selection), the UI fields are rendered for one policy but validation runs against another, causing valid category/tag choices to be rejected (or out-of-policy checks to be skipped) at submit time.
Useful? React with 👍 / 👎.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeScreen.Recording.2026-05-25.at.9.11.01.PM.moviOS: HybridAppScreen.Recording.2026-05-25.at.9.00.22.PM.moviOS: mWeb SafariScreen.Recording.2026-05-25.at.9.03.44.PM.movMacOS: Chrome / SafariScreen.Recording.2026-05-25.at.8.46.33.PM.mov |
| const theme = useTheme(); | ||
| const {translate} = useLocalize(); | ||
| const icons = useMemoizedLazyExpensifyIcons(['Sparkles', 'DownArrow']); | ||
| const {action, iouType, transactionID, policyID, isReadOnly, isPolicyExpenseChat, isEditingSplitBill} = useConfirmationFields(); |
There was a problem hiding this comment.
Shouldn't we use the policy that is being passed to MoneyRequestConfirmationListFooter from MoneyRequestConfirmationList?
There was a problem hiding this comment.
Good call, switched to using the policy
|
🚧 @Julesssss 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
5 participants
Explanation of Change
Follow-up to #90321. Replaces the remaining prop drilling in
MoneyRequestConfirmationListFooterwith a context provider and groups the long flat prop list into cohesive type bundles.MoneyRequestConfirmationFields/Provider.tsx+context.ts— exposes identity (transactionID,reportID,reportActionID,action,iouType,policyID) and coordination flags (isReadOnly,didConfirm,isEditingSplitBill,isNewManualExpenseFlowEnabled,isPolicyExpenseChat) to every block viauseConfirmationFields().fieldGroupTypes.ts— bundles flat props into cohesive types (AmountDisplay,DistanceData,DistanceFlags,ExpenseMode,ErrorState,ReceiptOptions,RequiredFlags,ToggleHandlers,VisibilityFlags,CompactControls,CompactState).MoneyRequestConfirmationListFooternow wraps its subtree in<ConfirmationFieldsProvider>and accepts grouped types instead of ~30 flat flags.ConfirmationFieldListreads identity/coordination via the context hook and subscribes totransaction+policydirectly via Onyx (no longer prop-drilled through the footer).SettingsFields,ClassificationFields,TransactionDetailsFields) consume the context for shared fields and accept only the props specific to their group.MoneyRequestConfirmationList.tsx(caller) updated to pass the new grouped types.Net effect: ~916 lines removed, ~563 added. Footer prop surface shrinks from ~30 flat flags to ~10 grouped types. ConfirmationFieldList's prop list collapses from ~50 to ~10.
PR 4 of 4 in the
MoneyRequestConfirmationListFooterdecomposition series.Fixed Issues
$ #90385
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari