Skip to content

Commit

Permalink
Merge pull request #38756 from tienifr/fix/37986
Browse files Browse the repository at this point in the history
Performance: Store `personalDetails` loading state in `personalDetailsMetadata` key
  • Loading branch information
puneetlath authored Mar 28, 2024
2 parents b370358 + 6172849 commit 50d35d8
Show file tree
Hide file tree
Showing 6 changed files with 36 additions and 15 deletions.
8 changes: 8 additions & 0 deletions src/ONYXKEYS.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,13 @@ const ONYXKEYS = {
/** Contains all the private personal details of the user */
PRIVATE_PERSONAL_DETAILS: 'private_personalDetails',

/**
* PERSONAL_DETAILS_METADATA is a perf optimization used to hold loading states of each entry in PERSONAL_DETAILS_LIST.
* A lot of components are connected to the PERSONAL_DETAILS_LIST entity and do not care about the loading state.
* Setting the loading state directly on the personal details entry caused a lot of unnecessary re-renders.
*/
PERSONAL_DETAILS_METADATA: 'personalDetailsMetadata',

/** Contains all the info for Tasks */
TASK: 'task',

Expand Down Expand Up @@ -549,6 +556,7 @@ type OnyxValuesMapping = {
[ONYXKEYS.INPUT_FOCUSED]: boolean;
[ONYXKEYS.PERSONAL_DETAILS_LIST]: OnyxTypes.PersonalDetailsList;
[ONYXKEYS.PRIVATE_PERSONAL_DETAILS]: OnyxTypes.PrivatePersonalDetails;
[ONYXKEYS.PERSONAL_DETAILS_METADATA]: Record<string, OnyxTypes.PersonalDetailsMetadata>;
[ONYXKEYS.TASK]: OnyxTypes.Task;
[ONYXKEYS.WORKSPACE_RATE_AND_UNIT]: OnyxTypes.WorkspaceRateAndUnit;
[ONYXKEYS.CURRENCY_LIST]: OnyxTypes.CurrencyList;
Expand Down
6 changes: 3 additions & 3 deletions src/libs/actions/PersonalDetails.ts
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ function openPublicProfilePage(accountID: number) {
const optimisticData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
key: ONYXKEYS.PERSONAL_DETAILS_METADATA,
value: {
[accountID]: {
isLoading: true,
Expand All @@ -298,7 +298,7 @@ function openPublicProfilePage(accountID: number) {
const successData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
key: ONYXKEYS.PERSONAL_DETAILS_METADATA,
value: {
[accountID]: {
isLoading: false,
Expand All @@ -310,7 +310,7 @@ function openPublicProfilePage(accountID: number) {
const failureData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
key: ONYXKEYS.PERSONAL_DETAILS_METADATA,
value: {
[accountID]: {
isLoading: false,
Expand Down
14 changes: 10 additions & 4 deletions src/pages/ProfilePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,17 @@ import type {TranslationPaths} from '@src/languages/types';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import type SCREENS from '@src/SCREENS';
import type {PersonalDetails, PersonalDetailsList, Report, Session} from '@src/types/onyx';
import type {PersonalDetails, PersonalDetailsList, PersonalDetailsMetadata, Report, Session} from '@src/types/onyx';
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import type {EmptyObject} from '@src/types/utils/EmptyObject';

type ProfilePageOnyxProps = {
/** The personal details of the person who is logged in */
personalDetails: OnyxEntry<PersonalDetailsList>;

/** Loading status of the personal details */
personalDetailsMetadata: OnyxEntry<Record<string, PersonalDetailsMetadata>>;

/** The report currently being looked at */
report: OnyxEntry<Report>;

Expand Down Expand Up @@ -74,12 +77,12 @@ const getPhoneNumber = ({login = '', displayName = ''}: PersonalDetails | EmptyO
return login ? Str.removeSMSDomain(login) : '';
};

function ProfilePage({personalDetails, route, session, report}: ProfilePageProps) {
function ProfilePage({personalDetails, personalDetailsMetadata, route, session, report}: ProfilePageProps) {
const styles = useThemeStyles();
const {translate, formatPhoneNumber} = useLocalize();
const accountID = Number(route.params?.accountID ?? 0);
const isCurrentUser = session?.accountID === accountID;
const details: PersonalDetails | EmptyObject = personalDetails?.[accountID] ?? (ValidationUtils.isValidAccountRoute(accountID) ? {} : {isLoading: false, accountID: 0, avatar: ''});
const details: PersonalDetails | EmptyObject = personalDetails?.[accountID] ?? (ValidationUtils.isValidAccountRoute(accountID) ? {} : {accountID: 0, avatar: ''});

const displayName = PersonalDetailsUtils.getDisplayNameOrDefault(details, undefined, undefined, isCurrentUser);
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
Expand All @@ -102,7 +105,7 @@ function ProfilePage({personalDetails, route, session, report}: ProfilePageProps
const phoneOrEmail = isSMSLogin ? getPhoneNumber(details) : login;

const hasMinimumDetails = !isEmptyObject(details.avatar);
const isLoading = Boolean(details?.isLoading) || isEmptyObject(details);
const isLoading = Boolean(personalDetailsMetadata?.[accountID]?.isLoading) || isEmptyObject(details);

// If the API returns an error for some reason there won't be any details and isLoading will get set to false, so we want to show a blocking screen
const shouldShowBlockingView = !hasMinimumDetails && !isLoading;
Expand Down Expand Up @@ -265,6 +268,9 @@ export default withOnyx<ProfilePageProps, ProfilePageOnyxProps>({
personalDetails: {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
},
personalDetailsMetadata: {
key: ONYXKEYS.PERSONAL_DETAILS_METADATA,
},
session: {
key: ONYXKEYS.SESSION,
},
Expand Down
10 changes: 7 additions & 3 deletions src/pages/settings/Profile/ProfileAvatar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,21 @@ import * as ValidationUtils from '@libs/ValidationUtils';
import * as PersonalDetails from '@userActions/PersonalDetails';
import ONYXKEYS from '@src/ONYXKEYS';
import type SCREENS from '@src/SCREENS';
import type {PersonalDetailsList} from '@src/types/onyx';
import type {PersonalDetailsList, PersonalDetailsMetadata} from '@src/types/onyx';

type ProfileAvatarOnyxProps = {
personalDetails: OnyxEntry<PersonalDetailsList>;
personalDetailsMetadata: OnyxEntry<Record<string, PersonalDetailsMetadata>>;
isLoadingApp: OnyxEntry<boolean>;
};

type ProfileAvatarProps = ProfileAvatarOnyxProps & StackScreenProps<AuthScreensParamList, typeof SCREENS.PROFILE_AVATAR>;

function ProfileAvatar({route, personalDetails, isLoadingApp = true}: ProfileAvatarProps) {
function ProfileAvatar({route, personalDetails, personalDetailsMetadata, isLoadingApp = true}: ProfileAvatarProps) {
const personalDetail = personalDetails?.[route.params.accountID];
const avatarURL = personalDetail?.avatar ?? '';
const accountID = Number(route.params.accountID ?? '');
const isLoading = personalDetail?.isLoading ?? (isLoadingApp && !Object.keys(personalDetail ?? {}).length);
const isLoading = personalDetailsMetadata?.[accountID]?.isLoading ?? (isLoadingApp && !Object.keys(personalDetail ?? {}).length);
const displayName = PersonalDetailsUtils.getDisplayNameOrDefault(personalDetail);

useEffect(() => {
Expand Down Expand Up @@ -55,6 +56,9 @@ export default withOnyx<ProfileAvatarProps, ProfileAvatarOnyxProps>({
personalDetails: {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
},
personalDetailsMetadata: {
key: ONYXKEYS.PERSONAL_DETAILS_METADATA,
},
isLoadingApp: {
key: ONYXKEYS.IS_LOADING_APP,
},
Expand Down
10 changes: 6 additions & 4 deletions src/types/onyx/PersonalDetails.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,6 @@ type PersonalDetails = OnyxCommon.OnyxValueWithOfflineFeedback<{
/** Flag for checking if data is from optimistic data */
isOptimisticPersonalDetail?: boolean;

/** Whether we are loading the data via the API */
isLoading?: boolean;

/** Field-specific server side errors keyed by microtime */
errorFields?: OnyxCommon.ErrorFields<'avatar'>;

Expand All @@ -81,8 +78,13 @@ type PersonalDetails = OnyxCommon.OnyxValueWithOfflineFeedback<{
status?: Status;
}>;

type PersonalDetailsMetadata = {
/** Whether we are waiting for the data to load via the API */
isLoading?: boolean;
};

type PersonalDetailsList = Record<string, PersonalDetails | null>;

export default PersonalDetails;

export type {Timezone, Status, SelectedTimezone, PersonalDetailsList};
export type {Timezone, Status, SelectedTimezone, PersonalDetailsList, PersonalDetailsMetadata};
3 changes: 2 additions & 1 deletion src/types/onyx/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import type Network from './Network';
import type {OnyxUpdateEvent, OnyxUpdatesFromServer} from './OnyxUpdatesFromServer';
import type {DecisionName, OriginalMessageIOU} from './OriginalMessage';
import type PersonalBankAccount from './PersonalBankAccount';
import type {PersonalDetailsList} from './PersonalDetails';
import type {PersonalDetailsList, PersonalDetailsMetadata} from './PersonalDetails';
import type PersonalDetails from './PersonalDetails';
import type PlaidData from './PlaidData';
import type Policy from './Policy';
Expand Down Expand Up @@ -110,6 +110,7 @@ export type {
PersonalBankAccount,
PersonalDetails,
PersonalDetailsList,
PersonalDetailsMetadata,
PlaidData,
Policy,
PolicyCategories,
Expand Down

0 comments on commit 50d35d8

Please sign in to comment.