From 22acadf4ccd9af0cd6ca5ae6a17cf2ba09dd39c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Mon, 15 Jan 2024 08:37:53 +0000 Subject: [PATCH] fix: role form sluggishness (#5888) This seems to improve the performance in the role form while still maintaining the same validation logic. A big factor was the memoization of the categories calculation and respective elements, which is especially impactful when there are many environments. --- .../admin/roles/RoleForm/RoleForm.tsx | 98 +++------------- .../RolePermissionCategories.tsx | 110 ++++++++++++++++++ .../RolePermissionCategory.tsx} | 6 +- .../admin/roles/RoleForm/useRoleForm.ts | 27 +---- .../admin/roles/RoleModal/RoleModal.tsx | 2 + 5 files changed, 137 insertions(+), 106 deletions(-) create mode 100644 frontend/src/component/admin/roles/RoleForm/RolePermissionCategories/RolePermissionCategories.tsx rename frontend/src/component/admin/roles/RoleForm/{PermissionAccordion/PermissionAccordion.tsx => RolePermissionCategories/RolePermissionCategory.tsx} (97%) diff --git a/frontend/src/component/admin/roles/RoleForm/RoleForm.tsx b/frontend/src/component/admin/roles/RoleForm/RoleForm.tsx index 5e47919d5e..fcef2a4f1d 100644 --- a/frontend/src/component/admin/roles/RoleForm/RoleForm.tsx +++ b/frontend/src/component/admin/roles/RoleForm/RoleForm.tsx @@ -1,29 +1,11 @@ import { Alert, styled } from '@mui/material'; import Input from 'component/common/Input/Input'; -import { PermissionAccordion } from './PermissionAccordion/PermissionAccordion'; -import { - Person as UserIcon, - Topic as TopicIcon, - CloudCircle as CloudCircleIcon, -} from '@mui/icons-material'; -import { ICheckedPermissions, IPermission } from 'interfaces/permissions'; +import { ICheckedPermissions } from 'interfaces/permissions'; import { IRoleFormErrors } from './useRoleForm'; -import { - flattenProjectPermissions, - getCategorizedProjectPermissions, - getCategorizedRootPermissions, - toggleAllPermissions, - togglePermission, -} from 'utils/permissions'; -import usePermissions from 'hooks/api/getters/usePermissions/usePermissions'; import { PredefinedRoleType } from 'interfaces/role'; -import { - ENVIRONMENT_PERMISSION_TYPE, - PROJECT_PERMISSION_TYPE, - PROJECT_ROLE_TYPES, - ROOT_ROLE_TYPE, -} from '@server/util/constants'; +import { ROOT_ROLE_TYPE } from '@server/util/constants'; import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; +import { RolePermissionCategories } from './RolePermissionCategories/RolePermissionCategories'; const StyledInputDescription = styled('p')(({ theme }) => ({ display: 'flex', @@ -55,6 +37,7 @@ interface IRoleFormProps { setCheckedPermissions: React.Dispatch< React.SetStateAction >; + validatePermissions: (permissions: ICheckedPermissions) => boolean; errors: IRoleFormErrors; showErrors: boolean; } @@ -71,41 +54,8 @@ export const RoleForm = ({ showErrors, validateName, validateDescription, + validatePermissions, }: IRoleFormProps) => { - const { permissions } = usePermissions({ - revalidateIfStale: false, - revalidateOnReconnect: false, - revalidateOnFocus: false, - }); - - const isProjectRole = PROJECT_ROLE_TYPES.includes(type); - - const categories = isProjectRole - ? getCategorizedProjectPermissions( - flattenProjectPermissions( - permissions.project, - permissions.environments, - ), - ) - : getCategorizedRootPermissions(permissions.root); - - const onPermissionChange = (permission: IPermission) => { - const newCheckedPermissions = togglePermission( - checkedPermissions, - permission, - ); - setCheckedPermissions(newCheckedPermissions); - }; - - const onCheckAll = (permissions: IPermission[]) => { - const newCheckedPermissions = toggleAllPermissions( - checkedPermissions, - permissions, - ); - - setCheckedPermissions(newCheckedPermissions); - }; - const handleOnBlur = (callback: Function) => { setTimeout(() => callback(), 300); }; @@ -121,7 +71,10 @@ export const RoleForm = ({ error={Boolean(errors.name)} errorText={errors.name} value={name} - onChange={(e) => setName(e.target.value)} + onChange={(e) => { + validateName(e.target.value); + setName(e.target.value); + }} onBlur={(e) => handleOnBlur(() => validateName(e.target.value))} autoComplete='off' /> @@ -133,7 +86,10 @@ export const RoleForm = ({ error={Boolean(errors.description)} errorText={errors.description} value={description} - onChange={(e) => setDescription(e.target.value)} + onChange={(e) => { + validateDescription(e.target.value); + setDescription(e.target.value); + }} onBlur={(e) => handleOnBlur(() => validateDescription(e.target.value)) } @@ -145,28 +101,12 @@ export const RoleForm = ({ You must select at least one permission. - {categories.map(({ label, type, permissions }) => ( - - ) : type === ENVIRONMENT_PERMISSION_TYPE ? ( - - ) : ( - - ) - } - permissions={permissions} - checkedPermissions={checkedPermissions} - onPermissionChange={(permission: IPermission) => - onPermissionChange(permission) - } - onCheckAll={() => onCheckAll(permissions)} - /> - ))} + ( diff --git a/frontend/src/component/admin/roles/RoleForm/RolePermissionCategories/RolePermissionCategories.tsx b/frontend/src/component/admin/roles/RoleForm/RolePermissionCategories/RolePermissionCategories.tsx new file mode 100644 index 0000000000..6a622ee651 --- /dev/null +++ b/frontend/src/component/admin/roles/RoleForm/RolePermissionCategories/RolePermissionCategories.tsx @@ -0,0 +1,110 @@ +import { + Person as UserIcon, + Topic as TopicIcon, + CloudCircle as CloudCircleIcon, +} from '@mui/icons-material'; +import { + ENVIRONMENT_PERMISSION_TYPE, + PROJECT_PERMISSION_TYPE, + PROJECT_ROLE_TYPES, +} from '@server/util/constants'; +import usePermissions from 'hooks/api/getters/usePermissions/usePermissions'; +import { ICheckedPermissions, IPermission } from 'interfaces/permissions'; +import { PredefinedRoleType } from 'interfaces/role'; +import { + flattenProjectPermissions, + getCategorizedProjectPermissions, + getCategorizedRootPermissions, + toggleAllPermissions, + togglePermission, +} from 'utils/permissions'; +import { RolePermissionCategory } from './RolePermissionCategory'; +import { useMemo } from 'react'; + +interface IPermissionCategoriesProps { + type: PredefinedRoleType; + checkedPermissions: ICheckedPermissions; + setCheckedPermissions: React.Dispatch< + React.SetStateAction + >; + validatePermissions: (permissions: ICheckedPermissions) => boolean; +} + +export const RolePermissionCategories = ({ + type, + checkedPermissions, + setCheckedPermissions, + validatePermissions, +}: IPermissionCategoriesProps) => { + const { permissions } = usePermissions({ + revalidateIfStale: false, + revalidateOnReconnect: false, + revalidateOnFocus: false, + }); + + const isProjectRole = PROJECT_ROLE_TYPES.includes(type); + + const categories = useMemo( + () => + isProjectRole + ? getCategorizedProjectPermissions( + flattenProjectPermissions( + permissions.project, + permissions.environments, + ), + ) + : getCategorizedRootPermissions(permissions.root), + [permissions, isProjectRole], + ); + + const onPermissionChange = (permission: IPermission) => { + const newCheckedPermissions = togglePermission( + checkedPermissions, + permission, + ); + validatePermissions(newCheckedPermissions); + setCheckedPermissions(newCheckedPermissions); + }; + + const onCheckAll = (permissions: IPermission[]) => { + const newCheckedPermissions = toggleAllPermissions( + checkedPermissions, + permissions, + ); + validatePermissions(newCheckedPermissions); + setCheckedPermissions(newCheckedPermissions); + }; + + return useMemo( + () => ( + <> + {categories.map(({ label, type, permissions }) => ( + + ) : type === ENVIRONMENT_PERMISSION_TYPE ? ( + + ) : ( + + ) + } + permissions={permissions} + checkedPermissions={checkedPermissions} + onPermissionChange={(permission: IPermission) => + onPermissionChange(permission) + } + onCheckAll={() => onCheckAll(permissions)} + /> + ))} + + ), + [categories, checkedPermissions], + ); +}; diff --git a/frontend/src/component/admin/roles/RoleForm/PermissionAccordion/PermissionAccordion.tsx b/frontend/src/component/admin/roles/RoleForm/RolePermissionCategories/RolePermissionCategory.tsx similarity index 97% rename from frontend/src/component/admin/roles/RoleForm/PermissionAccordion/PermissionAccordion.tsx rename to frontend/src/component/admin/roles/RoleForm/RolePermissionCategories/RolePermissionCategory.tsx index ce50bb5fc0..c77f7f2df7 100644 --- a/frontend/src/component/admin/roles/RoleForm/PermissionAccordion/PermissionAccordion.tsx +++ b/frontend/src/component/admin/roles/RoleForm/RolePermissionCategories/RolePermissionCategory.tsx @@ -1,4 +1,4 @@ -import { ReactNode, useMemo, useState, VFC } from 'react'; +import { ReactNode, useMemo, useState } from 'react'; import { Accordion, AccordionDetails, @@ -42,7 +42,7 @@ const StyledTitle = styled(StringTruncator)(({ theme }) => ({ marginRight: theme.spacing(1), })); -export const PermissionAccordion: VFC = ({ +export const RolePermissionCategory = ({ title, permissions, checkedPermissions, @@ -51,7 +51,7 @@ export const PermissionAccordion: VFC = ({ context, onPermissionChange, onCheckAll, -}) => { +}: IEnvironmentPermissionAccordionProps) => { const [expanded, setExpanded] = useState(isInitiallyExpanded); const permissionMap = useMemo( () => diff --git a/frontend/src/component/admin/roles/RoleForm/useRoleForm.ts b/frontend/src/component/admin/roles/RoleForm/useRoleForm.ts index a926d9218a..da20b592ef 100644 --- a/frontend/src/component/admin/roles/RoleForm/useRoleForm.ts +++ b/frontend/src/component/admin/roles/RoleForm/useRoleForm.ts @@ -24,7 +24,7 @@ export const useRoleForm = ( initialDescription = '', initialPermissions: IPermission[] = [], ) => { - const { roles } = useRoles(); + const { roles, projectRoles } = useRoles(); const [name, setName] = useState(initialName); const [description, setDescription] = useState(initialDescription); @@ -47,28 +47,6 @@ export const useRoleForm = ( setCheckedPermissions(newCheckedPermissions); }, [initialPermissions.length]); - useEffect(() => { - if (name !== '') { - validateName(name); - } else { - clearError(ErrorField.NAME); - } - }, [name]); - - useEffect(() => { - if (description !== '') { - validateDescription(description); - } else { - clearError(ErrorField.DESCRIPTION); - } - }, [description]); - - useEffect(() => { - if (validated) { - validatePermissions(checkedPermissions); - } - }, [checkedPermissions]); - const getRolePayload = (type: PredefinedRoleType = ROOT_ROLE_TYPE) => ({ name, description, @@ -79,7 +57,7 @@ export const useRoleForm = ( }); const isNameUnique = (name: string) => { - return !roles.some( + return ![...roles, ...projectRoles].some( (existingRole: IRole) => existingRole.name !== initialName && existingRole.name.toLowerCase() === name.toLowerCase(), @@ -168,6 +146,7 @@ export const useRoleForm = ( validateDescription, checkedPermissions, setCheckedPermissions, + validatePermissions, getRolePayload, errors, showErrors, diff --git a/frontend/src/component/admin/roles/RoleModal/RoleModal.tsx b/frontend/src/component/admin/roles/RoleModal/RoleModal.tsx index eab3fb1d22..e65cc9650a 100644 --- a/frontend/src/component/admin/roles/RoleModal/RoleModal.tsx +++ b/frontend/src/component/admin/roles/RoleModal/RoleModal.tsx @@ -54,6 +54,7 @@ export const RoleModal = ({ validateDescription, checkedPermissions, setCheckedPermissions, + validatePermissions, getRolePayload, errors, showErrors, @@ -143,6 +144,7 @@ export const RoleModal = ({ validateDescription={validateDescription} checkedPermissions={checkedPermissions} setCheckedPermissions={setCheckedPermissions} + validatePermissions={validatePermissions} errors={errors} showErrors={showErrors} />