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

refactor: [M3-8981] - User Preferences optimization with selector pattern (Part 1) #11386

Merged
merged 8 commits into from
Dec 12, 2024
Merged

refactor: [M3-8981] - User Preferences optimization with selector pattern (Part 1) #11386

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ad02ddb
initial refactor
bnussman Dec 9, 2024
a31c62c
fix type-safety issues
bnussman Dec 9, 2024
08308d9
Merge branch 'develop' into M3-8981-preference-optimizations
bnussman Dec 10, 2024
4fe21d3
Merge branch 'develop' into M3-8981-preference-optimizations
bnussman Dec 10, 2024
605fa4f
Update packages/manager/src/queries/profile/preferences.ts
bnussman-akamai Dec 11, 2024
b7e1bc9
Merge branch 'develop' into M3-8981-preference-optimizations
bnussman Dec 11, 2024
d185cc4
fix re-render at maincontent level @mjac0bs
bnussman Dec 11, 2024
f4f9070
Update packages/manager/src/queries/profile/preferences.ts
bnussman-akamai Dec 12, 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
36 changes: 19 additions & 17 deletions packages/manager/src/components/PreferenceToggle/PreferenceToggle.tsx
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to make some changes here to improve type-safety. See other comment in LinodesLanding.tsx

Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,30 @@ import {
usePreferences,
} from 'src/queries/profile/preferences';

export interface PreferenceToggleProps<T> {
preference: T;
togglePreference: () => T;
}
import type { ManagerPreferences } from 'src/types/ManagerPreferences';

interface RenderChildrenProps<T> {
preference: T;
togglePreference: () => T;
preference: NonNullable<T>;
togglePreference: () => NonNullable<T>;
}

type RenderChildren<T> = (props: RenderChildrenProps<T>) => JSX.Element;

interface Props<T> {
children: RenderChildren<T>;
initialSetCallbackFn?: (value: T) => void;
preferenceKey: string;
preferenceOptions: [T, T];
toggleCallbackFn?: (value: T) => void;
value?: T;
interface Props<Key extends keyof ManagerPreferences> {
children: RenderChildren<ManagerPreferences[Key]>;
initialSetCallbackFn?: (value: ManagerPreferences[Key]) => void;
preferenceKey: Key;
preferenceOptions: [ManagerPreferences[Key], ManagerPreferences[Key]];
toggleCallbackFn?: (value: ManagerPreferences[Key]) => void;
value?: ManagerPreferences[Key];
}

export const PreferenceToggle = <T,>(props: Props<T>) => {
/**
* @deprecated There are more simple ways to use preferences. Look into using `usePreferences` directly.
*/
export const PreferenceToggle = <Key extends keyof ManagerPreferences>(
props: Props<Key>
) => {
const {
children,
preferenceKey,
Expand All @@ -38,7 +40,7 @@ export const PreferenceToggle = <T,>(props: Props<T>) => {
const { mutateAsync: updateUserPreferences } = useMutatePreferences();

const togglePreference = () => {
let newPreferenceToSet: T;
let newPreferenceToSet: ManagerPreferences[Key];

if (preferences?.[preferenceKey] === undefined) {
// Because we default to preferenceOptions[0], toggling with no preference should pick preferenceOptions[1]
Expand All @@ -58,11 +60,11 @@ export const PreferenceToggle = <T,>(props: Props<T>) => {
toggleCallbackFn(newPreferenceToSet);
}

return newPreferenceToSet;
return newPreferenceToSet!;
};

return children({
preference: value ?? preferences?.[preferenceKey] ?? preferenceOptions[0],
preference: value ?? preferences?.[preferenceKey] ?? preferenceOptions[0]!,
togglePreference,
});
};
13 changes: 6 additions & 7 deletions packages/manager/src/components/PrimaryNav/PrimaryNav.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,12 @@ export const PrimaryNav = (props: PrimaryNavProps) => {

const { isIAMEnabled, isIAMBeta } = useIsIAMEnabled();

const { data: preferences } = usePreferences();
const { data: collapsedSideNavPreference } = usePreferences(
(preferences) => preferences?.collapsedSideNavProductFamilies
);
Comment on lines +96 to +98
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the new pattern in-use! ✨

Before: The primary nav would re-render when any preference was updated
After: The primary nav will only re-render when preferences.collapsedSideNavProductFamilies updates 🎉

Copy link
Contributor

@pmakode-akamai pmakode-akamai Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This could be unrelated, but I’m noticing that PrimaryNav re-renders when navigating to Create Image, while it doesn’t re-render when selecting any products from the Side Nav or any other Create {Product}.
Screenshot 2024-12-11 at 6 40 12 PM

understood - it's a Create Image, but is PrimaryNav supposed to get re-rendered in such cases??

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that is unrelated. From my understanding, the primary nav will re-render when we change routes no matter what.


const collapsedAccordions = collapsedSideNavPreference ?? [];

const { mutateAsync: updatePreferences } = useMutatePreferences();

const productFamilyLinkGroups: ProductFamilyLinkGroup<
Expand Down Expand Up @@ -253,10 +258,6 @@ export const PrimaryNav = (props: PrimaryNavProps) => {
]
);

const [collapsedAccordions, setCollapsedAccordions] = React.useState<
number[]
>(preferences?.collapsedSideNavProductFamilies ?? []);
Comment on lines -256 to -258
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need to use extra state here. Because our user preferences optimistically update, we can essentially treat it as state.


const accordionClicked = (index: number) => {
let updatedCollapsedAccordions;
if (collapsedAccordions.includes(index)) {
Expand All @@ -266,13 +267,11 @@ export const PrimaryNav = (props: PrimaryNavProps) => {
updatePreferences({
collapsedSideNavProductFamilies: updatedCollapsedAccordions,
});
setCollapsedAccordions(updatedCollapsedAccordions);
} else {
updatedCollapsedAccordions = [...collapsedAccordions, index];
updatePreferences({
collapsedSideNavProductFamilies: updatedCollapsedAccordions,
});
setCollapsedAccordions(updatedCollapsedAccordions);
}
};

Expand Down
12 changes: 8 additions & 4 deletions packages/manager/src/features/Linodes/LinodesDetail/LinodeResize/LinodeResize.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,12 @@ export const LinodeResize = (props: Props) => {
);

const { data: types } = useAllTypes(open);
const { data: preferences } = usePreferences(open);

const { data: typeToConfirmPreference } = usePreferences(
(preferences) => preferences?.type_to_confirm,
open
);

const { enqueueSnackbar } = useSnackbar();
const [confirmationText, setConfirmationText] = React.useState('');
const [resizeError, setResizeError] = React.useState<string>('');
Expand Down Expand Up @@ -162,8 +167,7 @@ export const LinodeResize = (props: Props) => {
const tableDisabled = hostMaintenance || isLinodesGrantReadOnly;

const submitButtonDisabled =
preferences?.type_to_confirm !== false &&
confirmationText !== linode?.label;
typeToConfirmPreference !== false && confirmationText !== linode?.label;

const type = types?.find((t) => t.id === linode?.type);

Expand Down Expand Up @@ -323,7 +327,7 @@ export const LinodeResize = (props: Props) => {
title="Confirm"
typographyStyle={{ marginBottom: 8 }}
value={confirmationText}
visible={preferences?.type_to_confirm}
visible={typeToConfirmPreference}
/>
</Box>
<Box display="flex" justifyContent="flex-end">
Expand Down
9 changes: 4 additions & 5 deletions packages/manager/src/features/Linodes/LinodesLanding/LinodesLanding.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ import type { ExtendedStatus } from './utils';
import type { Config } from '@linode/api-v4/lib/linodes/types';
import type { APIError } from '@linode/api-v4/lib/types';
import type { RouteComponentProps } from 'react-router-dom';
import type { PreferenceToggleProps } from 'src/components/PreferenceToggle/PreferenceToggle';
import type { WithFeatureFlagProps } from 'src/containers/flags.container';
import type { WithProfileProps } from 'src/containers/profile.container';
import type { DialogType } from 'src/features/Linodes/types';
Expand Down Expand Up @@ -308,17 +307,17 @@ class ListLinodes extends React.Component<CombinedProps, State> {
)}
<DocumentTitleSegment segment="Linodes" />
<ProductInformationBanner bannerLocation="Linodes" />
<PreferenceToggle<boolean>
<PreferenceToggle
preferenceKey="linodes_group_by_tag"
preferenceOptions={[false, true]}
toggleCallbackFn={sendGroupByAnalytic}
>
{({
preference: linodesAreGrouped,
togglePreference: toggleGroupLinodes,
}: PreferenceToggleProps<boolean>) => {
Copy link
Member Author

@bnussman-akamai bnussman-akamai Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I improved the type-safety of PreferenceToggle. These explicit type annotations are no longer needed because the type is now properly inferred

}) => {
return (
<PreferenceToggle<'grid' | 'list'>
<PreferenceToggle
preferenceKey="linodes_view_style"
preferenceOptions={['list', 'grid']}
toggleCallbackFn={this.changeView}
Expand All @@ -331,7 +330,7 @@ class ListLinodes extends React.Component<CombinedProps, State> {
{({
preference: linodeViewPreference,
togglePreference: toggleLinodeView,
}: PreferenceToggleProps<'grid' | 'list'>) => {
}) => {
return (
<React.Fragment>
<React.Fragment>
Expand Down
35 changes: 19 additions & 16 deletions packages/manager/src/queries/profile/preferences.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,39 +1,42 @@
import { updateUserPreferences } from '@linode/api-v4';
import {
QueryClient,
useMutation,
useQuery,
useQueryClient,
} from '@tanstack/react-query';

import { ManagerPreferences } from 'src/types/ManagerPreferences';
import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query';

import { queryPresets } from '../base';
import { profileQueries } from './profile';

import type { APIError } from '@linode/api-v4';
import type { QueryClient } from '@tanstack/react-query';
import type { ManagerPreferences } from 'src/types/ManagerPreferences';

export const usePreferences = (enabled = true) =>
useQuery<ManagerPreferences, APIError[]>({
export const usePreferences = <TData = ManagerPreferences>(
bnussman-akamai marked this conversation as resolved.
Show resolved Hide resolved
select?: (data: ManagerPreferences | undefined) => TData,
enabled = true
) =>
useQuery({
...profileQueries.preferences,
...queryPresets.oneTimeFetch,
enabled,
select,
});

export const useMutatePreferences = (replace = false) => {
const { data: preferences } = usePreferences(!replace);
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed this use of usePreferences and implemented mutationFn differently.

Using usePreferences here would cause any components that uses useMutatePreferences to re-render when preferences update. This change will eliminate that path to extra re-renders.

const queryClient = useQueryClient();

return useMutation<
ManagerPreferences,
APIError[],
Partial<ManagerPreferences>
>({
mutationFn: (data) =>
updateUserPreferences({
...(!replace && preferences !== undefined ? preferences : {}),
...data,
}),
async mutationFn(data) {
if (replace) {
return updateUserPreferences(data);
} else {
const existingPreferences = await queryClient.ensureQueryData<ManagerPreferences>(
profileQueries.preferences
);
return updateUserPreferences({ ...existingPreferences, ...data });
}
},
bnussman-akamai marked this conversation as resolved.
Show resolved Hide resolved
onMutate: (data) => updatePreferenceData(data, replace, queryClient),
});
};
Expand Down
6 changes: 4 additions & 2 deletions packages/manager/src/types/ManagerPreferences.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { UserPreferences } from '@linode/api-v4';
import type { AclpConfig } from '@linode/api-v4';
import type { Order } from 'src/hooks/useOrder';
import type { ThemeChoice } from 'src/utilities/theme';

Expand All @@ -14,9 +14,11 @@ export interface DismissedNotification {
label?: string;
}

export interface ManagerPreferences extends UserPreferences {
export interface ManagerPreferences {
Copy link
Member Author

Choose a reason for hiding this comment

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

This extends was causing type-safety issues. It basically made ManagerPreferences extend Record<string, any>, which allowed any string key. Removing the extends allow us to be more strict.

aclpPreference?: AclpConfig; // Why is this type in @linode/api-v4?
avatarColor?: string;
backups_cta_dismissed?: boolean;
collapsedSideNavProductFamilies?: number[];
desktop_sidebar_open?: boolean;
Copy link
Member Author

@bnussman-akamai bnussman-akamai Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

These were types missing... Now that type-safety is better, developers will see type errors that will catch this

dismissed_notifications?: Record<string, DismissedNotification>;
domains_group_by_tag?: boolean;
Expand Down
12 changes: 9 additions & 3 deletions packages/manager/src/utilities/theme.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,19 @@ export const getThemeFromPreferenceValue = (
};

export const useColorMode = () => {
// Make sure we are authenticated before we fetch preferences.
const isAuthenticated = !!useAuthentication().token;
const { data: preferences } = usePreferences(isAuthenticated);

const { data: themePreference } = usePreferences(
(preferences) => preferences?.theme,
// Make sure we are authenticated before we fetch preferences.
// If we don't, we get an authentication loop.
isAuthenticated
);

const isSystemInDarkMode = useMediaQuery('(prefers-color-scheme: dark)');

const colorMode = getThemeFromPreferenceValue(
preferences?.theme,
themePreference,
isSystemInDarkMode
);

Expand Down
Loading