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 part 2 #30354

Merged
merged 11 commits into from
Nov 2, 2023
Merged

[Perf] ReportScreen rendering optimisation part 2 #30354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1b5ad38
BaseUserDetailsTooltip
kacper-mikolajczak Oct 25, 2023
d8ee1fb
TaskPreview
kacper-mikolajczak Oct 25, 2023
fe8c194
withCurrentUserPersonalDetails
kacper-mikolajczak Oct 25, 2023
87cb616
Merge branch 'main' into perf/report-screen-tti-2
kacper-mikolajczak Oct 27, 2023
1945ce6
add missing function execution
kacper-mikolajczak Oct 30, 2023
bd84424
Merge branch 'main' into perf/report-screen-tti-2
kacper-mikolajczak Oct 30, 2023
b4a9f5d
resolve aliases in CurrentUserPersonalDetails
kacper-mikolajczak Oct 30, 2023
2a77766
fix prettier issues
kacper-mikolajczak Oct 30, 2023
d35caae
merge main
kacper-mikolajczak Nov 2, 2023
87916f6
swap || to ??
kacper-mikolajczak Nov 2, 2023
0543ad1
add waiter in UnreadIndicatorsTest
kacper-mikolajczak Nov 2, 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
15 changes: 4 additions & 11 deletions src/components/ReportActionItem/TaskPreview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import _ from 'underscore';
import Checkbox from '@components/Checkbox';
import Icon from '@components/Icon';
import * as Expensicons from '@components/Icon/Expensicons';
import {usePersonalDetails} from '@components/OnyxProvider';
import PressableWithoutFeedback from '@components/Pressable/PressableWithoutFeedback';
import RenderHTML from '@components/RenderHTML';
import withCurrentUserPersonalDetails, {withCurrentUserPersonalDetailsDefaultProps, withCurrentUserPersonalDetailsPropTypes} from '@components/withCurrentUserPersonalDetails';
Expand All @@ -17,7 +18,6 @@ import * as LocalePhoneNumber from '@libs/LocalePhoneNumber';
import Navigation from '@libs/Navigation/Navigation';
import * as ReportUtils from '@libs/ReportUtils';
import reportActionPropTypes from '@pages/home/report/reportActionPropTypes';
import personalDetailsPropType from '@pages/personalDetailsPropType';
import styles from '@styles/styles';
import * as StyleUtils from '@styles/StyleUtils';
import * as Session from '@userActions/Session';
Expand All @@ -27,9 +27,6 @@ import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';

const propTypes = {
/** All personal details asssociated with user */
personalDetailsList: PropTypes.objectOf(personalDetailsPropType),

/** The ID of the associated taskReport */
taskReportID: PropTypes.string.isRequired,

Expand Down Expand Up @@ -59,12 +56,12 @@ const propTypes = {

const defaultProps = {
...withCurrentUserPersonalDetailsDefaultProps,
personalDetailsList: {},
taskReport: {},
isHovered: false,
};

function TaskPreview(props) {
const personalDetails = usePersonalDetails() || CONST.EMPTY_OBJECT;
// The reportAction might not contain details regarding the taskReport
// Only the direct parent reportAction will contain details about the taskReport
// Other linked reportActions will only contain the taskReportID and we will grab the details from there
Expand All @@ -73,8 +70,8 @@ function TaskPreview(props) {
: props.action.childStateNum === CONST.REPORT.STATE_NUM.SUBMITTED && props.action.childStatusNum === CONST.REPORT.STATUS.APPROVED;
const taskTitle = props.taskReport.reportName || props.action.childReportName;
const taskAssigneeAccountID = Task.getTaskAssigneeAccountID(props.taskReport) || props.action.childManagerAccountID;
const assigneeLogin = lodashGet(props.personalDetailsList, [taskAssigneeAccountID, 'login'], '');
const assigneeDisplayName = lodashGet(props.personalDetailsList, [taskAssigneeAccountID, 'displayName'], '');
const assigneeLogin = lodashGet(personalDetails, [taskAssigneeAccountID, 'login'], '');
const assigneeDisplayName = lodashGet(personalDetails, [taskAssigneeAccountID, 'displayName'], '');
const taskAssignee = assigneeDisplayName || LocalePhoneNumber.formatPhoneNumber(assigneeLogin);
const htmlForTaskPreview =
taskAssignee && taskAssigneeAccountID !== 0
Expand Down Expand Up @@ -132,9 +129,5 @@ export default compose(
key: ({taskReportID}) => `${ONYXKEYS.COLLECTION.REPORT}${taskReportID}`,
initialValue: {},
},
personalDetailsList: {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
initialValue: {},
},
}),
)(TaskPreview);
14 changes: 5 additions & 9 deletions src/components/UserDetailsTooltip/BaseUserDetailsTooltip.web.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,23 @@ import Str from 'expensify-common/lib/str';
import lodashGet from 'lodash/get';
import React, {useCallback} from 'react';
import {Text, View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
import Avatar from '@components/Avatar';
import {usePersonalDetails} from '@components/OnyxProvider';
import Tooltip from '@components/Tooltip';
import useLocalize from '@hooks/useLocalize';
import * as LocalePhoneNumber from '@libs/LocalePhoneNumber';
import * as ReportUtils from '@libs/ReportUtils';
import * as UserUtils from '@libs/UserUtils';
import styles from '@styles/styles';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import {defaultProps, propTypes} from './userDetailsTooltipPropTypes';

function BaseUserDetailsTooltip(props) {
const {translate} = useLocalize();
const personalDetails = usePersonalDetails();

const userDetails = lodashGet(props.personalDetailsList, props.accountID, props.fallbackUserDetails);
const userDetails = lodashGet(personalDetails, props.accountID, props.fallbackUserDetails);
let userDisplayName = ReportUtils.getDisplayNameForParticipant(props.accountID);
let userLogin = (userDetails.login || '').trim() && !_.isEqual(userDetails.login, userDetails.displayName) ? Str.removeSMSDomain(userDetails.login) : '';
let userAvatar = userDetails.avatar;
Expand All @@ -27,7 +27,7 @@ function BaseUserDetailsTooltip(props) {
// We replace the actor's email, name, and avatar with the Copilot manually for now. This will be improved upon when
// the Copilot feature is implemented.
if (props.delegateAccountID) {
const delegateUserDetails = lodashGet(props.personalDetailsList, props.delegateAccountID, {});
const delegateUserDetails = lodashGet(personalDetails, props.delegateAccountID, {});
const delegateUserDisplayName = ReportUtils.getDisplayNameForParticipant(props.delegateAccountID);
userDisplayName = `${delegateUserDisplayName} (${translate('reportAction.asCopilot')} ${userDisplayName})`;
userLogin = delegateUserDetails.login;
Expand Down Expand Up @@ -79,8 +79,4 @@ BaseUserDetailsTooltip.propTypes = propTypes;
BaseUserDetailsTooltip.defaultProps = defaultProps;
BaseUserDetailsTooltip.displayName = 'BaseUserDetailsTooltip';

export default withOnyx({
personalDetailsList: {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
},
})(BaseUserDetailsTooltip);
export default BaseUserDetailsTooltip;
11 changes: 4 additions & 7 deletions src/components/withCurrentUserPersonalDetails.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,14 @@ import React, {ComponentType, ForwardedRef, RefAttributes, useMemo} from 'react'
import {OnyxEntry, withOnyx} from 'react-native-onyx';
import getComponentDisplayName from '@libs/getComponentDisplayName';
import personalDetailsPropType from '@pages/personalDetailsPropType';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {PersonalDetails, Session} from '@src/types/onyx';
import {usePersonalDetails} from './OnyxProvider';

type CurrentUserPersonalDetails = PersonalDetails | Record<string, never>;

type OnyxProps = {
/** Personal details of all the users, including current user */
personalDetails: OnyxEntry<Record<string, PersonalDetails>>;

/** Session of the current user */
session: OnyxEntry<Session>;
};
Expand All @@ -34,8 +33,9 @@ export default function <TProps extends ComponentProps, TRef>(
WrappedComponent: ComponentType<TProps & RefAttributes<TRef>>,
): ComponentType<Omit<Omit<TProps, keyof HOCProps> & RefAttributes<TRef>, keyof OnyxProps>> {
function WithCurrentUserPersonalDetails(props: Omit<TProps, keyof HOCProps>, ref: ForwardedRef<TRef>) {
const personalDetails = usePersonalDetails() ?? CONST.EMPTY_OBJECT;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is against https://github.com/Expensify/App/blob/main/contributingGuides/STYLE.md#accessing-object-properties-and-default-values

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eVoloshchak For TS files I think we should adhere to these nullish-coalescing rules 👍

Here is an example that is directly line below code you've mentioned:

const personalDetails = usePersonalDetails() ?? CONST.EMPTY_OBJECT;
const accountID = props.session?.accountID ?? 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we shouldn't use async/await, use the native Promise instead

Answered by Jack already.

we're calling waitForBatchedUpdatesWithAct twice, could you explain why is that needed and how does this resolve the issue? Doesn't seem right.

It was mostly trial and error process, but what I found out is that there is a race condition in the test that did not make the tests in any case deterministic (sometimes on test failed, another time two of them failed). In this case waitForBatchedUpdatesWithAct helps by waiting for next "tick". Jokingly, we could say that the code now is too fast :D

was this test broken by this particular PR? If it's unrelated to this PR, we should fix it in a separate PR to avoid confusion in the future

It was caused by this PR

Hope that answers your concerns 👍

const accountID = props.session?.accountID ?? 0;
const accountPersonalDetails = props.personalDetails?.[accountID];
const accountPersonalDetails = personalDetails?.[accountID];
const currentUserPersonalDetails: CurrentUserPersonalDetails = useMemo(
() => (accountPersonalDetails ? {...accountPersonalDetails, accountID} : {}),
[accountPersonalDetails, accountID],
Expand All @@ -55,9 +55,6 @@ export default function <TProps extends ComponentProps, TRef>(
const withCurrentUserPersonalDetails = React.forwardRef(WithCurrentUserPersonalDetails);

return withOnyx<Omit<TProps, keyof HOCProps> & RefAttributes<TRef>, OnyxProps>({
personalDetails: {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
},
session: {
key: ONYXKEYS.SESSION,
},
Expand Down
3 changes: 2 additions & 1 deletion tests/ui/UnreadIndicatorsTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,8 @@ function signInAndGetAppWithUnreadChat() {
// Render the App and sign in as a test user.
render(<App />);
return waitForBatchedUpdatesWithAct()
.then(() => {
.then(async () => {
await waitForBatchedUpdatesWithAct();
const hintText = Localize.translateLocal('loginForm.loginForm');
const loginForm = screen.queryAllByLabelText(hintText);
expect(loginForm).toHaveLength(1);
Expand Down
Loading