Remove Onyx.connect() for the key: ONYXKEYS.COLLECTION.REPORT in src/libs/ReportUtils.ts (part 4)#90174
Conversation
melvin-bot
Bot
requested review from
DylanDylann and
heyjennahay
and removed request for
a team
|
@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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -162,7 +162,9 @@ function shouldShowBulkDuplicateOption({ | |||
| } | |||
|
|
|||
| const reportID = selectedTransactions[id]?.reportID; | |||
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The expression allReports?.[\${ONYXKEYS.COLLECTION.REPORT}${reportID}`]is repeated across multiple call sites in this file and inuseSelectedTransactionsActions.ts. This boilerplate for looking up a report from the allReportscollection and passing it as the 5th positional argument (with threeundefined` placeholders) is duplicated 6+ times.
Consider extracting a helper function to encapsulate this pattern:
function getReportFromAllReports(reportID: string | undefined) {
return reportID ? allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`] : undefined;
}Then each call site becomes:
getReportOrDraftReport(reportID, searchReports, undefined, undefined, getReportFromAllReports(reportID))Or better yet, consider whether getReportOrDraftReport itself should accept a named options object instead of positional parameters to avoid the undefined, undefined, undefined pattern entirely.
Reviewed at: a13d9e2 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
This is a common pattern used throughout the app for looking up Onyx collection entries. Introducing a helper like:
function getReportFromAllReports(reportID: string | undefined) {
return reportID ? allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`] : undefined;
}would just replace allReports?.[${ONYXKEYS.COLLECTION.REPORT}${reportID}] with getReportFromAllReports(reportID) — it doesn't reduce complexity or improve readability in a meaningful way. The lookup is a single expression and is already self-explanatory. I'll leave this as-is.
|
|
||
| // The report must be in Onyx so the pre-inserted screen can render immediately. | ||
| const isDestinationReportLoaded = !!destinationReportID && !!getReportOrDraftReport(destinationReportID)?.reportID; | ||
| const isDestinationReportLoaded = !!destinationReportID && !!getReportOrDraftReport(destinationReportID, undefined, undefined, undefined, destinationReport)?.reportID; |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The pattern getReportOrDraftReport(reportID, undefined, undefined, undefined, report) with three positional undefined placeholders is duplicated across this file and SubmitExpenseOrchestrator.tsx (which has the same pattern twice). This is the same code smell present in the hooks files.
Since getReportOrDraftReport already has a positional API with 5 parameters, and most callers only need the 1st and 5th, consider refactoring it to accept a named options object:
function getReportOrDraftReport({ reportID, searchReports, fallbackReport, reportDrafts, report }: GetReportOrDraftReportParams)This would eliminate all the undefined placeholders and make call sites self-documenting.
Reviewed at: a13d9e2 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
ince getReportOrDraftReport already has a positional API with 5 parameters, and most callers only need the 1st and 5th, consider refactoring it to accept a named options object:
function getReportOrDraftReport({ reportID, searchReports, fallbackReport, reportDrafts, report }: GetReportOrDraftReportParams)
That plan sounds good. However, I think we can do it in another PR.
cc @DylanDylann
| const {targetTransaction, sourceTransaction, targetTransactionReport, sourceTransactionReport, targetTransactionPolicy, sourceTransactionPolicy} = useMergeTransactions({ | ||
| mergeTransaction, | ||
| }); | ||
| const [targetTransactionReportByID] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${targetTransaction?.reportID}`); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
This useOnyx call subscribes to targetTransactionReportByID using targetTransaction?.reportID, but targetTransactionReport is already available from useMergeTransactions() on line 51 and represents the same report (for reported expenses, getReportIDForExpense returns transaction.reportID). This creates a redundant Onyx subscription that will cause unnecessary re-renders.
Consider passing the already-available targetTransactionReport as the transactionReport parameter instead of creating a new subscription:
getTransactionReportName({
translate,
reportAction: undefined,
transaction: targetTransaction,
transactions: [targetTransaction],
reports: targetTransactionReport ? [targetTransactionReport] : [],
transactionReport: targetTransactionReport,
})This eliminates the extra useOnyx subscription entirely.
Reviewed at: a13d9e2 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
heyjennahay
left a comment
heyjennahay
left a comment
There was a problem hiding this comment.
No product review required
|
@truph01 Could you check the AI review and fix the failed check? |
|
@DylanDylann All comments are resolved |
| transaction?: OnyxEntry<Transaction>; | ||
| transactions?: Transaction[]; | ||
| reports?: Report[]; | ||
| transactionReport?: OnyxEntry<Report>; |
There was a problem hiding this comment.
Do we plan to remove "?" in the next PRs? If yes, please leave a comment
| * @deprecated Moved to src/libs/ReportNameUtils.ts. | ||
| */ | ||
| function getReportName(reportNameInformation: GetReportNameParams): string { | ||
| const {report, policy, parentReportActionParam, personalDetails, invoiceReceiverPolicy, reportAttributes, transactions, isReportArchived, reports, conciergeReportID} = |
There was a problem hiding this comment.
Are you planning to refactor getReportName in the next PRs?
There was a problem hiding this comment.
@DylanDylann Yes, I will refactor getReportName in the next PR
There was a problem hiding this comment.
@truph01 Could you merge the latest main again? Now this getReportName function can only be used in Search page. Could you verify again if it's necessary to refactor this function anymore
There was a problem hiding this comment.
Could you verify again if it's necessary to refactor this function anymore
@DylanDylann The getReportName is still called in getSearchReportName function, so I think we still need to refactor this function (getReportName).
|
@DylanDylann I resolved your comments |
| transaction?: OnyxEntry<Transaction>; | ||
| transactions?: Transaction[]; | ||
| reports?: Report[]; | ||
| transactionReport?: OnyxEntry<Report>; |
| /** Used to identify the Concierge chat so its name can be set to the Concierge display name */ | ||
| conciergeReportID?: string; | ||
| /** The transaction linked to the report action */ | ||
| transaction?: OnyxEntry<Transaction>; |
There was a problem hiding this comment.
Please add a comment to remind us to remove ? later
There was a problem hiding this comment.
It isn't scope of this PR anymore based on #90174 (comment)
| transaction: targetTransaction, | ||
| transactions: [targetTransaction], | ||
| reports: targetTransactionReport ? [targetTransactionReport] : [], | ||
| transactionReport: targetTransactionReport, |
There was a problem hiding this comment.
@truph01 These looks like redundant parameters. Could we make it cleaner?
|
The test failure is due to a timeout. @truph01 Could you merge main again to re-run the test? |
|
@truph01 Could you resolve the conflict? |
|
@truph01 one failed check, could you solve it? |
|
The lint failure isn't related to this PR. |
Let's merge main. It should be good now |
|
@Gonals Kindly bump |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @Gonals 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/Gonals in version: 9.4.8-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 No help site changes required. This PR is part 4 of an internal refactor to remove
There are no user-facing changes — no new features, UI elements, tabs, settings, or button labels are introduced or modified. Nothing under @truph01, since no help site changes were required, there is no linked help site PR to review. If you believe a documentation update is needed, let me know what behavior changed and I'll draft one. |
|
🚀 Deployed to staging by https://github.com/Gonals in version: 9.4.8-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.4.8-3 🚀
|
6 participants
Explanation of Change
This PR focus on refactoring all places
Fixed 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-04.at.16.02.16.mov