Skip to content

Commit

Permalink
Report screen performance improvements
Browse files Browse the repository at this point in the history
- Add initial values to different components to allow immediate mounting/rendering
- Delay the initialization of certain values to allow React to flush the queue immediately
- Remove unnecessary Onyx values
  • Loading branch information
ospfranco committed Sep 8, 2023
1 parent 6effb37 commit 3619a25
Show file tree
Hide file tree
Showing 13 changed files with 263 additions and 182 deletions.
1 change: 1 addition & 0 deletions src/components/ExceededCommentLength.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,5 +54,6 @@ ExceededCommentLength.displayName = 'ExceededCommentLength';
export default withOnyx({
comment: {
key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`,
initialValue: null,
},
})(ExceededCommentLength);
2 changes: 2 additions & 0 deletions src/components/ReportActionItem/TaskPreview.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,11 @@ export default compose(
withOnyx({
taskReport: {
key: ({taskReportID}) => `${ONYXKEYS.COLLECTION.REPORT}${taskReportID}`,
initialValue: {},
},
personalDetailsList: {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
initialValue: {},
},
}),
)(TaskPreview);
129 changes: 129 additions & 0 deletions src/libs/Navigation/AppNavigator/ReportScreenIDSetter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
import {useEffect} from 'react';
import PropTypes from 'prop-types';
import lodashGet from 'lodash/get';
import {withOnyx} from 'react-native-onyx';
import ONYXKEYS from '../../../ONYXKEYS';
import * as ReportUtils from '../../ReportUtils';
import reportPropTypes from '../../../pages/reportPropTypes';
import {withNavigationPropTypes} from '../../../components/withNavigation';
import * as App from '../../actions/App';
import usePermissions from '../../../hooks/usePermissions';
import CONST from '../../../CONST';
import Navigation from '../Navigation';

const propTypes = {
/** Available reports that would be displayed in this navigator */
reports: PropTypes.objectOf(reportPropTypes),

/** The policies which the user has access to */
policies: PropTypes.objectOf(
PropTypes.shape({
/** The policy name */
name: PropTypes.string,

/** The type of the policy */
type: PropTypes.string,
}),
),

isFirstTimeNewExpensifyUser: PropTypes.bool,

/** Navigation route context info provided by react navigation */
route: PropTypes.shape({
/** Route specific parameters used on this screen */
params: PropTypes.shape({
/** If the admin room should be opened */
openOnAdminRoom: PropTypes.bool,

/** The ID of the report this screen should display */
reportID: PropTypes.string,
}),
}).isRequired,

...withNavigationPropTypes,
};

const defaultProps = {
reports: {},
policies: {},
isFirstTimeNewExpensifyUser: false,
};

/**
* Get the most recently accessed report for the user
*
* @param {Object} reports
* @param {Boolean} [ignoreDefaultRooms]
* @param {Object} policies
* @param {Boolean} isFirstTimeNewExpensifyUser
* @param {Boolean} openOnAdminRoom
* @returns {Number}
*/
const getLastAccessedReportID = (reports, ignoreDefaultRooms, policies, isFirstTimeNewExpensifyUser, openOnAdminRoom) => {
// If deeplink url is of an attachment, we should show the report that the attachment comes from.
const currentRoute = Navigation.getActiveRoute();
const matches = CONST.REGEX.ATTACHMENT_ROUTE.exec(currentRoute);
const reportID = lodashGet(matches, 1, null);
if (reportID) {
return reportID;
}

const lastReport = ReportUtils.findLastAccessedReport(reports, ignoreDefaultRooms, policies, isFirstTimeNewExpensifyUser, openOnAdminRoom);

return lodashGet(lastReport, 'reportID');
};

// This wrapper is reponsible for opening the last accessed report if there is no reportID specified in the route params
function ReportScreenIDSetter(props) {
const {canUseDefaultRooms} = usePermissions();

useEffect(() => {
// Don't update if there is a reportID in the params already
if (lodashGet(props.route, 'params.reportID', null)) {
App.confirmReadyToOpenApp();
return;
}

// If there is no reportID in route, try to find last accessed and use it for setParams
const reportID = getLastAccessedReportID(
props.reports,
!canUseDefaultRooms,
props.policies,
props.isFirstTimeNewExpensifyUser,
lodashGet(props.route, 'params.openOnAdminRoom', false),
);

// It's possible that props.reports aren't fully loaded yet
// in that case the reportID is undefined
if (reportID) {
performance.mark('ReportScreenConfirmer_hook');
performance.measure('ReportScreenConfirmer_hook_meassure', {start: 'ReportScreenConfirmer_hook', duration: 100});
props.navigation.setParams({reportID: String(reportID)});
} else {
App.confirmReadyToOpenApp();
}
}, [props.route, props.navigation, props.reports, canUseDefaultRooms, props.policies, props.isFirstTimeNewExpensifyUser]);

// The ReportScreen without the reportID set will display a skeleton
// until the reportID is loaded and set in the route param
return null;
}

ReportScreenIDSetter.propTypes = propTypes;
ReportScreenIDSetter.defaultProps = defaultProps;
ReportScreenIDSetter.displayName = 'ReportScreenIDSetter';

export default withOnyx({
reports: {
key: ONYXKEYS.COLLECTION.REPORT,
allowStaleData: true,
},
policies: {
key: ONYXKEYS.COLLECTION.POLICY,
allowStaleData: true,
},
isFirstTimeNewExpensifyUser: {
key: ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER,
initialValue: false,
},
})(ReportScreenIDSetter);
112 changes: 14 additions & 98 deletions src/libs/Navigation/AppNavigator/ReportScreenWrapper.js
Original file line number Diff line number Diff line change
@@ -1,36 +1,10 @@
import React, {useEffect} from 'react';
import PropTypes from 'prop-types';
import lodashGet from 'lodash/get';
import {withOnyx} from 'react-native-onyx';

import ONYXKEYS from '../../../ONYXKEYS';

import ReportScreen from '../../../pages/home/ReportScreen';
import * as ReportUtils from '../../ReportUtils';
import reportPropTypes from '../../../pages/reportPropTypes';
import React from 'react';
import {withNavigationPropTypes} from '../../../components/withNavigation';
import * as App from '../../actions/App';
import usePermissions from '../../../hooks/usePermissions';
import CONST from '../../../CONST';
import Navigation from '../Navigation';
import ReportScreen from '../../../pages/home/ReportScreen';
import ReportScreenIDSetter from './ReportScreenIDSetter';

const propTypes = {
/** Available reports that would be displayed in this navigator */
reports: PropTypes.objectOf(reportPropTypes),

/** The policies which the user has access to */
policies: PropTypes.objectOf(
PropTypes.shape({
/** The policy name */
name: PropTypes.string,

/** The type of the policy */
type: PropTypes.string,
}),
),

isFirstTimeNewExpensifyUser: PropTypes.bool,

/** Navigation route context info provided by react navigation */
route: PropTypes.shape({
/** Route specific parameters used on this screen */
Expand All @@ -46,82 +20,24 @@ const propTypes = {
...withNavigationPropTypes,
};

const defaultProps = {
reports: {},
policies: {},
isFirstTimeNewExpensifyUser: false,
};

/**
* Get the most recently accessed report for the user
*
* @param {Object} reports
* @param {Boolean} [ignoreDefaultRooms]
* @param {Object} policies
* @param {Boolean} isFirstTimeNewExpensifyUser
* @param {Boolean} openOnAdminRoom
* @returns {Number}
*/
const getLastAccessedReportID = (reports, ignoreDefaultRooms, policies, isFirstTimeNewExpensifyUser, openOnAdminRoom) => {
// If deeplink url is of an attachment, we should show the report that the attachment comes from.
const currentRoute = Navigation.getActiveRoute();
const matches = CONST.REGEX.ATTACHMENT_ROUTE.exec(currentRoute);
const reportID = lodashGet(matches, 1, null);
if (reportID) {
return reportID;
}

const lastReport = ReportUtils.findLastAccessedReport(reports, ignoreDefaultRooms, policies, isFirstTimeNewExpensifyUser, openOnAdminRoom);

return lodashGet(lastReport, 'reportID');
};
const defaultProps = {};

// This wrapper is reponsible for opening the last accessed report if there is no reportID specified in the route params
function ReportScreenWrapper(props) {
const {canUseDefaultRooms} = usePermissions();

useEffect(() => {
// Don't update if there is a reportID in the params already
if (lodashGet(props.route, 'params.reportID', null)) {
App.confirmReadyToOpenApp();
return;
}

// If there is no reportID in route, try to find last accessed and use it for setParams
const reportID = getLastAccessedReportID(
props.reports,
!canUseDefaultRooms,
props.policies,
props.isFirstTimeNewExpensifyUser,
lodashGet(props.route, 'params.openOnAdminRoom', false),
);

// It's possible that props.reports aren't fully loaded yet
// in that case the reportID is undefined
if (reportID) {
props.navigation.setParams({reportID: String(reportID)});
} else {
App.confirmReadyToOpenApp();
}
}, [props.route, props.navigation, props.reports, canUseDefaultRooms, props.policies, props.isFirstTimeNewExpensifyUser]);

// The ReportScreen without the reportID set will display a skeleton
// until the reportID is loaded and set in the route param
return <ReportScreen route={props.route} />;
return (
<>
<ReportScreen route={props.route} />
<ReportScreenIDSetter
route={props.route}
navigation={props.navigation}
/>
</>
);
}

ReportScreenWrapper.propTypes = propTypes;
ReportScreenWrapper.defaultProps = defaultProps;
ReportScreenWrapper.displayName = 'ReportScreenWrapper';

export default withOnyx({
reports: {
key: ONYXKEYS.COLLECTION.REPORT,
},
policies: {
key: ONYXKEYS.COLLECTION.POLICY,
},
isFirstTimeNewExpensifyUser: {
key: ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER,
},
})(ReportScreenWrapper);
export default ReportScreenWrapper;
5 changes: 4 additions & 1 deletion src/libs/Navigation/NavigationRoot.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,10 @@ function NavigationRoot(props) {
if (!state) {
return;
}
updateCurrentReportID(state);
// Performance optimization to avoid context consumers to delay first render
setTimeout(() => {
updateCurrentReportID(state);
}, 0);
parseAndLogRoute(state);
animateStatusBarBackgroundColor();
};
Expand Down
36 changes: 8 additions & 28 deletions src/pages/home/HeaderView.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import reportPropTypes from '../reportPropTypes';
import ONYXKEYS from '../../ONYXKEYS';
import ThreeDotsMenu from '../../components/ThreeDotsMenu';
import * as Task from '../../libs/actions/Task';
import reportActionPropTypes from './report/reportActionPropTypes';
import PressableWithoutFeedback from '../../components/Pressable/PressableWithoutFeedback';
import PinButton from '../../components/PinButton';
import TaskHeaderActionButton from '../../components/TaskHeaderActionButton';
Expand All @@ -44,33 +43,22 @@ const propTypes = {
/** Onyx Props */
parentReport: reportPropTypes,

/** The details about the account that the user is signing in with */
account: PropTypes.shape({
/** URL to the assigned guide's appointment booking calendar */
guideCalendarLink: PropTypes.string,
}),
/** URL to the assigned guide's appointment booking calendar */
guideCalendarLink: PropTypes.string,

/** Current user session */
session: PropTypes.shape({
accountID: PropTypes.number,
}),

/** The report actions from the parent report */
// TO DO: Replace with HOC https://github.com/Expensify/App/issues/18769.
// eslint-disable-next-line react/no-unused-prop-types
parentReportActions: PropTypes.objectOf(PropTypes.shape(reportActionPropTypes)),

...windowDimensionsPropTypes,
...withLocalizePropTypes,
};

const defaultProps = {
personalDetails: {},
parentReportActions: {},
report: null,
account: {
guideCalendarLink: null,
},
guideCalendarLink: null,
parentReport: {},
session: {
accountID: 0,
Expand All @@ -92,11 +80,10 @@ function HeaderView(props) {
const parentNavigationSubtitleData = ReportUtils.getParentNavigationSubtitle(reportHeaderData);
const isConcierge = ReportUtils.hasSingleParticipant(props.report) && _.contains(participants, CONST.ACCOUNT_ID.CONCIERGE);
const isAutomatedExpensifyAccount = ReportUtils.hasSingleParticipant(props.report) && ReportUtils.hasAutomatedExpensifyAccountIDs(participants);
const guideCalendarLink = lodashGet(props.account, 'guideCalendarLink');

// We hide the button when we are chatting with an automated Expensify account since it's not possible to contact
// these users via alternative means. It is possible to request a call with Concierge so we leave the option for them.
const shouldShowCallButton = (isConcierge && guideCalendarLink) || (!isAutomatedExpensifyAccount && !isTaskReport);
const shouldShowCallButton = (isConcierge && props.guideCalendarLink) || (!isAutomatedExpensifyAccount && !isTaskReport);
const threeDotMenuItems = [];
if (isTaskReport) {
const canModifyTask = Task.canModifyTask(props.report, props.session.accountID);
Expand Down Expand Up @@ -219,7 +206,7 @@ function HeaderView(props) {
{shouldShowCallButton && (
<VideoChatButtonAndMenu
isConcierge={isConcierge}
guideCalendarLink={guideCalendarLink}
guideCalendarLink={props.guideCalendarLink}
/>
)}
<PinButton report={props.report} />
Expand All @@ -244,17 +231,10 @@ export default compose(
withWindowDimensions,
withLocalize,
withOnyx({
account: {
guideCalendarLink: {
key: ONYXKEYS.ACCOUNT,
selector: (account) =>
account && {
guideCalendarLink: account.guideCalendarLink,
primaryLogin: account.primaryLogin,
},
},
parentReportActions: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`,
canEvict: false,
selector: (account) => (account && account.guideCalendarLink ? account.guideCalendarLink : null),
initialValue: null,
},
parentReport: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT}${report.parentReportID || report.reportID}`,
Expand Down
Loading

0 comments on commit 3619a25

Please sign in to comment.