Skip to content

perf: avoid using getOrderedReportIDs when calculating GBR/RBR state #55260

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jan 23, 2025
Merged

perf: avoid using getOrderedReportIDs when calculating GBR/RBR state #55260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6ef0b18
avoid using getOrderedReportIDs when calculating RBR state
TMisiukiewicz Jan 13, 2025
cc48a39
Merge branch 'main' into perf/improve-chat-switch
TMisiukiewicz Jan 15, 2025
20adc9c
fix changed
TMisiukiewicz Jan 15, 2025
b5d2009
Merge remote-tracking branch 'upstream/main' into perf/improve-chat-s…
TMisiukiewicz Jan 20, 2025
f1f00db
Merge remote-tracking branch 'upstream/main' into perf/improve-chat-s…
TMisiukiewicz Jan 23, 2025
52caad0
add early return
TMisiukiewicz Jan 23, 2025
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
23 changes: 5 additions & 18 deletions src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import Text from '@components/Text';
import EducationalTooltip from '@components/Tooltip/EducationalTooltip';
import useActiveWorkspace from '@hooks/useActiveWorkspace';
import useBottomTabIsFocused from '@hooks/useBottomTabIsFocused';
import useCurrentReportID from '@hooks/useCurrentReportID';
import useLocalize from '@hooks/useLocalize';
import {useReportIDs} from '@hooks/useReportIDs';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import getPlatform from '@libs/getPlatform';
Expand Down Expand Up @@ -70,17 +70,10 @@ function BottomTabBar({selectedTab}: BottomTabBarProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
const {activeWorkspaceID} = useActiveWorkspace();
const {currentReportID} = useCurrentReportID() ?? {};
const {orderedReportIDs} = useReportIDs();
const [user] = useOnyx(ONYXKEYS.USER);
const [betas] = useOnyx(ONYXKEYS.BETAS);
const [priorityMode] = useOnyx(ONYXKEYS.NVP_PRIORITY_MODE);
const [reports] = useOnyx(ONYXKEYS.COLLECTION.REPORT);
const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY);
const [reportActions] = useOnyx(ONYXKEYS.COLLECTION.REPORT_ACTIONS);
const [transactionViolations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS);
const [chatTabBrickRoad, setChatTabBrickRoad] = useState<BrickRoad>(() =>
getChatTabBrickRoad(activeWorkspaceID, currentReportID, reports, betas, policies, priorityMode, transactionViolations),
);
const [chatTabBrickRoad, setChatTabBrickRoad] = useState<BrickRoad>(undefined);
const isFocused = useBottomTabIsFocused();
const platform = getPlatform();
const isWebOrDesktop = platform === CONST.PLATFORM.WEB || platform === CONST.PLATFORM.DESKTOP;
Expand All @@ -89,10 +82,10 @@ function BottomTabBar({selectedTab}: BottomTabBarProps) {
selectedTab !== SCREENS.HOME && isFocused,
);
useEffect(() => {
setChatTabBrickRoad(getChatTabBrickRoad(activeWorkspaceID, currentReportID, reports, betas, policies, priorityMode, transactionViolations));
setChatTabBrickRoad(getChatTabBrickRoad(activeWorkspaceID, orderedReportIDs));
// We need to get a new brick road state when report actions are updated, otherwise we'll be showing an outdated brick road.
// That's why reportActions is added as a dependency here
}, [activeWorkspaceID, transactionViolations, reports, reportActions, betas, policies, priorityMode, currentReportID]);
}, [activeWorkspaceID, orderedReportIDs, reportActions]);

const navigateToChats = useCallback(() => {
if (selectedTab === SCREENS.HOME) {
Expand Down Expand Up @@ -138,12 +131,6 @@ function BottomTabBar({selectedTab}: BottomTabBarProps) {
selectedTab={selectedTab}
chatTabBrickRoad={chatTabBrickRoad}
activeWorkspaceID={activeWorkspaceID}
reports={reports}
currentReportID={currentReportID}
betas={betas}
policies={policies}
transactionViolations={transactionViolations}
priorityMode={priorityMode}
/>
)}
<View style={styles.bottomTabBarContainer}>
Expand Down
18 changes: 7 additions & 11 deletions src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/DebugTabView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React, {useCallback, useMemo} from 'react';
import {View} from 'react-native';
import type {OnyxCollection, OnyxEntry} from 'react-native-onyx';
import type {OnyxEntry} from 'react-native-onyx';
import {useOnyx} from 'react-native-onyx';
import Button from '@components/Button';
import Icon from '@components/Icon';
Expand All @@ -9,6 +9,7 @@ import Text from '@components/Text';
import type {IndicatorStatus} from '@hooks/useIndicatorStatus';
import useIndicatorStatus from '@hooks/useIndicatorStatus';
import useLocalize from '@hooks/useLocalize';
import {useReportIDs} from '@hooks/useReportIDs';
import useStyleUtils from '@hooks/useStyleUtils';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
Expand All @@ -21,18 +22,12 @@ import ONYXKEYS from '@src/ONYXKEYS';
import type {Route} from '@src/ROUTES';
import ROUTES from '@src/ROUTES';
import SCREENS from '@src/SCREENS';
import type {Beta, Policy, PriorityMode, ReimbursementAccount, Report, TransactionViolations} from '@src/types/onyx';
import type {ReimbursementAccount} from '@src/types/onyx';

type DebugTabViewProps = {
selectedTab?: string;
chatTabBrickRoad: BrickRoad;
activeWorkspaceID?: string;
currentReportID: string | undefined;
reports: OnyxCollection<Report>;
betas: OnyxEntry<Beta[]>;
policies: OnyxCollection<Policy>;
transactionViolations: OnyxCollection<TransactionViolations>;
priorityMode: OnyxEntry<PriorityMode>;
};

function getSettingsMessage(status: IndicatorStatus | undefined): TranslationPaths | undefined {
Expand Down Expand Up @@ -97,13 +92,14 @@ function getSettingsRoute(status: IndicatorStatus | undefined, reimbursementAcco
}
}

function DebugTabView({selectedTab = '', chatTabBrickRoad, activeWorkspaceID, currentReportID, reports, betas, policies, transactionViolations, priorityMode}: DebugTabViewProps) {
function DebugTabView({selectedTab = '', chatTabBrickRoad, activeWorkspaceID}: DebugTabViewProps) {
const StyleUtils = useStyleUtils();
const theme = useTheme();
const styles = useThemeStyles();
const {translate} = useLocalize();
const [reimbursementAccount] = useOnyx(ONYXKEYS.REIMBURSEMENT_ACCOUNT);
const {status, indicatorColor, policyIDWithErrors} = useIndicatorStatus();
const {orderedReportIDs} = useReportIDs();

const message = useMemo((): TranslationPaths | undefined => {
if (selectedTab === SCREENS.HOME) {
Expand Down Expand Up @@ -137,7 +133,7 @@ function DebugTabView({selectedTab = '', chatTabBrickRoad, activeWorkspaceID, cu

const navigateTo = useCallback(() => {
if (selectedTab === SCREENS.HOME && !!chatTabBrickRoad) {
const report = getChatTabBrickRoadReport(activeWorkspaceID, currentReportID, reports, betas, policies, priorityMode, transactionViolations);
const report = getChatTabBrickRoadReport(activeWorkspaceID, orderedReportIDs);

if (report) {
Navigation.navigate(ROUTES.DEBUG_REPORT.getRoute(report.reportID));
Expand All @@ -150,7 +146,7 @@ function DebugTabView({selectedTab = '', chatTabBrickRoad, activeWorkspaceID, cu
Navigation.navigate(route);
}
}
}, [selectedTab, chatTabBrickRoad, activeWorkspaceID, currentReportID, reports, betas, policies, priorityMode, transactionViolations, status, reimbursementAccount, policyIDWithErrors]);
}, [selectedTab, chatTabBrickRoad, activeWorkspaceID, orderedReportIDs, status, reimbursementAccount, policyIDWithErrors]);

if (!([SCREENS.HOME, SCREENS.SETTINGS.ROOT] as string[]).includes(selectedTab) || !indicator) {
return null;
Expand Down
58 changes: 18 additions & 40 deletions src/libs/WorkspacesSettingsUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,16 @@ import Onyx from 'react-native-onyx';
import type {OnyxCollection, OnyxEntry} from 'react-native-onyx';
import type {ValueOf} from 'type-fest';
import type {LocaleContextProps} from '@components/LocaleContextProvider';
import type {PolicySelector} from '@hooks/useReportIDs';
import CONST from '@src/CONST';
import type {TranslationPaths} from '@src/languages/types';
import ONYXKEYS from '@src/ONYXKEYS';
import type {Beta, Policy, PriorityMode, ReimbursementAccount, Report, ReportAction, ReportActions, TransactionViolation, TransactionViolations} from '@src/types/onyx';
import type {Policy, ReimbursementAccount, Report, ReportAction, ReportActions, TransactionViolations} from '@src/types/onyx';
import type {PolicyConnectionSyncProgress, Unit} from '@src/types/onyx/Policy';
import {isConnectionInProgress} from './actions/connections';
import * as CurrencyUtils from './CurrencyUtils';
import {convertToDisplayString} from './CurrencyUtils';
import {isPolicyAdmin, shouldShowCustomUnitsError, shouldShowEmployeeListError, shouldShowPolicyError, shouldShowSyncError, shouldShowTaxRateError} from './PolicyUtils';
import * as ReportActionsUtils from './ReportActionsUtils';
import * as ReportUtils from './ReportUtils';
import SidebarUtils from './SidebarUtils';
import {getOneTransactionThreadReportID} from './ReportActionsUtils';
import {getAllReportErrors, hasReportViolations, isReportOwner, isUnread, isUnreadWithMention, requiresAttentionFromCurrentUser, shouldDisplayViolationsRBRInLHN} from './ReportUtils';

type CheckingMethod = () => boolean;

Expand Down Expand Up @@ -69,13 +67,13 @@ Onyx.connect({
*/
const getBrickRoadForPolicy = (report: Report, altReportActions?: OnyxCollection<ReportActions>): BrickRoad => {
const reportActions = (altReportActions ?? allReportActions)?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`] ?? {};
const reportErrors = ReportUtils.getAllReportErrors(report, reportActions);
const oneTransactionThreadReportID = ReportActionsUtils.getOneTransactionThreadReportID(report.reportID, reportActions);
const reportErrors = getAllReportErrors(report, reportActions);
const oneTransactionThreadReportID = getOneTransactionThreadReportID(report.reportID, reportActions);
let doesReportContainErrors = Object.keys(reportErrors ?? {}).length !== 0 ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined;

if (!doesReportContainErrors) {
const shouldDisplayViolations = ReportUtils.shouldDisplayViolationsRBRInLHN(report, allTransactionViolations);
const shouldDisplayReportViolations = ReportUtils.isReportOwner(report) && ReportUtils.hasReportViolations(report.reportID);
const shouldDisplayViolations = shouldDisplayViolationsRBRInLHN(report, allTransactionViolations);
const shouldDisplayReportViolations = isReportOwner(report) && hasReportViolations(report.reportID);
const hasViolations = shouldDisplayViolations || shouldDisplayReportViolations;
if (hasViolations) {
doesReportContainErrors = CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR;
Expand All @@ -85,7 +83,7 @@ const getBrickRoadForPolicy = (report: Report, altReportActions?: OnyxCollection
if (oneTransactionThreadReportID && !doesReportContainErrors) {
const oneTransactionThreadReport = reportsCollection?.[`${ONYXKEYS.COLLECTION.REPORT}${oneTransactionThreadReportID}`];

if (ReportUtils.shouldDisplayViolationsRBRInLHN(oneTransactionThreadReport, allTransactionViolations)) {
if (shouldDisplayViolationsRBRInLHN(oneTransactionThreadReport, allTransactionViolations)) {
doesReportContainErrors = CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR;
}
}
Expand All @@ -100,8 +98,8 @@ const getBrickRoadForPolicy = (report: Report, altReportActions?: OnyxCollection
const itemParentReportActions = allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`] ?? {};
itemParentReportAction = report.parentReportActionID ? itemParentReportActions[report.parentReportActionID] : undefined;
}
const reportOption = {...report, isUnread: ReportUtils.isUnread(report), isUnreadWithMention: ReportUtils.isUnreadWithMention(report)};
const shouldShowGreenDotIndicator = ReportUtils.requiresAttentionFromCurrentUser(reportOption, itemParentReportAction);
const reportOption = {...report, isUnread: isUnread(report), isUnreadWithMention: isUnreadWithMention(report)};
const shouldShowGreenDotIndicator = requiresAttentionFromCurrentUser(reportOption, itemParentReportAction);
return shouldShowGreenDotIndicator ? CONST.BRICK_ROAD_INDICATOR_STATUS.INFO : undefined;
};

Expand Down Expand Up @@ -138,23 +136,12 @@ function hasWorkspaceSettingsRBR(policy: Policy) {
);
}

function getChatTabBrickRoadReport(
policyID: string | undefined,
currentReportId: string | undefined,
reports: OnyxCollection<Report>,
betas: OnyxEntry<Beta[]>,
policies: OnyxCollection<PolicySelector>,
priorityMode: OnyxEntry<PriorityMode>,
transactionViolations: OnyxCollection<TransactionViolation[]>,
policyMemberAccountIDs: number[] = [],
): OnyxEntry<Report> {
const reportIDs = SidebarUtils.getOrderedReportIDs(currentReportId, reports, betas, policies, priorityMode, transactionViolations, policyID, policyMemberAccountIDs);
if (!reportIDs.length) {
function getChatTabBrickRoadReport(policyID: string | undefined, orderedReportIDs: string[] = []): OnyxEntry<Report> {
if (!orderedReportIDs.length) {
return undefined;
}

const allReports = reportIDs.map((reportID) => reports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]);

const allReports = orderedReportIDs.map((reportID) => reportsCollection?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is necessary, but since we were returning undefined when reportIDs is empty, should we do the same for orderedReportIDs?

Suggested change
const allReports = orderedReportIDs.map((reportID) => reportsCollection?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]);
if (!orderedReportIDs.length) {
return undefined;
}
const allReports = orderedReportIDs.map((reportID) => reportsCollection?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]);

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @adhorodyski @TMisiukiewicz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah we can do that for better readability, updated the PR

// If policyID is undefined, then all reports are checked whether they contain any brick road
const policyReports = policyID ? Object.values(allReports).filter((report) => report?.policyID === policyID) : Object.values(allReports);

Expand All @@ -180,17 +167,8 @@ function getChatTabBrickRoadReport(
return undefined;
}

function getChatTabBrickRoad(
policyID: string | undefined,
currentReportId: string | undefined,
reports: OnyxCollection<Report>,
betas: OnyxEntry<Beta[]>,
policies: OnyxCollection<PolicySelector>,
priorityMode: OnyxEntry<PriorityMode>,
transactionViolations: OnyxCollection<TransactionViolation[]>,
policyMemberAccountIDs: number[] = [],
): BrickRoad | undefined {
const report = getChatTabBrickRoadReport(policyID, currentReportId, reports, betas, policies, priorityMode, transactionViolations, policyMemberAccountIDs);
function getChatTabBrickRoad(policyID: string | undefined, orderedReportIDs: string[]): BrickRoad | undefined {
const report = getChatTabBrickRoadReport(policyID, orderedReportIDs);
return report ? getBrickRoadForPolicy(report) : undefined;
}

Expand Down Expand Up @@ -250,7 +228,7 @@ function getWorkspacesUnreadStatuses(reports: OnyxCollection<Report>): Record<st

// When the only message of a report is deleted lastVisibileActionCreated is not reset leading to wrongly
// setting it Unread so we add additional condition here to avoid read workspace indicator from being bold.
workspacesUnreadStatuses[policyID] = ReportUtils.isUnread(report) && !!report.lastActorAccountID;
workspacesUnreadStatuses[policyID] = isUnread(report) && !!report.lastActorAccountID;
});

return workspacesUnreadStatuses;
Expand Down Expand Up @@ -300,7 +278,7 @@ function getOwnershipChecksDisplayText(
title = translate('workspace.changeOwner.ownerOwesAmountTitle');
text = translate('workspace.changeOwner.ownerOwesAmountText', {
email: ownerOwesAmount?.ownerEmail,
amount: CurrencyUtils.convertToDisplayString(ownerOwesAmount?.amount, ownerOwesAmount?.currency),
amount: convertToDisplayString(ownerOwesAmount?.amount, ownerOwesAmount?.currency),
});
buttonText = translate('workspace.changeOwner.ownerOwesAmountButtonText');
break;
Expand Down
Loading