feat: Conversation list polished #1514
Conversation
WalkthroughThis pull request introduces significant refactoring and simplification across multiple components of the application, focusing on conversation management, query handling, and state management. The changes primarily involve removing unnecessary complexity, updating import paths, and introducing more modular and efficient hooks and query management. Key areas of modification include conversation list handling, error tracking, topic management, and conversation data retrieval. Changes
Sequence DiagramsequenceDiagram
participant User
participant ConversationList
participant ConversationQuery
participant AccountStore
participant TopicAPI
User->>ConversationList: Interact with conversations
ConversationList->>ConversationQuery: Fetch conversations
ConversationQuery->>AccountStore: Get current account
AccountStore-->>ConversationQuery: Return account details
ConversationQuery->>TopicAPI: Retrieve conversation topics
TopicAPI-->>ConversationQuery: Return conversation data
ConversationQuery-->>ConversationList: Render conversations
Possibly related PRs
Suggested Reviewers
Poem
Finishing Touches
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 (
|
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🔭 Outside diff range comments (2)
features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversations.tsx (1)
Line range hint
85-98: Add error boundary around PinnedConversationWrapper.The error handling in PinnedConversationWrapper could be improved to prevent the entire list from crashing.
Consider wrapping the component with an error boundary or implementing more graceful error handling:
if (!conversation) { captureError( new Error( `Couldn't find conversation ${topic} in PinnedConversationWrapper` ) ); - return null; + return <ErrorFallback topic={topic} />; }components/XmtpEngine.tsx (1)
Line range hint
124-139: Handle potential race conditions in syncAccounts.The current implementation might have race conditions when multiple sync operations are triggered simultaneously.
Consider using a queue or debounce mechanism:
async syncAccounts(accountsToSync: string[]) { + const pendingSync = new Set(); accountsToSync.forEach((a) => { - if (!this.syncingAccounts[a]) { + if (!this.syncingAccounts[a] && !pendingSync.has(a)) { + pendingSync.add(a); logger.info(`[XmtpEngine] Syncing account ${a}`); this.syncedAccounts[a] = true; this.syncingAccounts[a] = true; syncConversationListXmtpClient(a) .then(() => { this.syncingAccounts[a] = false; + pendingSync.delete(a); }) .catch(() => { this.syncingAccounts[a] = false; + pendingSync.delete(a); }); } }); }
🧹 Nitpick comments (36)
features/conversation-list/hooks/use-delete-dm.ts (1)
61-112: Add loading state management.The UI might benefit from showing loading states during delete/block operations to prevent multiple clicks and provide better feedback.
Consider adding a loading state:
export const useDeleteDm = (dm: DmWithCodecsType) => { + const [isLoading, setIsLoading] = useState(false); // ... existing code ... return useCallback(() => { + if (isLoading) return; const actions = [ { label: translate("delete"), action: async () => { try { + setIsLoading(true); await deleteDmAsync(); } catch (error) { captureErrorWithToast(error); + } finally { + setIsLoading(false); } }, }, // ... rest of the code ... ]; - }, [colorScheme, dm, currentAccount, preferredName, deleteDmAsync]); + }, [colorScheme, dm, currentAccount, preferredName, deleteDmAsync, isLoading]);data/store/chatStore.ts (1)
44-46: Consider persisting thereconnectingstate if neededCurrently, only
groupInviteLinksare persisted in thepartializefunction. If thereconnectingstate should be retained across sessions, consider including it in the persisted state.Apply this diff to include
reconnectingin the persisted state:partialize: (state) => { return { + reconnecting: state.reconnecting, groupInviteLinks: state.groupInviteLinks, }; },features/conversation-list/hooks/use-conversation-is-unread.ts (1)
37-52: OptimizeuseMemodependenciesIn the
useMemohook, you might consider excludingisLoadingfrom the dependency array if it's not used within the memoized function. This can prevent unnecessary re-computations whenisLoadingchanges.Apply this diff to adjust the dependencies:
return useMemo(() => { // function body - }, [lastMessage, currentInboxId, isLoading, readUntil]); + }, [lastMessage, currentInboxId, readUntil]);queries/use-conversation-data-query.ts (2)
33-35: Addaccountto theenabledconditionIn
getConversationDataQueryOptions, the query is enabled based on!!topic. Ifaccountcan also beundefined, consider adding it to theenabledcondition to prevent potential errors.Apply this diff to update the
enabledflag:return { queryKey: conversationDataQueryKey(account, topic), queryFn: () => getConversationData({ account, topic: topic! }), - enabled: !!topic, + enabled: !!account && !!topic, };
81-83: Simplify resolver matching logicIn the
resolverfunction, constructing strings to compare accounts and topics might be error-prone. Consider comparing the properties directly for better clarity and performance.Apply this diff to simplify the comparison:
const match = items.find( (item) => - `${query.account}-${query.topic}` === `${item.account}-${item.topic}` + query.account === item.account && query.topic === item.topic );features/conversation-list/conversation-list-item/conversation-list-item-group.tsx (1)
88-92: Improve hit area handling forContextMenuButtonThe
hitSlopproperty is not functioning as expected withContextMenuButton, and padding is used as a workaround. Investigate the underlying issue withhitSloponContextMenuButtonto implement a proper solution that enhances user interaction without affecting the layout.features/conversation-list/conversation-list.screen-header.tsx (2)
96-102: Avoid displaying unimplemented menu itemsThe menu action for
"all-chats"is not implemented and currently triggers an alert. To improve user experience, consider removing or disabling this menu item until the feature is available.
151-151: Ensure consistent translation key usageThe translation key
"App settings"may not align with the project's key naming conventions. Consider using a consistent format, such as"app_settings", to maintain uniformity across the translation files.features/conversation-list/conversation-list.screen.tsx (1)
199-214: Enhance user experience during data loadingFiltering out conversations while their data queries are loading can lead to conversations appearing and disappearing in the list, causing a jarring user experience. Consider including all conversations in the list and displaying loading indicators for those still fetching data.
features/conversation-list/hooks/use-conversation-is-pinned.ts (1)
17-20: Consider exposing loading and error states.The hook only returns
isPinnedbut consumers might need to know if the data is still loading or if there was an error fetching it.return { isPinned: data?.isPinned, + isLoading: isLoading, + error: error, };features/conversation-list/hooks/use-conversations-count.ts (1)
13-20: Improve error handling and performance.A few suggestions to enhance robustness and performance:
- Handle potential null account state
- Consider debouncing count updates for better performance
- Memoize the query config to prevent unnecessary re-renders
+ const queryConfig = useMemo( + () => ({ + ...conversationListQueryConfig({ + account: account ?? undefined, + context: "useConversationsCount", + }), + }), + [account] + ); useEffect(() => { - const unsubscribe = new QueryObserver(queryClient, { - ...conversationListQueryConfig({ - account: account!, - context: "useConversationsCount", - }), - }).subscribe(({ data }) => { - setCount(data?.length ?? 0); + const unsubscribe = new QueryObserver( + queryClient, + queryConfig + ).subscribe(({ data, error }) => { + if (error) { + console.error('Failed to fetch conversations:', error); + } + setCount(data?.length ?? 0); });features/conversation-list/conversation-list-item/conversation-list-item-swipeable/conversation-list-item-swipeable-toggle-read-action.tsx (1)
12-14: Consider memoizing hook parameters.To prevent unnecessary re-renders, consider memoizing the parameters object passed to
useConversationIsUnread.+ const hookParams = useMemo( + () => ({ + topic: props.topic, + }), + [props.topic] + ); - const { isUnread } = useConversationIsUnread({ - topic: props.topic, - }); + const { isUnread } = useConversationIsUnread(hookParams);features/conversation-list/conversation-list-item/conversation-list-item-avatar-skeleton.tsx (2)
32-35: Memoize gradient colors array.The colors array is recreated on every render. Consider memoizing it for better performance.
+ const gradientColors = useMemo( + () => [ + hexToRGBA(theme.colors.background.surface, 0), + theme.colors.background.surface, + ], + [theme.colors.background.surface] + ); <LinearGradient isAbsoluteFill style={{ borderRadius: 999, height: size, width: size, }} - colors={[ - hexToRGBA(theme.colors.background.surface, 0), - theme.colors.background.surface, - ]} + colors={gradientColors} />
Line range hint
16-23: Memoize container style object.The style object is recreated on every render. Consider memoizing it for better performance.
+ const containerStyle = useMemo( + () => ({ + width: size, + height: size, + borderRadius: 999, + backgroundColor: color, + }), + [size, color] + ); <Center - style={{ - width: size, - height: size, - borderRadius: 999, - backgroundColor: color, - }} + style={containerStyle} >features/conversation-list/conversation-list-empty.tsx (1)
16-17: Consider extracting the magic number.The number of skeleton items (8) should be extracted into a named constant with a clear explanation of why this specific number was chosen.
+const SKELETON_ITEMS_COUNT = 8; // Number of items that typically fit in the viewport {/* 8 to fill up the screen */} -{new Array(8).fill(null).map((_, index) => ( +{new Array(SKELETON_ITEMS_COUNT).fill(null).map((_, index) => (features/blocked-chats/use-blocked-chats.ts (1)
27-39: Consider memoizing filter conditions.The filter conditions could be memoized separately to prevent unnecessary recalculations when only one dependency changes.
+ const isConversationBlocked = useCallback( + (conversation: Conversation, query: QueryResult) => + query?.data?.isDeleted || conversation.state === "denied", + [] + ); const blockedConversations = useMemo(() => { if (!data) return []; return data.filter((conversation, index) => { const query = conversationsDataQueries[index]; - return ( - query?.data?.isDeleted || - conversation.state === "denied" - ); + return isConversationBlocked(conversation, query); }); }, [data, conversationsDataQueries]);hooks/use-current-account-group-members-info.ts (1)
52-52: Improve type safety in filter operation.The type assertion after filter could be replaced with a type predicate for better type safety.
- .filter(Boolean) as IMemberInfo[], + .filter((info): info is IMemberInfo => info !== null),features/conversation-list/hooks/useShouldShowConnecting.tsx (1)
Line range hint
19-52: Refactor interval management for better clarity.The interval management logic is complex and could be simplified. Also, the 15-second delay should be a named constant.
+const CONNECTION_WARNING_DELAY_MS = 15000; + if (!isInternetReachable || reconnecting) { if (conditionTrueTime.current === 0) { conditionTrueTime.current = Date.now(); } interval = setInterval(() => { - if (Date.now() - conditionTrueTime.current >= 15000) { + if (Date.now() - conditionTrueTime.current >= CONNECTION_WARNING_DELAY_MS) { sentryTrackMessage("Connecting has been show for 15 seconds", { isInternetReachable, reconnecting, });features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation-group.tsx (1)
Line range hint
31-32: Simplify displayMessagePreview condition.The condition could be more readable by combining the checks.
- const displayMessagePreview = - group.lastMessage && isTextMessage(group.lastMessage) && isUnread; + const displayMessagePreview = isUnread && group.lastMessage && isTextMessage(group.lastMessage);features/conversation-list/hooks/use-pin-or-unpin-conversation.tsx (1)
37-51: Consider adding loading state during optimistic update.The optimistic update is well implemented with proper rollback, but it might be helpful to track and expose the loading state to prevent duplicate mutations.
return { pinOrUnpinConversationAsync: pinOrUnpinConversationAsync, + isPending: isPending, };features/conversation-list/conversation-list-requests-list-item.tsx (1)
38-50: Consider accessibility improvements for the chat bubble icon.The Image component should have accessibility props for screen readers.
<Image source={theme.isDark ? require("@/assets/icons/chat-bubble-dark.png") : require("@/assets/icons/chat-bubble-light.png") } + accessibilityLabel={translate("chat_requests_icon")} + accessibilityRole="image" style={{ width: theme.avatarSize.sm, aspectRatio: 1, }} contentFit="contain" />components/Snackbar/SnackbarBackdrop/SnackbarBackdrop.tsx (2)
44-47: Consider memoizing color values to prevent unnecessary recalculations.The
hexToRGBAcalculations could be memoized to optimize performance.+import { useMemo } from "react"; +const gradientColors = useMemo(() => [ + hexToRGBA(theme.colors.background.surface, 0), + hexToRGBA(theme.colors.background.surface, 1), +], [theme.colors.background.surface]); <LinearGradient locations={[0, 0.55]} - colors={[ - hexToRGBA(theme.colors.background.surface, 0), - hexToRGBA(theme.colors.background.surface, 1), - ]} + colors={gradientColors} style={StyleSheet.absoluteFill} />
Line range hint
50-70: Track MaskedView stability issues.The TODO comment about MaskedView stability issues should be tracked in the issue tracker.
Would you like me to create a GitHub issue to track the MaskedView stability investigation and potential reintegration?
queries/useConversationQuery.ts (1)
86-92: Consider enhancing error handling in getOrFetchConversation.While the implementation follows a good cache-first strategy, it would benefit from explicit error handling to provide better feedback when queries fail.
export function getOrFetchConversation(args: IArgs) { const conversation = getConversationQueryData(args); if (conversation) { return Promise.resolve(conversation); } - return queryClient.fetchQuery(getConversationQueryOptions(args)); + return queryClient.fetchQuery(getConversationQueryOptions(args)) + .catch(error => { + console.error('Failed to fetch conversation:', error); + throw error; + }); }features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation-dm.tsx (2)
Line range hint
48-53: Simplify displayMessagePreview logic.The condition can be made more concise and readable.
- const displayMessagePreview = - conversation.lastMessage && - isTextMessage(conversation.lastMessage) && - isUnread; + const displayMessagePreview = isUnread && + conversation.lastMessage && + isTextMessage(conversation.lastMessage);
Line range hint
29-31: Remove redundant null assertion.The currentAccount is already asserted as non-null in the declaration.
const { data: peerInboxId } = useDmPeerInboxId({ - account: currentAccount!, + account: currentAccount, topic: conversationTopic, });features/conversation-list/hooks/use-toggle-read-status.ts (2)
69-80: Remove duplicate error capture.The error is captured twice: once in
onErrorand again intoggleReadStatusAsync.onError: (error, _, context) => { const currentAccount = getCurrentAccount(); - captureError(error); setConversationDataQueryData({ account: currentAccount!, topic, data: { readUntil: context?.previousReadUntil, }, }); captureErrorWithToast(error); },
15-18: Enhance type safety for the hook props.Consider making the topic property required explicitly.
-type UseToggleReadStatusProps = { +type UseToggleReadStatusProps = Readonly<{ topic: ConversationTopic; -}; +}>;utils/date.ts (1)
Line range hint
69-92: Consider adding i18n support for time units.The compact relative time strings should be internationalized for better global support.
Add i18n support:
if (years > 0) return `${years}y`; - if (months > 0) return `${months}mo`; - if (weeks > 0) return `${weeks}w`; - if (days > 0) return `${days}d`; - if (hours > 0) return `${hours}h`; - if (minutes > 0) return `${minutes}m`; - if (seconds === 0) return "Just now"; - return `${Math.max(seconds, 0)}s`; + if (months > 0) return translate("time.months_short", { count: months }); + if (weeks > 0) return translate("time.weeks_short", { count: weeks }); + if (days > 0) return translate("time.days_short", { count: days }); + if (hours > 0) return translate("time.hours_short", { count: hours }); + if (minutes > 0) return translate("time.minutes_short", { count: minutes }); + if (seconds === 0) return translate("time.just_now"); + return translate("time.seconds_short", { count: Math.max(seconds, 0) });utils/api/topics.ts (1)
20-24: Add rate limiting protection.Consider implementing rate limiting to prevent API abuse.
Implement a rate limiter utility:
class RateLimiter { private timestamps: number[] = []; constructor(private windowMs: number, private maxRequests: number) {} async acquireToken(): Promise<void> { const now = Date.now(); this.timestamps = this.timestamps.filter(t => now - t < this.windowMs); if (this.timestamps.length >= this.maxRequests) { const oldestTimestamp = this.timestamps[0]; const waitTime = this.windowMs - (now - oldestTimestamp); await new Promise(resolve => setTimeout(resolve, waitTime)); } this.timestamps.push(now); } } const rateLimiter = new RateLimiter(60000, 100); // 100 requests per minuteThen use it before each API call:
export async function getTopics(args: { account: string }) { const { account } = args; + await rateLimiter.acquireToken(); const { data } = await api.get(`/api/topics`, { headers: await getXmtpApiHeaders(account), });Also applies to: 34-44, 54-61, 69-76, 84-91, 99-106, 114-121, 129-135
features/conversation-list/hooks/use-delete-group.ts (1)
44-50: Enhance error handling in onError callback.The error recovery is good, but we should log the error for debugging purposes.
onError: (error, _, context) => { + logger.error("[useDeleteGroup] Failed to delete group", error); setConversationDataQueryData({ account: currentAccount, topic: groupTopic, data: { isDeleted: context?.previousIsDeleted }, }); },features/conversation-list/conversation-list-item/conversation-list-item-dm.tsx (1)
Line range hint
81-92: Consider memoizing callback functions.The
deleteDmandtoggleReadStatusAsynccallbacks should be memoized to prevent unnecessary re-renders.- const deleteDm = useDeleteDm(conversation); + const deleteDm = useMemo( + () => useDeleteDm(conversation), + [conversation] + ); const { toggleReadStatusAsync } = useToggleReadStatus({ topic, }); + const memoizedToggleReadStatus = useCallback( + () => toggleReadStatusAsync(), + [toggleReadStatusAsync] + ); return ( <ConversationListItemSwipeable renderRightActions={renderRightActions} renderLeftActions={renderLeftActions} onLeftSwipe={deleteDm} - onRightSwipe={toggleReadStatusAsync} + onRightSwipe={memoizedToggleReadStatus} >features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversations.tsx (1)
30-33: Consider adding a loading skeleton.The component returns null when loading, which might cause layout shifts. Consider showing the skeleton while loading.
if (isLoading) { - return null; + return <PinnedConversationsSkeleton />; }components/XmtpEngine.tsx (1)
Line range hint
106-112: Add timeout handling for sync operations.Long-running sync operations could block the UI or cause issues with app state transitions.
Consider adding a timeout mechanism:
showActionSheetWithOptions( { options: actions.map((a) => a.label), cancelButtonIndex: actions.length - 1, destructiveButtonIndex: [0, 1], title, ...actionSheetColors(colorScheme), + timeout: 30000, // 30 seconds timeout }, async (selectedIndex?: number) => { if (selectedIndex !== undefined) { + try { await Promise.race([ actions[selectedIndex].action(), + new Promise((_, reject) => + setTimeout(() => reject(new Error('Sync timeout')), 30000) + ), ]); + } catch (error) { + if (error.message === 'Sync timeout') { + logger.warn('[XmtpEngine] Sync operation timed out'); + } + throw error; + } } } );features/conversation-list/hooks/use-conversation-list-item-context-menu-default-props.tsx (1)
133-133: Consider adding error handling for toggleReadStatusAsync.Since
toggleReadStatusAsyncis an async operation, consider wrapping it with try-catch to handle potential errors.- onPress: toggleReadStatusAsync, + onPress: async () => { + try { + await toggleReadStatusAsync(); + } catch (error) { + captureErrorWithToast(error); + } + },utils/xmtpRN/sync.ts (1)
Line range hint
65-65: Consider improving error typing.The
error: anyparameter inonSyncLostfunction could also benefit from usingunknowntype for consistency.-export const onSyncLost = async (account: string, error: any) => { +export const onSyncLost = async (account: string, error: unknown) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
assets/icons/chat-bubble-dark.pngis excluded by!**/*.pngassets/icons/chat-bubble-light.pngis excluded by!**/*.png
📒 Files selected for processing (69)
components/AccountSettingsButton.tsx(2 hunks)components/ErroredHeader.tsx(0 hunks)components/Snackbar/SnackbarBackdrop/SnackbarBackdrop.tsx(3 hunks)components/XmtpEngine.tsx(1 hunks)data/store/accountsStore.ts(1 hunks)data/store/chatStore.ts(2 hunks)design-system/auto-image.tsx(1 hunks)features/blocked-chats/blocked-chats.tsx(1 hunks)features/blocked-chats/use-blocked-chats.ts(1 hunks)features/blocked-chats/useAllBlockedChats.ts(0 hunks)features/blocked-chats/useV3BlockedChats.ts(0 hunks)features/conversation-list/ConversationListNav.ios.deprecated.tsx(0 hunks)features/conversation-list/components/ChatNullState.tsx(0 hunks)features/conversation-list/components/HiddenRequestsButton.tsx(0 hunks)features/conversation-list/components/NewConversationButton.tsx(0 hunks)features/conversation-list/components/ProfileSettingsButton.tsx(0 hunks)features/conversation-list/components/conversation-list-item/V3GroupConversationListItem.tsx(0 hunks)features/conversation-list/conversation-list-empty.tsx(1 hunks)features/conversation-list/conversation-list-item/conversation-list-item-avatar-skeleton.tsx(2 hunks)features/conversation-list/conversation-list-item/conversation-list-item-dm.tsx(6 hunks)features/conversation-list/conversation-list-item/conversation-list-item-group.tsx(1 hunks)features/conversation-list/conversation-list-item/conversation-list-item-swipeable/conversation-list-item-swipeable-toggle-read-action.tsx(2 hunks)features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation-avatar.tsx(1 hunks)features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation-dm.tsx(4 hunks)features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation-group.tsx(3 hunks)features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation-message-preview.tsx(1 hunks)features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversations.tsx(3 hunks)features/conversation-list/conversation-list-requests-list-item.tsx(1 hunks)features/conversation-list/conversation-list.screen-header.tsx(1 hunks)features/conversation-list/conversation-list.screen.tsx(5 hunks)features/conversation-list/conversation-list.tsx(2 hunks)features/conversation-list/hooks/use-conversation-by-topic.ts(0 hunks)features/conversation-list/hooks/use-conversation-is-pinned.ts(1 hunks)features/conversation-list/hooks/use-conversation-is-unread.ts(1 hunks)features/conversation-list/hooks/use-conversation-list-item-context-menu-default-props.tsx(5 hunks)features/conversation-list/hooks/use-conversations-count.ts(1 hunks)features/conversation-list/hooks/use-delete-dm.ts(1 hunks)features/conversation-list/hooks/use-delete-group.ts(1 hunks)features/conversation-list/hooks/use-pin-or-unpin-conversation.tsx(1 hunks)features/conversation-list/hooks/use-pinned-conversations.tsx(1 hunks)features/conversation-list/hooks/use-toggle-read-status.ts(1 hunks)features/conversation-list/hooks/useHandleDeleteDm.ts(0 hunks)features/conversation-list/hooks/useHandleDeleteGroup.ts(0 hunks)features/conversation-list/hooks/useMessageIsUnread.ts(0 hunks)features/conversation-list/hooks/useMessagePlainText.ts(1 hunks)features/conversation-list/hooks/useShouldShowConnecting.tsx(3 hunks)features/conversation-list/hooks/useToggleReadStatus.ts(0 hunks)features/conversation-list/stores/conversation-list.store.tsx(0 hunks)features/conversation-list/useConversationListRequestCount.ts(0 hunks)features/conversation-list/useGroupConversationListAvatarInfo.ts(0 hunks)features/conversation-list/useV3RequestItemCount.ts(0 hunks)features/conversation/conversation-composer/conversation-composer-reply-preview.tsx(2 hunks)features/conversation/conversation.tsx(3 hunks)features/conversation/utils/conversation-is-unread-by-current-account.ts(1 hunks)features/notifications/utils/background/groupMessageNotification.ts(5 hunks)hooks/use-current-account-group-members-info.ts(1 hunks)hooks/useShouldShowErrored.ts(1 hunks)hooks/useTopicsData.ts(0 hunks)queries/QueryKeys.ts(2 hunks)queries/use-conversation-data-query.ts(1 hunks)queries/useConversationQuery.ts(1 hunks)screens/Accounts/Accounts.tsx(1 hunks)screens/Navigation/ConversationRequestsListNav.ios.tsx(1 hunks)theme/useAppTheme.ts(5 hunks)utils/api.ts(1 hunks)utils/api/topics.ts(1 hunks)utils/date.test.ts(2 hunks)utils/date.ts(2 hunks)utils/xmtpRN/sync.ts(2 hunks)
💤 Files with no reviewable changes (19)
- hooks/useTopicsData.ts
- features/conversation-list/useConversationListRequestCount.ts
- features/blocked-chats/useAllBlockedChats.ts
- features/conversation-list/components/HiddenRequestsButton.tsx
- components/ErroredHeader.tsx
- features/conversation-list/hooks/use-conversation-by-topic.ts
- features/conversation-list/components/NewConversationButton.tsx
- features/conversation-list/hooks/useToggleReadStatus.ts
- features/conversation-list/components/ProfileSettingsButton.tsx
- features/conversation-list/ConversationListNav.ios.deprecated.tsx
- features/conversation-list/hooks/useHandleDeleteDm.ts
- features/conversation-list/hooks/useHandleDeleteGroup.ts
- features/conversation-list/components/conversation-list-item/V3GroupConversationListItem.tsx
- features/conversation-list/useGroupConversationListAvatarInfo.ts
- features/conversation-list/useV3RequestItemCount.ts
- features/conversation-list/stores/conversation-list.store.tsx
- features/conversation-list/components/ChatNullState.tsx
- features/blocked-chats/useV3BlockedChats.ts
- features/conversation-list/hooks/useMessageIsUnread.ts
✅ Files skipped from review due to trivial changes (3)
- features/conversation-list/hooks/useMessagePlainText.ts
- screens/Navigation/ConversationRequestsListNav.ios.tsx
- features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation-avatar.tsx
🧰 Additional context used
🪛 GitHub Check: tsc
utils/date.test.ts
[failure] 1-1:
Module '"date-fns"' has no exported member 'enUS'.
[failure] 1-1:
Module '"date-fns"' has no exported member 'fr'.
🪛 GitHub Actions: Typescript CI
utils/date.test.ts
[error] 1-1: Module 'date-fns' has no exported member 'enUS'
🔇 Additional comments (34)
features/conversation-list/hooks/use-delete-dm.ts (3)
1-18: LGTM! Well-organized imports.The imports are logically grouped and all appear to be necessary for the hook's functionality.
32-59: Well-implemented mutation with optimistic updates!The implementation follows React Query best practices:
- Optimistic updates for better UX
- Proper error handling with state restoration
- Clean separation of concerns
23-23: Consider adding runtime validation for currentAccount.The non-null assertion (
!) onuseCurrentAccount()could lead to runtime errors if the account is not available. Consider adding a guard clause or error handling.data/store/chatStore.ts (1)
19-38: Implementation looks goodThe store is properly initialized with state variables and methods for managing
reconnectingstatus andgroupInviteLinks. The usage ofzustandwithpersistmiddleware is correctly implemented.features/conversation-list/hooks/use-conversation-is-unread.ts (1)
16-17: 🛠️ Refactor suggestionEnsure
currentAccountis defined before usingThe
currentAccountobtained fromuseCurrentAccount()could potentially beundefined. UsingcurrentAccount!asserts that it is defined, which might lead to runtime errors if it's not. Consider adding a check or enabling the query only whencurrentAccountis available.Apply this diff to add an
enabledflag to the queries:const { data: readUntil, isLoading } = useQuery({ ...getConversationDataQueryOptions({ account: currentAccount!, topic, }), + enabled: !!currentAccount, select: (data) => data?.readUntil, }); const { data: lastMessage } = useQuery({ ...getConversationQueryOptions({ account: currentAccount!, topic, }), + enabled: !!currentAccount, select: (data) => data?.lastMessage, });queries/use-conversation-data-query.ts (1)
105-111:⚠️ Potential issueHandle undefined conversation data
When accessing
conversationsData[arg.topic], there's a possibility that the topic might not exist inconversationsData. To prevent runtime errors, consider adding a check or a fallback.Apply this diff to include a fallback mechanism:
return groupArgs.map((arg) => ({ - ...conversationsData[arg.topic], + ...(conversationsData[arg.topic] || {}), account, topic: arg.topic, }));✅ Verification successful
Handle undefined conversation data
The review comment is correct. The
getTopicsfunction returns a validated record of topics, but accessingconversationsData[arg.topic]could still returnundefinedif the specific topic doesn't exist in the record. The suggested fallback mechanism using|| {}is appropriate and will prevent runtime errors.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find type definitions and usage of conversationsData ast-grep --pattern 'interface $_ { $$$ conversationsData: $_; $$$ }' # Look for type declarations ast-grep --pattern 'type $_ = { $$$ conversationsData: $_; $$$ }' # Find where conversationsData is initialized or validated rg "conversationsData\s*=" -A 2 # Look for similar spread patterns rg "\.\.\." --type typescriptLength of output: 451
Script:
#!/bin/bash # Find getTopics implementation and type ast-grep --pattern 'getTopics = async ($$$) => { $$$ }' ast-grep --pattern 'function getTopics($$$) { $$$ }' # Look for type definitions with proper extension rg --type ts "conversationsData" -B 2 -A 2 # Find null checks around conversationsData rg --type ts "if.*conversationsData" -B 2 -A 2 # Find the file containing the code under review for more context fd "use-conversation-data-query.ts" --exec cat {}Length of output: 8905
features/conversation-list/conversation-list-item/conversation-list-item-group.tsx (2)
34-35: Check handling of undefinedlastMessagetimestampWhen
group.lastMessageis undefined,timestampdefaults to0. Ensure thatgetCompactRelativeTimecan appropriately handle a timestamp of0without causing unintended results.
1-112: Well-structured component with effective performance optimizationsThe
ConversationListItemGroupcomponent is well-implemented, utilizinguseMemoanduseCallbackappropriately for performance optimization. The code is clean, readable, and follows best practices.utils/api.ts (1)
3-35: Imports updated appropriatelyThe added import statements correctly include the necessary types and functions for the new features introduced in the file. There are no issues with the imports.
hooks/useShouldShowErrored.ts (1)
5-5: Simplify error condition to focus on internet connectivityThe function now returns
!isInternetReachable, which simplifies the logic by removing the dependency on the chat error state. Ensure that this change aligns with the application's intended error handling, as it now only considers internet connectivity.features/blocked-chats/blocked-chats.tsx (1)
3-3: LGTM! Clean migration to the new hook.The changes successfully migrate from
useAllBlockedChatstouseBlockedChatswhile maintaining the same component behavior.Also applies to: 12-12
features/conversation-list/conversation-list-empty.tsx (1)
7-9: LGTM! Good use of memoization.The component is properly memoized to prevent unnecessary re-renders.
features/conversation-list/hooks/use-pinned-conversations.tsx (1)
16-23: Consider handling stale queries.When the conversations array changes, old queries might still be loading. Consider adding a way to cancel stale queries.
features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation-message-preview.tsx (1)
9-10: LGTM! Improved naming conventions.The renaming of types and components to include "Conversation" provides better context and clarity. The "I" prefix for interface types follows TypeScript conventions.
Also applies to: 13-15
hooks/use-current-account-group-members-info.ts (1)
24-30: Add error handling for failed queries.The hook should handle query errors gracefully and potentially provide error state to consumers.
Consider adding error handling:
- const { data: members } = useGroupMembersQuery({ + const { data: members, error } = useGroupMembersQuery({ account: currentAccount, topic: groupTopic, queryOptions: { enabled: Boolean(groupTopic), }, }); + + if (error) { + console.error('Failed to fetch group members:', error); + }screens/Accounts/Accounts.tsx (1)
Line range hint
1-67: Verify impact of removing error handling.The removal of error handling logic (
erroredAccounts) might impact user experience if errors are no longer visible to users.Let's check for any remaining error handling implementations:
✅ Verification successful
Error handling removal appears intentional and safe
The only error handling present in the codebase is for store hydration, which is unrelated to the UI-level error handling that was removed. The Accounts component's primary function is to display and manage account navigation, where basic error states might not be necessary for a clean user experience.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for error handling in related account components echo "Checking for error handling implementations..." # Search for error handling in account-related files rg -l 'error|Error' --type ts --type tsx | grep -i 'account' # Check for error-related components ast-grep --pattern 'function $name() { $$$ error $$$ }'Length of output: 255
Script:
#!/bin/bash # Search for error handling in account-related files echo "=== Searching for error handling in account files ===" rg -t typescript -t javascriptreact -t typescriptreact 'error|Error' --context 2 | grep -i -B2 -A2 'account' echo -e "\n=== Checking account store for error states ===" rg -t typescript -t typescriptreact 'error|Error' ./data/store/accountsStore.ts || true echo -e "\n=== Looking for error UI components ===" ast-grep --pattern 'const $name = $$$error$$$' ast-grep --pattern 'interface $name { $$$error$$$; }'Length of output: 695
Script:
#!/bin/bash echo "=== Checking Accounts component and related files ===" # Check the Accounts component rg 'error|Error' screens/Accounts/Accounts.tsx || true # Check the accounts store rg 'error|Error' data/store/accountsStore.ts || true # Check account-related components rg 'error|Error' 'components/Account.*' || true echo -e "\n=== Checking for error-related imports ===" rg 'import.*Error' screens/Accounts/Accounts.tsx || trueLength of output: 616
queries/QueryKeys.ts (1)
47-50: LGTM! Clean implementation of the new query key.The new query key follows the established pattern and maintains consistency with existing implementations.
design-system/auto-image.tsx (1)
5-14: LGTM! Well-documented props interface.The
AutoImagePropstype extendsIImagePropswith clear JSDoc comments for the optionalmaxWidthandmaxHeightproperties.utils/date.ts (1)
61-67: LGTM! Clear timestamp normalization logic.The function correctly handles both nanosecond and millisecond timestamps.
features/conversation-list/conversation-list.tsx (1)
68-70: LGTM! Clean component rendering logic.The conditional rendering based on conversation type is clear and maintainable.
utils/api/topics.ts (1)
6-15: Consider adding more fields to the TopicSchema.The schema might need additional fields for future extensibility.
Let's check if there are any additional fields used in the codebase:
features/conversation-list/hooks/use-delete-group.ts (1)
20-20: 🛠️ Refactor suggestionAdd null check for currentAccount.
The non-null assertion (
!) oncurrentAccountcould lead to runtime errors if the account is not available.Add a null check:
- const currentAccount = useCurrentAccount()!; + const currentAccount = useCurrentAccount(); + if (!currentAccount) { + throw new Error("Current account not found"); + }features/conversation-list/conversation-list-item/conversation-list-item-dm.tsx (1)
26-28: Good use of memo with named function.The component is properly memoized with a descriptive name, which helps with debugging in React DevTools.
features/conversation-list/hooks/use-conversation-list-item-context-menu-default-props.tsx (1)
101-106: LGTM! Proper error handling with async/await.The error handling has been improved with proper async/await syntax and error capture using toast notifications.
utils/xmtpRN/sync.ts (1)
53-55: LGTM! Improved type safety in error handling.The change from
anytounknowntype for error handling improves type safety.🧰 Tools
🪛 Biome (1.9.4)
[error] 54-54: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the catch clause.(lint/complexity/noUselessCatch)
components/AccountSettingsButton.tsx (1)
Line range hint
108-120: Review the condition for showing error icon.The condition
if (isInternetReachable)seems incorrect as it shows the error icon when internet is reachable. Should this beif (!isInternetReachable)instead?theme/useAppTheme.ts (2)
Line range hint
172-182: LGTM! Simplified theme determination logic.The changes streamline the theme handling by directly using
colorSchemeinstead of relying on navigation theme, which reduces complexity while maintaining functionality.
1-3: LGTM! Improved import organization.Moving animation and layout imports to the top improves code organization and readability.
features/notifications/utils/background/groupMessageNotification.ts (1)
1-1: LGTM! Consistent timestamp normalization.Updated to use
normalizeTimestampToMsfor consistent timestamp handling across the application.features/conversation/conversation-composer/conversation-composer-reply-preview.tsx (1)
21-21: LGTM! More descriptive hook name.Renamed
useMessageTexttouseMessagePlainTextfor better clarity about the hook's purpose.Also applies to: 236-236
data/store/accountsStore.ts (2)
6-7: LGTM! Improved import organization.Added explicit imports for
removeLogoutTaskandmmkvutilities.
Line range hint
147-153: Verify the impact of commented lastAsyncUpdate initialization.The initialization of
lastAsyncUpdateis commented out. This might affect how new accounts are handled.✅ Verification successful
Commented lastAsyncUpdate initialization appears intentional
The commented code would have prevented async migrations for new accounts by setting
lastAsyncUpdateto the latest version. With it commented out, new accounts will start with the default value of 0 from settingsStore, ensuring they go through the migration process. This appears to be an intentional change to standardize account initialization.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for lastAsyncUpdate usage to understand potential impact rg "lastAsyncUpdate" --type ts --type tsx -A 3 # Look for any async migration related code rg "asyncMigration|updateSteps" --type ts --type tsxLength of output: 157
Script:
#!/bin/bash # Search for lastAsyncUpdate in TypeScript files with more context rg "lastAsyncUpdate" -g "*.{ts,tsx}" -B 3 -A 3 # Search for variable declarations specifically ast-grep --pattern 'const lastAsyncUpdate = $_' # Search for type declarations or interfaces that might use it ast-grep --pattern 'interface $_ { lastAsyncUpdate: $_ }'Length of output: 1671
features/conversation/conversation.tsx (2)
5-6: LGTM! Import changes follow consistent naming conventions.The updated import paths follow the kebab-case naming convention, improving consistency across the codebase.
Also applies to: 53-53
226-232: LGTM! Clear and well-structured hook usage.The hook implementation follows good practices with clear naming and proper separation of concerns.
features/conversation/utils/conversation-is-unread-by-current-account.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (5)
features/conversation-requests-list/useV3RequestItems.tsx (1)
Line range hint
21-67: Add cleanup logic to prevent memory leaks.The useEffect contains async operations but lacks cleanup logic. This could lead to memory leaks if the component unmounts while operations are pending.
Consider this implementation:
useEffect(() => { + let mounted = true; const fetchGroups = async () => { const likelyNotSpam: ConversationWithCodecsType[] = []; const likelySpam: ConversationWithCodecsType[] = []; await Promise.allSettled( (data ?? []).map(async (conversation) => { // ... existing code ... }) ); + if (mounted) { setLikelyNotSpam(likelyNotSpam); setLikelySpam(likelySpam); + } }; fetchGroups(); + return () => { + mounted = false; + }; }, [data]);components/StateHandlers/HydrationStateHandler.tsx (1)
Remove unused import and reorganize import statements
The
getXmtpClientimport is unused in this component and should be removed. Consider reorganizing the remaining imports into the following groups:
- React imports
- Query imports (@/queries/*)
- Store imports (@data/store/*)
- Utility imports (@utils/*)
- Relative imports (../)
🔗 Analysis chain
Line range hint
1-10: Verify the necessity of imported utilities.Several utilities are imported but their usage patterns could be optimized:
getXmtpClientis imported but never used- Consider grouping related imports (e.g., queries, stores, utilities)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other usages of getXmtpClient in the codebase rg "getXmtpClient" --type ts --type tsxLength of output: 67
Script:
#!/bin/bash # Search for getXmtpClient usage and imports rg "getXmtpClient" -t ts -t typescript --context 2 # Check the full content of HydrationStateHandler to verify usage rg -t typescript -t ts "^export default function HydrationStateHandler" -A 50 # Look for other files with similar imports to understand grouping patterns rg -t typescript -t ts "^import.*from.*@data/store|^import.*from.*@utils|^import.*from.*@queries" --context 1Length of output: 347
Script:
#!/bin/bash # Search for getXmtpClient usage using ast-grep ast-grep --pattern 'getXmtpClient($$$)' # Look for the file content fd HydrationStateHandler.tsx --exec cat {} # Search for imports and usage without type flags rg "getXmtpClient"Length of output: 12398
features/conversation-requests-list/ConversationRequestsListNav.ios.tsx (2)
Line range hint
31-31: Improve type safety for navigation ref.The navigation ref is typed as 'any', which bypasses TypeScript's type checking benefits.
-const navRef = useRef<any>(); +const navRef = useRef<NativeStackNavigationProp<NavigationParamList, "ChatsRequests">>();Ensure safe cleanup of navigation listener.
The focus listener cleanup might be unsafe if the unsubscribe function is undefined.
useEffect(() => { const unsubscribe = navRef.current?.addListener("focus", () => { if (likelyNotSpam.length === 0 && likelySpam.length === 0) { navRef.current?.goBack(); } }); - return unsubscribe; + return () => unsubscribe?.(); }, [likelyNotSpam.length, likelySpam.length]);Also applies to: 108-116
Line range hint
143-150: Consider adding loading and error states.The component doesn't handle initial loading or error states from the
useRequestItemshook, which might lead to a poor user experience.return ( <NativeStack.Screen name="ChatsRequests" options={navigationOptions}> {(navigationProps) => { navRef.current = navigationProps.navigation; return ( <> <GestureHandlerRootView style={styles.root}> - <View style={styles.container}>{renderContent()}</View> + <View style={styles.container}> + {isLoading ? ( + <ActivityIndicator /> + ) : error ? ( + <Text style={styles.errorText}>{error.message}</Text> + ) : ( + renderContent() + )} + </View> </GestureHandlerRootView> </> ); }} </NativeStack.Screen> );utils/xmtpRN/messages.ts (1)
Potential message duplication after removing source filtering
The removal of message source filtering could lead to duplicate processing since:
- Text messages from the current user are meant to be handled locally via react-query
- No alternative message deduplication mechanism was found in the codebase
- The planned optimistic update implementations are still pending (marked as TODOs)
Consider either:
- Restoring the message source filtering
- Implementing the planned optimistic updates first
- Adding message deduplication logic elsewhere in the message processing pipeline
🔗 Analysis chain
Line range hint
48-57: Verify the removal of message source filtering.The removal of message source filtering (
isMessageFromOtherUserandisNonTextMessagechecks) means all messages will be processed uniformly. This could potentially lead to duplicate updates for text messages from the current user, as they were previously handled locally for optimistic updates.Let's verify if this change impacts message handling:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining optimistic update handling rg -l 'optimistic.*update' --type ts # Check for any message duplication prevention logic elsewhere ast-grep --pattern 'function $_(message) { $$$ if ($message.sender === $user) { $$$ } $$$ }'Length of output: 253
Script:
#!/bin/bash # Check content of files with optimistic updates rg -A 5 -B 5 'optimistic.*update' --type ts # Look for message deduplication logic with different patterns ast-grep --pattern 'if ($message.id === $id || $message.sender === $user) { $$$ }' # Search for any message uniqueness checks rg -A 3 'message.*unique|duplicate.*message|dedup.*message' --type tsLength of output: 3458
Script:
#!/bin/bash # Check for react-query message handling rg -A 5 "useQuery.*message" --type ts rg -A 5 "queryKey.*message" --type ts # Look for any message handling in mutation hooks ast-grep --pattern 'useMutation({ $$$ mutationFn: $sendMessage, $$$ })'Length of output: 2180
♻️ Duplicate comments (5)
features/conversation-list/hooks/use-conversation-is-pinned.ts (1)
11-16:⚠️ Potential issueHandle potential null account state.
The non-null assertion (
!) oncurrentAccountcould lead to runtime errors if no account is selected.Apply this fix:
- account: currentAccount!, + account: currentAccount ?? undefined,Additionally, consider adding early return or error handling:
const currentAccount = useCurrentAccount(); + +if (!currentAccount) { + return { isPinned: false }; +}utils/date.test.ts (1)
1-1:⚠️ Potential issueFix incorrect date-fns imports.
The locale imports should be from 'date-fns/locale' instead of 'date-fns'.
-import { format, enUS, fr } from "date-fns"; +import { format } from "date-fns"; +import { enUS, fr } from "date-fns/locale";🧰 Tools
🪛 GitHub Check: tsc
[failure] 1-1:
Module '"date-fns"' has no exported member 'enUS'.
[failure] 1-1:
Module '"date-fns"' has no exported member 'fr'.features/conversation-list/hooks/use-delete-dm.ts (1)
79-94:⚠️ Potential issueImprove error handling in delete+block action.
The delete+block action has potential issues:
- If blocking fails after deletion succeeds, the changes aren't reverted
- Possible race condition between delete and block operations
Consider this improved implementation:
{ label: translate("delete_and_block"), action: async () => { try { - await deleteDmAsync(); - await dm.updateConsent("denied"); const peerInboxId = await dm.peerInboxId(); + // Execute operations in parallel for better performance + await Promise.all([ + deleteDmAsync(), + dm.updateConsent("denied"), + consentToInboxIdsOnProtocolByAccount({ + account: currentAccount, + inboxIds: [peerInboxId], + consent: "deny", + }) + ]); - await consentToInboxIdsOnProtocolByAccount({ - account: currentAccount, - inboxIds: [peerInboxId], - consent: "deny", - }); } catch (error) { captureErrorWithToast(error); + // Consider adding logic to revert any partial changes } }, },features/conversation-list/hooks/use-toggle-read-status.ts (1)
17-56:⚠️ Potential issueAdd error handling and prevent race conditions.
Several improvements needed for robustness:
- Handle potential null return from getCurrentAccount()
- Add race condition prevention (as noted in previous review)
- Add retry configuration for network failures
export const useToggleReadStatus = ({ topic }: UseToggleReadStatusProps) => { + const [isPending, setIsPending] = useState(false); + const { mutateAsync: markAsReadAsync } = useMutation({ + retry: 3, mutationFn: async () => { const currentAccount = getCurrentAccount()!; + if (!currentAccount) { + throw new Error('No account found'); + } await markTopicAsRead({ account: currentAccount, topic, readUntil: new Date().getTime(), }); }, onMutate: () => { + if (isPending) return false; + setIsPending(true); const currentAccount = getCurrentAccount()!; + if (!currentAccount) return false; // ... rest of the code }, + onSettled: () => { + setIsPending(false); + }, });features/conversation/utils/conversation-is-unread-by-current-account.ts (1)
9-12:⚠️ Potential issueNormalize
readUntilto ensure accurate timestamp comparisonCurrently, only
lastMessageSentis normalized to milliseconds usingnormalizeTimestampToMs, butreadUntilis not. This could lead to incorrect comparisons ifreadUntilis in a different unit.Apply this fix:
const timestamp = normalizeTimestampToMs(lastMessageSent); +const readUntilNormalized = normalizeTimestampToMs(readUntil); -return readUntil < timestamp; +return readUntilNormalized < timestamp;
🧹 Nitpick comments (26)
features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx (1)
Line range hint
156-156: Consider component decomposition for better maintainability.The component currently handles multiple responsibilities (reactions, positioning, modal state, contexts). Consider:
- Addressing the TODO by making
ConversationMessagemore presentational- Splitting into smaller, focused components:
- MessageReactionHandler
- ContextMenuPositioner
- ModalContainer
This would improve maintainability and testability while reducing the complexity of state management.
Would you like me to propose a detailed component breakdown?
features/conversation-list/hooks/use-conversation-is-pinned.ts (2)
6-10: Add JSDoc documentation for better developer experience.Consider adding JSDoc documentation to describe the hook's purpose, parameters, and return value.
+/** + * Hook to check if a conversation is pinned for the current account. + * @param args.conversationTopic - The topic of the conversation to check + * @returns {Object} Object containing isPinned status + */ export function useConversationIsPinned(args: { conversationTopic: ConversationTopic; })
22-25: Add explicit return type for better type safety.Consider adding a return type to make the hook's interface more explicit.
-export function useConversationIsPinned(args: { +export function useConversationIsPinned(args: { conversationTopic: ConversationTopic; -}) { +}): { isPinned: boolean | undefined } {features/conversation-requests-list/useV3RequestItems.tsx (1)
Line range hint
21-67: Extract magic numbers and simplify error handling.The spam score threshold is hardcoded and the error handling could be simplified.
Consider these improvements:
+ const SPAM_SCORE_THRESHOLD = 1; + + const getMessageContent = (lastMessage: any) => { + try { + const decodedContent = lastMessage?.content() ?? lastMessage?.fallback ?? ""; + return typeof decodedContent === "string" ? decodedContent : (lastMessage?.fallback ?? ""); + } catch { + return lastMessage?.fallback ?? ""; + } + }; useEffect(() => { const fetchGroups = async () => { const likelyNotSpam: ConversationWithCodecsType[] = []; const likelySpam: ConversationWithCodecsType[] = []; await Promise.allSettled( (data ?? []).map(async (conversation) => { if (conversation.state !== "unknown") { return; } - const lastMessage = conversation.lastMessage; - let content = ""; - try { - const decodedContent = lastMessage?.content() ?? lastMessage?.fallback ?? ""; - if (typeof decodedContent === "string") { - content = decodedContent; - } else { - content = lastMessage?.fallback ?? ""; - } - } catch { - content = lastMessage?.fallback ?? ""; - } + const content = getMessageContent(conversation.lastMessage); const contentType = getMessageContentType(lastMessage?.contentTypeId!); let spamScore = 0; try { if (contentType) { spamScore = await getV3SpamScore({ message: content, contentType, }); } } catch (error) { logger.error("Couldn't get spam score", error); } - if (spamScore > 1) { + if (spamScore > SPAM_SCORE_THRESHOLD) { likelySpam.push(conversation); } else { likelyNotSpam.push(conversation); } }) ); setLikelyNotSpam(likelyNotSpam); setLikelySpam(likelySpam); }; fetchGroups(); }, [data]);utils/api/topics.ts (2)
71-71: Consider batch operations for better performance.The API endpoints accept arrays but are only used with single items. Consider adding batch operations to reduce API calls when processing multiple topics.
Example usage:
export async function markTopicsAsRead(args: { account: string; topics: ConversationTopic[]; readUntil: number; }) { // ... similar implementation }Also applies to: 96-96, 121-121, 143-143, 165-165, 187-187
21-195: Add rate limiting protection.The API functions lack rate limiting protection which could lead to API throttling under heavy usage.
Consider implementing a rate limiter:
import rateLimit from 'some-rate-limit-library'; const limiter = rateLimit({ windowMs: 60 * 1000, // 1 minute max: 100 // limit each account to 100 requests per minute }); // Usage in each function: await limiter.consume(account); // This will throw if rate limit is exceededqueries/queryClient.ts (1)
9-10: Consider adding the time values to the comments.While the renamed constants are more descriptive, it would be helpful to include the actual time values in the comments for quick reference.
features/conversation-list/hooks/use-delete-dm.ts (1)
38-62: Enhance mutation error handling and reduce duplication.Consider these improvements:
- Extract the hardcoded context string to a constant
- Add specific error type handling beyond the generic catch-all
+const MUTATION_CONTEXT = "useDeleteDm" as const; + const { mutateAsync: deleteDmAsync } = useMutation({ mutationFn: () => deleteTopic({ account: currentAccount, topic: dm.topic, }), onMutate: () => { const previousIsDeleted = getConversationDataQueryData({ account: currentAccount, topic: dm.topic, - context: "useDeleteDm", + context: MUTATION_CONTEXT, })?.isDeleted; setConversationDataQueryData({ account: currentAccount, topic: dm.topic, - context: "useDeleteDm", + context: MUTATION_CONTEXT, data: { isDeleted: true }, }); return { previousIsDeleted }; }, - onError: (error, _, context) => { + onError: (error: unknown, _, context) => { + if (error instanceof NetworkError) { + // Handle network-specific errors + captureErrorWithToast(error, "Network error while deleting DM"); + } setConversationDataQueryData({ account: currentAccount, topic: dm.topic, - context: "useDeleteDm", + context: MUTATION_CONTEXT, data: { isDeleted: context?.previousIsDeleted }, }); }, });components/StateHandlers/HydrationStateHandler.tsx (1)
Line range hint
11-73: Consider architectural improvements for better maintainability and error handling.While the current implementation works, there are several opportunities for improvement:
- The useEffect contains complex async logic without cleanup handling, which could lead to memory leaks if the component unmounts during hydration.
- Error handling could be enhanced to provide better feedback for hydration failures.
- The comment suggests potential optimization for selected account only, which could improve performance.
Consider these improvements:
export default function HydrationStateHandler() { useEffect(() => { let mounted = true; const hydrate = async () => { try { const startTime = new Date().getTime(); const accounts = getAccountsList(); // Handle wallet initialization if (accounts.length === 0) { await getInstalledWallets(false); } else { getInstalledWallets(false); } if (!mounted) return; // Fetch conversations logger.debug("[Hydration] Fetching persisted conversation list for all accounts"); const results = await Promise.allSettled( accounts.map((account) => hydrateAccount(account)) ); if (!mounted) return; // Handle results const errors = results.filter((result) => result.status === "rejected"); if (errors.length > 0) { logger.error("[Hydration] Failed to hydrate some accounts:", errors); } useAppStore.getState().setHydrationDone(true); logger.debug(`[Hydration] Completed in ${(new Date().getTime() - startTime) / 1000}s`); } catch (error) { logger.error("[Hydration] Critical error during hydration:", error); // Consider setting an error state or retrying } }; hydrate(); return () => { mounted = false }; }, []); return null; } // Separate concerns into helper functions async function hydrateAccount(account: string) { const startTime = new Date().getTime(); logger.debug(`[Hydration] Starting for account ${account}`); await fetchPersistedConversationListQuery({ account }); await prefetchInboxIdQuery({ account }); logger.debug( `[Hydration] Completed for ${account} in ${(new Date().getTime() - startTime) / 1000}s` ); }features/conversation-requests-list/ConversationRequestsListNav.ios.tsx (1)
Line range hint
37-38: Remove unused variables from destructuring.The
isRefetchingandrefetchvariables are destructured but never used.- const { likelySpam, likelyNotSpam, isRefetching, refetch } = + const { likelySpam, likelyNotSpam } = useRequestItems();utils/xmtpRN/messages.ts (2)
73-73: Maintain consistency in context propagation.While
updateConversationQueryDataincludes the context parameter,updateConversationInConversationListQuerydoes not. For consistent debugging and tracing, consider adding the context parameter to both functions.updateConversationInConversationListQuery({ account: client.address, topic: message.topic as ConversationTopic, + context: "streamAllMessages", conversationUpdate: { lastMessage: message, }, });Also applies to: 82-91
Line range hint
126-176: Reduce code duplication in group metadata updates.The current implementation repeats the same pattern for each metadata field (name, photo, description). Consider refactoring this into a more maintainable structure.
Here's a suggested refactor:
type MetadataField = { fieldName: string; queryField: string; }; const METADATA_FIELDS: MetadataField[] = [ { fieldName: 'group_name', queryField: 'name' }, { fieldName: 'group_image_url_square', queryField: 'imageUrlSquare' }, { fieldName: 'description', queryField: 'description' } ]; // In handleGroupUpdatedMessage if (content.metadataFieldsChanged.length > 0) { for (const field of content.metadataFieldsChanged) { const metadataField = METADATA_FIELDS.find(f => f.fieldName === field.fieldName); if (metadataField && field.newValue) { const update = { [metadataField.queryField]: field.newValue }; updateConversationQueryData({ account, topic, context: "handleGroupUpdatedMessage", conversationUpdate: update, }); updateConversationInConversationListQuery({ account, topic, context: "handleGroupUpdatedMessage", conversationUpdate: update, }); } } }features/conversation-list/hooks/use-toggle-read-status.ts (1)
124-125: Consider exposing additional state information.The hook could be more useful by providing loading state, current read status, and error state to consumers.
- return { toggleReadStatusAsync }; + return { + toggleReadStatusAsync, + isLoading, + isUnread: conversationData?.markedAsUnread ?? convoIsUnreadByTimestamp, + error + };features/conversation/conversation.utils.ts (1)
13-13: Fix the typo in the commentThe word "Wether" should be "Whether".
Apply this diff to correct the typo:
-// Wether a conversation is blocked +// Whether a conversation is blockedqueries/useGroupQuery.ts (2)
Line range hint
1-4: Consider consolidating with conversation query as suggested in comment.Similar to useDmQuery, this file suggests consolidating with useConversationQuery. This would reduce code duplication and simplify the codebase.
71-74: Add context parameter to getGroupQueryData call.For consistency with other query functions, consider adding the context parameter here:
const previousGroup = getGroupQueryData({ account, topic, + context: "updateGroupQueryData", });queries/useDmConsentMutation.ts (1)
Line range hint
82-89: Consider adding context to setDmQueryData in error handling.For consistency, consider adding context to setDmQueryData calls in the error handling path:
setDmQueryData({ account: currentAccount, peer: topic, dm: updatedDm, + context: "useDmConsentMutation.onError" });queries/useConversationQuery.ts (1)
77-83: Consider enhancing error handling in getOrFetchConversation.While the new utility function is useful, it could benefit from explicit error handling.
Consider adding try-catch and proper error propagation:
export function getOrFetchConversation(args: IArgs) { const conversation = getConversationQueryData(args); if (conversation) { return Promise.resolve(conversation); } - return queryClient.fetchQuery(getConversationQueryOptions(args)); + return queryClient.fetchQuery(getConversationQueryOptions(args)) + .catch(error => { + logger.error( + `[getOrFetchConversation] Failed to fetch conversation for account ${args.account}`, + error + ); + throw error; + }); }features/conversation-list/conversation-list-awaiting-requests.tsx (1)
26-26: Consider i18n for user-facing stringsThe strings "Requests" and "awaiting your response" should be internationalized for better maintainability and localization support.
Also applies to: 22-23
features/conversation-list/hooks/use-conversation-is-unread.ts (2)
48-51: Remove or implement commented codeThe commented code about
currentUserInboxIdshould either be implemented or removed. If it's a UX consideration, it should be documented why it was decided against.
37-62: Optimize memo dependenciesThe
useMemodependencies array includes derived values that could be optimized.Consider this optimization:
}, [ - lastMessage, + lastMessage?.sentNs, conversationData?.readUntil, conversationData?.markedAsUnread, ]);features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversations.tsx (3)
22-27: Consider consolidating loading statesThe component manages two separate loading states. Consider combining them into a single loading state for simpler logic.
- const { pinnedConversations, isLoading } = usePinnedConversations(); - const { - count: conversationsCount, - isLoading: conversationsCountIsLoading, - } = useConversationsCount(); + const { pinnedConversations, isLoading: isPinnedLoading } = usePinnedConversations(); + const { count: conversationsCount, isLoading: isCountLoading } = useConversationsCount(); + const isLoading = isPinnedLoading || isCountLoading;
38-41: Document the "onboarded" conditionThe comment about user being "onboarded" needs more context. Consider adding a detailed comment explaining the business logic behind this condition.
- // Don't show anything because user has "onboarded" and don't need placeholders anymore + /** + * Hide pinned conversations section when: + * 1. User has at least one conversation (onboarded) + * 2. No conversations are pinned + * This prevents showing empty space for users who don't use pinning feature. + */
71-82: Improve skeleton component maintainabilityConsider these improvements:
- Move colors to theme constants
- Abstract repeated skeleton items into a reusable component
+ const SKELETON_COLORS = { + red: { color: "#FF8080", opacity: 0.15 }, + yellow: { color: "#FFE580", opacity: 0.3 }, + blue: { color: "#80D9FF", opacity: 0.15 }, + }; + const SkeletonItem = memo(({ color, opacity, size }) => ( + <ConversationListItemAvatarSkeleton + color={hexToRGBA(color, opacity)} + size={size} + /> + )); return ( <AnimatedHStack style={themed($container)}> - <ConversationListItemAvatarSkeleton - color={hexToRGBA("#FF8080", 0.15)} - size={avatarSize} - /> + {Object.entries(SKELETON_COLORS).map(([key, { color, opacity }]) => ( + <SkeletonItem + key={key} + color={color} + opacity={opacity} + size={avatarSize} + /> + ))} </AnimatedHStack> );features/conversation-list/conversation-list.screen.tsx (2)
9-39: Consider organizing imports by categoryThe imports could be better organized into categories (components, hooks, utils, types) for improved readability.
+ // Core React & React Native import React, { memo, useCallback, useMemo } from "react"; import { TouchableOpacity, useColorScheme } from "react-native"; + // Navigation & External Libraries import { NativeStackScreenProps } from "@react-navigation/native-stack"; import { ContextMenuView } from "react-native-ios-context-menu"; + // Components import { Screen } from "@/components/Screen/ScreenComp/Screen"; import { ConversationList } from "@/features/conversation-list/conversation-list"; + // Hooks & Utils import { useCurrentAccount, useSettingsStore } from "@/data/store/accountsStore"; import { useSelect } from "@/data/store/storeHelpers";
Line range hint
46-189: Enhance error handling feedbackConsider providing user feedback when the refresh operation fails.
const handleRefresh = useCallback(async () => { try { await refetchConversations(); } catch (error) { captureError(error); + // Show toast or error message to user + showErrorToast(translate("errors.refresh_failed")); } }, [refetchConversations]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
components/StateHandlers/HydrationStateHandler.tsx(1 hunks)features/blocked-chats/use-blocked-chats.ts(1 hunks)features/conversation-list/conversation-list-awaiting-requests.tsx(1 hunks)features/conversation-list/conversation-list-item/conversation-list-item-swipeable/conversation-list-item-swipeable-delete-action.tsx(1 hunks)features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation-dm.tsx(4 hunks)features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation-group.tsx(3 hunks)features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation.tsx(1 hunks)features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversations.tsx(4 hunks)features/conversation-list/conversation-list.screen.tsx(5 hunks)features/conversation-list/hooks/use-conversation-is-pinned.ts(1 hunks)features/conversation-list/hooks/use-conversation-is-unread.ts(1 hunks)features/conversation-list/hooks/use-conversations-count.ts(1 hunks)features/conversation-list/hooks/use-delete-dm.ts(1 hunks)features/conversation-list/hooks/use-delete-group.ts(1 hunks)features/conversation-list/hooks/use-pin-or-unpin-conversation.tsx(1 hunks)features/conversation-list/hooks/use-pinned-conversations.tsx(1 hunks)features/conversation-list/hooks/use-toggle-read-status.ts(1 hunks)features/conversation-list/hooks/useMessagePlainText.ts(1 hunks)features/conversation-requests-list/ConversationRequestsListNav.ios.tsx(2 hunks)features/conversation-requests-list/useV3RequestItems.tsx(1 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx(2 hunks)features/conversation/conversation.tsx(4 hunks)features/conversation/conversation.utils.ts(1 hunks)features/conversation/utils/conversation-is-unread-by-current-account.ts(1 hunks)queries/queryClient.constants.ts(1 hunks)queries/queryClient.ts(1 hunks)queries/use-conversation-data-query.ts(1 hunks)queries/useConversationListQuery.ts(11 hunks)queries/useConversationMessages.ts(1 hunks)queries/useConversationPreviewMessages.ts(2 hunks)queries/useConversationQuery.ts(4 hunks)queries/useDmConsentMutation.ts(1 hunks)queries/useDmPeerInbox.ts(1 hunks)queries/useDmQuery.ts(2 hunks)queries/useGroupQuery.ts(5 hunks)screens/ConversationReadOnly.tsx(1 hunks)screens/Navigation/Navigation.tsx(1 hunks)theme/animations.ts(1 hunks)utils/api/topics.ts(1 hunks)utils/date.test.ts(2 hunks)utils/mmkv.ts(2 hunks)utils/xmtpRN/messages.ts(5 hunks)
✅ Files skipped from review due to trivial changes (3)
- theme/animations.ts
- queries/queryClient.constants.ts
- screens/Navigation/Navigation.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- features/conversation-list/hooks/useMessagePlainText.ts
- features/conversation-list/hooks/use-delete-group.ts
- features/conversation-list/hooks/use-pin-or-unpin-conversation.tsx
- features/blocked-chats/use-blocked-chats.ts
- features/conversation-list/hooks/use-pinned-conversations.tsx
- features/conversation/conversation.tsx
- features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation-dm.tsx
- features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation-group.tsx
🧰 Additional context used
📓 Learnings (1)
queries/useConversationListQuery.ts (1)
Learnt from: alexrisch
PR: ephemeraHQ/converse-app#1313
File: queries/useV3ConversationListQuery.ts:196-268
Timestamp: 2024-12-05T18:33:24.472Z
Learning: In the TypeScript file `queries/useV3ConversationListQuery.ts`, helper functions like `updateGroupImageToConversationListQuery`, `updateGroupNameToConversationListQuery`, and `updateGroupDescriptionToConversationListQuery` are used to reduce dependency on exact types or creating objects that get passed unnecessarily, even though they internally use `updateGroupMetadataToConversationListQuery`.
🪛 GitHub Check: tsc
utils/date.test.ts
[failure] 1-1:
Module '"date-fns"' has no exported member 'enUS'.
[failure] 1-1:
Module '"date-fns"' has no exported member 'fr'.
🔇 Additional comments (44)
features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx (2)
25-26: LGTM! Clean import organization.The imports are now better organized with React Native core imports grouped together.
57-57: LGTM! Improved code organization.The added spacing improves readability by clearly separating variable declarations from hook usage, following common React component organization patterns.
features/conversation-list/hooks/use-conversation-is-pinned.ts (2)
1-5: LGTM! Imports are clean and well-organized.All necessary dependencies are properly imported with no redundancy.
13-20: LGTM! Query setup follows React Query best practices.Good use of:
- Query options spreading
- Data selection
- Context string for debugging
utils/date.test.ts (1)
156-190: Well-structured and comprehensive test coverage!The test cases effectively cover:
- All time units (seconds to years)
- Edge cases (falsy input, future dates)
- Proper formatting of the output
features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation.tsx (2)
14-20: LGTM! Improved type naming convention.The renamed interface follows a more standardized naming pattern and maintains clear, well-typed props definition.
22-24: Verify component renaming across the codebase.The component renaming follows a consistent naming convention and maintains clean implementation. Let's ensure all imports are updated accordingly.
✅ Verification successful
Component renaming is complete and consistent
All references to the renamed component have been properly updated throughout the codebase. The remaining "PinnedConversation" prefixed components (Avatar, MessagePreview) are separate, correctly named components that are part of the same feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old component name rg "PinnedConversation[^List]" --type tsx # Search for imports of the new component name to verify correct usage rg "ConversationListPinnedConversation" --type tsxLength of output: 150
Script:
#!/bin/bash # Search for any remaining references to the old component name rg "PinnedConversation[^List]" -g "*.{ts,tsx}" # Search for imports and usage of the new component name rg "ConversationListPinnedConversation" -g "*.{ts,tsx}" -A 2Length of output: 16891
features/conversation-requests-list/useV3RequestItems.tsx (2)
1-7: LGTM! Imports are well-organized and complete.The imports cover all necessary dependencies and follow a logical organization pattern.
Line range hint
9-19: Verify impact of removed query options.The removal of
queryOptionsfromuseConversationListQuerymight affect data freshness sincerefetchOnWindowFocusandrefetchOnMountare no longer specified. Please ensure this aligns with the intended behavior.Let's verify other usages of this query to ensure consistency:
features/conversation-list/conversation-list-item/conversation-list-item-swipeable/conversation-list-item-swipeable-delete-action.tsx (2)
Line range hint
6-19: Clean and well-structured implementation!The component follows React best practices with proper memoization, TypeScript types, and clean props passing.
15-15: Verify contrast ratio for accessibility.The change to
theme.colors.global.whiteon a caution background color should be verified for sufficient contrast ratio to ensure accessibility compliance.Let's check for similar color usage patterns in swipeable actions:
✅ Verification successful
White icon color is appropriate for delete action
The use of
theme.colors.global.whitefor the delete icon provides good contrast against the caution background color, following common patterns for destructive actions. The different styling from other swipeable actions (like toggle read) is intentional to distinguish the destructive nature of the delete action.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other swipeable actions using white color rg -l "colors\.global\.white.*SwipeableAction" # Search for theme color usage in swipeable components ast-grep --pattern 'SwipeableAction.*iconColor={$_}'Length of output: 103
Script:
#!/bin/bash # Find all swipeable-related files fd swipeable # Look for color usage in these files rg "theme\.colors" --type typescript --type typescriptreact -A 2 # Find SwipeableAction components and their props ast-grep --pattern 'const $_ = memo(({ $$ }: $_) => { $$$ })'Length of output: 938
Script:
#!/bin/bash # Check contents of swipeable action files cat features/conversation-list/conversation-list-item/conversation-list-item-swipeable/conversation-list-item-swipeable-delete-action.tsx cat features/conversation-list/conversation-list-item/conversation-list-item-swipeable/conversation-list-item-swipeable-toggle-read-action.tsx # Look for color usage with correct file type rg "theme\.colors" -t tsx -A 2 # Find specific icon color props ast-grep --pattern 'iconColor={theme.colors.$_}'Length of output: 1925
utils/api/topics.ts (2)
25-28: Add retry logic for API calls.API calls should include retry logic for transient failures.
Also applies to: 46-51, 69-73, 94-98, 119-123, 141-145, 163-167, 185-188
17-19: Plan migration away from deprecated status field.The
statusfield is marked as deprecated. Consider creating a migration plan to:
- Update consumers to use the new boolean fields
- Remove the deprecated field in a future version
Let's check for usage of the deprecated field:
✅ Verification successful
Safe to remove the deprecated status field
The field is not actively used in the codebase. You can safely remove it from the
TopicSchemawithout any migration needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of the deprecated status field rg -A 3 'status.*(?:unread|read|deleted)' --type tsLength of output: 270
Script:
#!/bin/bash # Search for any references to topic.status or status property access rg -g '*.ts' -g '*.tsx' '\.status|status:' # Search for TypeScript interfaces or types that might reference the status field ast-grep --pattern 'interface $_ { $$$ status$$$ $$$ }' # Search for any TODO or migration comments related to status rg -g '*.ts' -g '*.tsx' '@deprecated.*status|TODO.*status'Length of output: 11077
queries/queryClient.ts (1)
8-8: LGTM! Well-documented query configuration changes.The added options effectively prevent unnecessary refetches with clear documentation explaining each setting's purpose. This should help optimize performance by reducing unnecessary network calls.
Also applies to: 11-11, 12-12, 13-13
utils/mmkv.ts (2)
8-8: LGTM! Consistent constant renaming.The changes maintain consistency with the QueryClient configuration updates while preserving the existing security measures and memory leak prevention.
Also applies to: 74-74
74-74: Verify the maxAge configuration across the codebase.Let's ensure the DEFAULT_GC_TIME value is appropriate for persisted queries and consistent with other persistence configurations in the codebase.
✅ Verification successful
The maxAge configuration using DEFAULT_GC_TIME (24 hours) is appropriate and consistent
The DEFAULT_GC_TIME value of 24 hours is well-aligned with the codebase's caching strategy:
- It matches the default query client configuration
- Queries requiring different retention periods (like profile socials) explicitly override it
- It provides a reasonable duration for persisted query cache maintenance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other persistence configurations that might need alignment rg -l "maxAge|gcTime|staleTime" --type tsLength of output: 257
Script:
#!/bin/bash # Find DEFAULT_GC_TIME definition rg "DEFAULT_GC_TIME.*=" --type ts -A 1 # Get context around maxAge/gcTime/staleTime configurations echo "=== maxAge configurations ===" rg "maxAge.*=" --type ts -B 1 -A 1 echo "=== gcTime configurations ===" rg "gcTime.*=" --type ts -B 1 -A 1 echo "=== staleTime configurations ===" rg "staleTime.*=" --type ts -B 1 -A 1Length of output: 498
Script:
#!/bin/bash # Search with more flexible patterns including object property definitions echo "=== maxAge configurations ===" rg "maxAge.*[=:]" --type ts -B 2 -A 2 echo "=== gcTime configurations ===" rg "gcTime.*[=:]" --type ts -B 2 -A 2 echo "=== staleTime configurations ===" rg "staleTime.*[=:]" --type ts -B 2 -A 2 # Also search for these terms within object definitions echo "=== Configurations within objects ===" ast-grep --pattern 'const $_ = { $$$ maxAge: $_, $$$ }' ast-grep --pattern '{ $$$ staleTime: $_, $$$ }'Length of output: 4262
features/conversation-list/hooks/use-delete-dm.ts (2)
23-23: Add runtime validation for currentAccount.The non-null assertion (
!) assumes currentAccount will always exist. Consider adding a runtime check to handle the case when it doesn't.
30-30: Add null check for peerInboxId.The
peerInboxIdfromuseDmPeerInboxIdmight be undefined when passed tousePreferredInboxName.components/StateHandlers/HydrationStateHandler.tsx (1)
27-29: LGTM! Debug message accurately reflects the behavior.The updated log message better describes the actual behavior of fetching conversation lists for all accounts.
features/conversation-requests-list/ConversationRequestsListNav.ios.tsx (2)
1-2: LGTM! Import paths have been reorganized logically.The import path changes reflect a better organization of the codebase, with feature-specific components properly grouped and local imports made relative.
Also applies to: 20-20, 26-26
Line range hint
41-71: Function name doesn't match its behavior.The
clearAllSpamfunction operates onlikelyNotSpamconversations, but its name suggests it handles spam messages. This could lead to confusion.Consider renaming to better reflect its purpose:
-const clearAllSpam = useCallback(() => { +const clearAllRequests = useCallback(() => {Add error handling for the consent operation.
The consent operation lacks error handling, which could leave the UI in an inconsistent state if the operation fails.
Add try-catch block:
[options.clearAll]: async () => { setClearingAll(true); + try { // @todo => handle groups here const peers = Array.from( new Set( likelyNotSpam.map((c) => "addedByInboxId" in c ? c.addedByInboxId : undefined ) ) ).filter((peer) => !!peer) as string[]; await consentToAddressesOnProtocolByAccount({ account, addresses: peers, consent: "deny", }); + } catch (error) { + // Handle error appropriately + console.error('Failed to clear requests:', error); + } finally { setClearingAll(false); + } navRef.current?.goBack(); },Address the TODO comment for group handling.
The TODO comment indicates missing functionality for handling groups.
Let me check the codebase for group-related implementations:
utils/xmtpRN/messages.ts (1)
Line range hint
29-35: LGTM: Robust error handling and secure logging.The implementation demonstrates good practices:
- Consistent error handling with
captureError- Secure logging with production data redaction
features/conversation-list/hooks/use-toggle-read-status.ts (4)
1-15: LGTM! Well-organized imports and clean type definition.The imports are properly organized and the type definition is focused and clear.
96-122: 🛠️ Refactor suggestionAdd error handling and loading state management.
The toggle callback needs error handling and should manage loading state to prevent race conditions.
+ const [isLoading, setIsLoading] = useState(false); + const toggleReadStatusAsync = useCallback(async () => { + if (isLoading) return; + setIsLoading(true); + try { const currentAccount = getCurrentAccount()!; + if (!currentAccount) { + throw new Error('No account found'); + } // ... rest of the code if (isUnread) { await markAsReadAsync(); } else { await markAsUnreadAsync(); } + } catch (error) { + console.error('Failed to toggle read status:', error); + throw error; + } finally { + setIsLoading(false); + } - }, [markAsReadAsync, markAsUnreadAsync, topic]); + }, [markAsReadAsync, markAsUnreadAsync, topic, isLoading]);Likely invalid or redundant comment.
58-94: 🛠️ Refactor suggestionReduce duplication and apply same safeguards to unread mutation.
The unread mutation needs the same improvements as the read mutation. Consider extracting common mutation configuration to reduce duplication.
+const createMutation = (operation: 'read' | 'unread') => ({ + retry: 3, + onMutate: (isPending: boolean, setIsPending: (value: boolean) => void) => { + if (isPending) return false; + setIsPending(true); + const currentAccount = getCurrentAccount()!; + if (!currentAccount) return false; + // ... rest of the shared logic + }, + onSettled: (setIsPending: (value: boolean) => void) => { + setIsPending(false); + }, +}); const { mutateAsync: markAsUnreadAsync } = useMutation({ + ...createMutation('unread'), mutationFn: async () => { const currentAccount = getCurrentAccount()!; + if (!currentAccount) { + throw new Error('No account found'); + } await markTopicAsUnread({ account: currentAccount, topic, }); }, // ... rest using shared configuration });Likely invalid or redundant comment.
1-125: Verify query key consistency and cleanup.Ensure that the query keys used in
getConversationDataQueryDataandsetConversationDataQueryDataare consistent across the application and that the mutations are properly cleaned up on unmount.features/conversation/conversation.utils.ts (1)
11-11: Update ofgetConversationQueryDatacall is appropriateAdding the
contextparameter to thegetConversationQueryDatacall aligns with the updated function signature and enhances context tracking.queries/useDmPeerInbox.ts (1)
19-19: Addition ofcontextparameter enhances traceabilityIncluding
context: "useDmPeerInboxId"in theuseConversationQuerycall provides better context for debugging and aligns with the pattern used elsewhere in the codebase.queries/useDmQuery.ts (2)
31-31: LGTM: Context parameters improve traceability.Adding context parameters to setConversationQueryData calls enhances debugging and tracing capabilities by identifying the source of data updates.
Also applies to: 55-55
Line range hint
1-3: Consider consolidating with conversation query as suggested in TODO.The TODO comment raises a valid architectural point. Having separate DM and conversation queries might lead to code duplication. Consider consolidating this into the conversation query with a "peer" argument for better maintainability.
Let's analyze the potential impact of this consolidation:
✅ Verification successful
TODO comment's suggestion to consolidate with conversation query is valid and recommended
The DM query is already tightly integrated with the conversation query system and maintains conversation data internally. Consolidating them would:
- Reduce code duplication
- Provide DM operations with additional conversation query features
- Simplify the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check usage of DM-specific vs conversation queries echo "DM Query Usage:" rg -l "useDmQuery|setDmQueryData|getDmQueryData" echo "\nConversation Query Usage:" rg -l "useConversationQuery|setConversationQueryData|getConversationQueryData"Length of output: 879
Script:
#!/bin/bash # Check the implementation details echo "DM Query Implementation:" rg -A 10 "export function useDmQuery" queries/useDmQuery.ts echo "\nConversation Query Implementation:" rg -A 10 "export function useConversationQuery" queries/useConversationQuery.tsLength of output: 568
Script:
#!/bin/bash # Check conversation query and getDm implementation echo "Conversation Query Structure:" ast-grep --pattern 'export function useConversationQuery($_) { $$$ }' queries/useConversationQuery.ts echo "\ngetDm Implementation:" ast-grep --pattern 'export async function getDm($_) { $$$ }' queries/useDmQuery.tsLength of output: 318
Script:
#!/bin/bash # Get full content of both files echo "DM Query File Content:" cat queries/useDmQuery.ts echo -e "\nConversation Query File Content:" cat queries/useConversationQuery.tsLength of output: 4245
queries/useConversationPreviewMessages.ts (1)
46-49: LGTM: Improved query configuration.The changes enhance the query setup by combining conversation messages options with cache-only options, making the configuration more modular and reusable.
queries/useDmConsentMutation.ts (1)
53-53: LGTM: Added context parameter for better traceability.The addition of the context parameter is consistent with the changes in other files.
screens/ConversationReadOnly.tsx (1)
34-34: LGTM! Context parameter addition is appropriate.The added context parameter "conversation-read-only" provides clear traceability for the query's origin.
queries/useConversationListQuery.ts (3)
23-25: LGTM! Logging enhancement improves traceability.The added logging statement provides useful debug information about the observer creation.
33-36: API simplification looks good.Removing the queryOptions parameter simplifies the API while maintaining functionality through the config function.
Line range hint
226-239: Function rename improves clarity.Renaming to
getConversationListQueryConfigbetter reflects the function's purpose as a getter.queries/useConversationMessages.ts (1)
126-135: LGTM! Function visibility and context changes are appropriate.Making getConversationMessagesQueryOptions public improves reusability, and the added context parameter enhances traceability.
queries/useConversationQuery.ts (2)
13-13: LGTM! Context parameter addition is consistent.Adding context to IArgs type maintains consistency with the broader refactoring effort.
25-28: API simplification looks good.Removing queryOptions and using spread operator with getConversationQueryOptions makes the API cleaner.
features/conversation-list/conversation-list-awaiting-requests.tsx (1)
37-38: Consider implementing the TODO comment using SkiaThe TODO comment suggests using Skia for better performance. This could improve the rendering quality and performance of the icon.
Let me check if Skia is already a dependency:
Would you like me to help implement the Skia-based icon with the shield overlay?
features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversations.tsx (3)
3-8: Well-structured modular architecture!The shift from store-based state management to custom hooks (
usePinnedConversations,useConversationsCount) improves modularity and separation of concerns.
103-106: Clean conditional rendering implementation!The component effectively handles different conversation types with clear separation between group and DM conversations.
48-58: Add null check for pinnedConversationsThe map operation could fail if
pinnedConversationsis null. Consider adding a null check or providing a default empty array.- {pinnedConversations?.map((conversation) => ( + {(pinnedConversations ?? []).map((conversation) => (features/conversation-list/conversation-list.screen.tsx (1)
Line range hint
137-150: Clarify or remove incomplete States componentThe component currently returns
nullin all cases and contains TODO comments. Either implement the intended functionality or remove the component if it's no longer needed.Would you like me to help implement the loading states or create an issue to track this task?
utils/api/topics.ts
Outdated
| const parseResult = z.record(TopicSchema).safeParse(data); | ||
| if (!parseResult.success) { | ||
| logger.error("[API TOPICS] getTopics parse error:", parseResult.error); | ||
| } | ||
| return data as Record<string, z.infer<typeof TopicSchema>>; | ||
| } |
There was a problem hiding this comment.
Improve error handling in API functions.
The current error handling only logs parse errors but continues execution with potentially invalid data. This could lead to runtime errors downstream.
Consider this pattern instead:
function handleParseError(result: z.SafeParseError<any>, operation: string): never {
logger.error(`[API TOPICS] ${operation} parse error:`, result.error);
throw new Error(`Invalid response from ${operation}`);
}
// Usage in each function:
const parseResult = TopicSchema.safeParse(data);
if (!parseResult.success) {
handleParseError(parseResult, "getTopic");
}
return parseResult.data;Also applies to: 52-57, 75-83, 100-108, 125-130, 147-152, 169-174, 190-195
| account, | ||
| topic, | ||
| queryOptions: cacheOnlyQueryOptions, | ||
| getConversationMessagesQueryOptions; |
There was a problem hiding this comment.
Remove unnecessary standalone reference.
This line appears to be an accidental standalone reference to getConversationMessagesQueryOptions without any effect.
- getConversationMessagesQueryOptions;
| group: GroupWithCodecsType; | ||
| }; | ||
|
|
||
| export const ConversationListItemGroup = memo( |
There was a problem hiding this comment.
GroupConversationListItem & DmConversationListItem make more sense to me
There was a problem hiding this comment.
same feedback on names here while we're renaming stuff
I would name these kinds of things group-conversation and dm-conversation because doing it the other way (conversation-group) makes it seem like this is supposed to be a group of something.
Super nit but just my 2 ¢
| queryClient.setQueryData<IConversationDataQueryData>( | ||
| getConversationDataQueryOptions(args).queryKey, | ||
| (previousData) => { | ||
| if (!previousData) return data as IConversationDataQueryData; |
There was a problem hiding this comment.
using queryOptions from above will make it so we don't need manually types!
| return batchedGetConversationTopicData.fetch(args); | ||
| } | ||
|
|
||
| const batchedGetConversationTopicData = create({ |
There was a problem hiding this comment.
Can you just help me wrap my head around this pattern?
useQuery -> getOptions -> use batcher to batch where react query will trigger multiple parallel requests in a short time window (50ms?) -> return results -> hydrate cache -> use staleTime and gcTime from react-query to control how long those results are valid for?
| const currentAccount = useCurrentAccount(); | ||
|
|
||
| const { data: isPinned } = useQuery({ | ||
| ...getConversationDataQueryOptions({ |
There was a problem hiding this comment.
tiny nicer syntax here could be asking for select in getQueryOptions and defaulting to returning data: any so that we could just say useQuery(getQueryOptions({selector: data => data.property, account, topic, context})) rather than needing to destructure AND create the args
| }) | ||
| ); | ||
|
|
||
| const { data: lastMessage, isLoading: isLoadingLastMessage } = useQuery({ |
There was a problem hiding this comment.
Just a rq question from me: if we already have conversationData, when will lastMessage be different than conversationData.lastMessage? In other words why have two useQuery here?
| ]); | ||
| onPress: async () => { | ||
| try { | ||
| await pinOrUnpinConversationAsync(); |
There was a problem hiding this comment.
Can we optimisitcally pin or unpin?
| @@ -158,9 +146,9 @@ function useConversationContextMenuDeleteItem(args: { | |||
| const handleDeleteFn = conversation | |||
| ? isConversationGroup(conversation) | |||
There was a problem hiding this comment.
why is delete different for group than dm?
queries/useConversationListQuery.ts
Outdated
| @@ -207,7 +223,7 @@ const getConversationList = async (args: { | |||
| } | |||
| }; | |||
|
|
|||
| export const conversationListQueryConfig = (args: { | |||
| export const getConversationListQueryConfig = (args: { | |||
There was a problem hiding this comment.
I think if we use the select pattern I mentioned earlier here, we could use a hook rather than a queryObserver for conversation list length - looking forward to chatting more shortly.
| const [count, setCount] = useState(0); | ||
| const [isLoading, setIsLoading] = useState(true); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Let's come back to this!
| const [isLoading, setIsLoading] = useState(true); | ||
|
|
||
| useEffect(() => { | ||
| const unsubscribe = createConversationListQueryObserver({ |
There was a problem hiding this comment.
Let's come back to this #1514 (comment)
| }, | ||
| }); | ||
|
|
||
| return useCallback(() => { |
There was a problem hiding this comment.
I think this should be up to the caller:
Something like
useDeleteDm({onDeleteTriggered: () => void}) and then a useDeleteDmWithActionSheet but if this is the only usage than that's okay. It's just weird having these ui actions in the hook
| name: string; | ||
| }; | ||
|
|
||
| export function useCurrentAccountGroupMembersInfo(args: { |
There was a problem hiding this comment.
I'd name this useGroupMembersInfoForCurrentAccount
There was a problem hiding this comment.
do we want to start moving new files to features/conversation/something?
|
|
||
| export type ConversationQueryData = Awaited<ReturnType<typeof getConversation>>; | ||
|
|
||
| type IArgs = { | ||
| account: string; | ||
| topic: ConversationTopic; | ||
| context: string; |
There was a problem hiding this comment.
I think context is too overloaded a term to be used just for logging. Other options could be
source
location
caller
etc
| @@ -0,0 +1,195 @@ | |||
| import type { ConversationTopic } from "@xmtp/react-native-sdk"; | |||
| import { z } from "zod"; | |||
There was a problem hiding this comment.
zod is amazing especially for apis - nice stuff here
a couple nits: can we extract the types instead of return z.infer into something like IMessageResponse = z.infer (or whatever the syntax is)
what kind of error handling do we want on improper validation?
utils/api.ts
Outdated
| @@ -38,7 +41,10 @@ export const SnackbarBackdrop = memo(() => { | |||
| > | |||
| <LinearGradient | |||
| locations={[0, 0.55]} | |||
| colors={["rgba(255, 255,255,0.0)", "rgba(255, 255,255,1)"]} | |||
| colors={[ | |||
| hexToRGBA(theme.colors.background.surface, 0), | |||
There was a problem hiding this comment.
This will fix the dark mode backsplash yes?
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (2)
features/GroupInvites/joinGroup/JoinGroup.client.ts (1)
Line range hint
267-267: Address type inconsistencies in test fixtures.Several type-related issues need attention:
- Inconsistent return types between fixtures (GroupsDataEntity vs ConversationDataEntity)
- Multiple @ts-expect-error annotations suggesting type mismatches
- Unresolved TODOs about type fixtures
Consider creating proper type fixtures to resolve these issues:
// Create a helper to generate properly typed fixtures const createConversationFixture = (id: ConversationId): Conversation => ({ id, createdAt: new Date().getTime(), members: async () => [], topic: `topic_${id}` as ConversationTopic, isGroupActive: true, state: "allowed", creatorInboxId: async () => "0xabc" as InboxId, name: "Group Name", addedByInboxId: "0x123" as InboxId, imageUrlSquare: "https://www.google.com", description: "Group Description", });Also applies to: 313-313, 385-385, 386-386, 387-387, 388-388, 389-389, 390-390, 391-391, 392-392, 393-393, 394-394, 395-395, 396-396, 397-397
utils/xmtpRN/messages.ts (1)
Line range hint
76-81: Enhance error handling in message streaming.The error handling simply captures errors without any retry mechanism or user feedback for failed message updates.
Consider implementing a retry mechanism for transient failures:
async function withRetry<T>(operation: () => Promise<T>, maxRetries = 3): Promise<T> { for (let i = 0; i < maxRetries; i++) { try { return await operation(); } catch (error) { if (i === maxRetries - 1) throw error; await new Promise(resolve => setTimeout(resolve, Math.pow(2, i) * 1000)); } } throw new Error('Max retries reached'); }
♻️ Duplicate comments (4)
features/blocked-chats/use-blocked-chats.ts (1)
11-12:⚠️ Potential issueValidate currentAccount before using non-null assertion.
The non-null assertion operator needs validation to prevent runtime errors.
+ if (!currentAccount) { + throw new Error("No current account found"); + } const { data } = useConversationsQuery({ - account: currentAccount!, + account: currentAccount, context: "useBlockedChats", });Also applies to: 18-19
features/conversation-list/hooks/use-conversation-is-unread.ts (1)
22-23:⚠️ Potential issueAdd error handling for undefined account.
The hook uses non-null assertions (
!) oncurrentAccount, which could lead to runtime errors if the account is undefined.Apply this fix:
-account: currentAccount!, +if (!currentAccount) { + return { isUnread: false, isLoading: false }; +} +account: currentAccount,Also applies to: 30-31
features/conversation-list/conversation-list-awaiting-requests.tsx (1)
62-63:⚠️ Potential issueAdd error handling for undefined account.
The
useRequestsCounthook uses a non-null assertion onaccount, which could lead to runtime errors.Apply this fix:
-account: account!, +if (!account) { + return { count: 0, isLoading: false }; +} +account,utils/api/topics.ts (1)
33-37:⚠️ Potential issueImprove error handling in parse results.
The current error handling only logs parse errors but continues execution with potentially invalid data.
Implement proper error handling:
function handleParseError(result: z.SafeParseError<any>, operation: string): never { logger.error(`[API TOPICS] ${operation} parse error:`, result.error); throw new Error(`Invalid response from ${operation}`); }
🧹 Nitpick comments (34)
.cursorrules (2)
8-8: Fix typo in variable naming guideline.There's a typo in the word "mater" which should be "matter".
-Doesn't mater if it's long. For example this is good: "useGroupMembersInfoForCurrentAccount". +Doesn't matter if it's long. For example this is good: "useGroupMembersInfoForCurrentAccount".
13-14: Remove empty bullet point.The empty bullet point on line 14 should be removed as it serves no purpose.
- Comments should be above the code they are describing. - + Comments should be above the code they are describing.queries/queryClient.ts (2)
6-12: Good addition of centralized error handling!The QueryCache error handler provides valuable context by including the query key in error messages. This will help with debugging and monitoring.
Consider adding the query status to the error context for even better debugging:
queryCache: new QueryCache({ onError: (error, query) => { captureError(error, { - message: `Error in query: ${query.queryKey.join(", ")}`, + message: `Error in query: ${query.queryKey.join(", ")} (status: ${query.state.status})`, }); }, }),
15-26: Well-documented query configuration to prevent infinite loops!The configuration effectively prevents various scenarios that could cause infinite refetch loops, with clear comments explaining each setting's purpose.
Consider adding a code comment referencing the documentation or internal discussion that led to these decisions, to help future maintainers understand the rationale behind disabling these features:
defaultOptions: { queries: { + // See discussion: <link-to-discussion> + // These settings are disabled to prevent unnecessary refetches + // and to allow manual control over data freshness // libXmtp handles retries retry: false,features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversations.tsx (3)
30-35: Consider showing skeleton during loading statesCurrently, the component returns
nullduring loading, which might cause layout shifts. Consider showing the skeleton component during loading states for a smoother user experience.- if (isLoadingPinnedConversations || conversationsCountIsLoading) { - return null; - } - - if (conversationsCount === 0) { + if (isLoadingPinnedConversations || conversationsCountIsLoading || conversationsCount === 0) { return <PinnedConversationsSkeleton />; }
73-84: Consider moving skeleton colors to theme constantsThe hardcoded hex colors with opacity could be moved to theme constants for better maintainability and consistency.
+ // In your theme file + export const skeletonColors = { + primary: "#FF8080", + secondary: "#FFE580", + tertiary: "#80D9FF", + opacity: { + low: 0.15, + medium: 0.3, + }, + }; // In this file <ConversationListItemAvatarSkeleton - color={hexToRGBA("#FF8080", 0.15)} + color={hexToRGBA(theme.skeletonColors.primary, theme.skeletonColors.opacity.low)} size={avatarSize} />
Line range hint
95-102: Enhance error message with more contextConsider adding more context to the error message to help with debugging.
captureError( new Error( - `Couldn't find conversation ${topic} in PinnedConversationWrapper` + `Failed to load pinned conversation. Topic: ${topic}, Component: PinnedConversationWrapper, Available: ${!!conversation}` ) );features/conversation-list/hooks/useShouldShowConnectingOrSyncing.tsx (1)
Line range hint
16-39: Consider enhancing the error tracking implementation.The current implementation has a few areas for improvement:
- The 15-second threshold is hardcoded
- The error message lacks context about why the load is taking long
- The interval continues to run every second even though we only need it at the 15-second mark
Consider these improvements:
+ const LOAD_TIMEOUT_MS = 15000; + const LOAD_CHECK_INTERVAL_MS = 15000; useEffect(() => { let interval: NodeJS.Timeout | undefined = undefined; if (!initialLoadDoneOnce) { if (conditionTrueTime.current === 0) { conditionTrueTime.current = Date.now(); } interval = setInterval(() => { - if (Date.now() - conditionTrueTime.current >= 15000) { + const loadDuration = Date.now() - conditionTrueTime.current; + if (loadDuration >= LOAD_TIMEOUT_MS) { sentryTrackMessage( - "Initial load has been running for 15 seconds" + `Initial load timeout after ${loadDuration}ms. Account: ${currentAccount?.id}` ); conditionTrueTime.current = 0; clearInterval(interval); } - }, 1000); + }, LOAD_CHECK_INTERVAL_MS);features/conversation-list/hooks/use-conversation-by-topic.ts (1)
9-12: Consider adding proper null checks for account.The non-null assertion (
!) onaccountcould lead to runtime errors if the account is null. Consider adding proper null checks or error handling.- const { data: conversations } = useConversationsQuery({ - account: account!, + if (!account) { + return undefined; + } + const { data: conversations } = useConversationsQuery({ + account, context: "useConversationListConversation", });features/GroupInvites/joinGroup/JoinGroup.client.ts (1)
Line range hint
1-24: Great architectural patterns, with room for minor improvements.The code demonstrates excellent use of:
- Static factory methods for test scenarios
- Dependency injection for better testability
- Comprehensive documentation
- Dynamic imports for test isolation
Consider these improvements:
- Add error handling for the dynamic import in
liveFetchGroupsByAccount- Consider adding retry logic for failed imports
- Add logging for failed operations
Example improvement:
const liveFetchGroupsByAccount = async ( account: string ): Promise<ConversationDataEntity> => { + try { const { fetchConversationsQuery } = await import( "@/queries/conversations-query" ); const conversationList: IConversationsQuery = await fetchConversationsQuery({ account }); return entify(conversationList, (conversation) => conversation.id); + } catch (error) { + logger.error('[liveFetchGroupsByAccount] Failed to fetch groups', { error, account }); + throw error; + } };Also applies to: 93-190
data/helpers/conversations/spamScore.ts (3)
8-8: Fix typo in type name and consider additional spam indicators.While the parameter rename to
messageTextis more descriptive, there's a typo in the type name (V3SpameScoreParamsshould beV3SpamScoreParams).Additionally, consider expanding the type to include more spam indicators such as sender reputation, message frequency, or time patterns.
-type V3SpameScoreParams = { +type V3SpamScoreParams = { messageText: string; contentType: IConvosContentType; + // Consider adding: + senderReputation?: number; + messageFrequency?: number; + timePattern?: string; };
Line range hint
16-17: Implement missing spam detection features.The TODOs indicate two critical missing features:
- Approved sender verification
- Sender-receiver relationship scoring
These features would significantly improve spam detection accuracy.
Would you like me to help create GitHub issues for tracking these features? I can provide detailed implementation suggestions for both features.
Line range hint
1-40: Consider implementing a more robust spam detection system.The current implementation could benefit from a more comprehensive approach:
- Move spam detection logic to a dedicated service
- Implement rate limiting and request tracking
- Consider using existing spam detection services or libraries
- Add telemetry for monitoring false positives/negatives
This would improve maintainability and accuracy while making it easier to evolve the spam detection system over time.
features/conversation-requests-list/conversation-requests-list.screen-header.tsx (1)
5-15: LGTM! Consider adding error handling for navigation.The hook implementation is clean and follows good practices. However, consider wrapping the navigation action in a try-catch block to handle potential navigation errors gracefully.
export function useConversationRequestsListScreenHeader() { const router = useRouter(); useHeader({ safeAreaEdges: ["top"], onBack: () => { + try { router.goBack(); + } catch (error) { + logger.error('[Navigation] Failed to go back:', error); + } }, title: translate("message_requests"), }); }features/conversation-requests-list/conversation-requests-list.nav.tsx (1)
11-12: Consider aligning screen name with route path for consistency.The screen name "ChatsRequests" differs from the path pattern "conversation-requests". Consider using a consistent naming convention.
<NativeStack.Screen - name="ChatsRequests" + name="ConversationRequests" component={ConversationRequestsListScreen} />features/conversation-requests-list/conversation-requests-list-segment-controller.tsx (2)
22-32: Extract styles to a separate StyleSheet.Consider moving inline styles to a StyleSheet object for better maintainability and performance.
+ const styles = StyleSheet.create({ + container: { + flexDirection: "row", + borderRadius: 8, + padding: 2, + height: 32, + marginTop: 10, + marginBottom: 2, + marginHorizontal: 16, + }, + }); <View - style={{ - flexDirection: "row", - backgroundColor: tertiaryBackgroundColor(colorScheme), - borderRadius: 8, - padding: 2, - height: 32, - marginTop: 10, - marginBottom: 2, - marginHorizontal: 16, - }} + style={[styles.container, { backgroundColor: tertiaryBackgroundColor(colorScheme) }]} >
64-67: Remove redundant color definition.The color property is redundantly set for selected items with the same value.
index === selectedIndex && { fontWeight: "500", - color: textPrimaryColor(colorScheme), },features/conversation-requests-list/conversation-requests-list.screen.tsx (1)
38-42: Consider adding a TypeScript interface for props.For better type safety and documentation, consider extracting the props interface:
+interface ConversationListWrapperProps { + selectedSegment: number; +} + -const ConversationListWrapper = memo(function ConversationListWrapper({ +const ConversationListWrapper = memo(function ConversationListWrapper({ selectedSegment, -}: { - selectedSegment: number; -}) { +}: ConversationListWrapperProps) {features/conversation/hooks/use-mark-conversation-as-unread.ts (1)
28-28: Extract the context string into a constant.The string "useToggleReadStatus" is repeated multiple times. Consider extracting it into a constant to avoid typos and make maintenance easier.
+const TOGGLE_READ_STATUS_CONTEXT = "useToggleReadStatus"; + export function useMarkConversationAsUnread(args: { topic: ConversationTopic; }) { // ... rest of the code const previousData = getConversationDataQueryData({ account: currentAccount, topic, - context: "useToggleReadStatus", + context: TOGGLE_READ_STATUS_CONTEXT, });Also applies to: 34-34, 47-47
queries/useGroupNameMutation.ts (1)
Line range hint
26-28: Make the error message more specific.The current error message could be more helpful by specifying which required data is missing.
-throw new Error("Missing required data in useGroupNameMutation"); +throw new Error( + `Missing required data in useGroupNameMutation: ${!group ? 'group' : ''} ${!account ? 'account' : ''} ${!topic ? 'topic' : ''}` +);features/conversation/hooks/use-mark-conversation-as-read.ts (1)
19-19: Cache the timestamp value.The current timestamp is calculated twice. Consider caching it to ensure consistency.
mutationFn: async () => { const currentAccount = getCurrentAccount()!; + const timestamp = new Date().getTime(); await markTopicAsRead({ account: currentAccount, topic, - readUntil: new Date().getTime(), + readUntil: timestamp, }); }, // ... updateData: { - readUntil: new Date().getTime(), + readUntil: timestamp, markedAsUnread: false, },Also applies to: 35-35
hooks/use-group-members-info-for-current-account.ts (2)
24-32: Consider adding error handling for the currentAccount assertion.The non-null assertion (
!) oncurrentAccountcould lead to runtime errors if the account is not available.Consider this safer approach:
- const currentAccount = useCurrentAccount()!; + const currentAccount = useCurrentAccount(); + + if (!currentAccount) { + return { groupMembersInfo: [] }; + }
41-56: Consider adding loading state to the return value.The hook could benefit from exposing loading state to help components handle data fetching states appropriately.
Consider adding:
return { groupMembersInfo, + isLoading: socialsData.some(query => query.isLoading), + error: socialsData.find(query => query.error)?.error };queries/useBlockGroupMutation.ts (1)
Line range hint
21-23: Consider adding validation for empty topic.The function checks for empty topic in
mutationFnbut not during initialization.Consider adding early validation:
export const useBlockGroupMutation = ( account: string, topic: ConversationTopic ) => { + if (!topic || !account) { + throw new Error("Missing required parameters: account and topic must be defined"); + }features/conversation/hooks/use-dm-consent.tsx (1)
21-24: Consider adding input validation for required parameters.The function could benefit from early validation of required parameters.
Consider adding:
export function useDmConsent(args: { peerInboxId: InboxId; conversationId: ConversationId; topic: ConversationTopic; }) { const { peerInboxId, conversationId, topic } = args; + + if (!peerInboxId || !conversationId || !topic) { + throw new Error("Missing required parameters"); + }features/conversation/utils/is-conversation-consent-unknown.ts (1)
3-7: Consider enhancing type safety with literal types.The function could benefit from using a literal type for the conversation state to prevent potential errors.
+type ConversationState = "unknown" | "denied" | "allowed"; export function isConversationConsentUnknown( conversation: ConversationWithCodecsType ) { - return conversation.state === "unknown"; + return conversation.state === ("unknown" as ConversationState); }features/blocked-chats/use-blocked-chats.ts (3)
15-23: Consider memoizing query configuration.The queries array is recreated on every render. Consider memoizing it to optimize performance.
+ const queryConfigs = useMemo( + () => + (data ?? []).map((conversation) => + getConversationDataQueryOptions({ + account: currentAccount, + topic: conversation.topic, + context: "useBlockedChats", + }) + ), + [data, currentAccount] + ); const conversationsDataQueries = useQueries({ - queries: (data ?? []).map((conversation) => - getConversationDataQueryOptions({ - account: currentAccount!, - topic: conversation.topic, - context: "useBlockedChats", - }) - ), + queries: queryConfigs, });
28-36: Extract filter conditions into named constants for clarity.The filter conditions could be more explicit to improve readability.
return data.filter((conversation, index) => { const query = conversationsDataQueries[index]; + const isDeleted = query?.data?.isDeleted; + const isDenied = conversation.state === "denied"; return ( - // Include deleted conversations - query?.data?.isDeleted || - // Include denied conversations - conversation.state === "denied" + isDeleted || isDenied ); });
7-40: Consider splitting the hook into smaller, focused hooks.The hook handles multiple responsibilities: fetching conversations, managing queries, and filtering data. Consider splitting it into smaller, more focused hooks for better maintainability.
Example structure:
const useConversationsData = (account: string) => { // Handle basic conversation fetching }; const useConversationQueries = (conversations: Conversation[]) => { // Handle conversation queries }; const useBlockedConversationsFilter = (conversations: Conversation[], queries: Query[]) => { // Handle filtering logic }; export const useBlockedChatsForCurrentAccount = () => { // Compose the above hooks };features/conversation-list/hooks/use-conversation-is-unread.ts (2)
28-35: Consider consolidating queries to improve performance.The hook makes two separate queries to fetch conversation data and last message, which could be consolidated since
conversationDataalready containslastMessage.Consider removing the second query and using
conversationData.lastMessageinstead.
47-51: Clean up commented code.The commented code about sender inbox ID check should either be implemented or removed to maintain code cleanliness.
Either implement the inbox ID check or remove the commented code block.
features/conversation-list/conversation-list-awaiting-requests.tsx (1)
21-28: Enhance accessibility for the requests list item.The
ConversationListItemshould have proper accessibility labels to indicate the number of pending requests.Add accessibility props:
<ConversationListItem title="Requests" + accessibilityLabel={`${count} chat requests awaiting your response`} + accessibilityRole="button" onPress={() => { navigation.navigate("ChatsRequests"); }} subtitle={`${count} awaiting your response`}utils/xmtpRN/messages.ts (1)
Line range hint
47-53: Clean up commented code.The commented code about message handling conditions should be removed or implemented.
Either implement the message handling logic or remove the commented code block.
utils/api/topics.ts (1)
7-11: Extract zod schema types.Instead of using z.infer directly in the code, extract the types for better maintainability.
Extract types:
const MessageResponseSchema = z.object({ message: z.string(), }); export type IMessageResponse = z.infer<typeof MessageResponseSchema>; export type ITopic = z.infer<typeof TopicSchema>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
queries/__snapshots__/QueryKeys.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (58)
.cursorrules(1 hunks)components/StateHandlers/HydrationStateHandler.tsx(2 hunks)components/swipeable.tsx(0 hunks)data/helpers/conversations/spamScore.ts(2 hunks)design-system/auto-sized-image.tsx(1 hunks)features/GroupInvites/joinGroup/JoinGroup.client.ts(2 hunks)features/blocked-chats/blocked-chats.tsx(1 hunks)features/blocked-chats/use-blocked-chats.ts(1 hunks)features/conversation-list/conversation-list-awaiting-requests.tsx(1 hunks)features/conversation-list/conversation-list-item/conversation-list-item-group.tsx(1 hunks)features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversations.tsx(4 hunks)features/conversation-list/conversation-list.screen.tsx(5 hunks)features/conversation-list/hooks/use-conversation-by-topic.ts(1 hunks)features/conversation-list/hooks/use-conversation-is-pinned.ts(1 hunks)features/conversation-list/hooks/use-conversation-is-unread.ts(1 hunks)features/conversation-list/hooks/use-conversations-count.ts(1 hunks)features/conversation-list/hooks/use-delete-dm.ts(1 hunks)features/conversation-list/hooks/use-delete-group.ts(1 hunks)features/conversation-list/hooks/use-pin-or-unpin-conversation.tsx(1 hunks)features/conversation-list/hooks/use-pinned-conversations.tsx(1 hunks)features/conversation-list/hooks/use-toggle-read-status.ts(1 hunks)features/conversation-list/hooks/useShouldShowConnectingOrSyncing.tsx(1 hunks)features/conversation-requests-list/ConversationRequestsListNav.ios.tsx(1 hunks)features/conversation-requests-list/ConversationRequestsListNav.tsx(1 hunks)features/conversation-requests-list/conversation-requests-list-segment-controller.tsx(1 hunks)features/conversation-requests-list/conversation-requests-list.nav.tsx(1 hunks)features/conversation-requests-list/conversation-requests-list.screen-header.tsx(1 hunks)features/conversation-requests-list/conversation-requests-list.screen.tsx(1 hunks)features/conversation-requests-list/useRequestItems.tsx(0 hunks)features/conversation-requests-list/useV3RequestItems.tsx(0 hunks)features/conversation/conversation-consent-popup/conversation-consent-popup-dm.tsx(2 hunks)features/conversation/conversation-new-dm.tsx(2 hunks)features/conversation/conversation.tsx(4 hunks)features/conversation/hooks/use-dm-consent.tsx(5 hunks)features/conversation/hooks/use-mark-conversation-as-read.ts(1 hunks)features/conversation/hooks/use-mark-conversation-as-unread.ts(1 hunks)features/conversation/utils/is-conversation-consent-unknown.ts(1 hunks)features/notifications/utils/accountTopicSubscription.ts(2 hunks)hooks/use-group-members-info-for-current-account.ts(1 hunks)package.json(1 hunks)queries/QueryKeys.test.ts(2 hunks)queries/conversation-data-query.ts(1 hunks)queries/conversations-query.ts(1 hunks)queries/queryClient.ts(1 hunks)queries/unknown-consent-conversations-query.ts(1 hunks)queries/useAllowGroupMutation.ts(3 hunks)queries/useBlockGroupMutation.ts(3 hunks)queries/useConversationListQuery.ts(0 hunks)queries/useGroupDescriptionMutation.ts(3 hunks)queries/useGroupNameMutation.ts(3 hunks)queries/useGroupPhotoMutation.ts(3 hunks)screens/Navigation/Navigation.tsx(1 hunks)utils/api/topics.ts(1 hunks)utils/date.test.ts(2 hunks)utils/date.ts(2 hunks)utils/xmtpRN/conversations.ts(2 hunks)utils/xmtpRN/messages.ts(6 hunks)utils/xmtpRN/sync.ts(2 hunks)
💤 Files with no reviewable changes (4)
- components/swipeable.tsx
- features/conversation-requests-list/useV3RequestItems.tsx
- features/conversation-requests-list/useRequestItems.tsx
- queries/useConversationListQuery.ts
✅ Files skipped from review due to trivial changes (4)
- features/conversation-requests-list/ConversationRequestsListNav.tsx
- features/conversation-requests-list/ConversationRequestsListNav.ios.tsx
- package.json
- queries/useGroupDescriptionMutation.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- features/conversation-list/hooks/use-conversations-count.ts
- features/conversation-list/hooks/use-conversation-is-pinned.ts
- components/StateHandlers/HydrationStateHandler.tsx
- features/conversation-list/hooks/use-pin-or-unpin-conversation.tsx
- features/conversation-list/hooks/use-pinned-conversations.tsx
- screens/Navigation/Navigation.tsx
- features/conversation-list/hooks/use-delete-dm.ts
- utils/xmtpRN/sync.ts
- features/conversation-list/hooks/use-toggle-read-status.ts
- features/blocked-chats/blocked-chats.tsx
- utils/date.ts
- features/conversation-list/hooks/use-delete-group.ts
- features/conversation/conversation.tsx
🧰 Additional context used
📓 Learnings (3)
queries/useAllowGroupMutation.ts (1)
Learnt from: alexrisch
PR: ephemeraHQ/converse-app#1313
File: queries/useV3ConversationListQuery.ts:196-268
Timestamp: 2024-12-05T18:33:24.472Z
Learning: In the TypeScript file `queries/useV3ConversationListQuery.ts`, helper functions like `updateGroupImageToConversationListQuery`, `updateGroupNameToConversationListQuery`, and `updateGroupDescriptionToConversationListQuery` are used to reduce dependency on exact types or creating objects that get passed unnecessarily, even though they internally use `updateGroupMetadataToConversationListQuery`.
queries/useBlockGroupMutation.ts (1)
Learnt from: alexrisch
PR: ephemeraHQ/converse-app#1313
File: queries/useV3ConversationListQuery.ts:196-268
Timestamp: 2024-12-05T18:33:24.472Z
Learning: In the TypeScript file `queries/useV3ConversationListQuery.ts`, helper functions like `updateGroupImageToConversationListQuery`, `updateGroupNameToConversationListQuery`, and `updateGroupDescriptionToConversationListQuery` are used to reduce dependency on exact types or creating objects that get passed unnecessarily, even though they internally use `updateGroupMetadataToConversationListQuery`.
features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversations.tsx (1)
Learnt from: technoplato
PR: ephemeraHQ/converse-app#1514
File: features/conversation-list/hooks/use-pinned-conversations.tsx:12-13
Timestamp: 2025-01-14T20:32:55.413Z
Learning: Non-null assertions for currentAccount are acceptable as the account context is guaranteed by the app's architecture.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (32)
.cursorrules (1)
8-13: LGTM! Changes improve code style clarity.The additions to the style guide enhance code readability by:
- Encouraging descriptive variable names, even if longer
- Providing clear guidance on comment placement
These changes align well with the document's core principles of preferring clean, readable code.
utils/date.test.ts (3)
1-10: LGTM! Imports are now correctly structured.The imports follow the recommended pattern for date-fns with locales imported from 'date-fns/locale'.
158-194: Update test suite name to match the function being tested.The test suite is still named "getMinimalDate" but it's testing the
getCompactRelativeTimefunction.-describe("getMinimalDate", () => { +describe("getCompactRelativeTime", () => {
159-193: LGTM! Comprehensive test coverage for time ranges.The test cases thoroughly verify:
- Edge cases (falsy input, future dates)
- All time ranges (seconds to years)
- Correct formatting of compact time strings
queries/queryClient.ts (1)
1-3: LGTM! Clear imports and well-named constants.The imports are properly organized and the constants have been renamed to better reflect their purpose as default values.
features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversations.tsx (2)
3-8: Well-structured modularization of conversation components and hooks!The separation of concerns into dedicated hooks and components improves maintainability and reusability.
Line range hint
20-62: Well-implemented component with proper separation of concerns!The component effectively handles:
- Loading states
- Error cases
- Type safety
- Proper memoization
- Themed styling
features/conversation-list/hooks/useShouldShowConnectingOrSyncing.tsx (1)
4-4: Verify consistent usage of the new query hook across the codebase.The change from
useConversationListQuerytouseConversationsQueryappears to be part of a broader refactoring effort.Let's verify that the old query hook has been completely replaced:
✅ Verification successful
✓ Verified: Query hook has been consistently updated
The old
useConversationListQueryhas been completely replaced withuseConversationsQueryacross the codebase, and all imports are using the correct path.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of the old query hook rg "useConversationListQuery" # Search for consistent usage of the new query hook rg "useConversationsQuery"Length of output: 1070
features/conversation-list/hooks/use-conversation-by-topic.ts (1)
9-12: Verify impact of removed query options.The removal of
refetchOnWindowFocusandrefetchOnMountoptions might affect the conversation refresh behavior. Please verify if this change aligns with the expected user experience.Let's check if these options are configured elsewhere:
✅ Verification successful
Removal of query options is safe and aligned with global defaults
The removed options (
refetchOnWindowFocusandrefetchOnMount) are already configured globally inqueryClient.tswith the same values to prevent infinite refetch loops. The removal eliminates redundant configuration without changing the behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any global configuration of these query options rg -l 'refetchOnWindowFocus|refetchOnMount' --type ts # Search for the configuration of useConversationsQuery ast-grep --pattern 'useConversationsQuery({ $$$ })'Length of output: 1668
Script:
#!/bin/bash # Check the content of queryClient.ts for default configurations cat queries/queryClient.ts # Check for any specific cache configurations cat queries/cacheOnlyQueryOptions.tsLength of output: 1700
features/GroupInvites/joinGroup/JoinGroup.client.ts (2)
24-26: LGTM! Clean import refactoring.The import changes align well with the standardization of conversation queries across the application.
93-98: LGTM! Clean implementation of dynamic imports and type updates.The changes maintain good separation of concerns while updating to the new query interface. The dynamic import pattern is consistently applied, which helps with test isolation.
data/helpers/conversations/spamScore.ts (1)
13-14: Review async implementation necessity.The function is marked as
asyncbut doesn't contain anyawaitoperations. This suggests either:
- Missing async operations for the TODOs
- Unnecessary async wrapper that could be removed
Let's check if there are any async operations in other related files:
✅ Verification successful
The
asyncimplementation is correctly usedThe async implementation is justified due to:
- A planned async operation for checking spam score between sender and receiver
- Current usage in async context by callers
- Interface consistency for future implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for async calls to getV3SpamScore or related spam checking functions rg -t typescript -t javascript "getV3SpamScore|spam.*score" -B 2 -A 2Length of output: 106
Script:
#!/bin/bash # Search for function usage and related code rg "getV3SpamScore" -B 2 -A 2 # Search for TODO comments near spam score related code rg "TODO.*spam|spam.*TODO" -B 2 -A 2 # Search for the function definition using ast-grep ast-grep --pattern 'async function getV3SpamScore($_) { $$$ }'Length of output: 1822
features/notifications/utils/accountTopicSubscription.ts (1)
19-19: Verify consistent usage of the new query observer across the codebase.The change from
createConversationListQueryObservertocreateConversationsQueryObservermight require similar updates in other files.✅ Verification successful
✓ Observer usage is consistent across the codebase
The change is properly implemented with no remaining instances of the old observer. The new
createConversationsQueryObserveris used consistently with matching parameters.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of old observer rg "createConversationListQueryObserver" # Search for usage of new observer to ensure consistent parameters rg "createConversationsQueryObserver"Length of output: 545
Script:
#!/bin/bash # Get the implementation and usage context ast-grep --pattern 'export const createConversationsQueryObserver = ($_) => { $$$ }' echo "---" ast-grep --pattern 'const observer = createConversationsQueryObserver({ $$$ })'Length of output: 977
features/conversation-requests-list/conversation-requests-list.screen.tsx (3)
1-15: LGTM! Imports are well-organized and properly used.All imports are necessary and follow the project's conventions.
16-36: Well-structured component with proper optimization!Good use of:
- Memoization with React.memo
- Callback optimization with useCallback
- Clean component hierarchy
62-67: Consider handling null currentAccount case.The non-null assertion on
currentAccountcould be unsafe. Consider adding a guard clause or error state.- account: currentAccount!, + account: currentAccount ?? throw new Error('No current account'),features/conversation-list/conversation-list-item/conversation-list-item-group.tsx (1)
25-25: Consider renaming the component for consistencyAs previously suggested, renaming the component to
GroupConversationListItemmay improve code readability and maintain consistency with other components likeDmConversationListItem.queries/unknown-consent-conversations-query.ts (2)
8-8: Consider documenting the planned permanent solution.The comment "For now just reuse the global conversations query" suggests this is a temporary solution. Consider documenting the planned approach or creating a tracking issue for the permanent implementation.
Would you like me to help create a tracking issue for implementing a dedicated query?
9-24: Clean implementation with good type safety!The implementation effectively reuses existing query options while adding specific filtering logic. The TypeScript types ensure type safety, and the null coalescing operator provides good error handling.
features/conversation/hooks/use-mark-conversation-as-unread.ts (1)
17-17: Consider handling the case where getCurrentAccount returns null.The non-null assertion operator (!) is used with getCurrentAccount(), which could be risky if the account is not available. Consider adding proper error handling.
-const currentAccount = getCurrentAccount()!; +const currentAccount = getCurrentAccount(); +if (!currentAccount) { + throw new Error("No account available"); +}Also applies to: 24-24, 43-43
queries/useGroupNameMutation.ts (1)
39-42: Clean update to use the new conversation update function.The migration from
updateConversationInConversationListQuerytoupdateConversationInConversationsQueryis implemented correctly and consistently.Also applies to: 54-57
queries/useGroupPhotoMutation.ts (1)
10-10: LGTM! Verify the impact of the renamed import.The refactoring to use
updateConversationInConversationsQuerylooks good. The implementation maintains proper error handling and optimistic updates.Let's verify this renaming is consistent across the codebase:
Also applies to: 39-42, 54-57
✅ Verification successful
Import refactoring verified and consistent across the codebase
The renamed import is properly updated in all relevant files with no remaining references to the old function name.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining usage of old function name rg "updateConversationInConversationListQuery" # Verify the new function is properly imported rg -p "import.*updateConversationInConversationsQuery"Length of output: 984
queries/useBlockGroupMutation.ts (1)
Line range hint
34-40: LGTM! The conversation state updates are properly handled.The implementation correctly updates conversation state in both success and error cases, with proper rollback handling.
Also applies to: 55-61
features/conversation/hooks/use-dm-consent.tsx (1)
Line range hint
65-71: LGTM! The conversation updates are properly handled.The implementation correctly updates conversation state using the new query function, with proper error handling and rollback.
Also applies to: 93-99
design-system/auto-sized-image.tsx (2)
5-14: LGTM! Well-documented props interface.The
IAutoSizedImagePropsinterface is well-defined with clear JSDoc comments for each prop.
22-35: Consider handling edge cases for source prop.The component looks good, but there's a potential edge case when handling the source prop.
Consider adding type guards or error handling for cases where source might be undefined:
- const source = props.source as ImageURISource; + const source = props.source as ImageURISource | undefined; + if (!source) { + return null; + }features/conversation/conversation-consent-popup/conversation-consent-popup-dm.tsx (1)
Line range hint
41-45: LGTM! Clean hook migration.The migration from
useDmConsentMutationtouseDmConsentis clean and maintains the same functionality.queries/useAllowGroupMutation.ts (1)
Line range hint
93-99: LGTM! Consistent with established patterns.The update to use
updateConversationInConversationsQueryaligns with the established pattern from PR #1313 of reducing dependency on exact types.Also applies to: 124-130
queries/QueryKeys.test.ts (1)
43-49: LGTM! Well-structured test addition.The new test for
conversationDataQueryKeyfollows the established pattern and includes proper assertions.features/conversation/conversation-new-dm.tsx (1)
87-89: LGTM! The query update aligns with the codebase refactoring.The change from
addConversationToConversationListQuerytoaddConversationToConversationsQueryis consistent with the broader refactoring of conversation management.utils/xmtpRN/conversations.ts (1)
19-19: LGTM! The query update maintains consistency.The change to use
addConversationToConversationsQueryaligns with the standardized conversation management approach.features/conversation-list/conversation-list.screen.tsx (1)
204-215: Optimize conversation filtering logic.The current filtering implementation could be more maintainable and performant.
Extract filter conditions into named functions:
const isConversationValid = (conversation: Conversation, query: QueryResult) => ({ isAllowed: isConversationAllowed(conversation), isNotPinned: !query?.data?.isPinned, isNotDeleted: !query?.data?.isDeleted, isLoaded: !query?.isLoading, });
| const { isLoading } = useConversationsQuery({ | ||
| account: currentAccount!, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding null safety check for currentAccount.
The non-null assertion (!) on currentAccount could lead to runtime errors if the account is null. Consider adding a null check or using optional chaining.
Here's a safer approach:
- const { isLoading } = useConversationsQuery({
- account: currentAccount!,
- });
+ const { isLoading } = useConversationsQuery({
+ account: currentAccount,
+ enabled: !!currentAccount,
+ });📝 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.
| const { isLoading } = useConversationsQuery({ | |
| account: currentAccount!, | |
| }); | |
| const { isLoading } = useConversationsQuery({ | |
| account: currentAccount, | |
| enabled: !!currentAccount, | |
| }); |
| @@ -19,7 +19,7 @@ export const getV3SpamScore = async ({ | |||
| // TODO: Check spam score between sender and receiver | |||
|
|
|||
| // Check contents of last message | |||
| spamScore += computeMessageContentSpamScore(message, contentType); | |||
| spamScore += computeMessageContentSpamScore(messageText, contentType); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance spam detection accuracy.
The current implementation might generate false positives. Consider:
- Using weighted scores instead of binary values
- Adding more sophisticated patterns
- Implementing machine learning-based classification
Example implementation:
const SPAM_WEIGHTS = {
url: 0.5,
currency: 0.3,
knownSpamPhrase: 0.8,
// Add more indicators
};
function getUrlWeight(url: string): number {
// Check against allowlist/blocklist
// Parse domain reputation
return SPAM_WEIGHTS.url;
}| <TouchableOpacity | ||
| key={index} | ||
| style={[ | ||
| { | ||
| flex: 1, | ||
| paddingVertical: 6, | ||
| alignItems: "center", | ||
| justifyContent: "center", | ||
| }, | ||
| index === selectedIndex && { | ||
| backgroundColor: backgroundColor(colorScheme), | ||
| borderRadius: 6, | ||
| }, | ||
| index === 0 && { | ||
| borderTopLeftRadius: 6, | ||
| borderBottomLeftRadius: 6, | ||
| }, | ||
| index === options.length - 1 && { | ||
| borderTopRightRadius: 6, | ||
| borderBottomRightRadius: 6, | ||
| }, | ||
| ]} | ||
| onPress={() => onSelect(index)} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility support and avoid using index as key.
The TouchableOpacity components should have proper accessibility props, and using index as key should be avoided if options are unique.
{options.map((option, index) => (
<TouchableOpacity
- key={index}
+ key={option}
+ accessible={true}
+ accessibilityRole="tab"
+ accessibilityLabel={option}
+ accessibilityState={{ selected: index === selectedIndex }}
style={[...]}
onPress={() => onSelect(index)}
>Committable suggestion skipped: line range outside the PR's diff.
| useEffect(() => { | ||
| async function processConversations( | ||
| conversations: ConversationWithCodecsType[] | ||
| ) { | ||
| try { | ||
| const isSpamResults = await Promise.allSettled( | ||
| conversations.map(async (conversation) => { | ||
| try { | ||
| // ! since we check for lastMessage in the select, we can safely assume it's not undefined | ||
| const lastMessage = conversation.lastMessage!; | ||
| const messageText = getMessageStringContent(lastMessage); | ||
|
|
||
| if (!messageText) { | ||
| return true; | ||
| } | ||
|
|
||
| // Get spam score | ||
| const contentType = getMessageContentType( | ||
| lastMessage.contentTypeId! | ||
| ); | ||
|
|
||
| if (!contentType) { | ||
| return true; | ||
| } | ||
|
|
||
| const spamScore = await getV3SpamScore({ | ||
| messageText: messageText, | ||
| contentType, | ||
| }); | ||
|
|
||
| if (spamScore === 0) { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } catch (error) { | ||
| captureError(error); | ||
| return true; | ||
| } | ||
| }) | ||
| ); | ||
|
|
||
| const notSpams: ConversationWithCodecsType[] = []; | ||
| const spams: ConversationWithCodecsType[] = []; | ||
|
|
||
| isSpamResults.forEach((result, index) => { | ||
| if (result.status !== "fulfilled") { | ||
| return; | ||
| } | ||
| const conversation = conversations[index]; | ||
| const isSpam = result.value; | ||
| if (isSpam) { | ||
| spams.push(conversation); | ||
| } else { | ||
| notSpams.push(conversation); | ||
| } | ||
| }); | ||
|
|
||
| setLikelyNotSpam(notSpams); | ||
| setLikelySpam(spams); | ||
| } catch (error) { | ||
| captureErrorWithToast(error); | ||
| } | ||
| } | ||
|
|
||
| processConversations(conversations ?? []); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [numberOfConversations]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review effect dependencies and error handling.
Several concerns in the effect implementation:
- The dependency array only includes
numberOfConversations, butconversationsshould also be included - The error handling in the spam detection could be simplified
- The ESLint disable comment should include a justification
Consider refactoring the spam detection logic into a separate utility function to reduce complexity and improve maintainability. Example:
const processSpamDetection = async (conversation: ConversationWithCodecsType): Promise<boolean> => {
const lastMessage = conversation.lastMessage;
if (!lastMessage) return true;
const messageText = getMessageStringContent(lastMessage);
if (!messageText) return true;
const contentType = getMessageContentType(lastMessage.contentTypeId!);
if (!contentType) return true;
try {
const spamScore = await getV3SpamScore({
messageText,
contentType,
});
return spamScore !== 0;
} catch (error) {
captureError(error);
return true;
}
};Then update the effect:
useEffect(() => {
async function processConversations(conversations: ConversationWithCodecsType[]) {
try {
- const isSpamResults = await Promise.allSettled(
- conversations.map(async (conversation) => {
- try {
- // Complex spam detection logic
- } catch (error) {
- captureError(error);
- return true;
- }
- })
- );
+ const isSpamResults = await Promise.allSettled(
+ conversations.map(processSpamDetection)
+ );
const notSpams: ConversationWithCodecsType[] = [];
const spams: ConversationWithCodecsType[] = [];
isSpamResults.forEach((result, index) => {
if (result.status !== "fulfilled") return;
const conversation = conversations[index];
result.value ? spams.push(conversation) : notSpams.push(conversation);
});
setLikelyNotSpam(notSpams);
setLikelySpam(spams);
} catch (error) {
captureErrorWithToast(error);
}
}
processConversations(conversations ?? []);
- // eslint-disable-next-line react-hooks/exhaustive-deps
- }, [numberOfConversations]);
+ }, [conversations]); // Include conversations in dependencies| if (!messageText) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Review reveals implementation issues in spam classification.
The current implementation has several potential bugs:
- Non-text messages are automatically classified as spam
- Unknown content types are automatically classified as spam
- Messages with zero spam score are incorrectly marked as spam (should be the opposite)
These issues could lead to false positives and poor user experience, particularly for legitimate non-text messages like images or attachments.
🔗 Analysis chain
Review default spam classification.
The current implementation classifies non-text messages and unknown content types as spam by default. Consider if this is the intended behavior or if it should be handled differently.
Also applies to: 92-94
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for non-text message handling in other parts of the codebase
rg -A 2 "getMessageStringContent|getMessageContentType" --type typescript
Length of output: 110
Script:
#!/bin/bash
# Get the full context of the file under review
fd -e tsx "conversation-requests-list.screen.tsx" --exec cat {}
# Search for message classification and spam related code
rg -g "*.{ts,tsx}" "spam.*classif|message.*spam|spam.*filter" -A 2
# Search for message text and content handling
rg -g "*.{ts,tsx}" "messageText|message.*content|content.*type" -A 2
Length of output: 33792
| export const getConversationsQueryOptions = (args: IArgs) => { | ||
| const { account, context } = args; | ||
| return queryOptions({ | ||
| queryKey: conversationsQueryKey(account), |
There was a problem hiding this comment.
Include context in the query key to differentiate queries
The context parameter is not included in the queryKey, which may lead to cache inconsistencies when different contexts are used. Please include context in the queryKey to ensure that queries with different contexts are cached separately.
Suggested change:
- queryKey: conversationsQueryKey(account),
+ queryKey: conversationsQueryKey(account, context),Committable suggestion skipped: line range outside the PR's diff.
| import { getCurrentAccount } from "@/data/store/accountsStore"; | ||
| import { | ||
| getConversationDataQueryData, | ||
| updateConversationDataQueryData, | ||
| } from "@/queries/conversation-data-query"; | ||
| import { markTopicAsRead } from "@/utils/api/topics"; | ||
| import { useMutation } from "@tanstack/react-query"; | ||
| import { ConversationTopic } from "@xmtp/react-native-sdk"; | ||
|
|
||
| export function useMarkConversationAsRead(args: { topic: ConversationTopic }) { | ||
| const { topic } = args; | ||
|
|
||
| const { mutateAsync: markAsReadAsync } = useMutation({ | ||
| mutationFn: async () => { | ||
| const currentAccount = getCurrentAccount()!; | ||
| await markTopicAsRead({ | ||
| account: currentAccount, | ||
| topic, | ||
| readUntil: new Date().getTime(), | ||
| }); | ||
| }, | ||
| onMutate: () => { | ||
| const currentAccount = getCurrentAccount()!; | ||
| const previousData = getConversationDataQueryData({ | ||
| account: currentAccount, | ||
| topic, | ||
| context: "useToggleReadStatus", | ||
| }); | ||
|
|
||
| updateConversationDataQueryData({ | ||
| account: currentAccount, | ||
| topic, | ||
| context: "useToggleReadStatus", | ||
| updateData: { | ||
| readUntil: new Date().getTime(), | ||
| markedAsUnread: false, | ||
| }, | ||
| }); | ||
|
|
||
| return { previousData }; | ||
| }, | ||
| onError: (error, _, context) => { | ||
| const currentAccount = getCurrentAccount()!; | ||
| updateConversationDataQueryData({ | ||
| account: currentAccount, | ||
| topic, | ||
| context: "useToggleReadStatus", | ||
| updateData: context?.previousData, | ||
| }); | ||
| }, | ||
| }); | ||
|
|
||
| return { | ||
| markAsReadAsync, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider combining read/unread functionality into a single hook.
This implementation has significant overlap with use-mark-conversation-as-unread.ts. Consider creating a single hook that handles both read and unread states to reduce code duplication.
Example implementation:
type ReadStatus = 'read' | 'unread';
export function useMarkConversationReadStatus(args: {
topic: ConversationTopic
}) {
const { topic } = args;
const { mutateAsync: markStatusAsync } = useMutation({
mutationFn: async (status: ReadStatus) => {
const currentAccount = getCurrentAccount();
if (!currentAccount) {
throw new Error("No account available");
}
if (status === 'read') {
await markTopicAsRead({
account: currentAccount,
topic,
readUntil: new Date().getTime(),
});
} else {
await markTopicAsUnread({
account: currentAccount,
topic,
});
}
},
// ... rest of the implementation
});
return {
markAsReadAsync: () => markStatusAsync('read'),
markAsUnreadAsync: () => markStatusAsync('unread'),
};
}| @@ -50,6 +50,7 @@ export function useDmConsentMutation(args: { | |||
| const conversation = getConversationQueryData({ | |||
| account: currentAccount, | |||
| topic, | |||
| context: "useDmConsentMutation", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update the context string to match the new function name.
The context string still uses the old function name "useDmConsentMutation".
- context: "useDmConsentMutation",
+ context: "useDmConsent",📝 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.
| context: "useDmConsentMutation", | |
| context: "useDmConsent", |
| export function useAutoSizeImage( | ||
| remoteUri: string, | ||
| dimensions?: [maxWidth?: number, maxHeight?: number] | ||
| ): [width: number, height: number] { | ||
| const [[remoteWidth, remoteHeight], setRemoteImageDimensions] = useState([ | ||
| 0, 0, | ||
| ]); | ||
| const remoteAspectRatio = remoteWidth / remoteHeight; | ||
| const [maxWidth, maxHeight] = dimensions ?? []; | ||
|
|
||
| useLayoutEffect(() => { | ||
| if (!remoteUri) return; | ||
|
|
||
| RNImage.getSize(remoteUri, (w, h) => setRemoteImageDimensions([w, h])); | ||
| }, [remoteUri]); | ||
|
|
||
| if (Number.isNaN(remoteAspectRatio)) return [0, 0]; | ||
|
|
||
| if (maxWidth && maxHeight) { | ||
| const aspectRatio = Math.min( | ||
| maxWidth / remoteWidth, | ||
| maxHeight / remoteHeight | ||
| ); | ||
| return [remoteWidth * aspectRatio, remoteHeight * aspectRatio]; | ||
| } else if (maxWidth) { | ||
| return [maxWidth, maxWidth / remoteAspectRatio]; | ||
| } else if (maxHeight) { | ||
| return [maxHeight * remoteAspectRatio, maxHeight]; | ||
| } else { | ||
| return [remoteWidth, remoteHeight]; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for image loading failures.
The hook implementation is good, but it's missing error handling for Image.getSize.
Add error handling to prevent silent failures:
useLayoutEffect(() => {
if (!remoteUri) return;
- RNImage.getSize(remoteUri, (w, h) => setRemoteImageDimensions([w, h]));
+ RNImage.getSize(
+ remoteUri,
+ (w, h) => setRemoteImageDimensions([w, h]),
+ (error) => {
+ console.error('Failed to load image dimensions:', error);
+ setRemoteImageDimensions([0, 0]);
+ }
+ );
}, [remoteUri]);📝 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 function useAutoSizeImage( | |
| remoteUri: string, | |
| dimensions?: [maxWidth?: number, maxHeight?: number] | |
| ): [width: number, height: number] { | |
| const [[remoteWidth, remoteHeight], setRemoteImageDimensions] = useState([ | |
| 0, 0, | |
| ]); | |
| const remoteAspectRatio = remoteWidth / remoteHeight; | |
| const [maxWidth, maxHeight] = dimensions ?? []; | |
| useLayoutEffect(() => { | |
| if (!remoteUri) return; | |
| RNImage.getSize(remoteUri, (w, h) => setRemoteImageDimensions([w, h])); | |
| }, [remoteUri]); | |
| if (Number.isNaN(remoteAspectRatio)) return [0, 0]; | |
| if (maxWidth && maxHeight) { | |
| const aspectRatio = Math.min( | |
| maxWidth / remoteWidth, | |
| maxHeight / remoteHeight | |
| ); | |
| return [remoteWidth * aspectRatio, remoteHeight * aspectRatio]; | |
| } else if (maxWidth) { | |
| return [maxWidth, maxWidth / remoteAspectRatio]; | |
| } else if (maxHeight) { | |
| return [maxHeight * remoteAspectRatio, maxHeight]; | |
| } else { | |
| return [remoteWidth, remoteHeight]; | |
| } | |
| } | |
| export function useAutoSizeImage( | |
| remoteUri: string, | |
| dimensions?: [maxWidth?: number, maxHeight?: number] | |
| ): [width: number, height: number] { | |
| const [[remoteWidth, remoteHeight], setRemoteImageDimensions] = useState([ | |
| 0, 0, | |
| ]); | |
| const remoteAspectRatio = remoteWidth / remoteHeight; | |
| const [maxWidth, maxHeight] = dimensions ?? []; | |
| useLayoutEffect(() => { | |
| if (!remoteUri) return; | |
| RNImage.getSize( | |
| remoteUri, | |
| (w, h) => setRemoteImageDimensions([w, h]), | |
| (error) => { | |
| console.error('Failed to load image dimensions:', error); | |
| setRemoteImageDimensions([0, 0]); | |
| } | |
| ); | |
| }, [remoteUri]); | |
| if (Number.isNaN(remoteAspectRatio)) return [0, 0]; | |
| if (maxWidth && maxHeight) { | |
| const aspectRatio = Math.min( | |
| maxWidth / remoteWidth, | |
| maxHeight / remoteHeight | |
| ); | |
| return [remoteWidth * aspectRatio, remoteHeight * aspectRatio]; | |
| } else if (maxWidth) { | |
| return [maxWidth, maxWidth / remoteAspectRatio]; | |
| } else if (maxHeight) { | |
| return [maxHeight * remoteAspectRatio, maxHeight]; | |
| } else { | |
| return [remoteWidth, remoteHeight]; | |
| } | |
| } |
| account: currentAccount!, | ||
| context: "conversation-list-screen", |
There was a problem hiding this comment.
Add error handling for undefined account.
The component uses non-null assertions on currentAccount, which could lead to runtime errors.
Add proper error handling:
+if (!currentAccount) {
+ return { data: [], isLoading: false };
+}
const { data: conversations, ...rest } = useQuery({
...getConversationsQueryOptions({
- account: currentAccount!,
+ account: currentAccount,
context: "conversation-list-screen",
}),
});Also applies to: 194-195
Summary by CodeRabbit
I'll craft concise release notes focusing on the key user-facing changes:
Release Notes
Conversation Management
User Interface
Performance
Bug Fixes
Experimental Features