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.
Unleash · Jan 15, 2024 · 22acadf · 22acadf
1 parent ebd673e
commit 22acadf
Show file tree

Hide file tree

Showing 5 changed files with 137 additions and 106 deletions.
diff --git 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<ICheckedPermissions>
     >;
+    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 = ({
             <Alert severity='info'>
                 You must select at least one permission.
             </Alert>
-            {categories.map(({ label, type, permissions }) => (
-                <PermissionAccordion
-                    key={label}
-                    title={`${label} permissions`}
-                    context={label.toLowerCase()}
-                    Icon={
-                        type === PROJECT_PERMISSION_TYPE ? (
-                            <TopicIcon color='disabled' sx={{ mr: 1 }} />
-                        ) : type === ENVIRONMENT_PERMISSION_TYPE ? (
-                            <CloudCircleIcon color='disabled' sx={{ mr: 1 }} />
-                        ) : (
-                            <UserIcon color='disabled' sx={{ mr: 1 }} />
-                        )
-                    }
-                    permissions={permissions}
-                    checkedPermissions={checkedPermissions}
-                    onPermissionChange={(permission: IPermission) =>
-                        onPermissionChange(permission)
-                    }
-                    onCheckAll={() => onCheckAll(permissions)}
-                />
-            ))}
+            <RolePermissionCategories
+                type={type}
+                checkedPermissions={checkedPermissions}
+                setCheckedPermissions={setCheckedPermissions}
+                validatePermissions={validatePermissions}
+            />
             <ConditionallyRender
                 condition={showErrors}
                 show={() => (

diff --git a/.../src/component/admin/roles/RoleForm/RolePermissionCategories/RolePermissionCategories.tsx b/.../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<ICheckedPermissions>
+    >;
+    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 }) => (
+                    <RolePermissionCategory
+                        key={label}
+                        title={`${label} permissions`}
+                        context={label.toLowerCase()}
+                        Icon={
+                            type === PROJECT_PERMISSION_TYPE ? (
+                                <TopicIcon color='disabled' sx={{ mr: 1 }} />
+                            ) : type === ENVIRONMENT_PERMISSION_TYPE ? (
+                                <CloudCircleIcon
+                                    color='disabled'
+                                    sx={{ mr: 1 }}
+                                />
+                            ) : (
+                                <UserIcon color='disabled' sx={{ mr: 1 }} />
+                            )
+                        }
+                        permissions={permissions}
+                        checkedPermissions={checkedPermissions}
+                        onPermissionChange={(permission: IPermission) =>
+                            onPermissionChange(permission)
+                        }
+                        onCheckAll={() => onCheckAll(permissions)}
+                    />
+                ))}
+            </>
+        ),
+        [categories, checkedPermissions],
+    );
+};
diff --git a/...rmissionAccordion/PermissionAccordion.tsx → ...sionCategories/RolePermissionCategory.tsx b/...rmissionAccordion/PermissionAccordion.tsx → ...sionCategories/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<IEnvironmentPermissionAccordionProps> = ({
+export const RolePermissionCategory = ({
     title,
     permissions,
     checkedPermissions,
@@ -51,7 +51,7 @@ export const PermissionAccordion: VFC<IEnvironmentPermissionAccordionProps> = ({
     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
@@ -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
@@ -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}
                     />