feat(permissions): Add sorting feature for Users based on role#7654
feat(permissions): Add sorting feature for Users based on role#7654afsuyadi wants to merge 1 commit into
Conversation
|
@afsuyadi is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Code Review
This pull request introduces role-based sorting for users in both the EditPermissions and OrganisationUsersTable components. The review feedback highlights a performance bottleneck in EditPermissions where an O(N * M) lookup is performed on every render; this can be optimized to O(N + M) by using a Set of admin user IDs. Additionally, the reviewer recommends defining the static sorting options arrays outside of both components to provide stable references and prevent breaking memoization in child components.
| import { | ||
| PermissionLevel, | ||
| Req, | ||
| PermissionRoleType, | ||
| SortOrder, | ||
| } from 'common/types/requests' |
There was a problem hiding this comment.
Define the static sorting options array outside of the component to provide a stable reference, preventing unnecessary re-evaluations of memoized hooks in child components.
import {
PermissionLevel,
Req,
PermissionRoleType,
SortOrder,
} from 'common/types/requests'
const USER_SORT_OPTIONS = [
{
label: 'Role',
order: SortOrder.ASC,
value: '_roleRank',
},
]
| const usersWithRoleRank = users?.map((user) => ({ | ||
| ...user, | ||
| _roleRank: | ||
| user.role === 'ADMIN' | ||
| ? 0 | ||
| : permissions?.find((p) => p.user.id === user.id)?.admin | ||
| ? 1 | ||
| : 2, | ||
| })) |
There was a problem hiding this comment.
The current implementation performs an permissions?.find inside users?.map on every render. For organizations with many users and permissions, this can cause noticeable UI lag.
We can optimize this to Set of admin user IDs before mapping. Additionally, we should safely access p.user?.id to prevent potential runtime errors if p.user is undefined.
const adminUserIds = new Set(
permissions
?.filter((p) => p.admin && p.user?.id)
.map((p) => p.user.id) || []
)
const usersWithRoleRank = users?.map((user) => ({
...user,
_roleRank:
user.role === 'ADMIN'
? 0
: adminUserIds.has(user.id)
? 1
: 2,
}))
| sorting={[ | ||
| { | ||
| label: 'Role', | ||
| order: SortOrder.ASC, | ||
| value: '_roleRank', | ||
| }, | ||
| ]} |
| import { SortOrder } from 'common/types/requests' | ||
| interface OrganisationUsersTableProps { |
There was a problem hiding this comment.
Define the static sorting options array outside of the component to provide a stable reference, preventing unnecessary re-evaluations of memoized hooks in child components.
import { SortOrder } from 'common/types/requests'
const ROLE_SORT_OPTIONS = [
{
label: 'Role',
order: SortOrder.ASC,
value: 'role',
},
]
interface OrganisationUsersTableProps {
| sorting={[ | ||
| { | ||
| label: 'Role', | ||
| order: SortOrder.ASC, | ||
| value: 'role', | ||
| }, | ||
| ]} |
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to #7613
Users in organisation, project, and environment permissions settings can now be sorted by role, making it easy to distinguish admisnitrators from standard users at a glance.
Each user is assigned a role rank for sorting:
Sorting ASC puts administrators at the top; toggling reverses the order.
How did you test this code?
Tested manually on a local dev environment:
Screencast+from+01-06-26+14_56_46.mp4