Should we put some sort of TODO comment or something here?

Thanks for using CSS variables for all font sizes/colors

Does the order of elements in this array matter at all?

If not, should we just use a $ReadOnlySet anytime we have a collection of UserSurfacedPermissions?

Caught

Also, the reason for making onCloseModal is because later in the stack, I will be pushing an UnsavedChangesModal if there are any changes made if a user tries to exit, so I just set it up now.

in the diff Summary, but in future might be helpful to annotate this directly to put it "right in reviewers face."

Could probably consolidate these types into 2 instead of 3 by making css.clearPermissions the "enabled" style and optionally toggling the "disabled" style based on pendingRolePermissions.length.

const clearPermissionsClassNames = classNames({
   [css.clearPermissions]: true,
   [css.clearPermissionsDisabled]: pendingRolePermissions.length === 0,
 });

Doesn't really matter though, totally fine as is

We could make this O(1) instead of O(n) if pendingRolePermissions was a $ReadOnlySet instead of $ReadOnlyArray.

Again, probably won't make a huge difference in practice... but it's easy enough to change.

If we were to use a $ReadOnlySet, we could do something like:

setPendingRolePermissions(currentPermissions => {
   if (currentPermissions.has(option.userSurfacedPermission)) {
     const newPermissions = new Set(currentPermissions);
     newPermissions.delete(option.userSurfacedPermission);
     return newPermissions;
   } else {
     return new Set([...currentPermissions, option.userSurfacedPermission]);
   }
})

which imo is a bit more readable

It's not clear what's happening here. What exactly is modifyUserSurfacedPermissionOptions modifying? Why is it making those modifications?

The function's purpose isn't clear from its name.

It's obviously modifying ModifiedUserSurfacedPermissionOptions, but it's not clear WHAT modifications are being made or WHY the modifications are being made.

The API for the function is also unexpected. I would have expected something like:

($ReadOnlyArray<UserSurfacedPermissionOption>, ThreadType) => $ReadOnlyArray<ModifiedUserSurfacedPermissionOption>

That would keep things as general as possible, accepting only the necessary inputs.

Is this function specific to the CreateRolesModal component? If it's going to live in a utils file, it should have a clear name and API.

Here are my suggestions:

1. Simplify the API. The function should accept UserSurfacedPermissionOptions and a ThreadType, and return ModifiedUserSurfacedPermissionOptions.

2. Rename the function to more accurately reflect the modifications it makes. If it's hard to encapsulate the two modifications (filtering and adding statements) in one name, it might make sense to split them into separate functions. This would make the functionality more clear at the callsite (eg CreateRolesModal).

3. Annotate this utility function with a comment explaining its purpose.

Definitely let me know if I'm missing something here

Let's introduce a utility function called something like applicablePermissionsForThreadType or filterPermissionsByThreadType to contain this logic. That way it can be expanded/reused in the future for other threadTypes/permissions/etc.

We could maybe go with a similar approach to what we use in rawThreadInfoFromServerThreadInfo:

where we map thread types to a list of permissions to be excluded

Are these statements actually going to be used for anything? Or are they just to appease the EnumSettingsOption API on web?

If it's just to fit the API, we should see if we can change it. That'll also bring more cross-platform consistency to EnumSettingsOption.

Regardless, this functionality should be encapsulated in a separate function with a clear, descriptive name, rather than being buried within this modifyUserSurfacedPermissionOptions utility.