Skip to content
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

[Perf] ReportScreen rendering optimisation #30063

Merged
merged 8 commits into from
Oct 23, 2023
3 changes: 2 additions & 1 deletion src/components/OnyxProvider.tsx
Original file line number Diff line number Diff line change
@@ -6,7 +6,7 @@ import ComposeProviders from './ComposeProviders';
// Set up any providers for individual keys. This should only be used in cases where many components will subscribe to
// the same key (e.g. FlatList renderItem components)
const [withNetwork, NetworkProvider, NetworkContext] = createOnyxContext(ONYXKEYS.NETWORK);
const [withPersonalDetails, PersonalDetailsProvider] = createOnyxContext(ONYXKEYS.PERSONAL_DETAILS_LIST);
const [withPersonalDetails, PersonalDetailsProvider, , usePersonalDetails] = createOnyxContext(ONYXKEYS.PERSONAL_DETAILS_LIST);
Copy link
Contributor

Choose a reason for hiding this comment

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

100% intentional? just asking :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, good catch! ^^ Edu asked the same here.

tl;dr; It is intentional as the context value is no needed and eslint did not allow _ as a placeholder - so we stick to this - perfectly valid, although quirky syntax :D

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, a valid syntax that looks like a poorly resolved conflict 😂 Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

just wanted to make sure, fine with me then :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

❤️

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL

FWIW I think it might be more obvious to use the eslint disable. This looks really weird to me 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup the syntax is quite peculiar :D Wonder if there is an option in eslint to let us use _ as a placeholder (if I am not mistaken such syntax is used for example in Python).

I am not really a big fan of adding eslint disable comments ^^

Copy link
Contributor

Choose a reason for hiding this comment

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

Where there is a will there is a Stack Overflow! 🎉

Copy link
Contributor Author

@kacper-mikolajczak kacper-mikolajczak Oct 27, 2023

Choose a reason for hiding this comment

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

const [withCurrentDate, CurrentDateProvider] = createOnyxContext(ONYXKEYS.CURRENT_DATE);
const [withReportActionsDrafts, ReportActionsDraftsProvider] = createOnyxContext(ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS);
const [withBlockedFromConcierge, BlockedFromConciergeProvider] = createOnyxContext(ONYXKEYS.NVP_BLOCKED_FROM_CONCIERGE);
@@ -45,6 +45,7 @@ export default OnyxProvider;
export {
withNetwork,
withPersonalDetails,
usePersonalDetails,
withReportActionsDrafts,
withCurrentDate,
withBlockedFromConcierge,
19 changes: 16 additions & 3 deletions src/components/createOnyxContext.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {ComponentType, ForwardRefExoticComponent, ForwardedRef, PropsWithoutRef, ReactNode, RefAttributes, createContext, forwardRef} from 'react';
import React, {ComponentType, ForwardRefExoticComponent, ForwardedRef, PropsWithoutRef, ReactNode, RefAttributes, createContext, forwardRef, useContext} from 'react';
import {withOnyx} from 'react-native-onyx';
import Str from 'expensify-common/lib/str';
import getComponentDisplayName from '../libs/getComponentDisplayName';
@@ -29,7 +29,12 @@ type WithOnyxKey<TOnyxKey extends OnyxKeys> = <TNewOnyxKey extends string = TOny
) => WrapComponentWithConsumer<TNewOnyxKey, TTransformedValue>;

// createOnyxContext return type
type CreateOnyxContext<TOnyxKey extends OnyxKeys> = [WithOnyxKey<TOnyxKey>, ComponentType<Omit<ProviderPropsWithOnyx<TOnyxKey>, TOnyxKey>>, React.Context<OnyxKeyValue<TOnyxKey>>];
type CreateOnyxContext<TOnyxKey extends OnyxKeys> = [
WithOnyxKey<TOnyxKey>,
ComponentType<Omit<ProviderPropsWithOnyx<TOnyxKey>, TOnyxKey>>,
React.Context<OnyxKeyValue<TOnyxKey>>,
() => OnyxValues[TOnyxKey],
];

export default <TOnyxKey extends OnyxKeys>(onyxKeyName: TOnyxKey): CreateOnyxContext<TOnyxKey> => {
const Context = createContext<OnyxKeyValue<TOnyxKey>>(null);
@@ -77,5 +82,13 @@ export default <TOnyxKey extends OnyxKeys>(onyxKeyName: TOnyxKey): CreateOnyxCon
};
}

return [withOnyxKey, ProviderWithOnyx, Context];
const useOnyxContext = () => {
const value = useContext(Context);
if (value === null) {
throw new Error(`useOnyxContext must be used within a OnyxProvider [key: ${onyxKeyName}]`);
}
return value;
};

return [withOnyxKey, ProviderWithOnyx, Context, useOnyxContext];
};
12 changes: 4 additions & 8 deletions src/components/withCurrentUserPersonalDetails.js
Original file line number Diff line number Diff line change
@@ -5,6 +5,8 @@ import getComponentDisplayName from '../libs/getComponentDisplayName';
import ONYXKEYS from '../ONYXKEYS';
import personalDetailsPropType from '../pages/personalDetailsPropType';
import refPropTypes from './refPropTypes';
import {usePersonalDetails} from './OnyxProvider';
import CONST from '../CONST';

const withCurrentUserPersonalDetailsPropTypes = {
currentUserPersonalDetails: personalDetailsPropType,
@@ -18,25 +20,22 @@ export default function (WrappedComponent) {
const propTypes = {
forwardedRef: refPropTypes,

/** Personal details of all the users, including current user */
personalDetails: PropTypes.objectOf(personalDetailsPropType),

/** Session of the current user */
session: PropTypes.shape({
accountID: PropTypes.number,
}),
};
const defaultProps = {
forwardedRef: undefined,
personalDetails: {},
session: {
accountID: 0,
},
};

function WithCurrentUserPersonalDetails(props) {
const personalDetails = usePersonalDetails() || CONST.EMPTY_OBJECT;
const accountID = props.session.accountID;
const accountPersonalDetails = props.personalDetails[accountID];
const accountPersonalDetails = personalDetails[accountID];
const currentUserPersonalDetails = useMemo(() => ({...accountPersonalDetails, accountID}), [accountPersonalDetails, accountID]);
return (
<WrappedComponent
@@ -62,9 +61,6 @@ export default function (WrappedComponent) {
));

return withOnyx({
personalDetails: {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
},
session: {
key: ONYXKEYS.SESSION,
},
13 changes: 5 additions & 8 deletions src/pages/home/report/ReportActionItem.js
Original file line number Diff line number Diff line change
@@ -28,7 +28,7 @@ import MiniReportActionContextMenu from './ContextMenu/MiniReportActionContextMe
import * as ReportActionContextMenu from './ContextMenu/ReportActionContextMenu';
import * as ContextMenuActions from './ContextMenu/ContextMenuActions';
import * as EmojiPickerAction from '../../../libs/actions/EmojiPickerAction';
import {withBlockedFromConcierge, withNetwork, withPersonalDetails, withReportActionsDrafts} from '../../../components/OnyxProvider';
import {usePersonalDetails, withBlockedFromConcierge, withNetwork, withReportActionsDrafts} from '../../../components/OnyxProvider';
import RenameAction from '../../../components/ReportActionItem/RenameAction';
import InlineSystemMessage from '../../../components/InlineSystemMessage';
import styles from '../../../styles/styles';
@@ -49,7 +49,6 @@ import Icon from '../../../components/Icon';
import * as Expensicons from '../../../components/Icon/Expensicons';
import Text from '../../../components/Text';
import DisplayNames from '../../../components/DisplayNames';
import personalDetailsPropType from '../../personalDetailsPropType';
import ReportPreview from '../../../components/ReportActionItem/ReportPreview';
import ReportActionItemDraft from './ReportActionItemDraft';
import TaskPreview from '../../../components/ReportActionItem/TaskPreview';
@@ -111,7 +110,6 @@ const propTypes = {

...windowDimensionsPropTypes,
emojiReactions: EmojiReactionsPropTypes,
personalDetailsList: PropTypes.objectOf(personalDetailsPropType),

/** IOU report for this action, if any */
iouReport: reportPropTypes,
@@ -127,7 +125,6 @@ const defaultProps = {
draftMessage: '',
preferredSkinTone: CONST.EMOJI_DEFAULT_SKIN_TONE,
emojiReactions: {},
personalDetailsList: {},
shouldShowSubscriptAvatar: false,
hasOutstandingIOU: false,
iouReport: undefined,
@@ -136,6 +133,7 @@ const defaultProps = {
};

function ReportActionItem(props) {
const personalDetails = usePersonalDetails() || CONST.EMPTY_OBJECT;
const [isContextMenuActive, setIsContextMenuActive] = useState(ReportActionContextMenu.isActiveReportAction(props.action.reportActionID));
const [isHidden, setIsHidden] = useState(false);
const [moderationDecision, setModerationDecision] = useState(CONST.MODERATION.MODERATOR_DECISION_APPROVED);
@@ -362,7 +360,7 @@ function ReportActionItem(props) {
/>
);
} else if (props.action.actionName === CONST.REPORT.ACTIONS.TYPE.REIMBURSEMENTQUEUED) {
const submitterDisplayName = PersonalDetailsUtils.getDisplayNameOrDefault(props.personalDetailsList, [props.report.ownerAccountID, 'displayName'], props.report.ownerEmail);
const submitterDisplayName = PersonalDetailsUtils.getDisplayNameOrDefault(personalDetails, [props.report.ownerAccountID, 'displayName'], props.report.ownerEmail);
const paymentType = lodashGet(props.action, 'originalMessage.paymentType', '');

const isSubmitterOfUnsettledReport = ReportUtils.isCurrentUserSubmitter(props.report.reportID) && !ReportUtils.isSettled(props.report.reportID);
@@ -508,7 +506,7 @@ function ReportActionItem(props) {
numberOfReplies={numberOfThreadReplies}
mostRecentReply={`${props.action.childLastVisibleActionCreated}`}
isHovered={hovered}
icons={ReportUtils.getIconsForParticipants(oldestFourAccountIDs, props.personalDetailsList)}
icons={ReportUtils.getIconsForParticipants(oldestFourAccountIDs, personalDetails)}
onSecondaryInteraction={showPopover}
/>
</View>
@@ -628,7 +626,7 @@ function ReportActionItem(props) {
const isWhisper = whisperedToAccountIDs.length > 0;
const isMultipleParticipant = whisperedToAccountIDs.length > 1;
const isWhisperOnlyVisibleByUser = isWhisper && ReportUtils.isCurrentUserTheOnlyParticipant(whisperedToAccountIDs);
const whisperedToPersonalDetails = isWhisper ? _.filter(props.personalDetailsList, (details) => _.includes(whisperedToAccountIDs, details.accountID)) : [];
const whisperedToPersonalDetails = isWhisper ? _.filter(personalDetails, (details) => _.includes(whisperedToAccountIDs, details.accountID)) : [];
const displayNamesWithTooltips = isWhisper ? ReportUtils.getDisplayNamesWithTooltips(whisperedToPersonalDetails, isMultipleParticipant) : [];
return (
<PressableWithSecondaryInteraction
@@ -710,7 +708,6 @@ export default compose(
withWindowDimensions,
withLocalize,
withNetwork(),
withPersonalDetails(),
withBlockedFromConcierge({propName: 'blockedFromConcierge'}),
withReportActionsDrafts({
propName: 'draftMessage',
19 changes: 7 additions & 12 deletions src/pages/home/report/ReportActionItemSingle.js
Original file line number Diff line number Diff line change
@@ -9,12 +9,11 @@ import ReportActionItemFragment from './ReportActionItemFragment';
import styles from '../../../styles/styles';
import ReportActionItemDate from './ReportActionItemDate';
import Avatar from '../../../components/Avatar';
import personalDetailsPropType from '../../personalDetailsPropType';
import compose from '../../../libs/compose';
import withLocalize, {withLocalizePropTypes} from '../../../components/withLocalize';
import Navigation from '../../../libs/Navigation/Navigation';
import ROUTES from '../../../ROUTES';
import {withPersonalDetails} from '../../../components/OnyxProvider';
import {usePersonalDetails} from '../../../components/OnyxProvider';
import ControlSelection from '../../../libs/ControlSelection';
import * as ReportUtils from '../../../libs/ReportUtils';
import OfflineWithFeedback from '../../../components/OfflineWithFeedback';
@@ -37,9 +36,6 @@ const propTypes = {
/** All the data of the action */
action: PropTypes.shape(reportActionPropTypes).isRequired,

/** All of the personalDetails */
personalDetailsList: PropTypes.objectOf(personalDetailsPropType),

/** Styles for the outermost View */
// eslint-disable-next-line react/forbid-prop-types
wrapperStyles: PropTypes.arrayOf(PropTypes.object),
@@ -69,7 +65,6 @@ const propTypes = {
};

const defaultProps = {
personalDetailsList: {},
wrapperStyles: [styles.chatItem],
showHeader: true,
shouldShowSubscriptAvatar: false,
@@ -88,9 +83,10 @@ const showWorkspaceDetails = (reportID) => {
};

function ReportActionItemSingle(props) {
const personalDetails = usePersonalDetails() || CONST.EMPTY_OBJECT;
const actorAccountID = props.action.actionName === CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW && props.iouReport ? props.iouReport.managerID : props.action.actorAccountID;
let {displayName} = props.personalDetailsList[actorAccountID] || {};
const {avatar, login, pendingFields, status, fallbackIcon} = props.personalDetailsList[actorAccountID] || {};
let {displayName} = personalDetails[actorAccountID] || {};
kacper-mikolajczak marked this conversation as resolved.
Show resolved Hide resolved
const {avatar, login, pendingFields, status, fallbackIcon} = personalDetails[actorAccountID] || {};
let actorHint = (login || displayName || '').replace(CONST.REGEX.MERGED_ACCOUNT_PREFIX, '');
const displayAllActors = useMemo(() => props.action.actionName === CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW && props.iouReport, [props.action.actionName, props.iouReport]);
const isWorkspaceActor = ReportUtils.isPolicyExpenseChat(props.report) && (!actorAccountID || displayAllActors);
@@ -100,10 +96,10 @@ function ReportActionItemSingle(props) {
displayName = ReportUtils.getPolicyName(props.report);
actorHint = displayName;
avatarSource = ReportUtils.getWorkspaceAvatar(props.report);
} else if (props.action.delegateAccountID && props.personalDetailsList[props.action.delegateAccountID]) {
} else if (props.action.delegateAccountID && personalDetails[props.action.delegateAccountID]) {
// We replace the actor's email, name, and avatar with the Copilot manually for now. And only if we have their
// details. This will be improved upon when the Copilot feature is implemented.
const delegateDetails = props.personalDetailsList[props.action.delegateAccountID];
const delegateDetails = personalDetails[props.action.delegateAccountID];
const delegateDisplayName = delegateDetails.displayName;
actorHint = `${delegateDisplayName} (${props.translate('reportAction.asCopilot')} ${displayName})`;
displayName = actorHint;
@@ -116,7 +112,7 @@ function ReportActionItemSingle(props) {
if (displayAllActors) {
// The ownerAccountID and actorAccountID can be the same if the a user requests money back from the IOU's original creator, in that case we need to use managerID to avoid displaying the same user twice
const secondaryAccountId = props.iouReport.ownerAccountID === actorAccountID ? props.iouReport.managerID : props.iouReport.ownerAccountID;
const secondaryUserDetails = props.personalDetailsList[secondaryAccountId] || {};
const secondaryUserDetails = personalDetails[secondaryAccountId] || {};
const secondaryDisplayName = lodashGet(secondaryUserDetails, 'displayName', '');
displayName = `${primaryDisplayName} & ${secondaryDisplayName}`;
secondaryAvatar = {
@@ -270,7 +266,6 @@ ReportActionItemSingle.displayName = 'ReportActionItemSingle';

export default compose(
withLocalize,
withPersonalDetails(),
withOnyx({
betas: {
key: ONYXKEYS.BETAS,