fix: Group Admin Table #1250
fix: Group Admin Table #1250
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve modifications across several components and utility functions to enhance group management functionalities. Key updates include the addition of the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
alexrisch
force-pushed
the
ar/group-admin-updates
branch
from
f2b039f to
07ed7a8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (12)
hooks/useExistingGroupInviteLink.ts (1)
5-7: Add JSDoc comments to document the optional parameterConsider adding JSDoc comments to clearly document the behavior when
topicis undefined.+/** + * Hook to retrieve an existing group invite link from the chat store + * @param topic - The conversation topic. If undefined, returns undefined + * @returns The invite link for the given topic, or undefined if topic is undefined + */ export const useExistingGroupInviteLink = ( topic: ConversationTopic | undefined ): string | undefined => {queries/useGroupPermissionPolicyQuery.ts (1)
6-9: Add TypeScript return type definitionThe hook's return type should be explicitly defined for better type safety and documentation.
+import { UseQueryResult } from "@tanstack/react-query"; +import type { PermissionPolicy } from "@xmtp/react-native-sdk"; export const useGroupPermissionPolicyQuery = ( account: string, topic: ConversationTopic | undefined -) => { +): UseQueryResult<PermissionPolicy | undefined> => {utils/groupUtils/userCanDoGroupActions.ts (2)
3-8: Consider enhancing type definitions for better type safety and reusability.The type definitions are clear, but could be improved:
- Convert
MemberRoleto an enum for better type safety- Export
GetMemberRoleParamstype for reuse in other files-type MemberRole = "admin" | "super_admin" | "member"; +enum MemberRole { + ADMIN = "admin", + SUPER_ADMIN = "super_admin", + MEMBER = "member" +} -type GetMemberRoleParams = { +export type GetMemberRoleParams = { isSuperAdmin: boolean; isAdmin: boolean; };
10-17: Add JSDoc documentation for better code maintainability.The function logic is correct, but adding documentation would improve maintainability.
+/** + * Determines the member's role based on their admin status + * @param {GetMemberRoleParams} params - Object containing admin status flags + * @param {boolean} params.isSuperAdmin - Whether the user is a super admin + * @param {boolean} params.isAdmin - Whether the user is an admin + * @returns {MemberRole} The determined role (super_admin, admin, or member) + */ const getMemberRole = ({ isSuperAdmin, isAdmin, }: GetMemberRoleParams): MemberRole => {utils/groupUtils/getGroupMemberActions.ts (2)
32-62: Consider refactoring repetitive option building logicThe current implementation has repetitive patterns for adding options and managing indices. This could be simplified for better maintainability.
Consider refactoring using a more declarative approach:
interface ActionOption { condition: boolean; translationKey: string; index?: number; isDestructive?: boolean; } const actionOptions: ActionOption[] = [ { condition: canPromoteToAdmin, translationKey: "group_screen_member_actions.promote_to_admin" }, { condition: canPromoteToSuperAdmin, translationKey: "group_screen_member_actions.promote_to_super_admin" }, // ... other options ]; const { options, indices } = actionOptions.reduce( (acc, { condition, translationKey }, idx) => { if (condition) { acc.options.push(translate(translationKey)); acc.indices[translationKey] = idx; } return acc; }, { options: ["Profile page"], indices: {} } );
Line range hint
64-75: Add explicit return type interfaceThe return object structure could benefit from an explicit TypeScript interface definition for better type safety and documentation.
Consider adding:
interface GroupMemberActions { options: string[]; cancelButtonIndex: number; promoteAdminIndex?: number; promoteSuperAdminIndex?: number; revokeAdminIndex?: number; revokeSuperAdminIndex?: number; removeIndex?: number; destructiveButtonIndex?: number; } export const getGroupMemberActions = ( // ... parameters ... ): GroupMemberActions => { // ... implementation ... };queries/QueryKeys.ts (1)
Line range hint
23-131: Consider adding JSDoc comments for the permission systemSince this is part of a permission system update, it would be helpful to add JSDoc comments explaining the purpose and usage of the permission policy feature, especially how it differs from the existing GROUP_PERMISSIONS.
Example documentation:
/** * Query key for group permission policies. * Used to fetch the permission configuration that defines allowed actions for group members. * @param account - The user's account address * @param topic - The conversation topic identifier * @returns Query key array for permission policy lookups */ export const groupPermissionPolicyQueryKey = ...components/ConversationFlashList.tsx (1)
Line range hint
48-99: Consider enhancing component reusability.The refresh handling is well implemented, but consider extracting the refresh logic into a custom hook (e.g.,
useListRefresh) to make it reusable across other list components. This would encapsulate the refresh state management and error handling in one place.Example implementation:
// hooks/useListRefresh.ts const useListRefresh = (refetchFn: () => Promise<void>) => { const [isRefetching, setIsRefetching] = useState(false); const handleRefresh = async () => { try { setIsRefetching(true); await refetchFn(); } catch (error) { // Handle error } finally { setIsRefetching(false); } }; return { isRefetching, handleRefresh }; };containers/GroupScreenMembersTable.tsx (1)
119-122: Enhance error handling with specific error messagesThe current error handling uses a generic "An error occurred" message for all error cases. Consider providing more specific error messages based on the operation and error type to help users understand what went wrong.
- Alert.alert("An error occurred"); + Alert.alert( + translate("error_title"), + translate(`error_messages.${operation}_failed`, { error: e.message }) + );Also applies to: 129-131, 138-140, 147-149, 156-158
screens/ConversationList.tsx (2)
Line range hint
116-116: Please clarify the TODO comment.The TODO comment lacks context about what needs to be done. The effect appears to be working correctly by updating
flatListItemswhensearchQuery,profiles, oritemschange. If there are known issues or planned improvements, please document them in the comment.
Line range hint
144-145: Consider improving type safety for searchBarRef.The use of
as anytype casting bypasses TypeScript's type checking. Consider creating a proper type union forsearchBarRefthat includes bothSearchBarCommandsandTextInputmethods.Example approach:
type SearchBarRef = { clearText?: () => void; blur?: () => void; } & (SearchBarCommands | TextInput);i18n/translations/en.ts (1)
406-414: Enhance accessibility with descriptive labelsConsider adding more descriptive versions of these translations for screen readers and tooltips to provide better context about the impact of these actions.
Add aria labels and tooltips:
group_screen_member_actions: { profile_page: "Profile page", + profile_page_aria: "View member's profile page", promote_to_admin: "Promote to admin", + promote_to_admin_aria: "Promote member to group admin role with additional permissions", + promote_to_admin_tooltip: "Admins can manage members and group settings", promote_to_super_admin: "Promote to super admin", + promote_to_super_admin_aria: "Promote member to group super admin role with full permissions", + promote_to_super_admin_tooltip: "Super admins have complete control over the group", revoke_admin: "Revoke admin", + revoke_admin_aria: "Remove admin privileges from this member", revoke_super_admin: "Revoke super admin", + revoke_super_admin_aria: "Remove super admin privileges from this member", remove_member: "Remove from group", + remove_member_aria: "Remove this member from the group permanently", + remove_member_tooltip: "This action cannot be undone", cancel: "Cancel", + cancel_aria: "Cancel the current action", },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
components/Conversation/GroupConversationTitle.tsx(0 hunks)components/ConversationFlashList.tsx(2 hunks)containers/GroupScreenMembersTable.tsx(1 hunks)hooks/useExistingGroupInviteLink.ts(1 hunks)i18n/translations/en.ts(1 hunks)queries/QueryKeys.ts(2 hunks)queries/useGroupPermissionPolicyQuery.ts(1 hunks)screens/ConversationList.tsx(2 hunks)utils/groupUtils/getGroupMemberActions.ts(2 hunks)utils/groupUtils/userCanDoGroupActions.ts(1 hunks)
💤 Files with no reviewable changes (1)
- components/Conversation/GroupConversationTitle.tsx
🔇 Additional comments (16)
hooks/useExistingGroupInviteLink.ts (1)
6-10: Verify all usages of this hook due to breaking parameter change
The implementation looks good, but making topic optional is a breaking change that could affect dependent components.
Let's verify all usages of this hook have been updated:
✅ Verification successful
Breaking change is properly handled in all usages
Both usages of useExistingGroupInviteLink are already passing a potentially undefined topic parameter:
- In
GroupScreenAddition.tsx,topicis used in multiple hooks in the same way - In
useGroupPendingRequests.ts, the hook's parameter is explicitly typed astopic: ConversationTopic | undefined
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of useExistingGroupInviteLink to verify they handle the optional topic parameter
ast-grep --pattern 'useExistingGroupInviteLink($$$)'
Length of output: 250
Script:
#!/bin/bash
# Check the context of useExistingGroupInviteLink usage in both files
echo "=== GroupScreenAddition.tsx usage ==="
rg -B5 -A5 "useExistingGroupInviteLink" containers/GroupScreenAddition.tsx
echo -e "\n=== useGroupPendingRequests.ts usage ==="
rg -B5 -A5 "useExistingGroupInviteLink" hooks/useGroupPendingRequests.ts
Length of output: 2197
queries/useGroupPermissionPolicyQuery.ts (2)
12-12:
Remove non-null assertion operator
The non-null assertion (!) on topic in the query key is unsafe as topic is declared as optional. This could lead to runtime errors if topic is undefined.
- queryKey: groupPermissionPolicyQueryKey(account, topic!),
+ queryKey: groupPermissionPolicyQueryKey(account, topic),Let's verify if groupPermissionPolicyQueryKey handles undefined topics:
19-19: Verify the enabled condition
The enabled condition looks good as it properly checks for both required dependencies. However, let's verify its usage in the codebase to ensure consistent behavior.
✅ Verification successful
Let me gather more information about the usage context and implementation details.
Let me check how the group permission policy is used in the action handling.
Let me check the implementation of userCanDoGroupActions to complete the verification.
Based on the gathered information, I can now provide the final response:
The enabled condition is correctly implemented and safe
The enabled: !!group && !!topic condition in useGroupPermissionPolicyQuery is properly implemented because:
- The underlying
useGroupQuerysafely handles undefined topics by returning null - The query is used in
GroupScreenMembersTablewhere its results are passed togetGroupMemberActions getGroupMemberActionsanduserCanDoGroupActionsare designed to handle undefined permission policies safely
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find usages of this hook to verify how it handles undefined topics
rg -A 5 "useGroupPermissionPolicyQuery"
Length of output: 1342
Script:
#!/bin/bash
# Check the implementation of useGroupQuery to understand how it handles undefined topics
ast-grep --pattern 'export const useGroupQuery = $_'
# Check how GroupScreenMembersTable uses the query result
rg -B 5 -A 10 "groupPermissionPolicy" containers/GroupScreenMembersTable.tsx
Length of output: 2528
Script:
#!/bin/bash
# Check the implementation of getGroupMemberActions to understand how groupPermissionPolicy is used
ast-grep --pattern 'const getGroupMemberActions = $_'
# Check for any other direct usages of groupPermissionPolicy
rg "groupPermissionPolicy\."
Length of output: 5631
Script:
#!/bin/bash
# Check the implementation of userCanDoGroupActions
ast-grep --pattern 'export const userCanDoGroupActions = $_'
# Check for any type definitions of PermissionPolicySet
rg "type PermissionPolicySet"
Length of output: 1517
utils/groupUtils/userCanDoGroupActions.ts (1)
19-37: Verify integration with dependent components.
Let's verify that the permission checks are consistently implemented across the codebase.
✅ Verification successful
Let me gather more information about the integration, particularly focusing on the actual usage patterns and the GroupScreenMembersTable component mentioned in the summary.
Based on the gathered information, I can now provide the final response:
Permission checks are properly integrated across components
The userCanDoGroupActions function is correctly integrated within the codebase:
- Used by
getGroupMemberActionswhich is properly consumed inGroupScreenMembersTable - Proper hook integration via
useGroupPermissionPolicyQueryin components - Consistent usage of
PermissionPolicySettype across the codebase - Test coverage exists for permission-related functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of userCanDoGroupActions
# Check for direct usage of the function
echo "Checking direct usage of userCanDoGroupActions:"
rg "userCanDoGroupActions" -A 3
# Check for consistent permission policy types
echo "Checking PermissionPolicySet usage:"
rg "PermissionPolicySet" -A 3
# Check for related permission hooks
echo "Checking permission hooks:"
ast-grep --pattern 'use$_PermissionPolicy'
Length of output: 5185
Script:
#!/bin/bash
# Check for GroupScreenMembersTable implementation and its permission handling
echo "Checking GroupScreenMembersTable implementation:"
rg -l "GroupScreenMembersTable" | xargs rg "userCanDoGroupActions|getGroupMemberActions" -A 3
# Check for permission-related hooks and their usage
echo "Checking permission-related hooks:"
rg "usePermission|useGroupPermission" -A 3
# Check for any other files using getGroupMemberActions
echo "Checking getGroupMemberActions usage:"
rg "getGroupMemberActions" -A 3
Length of output: 13319
utils/groupUtils/getGroupMemberActions.ts (3)
1-10: LGTM! Type-safety improvements
The change from string to PermissionPolicySet for groupPermissionLevel parameter improves type safety and makes the permission handling more structured.
33-33: Verify translation keys exist
Ensure all translation keys used in this file exist in the translations file.
Also applies to: 40-40, 47-47, 53-53, 59-59, 62-62
✅ Verification successful
All translation keys are present and correctly defined
All translation keys used in getGroupMemberActions.ts are properly defined in the English translations file at i18n/translations/en.ts under the group_screen_member_actions namespace:
- promote_to_admin
- promote_to_super_admin
- revoke_admin
- revoke_super_admin
- remove_member
- cancel
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all translation keys exist in the translations file
rg -A 1 "group_screen_member_actions: {"
# List all used translation keys in this file
echo "Keys used in getGroupMemberActions.ts:"
rg -o 'group_screen_member_actions\.[a-z_]+' utils/groupUtils/getGroupMemberActions.ts
Length of output: 603
Script:
#!/bin/bash
# Search for translation keys in JSON/TS files
rg -l "group_screen_member_actions" --type json --type ts
# Once we find the translation files, let's examine their content
rg "group_screen_member_actions" -A 10 --type json --type ts
Length of output: 3828
12-19: Consider handling undefined groupPermissionLevel
While the permission check is now more centralized using userCanDoGroupActions, consider adding a safeguard for when groupPermissionLevel is undefined to prevent potential runtime issues.
Consider adding an early return or default value:
const canRemove =
!isCurrentUser &&
+ groupPermissionLevel &&
userCanDoGroupActions(
groupPermissionLevel,
"removeMemberPolicy",
isSuperAdmin,
isAdmin
);queries/QueryKeys.ts (2)
23-23: LGTM! Well-structured enum addition
The new GROUP_PERMISSION_POLICY enum entry follows the established naming conventions and is appropriately placed within the group metadata section.
128-131: LGTM! Verify consistent usage across the codebase
The new query key function follows the established patterns perfectly. Let's verify its integration with the permission policy feature.
✅ Verification successful
✅ Query key implementation is properly integrated
The verification shows that:
- The query key is correctly used in
useGroupPermissionPolicyQueryhook - The hook is properly integrated in
GroupScreenMembersTable - The implementation follows the established patterns and is used consistently with other permission-related utilities
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the new permission policy query key and ensure it's properly integrated
# Check for the corresponding query hook implementation
echo "Checking for query hook implementation..."
rg -l "useGroupPermissionPolicy|groupPermissionPolicyQueryKey"
# Look for direct usage of the GROUP_PERMISSION_POLICY enum
echo "Checking for enum usage..."
rg "GROUP_PERMISSION_POLICY"
# Check for any TODO or FIXME comments related to permissions
echo "Checking for related TODO/FIXME..."
rg -i "todo.*permission|fixme.*permission"
Length of output: 641
Script:
#!/bin/bash
# Let's examine the implementation details to verify proper integration
# Check the hook implementation
echo "=== Checking hook implementation ==="
rg -A 10 "useGroupPermissionPolicy" queries/useGroupPermissionPolicyQuery.ts
# Check how it's used in the GroupScreenMembersTable
echo -e "\n=== Checking usage in GroupScreenMembersTable ==="
rg -A 5 "useGroupPermissionPolicy" containers/GroupScreenMembersTable.tsx
# Check for any other potential usages of permission policy
echo -e "\n=== Checking other permission policy related code ==="
rg -l "permissionPolicy|PermissionPolicy"
Length of output: 1564
components/ConversationFlashList.tsx (2)
48-48: LGTM! Props interface update is well-typed.
The optional isRefetching boolean prop is correctly added to handle loading states.
99-99: Verify error handling in the parent component.
The refreshing state is correctly implemented, but ensure the parent component properly handles errors during the refresh operation to prevent the list from being stuck in a loading state.
containers/GroupScreenMembersTable.tsx (3)
53-61: LGTM! Well-implemented memoization
The admin status computations are correctly memoized with appropriate dependencies.
199-201: LGTM! Clean and internationalized rendering
The component's render logic is clean and properly uses translations for the title.
47-50: Verify integration with permission policy system
The component now uses the new permission policy system. Let's verify the integration with related files.
✅ Verification successful
Based on the results, I can now generate the final response as the integration appears to be properly implemented:
Permission policy integration is correctly implemented
The changes are well-integrated with the permission policy system:
- The
useGroupPermissionPolicyQueryhook is properly imported and used - The permission policy types are consistently defined across the codebase
- The component correctly integrates with
getGroupMemberActionsutility - All necessary dependencies and type definitions are in place from the SDK
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent usage of permission policy
echo "Checking permission policy usage..."
rg -A 2 "useGroupPermissionPolicyQuery"
echo "Checking permission policy type definitions..."
rg "PermissionPolicySet"
echo "Checking related permission utilities..."
rg "getGroupMemberActions"
Length of output: 3950
screens/ConversationList.tsx (2)
88-88: LGTM: Good addition of loading state feedback!
The addition of isRefetching state and its propagation to ConversationFlashList improves the user experience by providing visual feedback during data updates. This follows React Native best practices for handling loading states in list components.
Also applies to: 228-228
Line range hint 18-18: Verify the impact of removing profile refresh functionality.
The profile refresh functionality has been commented out both at import and usage. Please confirm if this change is intentional and whether profile synchronization on first login is handled elsewhere.
Also applies to: 107-107
| queryFn: () => { | ||
| if (!group) { | ||
| return; | ||
| } | ||
| return group.permissionPolicySet(); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for permissionPolicySet call
The permissionPolicySet() call could potentially throw errors, but there's no error handling in place. Consider wrapping it in a try-catch block.
queryFn: () => {
if (!group) {
return;
}
- return group.permissionPolicySet();
+ try {
+ return group.permissionPolicySet();
+ } catch (error) {
+ throw new Error(`Failed to fetch permission policy: ${error}`);
+ }
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| queryFn: () => { | |
| if (!group) { | |
| return; | |
| } | |
| return group.permissionPolicySet(); | |
| }, | |
| queryFn: () => { | |
| if (!group) { | |
| return; | |
| } | |
| try { | |
| return group.permissionPolicySet(); | |
| } catch (error) { | |
| throw new Error(`Failed to fetch permission policy: ${error}`); | |
| } | |
| }, |
| export const userCanDoGroupActions = ( | ||
| groupPermissionPolicy: PermissionPolicySet | undefined, | ||
| action: keyof PermissionPolicySet, | ||
| isSuperAdmin: boolean, | ||
| isAdmin: boolean | ||
| ) => { | ||
| const memberRole = getMemberRole({ isSuperAdmin, isAdmin }); | ||
| const policy = groupPermissionPolicy?.[action]; | ||
| // Edge cases | ||
| if (policy === "allow") return true; | ||
| if (policy === "deny") return false; | ||
| if ( | ||
| policy === "admin" && | ||
| (memberRole === "admin" || memberRole === "super_admin") | ||
| ) | ||
| return true; | ||
| if (policy === memberRole) return true; | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and improve documentation.
The permission logic is solid, but could benefit from better error handling and documentation.
+/**
+ * Checks if a user can perform specific group actions based on their role and the group's permission policy.
+ * Permission hierarchy:
+ * 1. Explicit "allow"/"deny" policies
+ * 2. "admin" policy (applies to both admin and super_admin)
+ * 3. Exact role match
+ * 4. Default deny
+ *
+ * @throws {Error} If action is not defined in PermissionPolicySet
+ */
export const userCanDoGroupActions = (
groupPermissionPolicy: PermissionPolicySet | undefined,
action: keyof PermissionPolicySet,
isSuperAdmin: boolean,
isAdmin: boolean
) => {
+ if (!groupPermissionPolicy) {
+ return false;
+ }
+
+ if (!(action in groupPermissionPolicy)) {
+ throw new Error(`Invalid action: ${action}`);
+ }
+
const memberRole = getMemberRole({ isSuperAdmin, isAdmin });
const policy = groupPermissionPolicy?.[action];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const userCanDoGroupActions = ( | |
| groupPermissionPolicy: PermissionPolicySet | undefined, | |
| action: keyof PermissionPolicySet, | |
| isSuperAdmin: boolean, | |
| isAdmin: boolean | |
| ) => { | |
| const memberRole = getMemberRole({ isSuperAdmin, isAdmin }); | |
| const policy = groupPermissionPolicy?.[action]; | |
| // Edge cases | |
| if (policy === "allow") return true; | |
| if (policy === "deny") return false; | |
| if ( | |
| policy === "admin" && | |
| (memberRole === "admin" || memberRole === "super_admin") | |
| ) | |
| return true; | |
| if (policy === memberRole) return true; | |
| return false; | |
| }; | |
| /** | |
| * Checks if a user can perform specific group actions based on their role and the group's permission policy. | |
| * Permission hierarchy: | |
| * 1. Explicit "allow"/"deny" policies | |
| * 2. "admin" policy (applies to both admin and super_admin) | |
| * 3. Exact role match | |
| * 4. Default deny | |
| * | |
| * @throws {Error} If action is not defined in PermissionPolicySet | |
| */ | |
| export const userCanDoGroupActions = ( | |
| groupPermissionPolicy: PermissionPolicySet | undefined, | |
| action: keyof PermissionPolicySet, | |
| isSuperAdmin: boolean, | |
| isAdmin: boolean | |
| ) => { | |
| if (!groupPermissionPolicy) { | |
| return false; | |
| } | |
| if (!(action in groupPermissionPolicy)) { | |
| throw new Error(`Invalid action: ${action}`); | |
| } | |
| const memberRole = getMemberRole({ isSuperAdmin, isAdmin }); | |
| const policy = groupPermissionPolicy?.[action]; | |
| // Edge cases | |
| if (policy === "allow") return true; | |
| if (policy === "deny") return false; | |
| if ( | |
| policy === "admin" && | |
| (memberRole === "admin" || memberRole === "super_admin") | |
| ) | |
| return true; | |
| if (policy === memberRole) return true; | |
| return false; | |
| }; |
| } | ||
| break; | ||
| case promoteAdminIndex: | ||
| logger.debug("Promoting member..."); | ||
| try { | ||
| await promoteToAdmin(a.inboxId); | ||
| } catch (e) { | ||
| logger.error(e); | ||
| Alert.alert("An error occurred"); | ||
| } | ||
| break; | ||
| case revokeAdminIndex: | ||
| logger.debug("Revoking admin..."); | ||
| try { | ||
| await revokeAdmin(a.inboxId); | ||
| } catch (e) { | ||
| logger.error(e); | ||
| Alert.alert("An error occurred"); | ||
| } | ||
| break; | ||
| case removeIndex: | ||
| logger.debug("Removing member..."); | ||
| try { | ||
| await removeMember([a.inboxId]); | ||
| } catch (e) { | ||
| logger.error(e); | ||
| Alert.alert("An error occurred"); | ||
| } | ||
| break; | ||
| default: | ||
| } | ||
| } | ||
| } | ||
| ); | ||
| }, | ||
| rightView: ( | ||
| <View style={styles.tableViewRight}> | ||
| {isSuperAdmin && <Text style={styles.adminText}>Super Admin</Text>} | ||
| {isAdmin && !isSuperAdmin && ( | ||
| <Text style={styles.adminText}>Admin</Text> | ||
| )} | ||
| <TableViewPicto | ||
| symbol="chevron.right" | ||
| color={textSecondaryColor(colorScheme)} | ||
| /> | ||
| </View> | ||
| ), | ||
| ); | ||
| }, | ||
| rightView: ( | ||
| <View style={styles.tableViewRight}> | ||
| {isSuperAdmin && ( | ||
| <Text style={styles.adminText}>Super Admin</Text> | ||
| )} | ||
| {isAdmin && !isSuperAdmin && ( | ||
| <Text style={styles.adminText}>Admin</Text> | ||
| )} | ||
| <TableViewPicto | ||
| symbol="chevron.right" | ||
| color={textSecondaryColor(colorScheme)} | ||
| /> | ||
| </View> | ||
| ), | ||
| }); | ||
| }); | ||
| }); | ||
| return items; | ||
| }, [ | ||
| colorScheme, | ||
| currentAccount, | ||
| currentAccountIsAdmin, | ||
| currentAccountIsSuperAdmin, | ||
| members, | ||
| profiles, | ||
| promoteToAdmin, | ||
| promoteToSuperAdmin, | ||
| removeMember, | ||
| revokeAdmin, | ||
| revokeSuperAdmin, | ||
| styles.adminText, | ||
| styles.tableViewRight, | ||
| topic, | ||
| ]); | ||
| return items; | ||
| }, [ | ||
| colorScheme, | ||
| currentAccount, | ||
| currentAccountIsSuperAdmin, | ||
| groupPermissionPolicy, | ||
| members, | ||
| profiles, | ||
| promoteToAdmin, | ||
| promoteToSuperAdmin, | ||
| removeMember, | ||
| revokeAdmin, | ||
| revokeSuperAdmin, | ||
| styles.adminText, | ||
| styles.tableViewRight, | ||
| topic, | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider breaking down the complex tableViewItems computation
The current implementation combines multiple responsibilities into a single useMemo hook with a large dependency array. This could impact the effectiveness of memoization and make the code harder to maintain.
Consider splitting it into smaller, focused functions:
const useGroupMemberItems = (members: Member[], currentAccount: string) => {
return useMemo(
() => sortGroupMembersByAdminStatus(members, currentAccount),
[members, currentAccount]
);
};
const useMemberActions = (
member: Member,
groupPermissionPolicy: PermissionPolicySet,
currentAccountIsSuperAdmin: boolean
) => {
return useMemo(
() => ({
options: getGroupMemberActions(
groupPermissionPolicy,
member.isCurrentUser,
member.isSuperAdmin,
member.isAdmin,
currentAccountIsSuperAdmin
),
handlers: {
handlePromote: () => { /* ... */ },
handleRevoke: () => { /* ... */ },
// ... other handlers
}
}),
[member, groupPermissionPolicy, currentAccountIsSuperAdmin]
);
};| type GroupScreenMembersTableProps = { | ||
| topic: ConversationTopic | undefined; | ||
| group: GroupWithCodecsType | undefined | null; | ||
| }; | ||
|
|
||
| export const GroupScreenMembersTable: FC<GroupScreenMembersTableProps> = ({ | ||
| topic, | ||
| group, | ||
| }) => { | ||
| const colorScheme = useColorScheme(); | ||
| const currentAccount = useCurrentAccount() as string; | ||
| const styles = useStyles(); | ||
| const { | ||
| members, | ||
| promoteToSuperAdmin, | ||
| promoteToAdmin, | ||
| revokeAdmin, | ||
| revokeSuperAdmin, | ||
| removeMember, | ||
| } = useGroupMembers(topic); | ||
| const profiles = useProfilesStore((s) => s.profiles); | ||
| const currentAccountIsAdmin = useMemo( | ||
| () => getAddressIsAdmin(members, currentAccount), | ||
| [currentAccount, members] | ||
| ); | ||
| const currentAccountIsSuperAdmin = useMemo( | ||
| () => getAddressIsSuperAdmin(members, currentAccount), | ||
| [currentAccount, members] | ||
| ); | ||
| export const GroupScreenMembersTable: FC<GroupScreenMembersTableProps> = memo( | ||
| ({ topic }) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused group prop from type definition
The group prop is defined in GroupScreenMembersTableProps but is not used in the component. This could lead to confusion about the component's requirements.
type GroupScreenMembersTableProps = {
topic: ConversationTopic | undefined;
- group: GroupWithCodecsType | undefined | null;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type GroupScreenMembersTableProps = { | |
| topic: ConversationTopic | undefined; | |
| group: GroupWithCodecsType | undefined | null; | |
| }; | |
| export const GroupScreenMembersTable: FC<GroupScreenMembersTableProps> = ({ | |
| topic, | |
| group, | |
| }) => { | |
| const colorScheme = useColorScheme(); | |
| const currentAccount = useCurrentAccount() as string; | |
| const styles = useStyles(); | |
| const { | |
| members, | |
| promoteToSuperAdmin, | |
| promoteToAdmin, | |
| revokeAdmin, | |
| revokeSuperAdmin, | |
| removeMember, | |
| } = useGroupMembers(topic); | |
| const profiles = useProfilesStore((s) => s.profiles); | |
| const currentAccountIsAdmin = useMemo( | |
| () => getAddressIsAdmin(members, currentAccount), | |
| [currentAccount, members] | |
| ); | |
| const currentAccountIsSuperAdmin = useMemo( | |
| () => getAddressIsSuperAdmin(members, currentAccount), | |
| [currentAccount, members] | |
| ); | |
| export const GroupScreenMembersTable: FC<GroupScreenMembersTableProps> = memo( | |
| ({ topic }) => { | |
| type GroupScreenMembersTableProps = { | |
| topic: ConversationTopic | undefined; | |
| }; | |
| export const GroupScreenMembersTable: FC<GroupScreenMembersTableProps> = memo( | |
| ({ topic }) => { |
| group_screen_member_actions: { | ||
| profile_page: "Profile page", | ||
| promote_to_admin: "Promote to admin", | ||
| promote_to_super_admin: "Promote to super admin", | ||
| revoke_admin: "Revoke admin", | ||
| revoke_super_admin: "Revoke super admin", | ||
| remove_member: "Remove from group", | ||
| cancel: "Cancel", | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate translations and improve consistency
There are a few improvements needed in the new translation object:
-
The translation "Promote to admin" is duplicated:
- Line 363:
promote_to_admin: "Promote to admin" - Line 408:
promote_to_admin: "Promote to admin"
- Line 363:
-
For consistency with other translation strings in the file, consider:
- Adding periods at the end of action strings
- Moving these translations under the existing group-related section
Consider this refactoring:
- promote_to_admin: "Promote to admin", // Remove this duplicate at line 363
// ... other translations ...
group_screen_member_actions: {
- profile_page: "Profile page",
- promote_to_admin: "Promote to admin",
- promote_to_super_admin: "Promote to super admin",
- revoke_admin: "Revoke admin",
- revoke_super_admin: "Revoke super admin",
- remove_member: "Remove from group",
- cancel: "Cancel",
+ profile_page: "Profile page.",
+ promote_to_admin: "Promote to admin.",
+ promote_to_super_admin: "Promote to super admin.",
+ revoke_admin: "Revoke admin.",
+ revoke_super_admin: "Revoke super admin.",
+ remove_member: "Remove from group.",
+ cancel: "Cancel."
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| group_screen_member_actions: { | |
| profile_page: "Profile page", | |
| promote_to_admin: "Promote to admin", | |
| promote_to_super_admin: "Promote to super admin", | |
| revoke_admin: "Revoke admin", | |
| revoke_super_admin: "Revoke super admin", | |
| remove_member: "Remove from group", | |
| cancel: "Cancel", | |
| }, | |
| group_screen_member_actions: { | |
| profile_page: "Profile page.", | |
| promote_to_admin: "Promote to admin.", | |
| promote_to_super_admin: "Promote to super admin.", | |
| revoke_admin: "Revoke admin.", | |
| revoke_super_admin: "Revoke super admin.", | |
| remove_member: "Remove from group.", | |
| cancel: "Cancel." | |
| }, |
alexrisch
force-pushed
the
feature/v3-split
branch
from
0183b42 to
b4ed98d
Compare
alexrisch
force-pushed
the
ar/group-admin-updates
branch
from
07ed7a8 to
ea77f02
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
containers/GroupScreenMembersTable.tsx (1)
117-122: Consider centralizing error handling logicThe error handling pattern is duplicated across different actions. Consider extracting it into a reusable wrapper function.
const handleMemberAction = async (action: () => Promise<void>, actionName: string) => { logger.debug(`${actionName}...`); try { await action(); } catch (e) { logger.error(e); Alert.alert("An error occurred"); } }; // Usage example: case promoteAdminIndex: await handleMemberAction( () => promoteToAdmin(a.inboxId), "Promoting member" ); break;Also applies to: 126-131, 135-140, 144-149, 153-158
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
components/ConversationFlashList.tsx(2 hunks)containers/GroupScreenMembersTable.tsx(1 hunks)hooks/useExistingGroupInviteLink.ts(1 hunks)i18n/translations/en.ts(1 hunks)queries/QueryKeys.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- components/ConversationFlashList.tsx
- hooks/useExistingGroupInviteLink.ts
- i18n/translations/en.ts
- queries/QueryKeys.ts
🔇 Additional comments (4)
containers/GroupScreenMembersTable.tsx (4)
33-34: LGTM: Good performance optimization with memo!
The component is correctly wrapped in memo to prevent unnecessary re-renders when props haven't changed.
47-50: LGTM: Clean integration of permission policy query!
The permission policy query is well-integrated with appropriate dependencies.
199-201: LGTM: Clean and simple render logic!
The render method is concise and properly uses translations.
26-26: Verify consistent usage of permission policy query
Let's verify that other components using group permissions have been updated to use the new query.
Also applies to: 47-50
✅ Verification successful
Permission policy query is correctly isolated to member table functionality
The new useGroupPermissionPolicyQuery is specifically used for member management in GroupScreenMembersTable.tsx. Other group-related components (GroupScreenName, GroupScreenImage, GroupScreenDescription, GroupScreenAddition) use appropriate admin utility functions for their permission checks and don't require the permission policy query as they handle different aspects of group management.
The changes are consistent with the component's responsibilities:
- Member table: Uses permission policy for member management
- Other components: Use admin utilities for basic group editing permissions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components that might need the permission policy query
echo "Searching for files with group permission checks..."
rg -l "getGroupMemberActions|getAddressIsAdmin|getAddressIsSuperAdmin" --type typescript
# Search for old permission level usage
echo "Checking for old permission level usage..."
rg "groupPermissionLevel" --type typescript
Length of output: 412
Script:
#!/bin/bash
# Search for other components that might need the permission policy query
echo "Searching for files with group permission checks..."
rg -l "getGroupMemberActions|getAddressIsAdmin|getAddressIsSuperAdmin"
# Search for old permission level usage
echo "Checking for old permission level usage..."
rg "groupPermissionLevel"
# Search for permission policy query usage
echo "Checking for permission policy query usage..."
rg "useGroupPermissionPolicyQuery"
Length of output: 1701
Script:
#!/bin/bash
# Let's check the implementation of files that might need the permission policy
echo "Checking GroupScreenName.tsx implementation..."
rg -A 5 "getGroupMemberActions|getAddressIsAdmin|getAddressIsSuperAdmin" containers/GroupScreenName.tsx
echo "Checking GroupScreenImage.tsx implementation..."
rg -A 5 "getGroupMemberActions|getAddressIsAdmin|getAddressIsSuperAdmin" containers/GroupScreenImage.tsx
echo "Checking GroupScreenDescription.tsx implementation..."
rg -A 5 "getGroupMemberActions|getAddressIsAdmin|getAddressIsSuperAdmin" containers/GroupScreenDescription.tsx
echo "Checking GroupScreenAddition.tsx implementation..."
rg -A 5 "getGroupMemberActions|getAddressIsAdmin|getAddressIsSuperAdmin" containers/GroupScreenAddition.tsx
Length of output: 3060
Added new query Updated use of permissions Added new translations Updated hooks Added new utils
alexrisch
force-pushed
the
ar/group-admin-updates
branch
from
ea77f02 to
217f7eb
Compare
Added new query Updated use of permissions Added new translations Updated hooks Added new utils
Added new query
Updated use of permissions
Added new translations
Updated hooks
Added new utils
Summary by CodeRabbit
Release Notes
New Features
ConversationFlashListfor data refetching.Improvements
GroupScreenMembersTablefor optimized rendering and improved permission handling.Bug Fixes