Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Members Query on Conversation List #1343

Merged
merged 2 commits into from
Dec 12, 2024
Merged

feat: Members Query on Conversation List #1343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
facdbfd
feat: Members Query on Conversation List
alexrisch Dec 11, 2024
f56b0a8
Safety on Component
alexrisch Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 1 addition & 17 deletions components/GroupAvatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,6 @@ type GroupAvatarProps = {
topic?: ConversationTopic;
pendingGroupMembers?: { address: string; uri?: string; name?: string }[];
excludeSelf?: boolean;
// Converstion List should not make requests across the bridge
// Use data from the initial sync, and as the query gets updated
onConversationListScreen?: boolean;
};

type GroupAvatarDumbProps = {
Expand Down Expand Up @@ -194,23 +191,10 @@ const GroupAvatar: React.FC<GroupAvatarProps> = ({
topic,
pendingGroupMembers,
excludeSelf = true,
onConversationListScreen = false,
}) => {
const colorScheme = useColorScheme();
const styles = getStyles(colorScheme);
const { members: groupMembers } = useGroupMembers(
topic!,
onConversationListScreen
? {
refetchOnWindowFocus: false,
refetchOnMount: false,
refetchOnReconnect: false,
refetchInterval: false,
staleTime: Infinity,
enabled: !!topic && !pendingGroupMembers,
}
: undefined
);
const { members: groupMembers } = useGroupMembers(topic!);
const profiles = useProfilesStore((s) => s.profiles);
const account = useCurrentAccount();

Expand Down
27 changes: 9 additions & 18 deletions features/conversation-list/useGroupConversationListAvatarInfo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,26 @@
import { useConversationListMembersQuery } from "@/queries/useGroupMembersQuery";
import { useInboxProfileSocialsQueries } from "@queries/useInboxProfileSocialsQuery";
import {
getPreferredInboxAddress,
getPreferredInboxAvatar,
getPreferredInboxName,
} from "@utils/profile";
import { GroupWithCodecsType } from "@utils/xmtpRN/client";
import { InboxId, Member } from "@xmtp/react-native-sdk";
import { useEffect, useMemo, useState } from "react";
import type { InboxId } from "@xmtp/react-native-sdk";
import { useMemo } from "react";

export const useGroupConversationListAvatarInfo = (
currentAccount: string,
group?: GroupWithCodecsType
) => {
// TODO: Move this to a query to get persistence
const [members, setMembers] = useState<Member[]>([]);

useEffect(() => {
if (!group) return;
if (group?.imageUrlSquare) {
return;
}
const fetchMembers = async () => {
const members = await group.members();
setMembers(members || []);
};
fetchMembers();
}, [group]);
const { data: membersData } = useConversationListMembersQuery(
currentAccount,
group
);
Comment on lines +16 to +19
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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:

  • group is null/undefined, or
  • group.imageUrlSquare exists

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.imageUrlSquare condition 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


const memberInboxIds = useMemo(() => {
return members.map((member) => member.inboxId);
}, [members]);
return membersData?.ids || [];
}, [membersData]);

const data = useInboxProfileSocialsQueries(currentAccount, memberInboxIds);

Expand Down
4 changes: 2 additions & 2 deletions features/conversations/components/GroupConversationTitle.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ export const GroupConversationTitle = memo(
onLongPress={onLongPress}
onPress={onPress}
subtitle={
displayMemberText && (
displayMemberText ? (
<Text preset="formLabel">
{memberText}
{requestsCount > 0 && (
Expand All @@ -110,7 +110,7 @@ export const GroupConversationTitle = memo(
</>
)}
</Text>
)
) : null
}
avatarComponent={avatarComponent}
/>
Expand Down
14 changes: 3 additions & 11 deletions hooks/useGroupMembers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,21 @@
import { QueryObserverOptions } from "@tanstack/react-query";

import { currentAccount } from "../data/store/accountsStore";
import { useAddToGroupMutation } from "../queries/useAddToGroupMutation";
import {
GroupMembersSelectData,
useGroupMembersQuery,
} from "../queries/useGroupMembersQuery";
import { useGroupMembersQuery } from "../queries/useGroupMembersQuery";
import { usePromoteToAdminMutation } from "../queries/usePromoteToAdminMutation";
import { usePromoteToSuperAdminMutation } from "../queries/usePromoteToSuperAdminMutation";
import { useRemoveFromGroupMutation } from "../queries/useRemoveFromGroupMutation";
import { useRevokeAdminMutation } from "../queries/useRevokeAdminMutation";
import { useRevokeSuperAdminMutation } from "../queries/useRevokeSuperAdminMutation";
import type { ConversationTopic } from "@xmtp/react-native-sdk";

export const useGroupMembers = (
topic: ConversationTopic,
queryOptions?: Partial<QueryObserverOptions<GroupMembersSelectData>>
) => {
export const useGroupMembers = (topic: ConversationTopic) => {
const account = currentAccount();

const {
data: members,
isLoading,
isError,
} = useGroupMembersQuery(account, topic, queryOptions);
} = useGroupMembersQuery(account, topic);
const { mutateAsync: promoteToAdmin } = usePromoteToAdminMutation(
account,
topic
Expand Down
100 changes: 52 additions & 48 deletions queries/useGroupMembersQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {
QueryObserverOptions,
SetDataOptions,
useQuery,
UseQueryOptions,
} from "@tanstack/react-query";
import { getCleanAddress } from "@utils/evm/getCleanAddress";
import { ConversationTopic, Member } from "@xmtp/react-native-sdk";
Expand All @@ -11,65 +11,69 @@ import { groupMembersQueryKey } from "./QueryKeys";
import { EntityObjectWithAddress, entifyWithAddress } from "./entify";
import { queryClient } from "./queryClient";
import { useGroupQuery } from "./useGroupQuery";
import { GroupWithCodecsType } from "@/utils/xmtpRN/client.types";

export type GroupMembersSelectData = EntityObjectWithAddress<Member, InboxId>;

const fetchGroupMembers = async (
group: GroupWithCodecsType | undefined | null
) => {
if (!group) {
return {
byId: {},
byAddress: {},
ids: [],
};
}
const members = await group.members();

return entifyWithAddress(
members,
(member) => member.inboxId,
(member) => getCleanAddress(member.addresses[0])
);
};

const groupMembersQueryConfig = (
account: string,
group: GroupWithCodecsType | undefined | null,
enabled: boolean
): UseQueryOptions<GroupMembersSelectData> => ({
queryKey: groupMembersQueryKey(account, group?.topic!),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
queryKey: groupMembersQueryKey(account, group?.topic!),
queryKey: groupMembersQueryKey(account, group?.topic ?? ''),

queryFn: () => fetchGroupMembers(group!),
enabled,
});

export const useGroupMembersQuery = (
account: string,
topic: ConversationTopic,
queryOptions?: Partial<QueryObserverOptions<GroupMembersSelectData>>
topic: ConversationTopic
) => {
const { data: group } = useGroupQuery(account, topic);
return useQuery<GroupMembersSelectData>({
queryKey: groupMembersQueryKey(account, topic!),
queryFn: async () => {
if (!group) {
return {
byId: {},
byAddress: {},
ids: [],
};
}
const updatedMembers = await group.members();
return entifyWithAddress(
updatedMembers,
(member) => member.inboxId,
// TODO: Multiple addresses support
(member) => getCleanAddress(member.addresses[0])
);
},
enabled: !!group && !!topic,
...queryOptions,
});
const enabled = !!group && !!topic;
return useQuery<GroupMembersSelectData>(
groupMembersQueryConfig(account, group, enabled)
);
};

export const useGroupMembersConversationScreenQuery = (
account: string,
topic: ConversationTopic,
queryOptions?: Partial<QueryObserverOptions<GroupMembersSelectData>>
topic: ConversationTopic
) => {
const { data: group } = useGroupQuery(account, topic);
return useQuery<GroupMembersSelectData>({
queryKey: groupMembersQueryKey(account, topic),
queryFn: async () => {
if (!group) {
return {
byId: {},
byAddress: {},
ids: [],
};
}
const members = await group.members();
return entifyWithAddress(
members,
(member) => member.inboxId,
// TODO: Multiple addresses support
(member) => getCleanAddress(member.addresses[0])
);
},
enabled: !!group,
...queryOptions,
});
const enabled = !!group && !!topic;
return useQuery<GroupMembersSelectData>(
groupMembersQueryConfig(account, group, enabled)
);
};

export const useConversationListMembersQuery = (
account: string,
group: GroupWithCodecsType | undefined | null
) => {
const enabled = !!group && !group.imageUrlSquare;
return useQuery<GroupMembersSelectData>(
groupMembersQueryConfig(account, group, enabled)
);
};

export const getGroupMembersQueryData = (
Expand All @@ -84,7 +88,7 @@ export const setGroupMembersQueryData = (
members: GroupMembersSelectData,
options?: SetDataOptions
) => {
queryClient.setQueryData(
queryClient.setQueryData<GroupMembersSelectData>(
groupMembersQueryKey(account, topic),
members,
options
Expand Down
Loading