We include the specialRole field in RoleInfo based on shouldIncludeSpecialRoleFieldInRoles which is determined by a code version check.
Details
Details
Hard coded shouldIncludeSpecialRoleFieldInRoles to true/false and observed that the specialRole field was included/excluded:
shouldIncludeSpecialRoleFieldInRoles = true:
shouldIncludeSpecialRoleFieldInRoles = false:
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| lib/shared/thread-utils.js | ||
|---|---|---|
| 889–898 ↗ | (On Diff #36865) | Actually, this approach is too hacky. Encoding the role permissions twice, while not expensive, is still wasteful. We should minimally encode ONCE and just include specialRole field from rolesWithFilteredThreadPermissions |
Comment Actions
few nit stuff inline, feel free to adopt or ignore
| lib/shared/thread-utils.js | ||
|---|---|---|
| 884–891 ↗ | (On Diff #36882) | This should be simpler |
| 893–903 ↗ | (On Diff #36882) | Nit: I personally would prefer if we defined Object.entries(rawThreadInfoWithoutSpecialRoles.roles).map(
([key, role]) => [
key,
{
...role,
specialRole: rolesWithFilteredThreadPermissions[key]?.specialRole,
},
],
),as a separate variable then passed that into Object.fromEntries() but up to you |
Comment Actions
consistency
| lib/shared/thread-utils.js | ||
|---|---|---|
| 884–891 ↗ | (On Diff #36882) | I'd prefer to group the early returns even though we have multiple calls to minimallyEncodeRawThreadInfo. I think the repetition is okay in this case. |
| 893–903 ↗ | (On Diff #36882) | I think I'd prefer to go from one object representation of roles to another object representation of roles without an intermediate array representation. Using Object.entries with entries within the call seems like a common pattern in codebase. |