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

Performance: Store personalDetails loading state in personalDetailsMetadata key #38756

Merged
merged 9 commits into from
Mar 28, 2024
Merged

Performance: Store personalDetails loading state in personalDetailsMetadata key #38756

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
78efe2f
Perf optimization by holding personal details loading state in metada…
tienifr Mar 21, 2024
8678241
Modify metadata comment
tienifr Mar 25, 2024
d35d677
Merge branch 'main' of https://github.com/tienifr/App into fix/37986
tienifr Mar 25, 2024
5eb3f49
Merge branch 'fix/37986' of https://github.com/tienifr/App into fix/3…
tienifr Mar 25, 2024
f09167e
Modify comment
tienifr Mar 25, 2024
e263ba4
Merge branch 'main' of https://github.com/tienifr/App into fix/37986
tienifr Mar 26, 2024
17f18e7
Modify comment for personalDetailsMetadata
tienifr Mar 26, 2024
233d24d
Merge branch 'fix/37986' of https://github.com/tienifr/App into fix/3…
tienifr Mar 26, 2024
6172849
modify comment
tienifr Mar 28, 2024
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
8 changes: 8 additions & 0 deletions src/ONYXKEYS.ts
View file
Open in desktop
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 @@ -546,6 +553,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
View file
Open in desktop
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
View file
Open in desktop
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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to connect to the whole thing, or can we use a selector to just connect to the one accountID we're interested in?

Copy link
Contributor Author

@tienifr tienifr Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There's no way we can retrieve the accountID from route in Onyx selector because selector does not expose the component's props in its parameters. Neither does Onyx key callback, because PERSONAL_DETAILS_METADATA is not a collection type. Wdyt?

},
session: {
key: ONYXKEYS.SESSION,
},
Expand Down
10 changes: 7 additions & 3 deletions src/pages/settings/Profile/ProfileAvatar.tsx
View file
Open in desktop
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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here about selector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same ^

},
isLoadingApp: {
key: ONYXKEYS.IS_LOADING_APP,
},
Expand Down
10 changes: 6 additions & 4 deletions src/types/onyx/PersonalDetails.ts
View file
Open in desktop
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
View file
Open in desktop
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 @@ -109,6 +109,7 @@ export type {
PersonalBankAccount,
PersonalDetails,
PersonalDetailsList,
PersonalDetailsMetadata,
PlaidData,
Policy,
PolicyCategories,
Expand Down
Loading