perf: Uplift common Onyx subscriptions from ReportActionItem to parent list components#83205
perf: Uplift common Onyx subscriptions from ReportActionItem to parent list components#83205
Conversation
…t list components Each ReportActionItem in a FlatList was creating its own useOnyx subscriptions for data that is identical across all items (e.g. CARD_LIST, BANK_ACCOUNT_LIST, NVP_INTRO_SELECTED). For a list of N items, this created N redundant subscriptions instead of 1. Moved 8 common subscriptions to the parent list components (ReportActionsList, MoneyRequestReportActionsList) and pass the data as props through ReportActionsListItemRenderer and ReportActionItemParentAction. Co-authored-by: Cursor <cursoragent@cursor.com>
melvin-bot
bot
requested review from
joekaufmanexpensify and
rojiphil
and removed request for
a team and
joekaufmanexpensify
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const [tryNewDot] = useOnyx(ONYXKEYS.NVP_TRY_NEW_DOT, {canBeMissing: false}); | ||
| const isTryNewDotNVPDismissed = !!tryNewDot?.classicRedirect?.dismissed; | ||
| const [introSelected] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED, {canBeMissing: true}); | ||
| const [allTransactionDrafts] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_DRAFT, {canBeMissing: true}); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
This block of 7 useOnyx calls and the usePolicyForMovingExpenses + policyIDForTags computation is duplicated identically in MoneyRequestReportActionsList.tsx (lines 170-177). Both list components fetch the exact same data for the exact same purpose (passing it down to ReportActionsListItemRenderer -> ReportActionItem).
Consider extracting these shared subscriptions into a custom hook, e.g.:
function useReportActionItemOnyxData(report: OnyxEntry<Report>) {
const [allTransactionDrafts] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_DRAFT, {canBeMissing: true});
const [cardList] = useOnyx(ONYXKEYS.CARD_LIST, {canBeMissing: true});
const [bankAccountList] = useOnyx(ONYXKEYS.BANK_ACCOUNT_LIST, {canBeMissing: true});
const [personalPolicyID] = useOnyx(ONYXKEYS.PERSONAL_POLICY_ID, {canBeMissing: true});
const {policyForMovingExpensesID} = usePolicyForMovingExpenses();
const policyIDForTags = report?.policyID === CONST.POLICY.OWNER_EMAIL_FAKE && policyForMovingExpensesID ? policyForMovingExpensesID : report?.policyID;
const [policyTags] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_TAGS}${policyIDForTags}`, {canBeMissing: true});
return {allTransactionDrafts, cardList, bankAccountList, personalPolicyID, policyTags};
}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 am not sure if this is going to be much cleaner in this case
![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: 4bfd92ed91
ℹ️ 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".
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.
|
Resolved conflicts in ReportActionItem.tsx, ReportActionsList.tsx, and MoneyRequestReportActionsList.tsx caused by main removing canBeMissing options from useOnyx calls. Updated our uplifted subscriptions to match. Co-authored-by: Cursor <cursoragent@cursor.com>
ReportActionItemParentAction now subscribes to per-ancestor reportMetadata and policyTags using collection selectors (matching the existing pattern for ancestorsReportNameValuePairs), instead of receiving a single shared value from the parent list. This ensures ancestors in thread views get correct per-report metadata and per-workspace tag labels. Also makes personalPolicyID and allTransactionDrafts optional in ReportActionItemProps so callers that use ReportActionItem directly (DuplicateTransactionItem, DebugReportActionPreview, DebugReportActionCreatePage, ChatListItem) don't need to provide them. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@adhorodyski Will you be able to test out the performance here? |
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Yes. FYI we're looking for a good, proper way to refactor these list items without causing mass disruption, while bringing in big performance gains with @TMisiukiewicz. As much as this can even bring in small gains, I'd say it's first (and foremost?) idiomatically incorrect, because truth is not all list elements will require these properties. I'll look into it more closely on tomorrow and get back to you! |
2 participants
Explanation of Change
ReportActionItemis rendered once per report action inside a FlatList. Previously, each instance created its ownuseOnyxsubscriptions for global/shared Onyx keys (e.g.CARD_LIST,BANK_ACCOUNT_LIST,NVP_INTRO_SELECTED, etc.). For a list of 50 report actions, this meant ~400-550 redundant Onyx subscriptions all returning the same data.This PR moves 8 common Onyx subscriptions (plus 3 internal ones from the
usePolicyForMovingExpenseshook) fromReportActionItemup to the parent list components (ReportActionsListandMoneyRequestReportActionsList), and passes the data as props throughReportActionsListItemRendererandReportActionItemParentAction.Subscriptions uplifted:
ONYXKEYS.NVP_INTRO_SELECTED(was already fetched in parent lists but not passed down — duplicate removed)ONYXKEYS.COLLECTION.TRANSACTION_DRAFTONYXKEYS.CARD_LISTONYXKEYS.BANK_ACCOUNT_LISTONYXKEYS.PERSONAL_POLICY_IDONYXKEYS.COLLECTION.REPORT_METADATA(was already fetched inReportActionsListbut not passed down — duplicate removed)ONYXKEYS.COLLECTION.POLICY_TAGS(same key for all items since they share the same report)usePolicyForMovingExpenses()hook (internally subscribes to 3 Onyx keys)Instance-specific subscriptions that remain in
ReportActionItem:useOriginalReportID(depends on each action)useReportIsArchived(depends on per-action originalReportID)useReportTransactions(per-action iouReport)linkedTransactionRouteError(per-action transactionID)This follows the existing pattern already used for
allReports,policies,personalDetails,userWalletTierName, etc.Files changed:
ReportActionItem.tsx— Removed 8useOnyxcalls and theusePolicyForMovingExpenseshook; accepts the data as props insteadReportActionsListItemRenderer.tsx— Added new props to type definition and passes them through toReportActionItemandReportActionItemParentActionReportActionItemParentAction.tsx— Added new props and passes them through to each ancestor'sReportActionItemReportActionsList.tsx— Added newuseOnyxcalls andusePolicyForMovingExpenseshook; passes data throughrenderItemMoneyRequestReportActionsList.tsx— Same additions asReportActionsListFixed Issues
$ #83204
Tests
Offline tests
QA Steps
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari