feat: Headers and New Conversation #1237
Conversation
Updated new conversation Handling Moved components to be in features branch Updated title components Bumped RN SDK Commented out api header work
alexrisch
force-pushed
the
ar/fix-new-dm
branch
from
bff475e to
6489942
Compare
WalkthroughThis pull request introduces significant changes to the conversation components of the application. It removes the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Outside diff range and nitpick comments (40)
features/conversations/hooks/useConversationTitleLongPress.ts (2)
5-7: Consider adding JSDoc documentation for the hook.Since this is a debug-only feature, it would be helpful to document its purpose and usage.
+/** + * A debug-only hook that enables copying conversation topic data to clipboard on long press. + * Only active when debug mode is enabled. + * @param topic The conversation topic to be copied + */ export const useConversationTitleLongPress = (topic: string) => {
8-15: Add user feedback and error handling for clipboard operations.The current implementation lacks user feedback and error handling for clipboard operations. Consider adding these improvements:
return useCallback(() => { if (!debugEnabled) return; - Clipboard.setString( - JSON.stringify({ - topic, - }) - ); + try { + const data = JSON.stringify({ + topic, + timestamp: new Date().toISOString(), + debug: true + }, null, 2); + Clipboard.setString(data); + // Consider adding a toast or alert to indicate successful copy + } catch (error) { + console.warn('Failed to copy debug data:', error); + // Consider showing an error message to the user + } }, [debugEnabled, topic]);features/conversation-list/hooks/useMessageIsUnread.ts (1)
17-18: Consider adding JSDoc for store selection keysThe type-safe store selection setup is good. For better maintainability, consider adding a JSDoc comment explaining the purpose of these keys.
+/** Keys required from ChatStore for conversation unread status determination */ const chatStoreSelectKeys: (keyof ChatStoreType)[] = ["topicsData"];screens/Conversation.tsx (1)
14-19: Verify topic validation completenessWhile the
isV3Topicvalidation is in place, consider adding type validation fortextPrefillto ensure data consistency.+ if (route.params?.text && typeof route.params.text !== 'string') { + console.warn('Invalid textPrefill type provided'); + return <VStack />; + } if (route.params?.topic && isV3Topic(route.params.topic)) { return ( <V3Conversationfeatures/conversations/components/ConversationTitleDumb.tsx (1)
10-15: Consider renaming the component to better reflect its purposeThe suffix "Dumb" in the component name doesn't follow React best practices. Consider renaming to something more descriptive like
BaseConversationTitleorPresentationalConversationTitleto better reflect its role as a presentational component.Additionally, consider adding JSDoc comments to document the props:
+/** + * A presentational component for rendering conversation titles with optional avatar + * @param title - The conversation title to display + * @param avatarComponent - Optional avatar component to render + * @param onLongPress - Callback fired on long press + * @param onPress - Callback fired on press + */ type ConversationTitleDumbProps = { title?: string; avatarComponent?: React.ReactNode; onLongPress?: () => void; onPress?: () => void; };features/conversations/components/V3ConversationFromPeer.tsx (2)
10-14: Add JSDoc documentation for the props type.Consider adding JSDoc documentation to describe the purpose and expected values of each prop.
+/** + * Props for the V3ConversationFromPeer component + * @param peer - The identifier of the peer to load the conversation with + * @param textPrefill - Optional text to prefill in the conversation + * @param skipLoading - Whether to skip the loading state and data fetching + */ type V3ConversationFromPeerProps = { peer: string; textPrefill?: string; skipLoading: boolean; };
1-57: Consider enhancing component reusability.The component effectively abstracts conversation loading logic, but consider these architectural improvements:
- Extract the loading and error states into reusable components.
- Consider implementing a higher-order component pattern to share this loading behavior with other conversation types.
- Add proper error boundaries for better error handling at the component level.
features/conversations/hooks/useGroupMembersAvatarData.ts (3)
9-11: Add JSDoc documentation for the type definitionConsider adding JSDoc documentation to describe the purpose of the type and what
ConversationTopicrepresents.+/** + * Props for the useGroupMembersAvatarData hook + * @property {ConversationTopic} topic - The conversation topic to fetch member data for + */ type UseGroupMembersAvatarDataProps = { topic: ConversationTopic; };
22-34: Simplify member address processing using array methodsThe current implementation can be made more concise and maintainable using array methods.
const memberAddresses = useMemo(() => { - const addresses: string[] = []; - for (const memberId of members?.ids ?? []) { - const member = members?.byId[memberId]; - if ( - member?.addresses[0] && - member?.addresses[0].toLowerCase() !== currentAccount?.toLowerCase() - ) { - addresses.push(member?.addresses[0]); - } - } - return addresses; + return (members?.ids ?? []) + .map(id => members?.byId[id]?.addresses[0]) + .filter((address): address is string => + !!address && address.toLowerCase() !== currentAccount.toLowerCase() + ); }, [members, currentAccount]);
13-59: Consider adding error boundary support and loading state handlingWhile the hook implementation is solid, consider these architectural improvements:
- Implement a custom error type for better error handling across the application
- Add loading state handling for better UX
- Consider adding retry logic for failed social data fetches
Example error type:
type GroupMemberError = { code: 'NO_ACCOUNT' | 'FETCH_ERROR' | 'DATA_MISMATCH'; message: string; details?: unknown; };screens/Navigation/ConversationNav.tsx (1)
27-27: Document the purpose of skipLoading parameterWhile the type definition is correct, please add a JSDoc comment explaining when and why this flag should be used, as it affects the behavior of multiple components.
+ /** When true, skips the loading state during navigation. Used by GroupConversationTitle and V3ConversationFromPeer components. */ skipLoading?: boolean;components/Search/NavigationChatButton.tsx (3)
Line range hint
34-46: Consider replacing setTimeout with navigation events.Using
setTimeoutfor navigation timing can be fragile. Consider using navigation events or callbacks for more reliable navigation timing.- setTimeout(() => { + // Use navigation events for more reliable timing + const unsubscribe = navigation.addListener('transitionEnd', () => { navigate("Conversation", { mainConversationWithPeer: address, focus: true, skipLoading: true, }); - }, 300); + unsubscribe(); + });
Line range hint
49-65: Enhance error handling in addToGroupIfPossible.The function could benefit from better error handling to prevent the loading state from getting stuck and to provide better feedback to users.
const addToGroupIfPossible = useCallback(async () => { if (loading) return; setLoading(true); - const allowed = await canMessageByAccount({ - account: currentAccount(), - peer: address, - }); - setLoading(false); - // canGroupMessage() returns lowercase addresses - if (!allowed) { - Alert.alert( - translate("cannot_be_added_to_group_yet", { - name: preferredName, - }) - ); - return; - } - addToGroup?.(); + try { + const allowed = await canMessageByAccount({ + account: currentAccount(), + peer: address, + }); + // canGroupMessage() returns lowercase addresses + if (!allowed) { + Alert.alert( + translate("cannot_be_added_to_group_yet", { + name: preferredName, + }) + ); + return; + } + await addToGroup?.(); + } catch (error) { + Alert.alert( + translate("error_adding_to_group", { + name: preferredName, + error: error.message + }) + ); + } finally { + setLoading(false); + } }, [loading, address, addToGroup, preferredName]);
Line range hint
49-49: Add cleanup for loading state.Consider adding a cleanup effect to reset the loading state when the component unmounts.
+ useEffect(() => { + return () => { + // Reset loading state on unmount to prevent memory leaks + setLoading(false); + }; + }, []);queries/useConversationQuery.ts (2)
56-56: Consider environment-specific logging.The debug logging statement might be better suited for development environments only.
Consider wrapping it in a development check:
- logger.info("[Crash Debug] queryFn fetching conversation with peer"); + if (__DEV__) { + logger.info("[Crash Debug] queryFn fetching conversation with peer"); + }
45-93: Consider enhancing error handling and type safety.While the hooks implement good null checking, consider these architectural improvements:
- Add error boundaries or error handling callbacks for failed queries
- Consider using discriminated unions for better type safety in the return types
- Add retry policies for failed network requests
- Consider implementing optimistic updates for better UX
Example error handling enhancement:
type ConversationError = { code: string; message: string; }; const useConversationWithPeerQuery = ( account: string, peer: string | undefined, options?: Partial< UseQueryOptions< ConversationWithCodecsType | null | undefined, ConversationError > > ) => { return useQuery({ ...options, retry: (failureCount, error) => { // Custom retry logic return failureCount < 3 && error.code !== 'UNAUTHORIZED'; }, // ... rest of the implementation }); };features/conversations/components/DmConversationTitle.tsx (4)
18-26: Consider adding runtime validation for the topic prop.While the TypeScript types provide compile-time safety, consider adding runtime validation for the
topicprop to handle potential undefined cases more gracefully, especially sinceuseCurrentAccountis used with a non-null assertion later.+import { z } from 'zod'; + +const topicSchema = z.instanceof(ConversationTopic); + type DmConversationTitleProps = { topic: ConversationTopic; };
66-68: Improve loading state handling.The current implementation returns null during loading, which might cause layout shifts. Consider showing a skeleton loader instead.
- if (!displayAvatar) return null; + if (!displayAvatar) { + return ( + <ConversationTitleDumb + title="Loading..." + avatarComponent={<SkeletonAvatar size={AvatarSizes.conversationTitle} />} + /> + ); + }
88-91: Review platform-specific margin values.The negative margin on Android (
-theme.spacing.xxs) could potentially cause clipping issues. Consider using positive margins consistently across platforms and adjusting the container padding instead.const $avatar: ThemedStyle<ImageStyle> = (theme) => ({ - marginRight: Platform.OS === "android" ? theme.spacing.lg : theme.spacing.xxs, - marginLeft: Platform.OS === "ios" ? theme.spacing.zero : -theme.spacing.xxs, + marginRight: theme.spacing.sm, + marginLeft: theme.spacing.zero, });
44-86: Consider implementing error boundaries and retry logic.The component makes multiple API calls through hooks but lacks error boundaries and retry mechanisms. Consider:
- Implementing a higher-order error boundary component
- Adding retry logic for failed data fetches
- Implementing proper error states in the UI
This will improve the reliability and user experience of the conversation title component.
features/conversations/components/GroupConversationTitle.tsx (4)
29-29: Remove commented-out code.The commented line referencing
textInputRefshould either be implemented or removed to maintain clean code.- // textInputRef?.current?.blur();
27-36: Consider moving useUserInteraction to a separate file.The hook is well-implemented but should be moved to a separate file (e.g.,
hooks/useUserInteraction.ts) to improve modularity and reusability.
42-49: Remove redundant non-null assertions on topic.The topic parameter is already validated through the props type definition, making these non-null assertions unnecessary.
- topic! + topic - useGroupPhotoQuery(currentAccount, topic!); + useGroupPhotoQuery(currentAccount, topic);
42-50: Consider implementing data prefetching or combining queries.The component makes multiple separate queries (groupName, groupPhoto, memberData) which could impact performance. Consider:
- Implementing data prefetching
- Combining these queries into a single hook for better performance
components/Conversation/GroupConversationTitle.tsx (2)
27-36: Remove or implement the commented codeThere's a commented line referencing
textInputRef?.current?.blur(). This should either be implemented or removed if no longer needed.const useUserInteraction = ({ navigation, topic }: UseUserInteractionProps) => { const onPress = useCallback(() => { - // textInputRef?.current?.blur(); navigation.push("Group", { topic }); }, [navigation, topic]);
38-89: Consider splitting data fetching logic into a separate hookThe component currently handles multiple responsibilities. Consider extracting the data fetching logic into a custom hook like
useGroupConversationDatato improve maintainability and reusability.Example structure:
const useGroupConversationData = (currentAccount: Account, topic: ConversationTopic) => { const { data: groupName, isLoading: groupNameLoading } = useGroupNameQuery( currentAccount, topic ); const { data: groupPhoto, isLoading: groupPhotoLoading } = useGroupPhotoQuery( currentAccount, topic ); const { data: memberData } = useGroupMembersAvatarData({ topic }); return { groupName, groupPhoto, memberData, isLoading: groupNameLoading || groupPhotoLoading }; };components/V3DMListItem.tsx (7)
Line range hint
22-26: Consider using a more specific type for timestampThe
timestampinUseDisplayInfoPropscould benefit from a more specific type definition to better represent its Unix timestamp nature.type UseDisplayInfoProps = { - timestamp: number; + timestamp: number; // Unix timestamp in nanoseconds isUnread: boolean; };
73-74: Consider handling invalid timestamp valuesThe nullish coalescing operator handles undefined/null cases, but you might want to validate the timestamp value to ensure it's a valid number.
- const timestamp = conversation?.lastMessage?.sentNs ?? 0; + const timestamp = Number.isFinite(conversation?.lastMessage?.sentNs) + ? conversation.lastMessage.sentNs + : 0;
88-89: Fix typo in variable nameThere's a typo in the variable name "preffered" which should be "preferred".
- const prefferedName = usePreferredInboxName(peer); + const preferredName = usePreferredInboxName(peer);
Line range hint
102-106: Remove or implement commented codeThere's a commented out
openConversation()call. Either implement the functionality or remove the comment if it's no longer needed.
Line range hint
107-146: Add error handling for delete operationsThe delete operations perform several async operations without proper error handling. Consider wrapping these in try-catch blocks to handle potential failures gracefully.
const handleDelete = useCallback(() => { const options = [ translate("delete"), translate("delete_and_block"), translate("cancel"), ]; const title = `${translate("delete_chat_with")} ${preferredName}?`; const actions = [ - () => { + async () => { + try { saveTopicsData(currentAccount, { [topic]: { status: "deleted", timestamp: new Date().getTime(), }, }), setTopicsData({ [topic]: { status: "deleted", timestamp: new Date().getTime(), }, }); + } catch (error) { + // Handle error appropriately + console.error('Failed to delete conversation:', error); + } }, async () => { + try { saveTopicsData(currentAccount, { [topic]: { status: "deleted" }, }); setTopicsData({ [topic]: { status: "deleted", timestamp: new Date().getTime(), }, }); await conversation.updateConsent("denied"); const peerInboxId = await conversation.peerInboxId(); await consentToInboxIdsOnProtocolByAccount({ account: currentAccount, inboxIds: [peerInboxId], consent: "deny", }); + } catch (error) { + // Handle error appropriately + console.error('Failed to delete and block conversation:', error); + } }, ];
Line range hint
252-269: Fix inconsistency in unread status handlingThe
isUnreadprop is hardcoded tofalsewhile there's anisUnreadstate being tracked. This might cause incorrect rendering of the unread status.subtitle={`${timeToShow} ⋅ ${messageText}`} - isUnread={false} + isUnread={isUnread} contextMenuComponent={contextMenuComponent} rightIsDestructive={isBlockedChatView}
Line range hint
254-254: Remove or implement commented propThe
onRightActionPressprop is commented out. Either implement it or remove it if it's no longer needed.components/V3GroupConversationListItem.tsx (2)
Line range hint
83-146: TODO comments need to be addressed in handleDelete function.The implementation looks complete despite the TODO comment. Consider removing the TODO comment if the implementation is finished.
Would you like me to help remove these TODO comments or create an issue to track any remaining tasks?
Line range hint
147-189: TODO comments need to be addressed in handleRestore function.Similar to handleDelete, the implementation appears complete despite the TODO comment.
Would you like me to help remove these TODO comments or create an issue to track any remaining tasks?
features/conversations/components/V3Conversation.tsx (3)
105-105: Address the TODO comment to update valuesThere's a TODO comment indicating that values need to be updated (line 105~). Please ensure these values are properly set before merging.
Do you need assistance in updating these values?
238-238: Implement the DM placeholder componentThere's a TODO comment to add a DM placeholder (line 238~). Implementing this will enhance the user experience for direct messages.
Would you like help in creating the DM placeholder component?
287-287: PassframeTextInputFocusedstate toChatDumbCurrently,
frameTextInputFocusedis hardcoded tofalsewhen passed toChatDumb(line 287~). If the intention is to reflect the component's state, consider passing theframeTextInputFocusedstate variable.- frameTextInputFocused={false} + frameTextInputFocused={frameTextInputFocused}utils/xmtpRN/conversations.ts (2)
193-194: Clarify log message to indicate 'peer' instead of 'topic'When a conversation is not found in
getConversationByPeer, the log message could be more precise. It currently states "Conversation ${peer} not found", which may be ambiguous. Consider clarifying that it's the conversation with the specified peer that was not found.Proposed change:
logger.debug( - `[XMTPRN Conversations] Conversation ${peer} not found, syncing conversations` + `[XMTPRN Conversations] Conversation with peer ${peer} not found, syncing conversations` );
177-225: Refactor duplicate code in conversation retrieval functionsThe functions
getConversationByPeerandgetConversationByTopiccontain similar logic for retrieving and syncing conversations. Consider refactoring the common logic into a shared helper function to reduce code duplication and improve maintainability.For example, you could create a helper function like
findConversationWithSync:async function findConversationWithSync({ findConversationFn, client, identifier, includeSync, errorMessage, }: { findConversationFn: () => Promise<ConversationWithCodecsType | undefined>; client: ConverseXmtpClientType; identifier: string; includeSync: boolean; errorMessage: string; }) { logger.debug(`[XMTPRN Conversations] Getting conversation: ${identifier}`); const start = new Date().getTime(); let conversation = await findConversationFn(); if (!conversation) { logger.debug( `[XMTPRN Conversations] Conversation ${identifier} not found, syncing conversations` ); const syncStart = new Date().getTime(); await client.conversations.sync(); const syncEnd = new Date().getTime(); logger.debug( `[XMTPRN Conversations] Synced conversations in ${ (syncEnd - syncStart) / 1000 } sec` ); conversation = await findConversationFn(); } if (!conversation) { throw new Error(errorMessage); } if (includeSync) { const syncStart = new Date().getTime(); await conversation.sync(); const syncEnd = new Date().getTime(); logger.debug( `[XMTPRN Conversations] Synced conversation in ${ (syncEnd - syncStart) / 1000 } sec` ); } const end = new Date().getTime(); logger.debug( `[XMTPRN Conversations] Got conversation in ${(end - start) / 1000} sec` ); return conversation; }Then refactor
getConversationByPeer:export const getConversationByPeer = async ({ client, peer, includeSync = false, }: GetConversationByPeerParams) => { - // Existing implementation + return findConversationWithSync({ + findConversationFn: () => client.conversations.findDmByAddress(peer), + client, + identifier: peer, + includeSync, + errorMessage: `Conversation with peer ${peer} not found`, + }); };And refactor
getConversationByTopicsimilarly.Also applies to: 233-240
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
components/Conversation/ConversationTitleDumb.tsx(0 hunks)components/Conversation/GroupConversationTitle.tsx(1 hunks)components/Conversation/V3Conversation.tsx(6 hunks)components/Conversation/V3ConversationHeader.tsx(0 hunks)components/Search/NavigationChatButton.tsx(2 hunks)components/V3DMListItem.tsx(1 hunks)components/V3GroupConversationListItem.tsx(1 hunks)features/conversation-list/hooks/useMessageIsUnread.ts(1 hunks)features/conversations/components/ConversationTitleDumb.tsx(1 hunks)features/conversations/components/DmConversationTitle.tsx(1 hunks)features/conversations/components/GroupConversationTitle.tsx(1 hunks)features/conversations/components/V3Conversation.tsx(1 hunks)features/conversations/components/V3ConversationFromPeer.tsx(1 hunks)features/conversations/hooks/useConversationTitleLongPress.ts(1 hunks)features/conversations/hooks/useGroupMembersAvatarData.ts(1 hunks)ios/Podfile(1 hunks)package.json(1 hunks)queries/QueryKeys.ts(2 hunks)queries/useConversationQuery.ts(2 hunks)queries/useDmPeerAddressQuery.ts(1 hunks)screens/Conversation.tsx(1 hunks)screens/Navigation/ConversationNav.tsx(2 hunks)utils/xmtpRN/api.ts(2 hunks)utils/xmtpRN/conversations.ts(3 hunks)
💤 Files with no reviewable changes (2)
- components/Conversation/ConversationTitleDumb.tsx
- components/Conversation/V3ConversationHeader.tsx
✅ Files skipped from review due to trivial changes (1)
- utils/xmtpRN/api.ts
🔇 Additional comments (32)
features/conversations/hooks/useConversationTitleLongPress.ts (2)
1-3: LGTM! Imports are well-organized and necessary.
The imports follow good practices with a clear separation between internal components, external packages, and React hooks.
5-16: Verify the hook's integration with conversation components.
The hook seems to be used by conversation title components. Let's verify its integration to ensure consistent usage.
✅ Verification successful
Let me check the implementation in the conversation title components to ensure consistent usage.
Hook integration is consistent across conversation components
The useConversationTitleLongPress hook is properly integrated and consistently used in both DM and Group conversation title components. All components:
- Import the hook correctly
- Pass the required
topicparameter - Use the returned
onLongPresshandler in the same way - Only activate the clipboard functionality when debug mode is enabled
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the hook is being used in conversation components
# Look for usage of useConversationTitleLongPress
rg -l "useConversationTitleLongPress"
# Check for other clipboard-related operations in conversation components
rg -A 3 "Clipboard\." "features/conversations/"
# Look for other debug-related hooks in conversation components
ast-grep --pattern 'useDebugEnabled'
Length of output: 1208
Script:
#!/bin/bash
# Check implementation in conversation title components
rg -A 10 "useConversationTitleLongPress" "features/conversations/components/GroupConversationTitle.tsx" "features/conversations/components/DmConversationTitle.tsx" "components/Conversation/GroupConversationTitle.tsx"
Length of output: 6443
queries/useDmPeerAddressQuery.ts (2)
1-4: LGTM! Well-structured imports.
The imports are clean, properly typed, and follow good practices.
6-15: Consider enhancing error handling and verify caching strategy.
The hook implementation follows React Query best practices and is well-typed. However, consider two improvements:
- Add explicit error handling for failed peer address lookups
- Verify if cache-only strategy meets real-time messaging requirements
Consider adding error handling like this:
export const useDmPeerAddressQuery = (
account: string,
topic: ConversationTopic
) => {
return useQuery({
queryKey: ["dmPeerAddress", account, topic],
queryFn: () => getPeerAddressFromTopic(account, topic),
+ retry: false,
+ onError: (error) => {
+ console.error('Failed to fetch peer address:', error);
+ },
...cacheOnlyQueryOptions,
});
};✅ Verification successful
Cache-only strategy is appropriate, but error handling should be added
The cache-only configuration is consistently used across the codebase for XMTP-related queries (useConversationMessages, V3Message) and is appropriate for this use case as it prevents unnecessary refetches of peer addresses which are unlikely to change frequently.
However, error handling should be added since getPeerAddressFromTopic involves async operations that could fail (it calls getConversationByTopicByAccount and potentially getPeerAddressDm). The suggested error handling in the review comment is valid.
- Add error handling to
useDmPeerAddressQueryas suggested in the review comment - Keep the existing cache-only strategy as it's consistent with the codebase pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of getPeerAddressFromTopic and cache strategy
# Check if there are other instances of real-time XMTP queries
echo "Checking for other XMTP query implementations..."
rg -l "useQuery.*xmtp" --type typescript
# Look for error handling patterns in other query hooks
echo "Checking error handling patterns in other query hooks..."
rg "useQuery.*error" --type typescript
# Check cache configuration patterns
echo "Checking cache configuration patterns..."
rg "cacheOnlyQueryOptions" --type typescript -A 5
Length of output: 559
Script:
#!/bin/bash
# Let's try without typescript file type specification
# Check if there are other instances of real-time XMTP queries
echo "Checking for other XMTP query implementations..."
rg -l "useQuery.*xmtp"
# Look for error handling patterns in other query hooks
echo "Checking error handling patterns in other query hooks..."
rg "useQuery.*error"
# Check cache configuration patterns
echo "Checking cache configuration patterns..."
rg "cacheOnlyQueryOptions" -A 5
# Check the implementation of getPeerAddressFromTopic
echo "Checking getPeerAddressFromTopic implementation..."
rg "getPeerAddressFromTopic" -A 10
# Look for any existing error handling in query hooks
echo "Checking existing error handling in query hooks..."
rg "onError.*useQuery"
Length of output: 4889
features/conversation-list/hooks/useMessageIsUnread.ts (2)
2-2: LGTM! Clean import organization
The imports are well-organized and properly scoped to the requirements of the refactored implementation.
Also applies to: 7-8
25-25: Consider test implications of store dependency
While moving topicsData to the store reduces prop drilling, it introduces an implicit store dependency. Ensure that tests are updated to mock the store appropriately.
Let's check for existing tests that might need updates:
screens/Conversation.tsx (2)
1-1: LGTM: Import changes align with feature organization
The relocation of conversation components to the features directory improves code organization and modularity.
Also applies to: 7-8
11-13: Verify removal of navigation prop impact
The navigation prop has been removed from the component parameters. While this aligns with the component refactoring, we should verify that child components don't require navigation functionality.
✅ Verification successful
Navigation prop removal is safe
The removal of the navigation prop from component parameters is safe because both V3Conversation components (in features/conversations/components/ and components/Conversation/) use the useNavigation hook internally to access navigation functionality. This is actually a better practice as it follows React Navigation's recommended approach of using hooks for navigation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for navigation usage in V3Conversation and V3ConversationFromPeer
ast-grep --pattern 'navigation.$_' ../features/conversations/components/V3Conversation.tsx ../features/conversations/components/V3ConversationFromPeer.tsx
# Search for useNavigation hook usage
rg -l "useNavigation" ../features/conversations/components/
Length of output: 508
Script:
#!/bin/bash
# First, let's find the correct paths using fd
fd -t f "V3Conversation.tsx|V3ConversationFromPeer.tsx"
# Search for navigation usage in the entire codebase
ast-grep --pattern 'navigation.$_'
# Search for useNavigation hook usage
rg -l "useNavigation"
Length of output: 6385
features/conversations/components/ConversationTitleDumb.tsx (2)
1-56: Verify integration with other conversation components
The component implementation looks solid, but let's verify its integration with the new group conversation features mentioned in the PR description.
✅ Verification successful
Integration verified successfully
The component is properly integrated within the conversation feature:
- Used as the base component by both
DmConversationTitleandGroupConversationTitle - Correctly imported and implemented in both the new features directory and legacy components
- No duplicate implementations found
- Properly handles both group and direct message conversation scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how this component is being used in other files
rg -t typescript -t javascript "ConversationTitleDumb|GroupConversationTitle" --glob "features/**"
# Verify no duplicate conversation title implementations exist
ast-grep --pattern 'export function $TITLE_COMPONENT({ $$$props }) { $$$body }' | rg -i 'conversation.*title'
Length of output: 247
Script:
#!/bin/bash
# Let's try a different approach to find usage and implementations
# Search for imports and usage of ConversationTitleDumb
rg "ConversationTitleDumb" -A 3 -B 3
# Search for any conversation title related components
fd -e tsx -e ts -e js . | xargs rg -l "conversation.*title" -i
# Look for similar components in the features directory
fd . features -e tsx -e ts | xargs rg -l "Title.*Conversation|Conversation.*Title" -i
Length of output: 6268
44-56: Improve style configuration and maintainability
The styles contain magic numbers and platform-specific adjustments that could be better organized.
Consider these improvements:
- Move the hardcoded Android offset to the theme:
- left: Platform.OS === "android" ? -36 : spacing.zero,
+ left: Platform.OS === "android" ? spacing.androidTitleOffset : spacing.zero,- Consider extracting platform-specific font sizes to a constant or theme:
- fontSize: Platform.OS === "ios" ? 16 * getTitleFontScale() : 18,
+ fontSize: Platform.select({
+ ios: typography.sizes.medium * getTitleFontScale(),
+ android: typography.sizes.large,
+ }),Let's check if the hardcoded values are used elsewhere:
features/conversations/components/V3ConversationFromPeer.tsx (3)
1-9: LGTM! Imports are well-organized and necessary.
The imports are logically grouped and all appear to be used within the component.
52-57: LGTM! Styles are well-structured and properly typed.
The container styles are appropriate for centering the loading indicator and follow the app's theming system.
16-21: Clarify the performance concerns in the documentation.
The comment mentions potential performance issues but doesn't specify what they are or how they might be addressed. Consider:
- What specific performance issues need attention?
- What metrics indicate a performance problem?
- What approaches could improve performance?
Let's check for potential performance bottlenecks in the component's usage:
features/conversations/hooks/useGroupMembersAvatarData.ts (2)
1-7: LGTM! Well-organized imports
The imports are logically organized and all are actively used within the hook implementation.
38-56: Verify array index correlation safety
The code assumes perfect correlation between data and memberAddresses arrays. Consider adding validation to prevent potential index mismatches.
const memberData = useMemo(() => {
+ if (data.length !== memberAddresses.length) {
+ console.warn('Data length mismatch in useGroupMembersAvatarData');
+ return [];
+ }
return data.map(({ data: socials }, index) =>
socials
? {
address: memberAddresses[index],
uri: getPreferredAvatar(socials),
name: getPreferredName(socials, memberAddresses[index]),
}
: {
address: memberAddresses[index],
uri: undefined,
name: memberAddresses[index],
}
);
}, [data, memberAddresses]);✅ Verification successful
Let me check the implementation of useProfileSocialsQueries to understand how it maintains array correlation.
Let me check the implementation of profileSocialesQueryConfig to understand how the query results are structured.
Based on the implementation details found, I can now provide the final response:
Array index correlation is safely maintained
The implementation of useProfileSocialsQueries uses React Query's useQueries hook with a direct mapping between input addresses and query configurations, which guarantees index correlation. The array order is preserved through the mapping operation, making the current implementation safe.
- The
useProfileSocialsQueriesmaps each address to its corresponding query config usingpeerAddresses.map - React Query's
useQueriesmaintains the order of results matching the input array - The subsequent mapping in
useGroupMembersAvatarDatapreserves this correlation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of useProfilesSocials to understand if it maintains array order
rg "useProfilesSocials" -A 5 -B 5
Length of output: 3399
Script:
#!/bin/bash
# Search for useProfileSocialsQueries implementation
rg "useProfileSocialsQueries" -A 10 -B 2
# Also search for any tests that might reveal the behavior
rg "useProfileSocialsQueries.*test" --glob "*.{test,spec}.*"
Length of output: 1541
Script:
#!/bin/bash
# Search for profileSocialesQueryConfig implementation
rg "profileSocialesQueryConfig" -A 10 -B 2
# Also search for useQueries usage to understand array correlation
ast-grep --pattern 'useQueries({
queries: $$$
})'
Length of output: 3460
screens/Navigation/ConversationNav.tsx (1)
Line range hint 44-67: Verify integration with GroupConversationTitle component
The empty navigation title suggests that the conversation title rendering has been moved to a different component. Let's verify this integration.
✅ Verification successful
Based on the code from V3Conversation.tsx, I can now generate the final response as we have sufficient evidence to verify the integration:
Empty navigation title is correct as GroupConversationTitle is properly integrated
The empty title in ConversationNav is intentional as the title is being set dynamically through navigation.setOptions in V3Conversation component:
- GroupConversationTitle is rendered as headerTitle for group conversations
- DmConversationTitle is rendered for direct message conversations
- The title component selection is properly handled based on conversation version
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how GroupConversationTitle is being used
ast-grep --pattern 'GroupConversationTitle'
# Check if there are any other navigation configurations that might conflict
rg -A 5 "headerTitle|title:" --type tsx
Length of output: 816
Script:
#!/bin/bash
# Search for navigation title configurations with proper file type
rg -t ts -t tsx -A 5 'headerTitle|title:'
# Look for GroupConversationTitle implementation
ast-grep --pattern 'const GroupConversationTitle = memo($$$)'
# Check for other navigation configurations in ConversationNav
rg -t ts -t tsx -A 10 'navigationOptions.*=.*{' screens/Navigation/
Length of output: 9242
Script:
#!/bin/bash
# Check where GroupConversationTitle is rendered in navigation
rg -t ts -t tsx -B 5 -A 5 'headerTitle.*GroupConversationTitle'
# Look for navigation title setup in ConversationNav with fixed regex
rg -t ts -t tsx 'navigationOptions\s*=' screens/Navigation/
# Check V3Conversation component to see how GroupConversationTitle is integrated
cat components/Conversation/V3Conversation.tsx
Length of output: 8187
components/Search/NavigationChatButton.tsx (1)
44-44: Verify the skipLoading parameter handling in the Conversation screen.
The addition of skipLoading: true looks good, but let's verify how this parameter is handled in the target screen.
✅ Verification successful
The skipLoading parameter is properly integrated and used
The parameter is correctly:
- Defined in the navigation params type (
ConversationNavParams) - Passed through to the Conversation screen
- Used with a fallback value (
route.params.skipLoading ?? true)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of skipLoading parameter in the Conversation screen
# and related components to ensure proper handling.
# Check for skipLoading parameter usage
rg -A 5 "skipLoading" "screens/"
# Check the ConversationNavParams type definition
ast-grep --pattern 'type ConversationNavParams = {
$$$
skipLoading$_
$$$
}'
Length of output: 707
Script:
#!/bin/bash
# Let's verify the component that actually uses the skipLoading prop
ast-grep --pattern 'function $_(props: {$$$skipLoading$$$}) {
$$$
}'
# Also check for any other components passing this prop
rg "skipLoading={" -A 2
Length of output: 188
Script:
#!/bin/bash
# Let's verify the component that actually uses the skipLoading prop with proper escaping
rg 'skipLoading=\{' -A 2
# Also check for any skipLoading prop type definitions
ast-grep --pattern 'interface $_ {
$$$
skipLoading$_
$$$
}'
Length of output: 240
queries/useConversationQuery.ts (2)
10-19: LGTM! Import statements are well-organized.
The new imports are properly structured and necessary for the added functionality.
73-73: Verify the impact of optional topic parameter.
The topic parameter has been made optional, which could affect dependent components. Ensure all callers handle the possibility of undefined topics correctly.
Also applies to: 80-80, 93-93
✅ Verification successful
Let me gather more context about the topic parameter and its handling.
Optional topic parameter is properly handled throughout the codebase
The codebase shows comprehensive handling of the optional topic parameter:
- All query hooks have proper null checks with
enabled: !!topicconditions - Components and mutations consistently check for topic existence before operations
- The two direct usages in V3Conversation components use non-null assertion (
topic!) indicating they're used in contexts where topic is guaranteed to exist - Multiple mutation handlers include explicit guards like
if (!topic) { return; }
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of useConversationScreenQuery to verify they handle undefined topics
ast-grep --pattern 'useConversationScreenQuery($$$)'
# Also check for direct topic access that might not handle undefined
rg -A 2 'useConversationScreenQuery.*\)'
Length of output: 1518
Script:
#!/bin/bash
# Check how topic is used in the useConversationQuery file
rg -A 3 "topic[?:]" queries/useConversationQuery.ts
# Check the type definition and usage of ConversationTopic
ast-grep --pattern 'type ConversationTopic = $$$'
# Check if there's any null checks for topic in the components
rg -A 3 "(!topic|topic\s*===\s*(undefined|null)|typeof\s+topic|topic\?)"
Length of output: 12050
queries/QueryKeys.ts (2)
7-7: LGTM! Enum addition is well-placed and consistent.
The new CONVERSATION_WITH_PEER enum value follows the established naming convention and is appropriately placed in the Conversations section.
48-52: LGTM! Query key function implementation is consistent.
The conversationWithPeerQueryKey function follows the established patterns:
- Consistent parameter types
- Account case normalization
- Array structure matching other query keys
Let's verify the usage of this new query key:
✅ Verification successful
Let me verify the implementation and usage by checking the actual implementation of the query hook.
Let me check the actual implementation of the query hook in useConversationQuery.ts to ensure proper parameter handling.
Query key implementation and usage are correctly implemented and integrated
The verification confirms that:
- The query key is properly used in
useConversationWithPeerQueryhook - The hook correctly handles the parameters (account and peer)
- The component
V3ConversationFromPeeruses the hook with correct parameters - The implementation follows the same pattern as other conversation queries in the file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation and usage of the new query key
# Expected: Find references to the new query key in query hooks/components
# Search for direct usage of CONVERSATION_WITH_PEER
rg "CONVERSATION_WITH_PEER" --type ts
# Search for usage of conversationWithPeerQueryKey function
rg "conversationWithPeerQueryKey" --type ts
# Search for potential query hook implementation
rg "useConversationWithPeer" --type ts
Length of output: 861
Script:
#!/bin/bash
# Check the implementation of useConversationWithPeerQuery
ast-grep --pattern 'export const useConversationWithPeerQuery = ($_) => {
$$$
}'
# Check the component usage for proper parameters
ast-grep --pattern 'useConversationWithPeerQuery($$$)'
Length of output: 753
Script:
#!/bin/bash
# Get the content of useConversationQuery.ts
cat queries/useConversationQuery.ts
Length of output: 2844
features/conversations/components/GroupConversationTitle.tsx (2)
92-95: LGTM! Well-structured platform-specific styling.
The styling implementation properly handles platform differences and follows the theme system.
1-90: Verify the impact of replacing ConversationTitleDumb.
According to the AI summary, this component replaces ConversationTitleDumb but still imports and uses it. Let's verify the migration status and potential breaking changes.
components/Conversation/GroupConversationTitle.tsx (2)
91-94: LGTM! Well-structured platform-specific styling
The styling implementation is clean, type-safe, and properly handles platform differences using theme values.
40-40: Consider handling the case where currentAccount is undefined
The non-null assertion (!) on currentAccount could lead to runtime errors if currentAccount is undefined.
- const currentAccount = useCurrentAccount()!;
+ const currentAccount = useCurrentAccount();
+ if (!currentAccount) return null;✅ Verification successful
Non-null assertion is safe here as the component is rendered only when authenticated
Looking at the codebase usage patterns, the non-null assertion on useCurrentAccount() in GroupConversationTitle.tsx is actually safe because:
- The hook implementation in
data/store/accountsStore.tsreturnsundefinedonly when the account is a temporary one - The component
GroupConversationTitleis rendered within conversation views that are only accessible when authenticated - The hook is consistently used with non-null assertion across similar authenticated contexts in the codebase (e.g., in V3Conversation.tsx, DmConversationTitle.tsx, etc.)
- The value is immediately used to query group name data, which would fail anyway if there was no authenticated account
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how currentAccount is handled across the codebase
rg "useCurrentAccount" -A 5 --type typescript
Length of output: 80
Script:
#!/bin/bash
# Check how currentAccount is handled across the codebase, using ts extension
rg "useCurrentAccount" -A 5 --type ts,tsx
Length of output: 72
Script:
#!/bin/bash
# Search for useCurrentAccount usage patterns in all files
rg "useCurrentAccount" -A 5
# Also find the hook definition to understand its implementation
ast-grep --pattern 'const useCurrentAccount = $_'
Length of output: 71611
ios/Podfile (1)
21-21: Verify XMTP version compatibility across the project
The XMTP version has been bumped from 3.0.4 to 3.0.6. This change affects both the main app and notification extension targets.
Let's verify the version compatibility:
Consider adding version constraints in a central configuration file to ensure coordinated updates between package.json and Podfile in the future.
✅ Verification successful
✓ XMTP version update is properly synchronized
The version bump from 3.0.4 to 3.0.6 is correctly synchronized between package.json and Podfile:
- package.json:
"@xmtp/react-native-sdk": "^3.0.6" - Podfile:
$xmtpVersion = '3.0.6'
The version is consistently applied to both the main app and notification extension targets through the shared variable in the Podfile.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if package.json has matching XMTP SDK version
rg -A 5 '"@xmtp/react-native-sdk"' package.json
# Check XMTP changelog for breaking changes
curl -s https://raw.githubusercontent.com/xmtp/xmtp-react-native/main/CHANGELOG.md | head -n 20
# Check for any security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: COCOAPODS, package: "XMTP") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 1145
Script:
#!/bin/bash
# Let's try a different approach to check for version compatibility and changes
# Check package.json version
echo "Package.json XMTP SDK version:"
rg '"@xmtp/react-native-sdk"' package.json
# Check Podfile version
echo -e "\nPodfile XMTP version:"
rg '\$xmtpVersion' ios/Podfile
# Look for any other XMTP version references
echo -e "\nOther XMTP version references:"
rg -g '!package-lock.json' -g '!yarn.lock' 'xmtp.*3\.0\.' .
# Check if there are any migration guides or breaking changes mentioned
echo -e "\nChecking for migration or breaking change mentions:"
rg -i 'breaking|migration|upgrade' -g '!{package-lock.json,yarn.lock}' -A 3 .
Length of output: 7295
package.json (1)
83-83: LGTM on SDK version bump, verify changelog for breaking changes.
The update of @xmtp/react-native-sdk from ^3.0.0 to ^3.0.6 is a patch version increment, which should only contain backwards-compatible bug fixes. This aligns with the PR's objectives for updating conversation components.
Let's verify the changelog and any potential breaking changes:
✅ Verification successful
SDK version bump is safe to proceed
The version update from 3.0.0 to 3.0.6 contains only a bug fix related to message processing, with no breaking changes. The changelog shows a single patch that improves message handling by properly awaiting processMessage and handling object types. No breaking changes were found in the repository's issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the changelog and git history for the SDK version update
# Fetch the latest release information for @xmtp/react-native-sdk
curl -s "https://api.github.com/repos/xmtp/xmtp-react-native/releases" | jq -r '.[] | select(.tag_name == "v3.0.6") | .body'
# Check if there are any issues or PRs mentioning potential breaking changes
gh api graphql -f query='
{
search(query: "repo:xmtp/xmtp-react-native is:issue label:breaking-change", type: ISSUE, first: 5) {
nodes {
... on Issue {
title
url
labels(first: 5) {
nodes {
name
}
}
}
}
}
}'
Length of output: 730
components/V3GroupConversationListItem.tsx (3)
Line range hint 190-367: Well-implemented user interaction patterns!
Great implementation of:
- Haptic feedback for enhanced mobile experience
- Clear action sheet options with proper destructive actions
- Proper cleanup and state updates
Line range hint 368-428: Excellent component implementation!
Great job on:
- Using memoization for performance optimization
- Clean separation of concerns
- Proper component composition with avatar fallbacks
- Consistent styling approach
48-49: LGTM! State management optimization.
Good optimization by selecting only the required store actions. This reduces unnecessary re-renders.
Let's verify no other store actions are needed:
✅ Verification successful
State management hooks are correctly selected
The verification confirms that setTopicsData and setPinnedConversations are the appropriate store actions needed in this component. The component uses these actions for:
- Updating conversation data (
setTopicsData) - Managing pinned conversations (
setPinnedConversations)
This matches the pattern used in similar components like V3DMListItem.tsx, which uses the same store actions for identical functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other useChatStore usages in the component
rg "useChatStore" --type typescript
Length of output: 70
Script:
#!/bin/bash
# Search for useChatStore usages with ripgrep, without file type filter
rg "useChatStore"
# Also search for any other potential store actions being used
rg "setPinnedConversations|setTopicsData"
Length of output: 8082
components/Conversation/V3Conversation.tsx (2)
100-100: Ensure useRouter provides necessary navigation methods
You replaced the navigation prop with const navigation = useRouter();. Verify that the useRouter hook provides all the navigation methods used in this component, such as navigation.setOptions and event listeners, to maintain navigation functionality.
Run the following script to check the usage of navigation methods:
✅ Verification successful
Let me verify the Next.js Router API to ensure these methods are available.
Let me check the custom navigation hook implementation to verify the available methods.
useRouter is correctly implemented with required navigation methods
The custom useRouter hook is a wrapper around React Navigation's useNavigation hook that returns a NativeStackNavigationProp object. The implementation confirms it provides all the necessary methods used in the component, including setOptions and event listeners like addListener for "beforeRemove". The change from prop to hook is safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check all usages of `navigation` in `V3Conversation.tsx`.
# Test: Search for instances of `navigation.` in the file.
# Expect: Ensure that `useRouter` provides these methods.
rg 'navigation\.' components/Conversation/V3Conversation.tsx -A 2
Length of output: 267
Script:
#!/bin/bash
# Check imports to determine which router is being used
rg "useRouter" components/Conversation/V3Conversation.tsx -B 2 -A 2
# Check if this is a Next.js or React Navigation project
fd package.json --exec cat {} | grep -E "next|@react-navigation"
Length of output: 1098
Script:
#!/bin/bash
# Check the implementation of custom useNavigation hook
rg -g "useNavigation.{ts,tsx,js,jsx}" "export" -A 20
Length of output: 1806
Script:
#!/bin/bash
# Get more context of the useRouter implementation to see all methods
rg -g "useNavigation.{ts,tsx,js,jsx}" "export function useRouter" -A 50
Length of output: 2501
98-98: Verify all usages of V3Conversation are updated with new props
The V3Conversation component now accepts topic and an optional textPrefill instead of route and navigation. Ensure that all instances where V3Conversation is used have been updated to pass the correct props to prevent runtime errors due to incorrect prop usage.
Run the following script to find all usages of V3Conversation and check their props:
| /> | ||
| ); | ||
| } | ||
| return <VStack />; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error state handling
The empty VStack fallback provides no feedback to users when route parameters are invalid or missing.
- return <VStack />;
+ return (
+ <VStack spacing={4} p={4}>
+ <Text>Unable to load conversation</Text>
+ <Text variant="caption">Invalid or missing conversation parameters</Text>
+ </VStack>
+ );📝 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.
| return <VStack />; | |
| return ( | |
| <VStack spacing={4} p={4}> | |
| <Text>Unable to load conversation</Text> | |
| <Text variant="caption">Invalid or missing conversation parameters</Text> | |
| </VStack> | |
| ); |
| if (route.params?.mainConversationWithPeer) { | ||
| return ( | ||
| <V3ConversationFromPeer | ||
| peer={route.params.mainConversationWithPeer} | ||
| textPrefill={route.params.text} | ||
| skipLoading={route.params.skipLoading ?? true} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document skipLoading behavior and add peer validation
The skipLoading parameter defaults to true, but its purpose and implications aren't clear. Additionally, peer validation should be added.
+ // Validate peer parameter
+ if (!route.params?.mainConversationWithPeer?.id) {
+ console.warn('Invalid peer provided');
+ return <VStack />;
+ }
if (route.params?.mainConversationWithPeer) {
return (
<V3ConversationFromPeer
peer={route.params.mainConversationWithPeer}
textPrefill={route.params.text}
+ // skipLoading: true skips initial loading state for better UX
skipLoading={route.params.skipLoading ?? true}
/>
);
}📝 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.
| if (route.params?.mainConversationWithPeer) { | |
| return ( | |
| <V3ConversationFromPeer | |
| peer={route.params.mainConversationWithPeer} | |
| textPrefill={route.params.text} | |
| skipLoading={route.params.skipLoading ?? true} | |
| /> | |
| ); | |
| } | |
| // Validate peer parameter | |
| if (!route.params?.mainConversationWithPeer?.id) { | |
| console.warn('Invalid peer provided'); | |
| return <VStack />; | |
| } | |
| if (route.params?.mainConversationWithPeer) { | |
| return ( | |
| <V3ConversationFromPeer | |
| peer={route.params.mainConversationWithPeer} | |
| textPrefill={route.params.text} | |
| // skipLoading: true skips initial loading state for better UX | |
| skipLoading={route.params.skipLoading ?? true} | |
| /> | |
| ); | |
| } |
| // Add an animation since we are loading certain information and makes it feel faster | ||
| <AnimatedHStack style={$container} entering={animations.fadeInUpSlow()}> | ||
| <TouchableOpacity | ||
| onLongPress={onLongPress} | ||
| onPress={onPress} | ||
| style={themed($touchableContainer)} | ||
| > | ||
| {avatarComponent} | ||
| <Text style={themed($title)} numberOfLines={1} allowFontScaling={false}> | ||
| {title} | ||
| </Text> | ||
| </TouchableOpacity> | ||
| </AnimatedHStack> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and documentation
The component should include accessibility attributes for better screen reader support.
Apply these improvements:
<TouchableOpacity
onLongPress={onLongPress}
onPress={onPress}
style={themed($touchableContainer)}
+ accessibilityRole="button"
+ accessibilityLabel={`Conversation ${title}`}
+ accessibilityHint="Double tap to view conversation details"
>
{avatarComponent}
<Text
style={themed($title)}
numberOfLines={1}
allowFontScaling={false}
+ accessibilityRole="header"
>
{title}
</Text>
</TouchableOpacity>Also, improve the animation comment to better explain its purpose:
-// Add an animation since we are loading certain information and makes it feel faster
+// Animate the title entry to provide visual feedback while conversation data is loading📝 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.
| // Add an animation since we are loading certain information and makes it feel faster | |
| <AnimatedHStack style={$container} entering={animations.fadeInUpSlow()}> | |
| <TouchableOpacity | |
| onLongPress={onLongPress} | |
| onPress={onPress} | |
| style={themed($touchableContainer)} | |
| > | |
| {avatarComponent} | |
| <Text style={themed($title)} numberOfLines={1} allowFontScaling={false}> | |
| {title} | |
| </Text> | |
| </TouchableOpacity> | |
| </AnimatedHStack> | |
| // Animate the title entry to provide visual feedback while conversation data is loading | |
| <AnimatedHStack style={$container} entering={animations.fadeInUpSlow()}> | |
| <TouchableOpacity | |
| onLongPress={onLongPress} | |
| onPress={onPress} | |
| style={themed($touchableContainer)} | |
| accessibilityRole="button" | |
| accessibilityLabel={`Conversation ${title}`} | |
| accessibilityHint="Double tap to view conversation details" | |
| > | |
| {avatarComponent} | |
| <Text | |
| style={themed($title)} | |
| numberOfLines={1} | |
| allowFontScaling={false} | |
| accessibilityRole="header" | |
| > | |
| {title} | |
| </Text> | |
| </TouchableOpacity> | |
| </AnimatedHStack> |
| export const V3ConversationFromPeer = memo( | ||
| ({ peer, textPrefill, skipLoading }: V3ConversationFromPeerProps) => { | ||
| const currentAccount = useCurrentAccount()!; | ||
|
|
||
| const { data: conversation, isLoading } = useConversationWithPeerQuery( | ||
| currentAccount, | ||
| peer, | ||
| { | ||
| enabled: !skipLoading, | ||
| } | ||
| ); | ||
|
|
||
| const { themed } = useAppTheme(); | ||
| if (isLoading && !skipLoading) { | ||
| return ( | ||
| <VStack style={themed($container)}> | ||
| <ActivityIndicator /> | ||
| </VStack> | ||
| ); | ||
| } | ||
| return ( | ||
| <V3Conversation | ||
| peerAddress={peer} | ||
| topic={conversation?.topic} | ||
| textPrefill={textPrefill} | ||
| /> | ||
| ); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Address potential runtime issues and add error handling.
Several improvements could make the component more robust:
- The non-null assertion on
currentAccountcould lead to runtime errors. - Missing error handling for the query.
- The
topicprop type should be verified.
Consider applying these changes:
export const V3ConversationFromPeer = memo(
({ peer, textPrefill, skipLoading }: V3ConversationFromPeerProps) => {
- const currentAccount = useCurrentAccount()!;
+ const currentAccount = useCurrentAccount();
+ if (!currentAccount) {
+ return null; // or an error state component
+ }
- const { data: conversation, isLoading } = useConversationWithPeerQuery(
+ const { data: conversation, isLoading, error } = useConversationWithPeerQuery(
currentAccount,
peer,
{
enabled: !skipLoading,
}
);
const { themed } = useAppTheme();
+ if (error) {
+ return (
+ <VStack style={themed($container)}>
+ <Text>Error loading conversation</Text>
+ </VStack>
+ );
+ }
if (isLoading && !skipLoading) {Committable suggestion skipped: line range outside the PR's diff.
| const currentAccount = useCurrentAccount()!; | ||
| const { data: members, ...query } = useGroupMembersConversationScreenQuery( | ||
| currentAccount, | ||
| topic | ||
| ); |
There was a problem hiding this comment.
Add error handling for undefined currentAccount
The non-null assertion (!) on currentAccount could lead to runtime errors if the account is undefined. Consider adding proper error handling.
- const currentAccount = useCurrentAccount()!;
+ const currentAccount = useCurrentAccount();
+ if (!currentAccount) {
+ return { data: [], isLoading: false, isError: true, error: new Error('No current account found') };
+ }📝 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 currentAccount = useCurrentAccount()!; | |
| const { data: members, ...query } = useGroupMembersConversationScreenQuery( | |
| currentAccount, | |
| topic | |
| ); | |
| const currentAccount = useCurrentAccount(); | |
| if (!currentAccount) { | |
| return { data: [], isLoading: false, isError: true, error: new Error('No current account found') }; | |
| } | |
| const { data: members, ...query } = useGroupMembersConversationScreenQuery( | |
| currentAccount, | |
| topic | |
| ); |
| const conversationContextValue = useMemo( | ||
| () => ({ | ||
| topic, | ||
| conversation: undefined, | ||
| messageToPrefill, | ||
| inputRef: textInputRef, | ||
| mediaPreviewToPrefill: null, | ||
| mediaPreviewRef, | ||
| isBlockedPeer: false, | ||
| onReadyToFocus, | ||
| frameTextInputFocused, | ||
| setFrameTextInputFocused, | ||
| tagsFetchedOnceForMessage, | ||
| }), | ||
| [topic, messageToPrefill, onReadyToFocus, frameTextInputFocused] | ||
| ); |
There was a problem hiding this comment.
Pass the actual conversation to ConversationContext.Provider
In conversationContextValue, conversation is set to undefined (line 264~). If child components rely on conversation from the context, this may cause errors.
Consider passing the conversation object obtained from useData:
const conversationContextValue = useMemo(
() => ({
topic,
- conversation: undefined,
+ conversation,
messageToPrefill,
inputRef: textInputRef,
mediaPreviewToPrefill: null,
mediaPreviewRef,
isBlockedPeer: false,
onReadyToFocus,
frameTextInputFocused,
setFrameTextInputFocused,
tagsFetchedOnceForMessage,
}),
- [topic, messageToPrefill, onReadyToFocus, frameTextInputFocused]
+ [topic, conversation, messageToPrefill, onReadyToFocus, frameTextInputFocused]
);Committable suggestion skipped: line range outside the PR's diff.
| `[XMTPRN Conversations] Got conversation by topic in ${ | ||
| (end - start) / 1000 | ||
| } sec` | ||
| ); | ||
| return conversation; |
There was a problem hiding this comment.
Correct log message to reflect 'peer' instead of 'topic'
The log message in the getConversationByPeer function incorrectly states "Got conversation by topic". It should reflect "Got conversation by peer" to match the function's purpose.
Apply this diff to correct the log message:
logger.debug(
- `[XMTPRN Conversations] Got conversation by topic in ${
+ `[XMTPRN Conversations] Got conversation by peer in ${
(end - start) / 1000
} sec`
);📝 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.
| `[XMTPRN Conversations] Got conversation by topic in ${ | |
| (end - start) / 1000 | |
| } sec` | |
| ); | |
| return conversation; | |
| `[XMTPRN Conversations] Got conversation by peer in ${ | |
| (end - start) / 1000 | |
| } sec` | |
| ); | |
| return conversation; |
| const client = (await getXmtpClient(account)) as ConverseXmtpClientType; | ||
| return getConversationByPeer({ client, peer, includeSync }); | ||
| }; |
There was a problem hiding this comment.
Add null check for 'client' to prevent potential runtime errors
In getConversationByPeerByAccount, there is no check to ensure that client is not null before proceeding. This could lead to runtime errors if client is undefined.
Apply this diff to add a null check:
const client = (await getXmtpClient(account)) as ConverseXmtpClientType;
+ if (!client) {
+ throw new Error("Client not found");
+ }
return getConversationByPeer({ client, peer, includeSync });📝 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 client = (await getXmtpClient(account)) as ConverseXmtpClientType; | |
| return getConversationByPeer({ client, peer, includeSync }); | |
| }; | |
| const client = (await getXmtpClient(account)) as ConverseXmtpClientType; | |
| if (!client) { | |
| throw new Error("Client not found"); | |
| } | |
| return getConversationByPeer({ client, peer, includeSync }); | |
| }; |
| if (dm.version === ConversationVersion.DM) { | ||
| const peerAddress = await getPeerAddressDm(dm); | ||
| return peerAddress; | ||
| } | ||
| throw new Error("Conversation is not a DM"); | ||
| }; |
There was a problem hiding this comment.
Ensure 'peerAddress' is defined before returning
In getPeerAddressFromTopic, the function returns the peerAddress obtained from getPeerAddressDm. If peerAddress is undefined, it could lead to unexpected behavior. Consider handling the case where peerAddress is undefined.
Apply this diff to handle the undefined case:
if (dm.version === ConversationVersion.DM) {
const peerAddress = await getPeerAddressDm(dm);
+ if (!peerAddress) {
+ throw new Error("Peer address not found");
+ }
return peerAddress;
}
throw new Error("Conversation is not a DM");📝 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.
| if (dm.version === ConversationVersion.DM) { | |
| const peerAddress = await getPeerAddressDm(dm); | |
| return peerAddress; | |
| } | |
| throw new Error("Conversation is not a DM"); | |
| }; | |
| if (dm.version === ConversationVersion.DM) { | |
| const peerAddress = await getPeerAddressDm(dm); | |
| if (!peerAddress) { | |
| throw new Error("Peer address not found"); | |
| } | |
| return peerAddress; | |
| } | |
| throw new Error("Conversation is not a DM"); | |
| }; |
| export const getPeerAddressDm = async (dm: DmWithCodecsType) => { | ||
| const peerInboxId = await dm.peerInboxId(); | ||
| const peerAddress = (await dm.members()).find( | ||
| (member) => member.inboxId === peerInboxId | ||
| )?.addresses[0]; | ||
| return peerAddress; | ||
| }; |
There was a problem hiding this comment.
Handle the case where 'peerAddress' might be undefined
In getPeerAddressDm, if the peer address is not found, the function may return undefined. Ensure that the calling code handles this possibility to prevent runtime errors.
Consider adding a check to handle cases when peerAddress is undefined:
const peerAddress = (await dm.members()).find(
(member) => member.inboxId === peerInboxId
)?.addresses[0];
+ if (!peerAddress) {
+ throw new Error("Peer address not found");
+ }
return peerAddress;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getPeerAddressDm = async (dm: DmWithCodecsType) => { | |
| const peerInboxId = await dm.peerInboxId(); | |
| const peerAddress = (await dm.members()).find( | |
| (member) => member.inboxId === peerInboxId | |
| )?.addresses[0]; | |
| return peerAddress; | |
| }; | |
| export const getPeerAddressDm = async (dm: DmWithCodecsType) => { | |
| const peerInboxId = await dm.peerInboxId(); | |
| const peerAddress = (await dm.members()).find( | |
| (member) => member.inboxId === peerInboxId | |
| )?.addresses[0]; | |
| if (!peerAddress) { | |
| throw new Error("Peer address not found"); | |
| } | |
| return peerAddress; | |
| }; |
Updated new conversation Handling
Moved components to be in features branch
Updated title components
Bumped RN SDK
Commented out api header work
Summary by CodeRabbit
Release Notes
New Features
GroupConversationTitleandDmConversationTitlecomponents for enhanced conversation title display with avatars.V3ConversationandV3ConversationFromPeercomponents for improved conversation management.useConversationWithPeerQueryfor fetching conversations based on peer identifiers.Bug Fixes
Chores
package.jsonandPodfilefor improved performance and stability.