feat: Members Query on Conversation List #1343
Conversation
Updates Group Members on conversation list to use Queries Updates Group Avatar to remove old props Updates Group members queries to use config pattern
WalkthroughThe pull request introduces significant changes across multiple components and hooks related to group avatars and member management. Key modifications include the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
hooks/useGroupMembers.ts (1)
18-18: Consider adding error retry configurationSince this is a critical group data fetch, consider adding retry configuration to handle temporary network issues.
- } = useGroupMembersQuery(account, topic); + } = useGroupMembersQuery(account, topic, { + retry: 3, + retryDelay: (attemptIndex) => Math.min(1000 * 2 ** attemptIndex, 30000), + });features/conversation-list/useGroupConversationListAvatarInfo.ts (1)
Line range hint
27-43: Consider extracting member data transformationThe complex member data transformation logic could be moved to a separate utility function for better maintainability and reusability.
+const transformMemberData = ( + data: ReturnType<typeof useInboxProfileSocialsQueries>, + memberInboxIds: InboxId[] +) => + data + .map( + ({ data: socials }, index) => + socials && { + inboxId: memberInboxIds[index], + address: getPreferredInboxAddress(socials) ?? "", + uri: getPreferredInboxAvatar(socials), + name: getPreferredInboxName(socials), + } + ) + .filter(Boolean) as { + inboxId: InboxId; + address: string; + uri: string | undefined; + name: string; + }[]; const memberData = useMemo( - () => - data - .map(...) - .filter(Boolean) as {...}[], + () => transformMemberData(data, memberInboxIds), [data, memberInboxIds] );features/conversations/components/GroupConversationTitle.tsx (1)
Line range hint
101-113: LGTM! Consider extracting subtitle rendering logicThe change from
&&to ternary operator improves the null handling for the subtitle. However, the subtitle rendering logic could be extracted into a separate memoized component or function to improve readability and reusability.Consider extracting the subtitle logic:
+ const SubtitleContent = memo(({ memberText, requestsCount }: { + memberText: string, + requestsCount: number + }) => ( + <Text preset="formLabel"> + {memberText} + {requestsCount > 0 && ( + <> + {" • "} + <Text preset="formLabel" color="action"> + {translate("pending_count", { count: requestsCount })} + </Text> + </> + )} + </Text> + )); return ( <ConversationTitleDumb title={groupName ?? undefined} onLongPress={onLongPress} onPress={onPress} - subtitle={ - displayMemberText ? ( - <Text preset="formLabel"> - {memberText} - {requestsCount > 0 && ( - <> - {" • "} - <Text preset="formLabel" color="action"> - {translate("pending_count", { count: requestsCount })} - </Text> - </> - )} - </Text> - ) : null - } + subtitle={displayMemberText ? + <SubtitleContent + memberText={memberText} + requestsCount={requestsCount} + /> : null + } avatarComponent={avatarComponent} /> );components/GroupAvatar.tsx (1)
Line range hint
201-223: Consider extracting member processing logicThe
memoizedAndSortedGroupMemberslogic handles multiple responsibilities (filtering, transformation, and sorting). Consider extracting this into a separate hook or utility function for better maintainability and testability.+ type ProcessedMember = { + address: string; + uri?: string; + name?: string; + }; + + const useProcessedGroupMembers = ( + groupMembers: any, + profiles: any, + account: string | null, + excludeSelf: boolean + ): ProcessedMember[] => { + return useMemo(() => { + if (!groupMembers) return []; + const members = groupMembers.ids.reduce( + (acc: ProcessedMember[], id) => { + const member = groupMembers.byId[id]; + const address = member.addresses[0].toLowerCase(); + const senderSocials = getProfile(address, profiles)?.socials; + const shouldExclude = + excludeSelf && account && address === account.toLowerCase(); + if (shouldExclude) return acc; + acc.push({ + address, + uri: getPreferredAvatar(senderSocials), + name: getPreferredName(senderSocials, address), + }); + return acc; + }, + [] + ); + return members.sort((a, b) => (a.uri ? -1 : 1)); + }, [groupMembers, profiles, account, excludeSelf]); + }; const GroupAvatar: React.FC<GroupAvatarProps> = ({ // ...props }) => { - const memoizedAndSortedGroupMembers = useMemo(() => { - // ... existing logic - }, [groupMembers, profiles, account, excludeSelf]); + const memoizedAndSortedGroupMembers = useProcessedGroupMembers( + groupMembers, + profiles, + account, + excludeSelf + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
components/GroupAvatar.tsx(1 hunks)features/conversation-list/useGroupConversationListAvatarInfo.ts(1 hunks)features/conversations/components/GroupConversationTitle.tsx(2 hunks)hooks/useGroupMembers.ts(1 hunks)queries/useGroupMembersQuery.ts(3 hunks)
🔇 Additional comments (5)
queries/useGroupMembersQuery.ts (2)
18-35: LGTM: Well-structured error handling
The fetchGroupMembers function properly handles the case when group is undefined/null by returning an empty data structure, preventing potential runtime errors.
73-73: Verify the inverse condition for image URL
The query is enabled only when imageUrlSquare is absent. Please verify if this is the intended behavior, as it might prevent fetching members for groups with avatars.
✅ Verification successful
Let me search for the implementation of this query to understand its purpose better.
Let me check how the group members are used in the UI components to understand if this condition is correct.
Based on the code analysis, I can now provide the final response:
The condition for fetching group members is correct
The query is intentionally enabled only when imageUrlSquare is absent because it's used as a fallback for group avatars. When a group has no custom image (imageUrlSquare is null), the UI components (GroupConversationTitle, V3GroupConversationListItem, PinnedV3GroupConversation) display a composite avatar made up of group members' avatars using the GroupAvatarDumb component. This is a common pattern where member avatars are only needed for creating a fallback group avatar display.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of imageUrlSquare to verify the condition
rg "imageUrlSquare" -A 3 -B 3
Length of output: 8394
Script:
#!/bin/bash
# Search for the implementation of groupMembersQueryConfig
ast-grep --pattern 'const groupMembersQueryConfig = $_'
Length of output: 659
Script:
#!/bin/bash
# Search for components using group members data
rg "GroupAvatarDumb" -A 5 -B 5
Length of output: 7637
hooks/useGroupMembers.ts (1)
Line range hint 11-45: LGTM: Clean hook implementation
The hook provides a clean interface for group member operations, properly combining query and mutation capabilities.
features/conversation-list/useGroupConversationListAvatarInfo.ts (1)
Line range hint 1-43: LGTM: Clean transition to query-based approach
The switch from local state to query-based approach is well implemented, with proper data transformation and memoization.
components/GroupAvatar.tsx (1)
197-197: LGTM! Verify query behavior across different screens
The simplification of the useGroupMembers hook call improves code clarity. However, since the onConversationListScreen prop was removed, we should verify that this doesn't affect the query behavior in different contexts.
Let's verify the usage of useGroupMembers across the codebase:
✅ Verification successful
Let me verify the hook implementation to ensure the removal of onConversationListScreen parameter is safe.
Let me try a different approach to find the hook implementation.
Let me check the underlying query implementation to complete the verification.
Removal of onConversationListScreen prop is safe
The code analysis reveals that while there are separate query hooks (useGroupMembersQuery, useGroupMembersConversationScreenQuery, useConversationListMembersQuery), the useGroupMembers hook internally uses only useGroupMembersQuery. The removal of the onConversationListScreen prop is safe because:
- The base
useGroupMembershook was already using the generic query implementation - All current usages across the codebase use the same simple signature without screen-specific parameters
- Screen-specific optimizations are handled at the query level, not in the consumer hook
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of onConversationListScreen and useGroupMembers
# to ensure the removal doesn't break existing functionality
echo "Checking for any remaining onConversationListScreen usage..."
rg "onConversationListScreen"
echo "Checking useGroupMembers usage patterns..."
ast-grep --pattern 'useGroupMembers($$$)'
Length of output: 1193
Script:
#!/bin/bash
# Check the implementation of useGroupMembers hook
echo "Checking useGroupMembers hook implementation..."
rg -A 10 "export function useGroupMembers"
Length of output: 149
Script:
#!/bin/bash
# Find the file containing useGroupMembers hook definition
echo "Finding useGroupMembers hook file..."
fd "useGroupMembers" --type f
echo "Checking hook implementation..."
ast-grep --pattern 'const useGroupMembers = $$$'
ast-grep --pattern 'function useGroupMembers($$$)'
Length of output: 2520
Script:
#!/bin/bash
# Check the implementation of useGroupMembersQuery
echo "Checking query implementation..."
cat queries/useGroupMembersQuery.ts
Length of output: 3172
| group: GroupWithCodecsType | undefined | null, | ||
| enabled: boolean | ||
| ): UseQueryOptions<GroupMembersSelectData> => ({ | ||
| queryKey: groupMembersQueryKey(account, group?.topic!), |
There was a problem hiding this comment.
Remove non-null assertion operator
The non-null assertion on group?.topic! could lead to runtime errors if group is undefined. Consider using optional chaining or adding a type guard.
- queryKey: groupMembersQueryKey(account, group?.topic!),
+ queryKey: groupMembersQueryKey(account, group?.topic ?? ''),📝 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.
| queryKey: groupMembersQueryKey(account, group?.topic!), | |
| queryKey: groupMembersQueryKey(account, group?.topic ?? ''), |
| const { data: membersData } = useConversationListMembersQuery( | ||
| currentAccount, | ||
| group | ||
| ); |
There was a problem hiding this comment.
💡 Codebase verification
Query enabled condition could prevent member data fetching
The verification reveals that useConversationListMembersQuery is indeed conditionally enabled based on !!group && !group.imageUrlSquare. This means the query will not fetch member data when:
groupis null/undefined, orgroup.imageUrlSquareexists
This could be problematic as member data might be needed regardless of whether a group has an image URL or not, especially since the hook useGroupConversationListAvatarInfo appears to use this data for avatar-related functionality.
queries/useGroupMembersQuery.ts: Consider removing the!group.imageUrlSquarecondition from the enabled check unless there's a specific reason why member data is not needed when a group image exists.
🔗 Analysis chain
Verify query enabled condition
This query's enabled condition is tied to the absence of imageUrlSquare. Ensure this doesn't prevent necessary member data fetching.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential race conditions or missing data scenarios
rg "imageUrlSquare.*members" -A 5 -B 5
Length of output: 40
Script:
#!/bin/bash
# Let's check the implementation of useConversationListMembersQuery
ast-grep --pattern 'export function useConversationListMembersQuery($_) {
$$$
}'
# Also check the usage context of this hook
rg "useConversationListMembersQuery" -A 5 -B 5
Length of output: 2432
Performance Comparison ReportSignificant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
Render Count ChangesThere are no entries Render IssuesThere are no entries Added ScenariosThere are no entries Removed ScenariosThere are no entries |
* feat: Members Query on Conversation List Updates Group Members on conversation list to use Queries Updates Group Avatar to remove old props Updates Group members queries to use config pattern * Safety on Component
Updates Group Members on conversation list to use Queries
Updates Group Avatar to remove old props
Updates Group members queries to use config pattern
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
GroupAvatarcomponent by removing unnecessary complexity.useGroupMembersand related hooks.Documentation