Skip to content

perf: move report brick road statuses to a derived value #60827

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

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Mobile-Expensify
20 changes: 19 additions & 1 deletion src/components/LHNOptionsList/LHNOptionsList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
const [transactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION, {canBeMissing: false});
const [draftComments] = useOnyx(ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT, {canBeMissing: false});
const [transactionViolations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS, {canBeMissing: false});
const [reportAttributes] = useOnyx(ONYXKEYS.DERIVED.REPORT_ATTRIBUTES, {canBeMissing: false});

const theme = useTheme();
const styles = useThemeStyles();
Expand Down Expand Up @@ -136,6 +137,7 @@
const itemOneTransactionThreadReport = reports?.[`${ONYXKEYS.COLLECTION.REPORT}${getOneTransactionThreadReportID(reportID, itemReportActions, isOffline)}`];
const itemParentReportActions = reportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${itemFullReport?.parentReportID}`];
const itemParentReportAction = itemFullReport?.parentReportActionID ? itemParentReportActions?.[itemFullReport?.parentReportActionID] : undefined;
const itemReportAttributes = reportAttributes?.[reportID];

let invoiceReceiverPolicyID = '-1';
if (itemFullReport?.invoiceReceiver && 'policyID' in itemFullReport.invoiceReceiver) {
Expand Down Expand Up @@ -203,10 +205,11 @@
hasDraftComment={hasDraftComment}
transactionViolations={transactionViolations}
onLayout={onLayoutItem}
reportAttributes={itemReportAttributes}
/>
);
},
[

Check warning on line 212 in src/components/LHNOptionsList/LHNOptionsList.tsx

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

React Hook useCallback has a missing dependency: 'reportAttributes'. Either include it or remove the dependency array

Check warning on line 212 in src/components/LHNOptionsList/LHNOptionsList.tsx

View workflow job for this annotation

GitHub Actions / ESLint check

React Hook useCallback has a missing dependency: 'reportAttributes'. Either include it or remove the dependency array
draftComments,
onSelectRow,
optionMode,
Expand Down Expand Up @@ -238,8 +241,23 @@
preferredLocale,
transactions,
isOffline,
reportAttributes,
],
[
reportActions,
reports,
reportNameValuePairs,
transactionViolations,
policy,
personalDetails,
data.length,
draftComments,
optionMode,
preferredLocale,
transactions,
isOffline,
reportAttributes,
],
[reportActions, reports, reportNameValuePairs, transactionViolations, policy, personalDetails, data.length, draftComments, optionMode, preferredLocale, transactions, isOffline],
);

const previousOptionMode = usePrevious(optionMode);
Expand Down
10 changes: 4 additions & 6 deletions src/components/LHNOptionsList/OptionRowLHN.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
isPolicyExpenseChat,
isSystemChat,
isThread,
requiresAttentionFromCurrentUser,
} from '@libs/ReportUtils';
import {showContextMenu} from '@pages/home/report/ContextMenu/ReportActionContextMenu';
import FreeTrial from '@pages/settings/Subscription/FreeTrial';
Expand All @@ -57,10 +56,10 @@
const [isScreenFocused, setIsScreenFocused] = useState(false);
const {shouldUseNarrowLayout} = useResponsiveLayout();

const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${optionItem?.reportID}`);

Check failure on line 59 in src/components/LHNOptionsList/OptionRowLHN.tsx

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

useOnyx() calls require you to pass the "canBeMissing" param
const [activePolicyID] = useOnyx(ONYXKEYS.NVP_ACTIVE_POLICY_ID);

Check failure on line 60 in src/components/LHNOptionsList/OptionRowLHN.tsx

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

useOnyx() calls require you to pass the "canBeMissing" param
const [introSelected] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED);

Check failure on line 61 in src/components/LHNOptionsList/OptionRowLHN.tsx

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

useOnyx() calls require you to pass the "canBeMissing" param
const [isFullscreenVisible] = useOnyx(ONYXKEYS.FULLSCREEN_VISIBILITY);

Check failure on line 62 in src/components/LHNOptionsList/OptionRowLHN.tsx

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

useOnyx() calls require you to pass the "canBeMissing" param
const session = useSession();
const shouldShowWokspaceChatTooltip = isPolicyExpenseChat(report) && !isThread(report) && activePolicyID === report?.policyID && session?.accountID === report?.ownerAccountID;
const isOnboardingGuideAssigned = introSelected?.choice === CONST.ONBOARDING_CHOICES.MANAGE_TEAM && !session?.email?.includes('+');
Expand Down Expand Up @@ -119,8 +118,7 @@
return null;
}

const hasBrickError = optionItem.brickRoadIndicator === CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR;
const shouldShowGreenDotIndicator = !hasBrickError && requiresAttentionFromCurrentUser(optionItem, optionItem.parentReportAction);
const brickRoadIndicator = optionItem.brickRoadIndicator;
const textStyle = isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText;
const textUnreadStyle = shouldUseBoldText(optionItem) ? [textStyle, styles.sidebarLinkTextBold] : [textStyle];
const displayNameStyle = [styles.optionDisplayName, styles.optionDisplayNameCompact, styles.pre, textUnreadStyle, style];
Expand Down Expand Up @@ -316,7 +314,7 @@
<Text style={[styles.textLabel]}>{optionItem.descriptiveText}</Text>
</View>
) : null}
{hasBrickError && (
{brickRoadIndicator === CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR && (
<View style={[styles.alignItemsCenter, styles.justifyContentCenter]}>
<Icon
testID="RBR Icon"
Expand All @@ -331,7 +329,7 @@
style={[styles.flexRow, styles.alignItemsCenter]}
accessible={false}
>
{shouldShowGreenDotIndicator && (
{brickRoadIndicator === CONST.BRICK_ROAD_INDICATOR_STATUS.INFO && (
<View style={styles.ml2}>
<Icon
testID="GBR Icon"
Expand All @@ -352,7 +350,7 @@
/>
</View>
)}
{!shouldShowGreenDotIndicator && !hasBrickError && !!optionItem.isPinned && (
{!brickRoadIndicator && !!optionItem.isPinned && (
<View
style={styles.ml2}
accessibilityLabel={translate('sidebarScreen.chatPinned')}
Expand Down
11 changes: 2 additions & 9 deletions src/components/LHNOptionsList/OptionRowLHNData.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import useCurrentReportID from '@hooks/useCurrentReportID';
import SidebarUtils from '@libs/SidebarUtils';
import CONST from '@src/CONST';
import type {OptionData} from '@src/libs/ReportUtils';
import {hasReportViolations, isReportOwner, isSettled, shouldDisplayViolationsRBRInLHN} from '@src/libs/ReportUtils';
import OptionRowLHN from './OptionRowLHN';
import type {OptionRowLHNDataProps} from './types';

Expand All @@ -31,6 +30,7 @@ function OptionRowLHNData({
lastReportActionTransaction,
transactionViolations,
lastMessageTextFromReport,
reportAttributes,
...propsToForward
}: OptionRowLHNDataProps) {
const reportID = propsToForward.reportID;
Expand All @@ -39,10 +39,6 @@ function OptionRowLHNData({

const optionItemRef = useRef<OptionData>();

const shouldDisplayViolations = shouldDisplayViolationsRBRInLHN(fullReport, transactionViolations);
const isReportSettled = isSettled(fullReport);
const shouldDisplayReportViolations = !isReportSettled && isReportOwner(fullReport) && hasReportViolations(reportID);

const optionItem = useMemo(() => {
// Note: ideally we'd have this as a dependent selector in onyx!
const item = SidebarUtils.getOptionData({
Expand All @@ -54,9 +50,7 @@ function OptionRowLHNData({
preferredLocale: preferredLocale ?? CONST.LOCALES.DEFAULT,
policy,
parentReportAction,
hasViolations: !!shouldDisplayViolations || shouldDisplayReportViolations,
lastMessageTextFromReport,
transactionViolations,
invoiceReceiverPolicy,
});
// eslint-disable-next-line react-compiler/react-compiler
Expand Down Expand Up @@ -84,11 +78,10 @@ function OptionRowLHNData({
parentReportAction,
iouReportReportActions,
transaction,
transactionViolations,
receiptTransactions,
invoiceReceiverPolicy,
shouldDisplayReportViolations,
lastMessageTextFromReport,
reportAttributes,
]);

return (
Expand Down
5 changes: 4 additions & 1 deletion src/components/LHNOptionsList/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import type {OnyxCollection, OnyxEntry} from 'react-native-onyx';
import type {ValueOf} from 'type-fest';
import type CONST from '@src/CONST';
import type {OptionData} from '@src/libs/ReportUtils';
import type {Locale, PersonalDetailsList, Policy, Report, ReportAction, ReportActions, ReportNameValuePairs, Transaction, TransactionViolation} from '@src/types/onyx';
import type {Locale, PersonalDetailsList, Policy, Report, ReportAction, ReportActions, ReportAttributes, ReportNameValuePairs, Transaction, TransactionViolation} from '@src/types/onyx';

type OptionMode = ValueOf<typeof CONST.OPTION_MODE>;

Expand Down Expand Up @@ -102,6 +102,9 @@ type OptionRowLHNDataProps = {

/** Callback to execute when the OptionList lays out */
onLayout?: (event: LayoutChangeEvent) => void;

/** The attributes of the report */
reportAttributes?: ReportAttributes;
};

type OptionRowLHNProps = {
Expand Down
9 changes: 9 additions & 0 deletions src/libs/OptionsListUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
Report,
ReportAction,
ReportActions,
ReportAttributes,
ReportNameValuePairs,
TransactionViolation,
} from '@src/types/onyx';
Expand Down Expand Up @@ -364,6 +365,14 @@
},
});

let reportAttributes: OnyxEntry<Record<string, ReportAttributes>>;

Check failure on line 368 in src/libs/OptionsListUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

'reportAttributes' is assigned a value but never used

Check failure on line 368 in src/libs/OptionsListUtils.ts

View workflow job for this annotation

GitHub Actions / ESLint check

'reportAttributes' is assigned a value but never used
Onyx.connect({
key: ONYXKEYS.DERIVED.REPORT_ATTRIBUTES,
callback: (value) => {
reportAttributes = value;
},
});

const lastReportActions: ReportActions = {};
const allSortedReportActions: Record<string, ReportAction[]> = {};
let allReportActions: OnyxCollection<ReportActions>;
Expand Down
38 changes: 38 additions & 0 deletions src/libs/ReportUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10464,6 +10464,43 @@ function getChatListItemReportName(action: ReportAction & {reportName?: string},
return action?.reportName ?? '';
}

/**
* Generates report attributes for a report
* This function should be called only in reportAttributes.ts
* DO NOT USE THIS FUNCTION ANYWHERE ELSE
*/
function generateReportAttributes({
report,
reportActions,
transactionViolations,
}: {
report: OnyxEntry<Report>;
reportActions?: ReportActions;
transactionViolations: OnyxCollection<TransactionViolation[]>;
}) {
const isReportSettled = isSettled(report);
const isCurrentUserReportOwner = isReportOwner(report);
const doesReportHasViolations = hasReportViolations(report?.reportID);
const hasViolationsToDisplayInLHN = shouldDisplayViolationsRBRInLHN(report, transactionViolations);
const hasAnyViolations = hasViolationsToDisplayInLHN || (!isReportSettled && isCurrentUserReportOwner && doesReportHasViolations);
const reportErrors = getAllReportErrors(report, reportActions);
const hasErrors = Object.entries(reportErrors ?? {}).length > 0;
const oneTransactionThreadReportID = getOneTransactionThreadReportID(report?.reportID, reportActions);
const parentReportAction = report?.parentReportActionID ? reportActions?.[report.parentReportActionID] : undefined;
const requiresAttention = requiresAttentionFromCurrentUser(report, parentReportAction);

return {
doesReportHasViolations,
hasViolationsToDisplayInLHN,
hasAnyViolations,
reportErrors,
hasErrors,
oneTransactionThreadReportID,
parentReportAction,
requiresAttention,
};
}
Comment on lines +10476 to +10506
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created this function to:

  1. Keep reportAttributes.ts as clean as possible
  2. Those values can be later exposed by a derived value and it'll be possible to use them instead of calling those functions multiple times


export {
addDomainToShortMention,
completeShortMention,
Expand Down Expand Up @@ -10834,6 +10871,7 @@ export {
populateOptimisticReportFormula,
getOutstandingReports,
isReportOutstanding,
generateReportAttributes,
};

export type {
Expand Down
59 changes: 52 additions & 7 deletions src/libs/SidebarUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import type {ValueOf} from 'type-fest';
import type {PartialPolicyForSidebar} from '@hooks/useSidebarOrderedReportIDs';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {PersonalDetails, PersonalDetailsList, ReportActions, ReportNameValuePairs, TransactionViolation} from '@src/types/onyx';
import type {PersonalDetails, PersonalDetailsList, ReportActions, ReportAttributes, ReportNameValuePairs, TransactionViolation} from '@src/types/onyx';
import type Beta from '@src/types/onyx/Beta';
import type Policy from '@src/types/onyx/Policy';
import type PriorityMode from '@src/types/onyx/PriorityMode';
Expand Down Expand Up @@ -163,6 +163,14 @@ Onyx.connect({
},
});

let reportAttributes: OnyxEntry<Record<string, ReportAttributes>>;
Onyx.connect({
key: ONYXKEYS.DERIVED.REPORT_ATTRIBUTES,
callback: (value) => {
reportAttributes = value;
},
});

function compareStringDates(a: string, b: string): 0 | 1 | -1 {
if (a < b) {
return -1;
Expand Down Expand Up @@ -340,6 +348,47 @@ type ReasonAndReportActionThatHasRedBrickRoad = {
reportAction?: OnyxEntry<ReportAction>;
};

function hasAnyErrorsOrViolations(report: Report, reportActions: OnyxEntry<ReportActions>, hasAnyViolations: boolean, hasErrors: boolean, singleTransactionThread?: string) {
const {reportAction} = getAllReportActionsErrorsAndReportActionThatRequiresAttention(report, reportActions);
if (isArchivedReportWithID(report.reportID)) {
return false;
}

if (hasAnyViolations) {
return {
reason: CONST.RBR_REASONS.HAS_TRANSACTION_THREAD_VIOLATIONS,
};
}

if (hasErrors) {
return {
reason: CONST.RBR_REASONS.HAS_ERRORS,
reportAction,
};
}

if (singleTransactionThread) {
const transactionID = getTransactionID(singleTransactionThread);
const transaction = getTransaction(transactionID);
if (hasReceiptError(transaction)) {
return {
reason: CONST.RBR_REASONS.HAS_ERRORS,
};
}
}

const parentReportAction = getReportAction(report?.parentReportID, report?.parentReportActionID);
const transactionID = getTransactionID(report.reportID);
const transaction = getTransaction(transactionID);
if (isTransactionThread(parentReportAction) && hasReceiptError(transaction)) {
return {
reason: CONST.RBR_REASONS.HAS_ERRORS,
};
}

return false;
}

function getReasonAndReportActionThatHasRedBrickRoad(
report: Report,
reportActions: OnyxEntry<ReportActions>,
Expand Down Expand Up @@ -410,9 +459,7 @@ function getOptionData({
preferredLocale,
policy,
parentReportAction,
hasViolations,
lastMessageTextFromReport: lastMessageTextFromReportProp,
transactionViolations,
invoiceReceiverPolicy,
}: {
report: OnyxEntry<Report>;
Expand All @@ -423,10 +470,8 @@ function getOptionData({
preferredLocale: DeepValueOf<typeof CONST.LOCALES>;
policy: OnyxEntry<Policy> | undefined;
parentReportAction: OnyxEntry<ReportAction> | undefined;
hasViolations: boolean;
lastMessageTextFromReport?: string;
invoiceReceiverPolicy?: OnyxEntry<Policy>;
transactionViolations?: OnyxCollection<TransactionViolation[]>;
}): OptionData | undefined {
// When a user signs out, Onyx is cleared. Due to the lazy rendering with a virtual list, it's possible for
// this method to be called after the Onyx data has been cleared out. In that case, it's fine to do
Expand Down Expand Up @@ -485,7 +530,7 @@ function getOptionData({
result.isMoneyRequestReport = isMoneyRequestReport(report);
result.shouldShowSubscript = shouldReportShowSubscript(report);
result.pendingAction = report.pendingFields?.addWorkspaceRoom ?? report.pendingFields?.createChat;
result.brickRoadIndicator = shouldShowRedBrickRoad(report, reportActions, hasViolations, transactionViolations) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : '';
result.brickRoadIndicator = reportAttributes?.[report.reportID]?.brickRoadStatus;
result.ownerAccountID = report.ownerAccountID;
result.managerID = report.managerID;
result.reportID = report.reportID;
Expand Down Expand Up @@ -679,7 +724,6 @@ function getOptionData({

if (isJoinRequestInAdminRoom(report)) {
result.isUnread = true;
result.brickRoadIndicator = CONST.BRICK_ROAD_INDICATOR_STATUS.INFO;
}

if (!hasMultipleParticipants) {
Expand Down Expand Up @@ -824,4 +868,5 @@ export default {
getWelcomeMessage,
getReasonAndReportActionThatHasRedBrickRoad,
shouldShowRedBrickRoad,
hasAnyErrorsOrViolations,
};
Loading
Loading