-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Remove Onyx.connect() for the key: ONYXKEYS.COLLECTION.REPORT in src/libs/ReportUtils.ts (part 4) #90174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove Onyx.connect() for the key: ONYXKEYS.COLLECTION.REPORT in src/libs/ReportUtils.ts (part 4) #90174
Changes from all commits
a13d9e2
19694db
2da8f5f
f0a00b2
5fcb07c
985b593
b0a4856
7b1057c
4539dbb
9c1d54d
c734a15
7de4a90
f39797c
7e9c970
c89640c
a20fc25
e09f1ae
34818a8
9940ea1
4347c27
b1d4a59
475e230
90e0734
594bf74
16d4406
d399118
4cf9f1d
b578cd4
e739444
93181c0
afaea68
84cffa3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -441,6 +441,7 @@ function IOURequestStepConfirmation({ | |
| const isMRReport = isMoneyRequestReport(report); | ||
| const destinationReportID = | ||
| backToReport ?? (isPerDiemRequest && isMRReport && Navigation.getTopmostReportId() !== report?.reportID ? report?.chatReportID : report?.reportID) ?? selfDMReportID; | ||
| const [destinationReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${destinationReportID}`); | ||
|
|
||
| useEffect(() => { | ||
| if (hasPreInsertFired.current || !isTransactionReady || !getIsNarrowLayout()) { | ||
|
|
@@ -470,7 +471,7 @@ function IOURequestStepConfirmation({ | |
| const hasValidDestination = !!destinationReportID && Navigation.getTopmostReportId() !== destinationReportID; | ||
|
|
||
| // 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; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ CONSISTENCY-3 (docs)The pattern Since function getReportOrDraftReport({ reportID, searchReports, fallbackReport, reportDrafts, report }: GetReportOrDraftReportParams)This would eliminate all the Reviewed at: a13d9e2 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That plan sounds good. However, I think we can do it in another PR. |
||
|
|
||
| const shouldPreInsertReport = canUseReportPreInsert && isOutsideRHP && hasValidDestination && isDestinationReportLoaded; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ 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 theallReportscollection and passing it as the 5th positional argument (with threeundefined` placeholders) is duplicated 6+ times.Consider extracting a helper function to encapsulate this pattern:
Then each call site becomes:
Or better yet, consider whether
getReportOrDraftReportitself should accept a named options object instead of positional parameters to avoid theundefined, undefined, undefinedpattern 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a common pattern used throughout the app for looking up Onyx collection entries. Introducing a helper like:
would just replace
allReports?.[${ONYXKEYS.COLLECTION.REPORT}${reportID}]withgetReportFromAllReports(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.