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.

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: V3 Search #1261

Merged
merged 3 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
161 changes: 161 additions & 0 deletions features/conversations/utils/__tests__/search.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
import { dmMatchesSearchQuery, groupMatchesSearchQuery } from "../search";
import { getInboxProfileSocialsQueryData } from "@/queries/useInboxProfileSocialsQuery";
import type {
DmWithCodecsType,
GroupWithCodecsType,
} from "@/utils/xmtpRN/client";

// Mock getInboxProfileSocialsQueryData
jest.mock("@/queries/useInboxProfileSocialsQuery", () => ({
getInboxProfileSocialsQueryData: jest.fn(),
}));

// Mock getPreferredInboxName
jest.mock("@/utils/profile", () => ({
getPreferredInboxName: jest.fn((profiles) => profiles?.userNames?.[0] || ""),
}));

const mockGetInboxProfileSocialsQueryData =
getInboxProfileSocialsQueryData as jest.Mock;

describe("Search Query Matchers", () => {
const account = "testAccount";
const searchQuery = "test";
const mockInboxId = "mockInboxId";

beforeEach(() => {
jest.clearAllMocks();
});

describe("dmMatchesSearchQuery", () => {
it("returns true if inboxId matches the search query", async () => {
const mockDm = {
peerInboxId: jest.fn().mockResolvedValue(mockInboxId),
members: jest.fn().mockResolvedValue([]),
} as unknown as DmWithCodecsType;
Comment on lines +32 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve type safety in test mocks

Instead of using type casting with as unknown as DmWithCodecsType, consider creating a proper mock factory function that returns a correctly typed mock object. This would catch type issues earlier and make the tests more maintainable.

function createMockDm(overrides?: Partial<DmWithCodecsType>): DmWithCodecsType {
  return {
    peerInboxId: jest.fn().mockResolvedValue("defaultId"),
    members: jest.fn().mockResolvedValue([]),
    ...overrides
  };
}

Also applies to: 46-49


mockGetInboxProfileSocialsQueryData.mockReturnValue([
{ userNames: ["testUser"] },
]);

const result = await dmMatchesSearchQuery({
account,
searchQuery,
dm: mockDm,
});
expect(result).toBe(true);
});

it("returns true if a member's address matches the search query", async () => {
const mockDm = {
peerInboxId: jest.fn().mockResolvedValue("nonMatchingId"),
members: jest.fn().mockResolvedValue([{ addresses: ["testAddress"] }]),
} as unknown as DmWithCodecsType;

mockGetInboxProfileSocialsQueryData.mockReturnValue(null);

const result = await dmMatchesSearchQuery({
account,
searchQuery,
dm: mockDm,
});
expect(result).toBe(true);
});

it("returns false if neither inboxId nor members match the search query", async () => {
const mockDm = {
peerInboxId: jest.fn().mockResolvedValue("nonMatchingId"),
members: jest
.fn()
.mockResolvedValue([{ addresses: ["nonMatchingAddress"] }]),
} as unknown as DmWithCodecsType;

mockGetInboxProfileSocialsQueryData.mockReturnValue(null);

const result = await dmMatchesSearchQuery({
account,
searchQuery,
dm: mockDm,
});
expect(result).toBe(false);
});
});

describe("groupMatchesSearchQuery", () => {
it("returns true if group name matches the search query", async () => {
const mockGroup = {
name: "testGroupName",
members: jest.fn().mockResolvedValue([]),
} as unknown as GroupWithCodecsType;

const result = await groupMatchesSearchQuery({
account,
searchQuery,
group: mockGroup,
});
expect(result).toBe(true);
});

it("returns true if a member's inboxId matches the search query", async () => {
const mockGroup = {
name: "nonMatchingName",
members: jest
.fn()
.mockResolvedValue([
{ inboxId: mockInboxId, addresses: ["nonMatchingAddress"] },
]),
} as unknown as GroupWithCodecsType;

mockGetInboxProfileSocialsQueryData.mockReturnValue([
{ userNames: ["testUser"] },
]);

const result = await groupMatchesSearchQuery({
account,
searchQuery: "mockInbox",
group: mockGroup,
});
expect(result).toBe(true);
});

it("returns true if a member's address matches the search query", async () => {
const mockGroup = {
name: "nonMatchingName",
members: jest
.fn()
.mockResolvedValue([
{ inboxId: "nonMatchingId", addresses: ["testAddress"] },
]),
} as unknown as GroupWithCodecsType;

mockGetInboxProfileSocialsQueryData.mockReturnValue(null);

const result = await groupMatchesSearchQuery({
account,
searchQuery,
group: mockGroup,
});
expect(result).toBe(true);
});

it("returns false if neither group name, inboxId, nor members match the search query", async () => {
const mockGroup = {
name: "nonMatchingName",
members: jest
.fn()
.mockResolvedValue([
{ inboxId: "nonMatchingId", addresses: ["nonMatchingAddress"] },
]),
} as unknown as GroupWithCodecsType;

mockGetInboxProfileSocialsQueryData.mockReturnValue(null);

const result = await groupMatchesSearchQuery({
account,
searchQuery,
group: mockGroup,
});
expect(result).toBe(false);
});
});
});
113 changes: 113 additions & 0 deletions features/conversations/utils/search.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
import { getInboxProfileSocialsQueryData } from "@/queries/useInboxProfileSocialsQuery";
import { getPreferredInboxName } from "@/utils/profile";
import type {
DmWithCodecsType,
GroupWithCodecsType,
} from "@/utils/xmtpRN/client";
import type { InboxId } from "@xmtp/react-native-sdk";

type DmSearchParams = {
account: string;
searchQuery: string;
dm: DmWithCodecsType;
};

export const dmMatchesSearchQuery = async ({
account,
searchQuery,
dm,
}: DmSearchParams): Promise<boolean> => {
const inboxId = await dm.peerInboxId();
if (await inboxIdMatchesSearchQuery({ account, searchQuery, inboxId })) {
return true;
}
const members = await dm.members();
for (const member of members) {
if (
addressMatchesSearchQuery({
searchQuery,
address: member.addresses[0],
})
) {
return true;
}
}
return false;
};

type GroupSearchParams = {
account: string;
searchQuery: string;
group: GroupWithCodecsType;
};

export const groupMatchesSearchQuery = async ({
account,
searchQuery,
group,
}: GroupSearchParams): Promise<boolean> => {
if (group.name.toLowerCase().includes(searchQuery.toLowerCase())) {
return true;
}
Comment on lines +49 to +51
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize case-insensitive string comparisons

Converting strings to lowercase on every comparison is inefficient. Consider normalizing the search query once at the start.

 export const groupMatchesSearchQuery = async ({
   account,
   searchQuery,
   group,
 }: GroupSearchParams): Promise<boolean> => {
   if (!group) {
     return false;
   }
+  const normalizedQuery = searchQuery.toLowerCase();
-  if (group.name.toLowerCase().includes(searchQuery.toLowerCase())) {
+  if (group.name.toLowerCase().includes(normalizedQuery)) {
     return true;
   }

Committable suggestion skipped: line range outside the PR's diff.

const members = await group.members();
for (const member of members) {
if (
await inboxIdMatchesSearchQuery({
account,
searchQuery,
inboxId: member.inboxId,
})
) {
return true;
}
if (
addressMatchesSearchQuery({
searchQuery,
address: member.addresses[0],
})
) {
return true;
}
}
return false;
};

type InboxIdSearchParams = {
account: string;
searchQuery: string;
inboxId: InboxId;
};

const inboxIdMatchesSearchQuery = async ({
account,
searchQuery,
inboxId,
}: InboxIdSearchParams): Promise<boolean> => {
if (inboxId.toLowerCase().includes(searchQuery.toLowerCase())) {
return true;
}
const profiles = getInboxProfileSocialsQueryData(account, inboxId);
if (!profiles) {
return false;
}
Comment on lines +89 to +92
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

Fix missing await for async operation

The getInboxProfileSocialsQueryData call is likely asynchronous but isn't awaited, which could lead to race conditions.

-  const profiles = getInboxProfileSocialsQueryData(account, inboxId);
+  const profiles = await getInboxProfileSocialsQueryData(account, inboxId);
   if (!profiles) {
     return false;
   }

Committable suggestion skipped: line range outside the PR's diff.

const name = getPreferredInboxName(profiles);
if (name.toLowerCase().includes(searchQuery.toLowerCase())) {
return true;
}
return false;
};

type AddressSearchParams = {
searchQuery: string;
address: string;
};

const addressMatchesSearchQuery = ({
searchQuery,
address,
}: AddressSearchParams): boolean => {
if (!address) {
return false;
}
return address.toLowerCase().includes(searchQuery.toLowerCase());
};
1 change: 1 addition & 0 deletions queries/__snapshots__/QueryKeys.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ exports[`QueryKeys should match the snapshot 1`] = `
"GROUP_MEMBERS": "groupMembersv2",
"GROUP_NAME": "groupName",
"GROUP_PERMISSIONS": "groupPermissions",
"GROUP_PERMISSION_POLICY": "groupPermissionPolicy",
"GROUP_PHOTO": "groupPhoto",
"PENDING_JOIN_REQUESTS": "pendingJoinRequests",
"PINNED_FRAME": "pinnedFrame",
Expand Down
21 changes: 16 additions & 5 deletions queries/useInboxProfileSocialsQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const fetchInboxProfileSocials = async (inboxIds: InboxId) => {
return data;
};

const inboxProfileSocialesQueryConfig = (
const inboxProfileSocialsQueryConfig = (
account: string,
inboxId: InboxId | undefined
) => ({
Expand All @@ -54,7 +54,7 @@ export const useInboxProfileSocialsQuery = (
account: string,
inboxId: InboxId | undefined
) => {
return useQuery(inboxProfileSocialesQueryConfig(account, inboxId));
return useQuery(inboxProfileSocialsQueryConfig(account, inboxId));
};

export const useInboxProfileSocialsQueries = (
Expand All @@ -63,7 +63,7 @@ export const useInboxProfileSocialsQueries = (
) => {
return useQueries({
queries: inboxIds.map((inboxId) =>
inboxProfileSocialesQueryConfig(account, inboxId)
inboxProfileSocialsQueryConfig(account, inboxId)
),
});
};
Expand All @@ -73,7 +73,7 @@ export const fetchInboxProfileSocialsQuery = (
inboxId: InboxId
) => {
return queryClient.fetchQuery(
inboxProfileSocialesQueryConfig(account, inboxId)
inboxProfileSocialsQueryConfig(account, inboxId)
);
};

Expand All @@ -84,10 +84,21 @@ export const setInboxProfileSocialsQueryData = (
updatedAt?: number
) => {
return queryClient.setQueryData(
profileSocialsQueryKey(account, inboxId),
inboxProfileSocialsQueryConfig(account, inboxId).queryKey,
data,
{
updatedAt,
}
);
};

export const getInboxProfileSocialsQueryData = (
account: string,
inboxId: InboxId
): ProfileSocials[] | null => {
return (
queryClient.getQueryData(
inboxProfileSocialsQueryConfig(account, inboxId).queryKey
) ?? null
);
};
41 changes: 38 additions & 3 deletions screens/ConversationList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ import { useConversationListRequestCount } from "../features/conversation-list/u
import { useConversationListItems } from "../features/conversation-list/useConversationListItems";
import { ConversationWithCodecsType } from "@utils/xmtpRN/client";
import { ConversationContextMenu } from "@/components/ConversationContextMenu";
import { ConversationVersion } from "@xmtp/react-native-sdk";
import {
dmMatchesSearchQuery,
groupMatchesSearchQuery,
} from "@/features/conversations/utils/search";

type Props = {
searchBarRef:
Expand Down Expand Up @@ -109,9 +114,39 @@ function ConversationList({ navigation, route, searchBarRef }: Props) {
}, [initialLoadDoneOnce, currentAccount]);

useEffect(() => {
// TODO:
setFlatListItems({ items: items ?? [], searchQuery });
}, [searchQuery, profiles, items]);
const getFilteredItems = async () => {
const filteredItems: FlatListItemType[] = [];
for (const item of items ?? []) {
if (item.version === ConversationVersion.GROUP) {
const groupMatches = await groupMatchesSearchQuery({
account: currentAccount!,
searchQuery,
group: item,
});
Comment on lines +121 to +125
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

Improve type safety by validating currentAccount

The code uses non-null assertions (currentAccount!) which could lead to runtime errors if currentAccount is null.

Add early return or validation:

 const getFilteredItems = async () => {
+  if (!currentAccount) {
+    console.warn('No current account available for search');
+    return [];
+  }
   const filteredItems: FlatListItemType[] = [];
   for (const item of items ?? []) {
     if (item.version === ConversationVersion.GROUP) {
       const groupMatches = await groupMatchesSearchQuery({
-        account: currentAccount!,
+        account: currentAccount,
         searchQuery,
         group: item,
       });
       // ... rest of the code
     } else if (item.version === ConversationVersion.DM) {
       const dmMatches = await dmMatchesSearchQuery({
-        account: currentAccount!,
+        account: currentAccount,
         searchQuery,
         dm: item,
       });
       // ... rest of the code
     }
   }
   return filteredItems ?? [];
 };

Also applies to: 130-134

if (groupMatches) {
filteredItems.push(item);
}
} else if (item.version === ConversationVersion.DM) {
const dmMatches = await dmMatchesSearchQuery({
account: currentAccount!,
searchQuery,
dm: item,
});
if (dmMatches) {
filteredItems.push(item);
}
}
}
return filteredItems ?? [];
};
Comment on lines +117 to +141
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

Add error handling for async operations

The async filtering logic lacks error handling, which could lead to unhandled promise rejections if either dmMatchesSearchQuery or groupMatchesSearchQuery fails.

Add try-catch block:

 const getFilteredItems = async () => {
+  try {
     const filteredItems: FlatListItemType[] = [];
     for (const item of items ?? []) {
       if (item.version === ConversationVersion.GROUP) {
         const groupMatches = await groupMatchesSearchQuery({
           account: currentAccount!,
           searchQuery,
           group: item,
         });
         if (groupMatches) {
           filteredItems.push(item);
         }
       } else if (item.version === ConversationVersion.DM) {
         const dmMatches = await dmMatchesSearchQuery({
           account: currentAccount!,
           searchQuery,
           dm: item,
         });
         if (dmMatches) {
           filteredItems.push(item);
         }
       }
     }
     return filteredItems ?? [];
+  } catch (error) {
+    console.error('Search filtering failed:', error);
+    return [];
+  }
 };
📝 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
const getFilteredItems = async () => {
const filteredItems: FlatListItemType[] = [];
for (const item of items ?? []) {
if (item.version === ConversationVersion.GROUP) {
const groupMatches = await groupMatchesSearchQuery({
account: currentAccount!,
searchQuery,
group: item,
});
if (groupMatches) {
filteredItems.push(item);
}
} else if (item.version === ConversationVersion.DM) {
const dmMatches = await dmMatchesSearchQuery({
account: currentAccount!,
searchQuery,
dm: item,
});
if (dmMatches) {
filteredItems.push(item);
}
}
}
return filteredItems ?? [];
};
const getFilteredItems = async () => {
try {
const filteredItems: FlatListItemType[] = [];
for (const item of items ?? []) {
if (item.version === ConversationVersion.GROUP) {
const groupMatches = await groupMatchesSearchQuery({
account: currentAccount!,
searchQuery,
group: item,
});
if (groupMatches) {
filteredItems.push(item);
}
} else if (item.version === ConversationVersion.DM) {
const dmMatches = await dmMatchesSearchQuery({
account: currentAccount!,
searchQuery,
dm: item,
});
if (dmMatches) {
filteredItems.push(item);
}
}
}
return filteredItems ?? [];
} catch (error) {
console.error('Search filtering failed:', error);
return [];
}
};

if (searchQuery.trim().length > 0) {
getFilteredItems().then((filteredItems) => {
setFlatListItems({ items: filteredItems, searchQuery });
});
} else {
setFlatListItems({ items: items ?? [], searchQuery });
}
}, [searchQuery, profiles, items, currentAccount]);

// Search bar hook
useHeaderSearchBar({
Expand Down