-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[PR 2 of 13] Remove global Onyx reference from getOriginalReportID step 1 #81707
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
base: main
Are you sure you want to change the base?
Changes from all commits
30275bf
f7d1444
6f2cc86
17ba793
71edcd3
a351f64
f0b390a
9acac4a
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 |
|---|---|---|
|
|
@@ -2092,7 +2092,7 @@ function deleteReportComment( | |
| currentEmail: string, | ||
| ) { | ||
| const reportID = report?.reportID; | ||
| const originalReportID = getOriginalReportID(reportID, reportAction); | ||
| const originalReportID = getOriginalReportID(reportID, reportAction, undefined); | ||
|
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. ❌ [PERF-2] Return early before expensive work (docs)The function calls Fix: Extract simple checks first, then perform expensive work: const reportID = report?.reportID;
const reportActionID = reportAction.reportActionID;
if (\!reportActionID || \!reportID) {
return;
}
const originalReportID = getOriginalReportID(reportID, reportAction, undefined);
if (\!originalReportID) {
return;
}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. There is almost never a case where |
||
| const reportActionID = reportAction.reportActionID; | ||
|
|
||
| if (!reportActionID || !originalReportID || !reportID) { | ||
|
|
@@ -2420,13 +2420,13 @@ function editReportComment( | |
|
|
||
| /** Deletes the draft for a comment report action. */ | ||
| function deleteReportActionDraft(reportID: string | undefined, reportAction: ReportAction) { | ||
| const originalReportID = getOriginalReportID(reportID, reportAction); | ||
| const originalReportID = getOriginalReportID(reportID, reportAction, undefined); | ||
| Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${originalReportID}`, {[reportAction.reportActionID]: null}); | ||
| } | ||
|
|
||
| /** Saves the draft for a comment report action. This will put the comment into "edit mode" */ | ||
| function saveReportActionDraft(reportID: string | undefined, reportAction: ReportAction, draftMessage: string) { | ||
| const originalReportID = getOriginalReportID(reportID, reportAction); | ||
| const originalReportID = getOriginalReportID(reportID, reportAction, undefined); | ||
| Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${originalReportID}`, {[reportAction.reportActionID]: {message: draftMessage}}); | ||
| } | ||
|
|
||
|
|
@@ -3761,7 +3761,7 @@ function toggleEmojiReaction( | |
| currentUserAccountID: number, | ||
| ignoreSkinToneOnCompare = false, | ||
| ) { | ||
| const originalReportID = getOriginalReportID(reportID, reportAction); | ||
| const originalReportID = getOriginalReportID(reportID, reportAction, undefined); | ||
|
|
||
| if (!originalReportID) { | ||
| return; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,7 @@ Onyx.connect({ | |
| }); | ||
|
|
||
| function clearReportActionErrors(reportID: string, reportAction: ReportAction, keys?: string[]) { | ||
| const originalReportID = getOriginalReportID(reportID, reportAction); | ||
| const originalReportID = getOriginalReportID(reportID, reportAction, undefined); | ||
|
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. ❌ [PERF-2] Return early before expensive work (docs)The function calls Fix: Move the simple validation check before the expensive work: function clearReportActionErrors(reportID: string, reportAction: ReportAction, keys?: string[]) {
if (!reportAction?.reportActionID) {
return;
}
const originalReportID = getOriginalReportID(reportID, reportAction, undefined);
// ... rest of function
}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. There is almost never a case where reportAction.reportActionID wouldn't exist, so this is not a valuable optimization. |
||
|
|
||
| if (!reportAction?.reportActionID) { | ||
| return; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -176,6 +176,7 @@ function ReportDetailsPage({policy, report, route, reportMetadata}: ReportDetail | |
| const [conciergeReportID] = useOnyx(ONYXKEYS.CONCIERGE_REPORT_ID, {canBeMissing: true}); | ||
|
|
||
| const {reportActions} = usePaginatedReportActions(report.reportID); | ||
| const [reportActionsForOriginalReportID] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`, {canBeMissing: true}); | ||
|
|
||
| const {removeTransaction} = useSearchContext(); | ||
|
|
||
|
|
@@ -439,7 +440,7 @@ function ReportDetailsPage({policy, report, route, reportMetadata}: ReportDetail | |
| } | ||
|
|
||
| if (isTrackExpenseReport && !isDeletedParentAction) { | ||
| const actionReportID = getOriginalReportID(report.reportID, parentReportAction); | ||
| const actionReportID = getOriginalReportID(report.reportID, parentReportAction, reportActionsForOriginalReportID); | ||
| const whisperAction = getTrackExpenseActionableWhisper(iouTransactionID, moneyRequestReport?.reportID); | ||
|
Comment on lines
442
to
444
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.
The Useful? React with 👍 / 👎. |
||
| const actionableWhisperReportActionID = whisperAction?.reportActionID; | ||
| items.push({ | ||
|
|
@@ -621,6 +622,7 @@ function ReportDetailsPage({policy, report, route, reportMetadata}: ReportDetail | |
| allTransactionDrafts, | ||
| activePolicy, | ||
| parentReport, | ||
| reportActionsForOriginalReportID, | ||
| ]); | ||
|
|
||
| const displayNamesWithTooltips = useMemo(() => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -152,7 +152,6 @@ import { | |
| getIOUReportActionDisplayMessage, | ||
| getMovedActionMessage, | ||
| getMovedTransactionMessage, | ||
| getOriginalReportID, | ||
| getPolicyChangeMessage, | ||
| getReimbursementDeQueuedOrCanceledActionMessage, | ||
| getReimbursementQueuedActionMessage, | ||
|
|
@@ -239,6 +238,7 @@ type ContextMenuActionPayload = { | |
| reportAction: ReportAction; | ||
| transaction?: OnyxEntry<Transaction>; | ||
| reportID: string | undefined; | ||
| originalReportID: string | undefined; | ||
| currentUserAccountID: number; | ||
| report: OnyxEntry<ReportType>; | ||
| policy?: OnyxEntry<Policy>; | ||
|
|
@@ -460,12 +460,11 @@ const ContextMenuActions: ContextMenuAction[] = [ | |
|
|
||
| return hasReasoning(reportAction); | ||
| }, | ||
| onPress: (closePopover, {reportAction, reportID, translate, currentUserPersonalDetails}) => { | ||
| onPress: (closePopover, {reportAction, reportID, originalReportID, translate, currentUserPersonalDetails}) => { | ||
| if (!reportID) { | ||
| return; | ||
| } | ||
|
|
||
| const originalReportID = getOriginalReportID(reportID, reportAction); | ||
|
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. In both of these cases here, I was able to remove the call to |
||
| if (closePopover) { | ||
| hideContextMenu(false, () => { | ||
| KeyboardUtils.dismiss().then(() => { | ||
|
|
@@ -1084,8 +1083,7 @@ const ContextMenuActions: ContextMenuAction[] = [ | |
| const isDynamicWorkflowRoutedAction = isActionOfType(reportAction, CONST.REPORT.ACTIONS.TYPE.DYNAMIC_EXTERNAL_WORKFLOW_ROUTED); | ||
| return type === CONST.CONTEXT_MENU_TYPES.REPORT_ACTION && !isAttachmentTarget && !isMessageDeleted(reportAction) && !isDynamicWorkflowRoutedAction; | ||
| }, | ||
| onPress: (closePopover, {reportAction, reportID}) => { | ||
| const originalReportID = getOriginalReportID(reportID, reportAction); | ||
| onPress: (closePopover, {reportAction, originalReportID}) => { | ||
| getEnvironmentURL().then((environmentURL) => { | ||
| const reportActionID = reportAction?.reportActionID; | ||
| Clipboard.setString(`${environmentURL}/r/${originalReportID}/${reportActionID}`); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -185,6 +185,7 @@ function ReportActionsList({ | |
| const [isUserValidated] = useOnyx(ONYXKEYS.ACCOUNT, {selector: isUserValidatedSelector, canBeMissing: true}); | ||
| const [draftMessage] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}`, {canBeMissing: true}); | ||
| const [emojiReactions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_REACTIONS}`, {canBeMissing: true}); | ||
| const [reportActionsFromOnyx] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`, {canBeMissing: true}); | ||
| const [userBillingFundID] = useOnyx(ONYXKEYS.NVP_BILLING_FUND_ID, {canBeMissing: true}); | ||
| const [tryNewDot] = useOnyx(ONYXKEYS.NVP_TRY_NEW_DOT, {canBeMissing: false}); | ||
| const isTryNewDotNVPDismissed = !!tryNewDot?.classicRedirect?.dismissed; | ||
|
|
@@ -692,7 +693,7 @@ function ReportActionsList({ | |
|
|
||
| const renderItem = useCallback( | ||
| ({item: reportAction, index}: ListRenderItemInfo<OnyxTypes.ReportAction>) => { | ||
| const originalReportID = getOriginalReportID(report.reportID, reportAction); | ||
| const originalReportID = getOriginalReportID(report.reportID, reportAction, reportActionsFromOnyx); | ||
| const reportDraftMessages = draftMessage?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${originalReportID}`]; | ||
|
Comment on lines
694
to
697
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.
The Useful? React with 👍 / 👎. |
||
| const matchingDraftMessage = reportDraftMessages?.[reportAction.reportActionID]; | ||
| const matchingDraftMessageString = matchingDraftMessage?.message; | ||
|
|
@@ -767,6 +768,7 @@ function ReportActionsList({ | |
| isReportArchived, | ||
| reportNameValuePairs?.origin, | ||
| reportNameValuePairs?.originalID, | ||
| reportActionsFromOnyx, | ||
| ], | ||
| ); | ||
|
|
||
|
|
||
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 was 100% dead code, so easy to remove.