Skip to content

Commit 49570b0

Browse files
ParkreinerParkreiner
committed
refactor: simplify userRoleCell implementation
1 parent 3b36858 commit 49570b0

File tree

2 files changed

+89
-189
lines changed

2 files changed

+89
-189
lines changed

‎site/src/pages/UsersPage/UsersTable/UserRoleCell.tsx

Lines changed: 88 additions & 188 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,18 @@
1-
import { useLayoutEffect, useRef, useState } from "react";
1+
/**
2+
* @file Defines the visual logic for the Roles cell in the Users page table.
3+
*
4+
* The previous implementation tried to dynamically truncate the number of roles
5+
* that would get displayed in a cell, only truncating if there were more roles
6+
* than room in the cell. But there was a problem – that information can't
7+
* exist on the first render, because the DOM nodes haven't been made yet.
8+
*
9+
* The only way to avoid UI flickering was by juggling between useLayoutEffect
10+
* for direct DOM node mutations for any renders that had new data, and normal
11+
* state logic for all other renders. It was clunky, and required duplicating
12+
* the logic in two places (making things easy to accidentally break), so we
13+
* went with a simpler design. If we decide we really do need to display the
14+
* users like that, though, know that it will be painful
15+
*/
216
import { useTheme } from "@emotion/react";
317
import { type User, type Role } from "api/typesGenerated";
418

@@ -7,76 +21,9 @@ import { Pill } from "components/Pill/Pill";
721
import TableCell from "@mui/material/TableCell";
822
import Stack from "@mui/material/Stack";
923

10-
const fallbackRole: Role = {
11-
name: "member",
12-
display_name: "Member",
13-
} as const;
14-
15-
const roleNamesByAccessLevel: readonly string[] = [
16-
"owner",
17-
"user-admin",
18-
"template-admin",
19-
"auditor",
20-
];
21-
22-
function sortRolesByAccessLevel(roles: readonly Role[]) {
23-
return [...roles].sort(
24-
(r1, r2) =>
25-
roleNamesByAccessLevel.indexOf(r1.name) -
26-
roleNamesByAccessLevel.indexOf(r2.name),
27-
);
28-
}
29-
30-
type RoleDisplayInfo = Readonly<{
31-
hasOwner: boolean;
32-
roles: readonly Role[];
33-
}>;
34-
35-
function getRoleDisplayInfo(userRoles: readonly Role[]): RoleDisplayInfo {
36-
if (userRoles.length === 0) {
37-
return {
38-
hasOwner: false,
39-
roles: [fallbackRole],
40-
};
41-
}
42-
43-
const matchedOwnerRole = userRoles.find((role) => role.name === "owner");
44-
if (matchedOwnerRole !== undefined) {
45-
return {
46-
hasOwner: true,
47-
roles: [matchedOwnerRole],
48-
};
49-
}
50-
51-
const sortedRoles = [...userRoles].sort((r1, r2) => {
52-
if (r1.name === r2.name) {
53-
return 0;
54-
}
55-
56-
return r1.name < r2.name ? -1 : 1;
57-
});
58-
59-
return { hasOwner: false, roles: sortedRoles };
60-
}
61-
62-
function getSelectedRoleNames(roles: readonly Role[]) {
63-
const roleNameSet = new Set(roles.map((role) => role.name));
64-
if (roleNameSet.size === 0) {
65-
roleNameSet.add(fallbackRole.name);
66-
}
67-
68-
return roleNameSet;
69-
}
70-
71-
// Defined as a function to ensure that render approach and mutation approaches
72-
// in the component don't get out of sync
73-
function getOverflowButtonText(overflowCount: number) {
74-
return `+${overflowCount} more`;
75-
}
76-
77-
type Props = {
24+
type UserRoleCellProps = {
7825
canEditUsers: boolean;
79-
roles: undefined | readonly Role[];
26+
allAvailableRoles: Role[] | undefined;
8027
user: User;
8128
isLoading: boolean;
8229
oidcRoleSyncEnabled: boolean;
@@ -85,99 +32,24 @@ type Props = {
8532

8633
export function UserRoleCell({
8734
canEditUsers,
88-
roles,
35+
allAvailableRoles,
8936
user,
9037
isLoading,
9138
oidcRoleSyncEnabled,
9239
onUserRolesUpdate,
93-
}: Props) {
40+
}: UserRoleCellProps) {
9441
const theme = useTheme();
9542

96-
// Unless the user happens to be an owner, it is physically impossible for
97-
// React to know how many pills should be omitted for space reasons each time
98-
// a new set of roles comes in. Have to do a smoke-and-mirrors routine to help
99-
// mask that and avoid UI flickering
100-
const cellRef = useRef<HTMLDivElement>(null);
101-
const pillContainerRef = useRef<HTMLDivElement>(null);
102-
const overflowButtonRef = useRef<HTMLButtonElement>(null);
103-
104-
/**
105-
* @todo – The logic only works properly on the first render - the
106-
* moment you update a user's permissions, the UI doesn't do anything, even
107-
* with the gnarly manual state sync in place. The cached update is
108-
* triggering, but not the
109-
*
110-
* Likely causes:
111-
* 1. Mutation logic isn't getting applied properly
112-
* 2. Trace through the parent component logic and see if I need to update
113-
* things to reference the roles prop instead of user.roles
114-
*/
115-
116-
const roleDisplayInfo = getRoleDisplayInfo(user.roles);
117-
118-
// Have to do manual state syncs to make sure that cells change as roles get
119-
// updated; there isn't a good render key to use to simplify this, and the
120-
// MutationObserver API doesn't work with React's order of operations well
121-
// enough to avoid flickering - it's just not fast enough in the right ways
122-
const [cachedUser, setCachedUser] = useState(user);
123-
const [rolesToTruncate, setRolesToTruncate] = useState(
124-
roleDisplayInfo.hasOwner
125-
? user.roles.length - roleDisplayInfo.roles.length
126-
: null,
127-
);
128-
129-
if (user !== cachedUser) {
130-
const needTruncationSync =
131-
user.roles.length !== cachedUser.roles.length ||
132-
user.roles.every((role, index) => role === cachedUser.roles[index]);
133-
134-
setCachedUser(user);
135-
if (needTruncationSync) {
136-
setRolesToTruncate(null);
137-
}
138-
}
139-
140-
// Mutates the contents of the pill container to hide overflowing content on
141-
// the first render, and then updates rolesToTruncate so that these overflow
142-
// calculations can be done with 100% pure state/props calculations for all
143-
// re-renders (at least until the roles list changes by content again)
144-
useLayoutEffect(() => {
145-
const cell = cellRef.current;
146-
const pillContainer = pillContainerRef.current;
147-
if (rolesToTruncate !== null || cell === null || pillContainer === null) {
148-
return;
149-
}
150-
151-
let nodesRemoved = 0;
152-
const childrenCopy = [...pillContainer.children];
153-
154-
for (let i = childrenCopy.length - 1; i >= 0; i--) {
155-
const child = childrenCopy[i] as HTMLElement;
156-
if (pillContainer.clientWidth <= cell.clientWidth) {
157-
break;
158-
}
159-
160-
// Can't remove child, because then React will freak out about DOM nodes
161-
// disappearing in ways it wasn't aware of; have to rely on CSS styling
162-
child.style.visibility = "none";
163-
nodesRemoved++;
164-
}
165-
166-
setRolesToTruncate(nodesRemoved);
167-
if (overflowButtonRef.current !== null) {
168-
const mutationText = getOverflowButtonText(nodesRemoved);
169-
overflowButtonRef.current.innerText = mutationText;
170-
}
171-
}, [rolesToTruncate]);
172-
173-
const finalRoleList = roleDisplayInfo.roles;
43+
const [mainDisplayRole = fallbackRole, ...extraRoles] =
44+
sortRolesByAccessLevel(user.roles ?? []);
45+
const hasOwnerRole = mainDisplayRole.name === "owner";
17446

17547
return (
176-
<TableCell ref={cellRef}>
48+
<TableCell>
17749
<Stack direction="row" spacing={1}>
17850
{canEditUsers && (
17951
<EditRolesButton
180-
roles={sortRolesByAccessLevel(roles ?? [])}
52+
roles={sortRolesByAccessLevel(allAvailableRoles ?? [])}
18153
selectedRoleNames={getSelectedRoleNames(user.roles)}
18254
isLoading={isLoading}
18355
userLoginType={user.login_type}
@@ -193,43 +65,71 @@ export function UserRoleCell({
19365
/>
19466
)}
19567

196-
<Stack direction="row" spacing={1}>
197-
<Stack direction="row" spacing={1} ref={pillContainerRef}>
198-
{finalRoleList.map((role) => {
199-
const isOwnerRole = role.name === "owner";
200-
201-
return (
202-
<Pill
203-
key={role.name}
204-
text={role.display_name}
205-
css={{
206-
backgroundColor: isOwnerRole
207-
? theme.palette.info.dark
208-
: theme.palette.background.paperLight,
209-
borderColor: isOwnerRole
210-
? theme.palette.info.light
211-
: theme.palette.divider,
212-
}}
213-
/>
214-
);
215-
})}
216-
</Stack>
217-
218-
{/*
219-
* Have to render this, even when rolesToTruncate is null, in order
220-
* for the layoutEffect trick to work properly
221-
*/}
222-
{rolesToTruncate !== 0 && (
223-
<Pill
224-
text={getOverflowButtonText(rolesToTruncate ?? 0)}
225-
css={{
226-
backgroundColor: theme.palette.background.paperLight,
227-
borderColor: theme.palette.divider,
228-
}}
229-
/>
230-
)}
231-
</Stack>
68+
<Pill
69+
text={mainDisplayRole.display_name}
70+
css={{
71+
backgroundColor: hasOwnerRole
72+
? theme.palette.info.dark
73+
: theme.palette.background.paperLight,
74+
borderColor: hasOwnerRole
75+
? theme.palette.info.light
76+
: theme.palette.divider,
77+
}}
78+
/>
79+
80+
{extraRoles.length > 0 && <OverflowRolePill roles={extraRoles} />}
23281
</Stack>
23382
</TableCell>
23483
);
23584
}
85+
86+
type OverflowRolePillProps = {
87+
roles: readonly Role[];
88+
};
89+
90+
function OverflowRolePill({ roles }: OverflowRolePillProps) {
91+
const theme = useTheme();
92+
93+
return (
94+
<Pill
95+
text={`+${roles.length} more`}
96+
css={{
97+
backgroundColor: theme.palette.background.paperLight,
98+
borderColor: theme.palette.divider,
99+
}}
100+
/>
101+
);
102+
}
103+
104+
const fallbackRole: Role = {
105+
name: "member",
106+
display_name: "Member",
107+
} as const;
108+
109+
const roleNamesByAccessLevel: readonly string[] = [
110+
"owner",
111+
"user-admin",
112+
"template-admin",
113+
"auditor",
114+
];
115+
116+
function sortRolesByAccessLevel(roles: Role[]) {
117+
if (roles.length === 0) {
118+
return roles;
119+
}
120+
121+
return [...roles].sort(
122+
(r1, r2) =>
123+
roleNamesByAccessLevel.indexOf(r1.name) -
124+
roleNamesByAccessLevel.indexOf(r2.name),
125+
);
126+
}
127+
128+
function getSelectedRoleNames(roles: readonly Role[]) {
129+
const roleNameSet = new Set(roles.map((role) => role.name));
130+
if (roleNameSet.size === 0) {
131+
roleNameSet.add(fallbackRole.name);
132+
}
133+
134+
return roleNameSet;
135+
}

‎site/src/pages/UsersPage/UsersTable/UsersTableBody.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ export const UsersTableBody: FC<
150150

151151
<UserRoleCell
152152
canEditUsers={canEditUsers}
153-
roles={roles}
153+
allAvailableRoles={roles}
154154
user={user}
155155
oidcRoleSyncEnabled={oidcRoleSyncEnabled}
156156
isLoading={Boolean(isUpdatingUserRoles)}

0 commit comments

Comments
 (0)