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.

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

Jump to bottom

[Perf] ReportScreen rendering optimisation #30063

Merged
merged 8 commits into from
Oct 23, 2023
Merged

[Perf] ReportScreen rendering optimisation #30063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
61f7064
add useOnyxContext hook
kacper-mikolajczak Oct 20, 2023
93f9ac7
ReportActionItem swap withPersonalDetails to usePersonalDetails
kacper-mikolajczak Oct 20, 2023
0ff1605
ReportActionItemSingle swap withPersonalDetails to usePersonalDetails
kacper-mikolajczak Oct 20, 2023
946ad2b
withCurrentUserPersonalDetails swap withOnyx to usePersonalDetails
kacper-mikolajczak Oct 20, 2023
946d764
withCurrentUserPersonalDetails add default value for back-compat
kacper-mikolajczak Oct 20, 2023
e363e00
add default values for back-compat
kacper-mikolajczak Oct 20, 2023
df96e09
replace inline instances of empty object with a reference to EMPTY_OB…
kacper-mikolajczak Oct 20, 2023
1850885
Merge branch 'main' into perf/report-screen/tti
kacper-mikolajczak Oct 23, 2023
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
3 changes: 2 additions & 1 deletion src/components/OnyxProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

Choose a reason for hiding this comment

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

@marcaaron Let people decide! :D

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);
Expand Down Expand Up @@ -45,6 +45,7 @@ export default OnyxProvider;
export {
withNetwork,
withPersonalDetails,
usePersonalDetails,
withReportActionsDrafts,
withCurrentDate,
withBlockedFromConcierge,
Expand Down
19 changes: 16 additions & 3 deletions src/components/createOnyxContext.tsx
View file
Open in desktop
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';
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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];
};
13 changes: 5 additions & 8 deletions src/pages/home/report/ReportActionItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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';
Expand Down Expand Up @@ -111,7 +110,6 @@ const propTypes = {

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

/** IOU report for this action, if any */
iouReport: reportPropTypes,
Expand All @@ -127,7 +125,6 @@ const defaultProps = {
draftMessage: '',
preferredSkinTone: CONST.EMOJI_DEFAULT_SKIN_TONE,
emojiReactions: {},
personalDetailsList: {},
shouldShowSubscriptAvatar: false,
hasOutstandingIOU: false,
iouReport: undefined,
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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>
Expand Down Expand Up @@ -640,7 +638,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
Expand Down Expand Up @@ -722,7 +720,6 @@ export default compose(
withWindowDimensions,
withLocalize,
withNetwork(),
withPersonalDetails(),
withBlockedFromConcierge({propName: 'blockedFromConcierge'}),
withReportActionsDrafts({
propName: 'draftMessage',
Expand Down
19 changes: 7 additions & 12 deletions src/pages/home/report/ReportActionItemSingle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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),
Expand Down Expand Up @@ -69,7 +65,6 @@ const propTypes = {
};

const defaultProps = {
personalDetailsList: {},
wrapperStyles: [styles.chatItem],
showHeader: true,
shouldShowSubscriptAvatar: false,
Expand All @@ -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);
Expand All @@ -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;
Expand All @@ -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 = {
Expand Down Expand Up @@ -270,7 +266,6 @@ ReportActionItemSingle.displayName = 'ReportActionItemSingle';

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