Role page various fixes 2 #12416

Weiko · 2025-06-02T14:04:01Z

Fix: AvatarURL signedPath for workspace members were not consistent when queried multiple times and it was causing the frontend to wrongly interpret this as a change in the deepEqual condition
Use SaveAndCancel button to be consistent with data model page
When applying all object permission changes, a "smarter" logic applies and removes all permissions if read is unchecked for example
Hide settings permissions when Settings All Access is toggled

greptile-apps

PR Summary

This PR addresses role management and avatar URL inconsistencies, focusing on workspace member data handling and permissions logic. Here are the key changes:

Fixed avatar URL signedPath inconsistency by removing server-side signing and using enriched workspace member data from Recoil state
Implemented smarter permission cascade logic where disabling read access automatically disables other permissions (update, soft delete, destroy)
Added collapsible settings permissions section that hides when 'Settings All Access' is enabled
Replaced single Save button with SaveAndCancelButtons component for consistency with data model page
Improved role assignment workflow by using CurrentWorkspaceMember type instead of SearchRecord for better type safety

_{13 file(s) reviewed, 3 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-06-02T14:04:42Z

packages/twenty-front/src/modules/settings/roles/components/SettingsRolesQueryEffect.tsx

+          if (!isDeeplyEqual(currentDraftRole, role)) {
+            set(settingsDraftRoleFamilyState(role.id), role);
+          }


logic: This check prevents unnecessary updates but could mask legitimate changes if currentDraftRole contains local modifications that should be overwritten by the server state.

greptile-apps · 2025-06-02T14:05:18Z

...es/role-assignment/components/SettingsRoleAssignmentWorkspaceMemberPickerDropdownContent.tsx

+            placeholder: workspaceMember?.name.firstName ?? '',
+            placeholderColorSeed: workspaceMember?.id,
+            avatarUrl: workspaceMember?.avatarUrl,
          }}
-          text={workspaceMember.label}
+          text={workspaceMember?.name.firstName ?? ''}


style: Optional chaining is redundant here since isDefined filter was already applied. Remove the optional chaining operators.

Suggested change

placeholder: workspaceMember?.name.firstName ?? '',

placeholderColorSeed: workspaceMember?.id,

avatarUrl: workspaceMember?.avatarUrl,

}}

text={workspaceMember.label}

text={workspaceMember?.name.firstName ?? ''}

placeholder: workspaceMember.name.firstName ?? '',

placeholderColorSeed: workspaceMember.id,

avatarUrl: workspaceMember.avatarUrl,

}}

text={workspaceMember.name.firstName ?? ''}

greptile-apps · 2025-06-02T14:05:58Z

...es/role-permissions/objects-permissions/components/SettingsRolePermissionsObjectsSection.tsx

+          ...(value === true
+            ? {
+                canReadAllObjectRecords: value,
+              }
+            : {}),


style: This logic is repeated for multiple permissions. Consider extracting to a shared utility function to maintain DRY principles and ensure consistent behavior

github-actions · 2025-06-02T14:18:13Z

🚀 Preview Environment Ready!

Your preview environment is available at: http://bore.pub:46042

This environment will automatically shut down when the PR is closed or after 5 hours.

Weiko · 2025-06-02T14:25:45Z

packages/twenty-server/src/engine/metadata-modules/role/role.resolver.ts

@@ -188,17 +188,6 @@ export class RoleResolver {
        workspace.id,
      );

-    await Promise.all(


This was added in the last PR to include avatarUrls to the response but this is what broke the dirty state in the FE due to the sign being different after multiple requests (which could happen but the FE was able to not update the state via a isDeeplyEqual but this function was always returning false due to the avatarUrl containing the different hash). I'm reverting this and now we use the existing currentWorkspaceMembersState state to retrieve the correct avatarURL. I think this issue is not only scoped to permissions though and could happen in other surfaces 👀

"but this is what broke the dirty state in the FE due to the sign being different"
What is the sign?

When you query the API to retrieve an image, we need to sign it so the user can view its content, it looks like this:
http://localhost:3000/files/profile-picture/original/eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJmaWxlbmFtZSI6ImE5MDRhOTk4LTY0YjItNDcx....
which is then verified on the backend when opened to see if the image has been correctly signed (and it has an expiration date).
This encoding sign was added every time we were doing getRoles and is not idempotent (each query was returning a different sign) so the payload was always different even though the role was not changed

Weiko · 2025-06-02T14:27:02Z

packages/twenty-front/src/modules/settings/roles/components/SettingsRolesQueryEffect.tsx

          set(settingsPersistedRoleFamilyState(role.id), role);
+
+          if (!isDeeplyEqual(currentDraftRole, role)) {


This is probably redundant actually, the root cause was the avatarUrl thing, I can remove this

ijreilly

maybe introduced by the form update ? (on a role i m creating)

Enregistrement.de.l.ecran.2025-06-02.a.16.49.16.mov

ijreilly · 2025-06-02T14:39:07Z

packages/twenty-front/src/modules/settings/roles/components/SettingsRolesTableRow.tsx

@@ -62,6 +65,16 @@ export const SettingsRolesTableRow = ({ role }: SettingsRolesTableRowProps) => {
  const { getIcon } = useIcons();
  const Icon = getIcon(role.icon ?? 'IconUser');

+  const currentWorkspaceMembers = useRecoilValue(currentWorkspaceMembersState);
+
+  const enrichedWorkspaceMembers = role.workspaceMembers


what does enrich mean here ?

It means the workspaceMember returned by role only contains the id then we "enrich" it through the currentWorkspaceMembersState to get the rest such as the avatarUrl (with a sign in the URL that's not supposed to change since getCurrentUser is only called when we refresh the page)

I can change the name if it's not clear!

ijreilly · 2025-06-02T14:40:21Z

...wenty-front/src/modules/settings/roles/role-assignment/components/SettingsRoleAssignment.tsx

-      id: workspaceMemberSearchRecord.recordId,
-      name: `${workspaceMemberSearchRecord.label}`,
+      id: workspaceMember.id,
+      name: `${workspaceMember.name.firstName} ${workspaceMember.name.lastName}`,


are these always present (they are nullable in the db)? if not we may be displaying "undefined"

nevermind, they are nullable in the db but not in the schema

They could be empty strings in theory but not undefined yes, I believe label from the search query is already the concatenation of firstName + lastName so the result should be the same

ijreilly · 2025-06-02T14:44:22Z

.../role-permissions/settings-permissions/components/SettingsRolePermissionsSettingsSection.tsx

+      <AnimatedExpandableContainer
+        isExpanded={!settingsDraftRole.canUpdateAllSettings}
+        dimension="height"
+        animationDurations={{


should these be in a constant ? I suppose we would like all animations to be similar through the app

There is one constant ADVANCED_SETTINGS_ANIMATION_DURATION I believe, I can rename it to make it more generic if we indeed want to it be consistant through the app or create a new one

ijreilly · 2025-06-02T14:45:58Z

packages/twenty-server/src/engine/metadata-modules/role/role.resolver.ts

@@ -188,17 +188,6 @@ export class RoleResolver {
        workspace.id,
      );

-    await Promise.all(


"but this is what broke the dirty state in the FE due to the sign being different"
What is the sign?

Weiko · 2025-06-02T15:01:54Z

maybe introduced by the form update ? (on a role i m creating)

Enregistrement.de.l.ecran.2025-06-02.a.16.49.16.mov

I'm not sure this is new, I'll handle this in another ticket but when creating a role the input (where you see "Role Name" at the top) should be opened and force you to type something otherwise the role name will be empty (even though we display a placeholder "Role Name", actually the placeholder should also have a different color, that's also for the other ticket).
A more pragmatic approach: We could also set and send "Role name" during creation and the next time someone tries to create a role it will say the role name is already taken

ijreilly

great ! role creation / update form is much clearer now i feel !

- Fix: AvatarURL signedPath for workspace members were not consistent when queried multiple times and it was causing the frontend to wrongly interpret this as a change in the deepEqual condition - Use SaveAndCancel button to be consistent with data model page - When applying all object permission changes, a "smarter" logic applies and removes all permissions if read is unchecked for example - Hide settings permissions when Settings All Access is toggled

Weiko added 2 commits May 28, 2025 18:44

Role page various fixes 2

f53d498

Hide settings permissions when Settings All Access is toggled

1250fa0

greptile-apps bot reviewed Jun 2, 2025

View reviewed changes

Weiko commented Jun 2, 2025

View reviewed changes

charlesBochet added the -PR: awaiting review label Jun 2, 2025

charlesBochet assigned Weiko Jun 2, 2025

ijreilly reviewed Jun 2, 2025

View reviewed changes

ijreilly approved these changes Jun 2, 2025

View reviewed changes

Weiko merged commit 8e71000 into main Jun 2, 2025
57 checks passed

Weiko deleted the c--fe-role-various-fixes-2 branch June 2, 2025 18:24

		set(settingsPersistedRoleFamilyState(role.id), role);

		if (!isDeeplyEqual(currentDraftRole, role)) {

Role page various fixes 2 #12416

Role page various fixes 2 #12416

Uh oh!

Conversation

Weiko commented Jun 2, 2025

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

Uh oh!

greptile-apps bot Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijreilly left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Weiko commented Jun 2, 2025

Uh oh!

ijreilly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ijreilly left a comment •

edited

Loading