Remove Onyx.connect() for the key: ONYXKEYS.COLLECTION.REPORT in src/libs/ReportUtils.ts (part 6) - #93423
Remove Onyx.connect() for the key: ONYXKEYS.COLLECTION.REPORT in src/libs/ReportUtils.ts (part 6)#93423truph01 wants to merge 27 commits into
Conversation
|
@DylanDylann 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] |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| getTransactionThreadPrimaryAction(currentUserLogin ?? '', accountID, report, parentReport, transaction, transactionViolations, policy, false) | ||
| ); | ||
| const activePolicyExpenseChat = getPolicyExpenseChat(accountID, defaultExpensePolicy?.id); | ||
| const [allReports] = useOnyx(ONYXKEYS.COLLECTION.REPORT); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
Adding useOnyx(ONYXKEYS.COLLECTION.REPORT) without a selector subscribes to the entire reports collection, which is very large and changes frequently. This will cause the component to re-render on every single report change across the app, even though only one specific policy expense chat report is needed.
Consider using a selector that finds the policy expense chat inline, returning only the needed report (or undefined). For example:
const [activePolicyExpenseChat] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {
selector: (reports) =>
Object.values(reports ?? {}).find(
(report) =>
report?.policyID === defaultExpensePolicy?.id &&
isPolicyExpenseChat(report) &&
!isThread(report) &&
report?.ownerAccountID === accountID,
) ?? null,
});Since the selector output is a single report object (small), deepEqual will be cheap and will prevent re-renders when unrelated reports change.
Reviewed at: 3d333dd | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Resolved this concern
| const [amountOwed] = useOnyx(ONYXKEYS.NVP_PRIVATE_AMOUNT_OWED); | ||
| const [userBillingGracePeriodEnds] = useOnyx(ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_USER_BILLING_GRACE_PERIOD_END); | ||
| const [ownerBillingGracePeriodEnd] = useOnyx(ONYXKEYS.NVP_PRIVATE_OWNER_BILLING_GRACE_PERIOD_END); | ||
| const [allReports] = useOnyx(ONYXKEYS.COLLECTION.REPORT); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
Adding useOnyx(ONYXKEYS.COLLECTION.REPORT) without a selector subscribes to the entire reports collection. Since allReports is also added to the useMemo dependency array (line 89), the memoized computation will be invalidated on every report change across the app, defeating the purpose of useMemo.
Consider using a selector that directly computes the policy expense chat report, or extract the getPolicyExpenseChat logic into a selector so only the specific report is returned. This avoids both the unnecessary re-renders and the useMemo invalidation:
const [activePolicyExpenseChat] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {
selector: (reports) =>
Object.values(reports ?? {}).find(
(report) =>
report?.policyID === defaultExpensePolicy?.id &&
isPolicyExpenseChat(report) &&
!isThread(report) &&
report?.ownerAccountID === accountID,
) ?? null,
});Then use activePolicyExpenseChat directly in the useMemo instead of allReports.
Reviewed at: 3d333dd | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Resolved this concern
|
|
||
| // Global collections | ||
| const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST); | ||
| const [allTransactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
Adding useOnyx(ONYXKEYS.COLLECTION.REPORT) without a selector subscribes to the entire reports collection. The result is used only to call getPolicyExpenseChat, which searches for a single matching report. Every change to any report in the app will cause this hook (and its consumers) to re-render.
Consider using a selector to find the policy expense chat directly, returning only the single needed report:
const [activePolicyExpenseChat] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {
selector: (reports) =>
Object.values(reports ?? {}).find(
(report) =>
report?.policyID === defaultExpensePolicy?.id &&
isPolicyExpenseChat(report) &&
!isThread(report) &&
report?.ownerAccountID === accountID,
) ?? null,
});Reviewed at: 3d333dd | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Resolved this concern
| const [personalPolicyID] = useOnyx(ONYXKEYS.PERSONAL_POLICY_ID); | ||
| const [personalPolicy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${personalPolicyID}`); | ||
| const [draftTransactionIDs] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_DRAFT, {selector: validTransactionDraftIDsSelector}); | ||
| const [allReports] = useOnyx(ONYXKEYS.COLLECTION.REPORT); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
Adding useOnyx(ONYXKEYS.COLLECTION.REPORT) without a selector subscribes to the entire reports collection. This value is only used inside the saveFileAndInitMoneyRequest callback (not in the render path), so every report change will trigger a re-render of this hook and all its consumers unnecessarily.
Consider using a selector to find the specific policy expense chat directly:
const [policyExpenseChat] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {
selector: (reports) =>
Object.values(reports ?? {}).find(
(report) =>
report?.policyID === activePolicy?.id &&
isPolicyExpenseChat(report) &&
!isThread(report) &&
report?.ownerAccountID === currentUserPersonalDetails.accountID,
) ?? null,
});This returns a single small report object, so deepEqual is cheap and re-renders only happen when the specific policy expense chat changes.
Reviewed at: 3d333dd | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Resolved this concern
| const activePolicyExpenseChat = useMemo( | ||
| () => getPolicyExpenseChat(currentUserPersonalDetails.accountID, defaultExpensePolicy?.id), | ||
| [currentUserPersonalDetails.accountID, defaultExpensePolicy?.id], | ||
| () => getPolicyExpenseChat(currentUserPersonalDetails.accountID, defaultExpensePolicy?.id, allReports), |
There was a problem hiding this comment.
❌ PERF-11 (docs)
Adding allReports to the useMemo dependency array means this memoized value will be recomputed on every report change across the app, since the existing useOnyx(ONYXKEYS.COLLECTION.REPORT) subscription has no selector. The getPolicyExpenseChat function only needs to find a single matching report.
Consider replacing the allReports-based useMemo with a direct useOnyx call using a selector:
const [activePolicyExpenseChat] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {
selector: (reports) =>
Object.values(reports ?? {}).find(
(report) =>
report?.policyID === defaultExpensePolicy?.id &&
isPolicyExpenseChat(report) &&
!isThread(report) &&
report?.ownerAccountID === currentUserPersonalDetails.accountID,
) ?? null,
});This avoids recomputing on every report change and only updates when the specific policy expense chat changes.
Reviewed at: 3d333dd | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Resolved this concern
| }, [selectedTransactionIDs, allTransactions]); | ||
|
|
||
| const activePolicyExpenseChat = useMemo(() => getPolicyExpenseChat(currentUserAccountID, defaultExpensePolicy?.id), [currentUserAccountID, defaultExpensePolicy?.id]); | ||
| const activePolicyExpenseChat = useMemo(() => getPolicyExpenseChat(currentUserAccountID, defaultExpensePolicy?.id, allReports), [currentUserAccountID, defaultExpensePolicy?.id, allReports]); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
Adding allReports to the useMemo dependency array means this memoized value will be recomputed on every report change across the app, since the existing useOnyx(ONYXKEYS.COLLECTION.REPORT) subscription has no selector. The getPolicyExpenseChat function only needs to find a single matching report.
Consider replacing the allReports-based useMemo with a direct useOnyx call using a selector:
const [activePolicyExpenseChat] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {
selector: (reports) =>
Object.values(reports ?? {}).find(
(report) =>
report?.policyID === defaultExpensePolicy?.id &&
isPolicyExpenseChat(report) &&
!isThread(report) &&
report?.ownerAccountID === currentUserAccountID,
) ?? null,
});This avoids recomputing on every report change and only updates when the specific policy expense chat changes.
Reviewed at: 3d333dd | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Resolved this concern
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
|
@DylanDylann Could you review this PR? It is a separate flow, which isn't related to previous part. |
|
@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: bb368bb407
ℹ️ 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".
|
Let me know when this is ready |
|
@DylanDylann It is ready |
|
@codex review |
| () => policyExpenseChatSelector(currentUserPersonalDetails.accountID, defaultExpensePolicy?.id), | ||
| [currentUserPersonalDetails.accountID, defaultExpensePolicy?.id], | ||
| ); | ||
| const [activePolicyExpenseChat] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {selector: activePolicyExpenseChatSelector}); |
There was a problem hiding this comment.
Can we use policyExpenseChatSelector directly without activePolicyExpenseChatSelector
There was a problem hiding this comment.
These hooks don't compile with React Compiler, so there's no auto-memoization — inline would recreate the selector every render. Keeping the useMemo wrapper. Inline is fine in the other call sites because those files do compile.
|
|
||
| const activePolicyExpenseChat = useMemo(() => getPolicyExpenseChat(currentUserAccountID, defaultExpensePolicy?.id), [currentUserAccountID, defaultExpensePolicy?.id]); | ||
| const activePolicyExpenseChatSelector = useMemo(() => policyExpenseChatSelector(currentUserAccountID, defaultExpensePolicy?.id), [currentUserAccountID, defaultExpensePolicy?.id]); | ||
| const [activePolicyExpenseChat] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {selector: activePolicyExpenseChatSelector}); |
There was a problem hiding this comment.
These hooks don't compile with React Compiler, so there's no auto-memoization — inline would recreate the selector every render. Keeping the useMemo wrapper. Inline is fine in the other call sites because those files do compile.
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-08-18.at.17.18.52.mov |
|
@truph01 left some NITs. Love your idea 🚀 |
|
@DylanDylann I resolved all your comments |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b9169247b
ℹ️ 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 [, policyCollectionResult] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {selector: () => null}); | ||
|
|
||
| const accountID = currentUserPersonalDetails.accountID; | ||
| const [activePolicyExpenseChat] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {selector: policyExpenseChatSelector(accountID, defaultExpensePolicy?.id)}); |
There was a problem hiding this comment.
When the report collection is still hydrating after the other tracked Onyx values have loaded, this selector temporarily returns undefined, but isLoading becomes false because the new subscription's metadata is ignored. In the global-create confirmation flow, that empty result is interpreted as having no default participant and can auto-open the participant picker before the default workspace chat arrives; selecting someone during that window prevents the intended workspace auto-assignment. Capture this call's result metadata and include it in isLoadingOnyxValue(...), as is already done for the self-DM lookup.
Useful? React with 👍 / 👎.
3 participants
Explanation of Change
This PR focus on refactoring the places where
getPolicyExpenseChatare called directly in component and hookFixed Issues
$ #66416
PROPOSAL:
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
Screen.Recording.2026-06-12.at.19.12.43.mov