feat: V3 Split Logic #1089
feat: V3 Split Logic #1089
Conversation
WalkthroughThis pull request introduces multiple new React components and custom hooks primarily focused on enhancing the chat interface and conversation management within the application. Key components include 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 (
|
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: 58
🧹 Outside diff range and nitpick comments (59)
utils/appState/appStateIsBlurred.ts (1)
1-6: Add JSDoc documentation for the exported function.Adding documentation will improve code maintainability and provide better IDE support.
import { AppStateStatus } from "react-native"; const blurredStateRegex = /^(?:inactive|background)$/; +/** + * Checks if the React Native app state indicates the app is blurred (inactive or in background) + * @param appState - The current state of the React Native app + * @returns true if the app is in an inactive or background state, false otherwise + */ export const appStateIsBlurred = (appState: AppStateStatus): boolean => blurredStateRegex.test(appState);queries/useRefreshOnFocus.ts (1)
4-4: Consider adding type constraints for better type safety.The generic type parameter could benefit from constraints to ensure proper usage.
-export function useRefreshOnFocus<T>(refetch: () => Promise<T>) { +export function useRefreshOnFocus<T extends Record<string, unknown>>(refetch: () => Promise<T>) {hooks/useApprovedGroupsConversationList.ts (1)
5-19: Add TypeScript type definitions for better type safety.The hook lacks type definitions for its data structure and return type, which could make it harder to maintain and use correctly.
Consider adding these type definitions:
interface Group { state: 'allowed' | 'pending' | 'rejected'; // add other group properties } interface GroupsData { ids: string[]; byId: Record<string, Group>; } interface UseApprovedGroupsResult { data: GroupsData | undefined; isLoading: boolean; error: Error | null; // add other query result properties } export const useApprovedGroupsConversationList = (): UseApprovedGroupsResult => { // ... existing implementation }utils/mergeLists.ts (2)
1-6: Add input validation and improve documentation.While the type safety is good, consider adding runtime validation for the ordering functions and JSDoc documentation to clarify the expected behavior.
+/** + * Merges two ordered lists based on their respective ordering functions. + * @param list1 First list to merge + * @param list2 Second list to merge + * @param list1Order Function that returns a numeric order value for elements in list1 + * @param list2Order Function that returns a numeric order value for elements in list2 + * @returns Merged array containing elements from both lists in ascending order + * @throws {Error} If ordering functions return non-numeric values + */ export function mergeOrderedLists<L1, L2>( list1: L1[], list2: L2[], list1Order: (a: L1) => number, list2Order: (a: L2) => number ): (L1 | L2)[] { + // Validate first elements to ensure ordering functions return numbers + if (list1.length && isNaN(list1Order(list1[0]))) { + throw new Error('list1Order must return a number'); + } + if (list2.length && isNaN(list2Order(list2[0]))) { + throw new Error('list2Order must return a number'); + }
21-31: Consider using spread operator for remaining elements.The current implementation is correct, but could be more concise using the spread operator.
- while (i < list1.length) { - merged.push(list1[i]); - i++; - } - while (j < list2.length) { - merged.push(list2[j]); - j++; - } + merged.push(...list1.slice(i)); + merged.push(...list2.slice(j));queries/useGroupMessages.ts (1)
11-24: Consider adding TypeScript types for better type safety.The query implementation looks good, but could benefit from explicit typing for better maintainability and type safety.
Consider adding types:
interface GroupMessage { id: string; // add other message properties } interface QueryResult { ids: string[]; byId: Record<string, GroupMessage>; }utils/appState/waitUntilAppActive.ts (1)
Line range hint
10-33: Consider adding timeout and improving error handling.While the implementation is solid, consider these improvements for robustness:
- Add a maximum timeout to prevent infinite waiting
- Extract magic number
endDelayMsto a named constant- Add more detailed logging (e.g., previous state, retry count)
Here's a suggested implementation:
+const MAX_WAIT_TIME = 60000; // 1 minute +const DEFAULT_END_DELAY = 1000; // 1 second + export const waitUntilAppActive = async (endDelayMs: number) => { + const startTime = Date.now(); // If app is active, return immediatly if (AppState.currentState === "active") return; - logger.debug(`Waiting until app is back into active state...`); + logger.debug(`Waiting until app is back into active state from ${AppState.currentState}...`); return new Promise((resolve, reject) => { + const checkTimeout = () => { + if (Date.now() - startTime > MAX_WAIT_TIME) { + subscription.remove(); + reject(new Error('Timeout waiting for app to become active')); + return true; + } + return false; + }; + const subscription = AppState.addEventListener( "change", async (nextAppState) => { if (nextAppState === "active") { subscription.remove(); logger.debug( - `App was inactive for a while, waiting ${endDelayMs}ms more` + `App transitioned to active, waiting ${endDelayMs}ms for stability` ); await new Promise((r) => setTimeout(r, endDelayMs)); // Check if app is still active after endDelayMs if (AppState.currentState !== "active") { + if (checkTimeout()) return; await waitUntilAppActive(endDelayMs); } else { resolve(undefined); } } } ); + // Initial timeout check + checkTimeout(); }); };components/Chat/Message/TextMessage.tsx (1)
1-56: Add test coverage for the TextMessage component.This component lacks test coverage. Consider adding tests for:
- Rendering with different prop combinations
- Content validation
- User interactions
- Color scheme changes
Would you like me to help create a test file with the necessary test cases?
components/Chat/Message/MessageSender.tsx (1)
11-18: Add accessibility support to MessageSenderDumb component.While the component is appropriately "dumb" as intended, consider adding accessibility props to improve user experience for screen readers.
const MessageSenderDumb = ({ name }: MessageSenderDumbProps) => { const styles = useStyles(); return ( <View style={styles.groupSenderWrapper}> - <Text style={styles.groupSender}>{name}</Text> + <Text + style={styles.groupSender} + accessibilityRole="text" + accessibilityLabel={`Message sent by ${name}`} + > + {name} + </Text> </View> ); };App.web.tsx (2)
32-33: Consider moving initialization into a React effectThe current module-level initialization might cause issues with:
- Testing and mocking
- Hot module replacement
- Server-side rendering
- Proper cleanup on unmount
Consider moving the initialization into a React effect:
function App() { useEffect(() => { const initializeXmtp = async () => { try { await xmtpEngine.start(); await xmtpCron.start(); } catch (error) { console.error('Failed to initialize XMTP:', error); } }; initializeXmtp(); return () => { // Add cleanup if needed xmtpEngine.stop?.(); xmtpCron.stop?.(); }; }, []); // ... rest of component }
35-35: Consider using named export for App componentFollowing modern JavaScript best practices and the linter's suggestion, consider using a named export for better maintainability and explicit imports.
-export default function App() { +export function App() {🧰 Tools
🪛 GitHub Check: lint
[warning] 35-35:
Prefer named exportscomponents/Chat/Frame/FramesPreviews.tsx (4)
Line range hint
13-15: Update Props type definition to match new implementation.The Props type definition doesn't match the component's actual usage. While the component internally passes
messageId,messageTopic, andmessageFromMeto child components, it still accepts a singlemessageprop.Update the Props type to match the new structure:
-type Props = { - message: MessageToDisplay; -}; +type Props = { + messageId: string; + messageTopic: string; + messageFromMe: boolean; +};
Line range hint
19-29: Consider using useFramesStore selector for better performance.The current implementation reinitializes the frames state by directly accessing the store state. Using a selector with useFramesStore would automatically handle updates and potentially improve performance.
Consider refactoring to use a selector:
-const [framesForMessage, setFramesForMessage] = useState<{ - [messageId: string]: FrameWithType[]; -}>({ - [message.id]: useFramesStore - .getState() - .getFramesForURLs(message.converseMetadata?.frames || []), -}); +const framesForMessage = useFramesStore( + (state) => ({ + [message.id]: state.getFramesForURLs(message.converseMetadata?.frames || []) + }) +);
Line range hint
31-40: Simplify frame fetching logic using useEffect.The current implementation mixes imperative and declarative approaches. Using useEffect would make the data fetching logic clearer and help prevent potential race conditions.
Consider refactoring to use useEffect:
-const fetchTagsIfNeeded = useCallback(() => { - if (!tagsFetchedOnceForMessage.current[message.id]) { - tagsFetchedOnceForMessage.current[message.id] = true; - fetchFramesForMessage(account, message).then( - (frames: FramesForMessage) => { - setFramesForMessage({ [frames.messageId]: frames.frames }); - } - ); - } -}, [account, message, tagsFetchedOnceForMessage]); +useEffect(() => { + if (!tagsFetchedOnceForMessage.current[message.id]) { + tagsFetchedOnceForMessage.current[message.id] = true; + fetchFramesForMessage(account, message).then( + (frames: FramesForMessage) => { + setFramesForMessage({ [frames.messageId]: frames.frames }); + } + ); + } +}, [message.id, account]);
Line range hint
42-50: Optimize component recycling logic to prevent unnecessary updates.The current implementation might cause unnecessary re-renders by updating state during render. This pattern could be simplified using useEffect.
Consider refactoring to:
-if (message.id !== messageId.current) { - messageId.current = message.id; - fetchTagsIfNeeded(); - setFramesForMessage({ - [message.id]: useFramesStore - .getState() - .getFramesForURLs(message.converseMetadata?.frames || []), - }); -} +useEffect(() => { + messageId.current = message.id; + fetchTagsIfNeeded(); + setFramesForMessage({ + [message.id]: useFramesStore + .getState() + .getFramesForURLs(message.converseMetadata?.frames || []), + }); +}, [message.id]);screens/Group.tsx (2)
25-25: Consider documenting the store selector usage.The switch from
useCurrentAccounttouseChatStorerepresents a more consolidated approach to state management. However, the specific conversation selector could benefit from a brief comment explaining the type assertion toXmtpGroupConversation.const group = useChatStore( + // Safely assert group conversation type as we're in the Group screen (s) => s.conversations[route.params.topic] ) as XmtpGroupConversation;Also applies to: 30-33
Line range hint
35-54: LGTM: Clean component structure maintained.The component maintains a clean separation of concerns by delegating specific functionalities to sub-components. This modular approach aligns well with the PR's goal of creating "dumb components" that can support both V2 and V3 logic.
This pattern of breaking down the UI into focused sub-components will make it easier to:
- Gradually migrate components to V3
- Maintain parallel V2/V3 implementations where needed
- Test components in isolation
queries/useGroupsQuery.ts (2)
15-15: Remove debug console.log statement.The console.log statement appears to be debugging code that should be removed before merging.
- console.log("here2222");
23-37: Consider preserving performance logging.The commented-out code contains valuable performance logging that could be useful for debugging and monitoring. Consider keeping this logging functionality even with the new implementation.
+ const beforeSync = new Date().getTime(); // New implementation here + const afterSync = new Date().getTime(); + logger.debug( + `[Groups] Operation completed in ${(afterSync - beforeSync) / 1000} sec` + );data/store/appStore.ts (1)
22-28: Consider documenting the one-way state transition pattern.These state transitions (
setSplashScreenHiddenandsetHydrationDone) represent important architectural decisions. Consider adding JSDoc comments to explain why these states can only transition in one direction.Apply this diff to add documentation:
splashScreenHidden: boolean; + /** Sets splash screen as hidden. This is a one-way operation - once hidden, the splash screen cannot be shown again. */ setSplashScreenHidden: (hidden: true) => void; isInternetReachable: boolean; setIsInternetReachable: (reachable: boolean) => void; hydrationDone: boolean; + /** Marks hydration as complete. This is a one-way operation - once hydrated, the state cannot be reversed. */ setHydrationDone: (done: true) => void;queries/useGroupMembersQuery.ts (1)
37-38: Track multiple addresses support requirement.The TODO comment indicates a pending requirement for multiple addresses support. This should be tracked and addressed in future updates.
Would you like me to create a GitHub issue to track the implementation of multiple addresses support?
components/Chat/Message/MessageSenderAvatar.tsx (1)
24-47: Add accessibility support and prop documentation.The component could benefit from:
- Accessibility props for better screen reader support
- JSDoc documentation for the props interface
- Loading and error states for avatar
Consider these improvements:
type MessageSenderAvatarDumbProps = { + /** Flag to determine if this message is part of a series from the same sender */ hasNextMessageInSeries: boolean; + /** Callback fired when avatar is pressed */ onPress: () => void; + /** URI for the avatar image */ avatarUri: string | undefined; + /** Name to display for the avatar */ avatarName: string; }; export const MessageSenderAvatarDumb = ({ hasNextMessageInSeries, onPress, avatarUri, avatarName, }: MessageSenderAvatarDumbProps) => { const styles = useStyles(); return ( <View style={styles.groupSenderAvatarWrapper}> {!hasNextMessageInSeries ? ( - <TouchableOpacity onPress={onPress}> + <TouchableOpacity + onPress={onPress} + accessibilityRole="button" + accessibilityLabel={`View ${avatarName}'s profile`} + > <Avatar size={AvatarSizes.messageSender} uri={avatarUri} name={avatarName} /> </TouchableOpacity> ) : ( - <View style={styles.avatarPlaceholder} /> + <View + style={styles.avatarPlaceholder} + accessibilityElementsHidden={true} + /> )} </View> ); };components/Chat/Frame/FrameBottom.tsx (3)
Line range hint
40-45: Fix incorrect message.fromMe reference in backgroundColor styling.The backgroundColor calculation still uses the old
message.fromMesyntax which will cause a runtime error.Apply this fix:
backgroundColor: message.fromMe ? myMessageInnerBubbleColor(colorScheme) : messageInnerBubbleColor(colorScheme),should be:
backgroundColor: messageFromMe ? myMessageInnerBubbleColor(colorScheme) : messageInnerBubbleColor(colorScheme),
Line range hint
61-76: Consider extracting button press handler to useCallback.The button press handler with haptic feedback is recreated on every render. Consider optimizing it with useCallback.
Add this at the top of the file:
+import { useCallback } from "react";Then refactor the handler:
+const handleButtonPress = useCallback((button: FrameButtonType) => { + if (Platform.OS !== "web") { + Haptics.impactAsync(); + } + onButtonPress(button); +}, [onButtonPress]); // In the JSX: -onPress={() => { - if (Platform.OS !== "web") { - Haptics.impactAsync(); - } - onButtonPress(button); -}} +onPress={() => handleButtonPress(button)}
Line range hint
35-45: Consider extracting style computation logic.The backgroundColor computation logic in the style prop could be moved to the useStyles hook for better maintainability.
Consider refactoring like this:
const useStyles = (messageFromMe: boolean, frameType: string, colorScheme: ColorScheme) => { return StyleSheet.create({ frameBottom: { flexDirection: "row", flexWrap: "wrap", paddingHorizontal: 8, borderBottomLeftRadius: 14, borderBottomRightRadius: 14, + backgroundColor: messageFromMe + ? myMessageInnerBubbleColor(colorScheme) + : messageInnerBubbleColor(colorScheme), + paddingVertical: + frameType === "PREVIEW" ? 3 : frameType === "XMTP_FRAME" ? 4 : 0, }, // ... rest of the styles }); };Then update the usage:
-const styles = useStyles(); +const styles = useStyles(messageFromMe, frame.type, colorScheme); // In the JSX: -<View - style={[ - styles.frameBottom, - { - backgroundColor: messageFromMe - ? myMessageInnerBubbleColor(colorScheme) - : messageInnerBubbleColor(colorScheme), - paddingVertical: - frame.type === "PREVIEW" ? 3 : frame.type === "XMTP_FRAME" ? 4 : 0, - }, - ]} -> +<View style={styles.frameBottom}>components/ConversationListItem/ConversationListItem.tsx (2)
56-58: Add accessibility props to Pressable.The Pressable component should include accessibility props for better user experience.
- <Pressable onPress={onPress} onLongPress={onLongPress}> + <Pressable + onPress={onPress} + onLongPress={onLongPress} + accessible={true} + accessibilityRole="button" + accessibilityHint="Opens conversation details">
107-115: Remove or document commented code.There's a commented reference to
PictoTitleSubtitleAll. If this component will be implemented later, consider adding a TODO comment with more context. If not needed, remove the comment.export const ConversationListItem = { - // All: PictoTitleSubtitleAll, Accessory: AccessoryComponent, Left: LeftComponent,data/db/index.ts (2)
Line range hint
89-98: Document the PRAGMA statements and their performance implications.The SQLite optimization settings are crucial for performance but lack documentation explaining their impact and chosen values.
Add comments explaining each PRAGMA statement:
await Promise.all([ dataSource.query( - "PRAGMA journal_mode=WAL;PRAGMA synchronous=normal;PRAGMA temp_store=memory;" + // WAL mode for better concurrency, normal sync for better performance + // Store temp tables in memory for faster operations + "PRAGMA journal_mode=WAL;PRAGMA synchronous=normal;PRAGMA temp_store=memory;" ), dataSource.query("PRAGMA synchronous=normal;"), dataSource.query("PRAGMA temp_store=memory;"), + // Set maximum memory map size to ~30GB for better read performance dataSource.query("PRAGMA mmap_size=30000000000;"), ]);
Line range hint
190-196: Consider adding a grace period before database deletion.The immediate database deletion might interrupt ongoing operations. Consider adding a short delay or checking for active connections before deletion.
Here's a suggested implementation:
if (env !== "dev") { // Won't clear db in dev mode so Testflight users still have access logger.debug(`[ClearDB] Deleting SQlite file ${dbPath}`); + // Wait for potential ongoing operations to complete + await new Promise(resolve => setTimeout(resolve, 1000)); + // Check if there are any active connections + const activeConnections = await dataSource.query('PRAGMA database_list;'); + if (activeConnections.length > 0) { + logger.debug(`[ClearDB] Waiting for ${activeConnections.length} connections to close`); + await new Promise(resolve => setTimeout(resolve, 2000)); + } await RNFS.unlink(dbPath); logger.debug(`[ClearDB] Deleted SQlite file ${dbPath}`); }screens/ConversationList.tsx (1)
Line range hint
1-300: Architecture aligns well with V3 split logic objectives.The implementation successfully:
- Maintains backward compatibility with V2 conversations
- Introduces V3 group support through a clean abstraction
- Minimizes changes to the existing codebase
Consider documenting the V2/V3 split architecture decisions in the codebase to help future maintainers understand the design choices.
Would you like me to help draft architecture documentation that explains:
- The V2/V3 split strategy
- How conversations are merged and displayed
- Future considerations for V3 message implementation
screens/Conversation.tsx (2)
334-342: Consider adding error handling for edge cases.The HOC implementation effectively routes between V2 and V3 components based on topic type, aligning with the PR objectives. However, consider handling potential edge cases:
- When route.params is undefined
- When topic is malformed
Consider this safer implementation:
const ConversationHoc = ({ route, navigation, }: NativeStackScreenProps<NavigationParamList, "Conversation">) => { - if (route.params?.topic && isGroupTopic(route.params.topic)) { + try { + const topic = route.params?.topic; + if (topic && isGroupTopic(topic)) { + return <V3Conversation route={route} navigation={navigation} />; + } + } catch (error) { + console.error('Error determining conversation type:', error); + } return <Conversation route={route} navigation={navigation} />; };
334-342: Add documentation for the V2/V3 routing strategy.While the implementation effectively handles V2/V3 routing, adding documentation would help other developers understand:
- The routing logic and its purpose
- When to use V2 vs V3 conversations
- Migration path for future V3 transitions
Add JSDoc comments to explain the routing strategy:
+/** + * Higher-order component that routes between V2 and V3 conversation implementations. + * V3 is used for group topics, while V2 handles all other conversation types. + * This separation enables gradual migration from V2 to V3 architecture. + */ const ConversationHoc = ({ route, navigation, }: NativeStackScreenProps<NavigationParamList, "Conversation">) => {components/GroupAvatar.tsx (1)
134-189: Document the V2/V3 separation strategyWhile the implementation successfully separates presentation logic, consider adding documentation that explains:
- The relationship between V2 and V3 components
- When to use
GroupAvatarvsGroupAvatarDumb- Migration path for existing usage
This will help other developers understand the architectural decisions and facilitate the transition.
utils/xmtpRN/messages.ts (3)
Line range hint
247-248: Consider refactoring global state management for group members.The use of a global
groupMembersobject for state management introduces several risks:
- No visible cleanup mechanism could lead to memory leaks
- Potential race conditions in concurrent operations
- Risk of state inconsistency
Consider these alternatives:
- Use React's state management (e.g., Context, Redux) for better lifecycle management
- Implement a proper cache with TTL and cleanup
- Pass group members as parameters to avoid global state
Example implementation of a cache with cleanup:
class GroupMembersCache { private cache: { [topic: string]: { members: Member[], timestamp: number } } = {}; private TTL = 1000 * 60 * 60; // 1 hour set(topic: string, members: Member[]) { this.cache[topic] = { members, timestamp: Date.now() }; this.cleanup(); } get(topic: string): Member[] | undefined { const entry = this.cache[topic]; if (!entry) return undefined; if (Date.now() - entry.timestamp > this.TTL) { delete this.cache[topic]; return undefined; } return entry.members; } private cleanup() { const now = Date.now(); Object.keys(this.cache).forEach(topic => { if (now - this.cache[topic].timestamp > this.TTL) { delete this.cache[topic]; } }); } }Also applies to: 253-259
Line range hint
261-275: Enhance error handling for message decoding failures.While the current error handling prevents complete failure when decoding messages, consider these improvements:
- Add more context to Sentry error reporting
- Implement user notification for failed messages
- Consider retry mechanism for transient failures
messages.forEach((message) => { try { xmtpMessages.push(protocolMessageToStateMessage(message)); } catch (e) { sentryTrackError(e, { error: "Could not decode message", contentType: message.contentTypeId, + messageId: message.id, + topic: message.topic, + timestamp: message.sent, + error_details: e.message }); + // Optional: Notify user about failed message + if (config.env !== 'prod') { + logger.warn(`Failed to decode message ${message.id}`, { error: e.message }); + } } });
Line range hint
1-424: Consider architectural improvements for better separation of concerns.Given the PR's objective of implementing "dumb components" and splitting V2/V3 logic, consider these architectural improvements:
Split the file into focused modules:
messageStreaming.ts: Stream-related functionalitymessageSync.ts: Synchronization logicmessageProcessing.ts: Message processing and conversiongroupHandling.ts: Group-specific logicCreate clear boundaries between V2 and V3 logic:
- Consider using a strategy pattern to switch between V2/V3 implementations
- Add version indicators in function names or parameters
Example structure:
// messageProcessing.ts export interface MessageProcessor { processMessage(message: DecodedMessageWithCodecsType): XmtpMessage; } export class V2MessageProcessor implements MessageProcessor { processMessage(message: DecodedMessageWithCodecsType): XmtpMessage { // V2 specific logic } } export class V3MessageProcessor implements MessageProcessor { processMessage(message: DecodedMessageWithCodecsType): XmtpMessage { // V3 specific logic } } // Factory to get the appropriate processor export const getMessageProcessor = (version: 'v2' | 'v3'): MessageProcessor => { return version === 'v2' ? new V2MessageProcessor() : new V3MessageProcessor(); };This approach would:
- Make the V2/V3 split explicit
- Improve testability
- Simplify future migrations
- Reduce file complexity
components/Chat/Frame/FramePreview.tsx (5)
31-38: Props restructuring aligns well with architectural goals!Breaking down the
messageprop into specific properties (messageId,messageTopic,messageFromMe) is a good architectural decision that:
- Makes the component more focused and maintainable
- Reduces coupling with the message object structure
- Facilitates easier V2/V3 logic separation as mentioned in PR objectives
Line range hint
61-77: Consider extracting state reset logic into a separate functionWhile the state reset logic is correct, it could be more maintainable if extracted into a separate function.
Consider refactoring to:
+ const resetFrameState = useCallback(() => { + setFirstFrameLoaded(false); + setFirstImageRefreshed(false); + setFrame({ + ...initialFrame, + frameImage: undefined, + isInitialFrame: true, + uniqueId: uuidv4(), + }); + setFirstImageFailure(false); + setFrameTextInputValue(""); + }, [initialFrame]); if (messageId !== messageIdRef.current) { messageIdRef.current = messageId; - setFirstFrameLoaded(false); - setFirstImageRefreshed(false); - setFrame({ - ...initialFrame, - frameImage: undefined, - isInitialFrame: true, - uniqueId: uuidv4(), - }); - setFirstImageFailure(false); - setFrameTextInputValue(""); + resetFrameState(); }
Line range hint
88-136: Consider restructuring image handling logic for better readabilityWhile the image handling logic is thorough, it contains deeply nested conditionals that could be simplified.
Consider restructuring with early returns:
const handleInitialImage = async () => { if (!frame.isInitialFrame || firstFrameLoaded || fetchingInitialForMessageId.current) { return; } fetchingInitialForMessageId.current = messageId; const initialFrameImage = getFrameImage(frame); if (!initialFrameImage) { setFirstFrameLoaded(true); return; } const proxiedInitialImage = framesProxy.mediaUrl(initialFrameImage); if (fetchingInitialForMessageId.current !== messageIdRef.current) { return; } if (initialFrameImage.startsWith("data:")) { await handleDataUrl(initialFrameImage); return; } if (Platform.OS === "web") { await handleWebPlatform(proxiedInitialImage); return; } await handleNativePlatform(proxiedInitialImage); fetchingInitialForMessageId.current = undefined; };
191-191: Enhance error handling messagesWhile the error handling is present, the error messages could be more informative for debugging purposes.
Consider enhancing the error handling:
- throw new Error("Transaction frames not supported yet"); + throw new Error("Transaction frames are not supported on this platform. Platform: " + Platform.OS); - throw new Error("Could not get an external signer"); + throw new Error("External signer initialization failed. Please check wallet connection.");Also applies to: 300-303
Line range hint
347-366: Consider extracting opacity calculation logicThe opacity calculation could be more maintainable if extracted into a separate constant or function.
Consider refactoring to:
+ const getFrameOpacity = (isPosting: boolean, isRefreshing: boolean, isFromMe: boolean) => + isPosting || !isRefreshing ? (isFromMe ? 0.8 : 0.4) : 1; <View style={{ - opacity: - postingActionForButton !== undefined || !firstImageRefreshed - ? messageFromMe - ? 0.8 - : 0.4 - : 1, + opacity: getFrameOpacity( + postingActionForButton !== undefined, + firstImageRefreshed, + messageFromMe + ), }} >utils/conversation.ts (2)
Line range hint
503-509: Add error handling for edge cases in group conversation filtering.While excluding group conversations aligns with the V3 split logic, the current implementation could be more robust.
Consider adding error handling and logging:
if (conversation.isGroup) { + logger.debug('Skipping group conversation in search results', { + topic: conversation.topic, + reason: 'V3 split logic' + }); return false; } else { + if (!conversation.peerAddress) { + logger.warn('Conversation missing peer address', { + topic: conversation.topic + }); + return false; + } return ( conversation.peerAddress && matchedPeerAddresses.includes(conversation.peerAddress) ); }
Line range hint
450-509: Consider implementing a feature flag for V3 transition.The current approach of hard-coding group conversation exclusions could make the V2 to V3 transition more challenging. Consider implementing a feature flag to gradually roll out these changes.
Suggestions:
- Add a feature flag (e.g.,
ENABLE_V3_CONVERSATION_LOGIC) to control this behavior.- Create separate V3-specific utility functions instead of modifying existing ones.
- Consider implementing an adapter pattern to handle both V2 and V3 logic during the transition period.
This would provide:
- Easier rollback capability if issues arise
- Gradual transition from V2 to V3
- Better testing isolation for V3 features
components/Chat/Message/Message.tsx (2)
210-214: Add null content handlingWhile the implementation is clean, consider adding defensive programming for undefined/null content. The current implementation might fail silently if content is null and contentFallback is undefined.
<TextMessage fromMe={message.fromMe} hideBackground={hideBackground} - content={message.content || message.contentFallback} + content={message.content || message.contentFallback || ''} />
339-344: Consider using semantic layout componentsThe View wrapper with flex: 1 could be more semantic. Consider creating a styled component or using a more descriptive name.
-<View style={{ flex: 1 }}> +<View style={[styles.messageContentContainer, { flex: 1 }]}>Add to StyleSheet:
messageContentContainer: { flex: 1, // Add any other layout styles specific to message content },utils/xmtpRN/conversations.ts (3)
321-321: TODO comment needs clarification.The TODO comment "What is this doing? how to make it work with change" is too vague. Please clarify:
- What specific functionality needs investigation
- What changes are affecting it
- What needs to be done to make it work
Line range hint
357-375: Add null check for conversations array.While the simplified logic is good, consider adding a defensive check:
- const conversations = await listConversations(client); + const conversations = await listConversations(client) || []; const knownTopicsSet = new Set(knownTopics); const newConversations: ConversationWithCodecsType[] = []; const knownConversations: ConversationWithCodecsType[] = [];
Line range hint
382-389: Enhance error message in loadConversations.Consider adding more context to the error message:
const error = new Error(); error.name = "LOAD_CONVERSATIONS_FAILED"; - error.message = `${e}`; + error.message = `Failed to load conversations for account ${account}: ${e}`; throw error;queries/useGroupsConversationListQuery.ts (2)
25-27: UseDate.now()Instead ofnew Date().getTime()for TimestampIn lines 25 and 27, you can use
Date.now()for a more concise and readable way to get the current timestamp in milliseconds.Apply this diff to update the code:
- const beforeSync = new Date().getTime(); + const beforeSync = Date.now(); ... - const afterSync = new Date().getTime(); + const afterSync = Date.now();
92-107: Add Unit Tests for Query FunctionsTo ensure the reliability and maintainability of the new query functions, it's important to add unit tests. This will help catch potential bugs and make future refactoring safer.
components/XmtpEngine.tsx (1)
28-36: Consistency in Declaring Access ModifiersFor better code clarity and maintainability, it's recommended to explicitly declare access modifiers (
private,public, orprotected) for all class properties. Currently, the properties in theXmtpEngineclass lack explicit access modifiers, while those inXmtpCronare declared asprivate.Consider adding explicit access modifiers to the
XmtpEngineclass properties:class XmtpEngine { - appStoreSubscription: (() => void) | null = null; - appStateSubscription: NativeEventSubscription | null = null; - isInternetReachable: boolean = false; - hydrationDone: boolean = false; - syncedAccounts: { [account: string]: boolean } = {}; - syncingAccounts: { [account: string]: boolean } = {}; - appState: AppStateStatus = AppState.currentState; - started: boolean = false; + private appStoreSubscription: (() => void) | null = null; + private appStateSubscription: NativeEventSubscription | null = null; + private isInternetReachable: boolean = false; + private hydrationDone: boolean = false; + private syncedAccounts: { [account: string]: boolean } = {}; + private syncingAccounts: { [account: string]: boolean } = {}; + private appState: AppStateStatus = AppState.currentState; + private started: boolean = false; }This makes the code more readable and clarifies the intended access level of each property.
components/Chat/ChatDumb.tsx (1)
113-113: Consider renaminghideInputIfFrameFocusedfor clarityThe variable
hideInputIfFrameFocusedmay be misleading since it's assigned based onPlatform.OS !== "web". To improve readability, consider renaming it to better reflect its purpose, such asshouldHideInputOnNonWebPlatforms.Apply this diff to rename the variable:
113- const hideInputIfFrameFocused = Platform.OS !== "web"; 113+ const shouldHideInputOnNonWebPlatforms = Platform.OS !== "web";Also, update all references to this variable:
118 return frameTextInputFocused && hideInputIfFrameFocused + return frameTextInputFocused && shouldHideInputOnNonWebPlatforms 229 frameTextInputFocused && hideInputIfFrameFocused + frameTextInputFocused && shouldHideInputOnNonWebPlatformscomponents/Conversation/V3Conversation.tsx (3)
179-186: Ensure debug features are appropriately gatedThe
onLongPressfunction copies conversation details to the clipboard but is only enabled whendebugEnabledis true. Confirm if this behavior is intended or if it should be available in production builds.If this feature is meant for debugging, consider adding comments to clarify its purpose.
241-242: Remove commented-out codeCommented-out code at lines 241-242 can clutter the codebase and cause confusion. It's best to remove it if it's no longer needed.
Apply this diff to remove the commented code:
-// case "groupUpdated": -// return <ChatGroupUpdatedMessage message={message} />;
247-251: Handle errors when accessing message contentIn the
try-catchblock, if fetchingmessage.content()fails, the error isn't logged or handled. Consider logging the error for debugging purposes.Optionally, add error logging:
} catch (error) { content = message.fallback ?? ""; + console.error('Failed to get message content:', error); }components/V3GroupConversationListItem.tsx (4)
249-250: Remove unused empty callbacks to clean up the code.The
onWillRightSwipefunction is an empty callback:const onWillRightSwipe = useCallback(() => {}, []);If it is not needed, consider removing it to simplify the codebase and enhance readability.
Apply this diff to remove the unused function:
- const onWillRightSwipe = useCallback(() => {}, []);Also, remove
onWillRightSwipefrom the props passed toConversationListItemDumbon line 348 if it's no longer used.
234-234: Remove unused empty callbacks to simplify the code.The
onLeftPressfunction is an empty callback:const onLeftPress = useCallback(() => {}, []);If this callback is unnecessary, consider removing it to keep the code clean and maintainable.
Apply this diff:
- const onLeftPress = useCallback(() => {}, []);Also, remove
onLeftPressfrom the props passed toConversationListItemDumbon line 344 if it's not required.
356-356: Update subtitle to display relevant message preview.Currently, the subtitle is hardcoded:
subtitle={`${timeToShow} ⋅ Message exam`}Replace
"Message exam"with an actual message preview or pertinent information to provide users with meaningful context.Apply this diff:
- subtitle={`${timeToShow} ⋅ Message exam`} + subtitle={`${timeToShow} ⋅ ${group?.lastMessagePreview || ''}`}Ensure that
grouphas alastMessagePreviewproperty or adjust accordingly.
64-65: Avoid possible undefined behavior when accessingtopicsData.When checking
topicsData[topic]?.readUntil, ensure thattopicsDataandtopicare defined to prevent potential runtime errors.Consider adding checks or default values:
const readUntil = topicsData?.[topic]?.readUntil || 0;
📜 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 (47)
- App.tsx (2 hunks)
- App.web.tsx (2 hunks)
- components/Chat/ChatDumb.tsx (1 hunks)
- components/Chat/Frame/FrameBottom.tsx (5 hunks)
- components/Chat/Frame/FramePreview.tsx (11 hunks)
- components/Chat/Frame/FramesPreviews.tsx (1 hunks)
- components/Chat/Input/InputDumb.tsx (1 hunks)
- components/Chat/Message/Message.tsx (3 hunks)
- components/Chat/Message/MessageSender.tsx (1 hunks)
- components/Chat/Message/MessageSenderAvatar.tsx (1 hunks)
- components/Chat/Message/TextMessage.tsx (1 hunks)
- components/Conversation/ConversationTitleDumb.tsx (1 hunks)
- components/Conversation/V3Conversation.tsx (1 hunks)
- components/ConversationFlashList.tsx (5 hunks)
- components/ConversationList/GroupConversationItem.tsx (0 hunks)
- components/ConversationListItem.tsx (2 hunks)
- components/ConversationListItem/ConversationListItem.tsx (1 hunks)
- components/ConversationListItem/ConversationListItemDumb.tsx (1 hunks)
- components/ExternalWalletPicker.tsx (1 hunks)
- components/GroupAvatar.tsx (3 hunks)
- components/V3GroupConversationListItem.tsx (1 hunks)
- components/XmtpEngine.tsx (3 hunks)
- data/db/index.ts (1 hunks)
- data/store/appStore.ts (1 hunks)
- hooks/useApprovedGroupsConversationList.ts (1 hunks)
- hooks/useConversationListGroupItem.ts (1 hunks)
- package.json (1 hunks)
- queries/QueryKeys.ts (1 hunks)
- queries/useGroupMembersQuery.ts (2 hunks)
- queries/useGroupMessages.ts (1 hunks)
- queries/useGroupNameQuery.ts (2 hunks)
- queries/useGroupPhotoQuery.ts (2 hunks)
- queries/useGroupQuery.ts (2 hunks)
- queries/useGroupsConversationListQuery.ts (1 hunks)
- queries/useGroupsQuery.ts (2 hunks)
- queries/useRefreshOnFocus.ts (1 hunks)
- screens/Conversation.tsx (2 hunks)
- screens/ConversationList.tsx (3 hunks)
- screens/Group.tsx (2 hunks)
- utils/alert.ts (1 hunks)
- utils/appState/appStateIsBlurred.ts (1 hunks)
- utils/appState/waitUntilAppActive.ts (1 hunks)
- utils/conversation.ts (2 hunks)
- utils/mergeLists.ts (1 hunks)
- utils/xmtpRN/conversations.ts (6 hunks)
- utils/xmtpRN/messages.ts (1 hunks)
- utils/xmtpRN/sync.ts (3 hunks)
💤 Files with no reviewable changes (1)
- components/ConversationList/GroupConversationItem.tsx
✅ Files skipped from review due to trivial changes (3)
- components/ExternalWalletPicker.tsx
- package.json
- utils/alert.ts
🧰 Additional context used
🪛 GitHub Check: lint
App.web.tsx
[warning] 35-35:
Prefer named exports
🪛 GitHub Check: tsc
components/Chat/Input/InputDumb.tsx
[failure] 140-140:
'inputRef' is possibly 'null'.
[failure] 140-140:
Property 'current' does not exist on type 'MutableRefObject<TextInput | null> | ((instance: TextInput | null) => void)'.
[failure] 141-141:
'inputRef' is possibly 'null'.
[failure] 141-141:
Property 'current' does not exist on type 'MutableRefObject<TextInput | null> | ((instance: TextInput | null) => void)'.
🪛 Biome
components/Conversation/V3Conversation.tsx
[error] 244-244: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 245-245: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (71)
queries/useRefreshOnFocus.ts (2)
1-2: LGTM! Clean and specific imports.The imports are well-organized and only include what's necessary.
4-17: Verify integration with existing navigation patterns.Since this hook is part of the V3 split logic, we should verify it works with both V2 and V3 navigation patterns.
✅ Verification successful
Let me gather more information about navigation patterns and usage of this hook.
Based on the results, I can now provide a final response:
The hook is compatible with both V2 and V3 navigation patterns
The codebase exclusively uses React Navigation v6 (
@react-navigation/nativev6.0.16) with no legacy v5 imports found. TheuseRefreshOnFocushook:
- Uses the standard
useFocusEffectfrom@react-navigation/native- Is already being used in production code (e.g., in
useGroupMessages)- Follows the same focus handling patterns as the rest of the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage patterns of navigation hooks across V2 and V3 components # Look for other useFocusEffect usage patterns echo "Checking useFocusEffect patterns:" rg "useFocusEffect" -A 5 # Look for screen focus handling patterns echo "\nChecking screen focus handling patterns:" ast-grep --pattern 'useEffect($$$, [$$$navigation$$$])'Length of output: 943
Script:
#!/bin/bash # Search for imports and usage of useRefreshOnFocus echo "Checking useRefreshOnFocus usage:" rg "useRefreshOnFocus" -A 5 # Look for navigation version imports to understand which version is used where echo -e "\nChecking navigation version imports:" rg "@react-navigation/native" -A 2 rg "@react-navigation/native-v5" -A 2 # Check for screen components that might use navigation echo -e "\nChecking screen components:" fd -e tsx -e ts "Screen.tsx$|Screen.ts$" --exec rg -l "navigation|useFocusEffect|useRefreshOnFocus" {}Length of output: 16119
hooks/useConversationListGroupItem.ts (2)
1-3: LGTM! Clean and well-organized imports.The imports are properly structured and all are being utilized in the hook.
5-13:⚠️ Potential issueAdd type safety, error handling, and proper null checks.
The hook implementation could be improved in several areas:
- The
topicparameter lacks type constraints- Unsafe null assertion on
account- Missing error handling for query failures
- Return type is not explicitly defined
Consider applying these improvements:
-export const useConversationListGroupItem = (topic: string) => { +export const useConversationListGroupItem = (topic: ConversationTopic): { + conversation?: ConversationData; + isLoading: boolean; + error?: Error; +} => { const account = useCurrentAccount(); - const { data } = useGroupsConversationListQuery(account!, { + const { data, isLoading, error } = useGroupsConversationListQuery(account ?? undefined, { refetchOnWindowFocus: false, refetchOnMount: false, }); - return useMemo(() => data?.byId[topic], [data, topic]); + const conversation = useMemo(() => data?.byId[topic], [data, topic]); + + return { + conversation, + isLoading, + error, + }; };This implementation:
- Adds proper typing for the topic parameter
- Removes unsafe null assertion
- Provides loading and error states
- Explicitly defines the return type
Let's verify the ConversationTopic type exists:
hooks/useApprovedGroupsConversationList.ts (2)
1-3: LGTM! Clean and well-organized imports.The imports are logically structured and use proper path aliases.
18-18: Consider adding loading and error states to the return value.The hook spreads the rest of the query result but doesn't document what states are available.
Let's verify what states the query hook provides:
Consider documenting the available states in a comment:
// Returns { data, isLoading, error, refetch, ... } from useGroupsConversationListQuery return { data: filtered, ...rest };✅ Verification successful
Let me gather more information about the hook implementation to understand the full context.
I have enough context now to generate the final response.
The review comment is valid but can be more specific about the available states
The hook uses
useGroupsConversationListQuerywhich is built on top ofuseQueryfrom a query client library. Looking at the implementation:
- Line 7 shows the hook destructures
dataand spreads...rest- The base query hook returns standard query states from
useQuery- These states are implicitly passed through via
...restA more descriptive comment would help developers understand the available states:
// Returns { data: FilteredGroupConversationList, isLoading: boolean, error: Error | null, refetch: () => void } // from useGroupsConversationListQuery return { data: filtered, ...rest };🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the implementation of useGroupsConversationListQuery ast-grep --pattern 'export const useGroupsConversationListQuery = $_'Length of output: 902
Script:
#!/bin/bash # Search for the implementation of useApprovedGroupsConversationList hook ast-grep --pattern 'export const useApprovedGroupsConversationList = $_'Length of output: 1195
queries/useGroupMessages.ts (2)
26-26: LGTM! Verify refresh behavior.Adding automatic refresh on focus is a good UX improvement. This ensures users see the latest messages when returning to the app.
Let's verify the refresh implementation:
#!/bin/bash # Description: Check the refresh hook implementation # and its usage across the codebase # Check the refresh hook implementation ast-grep --pattern 'export const useRefreshOnFocus = $_' # Check for consistent usage across similar queries rg "useRefreshOnFocus"
5-6: LGTM! Verify query implementation compatibility.The transition to
useGroupConversationScreenQueryaligns with the V3 split logic objective, providing a more focused implementation for the conversation screen context.Let's verify the query implementation compatibility:
✅ Verification successful
Import changes are correctly implemented and aligned with V3 migration
The verification shows that:
useGroupConversationScreenQueryis properly exported fromuseGroupQuery.tsand is being used consistently across multiple V3-related components and queries- The implementation in
useGroupMessages.tscorrectly uses both new imports:
useGroupConversationScreenQueryfor fetching group datauseRefreshOnFocusfor handling data refresh on screen focusThe changes are part of a broader V3 implementation pattern, as evidenced by the usage in
V3Conversation.tsxand other V3-related files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old query # and verify the new query implementation # Check for any remaining old query references rg "useGroupQuery" # Verify the new query implementation ast-grep --pattern 'export const useGroupConversationScreenQuery = $_'Length of output: 424
Script:
#!/bin/bash # Let's try a more comprehensive search for both old and new query implementations # Search for any files containing 'useGroupQuery' echo "=== Files containing useGroupQuery ===" rg -l "useGroupQuery" # Search for implementation details of useGroupConversationScreenQuery echo -e "\n=== Implementation of useGroupConversationScreenQuery ===" rg "useGroupConversationScreenQuery" -A 5 # Check the actual file content to understand the changes echo -e "\n=== Content of queries/useGroupMessages.ts ===" cat queries/useGroupMessages.ts # Look for any potential V2/V3 related files echo -e "\n=== Files potentially related to V2/V3 split ===" rg -l "V[23]|version[23]" --type tsLength of output: 7559
utils/appState/waitUntilAppActive.ts (1)
1-1: LGTM! Good practice using absolute imports.The change from relative to absolute imports improves maintainability and reduces the risk of circular dependencies.
queries/useGroupNameQuery.ts (2)
9-9: Verify the migration from useGroupQuery to useGroupConversationScreenQuery.This change appears to be part of the V3 split logic implementation. Let's verify that all related files have been updated consistently.
18-18: LGTM! Verify data shape consistency.The implementation looks correct, but since we're switching to a screen-specific query, let's ensure that the
groupobject returned byuseGroupConversationScreenQuerymaintains the same interface, particularly thegroupName()method.✅ Verification successful
Based on the code analysis, I can now generate the final response. The implementation shows that
useGroupConversationScreenQueryreturns a group object that maintains the same interface, including thegroupName()method. This is evident from:
- The
useGroupNameQueryimplementation which uses the group object returned byuseGroupConversationScreenQueryand callsgroupName()method on it- The consistent usage pattern across other queries (photo, description, etc.) showing the same interface is maintained
Interface consistency verified - implementation is safe.
The group object returned by the new query maintains the same interface with the
groupName()method, ensuring data shape consistency.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the interface consistency between old and new implementations # Search for type definitions or interfaces related to the group object ast-grep --pattern 'interface $GROUP { $$$ groupName(): $RETURN $$$ }' # Look for actual implementations of groupName ast-grep --pattern 'groupName() { $$$ }'Length of output: 218
Script:
#!/bin/bash # Let's try a different approach to find the interface and implementation # Search for any interfaces or types containing groupName rg "groupName.*:" -A 5 -B 5 # Search for the old query usage rg "useGroupQuery" -A 2 -B 2 # Search for the new query implementation rg "useGroupConversationScreenQuery" -A 5 -B 5 # Look for any class or type definitions in the query files fd "Group.*Query\.ts" --exec cat {}Length of output: 61452
queries/useGroupPhotoQuery.ts (2)
Line range hint
1-67: Implementation follows React Query best practices.The overall implementation is clean and well-structured:
- Proper separation of concerns with dedicated utility functions
- Consistent use of React Query patterns
- Appropriate error handling through React Query
9-9: Verify data shape compatibility with new query hook.The switch from
useGroupQuerytouseGroupConversationScreenQuerylooks good, but we should verify that the returnedgroupobject maintains the same interface, particularly thegroupImageUrlSquare()method.Let's verify the interface compatibility:
Also applies to: 18-18
✅ Verification successful
The switch to
useGroupConversationScreenQueryis safe and maintains interface compatibilityBased on the verification results:
- Both query hooks return the same
groupobject fromclient.conversations.findGroup()- The
groupImageUrlSquare()method is consistently used across the codebase- The only difference is that
useGroupConversationScreenQueryadds agroup?.sync()call before returning, which doesn't affect the return type🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if both query hooks return compatible group objects # Test 1: Find the type definition or implementation of groupImageUrlSquare rg -A 5 "groupImageUrlSquare" # Test 2: Check the return type of both query hooks ast-grep --pattern 'export const useGroupQuery = $_' ast-grep --pattern 'export const useGroupConversationScreenQuery = $_'Length of output: 4027
components/Chat/Message/TextMessage.tsx (1)
1-56: Verify consistent component usage across the codebase.Let's ensure this component is being used consistently throughout the application.
✅ Verification successful
Based on the comprehensive search results, I can now provide the final response:
TextMessage component is used consistently across the codebase
The component is used correctly in both Message.tsx and V3Conversation.tsx with consistent prop passing:
- All instances properly provide the required
fromMeprop- The
hideBackgroundprop is consistently used for styling emoji messages- The
contentprop is properly populated with message contentThe implementation shows proper integration within the messaging system with no inconsistencies found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check TextMessage component usage patterns # Find all imports of TextMessage echo "Checking TextMessage imports:" rg "import.*TextMessage.*from.*Message/TextMessage" -A 2 # Check prop usage patterns echo "\nChecking prop usage patterns:" rg "<TextMessage.*?>" -A 2Length of output: 544
Script:
#!/bin/bash # Let's try a more comprehensive search for TextMessage usage # Search for TextMessage in JSX/TSX files with more context echo "Searching for TextMessage usage in components:" rg "<TextMessage" -t tsx -t jsx -A 5 # Search for any reference to TextMessage echo -e "\nSearching for TextMessage references:" rg "TextMessage" -t tsx -t jsx -A 2 # Look for similar message components to understand the pattern echo -e "\nSearching for other message components:" fd -e tsx -e jsx --exec grep -l "Message" {}Length of output: 2655
Script:
#!/bin/bash # Let's try different search patterns without file type restrictions # Search for TextMessage usage in components echo "Searching for TextMessage component usage:" rg "<TextMessage" -A 5 # Check Message.tsx which might be using TextMessage echo -e "\nChecking Message.tsx implementation:" cat "./components/Chat/Message/Message.tsx" # Check V3Conversation.tsx where we found the import echo -e "\nChecking V3Conversation.tsx implementation:" cat "./components/Conversation/V3Conversation.tsx"Length of output: 30329
components/Conversation/ConversationTitleDumb.tsx (1)
1-68: Verify integration with V3 conversation logic.The component follows the "dumb component" pattern mentioned in the PR objectives, but we should verify its integration with V3 conversation handling.
Let's check for proper component usage:
✅ Verification successful
Integration with V3 conversation logic is properly implemented
The verification shows that:
- ConversationTitleDumb is correctly imported and used in V3Conversation.tsx as a presentational component
- The component receives all required props (title, avatarComponent, onPress, onLongPress) from its parent
- The styling and behavior are consistent between V2 (ConversationTitle.tsx) and V3 implementations
- The "dumb component" pattern is correctly followed, with all business logic residing in the parent V3 component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ConversationTitleDumb usage in V3 components rg -l "ConversationTitleDumb" | grep -i "v3" # Check for any potential duplicate conversation title components rg -l "ConversationTitle[^D]" | grep -v "Dumb"Length of output: 433
Script:
#!/bin/bash # Let's check the implementation of V3Conversation.tsx to see how ConversationTitleDumb is used rg -A 10 "ConversationTitleDumb" "components/Conversation/V3Conversation.tsx" # Also check ConversationTitle.tsx to understand the relationship cat "components/Conversation/ConversationTitle.tsx"Length of output: 6478
components/Chat/Message/MessageSender.tsx (2)
1-9: LGTM! Clean imports and well-defined types.The imports are organized logically, and the type definitions are clear and purposeful.
32-43:⚠️ Potential issueVerify inbox ID handling and add error checks.
The component needs better error handling and the array access pattern could be unsafe.
Let's verify the inbox ID usage pattern:
Apply these safety improvements:
export const V3MessageSender = ({ inboxId }: V3MessageSenderProps) => { + if (!inboxId) { + return null; + } - const address = useInboxIdStore((s) => s.byInboxId[inboxId]?.[0]); + const addresses = useInboxIdStore((s) => s.byInboxId[inboxId] ?? []); + const address = addresses[0]; + if (!address) { + return null; + } const senderSocials = useProfilesStore( (s) => getProfile(address, s.profiles)?.socials ); const name = getPreferredName(senderSocials, address); + if (!name) { + return null; + } return <MessageSenderDumb name={name} />; };✅ Verification successful
Based on the search results, I can now generate the final response since I have enough context about the
byInboxIdusage pattern across the codebase:The array access pattern is safe, but explicit null checks would improve code clarity
The
byInboxIdusage is already protected by the optional chaining operator (?.), and the store's implementation shows thatbyInboxId[inboxId]is always initialized as an array before adding addresses. While the current implementation is safe, the suggested error checks would improve code clarity and provide better fallback behavior.
- The store ensures
byInboxId[inboxId]is always an array via:if (!newState.byInboxId[inboxId]) { newState.byInboxId[inboxId] = []; }- Similar access patterns with
?.[0]are used consistently across other components- The suggested null checks would make the component's behavior more explicit and maintainable
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of byInboxId to understand the data structure rg "byInboxId" -A 3Length of output: 5588
App.web.tsx (2)
12-12: LGTM: Clean import restructuringThe change to named imports provides better clarity on the specific functionalities being used from the XmtpEngine module.
Line range hint
44-57: Verify Privy configuration settingsThe Privy configuration contains important security and functionality settings. Let's verify these settings are correctly configured for both V2 and V3 compatibility.
✅ Verification successful
Privy configuration is correctly implemented across environments
The Privy configuration is properly structured with appropriate settings for both development and production environments:
- Development/Preview environments use Base Sepolia testnet
- Production environment uses Base mainnet
- Each environment has proper configuration for:
appIdfrom environment variablesdefaultChainset appropriately per environmentsupportedChainslimited to the default chain- Embedded wallets with secure defaults
The implementation follows best practices by:
- Using environment variables for sensitive values
- Having distinct chains for different environments
- Maintaining consistent wallet creation and signature behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other Privy configurations to ensure consistency rg -l 'privy.*appId|defaultChain|supportedChains' --type ts --type tsx # Look for any environment variable references for these settings rg -l 'PRIVY.*|CHAIN.*' .env*Length of output: 180
Script:
#!/bin/bash # Search for Privy configurations with correct file type patterns rg -l 'privy.*appId|defaultChain|supportedChains' --type typescript --type-add 'typescript:*.{ts,tsx}' # Look for config files that might contain Privy settings fd -e ts -e tsx -e js -e json --full-path '.*config.*' -x cat {} # Search for environment variable patterns in all files rg 'PRIVY_|CHAIN_' -lLength of output: 11141
components/Chat/Frame/FramesPreviews.tsx (1)
62-64: Verify FramePreview component compatibility with new props.The changes to prop passing align with the AI summary, but we should verify that the FramePreview component has been updated to handle these new props correctly.
✅ Verification successful
Props are correctly handled by FramePreview component
The FramePreview component is properly typed and accepts all the props being passed:
messageId: stringmessageTopic: stringmessageFromMe: booleaninitialFrame: FrameWithTypeThe prop passing is consistent with the component's interface definition, and no compatibility issues are present.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for FramePreview component definition to verify prop types ast-grep --pattern 'type Props = { $$$ messageId: string; messageTopic: string; messageFromMe: boolean; $$$ }'Length of output: 122
Script:
#!/bin/bash # Let's try a different approach to find the FramePreview component and its props rg "FramePreview" -A 10 -B 10 # Also search for interface/type definitions related to FramePreview rg "interface.*FramePreview|type.*FramePreview" -A 10Length of output: 3785
screens/Group.tsx (1)
Line range hint
1-25: Verify the impact of removing query invalidation imports.The removal of multiple query invalidation functions suggests a significant change in how group data is refreshed. While this aligns with the PR's goal of splitting V2/V3 logic, we should ensure that group data updates are still properly handled.
Let's verify where these query invalidations have moved:
✅ Verification successful
Query invalidation functions are properly maintained in a centralized location
The query invalidation functions have been moved to a more organized location and are now centrally managed in the message handlers. Specifically:
- All invalidation functions are defined in their respective query files (e.g.,
queries/useGroupNameQuery.ts)- They are imported and used in
data/helpers/messages/handleGroupUpdatedMessage.tswhich acts as a central handler for group updates- The handler intelligently invalidates only the relevant queries based on what changed (members, name, photo, etc.)
- Test coverage confirms the correct invalidation behavior for different scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for query invalidation usage in the new implementation rg -A 3 "invalidateGroup(Consent|Description|Members|Name|Photo)Query"Length of output: 8828
queries/useGroupsQuery.ts (3)
Line range hint
41-50: LGTM! Well-structured query configuration.The addition of optional queryOptions enhances the hook's flexibility while maintaining proper TypeScript typing.
Line range hint
52-93: Verify utility functions compatibility with new implementation.The utility functions (fetchGroupsQuery, invalidateGroupsQuery, etc.) are unchanged but now operate on empty group data. Please verify if these functions need updates to align with the V3 transition strategy.
#!/bin/bash # Check for usages of these utility functions rg -A 2 "fetchGroupsQuery|invalidateGroupsQuery|addGroupToGroupsQuery" --type ts
16-22: Verify the impact of bypassing group synchronization.The function creates a client but immediately returns empty data without using it. This effectively disables all group functionality. Please verify if this is intentional as part of the V3 transition.
✅ Verification successful
Let me gather more information about the usage and impact of this query.
Based on the code inspection, I can now provide a final response:
Bypassing group synchronization appears to be an intentional V3 transition change
The code in
useGroupsQuery.tsshows clear evidence of a deliberate transition:
- The original group synchronization code is commented out but preserved
- There are numerous group-related components and queries still in the codebase
- The function includes a debug log "here2222" suggesting this is a temporary state
- The commented code includes detailed performance logging, indicating this was a carefully considered change
This appears to be part of a larger V3 transition where group functionality is being temporarily disabled while preserving the infrastructure for future re-enablement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any direct dependencies on group data rg -l "useGroupsQuery|groupsQueryKey" --type tsLength of output: 2520
Script:
#!/bin/bash # Check the implementation of useGroupsQuery and its dependencies ast-grep --pattern 'export function useGroupsQuery($_) { $$$ }' # Check for any components or hooks using this query rg -A 3 'useGroupsQuery\(' --type ts # Check for any group-related UI components that might be affected rg -l 'Group|group' --type tsxLength of output: 165
Script:
#!/bin/bash # Check the implementation of useGroupsQuery and its dependencies cat queries/useGroupsQuery.ts # Check for any components or hooks using this query rg -A 3 'useGroupsQuery\(' --type typescript --type-add 'typescript:*.{ts,tsx}' # Check for any group-related UI components that might be affected rg -l 'Group|group' --type typescript --type-add 'typescript:*.{ts,tsx}'Length of output: 8092
data/store/appStore.ts (2)
28-28: LGTM! One-way state transition for hydration is appropriate.The type change from
booleanto literaltrueenforces that hydration can only be marked as complete and never reversed, which is the correct semantic for hydration state.Let's verify the usage patterns:
#!/bin/bash # Description: Check all usages of setHydrationDone to ensure they only set it to true # Expect: All calls should pass true or a variable that resolves to true # Search for direct function calls rg -A 2 "setHydrationDone\(" # Search for potential variable assignments that might be used ast-grep --pattern 'setHydrationDone($value)'
22-22: LGTM! One-way state transition for splash screen is a good practice.The type change from
booleanto literaltrueenforces that the splash screen can only be hidden and never shown again, which aligns with typical splash screen behavior in applications.Let's verify that this aligns with all usage patterns:
✅ Verification successful
Verified: Splash screen state transition is correctly implemented
The codebase analysis confirms that
setSplashScreenHiddenis used correctly with the literaltruevalue in all instances:
- In
InitialStateHandler.tsx, it's called withsetSplashScreenHidden(true)after hydration is complete- The state is initialized as
false(ortruefor web platform) inappStore.ts- The splash screen is hidden only once and never shown again
- The implementation is consistent with the native splash screen handling in both iOS and Android
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check all usages of setSplashScreenHidden to ensure they only set it to true # Expect: All calls should pass true or a variable that resolves to true # Search for direct function calls rg -A 2 "setSplashScreenHidden\(" # Search for potential variable assignments that might be used ast-grep --pattern 'setSplashScreenHidden($value)'Length of output: 493
Script:
#!/bin/bash # Let's try a different approach to find usages of setSplashScreenHidden # Search for any mention of setSplashScreenHidden rg "setSplashScreenHidden" # Also search for splash screen related code to understand the context rg -A 3 "splashScreen" -iLength of output: 13780
queries/QueryKeys.ts (1)
5-5: Consider version suffix and persistence implicationsGiven that this PR involves V2/V3 split logic:
- Consider adding a version suffix for consistency with other keys (e.g., "groupConversationListv3")
- Add a comment about persistence implications similar to the GROUPS key, especially if the response shape might change during V3 transition.
Let's check if other V3-related keys follow a version pattern:
Apply this diff to address these concerns:
- GROUP_CONVERSATION_LIST = "groupConversationList", + GROUP_CONVERSATION_LIST = "groupConversationListv3", // When changing the shape of response, update the key as persistence will breakqueries/useGroupMembersQuery.ts (3)
13-13: LGTM: Import change aligns with V3 split logic.The renamed import reflects a more specific scope and maintains consistency with the PR's objective of separating V2/V3 logic.
22-44: LGTM: Clean implementation with proper error handling.The updated implementation maintains the same functionality while integrating with the new conversation screen query. The conditional enabling and error handling for missing group data are well implemented.
Line range hint
46-67: Consider potential cache conflicts and maintenance overhead.The new
useGroupMembersConversationScreenQueryis nearly identical touseGroupMembersQueryand uses the same query key. This could lead to:
- Cache conflicts between the two hooks
- Potential race conditions
- Increased maintenance overhead
Let's check if these hooks are used in different contexts:
Consider:
- Using a shared implementation with a flag parameter if the only difference is the context
- Using distinct query keys if separate caching is needed
- Adding documentation explaining when to use each hook
components/Chat/Message/MessageSenderAvatar.tsx (2)
1-22: LGTM! Well-structured imports and type definitions.The imports are organized logically, and the type definitions are clear and properly typed.
79-103:⚠️ Potential issueRefactor duplicated logic and verify hardcoded value.
Issues found:
- Component duplicates logic from
MessageSenderAvatarhasNextMessageInSeriesis hardcoded tofalsewhich might be incorrectConsider refactoring to reduce duplication:
-type V3MessageSenderAvatarProps = { +type V3MessageSenderAvatarProps = Omit<MessageSenderAvatarProps, 'senderAddress'> & { inboxId: string; }; export const V3MessageSenderAvatar = ({ inboxId, + hasNextMessageInSeries = false, }: V3MessageSenderAvatarProps) => { const address = useInboxIdStore((s) => s.byInboxId[inboxId]?.[0]); - const senderSocials = useProfilesStore( - (s) => getProfile(address, s.profiles)?.socials - ); - - const openProfile = useCallback(() => { - navigate("Profile", { address }); - }, [address]); - return ( - <MessageSenderAvatarDumb - hasNextMessageInSeries={false} - onPress={openProfile} - avatarUri={getPreferredAvatar(senderSocials)} - avatarName={getPreferredName(senderSocials, address)} - /> - ); + return address ? ( + <MessageSenderAvatar + senderAddress={address} + hasNextMessageInSeries={hasNextMessageInSeries} + /> + ) : null; };components/Chat/Frame/FrameBottom.tsx (3)
57-58: LGTM: FrameTextInput prop passing is correct.The messageFromMe prop is correctly passed to the FrameTextInput component.
77-78: LGTM: FrameButton prop passing is correct.The messageFromMe prop is correctly passed to the FrameButton component.
87-89: LGTM: Text color styling is correctly updated.The conditional color styling now properly uses the messageFromMe boolean prop.
components/ConversationListItem/ConversationListItem.tsx (4)
1-18: LGTM! Well-structured imports and type definitions.The imports are clean and the
ConversationListItemContainerPropstype is well-defined with all necessary optional handlers.
20-25: LGTM! Clean default container styling.The default styling is well-defined using the theme system.
65-87: LGTM! Well-implemented text components.Good use of memoization and proper text truncation for the subtitle component.
1-115: Verify alignment with V3 Split Logic objectives.The implementation aligns well with the PR objectives by:
- Creating a "dumb" component structure with clear separation of concerns
- Maintaining functionality through well-defined props and composition API
- Setting up a foundation for V2/V3 coexistence
However, to ensure maintainability, let's verify the component usage across the codebase:
✅ Verification successful
Implementation aligns with V3 split and maintains backward compatibility
The verification confirms the implementation aligns well with the PR objectives:
- The component exists alongside both V2 and V3 implementations (
ConversationListItem.tsxandConversationListItemDumb.tsx), enabling gradual migration- The codebase shows clear V3 integration points (
V3Conversation.tsx,V3GroupConversationListItem.tsx)- The dumb component structure follows the pattern seen in other components (e.g.,
ConversationTitleDumb.tsx)The implementation provides a solid foundation for V2/V3 coexistence while maintaining a clean separation of concerns through the compositional API.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of ConversationListItem components echo "Searching for ConversationListItem usage patterns..." rg -A 3 "ConversationListItem\." --type typescript --type tsx # Look for potential V2/V3 integration points echo "Checking V2/V3 integration points..." rg -l "V[23].*Conversation" --type typescript --type tsxLength of output: 390
Script:
#!/bin/bash # Search for usage of ConversationListItem components without file type flags echo "Searching for ConversationListItem usage patterns..." rg -A 3 "ConversationListItem\." # Look for potential V2/V3 integration points echo "Checking V2/V3 integration points..." rg -l "V[23].*Conversation" # Check for any existing conversation-related components echo "Checking conversation-related components..." fd -e tsx -e ts "Conversation"Length of output: 3154
App.tsx (2)
58-60:⚠️ Potential issueMove initialization into React lifecycle with proper error handling.
The current implementation has several potential issues:
- No error handling for initialization failures
- No cleanup mechanism
- May cause issues with hot reloading
- Could start before other dependencies are ready
Consider wrapping the initialization in a React hook:
-xmtpEngine.start(); -xmtpCron.start(); const App = () => { + useEffect(() => { + const initializeXmtp = async () => { + try { + await xmtpEngine.start(); + await xmtpCron.start(); + } catch (error) { + logger.error('Failed to initialize XMTP:', error); + // Handle initialization failure + } + }; + + initializeXmtp(); + + return () => { + // Add cleanup if these instances provide stop/cleanup methods + xmtpEngine.stop?.(); + xmtpCron.stop?.(); + }; + }, []);This approach:
- Provides proper error handling
- Ensures cleanup on unmount
- Follows React best practices
- Makes the initialization sequence clear
Let's check if these instances provide cleanup methods:
#!/bin/bash # Search for stop/cleanup method definitions ast-grep --pattern 'stop() { $$$ }'
29-29: Consider dependency injection pattern for better testability.While transitioning from component-based to direct method invocations, importing singletons directly could make the code harder to test and maintain. Consider using a dependency injection pattern to improve testability and control over initialization order.
Let's check if these instances are used elsewhere in the codebase:
data/db/index.ts (2)
Line range hint
99-112: Consider a more granular error handling strategy for migrations.Currently, any migration error triggers a full database reset, which might be too aggressive and could lead to data loss.
Consider implementing a more nuanced approach:
- Log failed migrations
- Attempt to roll back the specific failed migration
- Only reset as a last resort
Let's check if there are any migration-related issues in the repository:
#!/bin/bash # Search for migration-related error patterns rg "Error running migrations|migration.*failed|rollback.*migration" --type ts
1-1: LGTM! Consider standardizing similar imports.The more specific import path is a good practice for better tree-shaking and module organization.
Let's check if other files still use the old import path:
✅ Verification successful
Let me gather more information to verify if there are any remaining imports from the old path pattern.
Import path change is consistent across the codebase
The specific import path
@utils/appState/waitUntilAppActiveis already being used consistently across the codebase. All imports from@utils/appStateare using the granular path pattern, indicating this change aligns with the existing codebase structure.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining imports from the old path rg "from ['\"]@utils/appState['\"]" --type tsLength of output: 169
Script:
#!/bin/bash # Search for any imports containing appState rg "from.*@utils/appState" --type ts -A 1Length of output: 555
components/ConversationFlashList.tsx (6)
11-11: LGTM: Import aligns with V3 integration strategy.The addition of V3GroupConversationListItem import supports the PR's objective of implementing V3 functionality alongside V2.
88-88: LGTM: Safe type guard added for scroll behavior.The type check
typeof c !== "string"ensures the scroll behavior remains intact for V2 conversations while safely handling V3 items.
101-106: LGTM: Clear and type-safe key extraction.The keyExtractor implementation cleanly handles both V2 and V3 items, with clear differentiation using the "v2" suffix for V2 items.
109-112: LGTM: Clean V3 component integration.The type check and V3 component rendering is implemented clearly and safely.
128-128:⚠️ Potential issueVerify the intentional removal of group conversation rendering.
The change to return
nullfor group conversations removes existing functionality. This might be intentional as part of the V2/V3 transition, but requires verification.#!/bin/bash # Search for GroupConversationItem usage and related components echo "Checking for group conversation handling..." rg -l "GroupConversationItem|isGroup|V3GroupConversation" | while read -r file; do echo "=== $file ===" rg "GroupConversationItem|isGroup|V3GroupConversation" "$file" doneConsider adding a comment explaining why group conversations return null, or if this is temporary during the V2/V3 transition.
31-31: Verify all component consumers handle the updated prop type.The items prop type has been expanded to
(ConversationFlatListItem | string)[], which is a breaking change.✅ Verification successful
Let me search for the actual usage of ConversationFlashList to verify how the items prop is being used.
Let me check how the items prop is being used within the ConversationFlashList component to understand the handling of both types.
Based on the implementation details found, I can now generate the final response.
The type change is properly handled in the component implementation
The ConversationFlashList component correctly handles both types in its renderItem implementation:
- String items are rendered using V3GroupConversationListItem
- ConversationFlatListItem objects are rendered using ConversationListItem
- The type change is intentional to support both V2 and V3 conversations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all instances where ConversationFlashList is imported and used echo "Searching for ConversationFlashList usage..." rg -l "ConversationFlashList" | while read -r file; do echo "=== $file ===" rg -A 10 "ConversationFlashList" "$file" doneLength of output: 29239
Script:
#!/bin/bash # Search for ConversationFlashList usage and implementation echo "=== Searching for ConversationFlashList usage ===" rg -A 10 "<ConversationFlashList" echo -e "\n=== Searching for ConversationFlashList implementation ===" rg -A 20 "export.*ConversationFlashList"Length of output: 4872
Script:
#!/bin/bash # Search for the renderItem implementation in ConversationFlashList echo "=== Searching ConversationFlashList implementation ===" rg -A 30 "renderItem.*=" components/ConversationFlashList.tsx # Search for where the items are prepared echo -e "\n=== Searching for items preparation ===" rg -A 10 "flatListItems" screens/ConversationList.tsxLength of output: 3394
utils/xmtpRN/sync.ts (2)
133-133: LGTM: Simplified topic timestamp initialization.The unified handling of all topics as conversation topics aligns with the PR's objective of simplifying the codebase.
Line range hint
136-190: Consider potential race conditions in sync process.The sequential execution of
loadConversationsandsyncConversationsMessagescould miss messages from conversations that arrive between these operations. Consider using a transaction timestamp or implementing optimistic concurrency control.Let's verify if there are any reported message sync issues:
screens/ConversationList.tsx (3)
1-1: LGTM: New imports align with V3 split logic objectives.The addition of
useApprovedGroupsConversationListandmergeOrderedListsimports supports the PR's objective of implementing V3 conversation handling while maintaining V2 compatibility.Also applies to: 9-9
87-90: Type enhancement improves flexibility for mixed conversation types.The state type update to include
stringalongsideConversationFlatListItemenables handling both V2 and V3 conversation formats, aligning with the PR's goal of supporting both versions.
138-142: LGTM: Effect dependencies properly track V3 groups.The addition of
approvedGroupsto the dependency array ensures the list updates when V3 group conversations change.screens/Conversation.tsx (2)
1-1: LGTM: Import changes align with V3 split architecture.The new imports support the V2/V3 logic separation strategy outlined in the PR objectives.
Also applies to: 9-9
344-344: LGTM: Export maintains gesture handling functionality.The export correctly wraps the new HOC while preserving the essential gesture handling functionality.
components/GroupAvatar.tsx (2)
51-55: LGTM: Clean type definition for dumb componentThe
GroupAvatarDumbPropstype definition is well-structured and properly implements the presentation-only component pattern mentioned in the PR objectives.
212-212: LGTM: Optimized query executionThe added condition
enabled: !!topic && !pendingGroupMembersproperly prevents unnecessary data fetching when pending members are already available.utils/xmtpRN/messages.ts (1)
181-184: Clarify the status of group message handling.The commented-out group message handling code suggests an incomplete transition. Given this PR's objective of splitting V2/V3 logic, please clarify:
- Is this intentionally disabled as part of the V2/V3 split?
- If yes, should we add a TODO comment explaining when this will be enabled?
- If no, should this code be removed to avoid confusion?
utils/conversation.ts (1)
450-452: Document the group conversation exclusion and verify impact.The early return for group conversations could impact existing functionality. While this aligns with the V3 split logic, it needs proper documentation and verification.
Let's verify the group conversation handling:
Add a comment explaining why group conversations are excluded and reference any related V3-specific handlers.
✅ Verification successful
Let me gather more context about the specific function and its V3-related changes.
Based on the gathered context, I can now generate the final response:
Early return for group conversations is intentional and correct
The early return for group conversations in
sortAndComputePreviewis correct as per the documented V3 migration strategy. From the README, we can see that "Groups and V3 Identity is not supported on Web" and is being handled separately. The function is part of the notification and preview computation system, and excluding groups here aligns with this architectural decision.
- The change is used by
NotificationsStateHandlercomponent for computing previews- Group conversations have their own dedicated handling as seen in
utils/xmtpRN/conversations.ts- The behavior is consistent with the documented limitations and ongoing V3 migration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential group conversation handling in V3-specific files # and verify that this change doesn't break existing functionality. # Test 1: Look for V3-specific group conversation handlers echo "Searching for V3 group conversation handlers..." rg -l "group.*conversation.*v3" --type ts # Test 2: Check for potential impacts on existing group functionality echo "Checking for potential impacts on group functionality..." rg -A 5 "isGroup.*true" --type tsLength of output: 3701
Script:
#!/bin/bash # Check the implementation of sortAndComputePreview and related V3 handling echo "Checking sortAndComputePreview implementation and V3 context..." rg -B 5 -A 10 "sortAndComputePreview" --type ts # Look for V3-specific conversation handling echo "Checking V3-specific conversation handling..." rg "version.*[\"']v3[\"']" --type ts -B 2 -A 2 # Check for any documentation about V3 migration echo "Checking V3 migration documentation..." fd "README|MIGRATION" --type f --exec cat {}Length of output: 7563
components/Chat/Message/Message.tsx (2)
32-35: Great architectural improvement!The extraction of MessageSender, MessageSenderAvatar, and TextMessage into separate components improves modularity and maintainability, aligning well with the PR's objective of splitting V2/V3 logic.
Line range hint
478-484: Verify optimal cache size for message renderingThe LimitedMap cache is set to 50 messages. Let's verify if this is optimal for your use case by analyzing message rendering patterns.
✅ Verification successful
Based on the analysis of the shell script results, I can now provide a final response regarding the cache size optimization:
Cache size of 50 messages is appropriate for the current implementation
The current cache size of 50 messages in
LimitedMapis well-justified for the following reasons:
- The app uses FlashList for optimized list rendering, which is specifically designed for performance with large lists
- The cache is specifically used to prevent re-renders when new messages arrive at the beginning of an inverted list
- The implementation shows careful consideration of performance:
- Only the last 20 messages are loaded initially for preview
- The chat UI switches between FlatList (for new conversations) and FlashList (for larger conversations) to optimize performance
- The caching mechanism is specifically designed to handle the message rendering pattern where new messages are pushed to the beginning
The current cache size provides a good balance between memory usage and performance optimization for the typical chat viewing patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze message patterns to determine optimal cache size # Check average number of visible messages in conversations rg -A 5 "FlashList|FlatList" . --type typescript # Look for any performance-related configurations rg -l "performance|optimization|cache" . --type typescriptLength of output: 181
Script:
#!/bin/bash # Let's try different approaches to analyze the caching implementation # Search for LimitedMap usage and cache size configurations rg -A 5 "LimitedMap|cacheSize|cache.*size" # Look for list rendering components and performance configurations rg -A 5 "FlashList|FlatList" # Find message-related performance optimizations rg -A 5 "performance|optimization|cache" "components/Chat"Length of output: 31618
utils/xmtpRN/conversations.ts (2)
5-5: LGTM: Import addition aligns with V3 split logic.The new import for group conversation list management supports the PR's objective of separating V2 and V3 functionality.
280-282: LGTM: Group streaming implementation.The implementation correctly:
- Updates the groups conversation list
- Emits events for new groups
- Maintains proper separation of concerns
queries/useGroupQuery.ts (1)
21-24: Verify ifuseGroupQueryrequires synchronizationIn
useGroupConversationScreenQuery,group.sync()is called to synchronize the group data. However,useGroupQuerydoes not perform this synchronization. Ensure thatuseGroupQuerydoes not require synchronization for its use cases, or consider adding it if up-to-date data is necessary.components/Conversation/V3Conversation.tsx (2)
275-290: Verify dependencies inuseMemohookIn the
avatarComponentuseMemo, ensure all dependencies are correctly specified. Missing dependencies can lead to stale values or unnecessary re-renders.Check if any dependencies, such as
groupPhotoLoading, should be included.useMemo(() => { if (displayAvatar) return null; return groupPhoto ? ( <Avatar uri={groupPhoto} size={AvatarSizes.conversationTitle} style={styles.avatar} /> ) : ( <GroupAvatarDumb members={memberData} size={AvatarSizes.conversationTitle} style={styles.avatar} /> ); -}, [displayAvatar, groupPhoto, styles.avatar, memberData]); +}, [displayAvatar, groupPhoto, styles.avatar, memberData]);
292-312:⚠️ Potential issueEnsure all dependencies are included in
useEffectThe
useEffecthook at lines 292-312 should include all dependencies that affect the side effects to prevent unexpected behavior.Verify that the dependency array contains all relevant variables:
useEffect(() => { navigation.setOptions({ headerTitle: () => ( <ConversationTitleDumb title={groupName} avatarComponent={avatarComponent} onLongPress={onLongPress} onPress={onPress} /> ), headerTintColor, }); -}, [navigation, groupName, avatarComponent, onLongPress, onPress, headerTintColor]); +}, [navigation, groupName, avatarComponent, onLongPress, onPress, headerTintColor]);Consider adding any missing dependencies that are used within the effect.
Likely invalid or redundant comment.
components/V3GroupConversationListItem.tsx (2)
280-280: Verify that member data is correctly populated for avatars.Ensure that
memberDatacontains the necessary information to render avatars properly, especially after the asynchronous fetch of members.Double-check that all member profiles are fetched and
memberDatais updated before rendering the avatars to prevent any missing or undefined data.
220-222: Handle potential navigation issues inonPress.The
onPresshandler navigates to the "Conversation" screen:const onPress = useCallback(() => { navigate("Conversation", { topic }); }, [topic]);Ensure that the
navigatefunction is correctly imported and that the "Conversation" route is properly set up to accept thetopicparameter.Confirm that navigating to the conversation works as expected without any errors.
components/ConversationListItem/ConversationListItemDumb.tsx (2)
365-385: Verify callback parameters foronSwipeableWillCloseandonSwipeableCloseThe
onSwipeableWillCloseandonSwipeableClosefunctions are defined with adirectionparameter, but theSwipeablecomponent's callbacks do not provide this parameter. This could lead to unexpected behavior or errors. Please verify whether thedirectionparameter is correctly received from theSwipeablecomponent or adjust the implementation accordingly.If the
directionparameter is not available, you may need to adjust the callbacks:-const onSwipeableWillClose = useCallback( - (direction: "left" | "right") => { - if (direction === "left") { +const onSwipeableWillCloseLeft = useCallback(() => { onWillLeftSwipe?.(); - } else { +}, [onWillLeftSwipe]); + +const onSwipeableWillCloseRight = useCallback(() => { onWillRightSwipe?.(); - } - }, - [onWillLeftSwipe, onWillRightSwipe] -); +}, [onWillRightSwipe]);And update the
Swipeablecomponent accordingly.
397-397: Verify usage of negativehitSlopvalueThe
hitSlopproperty in theSwipeablecomponent is set with a negativeleftvalue (hitSlop={{ left: -6 }}). NegativehitSlopvalues can reduce the touchable area and might have unintended effects on user interactions. Please verify that this negative value is intentional and achieves the desired effect.
utils/appState/appStateIsBlurred.ts
Outdated
| @@ -0,0 +1,6 @@ | |||
| import { AppStateStatus } from "react-native"; | |||
|
|
|||
| const blurredStateRegex = /inactive|background/; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using exact matches for better performance.
The current regex allows partial matches which isn't necessary for this use case.
-const blurredStateRegex = /inactive|background/;
+const blurredStateRegex = /^(?:inactive|background)$/;📝 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 blurredStateRegex = /inactive|background/; | |
| const blurredStateRegex = /^(?:inactive|background)$/; |
utils/appState/appStateIsBlurred.ts
Outdated
| export const appStateIsBlurredState = (appState: AppStateStatus) => | ||
| appState.match(blurredStateRegex); |
There was a problem hiding this comment.
Fix the function implementation to return a boolean.
The current implementation has two issues:
- The
match()method returnsnullor aRegExpMatchArray, not a boolean, which might cause unexpected behavior in conditional statements. - The function name is redundant with "State" appearing twice.
Apply this diff to fix the issues:
-export const appStateIsBlurredState = (appState: AppStateStatus) =>
- appState.match(blurredStateRegex);
+export const appStateIsBlurred = (appState: AppStateStatus): boolean =>
+ blurredStateRegex.test(appState);Additionally, using test() is more performant than match() for this use case as we only need to check if the pattern exists.
📝 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 appStateIsBlurredState = (appState: AppStateStatus) => | |
| appState.match(blurredStateRegex); | |
| export const appStateIsBlurred = (appState: AppStateStatus): boolean => | |
| blurredStateRegex.test(appState); |
utils/appState/appStateIsBlurred.ts
Outdated
| import { AppStateStatus } from "react-native"; | ||
|
|
||
| const blurredStateRegex = /inactive|background/; | ||
|
|
||
| export const appStateIsBlurredState = (appState: AppStateStatus) => | ||
| appState.match(blurredStateRegex); |
There was a problem hiding this comment.
💡 Codebase verification
Update required: Similar app state checks need to be migrated to the new utility
The verification reveals that while XmtpEngine.tsx has adopted the new appStateIsBlurredState utility, there are other components still using the direct regex pattern:
components/StateHandlers/WalletsStateHandler.tsx: Uses direct regexappState.current.match(/inactive|background/)components/StateHandlers/NotificationsStateHandler.tsx: Uses direct regexappState.current.match(/inactive|background/)
These components should be updated to use the new centralized utility for consistency and maintainability.
🔗 Analysis chain
Verify the impact of these changes across the codebase.
Since this is a new utility function that's part of app state management restructuring, we should verify its integration points.
Let me gather more specific information about the usage patterns to verify the integration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential usage of this new utility and related app state checks
# Test 1: Look for direct references to the new function
echo "Checking for direct usage of appStateIsBlurred(State)..."
rg -l "appStateIsBlurred(State)?"
# Test 2: Look for existing app state checks that might need to be updated
echo "Checking for existing app state checks..."
rg -l "AppStateStatus|appState\.match|inactive.*background"
# Test 3: Check the import updates mentioned in the summary
echo "Checking waitUntilAppActive import changes..."
rg -l "waitUntilAppActive"
Length of output: 896
Script:
#!/bin/bash
# Check the specific usage contexts of app state checks
# Look at XmtpEngine.tsx implementation since it uses the new utility
echo "Checking XmtpEngine.tsx implementation:"
rg -A 5 "appStateIsBlurred" components/XmtpEngine.tsx
# Check existing app state patterns in files that might need updates
echo "\nChecking app state patterns in other components:"
rg "AppStateStatus|appState\.match|inactive.*background" components/StateHandlers/NotificationsStateHandler.tsx components/StateHandlers/WalletsStateHandler.tsx
# Check the waitUntilAppActive implementation
echo "\nChecking waitUntilAppActive implementation:"
cat utils/appState/waitUntilAppActive.ts
Length of output: 2443
| useFocusEffect( | ||
| React.useCallback(() => { | ||
| if (firstTimeRef.current) { | ||
| firstTimeRef.current = false; | ||
| return; | ||
| } | ||
|
|
||
| refetch(); | ||
| }, [refetch]) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding debounce to prevent rapid refetches.
If the screen rapidly gains and loses focus, it could trigger multiple unnecessary refetches.
Consider using a debounce utility:
+import debounce from 'lodash/debounce';
export function useRefreshOnFocus<T>(refetch: () => Promise<T>) {
const firstTimeRef = React.useRef(true);
+ const debouncedRefetch = React.useMemo(
+ () => debounce(refetch, 1000, { leading: true, trailing: false }),
+ [refetch]
+ );
useFocusEffect(
React.useCallback(() => {
if (firstTimeRef.current) {
firstTimeRef.current = false;
return;
}
- refetch();
+ debouncedRefetch();
+
+ return () => {
+ debouncedRefetch.cancel();
+ };
}, [refetch])
);
}📝 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.
| useFocusEffect( | |
| React.useCallback(() => { | |
| if (firstTimeRef.current) { | |
| firstTimeRef.current = false; | |
| return; | |
| } | |
| refetch(); | |
| }, [refetch]) | |
| ); | |
| const debouncedRefetch = React.useMemo( | |
| () => debounce(refetch, 1000, { leading: true, trailing: false }), | |
| [refetch] | |
| ); | |
| useFocusEffect( | |
| React.useCallback(() => { | |
| if (firstTimeRef.current) { | |
| firstTimeRef.current = false; | |
| return; | |
| } | |
| debouncedRefetch(); | |
| return () => { | |
| debouncedRefetch.cancel(); | |
| }; | |
| }, [refetch]) | |
| ); |
| import { useMemo } from "react"; | ||
|
|
||
| export const useApprovedGroupsConversationList = () => { | ||
| const userAddress = useCurrentAccount()!; |
There was a problem hiding this comment.
Remove non-null assertion and add proper error handling.
The non-null assertion (!) on useCurrentAccount() could lead to runtime errors if the account is not available.
Consider this safer approach:
-const userAddress = useCurrentAccount()!;
+const userAddress = useCurrentAccount();
+if (!userAddress) {
+ return { data: { ids: [], byId: {} }, isLoading: false, error: new Error('No 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 userAddress = useCurrentAccount()!; | |
| const userAddress = useCurrentAccount(); | |
| if (!userAddress) { | |
| return { data: { ids: [], byId: {} }, isLoading: false, error: new Error('No account found') }; | |
| } |
| avatarComponent={avatarComponent} | ||
| ref={swipeableRef} | ||
| isUnread={showUnread} | ||
| showError={false} |
There was a problem hiding this comment.
Pass the correct 'showError' prop to display error indicators
Currently, showError is hardcoded to false, which means error states in messages may not be reflected in the UI. Previously, showError was calculated based on the message status and sender.
Update the showError prop to use the computed showError value:
- showError={false}
+ showError={showError}Committable suggestion was skipped due to low confidence.
| forwardRef<TextInput, ChatInputDumbProps>(function ChatInputDumb( | ||
| { inputHeight, messageToPrefill }: ChatInputDumbProps, | ||
| inputRef | ||
| ) { |
There was a problem hiding this comment.
Fix incorrect usage of inputRef when it's a function
The inputRef parameter can be either a ref object or a ref callback function, but the current code assumes it is always a ref object with a current property. Accessing inputRef.current when inputRef is a function will cause runtime errors and TypeScript errors.
Apply this diff to fix the issue:
forwardRef<TextInput, ChatInputDumbProps>(function ChatInputDumb(
{ inputHeight, messageToPrefill }: ChatInputDumbProps,
inputRef
) {
+ const localInputRef = useRef<TextInput>(null);
+ useImperativeHandle(inputRef, () => localInputRef.current);
// Replace all usages of 'inputRef.current' with 'localInputRef.current'
- if (inputRef.current) {
- inputRef.current?.focus();
+ if (localInputRef.current) {
+ localInputRef.current.focus();Also applies to: 140-145
| }, [ | ||
| numberOfLinesRef.current, | ||
| replyingToMessageRef.current, | ||
| mediaPreviewRef.current?.mediaURI, | ||
| ]); | ||
|
|
There was a problem hiding this comment.
Avoid including mutable ref.current values in dependency arrays
Including ref.current in dependency arrays of useCallback or useEffect is discouraged because changes to ref.current do not trigger re-renders, and the dependencies might not update as expected.
Remove ref.current values from the dependency array and ensure that the function uses the latest ref values when called.
}, [
- numberOfLinesRef.current,
- replyingToMessageRef.current,
- mediaPreviewRef.current?.mediaURI,
]);If you need the function to update based on changes to these values, consider using state variables instead of refs.
📝 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.
| }, [ | |
| numberOfLinesRef.current, | |
| replyingToMessageRef.current, | |
| mediaPreviewRef.current?.mediaURI, | |
| ]); | |
| }, []); | |
| useEffect(() => { | ||
| converseEventEmitter.on( | ||
| "triggerReplyToMessage", | ||
| (message: MessageToDisplay) => { | ||
| if (inputRef.current) { | ||
| inputRef.current?.focus(); | ||
| } | ||
| setReplyingToMessage(message); | ||
| replyingToMessageRef.current = message; | ||
| } | ||
| ); | ||
| return () => { | ||
| converseEventEmitter.off("triggerReplyToMessage"); | ||
| }; | ||
| }, [inputRef]); |
There was a problem hiding this comment.
Ensure event listeners are properly removed by specifying the listener function
When removing event listeners with converseEventEmitter.off, you need to pass both the event name and the listener function to properly unsubscribe. Otherwise, the listener will not be removed, leading to potential memory leaks.
Modify the code to store the listener function and use it in both on and off calls:
useEffect(() => {
+ const handleTriggerReplyToMessage = (message: MessageToDisplay) => {
+ if (inputRef.current) {
+ inputRef.current.focus();
+ }
+ setReplyingToMessage(message);
+ replyingToMessageRef.current = message;
+ };
- converseEventEmitter.on(
- "triggerReplyToMessage",
- (message: MessageToDisplay) => {
- if (inputRef.current) {
- inputRef.current?.focus();
- }
- setReplyingToMessage(message);
- replyingToMessageRef.current = message;
- }
- );
+ converseEventEmitter.on("triggerReplyToMessage", handleTriggerReplyToMessage);
return () => {
- converseEventEmitter.off("triggerReplyToMessage");
+ converseEventEmitter.off("triggerReplyToMessage", handleTriggerReplyToMessage);
};
}, [inputRef]);Repeat similar changes for other event listeners to ensure they are properly removed.
📝 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.
| useEffect(() => { | |
| converseEventEmitter.on( | |
| "triggerReplyToMessage", | |
| (message: MessageToDisplay) => { | |
| if (inputRef.current) { | |
| inputRef.current?.focus(); | |
| } | |
| setReplyingToMessage(message); | |
| replyingToMessageRef.current = message; | |
| } | |
| ); | |
| return () => { | |
| converseEventEmitter.off("triggerReplyToMessage"); | |
| }; | |
| }, [inputRef]); | |
| useEffect(() => { | |
| const handleTriggerReplyToMessage = (message: MessageToDisplay) => { | |
| if (inputRef.current) { | |
| inputRef.current.focus(); | |
| } | |
| setReplyingToMessage(message); | |
| replyingToMessageRef.current = message; | |
| }; | |
| converseEventEmitter.on("triggerReplyToMessage", handleTriggerReplyToMessage); | |
| return () => { | |
| converseEventEmitter.off("triggerReplyToMessage", handleTriggerReplyToMessage); | |
| }; | |
| }, [inputRef]); |
🧰 Tools
🪛 GitHub Check: tsc
[failure] 140-140:
'inputRef' is possibly 'null'.
[failure] 140-140:
Property 'current' does not exist on type 'MutableRefObject<TextInput | null> | ((instance: TextInput | null) => void)'.
[failure] 141-141:
'inputRef' is possibly 'null'.
[failure] 141-141:
Property 'current' does not exist on type 'MutableRefObject<TextInput | null> | ((instance: TextInput | null) => void)'.
| const onValidate = useCallback(async () => { | ||
| const waitForUploadToComplete = () => { | ||
| return new Promise<void>((resolve) => { | ||
| const interval = setInterval(() => { | ||
| if (mediaPreviewRef.current?.status === "uploaded") { | ||
| clearInterval(interval); | ||
| resolve(); | ||
| } | ||
| }, 200); | ||
| }); | ||
| }; | ||
| if (conversation) { | ||
| if (mediaPreviewRef.current) { | ||
| if (mediaPreviewRef.current?.status === "uploading") { | ||
| await waitForUploadToComplete(); | ||
| } | ||
| setReplyingToMessage(null); | ||
| replyingToMessageRef.current = null; | ||
| setInputValue(""); | ||
| numberOfLinesRef.current = 1; | ||
| handleAttachmentClosed(); | ||
| mediaPreviewRef.current = { | ||
| ...mediaPreviewRef.current, | ||
| status: "sending", | ||
| }; | ||
|
|
||
| setMediaPreview((prev) => | ||
| prev ? { ...prev, status: "sending" } : null | ||
| ); | ||
|
|
||
| if (preparedAttachmentMessageRef.current) { | ||
| await sendMessage(preparedAttachmentMessageRef.current); | ||
| preparedAttachmentMessageRef.current = null; | ||
| } | ||
| } | ||
|
|
||
| if (inputValue.length > 0) { | ||
| const messageToSend = { | ||
| conversation, | ||
| content: inputValue, | ||
| contentType: "xmtp.org/text:1.0", | ||
| referencedMessageId: replyingToMessage | ||
| ? replyingToMessage.id | ||
| : undefined, | ||
| }; | ||
| setInputValue(""); | ||
| setReplyingToMessage(null); | ||
| numberOfLinesRef.current = 1; | ||
| await new Promise((r) => setTimeout(r, 5)); | ||
| sendMessage(messageToSend); | ||
| } | ||
|
|
||
| converseEventEmitter.emit("scrollChatToMessage", { | ||
| index: 0, | ||
| }); | ||
| } | ||
| }, [ |
There was a problem hiding this comment.
Rename local onValidate to avoid shadowing the prop
The component defines a local onValidate function, which shadows the onValidate prop passed to the component. This can cause confusion and unintended behavior. Consider renaming the local function to something like handleValidate.
Apply this diff to resolve the naming conflict:
- const onValidate = useCallback(async () => {
+ const handleValidate = useCallback(async () => {
// Update all references to 'onValidate' within the component
- onSubmitEditing={() => {
- onValidate();
+ onSubmitEditing={() => {
+ handleValidate();
// And at the SendButton component
<SendButton
canSend={canSend}
- onPress={onValidate}
+ onPress={handleValidate}
sendButtonType={sendButtonType}
/>Ensure that the onValidate prop is used appropriately or remove it if it's not needed.
Committable suggestion was skipped due to low confidence.
Add timeout and error handling to prevent indefinite waiting
The waitForUploadToComplete function in onValidate waits indefinitely until the media upload status becomes "uploaded". If the upload fails or never completes, this could cause the app to hang. Consider adding a timeout and handling the "error" status to prevent this.
Apply this diff to enhance error handling:
const onValidate = useCallback(async () => {
const waitForUploadToComplete = () => {
return new Promise<void>((resolve, reject) => {
+ const timeout = setTimeout(() => {
+ clearInterval(interval);
+ reject(new Error("Upload timed out"));
+ }, 10000); // 10-second timeout
const interval = setInterval(() => {
if (mediaPreviewRef.current?.status === "uploaded") {
clearInterval(interval);
+ clearTimeout(timeout);
resolve();
+ } else if (mediaPreviewRef.current?.status === "error") {
+ clearInterval(interval);
+ clearTimeout(timeout);
+ reject(new Error("Upload failed"));
}
}, 200);
});
};
if (conversation) {
if (mediaPreviewRef.current) {
if (mediaPreviewRef.current?.status === "uploading") {
- await waitForUploadToComplete();
+ try {
+ await waitForUploadToComplete();
+ } catch (error) {
+ console.error(error);
+ // Optionally notify the user about the error
+ return;
+ }
}
// ... rest of the code ...This ensures the app doesn't hang indefinitely and handles upload errors gracefully.
📝 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 onValidate = useCallback(async () => { | |
| const waitForUploadToComplete = () => { | |
| return new Promise<void>((resolve) => { | |
| const interval = setInterval(() => { | |
| if (mediaPreviewRef.current?.status === "uploaded") { | |
| clearInterval(interval); | |
| resolve(); | |
| } | |
| }, 200); | |
| }); | |
| }; | |
| if (conversation) { | |
| if (mediaPreviewRef.current) { | |
| if (mediaPreviewRef.current?.status === "uploading") { | |
| await waitForUploadToComplete(); | |
| } | |
| setReplyingToMessage(null); | |
| replyingToMessageRef.current = null; | |
| setInputValue(""); | |
| numberOfLinesRef.current = 1; | |
| handleAttachmentClosed(); | |
| mediaPreviewRef.current = { | |
| ...mediaPreviewRef.current, | |
| status: "sending", | |
| }; | |
| setMediaPreview((prev) => | |
| prev ? { ...prev, status: "sending" } : null | |
| ); | |
| if (preparedAttachmentMessageRef.current) { | |
| await sendMessage(preparedAttachmentMessageRef.current); | |
| preparedAttachmentMessageRef.current = null; | |
| } | |
| } | |
| if (inputValue.length > 0) { | |
| const messageToSend = { | |
| conversation, | |
| content: inputValue, | |
| contentType: "xmtp.org/text:1.0", | |
| referencedMessageId: replyingToMessage | |
| ? replyingToMessage.id | |
| : undefined, | |
| }; | |
| setInputValue(""); | |
| setReplyingToMessage(null); | |
| numberOfLinesRef.current = 1; | |
| await new Promise((r) => setTimeout(r, 5)); | |
| sendMessage(messageToSend); | |
| } | |
| converseEventEmitter.emit("scrollChatToMessage", { | |
| index: 0, | |
| }); | |
| } | |
| }, [ | |
| const onValidate = useCallback(async () => { | |
| const waitForUploadToComplete = () => { | |
| return new Promise<void>((resolve, reject) => { | |
| const timeout = setTimeout(() => { | |
| clearInterval(interval); | |
| reject(new Error("Upload timed out")); | |
| }, 10000); // 10-second timeout | |
| const interval = setInterval(() => { | |
| if (mediaPreviewRef.current?.status === "uploaded") { | |
| clearInterval(interval); | |
| clearTimeout(timeout); | |
| resolve(); | |
| } else if (mediaPreviewRef.current?.status === "error") { | |
| clearInterval(interval); | |
| clearTimeout(timeout); | |
| reject(new Error("Upload failed")); | |
| } | |
| }, 200); | |
| }); | |
| }; | |
| if (conversation) { | |
| if (mediaPreviewRef.current) { | |
| if (mediaPreviewRef.current?.status === "uploading") { | |
| try { | |
| await waitForUploadToComplete(); | |
| } catch (error) { | |
| console.error(error); | |
| // Optionally notify the user about the error | |
| return; | |
| } | |
| } | |
| setReplyingToMessage(null); | |
| replyingToMessageRef.current = null; | |
| setInputValue(""); | |
| numberOfLinesRef.current = 1; | |
| handleAttachmentClosed(); | |
| mediaPreviewRef.current = { | |
| ...mediaPreviewRef.current, | |
| status: "sending", | |
| }; | |
| setMediaPreview((prev) => | |
| prev ? { ...prev, status: "sending" } : null | |
| ); | |
| if (preparedAttachmentMessageRef.current) { | |
| await sendMessage(preparedAttachmentMessageRef.current); | |
| preparedAttachmentMessageRef.current = null; | |
| } | |
| } | |
| if (inputValue.length > 0) { | |
| const messageToSend = { | |
| conversation, | |
| content: inputValue, | |
| contentType: "xmtp.org/text:1.0", | |
| referencedMessageId: replyingToMessage | |
| ? replyingToMessage.id | |
| : undefined, | |
| }; | |
| setInputValue(""); | |
| setReplyingToMessage(null); | |
| numberOfLinesRef.current = 1; | |
| await new Promise((r) => setTimeout(r, 5)); | |
| sendMessage(messageToSend); | |
| } | |
| converseEventEmitter.emit("scrollChatToMessage", { | |
| index: 0, | |
| }); | |
| } | |
| }, [ |
alexrisch
force-pushed
the
release/2.0.8
branch
from
455553f to
c80c528
Compare
alexrisch
force-pushed
the
ar/v3-split
branch
from
1301864 to
194052e
Compare
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (11)
components/ConversationFlashList.tsx (2)
76-81: Consider more explicit version suffixWhile the keyExtractor implementation is correct, consider making the version suffix more explicit for better maintainability.
- return typeof item === "string" ? item : item.topic + "v2"; + return typeof item === "string" ? item : `${item.topic}_version2`;
Line range hint
83-146: Consider extracting conversation item preparation logicThe renderItem function has grown complex with multiple responsibilities. Consider extracting the conversation item preparation logic to improve readability and maintainability.
const prepareConversationItemProps = ( conversation: ConversationWithLastMessagePreview, userAddress: string, profiles: Profiles, peersStatus: PeersStatus, colorScheme: ColorScheme, initialLoadDoneOnce: boolean, topicsData: TopicsData, openedConversationTopic: string ) => { const lastMessagePreview = conversation.lastMessagePreview; const socials = conversation.peerAddress ? getProfile(conversation.peerAddress, profiles)?.socials : undefined; return { conversationPeerAddress: conversation.peerAddress, conversationPeerAvatar: getPreferredAvatar(socials), colorScheme, conversationTopic: conversation.topic, conversationTime: lastMessagePreview?.message?.sent || conversation.createdAt, conversationName: conversationName(conversation, socials), showUnread: showUnreadOnConversation( initialLoadDoneOnce, lastMessagePreview, topicsData, conversation, userAddress ), lastMessagePreview: getLastMessagePreview(conversation, peersStatus, lastMessagePreview, userAddress), // ... other props }; };Then use it in renderItem:
if (!conversation.isGroup) { return <ConversationListItem {...prepareConversationItemProps( conversation, userAddress, profiles, peersStatus, colorScheme, initialLoadDoneOnce, topicsData, openedConversationTopic )} />; }components/Conversation/V3Conversation.tsx (2)
217-217: Remove debug console.log statementsRemove console.log statements before merging to production.
-console.log("messages", messages);-console.log("here1113", message?.contentTypeId);Also applies to: 236-236
199-201: Implement proper showChatInput logicThe TODO comment indicates that the showChatInput value needs to be updated. Currently, it's hardcoded to true.
Would you like help implementing the proper logic for determining showChatInput based on conversation state?
screens/Conversation.tsx (1)
325-335: Consider adding integration tests for the routing logic.The HOC implementation successfully separates V2/V3 logic, aligning with the PR objectives. However, to ensure reliable routing behavior:
- Add integration tests covering different routing scenarios:
- Group topic routing to V3
- Regular topic routing to V2
- mainConversationWithPeer routing
- Invalid/missing parameters
- Consider adding analytics/monitoring to track V2 vs V3 usage patterns
Would you like me to help generate the integration test suite for the routing logic?
components/GroupAvatar.tsx (1)
149-186: Consider future refactoring of shared rendering logicThere's still duplicate rendering logic between
GroupAvatarDumbandGroupAvatar. While not blocking for this PR's objectives, consider extracting this shared logic in a future PR to improve maintainability.This could be tracked as a separate task for future improvement.
Also applies to: 256-293
components/ConversationListItem/ConversationListItemDumb.tsx (1)
207-401: Well-aligned with V3 split architectureThis dumb component implementation aligns well with the PR objectives by:
- Separating presentation from logic
- Supporting both V2 and V3 functionality
- Maintaining clear component boundaries
Consider documenting the component's role in the V3 architecture to help other developers understand the split logic approach.
Consider adding a JSDoc comment to describe the component's role:
+/** + * A presentational component for rendering conversation list items. + * Part of the V3 split architecture, supporting both V2 and V3 functionality + * through props-based configuration. + */ export const ConversationListItemDumb = memo(function ConversationListItemDumb({queries/useGroupsConversationListQuery.ts (1)
22-24: Nitpick: UseDate.now()for concisenessInstead of using
new Date().getTime(), you can useDate.now()for a more concise and readable approach to get the current timestamp in milliseconds.Apply this diff to simplify the code:
- const beforeSync = new Date().getTime(); + const beforeSync = Date.now(); await client.conversations.syncGroups(); - const afterSync = new Date().getTime(); + const afterSync = Date.now();components/ConversationListItem/ConversationListItem.tsx (1)
56-56: Enhance accessibility by addingaccessibilityprops toPressable.To improve the accessibility of the
Pressablecomponent, consider addingaccessibilityRole,accessibilityLabel, and any other relevant accessibility props. This will assist users who rely on screen readers and other assistive technologies.components/V3GroupConversationListItem.tsx (2)
199-201: Implement or remove empty swipe handlers.The
onLeftSwipeandonRightSwipecallbacks are currently empty functions:const onLeftSwipe = useCallback(() => {}, []); const onRightSwipe = useCallback(() => {}, []);If these handlers are not needed, consider removing them to simplify the code. Otherwise, implement the desired functionality.
34-34: Address the TODO comment in theuseDatahook.There's a TODO comment indicating that certain items need attention:
// TODO ItemsPlease ensure that these items are resolved or provide additional context for any pending work.
Would you like assistance in implementing these items or creating a plan to address them?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
- components/Chat/ChatDumb.tsx (1 hunks)
- components/Conversation/ConversationTitleDumb.tsx (1 hunks)
- components/Conversation/V3Conversation.tsx (1 hunks)
- components/ConversationFlashList.tsx (5 hunks)
- components/ConversationListItem/ConversationListItem.tsx (1 hunks)
- components/ConversationListItem/ConversationListItemDumb.tsx (1 hunks)
- components/GroupAvatar.tsx (3 hunks)
- components/V3GroupConversationListItem.tsx (1 hunks)
- hooks/useApprovedGroupsConversationList.ts (1 hunks)
- hooks/useConversationListGroupItem.ts (1 hunks)
- package.json (1 hunks)
- queries/QueryKeys.ts (1 hunks)
- queries/useGroupMembersQuery.ts (2 hunks)
- queries/useGroupMessages.ts (1 hunks)
- queries/useGroupNameQuery.ts (2 hunks)
- queries/useGroupPhotoQuery.ts (2 hunks)
- queries/useGroupQuery.ts (2 hunks)
- queries/useGroupsConversationListQuery.ts (1 hunks)
- queries/useGroupsQuery.ts (3 hunks)
- screens/Conversation.tsx (2 hunks)
- screens/ConversationList.tsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- components/Conversation/ConversationTitleDumb.tsx
- hooks/useApprovedGroupsConversationList.ts
- hooks/useConversationListGroupItem.ts
- package.json
- queries/QueryKeys.ts
- queries/useGroupMembersQuery.ts
- queries/useGroupMessages.ts
- queries/useGroupNameQuery.ts
- queries/useGroupPhotoQuery.ts
- queries/useGroupsQuery.ts
🧰 Additional context used
🪛 GitHub Check: tsc
components/ConversationListItem/ConversationListItemDumb.tsx
[failure] 30-30:
Cannot find module '../../utils/device' or its corresponding type declarations.components/V3GroupConversationListItem.tsx
[failure] 311-311:
Property 'isBlockedChatView' is missing in type '{ onPress: () => void; onLongPress: () => void; renderRightActions: () => Element; renderLeftActions: () => Element; onLeftSwipe: () => void; selected: false; showError: false; showImagePreview: false; ... 6 more ...; isUnread: boolean; }' but required in type 'ConversationListItemDumbProps'.
🔇 Additional comments (29)
queries/useGroupQuery.ts (2)
Line range hint
55-70: LGTM! Cache management functions are well-implemented.The cache invalidation and data setting functions follow React Query best practices and provide necessary utilities for managing group data in the cache.
21-24:⚠️ Potential issueAdd error handling and retry configuration.
The
findGroupoperation could fail due to network issues or invalid group IDs. Consider adding error handling and configuring retry behavior.Apply this change:
const group = await client.conversations.findGroup( getGroupIdFromTopic(topic) ); + +return group; -return group;And add retry configuration to the query:
return useQuery({ queryKey: groupQueryKey(account, topic), + retry: (failureCount, error) => { + // Retry up to 3 times for network errors + return failureCount < 3 && error instanceof NetworkError; + }, queryFn: async () => {Likely invalid or redundant comment.
components/ConversationFlashList.tsx (2)
1-1: LGTM: Type changes align with V3 integrationThe updated imports and type changes properly support the V3/V2 split logic strategy, allowing for a gradual transition between versions.
Also applies to: 10-13, 32-32
Line range hint
166-180: Verify FlashList performance with large datasetsThe changes introduce new data merging and rendering logic. Please ensure performance remains acceptable with large datasets.
✅ Verification successful
The previous search failed because
--type testwasn't supported. Let's try a more comprehensive search to understand the FlashList implementation and any existing performance considerations.
FlashList performance optimizations are already in place
The component already implements key performance optimizations for FlashList:
- Uses
estimatedItemSize={77}on iOS and88on Android, which helps with initial rendering and scroll performance- Implements proper
keyExtractorfor stable item identification- Uses
extraDataprop correctly to control re-renders- Employs proper list ref management
The data merging with spread operator
[...(data?.ids || []), ...items]is a simple array concatenation that doesn't introduce significant performance overhead.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing performance tests rg -l "performance|benchmark" --type testLength of output: 72
Script:
#!/bin/bash # Search for test files and performance-related patterns more broadly fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx | xargs rg -l "performance|benchmark" # Look for FlashList usage and configuration rg -A 5 "FlashList" # Check for existing performance-related props or optimizations rg "estimatedItemSize|initialNumToRender|maxRenderPerBatch|windowSize"Length of output: 16446
components/Chat/ChatDumb.tsx (3)
52-90: LGTM! Well-structured styles implementation.The styles hook is well-implemented with proper memoization and theme awareness.
196-199: Uncomment messageListRef assignment.This is a duplicate of a previous review comment. The messageListRef assignment is still commented out, preventing scroll functionality from working.
208-216: Verify FlashList configuration.The current FlashList configuration might cause performance issues:
estimatedItemSizeseems low for a chat message (80px)- Disabled vertical scrollbar on Android might affect UX
screens/ConversationList.tsx (2)
84-89: LGTM: Clean integration of V3 supportThe hook integration and state type modification effectively support the coexistence of V2 and V3 conversations, aligning well with the PR's objective of maintaining functionality while transitioning to the new version.
136-141: LGTM: Correct dependency array updateThe addition of
approvedGroupsto the dependency array ensures that the list is properly updated when approved groups change.screens/Conversation.tsx (3)
1-1: LGTM: Clean import additions.The new imports for V3Conversation and isGroupTopic utility are well-organized and properly aliased.
Also applies to: 9-9
335-335: LGTM: Clean export modification.The export correctly wraps the new ConversationHoc with gestureHandlerRootHOC, maintaining gesture handling functionality.
325-333: 🛠️ Refactor suggestionConsider handling all route parameter cases.
The HOC handles the topic-based routing well, but there are a few edge cases to consider:
- The case where
route.params?.topicis undefined butroute.params?.mainConversationWithPeerexists- Adding error boundaries for safer component rendering
Let's verify the usage of mainConversationWithPeer in the codebase:
Consider enhancing the implementation:
const ConversationHoc = ({ route, navigation, }: NativeStackScreenProps<NavigationParamList, "Conversation">) => { + // Handle mainConversationWithPeer case first + if (!route.params?.topic && route.params?.mainConversationWithPeer) { + return <Conversation route={route} navigation={navigation} />; + } + if (route.params?.topic && isGroupTopic(route.params.topic)) { - return <V3Conversation route={route} navigation={navigation} />; + return ( + <ErrorBoundary fallback={<Conversation route={route} navigation={navigation} />}> + <V3Conversation route={route} navigation={navigation} /> + </ErrorBoundary> + ); } return <Conversation route={route} navigation={navigation} />; };components/GroupAvatar.tsx (3)
51-55: LGTM: Clean type definition for dumb componentThe
GroupAvatarDumbPropstype definition is well-structured and appropriately simplified for a presentational component.
134-189: LGTM: Well-implemented dumb componentThe
GroupAvatarDumbcomponent successfully implements a presentational component pattern, aligning with the PR's objective of splitting V2/V3 logic. The component correctly handles member rendering and position calculations.
212-212: LGTM: Smart optimization of data fetchingThe addition of
!pendingGroupMembersto theenabledcondition is a good optimization that prevents unnecessary API calls when pending members are provided.components/ConversationListItem/ConversationListItemDumb.tsx (3)
57-205: LGTM! Well-structured hooks and stylesThe custom hooks and styles are well-organized with:
- Proper memoization
- Platform-specific styling
- Consistent color theming
289-401: LGTM! Robust swipeable implementationThe swipeable implementation is well-structured with:
- Proper cleanup and memoization
- Platform-specific touch handling
- Comprehensive event handlers
259-267:⚠️ Potential issueAdd validation for imagePreviewUrl
This is a duplicate of a past review comment that's still valid. The Image component is rendered without validating that imagePreviewUrl is defined.
Apply this diff to add the validation:
- {showImagePreview && ( + {showImagePreview && imagePreviewUrl && ( <View style={styles.imagePreviewContainer}> <Image source={{ uri: imagePreviewUrl }} style={styles.imagePreview} contentFit="cover" /> </View> )}queries/useGroupsConversationListQuery.ts (9)
13-16: FunctiongroupConversationListKeyis correctly implementedThe key generation function effectively creates a unique query key based on the account, which is essential for caching and retrieving data in React Query.
18-18: Type aliasGroupConversationListTypeis appropriately definedDefining
GroupConversationListTypeusing theEntityObjectutility enhances type safety and clarity when working with group conversation data.
20-44: FunctiongroupsQueryFnfetches and processes group data effectivelyThe
groupsQueryFnfunction:
- Asynchronously retrieves the XMTP client for the given account.
- Synchronizes group conversations to ensure up-to-date data.
- Logs the duration of the synchronization process for debugging purposes.
- Fetches a list of groups with specific properties.
- Utilizes the
entifyfunction to transform the group data for efficient state management.
46-56: Ensure error handling for missingaccountparameterWhile the
enabledoption in theuseQueryhook checks for a truthyaccount, thegroupsQueryFnfunction does not handle scenarios whereaccountmight be undefined or invalid. Consider adding error handling ingroupsQueryFnto provide early feedback.Refer to the previous review comment for suggested changes.
58-63: FunctionfetchGroupsConversationListQueryis correctly implementedThe function manually fetches the group conversation list using the query client, which is useful for pre-fetching or updating data outside of React components.
65-69: FunctioninvalidateGroupsConversationListQuerycorrectly invalidates cacheInvalidating the query ensures that stale data is refetched, keeping the UI in sync with the latest group conversations.
71-75: FunctiongetGroupsConversationListQueryDataretrieves cached data appropriatelyAccessing the cached query data directly from the query client allows for synchronization of state across different parts of the application.
77-82: FunctionsetGroupsConversationListQueryDataupdates cached data effectivelyUpdating the query data ensures that the UI reflects new changes immediately without waiting for a refetch.
88-99: Prevent duplicate entries inidsarrayThere is a potential for duplicate
group.topicentries in theidsarray when adding a group that already exists. To maintain unique entries and prevent redundancy, filter out the group topic if it already exists.Refer to the previous review comment for the suggested fix.
components/ConversationListItem/ConversationListItem.tsx (2)
65-67: Ensure consistency with design system text presets.Check that the
presetvalues"bodyBold"inTitleComponentand"smaller"inSubtitleComponentalign with the presets defined in your design system. Consistent use of text styles helps maintain a uniform look and feel across the application.Also applies to: 73-87
48-54:⚠️ Potential issueVerify the correctness of the swipe direction handling in
onSwipeableWillClose.The
onSwipeableWillClosecallback may not provide thedirectionparameter as a string indicating"left"or"right". According to theSwipeablecomponent documentation, the parameter might be an event object or could be undefined. Please verify that the swipe direction is being handled correctly and adjust the implementation if necessary.
| export const useGroupConversationScreenQuery = ( | ||
| account: string, | ||
| topic: string | ||
| ) => { | ||
| return useQuery({ | ||
| queryKey: groupQueryKey(account, topic), | ||
| queryFn: async () => { | ||
| if (!topic) { | ||
| return null; | ||
| } | ||
| if (data instanceof Group) { | ||
| return data; | ||
| } | ||
| const client = xmtpClientByAccount[account]; | ||
| const client = (await getXmtpClient(account)) as ConverseXmtpClientType; | ||
| if (!client) { | ||
| return null; | ||
| } | ||
| // Recreate the group object with the client | ||
| return new Group(client!, data, (data as any)?.members); | ||
| const group = await client.conversations.findGroup( | ||
| getGroupIdFromTopic(topic) | ||
| ); | ||
| await group?.sync(); | ||
|
|
||
| return group; | ||
| }, | ||
| enabled: isGroupTopic(topic), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add staleTime configuration and reduce code duplication.
- Consider adding
staleTimeto prevent unnecessary refetches of group data that doesn't change frequently. - There's significant code duplication between this hook and
useGroupQuery.
Extract shared logic and apply these improvements:
+const fetchGroup = async (account: string, topic: string) => {
+ if (!topic) return null;
+ const client = (await getXmtpClient(account)) as ConverseXmtpClientType;
+ if (!client) return null;
+ return client.conversations.findGroup(getGroupIdFromTopic(topic));
+};
export const useGroupConversationScreenQuery = (
account: string,
topic: string
) => {
return useQuery({
queryKey: groupQueryKey(account, topic),
+ staleTime: 5 * 60 * 1000, // Consider data fresh for 5 minutes
queryFn: async () => {
- if (!topic) {
- return null;
- }
- const client = (await getXmtpClient(account)) as ConverseXmtpClientType;
- if (!client) {
- return null;
- }
- const group = await client.conversations.findGroup(
- getGroupIdFromTopic(topic)
- );
+ const group = await fetchGroup(account, topic);
await group?.sync();
return group;
},
enabled: isGroupTopic(topic),
});
};Committable suggestion was skipped due to low confidence.
components/ConversationFlashList.tsx
Outdated
| @@ -155,7 +164,7 @@ export default function ConversationFlashList({ | |||
| onScrollBeginDrag={onScroll} | |||
| alwaysBounceVertical={items.length > 0} | |||
| contentInsetAdjustmentBehavior="automatic" | |||
| data={items} | |||
| data={[...(data?.ids || []), ...items]} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing list data preparation
The spread operator for merging lists could impact performance with large datasets. Consider preparing the data outside the render cycle.
const prepareListData = useCallback(() => {
const groupIds = data?.ids || [];
return groupIds.concat(items);
}, [data?.ids, items]);
// In JSX:
data={prepareListData()}| const colorScheme = useColorScheme(); | ||
| const styles = useStyles(); | ||
|
|
||
| const hideInputIfFrameFocused = Platform.OS !== "web"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve platform check robustness.
The platform check could be more explicit to handle web platforms consistently.
-const hideInputIfFrameFocused = Platform.OS !== "web";
+const hideInputIfFrameFocused = Platform.OS === "ios" || Platform.OS === "android";📝 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 hideInputIfFrameFocused = Platform.OS !== "web"; | |
| const hideInputIfFrameFocused = Platform.OS === "ios" || Platform.OS === "android"; |
| const chatInputHeight = useSharedValue(0); | ||
| const chatInputDisplayedHeight = useDerivedValue(() => { | ||
| return frameTextInputFocused && hideInputIfFrameFocused | ||
| ? 0 | ||
| : chatInputHeight.value + DEFAULT_INPUT_HEIGHT; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider memoizing shared values.
The shared values and derived values should be memoized to prevent unnecessary recalculations during re-renders.
-const chatInputHeight = useSharedValue(0);
-const chatInputDisplayedHeight = useDerivedValue(() => {
+const chatInputHeight = useMemo(() => useSharedValue(0), []);
+const chatInputDisplayedHeight = useMemo(() => useDerivedValue(() => {
return frameTextInputFocused && hideInputIfFrameFocused
? 0
: chatInputHeight.value + DEFAULT_INPUT_HEIGHT;
-});
+}), [frameTextInputFocused, hideInputIfFrameFocused, chatInputHeight]);📝 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 chatInputHeight = useSharedValue(0); | |
| const chatInputDisplayedHeight = useDerivedValue(() => { | |
| return frameTextInputFocused && hideInputIfFrameFocused | |
| ? 0 | |
| : chatInputHeight.value + DEFAULT_INPUT_HEIGHT; | |
| }); | |
| const chatInputHeight = useMemo(() => useSharedValue(0), []); | |
| const chatInputDisplayedHeight = useMemo(() => useDerivedValue(() => { | |
| return frameTextInputFocused && hideInputIfFrameFocused | |
| ? 0 | |
| : chatInputHeight.value + DEFAULT_INPUT_HEIGHT; | |
| }), [frameTextInputFocused, hideInputIfFrameFocused, chatInputHeight]); |
| const handleOnLayout = useCallback(() => { | ||
| setTimeout(() => { | ||
| onReadyToFocus(); | ||
| }, 50); | ||
| }, [onReadyToFocus]); |
There was a problem hiding this comment.
Clean up setTimeout on unmount.
The setTimeout in handleOnLayout should be cleaned up to prevent potential memory leaks.
const handleOnLayout = useCallback(() => {
- setTimeout(() => {
- onReadyToFocus();
- }, 50);
+ const timer = setTimeout(() => {
+ onReadyToFocus();
+ }, 50);
+ return () => clearTimeout(timer);
}, [onReadyToFocus]);Committable suggestion was skipped due to low confidence.
| export type ConversationListItemDumbProps = { | ||
| title?: string; | ||
| subtitle?: string; | ||
| avatarComponent?: React.ReactNode; | ||
| rightComponent?: React.ReactNode; | ||
| renderRightActions: () => React.JSX.Element; | ||
| renderLeftActions: () => React.JSX.Element; | ||
| onLeftSwipe?: () => void; | ||
| onRightSwipe?: () => void; | ||
| onPress?: () => void; | ||
| onLongPress?: () => void; | ||
| onLeftActionPress?: () => void; | ||
| onWillLeftSwipe?: () => void; | ||
| onRightActionPress?: (defaultAction: () => void) => void; | ||
| onWillRightSwipe?: () => void; | ||
| contextMenuComponent: React.JSX.Element; | ||
| isUnread: boolean; | ||
| isBlockedChatView: boolean; | ||
| selected: boolean; | ||
| showError: boolean; | ||
| showImagePreview: boolean; | ||
| imagePreviewUrl: string | undefined; | ||
| }; |
There was a problem hiding this comment.
Consider making imagePreviewUrl required when showImagePreview is true
The imagePreviewUrl prop is optional but is used directly in the Image component when showImagePreview is true. This could lead to runtime errors.
Consider updating the type definition to enforce this constraint:
export type ConversationListItemDumbProps = {
// ... other props
- showImagePreview: boolean;
- imagePreviewUrl: string | undefined;
+ showImagePreview?: boolean;
+ imagePreviewUrl?: string;
+ // or alternatively
+ showImagePreview: boolean & (boolean extends true ? { imagePreviewUrl: string } : {});
};Committable suggestion was skipped due to low confidence.
| useAnimatedRef, | ||
| } from "react-native-reanimated"; | ||
|
|
||
| import { isDesktop } from "../../utils/device"; |
There was a problem hiding this comment.
💡 Codebase verification
Missing device utility module needs to be created
The isDesktop utility is imported from ../../utils/device but this module doesn't exist in the utils directory. A new device utility module needs to be created to provide the isDesktop functionality. The import is used to conditionally set the delayPressIn prop, which suggests this is a platform-specific utility.
- Create a new file
utils/device.tsto implement theisDesktoputility - Implement platform detection logic to determine if the current environment is desktop
🔗 Analysis chain
Fix missing module import
The import statement for ../../utils/device is failing. This module needs to be created or the import path needs to be corrected.
The previous search was too restrictive. Let's broaden our search to find any device-related files and check how isDesktop is used in the codebase to better understand the context and verify if this is a legitimate issue.
The previous results show that isDesktop is being used in the component, but we still need to locate the actual device utility file. Let's check common utility directories to find where this module should be or already exists.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the device utility module exists
fd -t f "device.ts" -x echo "Found device utility at: {}"
Length of output: 57
Script:
#!/bin/bash
# Let's try searching for device-related utilities with different extensions and names
fd -t f -e ts -e tsx -e js -e jsx "device" -x echo "Found device-related file: {}"
# Also search for isDesktop usage in the codebase
rg "isDesktop" -A 2
Length of output: 664
Script:
#!/bin/bash
# Let's check the utils directory structure to see if there's a device file with a different extension
fd -t f . utils/ -x echo "Found in utils: {}"
# Also check if there are any device-related utilities in other common locations
fd -t f . src/utils/ lib/utils/ -x echo "Found in other utils: {}"
Length of output: 4784
🧰 Tools
🪛 GitHub Check: tsc
[failure] 30-30:
Cannot find module '../../utils/device' or its corresponding type declarations.
| const defaultContainerStyle: ThemedStyle<ViewStyle> = (theme) => ({ | ||
| marginHorizontal: theme.spacing.sm, | ||
| marginVertical: theme.spacing.xs, | ||
| flex: 1, | ||
| width: "100%", | ||
| }); |
There was a problem hiding this comment.
Review the layout styles for potential redundancy.
In the defaultContainerStyle, both flex: 1 and width: "100%" are applied. Using flex: 1 along with width: "100%" might be redundant or lead to unexpected layout behavior. Consider removing one of these properties to ensure consistent styling.
| onRightSwipe={onRightSwipe} | ||
| avatarComponent={avatarComponent} | ||
| title={group?.name} | ||
| subtitle={`${timeToShow} ⋅ Message exam`} |
There was a problem hiding this comment.
Replace placeholder text in subtitle with actual content.
The subtitle prop currently uses a placeholder text 'Message exam'. Please update it to display the latest message preview or relevant information.
Consider modifying the subtitle to include the actual message content:
- subtitle={`${timeToShow} ⋅ Message exam`}
+ subtitle={`${timeToShow} ⋅ ${latestMessage}`}Ensure that latestMessage is defined and contains the appropriate text to display.
Committable suggestion was skipped due to low confidence.
| <ConversationListItemDumb | ||
| onPress={onPress} | ||
| onLongPress={onLongPress} | ||
| renderRightActions={renderRightActions} | ||
| renderLeftActions={renderLeftActions} | ||
| onLeftSwipe={onLeftSwipe} | ||
| selected={false} | ||
| showError={false} | ||
| showImagePreview={false} | ||
| imagePreviewUrl={undefined} | ||
| onRightSwipe={onRightSwipe} | ||
| avatarComponent={avatarComponent} | ||
| title={group?.name} | ||
| subtitle={`${timeToShow} ⋅ Message exam`} | ||
| contextMenuComponent={contextMenuComponent} | ||
| isUnread={isUnread} | ||
| /> |
There was a problem hiding this comment.
Add missing isBlockedChatView prop to ConversationListItemDumb.
The ConversationListItemDumb component requires the isBlockedChatView prop, but it is not being provided. This omission leads to a TypeScript error and may cause runtime issues.
Apply this diff to fix the issue:
<ConversationListItemDumb
onPress={onPress}
onLongPress={onLongPress}
renderRightActions={renderRightActions}
renderLeftActions={renderLeftActions}
onLeftSwipe={onLeftSwipe}
selected={false}
showError={false}
showImagePreview={false}
imagePreviewUrl={undefined}
onRightSwipe={onRightSwipe}
avatarComponent={avatarComponent}
title={group?.name}
- subtitle={`${timeToShow} ⋅ Message exam`}
+ subtitle={`${timeToShow} ⋅ ${messagePreview}`}
contextMenuComponent={contextMenuComponent}
isUnread={isUnread}
+ isBlockedChatView={isBlockedChatView}
/>Ensure that isBlockedChatView is defined and passed correctly from the useData hook or appropriate state.
Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: tsc
[failure] 311-311:
Property 'isBlockedChatView' is missing in type '{ onPress: () => void; onLongPress: () => void; renderRightActions: () => Element; renderLeftActions: () => Element; onLeftSwipe: () => void; selected: false; showError: false; showImagePreview: false; ... 6 more ...; isUnread: boolean; }' but required in type 'ConversationListItemDumbProps'.
Started some work on V3 Message Component Cleaned up several Components Cleaned up list merging Better query handling for messages
There was a problem hiding this comment.
Actionable comments posted: 25
🧹 Outside diff range and nitpick comments (14)
components/Chat/Message/MessageSender.tsx (2)
11-18: Add accessibility attributes for better screen reader support.The component should be more accessible to screen readers.
const MessageSenderDumb = ({ name }: MessageSenderDumbProps) => { const styles = useStyles(); return ( - <View style={styles.groupSenderWrapper}> - <Text style={styles.groupSender}>{name}</Text> + <View style={styles.groupSenderWrapper} accessibilityRole="text"> + <Text + style={styles.groupSender} + accessibilityLabel={`Message from ${name}`} + > + {name} + </Text> </View> ); };
1-63: Consider adding component documentation and examples.Since this file introduces new components for the V3 split, it would be beneficial to add JSDoc documentation with usage examples. This will help other developers understand when to use each component variant (V2 vs V3).
/** * @component MessageSender * @description Renders the sender's name for V2 conversations * @example * ```tsx * <MessageSender senderAddress="0x123..." /> * ``` */ /** * @component V3MessageSender * @description Renders the sender's name for V3 conversations using inbox ID * @example * ```tsx * <V3MessageSender inboxId="inbox_123" /> * ``` */components/Chat/Frame/FramesPreviews.tsx (2)
Line range hint
19-31: Consider extracting frame state management to a custom hook.The component manages multiple pieces of state across different mechanisms (ref, context, and useState). This complexity could be encapsulated in a custom hook like
useFrameManagementto improve maintainability and reusability.Example implementation:
function useFrameManagement(message: MessageToDisplay) { const messageId = useRef<string>(); const [framesForMessage, setFramesForMessage] = useState<{ [messageId: string]: FrameWithType[]; }>({}); useEffect(() => { if (message.id !== messageId.current) { messageId.current = message.id; setFramesForMessage({ [message.id]: useFramesStore .getState() .getFramesForURLs(message.converseMetadata?.frames || []), }); } }, [message.id]); return { frames: framesForMessage[message.id] || [], updateFrames: setFramesForMessage }; }
Line range hint
33-47: Add error handling and cleanup for frame fetching.The current implementation has potential issues:
- No cleanup for pending fetch requests if the component unmounts
- Missing error handling for failed fetches
- Possible race conditions with rapid message changes
Consider implementing this improved version:
useEffect(() => { let mounted = true; if (!tagsFetchedOnceForMessage.current[message.id]) { tagsFetchedOnceForMessage.current[message.id] = true; fetchFramesForMessage(account, message) .then((frames: FramesForMessage) => { if (mounted) { setFramesForMessage({ [frames.messageId]: frames.frames }); } }) .catch((error) => { console.error('Failed to fetch frames:', error); // Consider showing an error state to the user }); } return () => { mounted = false; }; }, [message.id, account]);screens/Group.tsx (1)
Line range hint
41-54: Consider reducing prop drilling with React Context.The topic prop is being passed down to multiple child components. Consider using React Context to avoid prop drilling and make the topic available to all group-related components.
Example implementation:
// GroupContext.tsx const GroupContext = React.createContext<{topic: string} | null>(null); export const GroupProvider: React.FC<{topic: string; children: React.ReactNode}> = ({ topic, children }) => ( <GroupContext.Provider value={{topic}}> {children} </GroupContext.Provider> ); export const useGroupContext = () => { const context = React.useContext(GroupContext); if (!context) { throw new Error('useGroupContext must be used within a GroupProvider'); } return context; };components/Chat/Frame/FrameBottom.tsx (2)
Line range hint
40-43: Fix incorrect prop reference in background color logic.The code still uses
message.fromMeinstead of the newmessageFromMeprop. This will cause runtime errors as themessageprop no longer exists.Apply this fix:
- backgroundColor: message.fromMe + backgroundColor: messageFromMe ? myMessageInnerBubbleColor(colorScheme) : messageInnerBubbleColor(colorScheme),
There's an inconsistency in the FrameBottom component that needs to be fixed
The verification revealed that while the component accepts
messageFromMeas a prop, it still usesmessage.fromMein the render logic. This needs to be corrected to use the prop instead.Location that needs attention:
components/Chat/Frame/FrameBottom.tsx: Replacemessage.fromMewithmessageFromMein the backgroundColor style calculation🔗 Analysis chain
Line range hint
1-110: Verify consistent prop usage across the codebase.Let's ensure all related components have been updated to use the new
messageFromMeprop structure.This will help identify:
- Any remaining instances of the old
message.fromMepattern that need to be updated- Confirm consistent typing of
messageFromMeas boolean in Frame components🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of message.fromMe and verify messageFromMe usage echo "Checking for remaining message.fromMe instances..." rg "message\.fromMe" --type tsx echo -e "\nVerifying messageFromMe prop usage in Frame components..." rg "messageFromMe.*boolean" --type tsx components/Chat/Frame/Length of output: 395
Script:
#!/bin/bash # Let's try without file type filtering since tsx extension isn't recognized echo "Checking for remaining message.fromMe instances..." rg "message\.fromMe" echo -e "\nVerifying messageFromMe prop usage in Frame components..." rg "messageFromMe.*boolean" components/Chat/Frame/ echo -e "\nChecking all messageFromMe usages in Frame components..." rg "messageFromMe" components/Chat/Frame/Length of output: 6190
components/Chat/Message/MessageSenderAvatar.tsx (1)
54-103: Consider extracting shared logic between V2 and V3 components.Both
MessageSenderAvatarandV3MessageSenderAvatarshare similar logic for profile lookup and navigation. Consider extracting this common functionality into a custom hook to reduce code duplication.Example implementation:
const useMessageSender = (id: string, isV3: boolean) => { const address = useInboxIdStore((s) => isV3 ? s.byInboxId[id]?.[0] : (s.byInboxId[id]?.[0] ?? id) ); const senderSocials = useProfilesStore( (s) => getProfile(address, s.profiles)?.socials ); const openProfile = useCallback(() => { if (address) { navigate("Profile", { address }); } }, [address]); return { address, senderSocials, openProfile, }; };components/ConversationFlashList.tsx (1)
Line range hint
81-134: Consider splitting renderItem logic into separate components.The renderItem function handles multiple types of items with complex conditional logic. This could be simplified using component composition.
Consider creating a separate component:
type ConversationItemProps = { item: ConversationFlatListItem | string; colorScheme: ColorSchemeName; userAddress: string; // ... other props }; const ConversationItem: React.FC<ConversationItemProps> = ({ item, ...props }) => { if (typeof item === "string") { return <V3GroupConversationListItem topic={item} />; } if (item.topic === "hiddenRequestsButton") { return <HiddenRequestsButton {...item} />; } if (item.isGroup) { return null; } return <ConversationListItem {...getConversationProps(item, props)} />; }; // In ConversationFlashList: const renderItem = useCallback( ({ item }) => ( <ConversationItem item={item} colorScheme={colorScheme} userAddress={userAddress} {...otherProps} /> ), [colorScheme, userAddress, /* other deps */] );components/Chat/Frame/FramePreview.tsx (2)
Line range hint
88-98: Enhance race condition protection in image loading.The message ID checks are well-placed to prevent race conditions, but the error handling could be improved:
if (fetchingInitialForMessageId.current !== messageIdRef.current) { + logger.debug('Aborting image fetch due to message change'); return; }Consider adding a cleanup function to the useEffect to abort any pending image fetches when the component unmounts or messageId changes.
Also applies to: 109-110, 121-122
333-336: Consider extracting opacity calculation.The nested ternary operation for opacity calculation could be simplified for better readability.
-opacity: - postingActionForButton !== undefined || !firstImageRefreshed - ? messageFromMe - ? 0.8 - : 0.4 - : 1, +const getOpacity = () => { + if (postingActionForButton !== undefined || !firstImageRefreshed) { + return messageFromMe ? 0.8 : 0.4; + } + return 1; +}; +opacity: getOpacity(),components/ConversationListItem/ConversationListItemDumb.tsx (1)
392-392: Consider documenting or adjusting the leftThreshold valueThe
leftThresholdis set to 10000 with a comment "Never trigger opening". This approach, while functional, could be improved.Consider either:
- Using
Infinityto better express the intent- Or removing the threshold if the left swipe action is not needed
- leftThreshold={10000} // Never trigger opening + leftThreshold={Infinity} // Prevent automatic openingutils/conversation.ts (1)
Line range hint
428-481: Consider documenting V2/V3 split strategy.The changes consistently move group conversation handling to V3 components. To ensure a smooth transition:
- Document the split strategy and migration path
- Consider adding feature flags to gradually roll out changes
- Ensure feature parity between V2 and V3 for group conversations
- Add tests to verify both V2 and V3 functionality
Consider creating a migration guide in the documentation to help users and developers understand:
- Which features are handled by V2 vs V3
- How to migrate existing group conversations
- Timeline for deprecating V2 group conversation handling
utils/xmtpRN/conversations.ts (1)
Line range hint
321-341: Clarify the TODO comment and document the function's purpose.The TODO comment "What is this doing? how to make it work with change" needs clarification. The function appears to:
- Fetch existing groups
- Sync all groups from the network
- Refresh and return updated groups
Consider replacing the TODO with proper documentation:
-// TODO:// What is this doing? how to make it work with change +/** + * Synchronizes and refreshes all groups for the given client. + * 1. Fetches existing groups from the local query cache + * 2. Triggers a full sync with the network + * 3. Refreshes the local cache with updated group data + * @param client The XMTP client instance + * @returns Array of updated group instances + */Would you like me to help document this function more thoroughly or create a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (23)
- components/Chat/Frame/FrameBottom.tsx (5 hunks)
- components/Chat/Frame/FramePreview.tsx (11 hunks)
- components/Chat/Frame/FramesPreviews.tsx (1 hunks)
- components/Chat/Input/InputDumb.tsx (1 hunks)
- components/Chat/Message/Message.tsx (3 hunks)
- components/Chat/Message/MessageSender.tsx (1 hunks)
- components/Chat/Message/MessageSenderAvatar.tsx (1 hunks)
- components/Chat/Message/TextMessage.tsx (1 hunks)
- components/Conversation/V3Conversation.tsx (1 hunks)
- components/ConversationFlashList.tsx (4 hunks)
- components/ConversationListItem.tsx (2 hunks)
- components/ConversationListItem/ConversationListItemDumb.tsx (1 hunks)
- components/V3GroupConversationListItem.tsx (1 hunks)
- queries/useGroupMembersQuery.ts (2 hunks)
- queries/useGroupMessages.ts (1 hunks)
- queries/useGroupsConversationListQuery.ts (1 hunks)
- queries/useGroupsQuery.ts (2 hunks)
- queries/useRefreshOnFocus.ts (1 hunks)
- screens/Group.tsx (2 hunks)
- utils/conversation.ts (2 hunks)
- utils/mergeLists.ts (1 hunks)
- utils/xmtpRN/conversations.ts (6 hunks)
- utils/xmtpRN/messages.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- queries/useGroupMembersQuery.ts
- queries/useGroupMessages.ts
- queries/useGroupsQuery.ts
- utils/mergeLists.ts
🧰 Additional context used
🪛 GitHub Check: tsc
components/Chat/Input/InputDumb.tsx
[failure] 36-36:
Cannot find module '../../../utils/device' or its corresponding type declarations.
[failure] 140-140:
'inputRef' is possibly 'null'.
[failure] 140-140:
Property 'current' does not exist on type 'MutableRefObject<TextInput | null> | ((instance: TextInput | null) => void)'.
[failure] 141-141:
'inputRef' is possibly 'null'.
[failure] 141-141:
Property 'current' does not exist on type 'MutableRefObject<TextInput | null> | ((instance: TextInput | null) => void)'.components/ConversationListItem/ConversationListItemDumb.tsx
[failure] 30-30:
Cannot find module '../../utils/device' or its corresponding type declarations.
[failure] 295-295:
Type 'string' is not assignable to type '"search" | "link" | "message" | "key" | "block" | "menu" | "xmark" | "xmark.circle.fill" | "plus" | "arrow.up.right" | "arrow.down" | "paperplane" | "account_circle" | "square.and.pencil" | ... 37 more ... | "star"'.
🪛 Biome
components/Conversation/V3Conversation.tsx
[error] 244-244: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 245-245: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (35)
queries/useRefreshOnFocus.ts (2)
1-3: LGTM! Imports are clean and necessary.
4-17: Verify the hook's integration with useGroupMessages.Let's ensure the hook is properly integrated with the group messages functionality.
✅ Verification successful
Let me check the implementation of useGroupMessages to verify the proper integration.
Hook is properly integrated with useGroupMessages
The
useRefreshOnFocushook is correctly integrated within theuseGroupMessagesquery:
- It's properly imported from the correct path
- It's used with the
refetchfunction from the query data- The integration follows the hook's contract by passing a function that returns a Promise
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how useRefreshOnFocus is imported and used # Search for imports of useRefreshOnFocus echo "Checking imports of useRefreshOnFocus:" rg "import.*useRefreshOnFocus" -A 3 # Search for usage of useRefreshOnFocus echo "\nChecking usage of useRefreshOnFocus:" rg "useRefreshOnFocus\(" -A 3Length of output: 734
Script:
#!/bin/bash # Get the implementation of useGroupMessages ast-grep --pattern 'export const useGroupMessages = ($$$) => { $$$ }'Length of output: 1308
components/Chat/Message/TextMessage.tsx (1)
12-31: Implementation aligns well with PR objectives.This component follows the "dumb component" pattern as intended, focusing purely on presentation without business logic. This aligns perfectly with the PR's goal of splitting V2/V3 logic and maintaining existing functionality.
components/Chat/Message/MessageSender.tsx (1)
1-9: LGTM! Clean imports and well-defined types.components/Chat/Frame/FramesPreviews.tsx (1)
62-64: Well-structured prop decomposition.Breaking down the message prop into specific properties (
messageId,messageTopic,messageFromMe) aligns well with the PR's objective of creating "dumb components". This makes the component interface more explicit and type-safe.screens/Group.tsx (1)
7-7: Verify the removal of query invalidation logic.The removal of multiple query invalidation imports suggests a significant change in how group data updates are handled. Please ensure that group data remains properly synchronized without these invalidations.
Let's verify the query invalidation usage across the codebase:
Also applies to: 25-25
✅ Verification successful
Let me gather more information about how the group updates are now handled with the new changes.
Based on the results, I can now provide a final response:
Query invalidation has been properly replaced with a new data synchronization mechanism
The removed query invalidation imports have been replaced with a robust alternative approach:
- Group updates are now handled through
handleGroupUpdatedMessage.tswhich still maintains the query invalidation for real-time updates- Direct mutations (setGroup* functions) use optimistic updates with proper rollback on errors
- The codebase uses a combination of:
- Centralized state management via useChatStore for UI state
- Query invalidation for backend sync
- Optimistic updates for better UX
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining usages of the removed query invalidation functions rg -l "invalidateGroup(Consent|Description|Members|Name|Photo)Query" # Check for alternative update mechanisms rg -A 3 "useGroupConversationScreenQuery"Length of output: 3927
Script:
#!/bin/bash # Check for useChatStore usage related to group updates rg -A 3 "useChatStore.*group" # Check the implementation of handleGroupUpdatedMessage cat data/helpers/messages/handleGroupUpdatedMessage.ts # Look for any new group update mechanisms rg -A 3 "updateGroup|setGroup"Length of output: 38998
components/Chat/Frame/FrameBottom.tsx (4)
14-14: LGTM! Props interface changes improve type safety.The change from
messageobject to explicitmessageFromMe: booleanprop improves type safety and makes the component's requirements clearer.Also applies to: 24-24
57-57: LGTM! Correct prop forwarding to FrameTextInput.The
messageFromMeprop is correctly passed down to the child component.
74-74: LGTM! Correct prop forwarding to FrameButton.The
messageFromMeprop is correctly passed down to the child component.
84-84: LGTM! Correct usage in text color styling.The
messageFromMeprop is correctly used for conditional text color styling.queries/useGroupsConversationListQuery.ts (2)
1-21: LGTM! Clean imports and type definitions.The imports are well-organized and the type definitions are clear and properly structured.
54-64: LGTM! Well-implemented query hook.The hook follows React Query best practices with proper typing and conditional enabling.
components/ConversationFlashList.tsx (2)
9-11: Verify type compatibility across the codebase.The change to support both
ConversationFlatListItemandstringtypes aligns with the PR's objective of supporting V3 conversations. However, this type change could affect other components that consume these items.#!/bin/bash # Search for components that might be affected by the type change rg -t typescript -t tsx "items.*ConversationFlatListItem\[\]"Also applies to: 30-30
81-84: Clarify the removal of group conversation rendering.The change to return
nullfor group conversations (line 100) seems abrupt and might lead to confusion. If this is intentional as part of the V2/V3 transition, please add a comment explaining why group conversations are being filtered out.Also applies to: 100-100
components/Conversation/V3Conversation.tsx (1)
243-258:⚠️ Potential issueFix switch case variable scoping
Variables declared in the switch case can leak into other cases. This was flagged by static analysis.
Wrap the case block in curly braces:
case "text": { let content = ""; let hideBackground = false; try { content = message.content() as string; hideBackground = isAllEmojisAndMaxThree(content); } catch { content = message.fallback ?? ""; } return ( <TextMessage fromMe={fromMe} hideBackground={hideBackground} content={content} /> ); }Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 244-244: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 245-245: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
components/Chat/Frame/FramePreview.tsx (5)
31-38: Props restructuring aligns with "dumb component" pattern.The decomposition of the
messageprop into atomic properties (messageId,messageTopic,messageFromMe) follows good practices for creating "dumb components". This change:
- Reduces coupling between the component and message object structure
- Makes the component's dependencies explicit
- Simplifies testing and maintenance
177-177: LGTM: Proper usage of conversation topic.The change from accessing topic through message object to using messageTopic prop is consistent with the component's new structure.
352-352: LGTM: Consistent prop passing.The messageFromMe prop is correctly passed to FrameBottom component, maintaining consistency with the new component structure.
61-61: Verify cleanup of stale state on message changes.The messageId reference handling looks good, but there might be edge cases around cleanup. Consider:
- Are there any async operations that need to be cancelled?
- Could any state updates occur after the component unmounts?
#!/bin/bash # Search for async operations and cleanup patterns rg -A 3 "useEffect|setTimeout|setInterval" components/Chat/Frame/FramePreview.tsxAlso applies to: 67-77
140-140: Review useEffect dependencies.The effect depends on
[firstFrameLoaded, frame, messageId]but uses other values from the closure scope. Verify iffirstImageRefreshedshould be included in the dependency array.✅ Verification successful
Let me search for more context around this useEffect to understand the complete scope.
Let me try a different approach to get the context of this useEffect.
After analyzing the complete file, I can now provide the final response:
The dependency array is correct as is.
The
firstImageRefreshedstate is only set within the effect and is not used as a dependency for any computation inside the effect. It's merely used to track the state of image refreshing. The current dependencies[firstFrameLoaded, frame, messageId]correctly cover all the external values used in the effect's logic:
firstFrameLoadedcontrols the main conditionframeis used to checkframe.isInitialFrameand get frame imagemessageIdis compared withmessageIdRef.current🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for variables used in the effect rg -A 10 "handleInitialImage" components/Chat/Frame/FramePreview.tsxLength of output: 347
Script:
#!/bin/bash # Get more context around the useEffect implementation rg -B 5 -A 15 "useEffect\(\(\) => \{.*handleInitialImage" components/Chat/Frame/FramePreview.tsxLength of output: 98
Script:
#!/bin/bash # Search for the complete file content to understand the context cat components/Chat/Frame/FramePreview.tsxLength of output: 12959
utils/xmtpRN/messages.ts (1)
181-184: Clarify the intention of the commented-out group conversation code.The commented-out code block suggests an incomplete or in-progress feature for handling group conversations. This could lead to confusion about the current state of group message handling, especially since there are other group-related functions in the file (e.g.,
syncGroupsMessages,handleGroupUpdatedMessage).Consider one of the following actions:
- If this is a planned feature, remove the commented code and create a GitHub issue to track the implementation
- If this is actively being worked on, add a TODO comment explaining the plan and any dependencies
- If this is no longer needed, remove the commented code entirely
Let's check if there are other instances of commented-out group-related code:
Since this PR is about "V3 Split Logic", consider documenting the group conversation handling strategy for both V2 and V3 in the codebase to maintain clarity during the transition.
components/ConversationListItem/ConversationListItemDumb.tsx (2)
261-269: Reusing existing review commentA previous review already identified the potential issue with
imagePreviewUrlbeing undefined whenshowImagePreviewis true.
291-301: Reusing existing review commentsPrevious reviews already identified:
- Missing
onPresshandler inrenderLeftActions- Inverted destructive action styling in
renderRightActionsAlso applies to: 303-328
🧰 Tools
🪛 GitHub Check: tsc
[failure] 295-295:
Type 'string' is not assignable to type '"search" | "link" | "message" | "key" | "block" | "menu" | "xmark" | "xmark.circle.fill" | "plus" | "arrow.up.right" | "arrow.down" | "paperplane" | "account_circle" | "square.and.pencil" | ... 37 more ... | "star"'.utils/conversation.ts (1)
428-430: Verify group conversation handling in V3 components.The early return for group conversations suggests these are now handled elsewhere. Please ensure that:
- Group conversations are properly handled by V3-specific components
- This change doesn't break existing group conversation functionality for users still on V2
Let's verify the V3 group conversation implementation:
✅ Verification successful
Let me gather more information about the V3 group conversation implementation by examining the identified files and their dependencies.
Based on the gathered information, I can now generate the final response:
Early return for group conversations is correctly handled by V3-specific components
The codebase shows a clear separation of concerns where:
- Group conversations are handled by a dedicated
V3GroupConversationListItemcomponent with full implementationConversationFlashListcorrectly routes group conversations to V3 components- The early return in
utils/conversation.tsis intentional as V3 implements its own sorting and preview computation logic🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for V3 group conversation handling echo "Searching for V3 group conversation components..." rg -l "V3.*Group.*Conversation" echo "Searching for group conversation logic in V3 components..." ast-grep --pattern 'const V3 = { $$$ group$_ { $$$ } $$$ }'Length of output: 423
Script:
#!/bin/bash # Check V3GroupConversationListItem implementation echo "=== V3GroupConversationListItem.tsx content ===" cat components/V3GroupConversationListItem.tsx echo -e "\n=== ConversationFlashList.tsx group handling ===" rg -A 5 "isGroup" components/ConversationFlashList.tsx echo -e "\n=== Searching for other V3 group conversation handlers ===" rg -l "V3.*group" --type ts --type tsx echo -e "\n=== Check imports of V3GroupConversationListItem ===" rg -l "V3GroupConversationListItem"Length of output: 10529
components/Chat/Message/Message.tsx (4)
32-35: LGTM: Clean component separationThe new imports align well with the PR's objective of implementing "dumb components" and splitting V2/V3 logic. This modular approach will make it easier to maintain and update components independently.
209-213: LGTM: Clean TextMessage implementationGood use of the new TextMessage component with appropriate props and content fallback handling. This separation of concerns aligns with the PR's goal of implementing dumb components.
329-334: LGTM: Clean MessageSenderAvatar implementationGood implementation of the MessageSenderAvatar component with appropriate conditional rendering and props passing. The series handling through hasNextMessageInSeries prop helps maintain visual consistency in message threads.
338-341: Referring to previous review commentThe complex conditional rendering logic for MessageSender remains. Please refer to the previous review comment about simplifying this logic using a shouldShowMessageSender variable.
utils/xmtpRN/conversations.ts (2)
Line range hint
357-375: LGTM! Clean implementation of conversation handling.The changes to
loadConversationseffectively handle both new and known conversations with proper error handling and data persistence.
5-5: Verify integration with V3 conversation list.The addition of
addGroupToGroupsConversationListQueryaligns with the PR's objective of splitting V2/V3 logic. Let's verify its integration:Also applies to: 280-282
✅ Verification successful
Both group query functions serve different purposes and are correctly used
The verification shows that:
addGroupToGroupsQueryis used in the sync flow for updating the groups query cacheaddGroupToGroupsConversationListQueryis used in the streaming flow for updating the conversation list cache- Both functions have similar but separate implementations, working with different query keys and data structures
- The usage aligns with the PR's objective of splitting V2/V3 logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation and usage of addGroupToGroupsConversationListQuery # Test 1: Check if the function is properly exported and used rg -A 2 "addGroupToGroupsConversationListQuery" # Test 2: Look for any potential duplicate group handling logic ast-grep --pattern 'addGroupToGroupsQuery($$$)'Length of output: 980
Script:
#!/bin/bash # Let's verify the relationship between these two similar functions # Check implementation of both functions rg -A 10 "^export const addGroupToGroupsQuery" rg -A 10 "^export const addGroupToGroupsConversationListQuery" # Check all usages to understand the context rg "addGroupToGroupsQuery|addGroupToGroupsConversationListQuery" --context 3Length of output: 4563
components/V3GroupConversationListItem.tsx (1)
96-109: Ensure array lengths match when mappingdataandmemberAddressesWhen mapping over
datato creatememberData, ensure thatdataandmemberAddressesare arrays of the same length to prevent index out-of-bounds errors.Run the following script to check for length mismatches:
Confirm that the lengths are equal before mapping.
components/ConversationListItem.tsx (4)
33-36: Imports from 'react-native-reanimated' are appropriateThe newly added imports
useSharedValue,useAnimatedRef, andrunOnJSfromreact-native-reanimatedare correctly imported and utilized in the component.
41-41: Correctly imported 'ConversationListItemDumb' componentThe import statement for
ConversationListItemDumbaccurately references the correct path, ensuring the component is available for use.
489-492: 'onLeftSwipe' callback functions as intendedThe
onLeftSwipefunction properly resets thetoggleUnreadStatusOnCloseflag and invokestoggleReadStatusto update the read status of the conversation.
495-512: Proper use of 'ConversationListItemDumb' with necessary propsThe
ConversationListItemDumbcomponent is utilized correctly, passing all required props including event handlers and display properties. This ensures the component behaves as expected within the conversation list.
| export function useRefreshOnFocus<T>(refetch: () => Promise<T>) { | ||
| const firstTimeRef = React.useRef(true); | ||
|
|
||
| useFocusEffect( | ||
| React.useCallback(() => { | ||
| if (firstTimeRef.current) { | ||
| firstTimeRef.current = false; | ||
| return; | ||
| } | ||
|
|
||
| refetch(); | ||
| }, [refetch]) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add cleanup logic for pending promises.
The hook should handle cleanup properly to prevent memory leaks and potential state updates after unmount.
Apply this diff to add proper cleanup:
export function useRefreshOnFocus<T>(refetch: () => Promise<T>) {
const firstTimeRef = React.useRef(true);
+ const abortControllerRef = React.useRef<AbortController>();
useFocusEffect(
React.useCallback(() => {
if (firstTimeRef.current) {
firstTimeRef.current = false;
return;
}
- refetch();
+ // Cancel any pending refetch
+ abortControllerRef.current?.abort();
+
+ // Create new abort controller for this refetch
+ abortControllerRef.current = new AbortController();
+
+ // Only proceed with refetch if not aborted
+ if (!abortControllerRef.current.signal.aborted) {
+ refetch();
+ }
+
+ return () => {
+ abortControllerRef.current?.abort();
+ };
}, [refetch])
);
}📝 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 useRefreshOnFocus<T>(refetch: () => Promise<T>) { | |
| const firstTimeRef = React.useRef(true); | |
| useFocusEffect( | |
| React.useCallback(() => { | |
| if (firstTimeRef.current) { | |
| firstTimeRef.current = false; | |
| return; | |
| } | |
| refetch(); | |
| }, [refetch]) | |
| ); | |
| } | |
| export function useRefreshOnFocus<T>(refetch: () => Promise<T>) { | |
| const firstTimeRef = React.useRef(true); | |
| const abortControllerRef = React.useRef<AbortController>(); | |
| useFocusEffect( | |
| React.useCallback(() => { | |
| if (firstTimeRef.current) { | |
| firstTimeRef.current = false; | |
| return; | |
| } | |
| // Cancel any pending refetch | |
| abortControllerRef.current?.abort(); | |
| // Create new abort controller for this refetch | |
| abortControllerRef.current = new AbortController(); | |
| // Only proceed with refetch if not aborted | |
| if (!abortControllerRef.current.signal.aborted) { | |
| refetch(); | |
| } | |
| return () => { | |
| abortControllerRef.current?.abort(); | |
| }; | |
| }, [refetch]) | |
| ); | |
| } |
| export const TextMessage = ({ | ||
| fromMe, | ||
| hideBackground, | ||
| content, | ||
| }: TextMessageProps) => { | ||
| const styles = useStyles(); | ||
| return ( | ||
| <View style={styles.messageContentContainer}> | ||
| <ClickableText | ||
| style={[ | ||
| styles.messageText, | ||
| fromMe ? styles.messageTextMe : undefined, | ||
| hideBackground ? styles.allEmojisAndMaxThree : undefined, | ||
| ]} | ||
| > | ||
| {content} | ||
| </ClickableText> | ||
| </View> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider memoizing the component for better performance.
The component could benefit from memoization to prevent unnecessary re-renders, especially in a chat interface where multiple messages are rendered.
Apply this improvement:
-export const TextMessage = ({
+export const TextMessage = memo(({
fromMe,
hideBackground,
content,
-}: TextMessageProps) => {
+}: TextMessageProps) => {
const styles = useStyles();
return (
<View style={styles.messageContentContainer}>
<ClickableText
style={[
styles.messageText,
fromMe ? styles.messageTextMe : undefined,
hideBackground ? styles.allEmojisAndMaxThree : undefined,
]}
>
{content}
</ClickableText>
</View>
);
-};
+});Don't forget to add the import:
import { memo } from 'react';| const useStyles = () => { | ||
| const colorScheme = useColorScheme(); | ||
| return useMemo( | ||
| () => | ||
| StyleSheet.create({ | ||
| messageContentContainer: { | ||
| paddingHorizontal: 13, | ||
| paddingVertical: 6, | ||
| }, | ||
| messageText: { | ||
| color: textPrimaryColor(colorScheme), | ||
| fontSize: 17, | ||
| }, | ||
| messageTextMe: { | ||
| color: inversePrimaryColor(colorScheme), | ||
| }, | ||
| allEmojisAndMaxThree: { | ||
| fontSize: 64, | ||
| paddingHorizontal: 0, | ||
| }, | ||
| }), | ||
| [colorScheme] | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Improve color scheme handling.
The current implementation doesn't handle cases where colorScheme is null, which could happen during initial render or theme changes.
Consider this improvement:
const useStyles = () => {
- const colorScheme = useColorScheme();
+ const colorScheme = useColorScheme() ?? 'light'; // Provide a default
return useMemo(
() =>
StyleSheet.create({
messageContentContainer: {
paddingHorizontal: 13,
paddingVertical: 6,
},
messageText: {
- color: textPrimaryColor(colorScheme),
+ color: textPrimaryColor(colorScheme as 'light' | 'dark'),
fontSize: 17,
},
messageTextMe: {
- color: inversePrimaryColor(colorScheme),
+ color: inversePrimaryColor(colorScheme as 'light' | 'dark'),
paddingHorizontal: 0,
},
// ... rest of the styles
}),
[colorScheme]
);
};Committable suggestion was skipped due to low confidence.
| export const V3MessageSender = ({ inboxId }: V3MessageSenderProps) => { | ||
| const address = useInboxIdStore((s) => s.byInboxId[inboxId]?.[0]); | ||
| const senderSocials = useProfilesStore( | ||
| (s) => getProfile(address, s.profiles)?.socials | ||
| ); | ||
| const name = getPreferredName(senderSocials, address); | ||
| return <MessageSenderDumb name={name} />; | ||
| }; |
There was a problem hiding this comment.
Add error handling for missing inbox data.
The component should handle cases where the inbox lookup returns undefined.
export const V3MessageSender = ({ inboxId }: V3MessageSenderProps) => {
const address = useInboxIdStore((s) => s.byInboxId[inboxId]?.[0]);
+ if (!address) {
+ return null; // or a fallback UI
+ }
const senderSocials = useProfilesStore(
(s) => getProfile(address, s.profiles)?.socials
);
const name = getPreferredName(senderSocials, address);
+ if (!name) {
+ return null; // or display address as fallback
+ }
return <MessageSenderDumb name={name} />;
};📝 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 V3MessageSender = ({ inboxId }: V3MessageSenderProps) => { | |
| const address = useInboxIdStore((s) => s.byInboxId[inboxId]?.[0]); | |
| const senderSocials = useProfilesStore( | |
| (s) => getProfile(address, s.profiles)?.socials | |
| ); | |
| const name = getPreferredName(senderSocials, address); | |
| return <MessageSenderDumb name={name} />; | |
| }; | |
| export const V3MessageSender = ({ inboxId }: V3MessageSenderProps) => { | |
| const address = useInboxIdStore((s) => s.byInboxId[inboxId]?.[0]); | |
| if (!address) { | |
| return null; // or a fallback UI | |
| } | |
| const senderSocials = useProfilesStore( | |
| (s) => getProfile(address, s.profiles)?.socials | |
| ); | |
| const name = getPreferredName(senderSocials, address); | |
| if (!name) { | |
| return null; // or display address as fallback | |
| } | |
| return <MessageSenderDumb name={name} />; | |
| }; |
| const useStyles = () => { | ||
| const colorScheme = useColorScheme(); | ||
| return useMemo( | ||
| () => | ||
| StyleSheet.create({ | ||
| groupSenderWrapper: { | ||
| flexDirection: "row", | ||
| flexBasis: "100%", | ||
| }, | ||
| groupSender: { | ||
| fontSize: 12, | ||
| color: textSecondaryColor(colorScheme), | ||
| marginLeft: 10, | ||
| marginVertical: 4, | ||
| }, | ||
| }), | ||
| [colorScheme] | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using responsive typography for better scaling.
While the styles are well-organized, consider using responsive typography to ensure text scales appropriately across different device sizes.
+import { ResponsiveValue, useResponsiveValue } from '@utils/responsive';
const useStyles = () => {
const colorScheme = useColorScheme();
+ const baseFontSize = useResponsiveValue({
+ default: 12,
+ tablet: 14,
+ desktop: 16,
+ });
return useMemo(
() =>
StyleSheet.create({
groupSenderWrapper: {
flexDirection: "row",
flexBasis: "100%",
},
groupSender: {
- fontSize: 12,
+ fontSize: baseFontSize,
color: textSecondaryColor(colorScheme),
marginLeft: 10,
marginVertical: 4,
},
}),
- [colorScheme]
+ [colorScheme, baseFontSize]
);
};📝 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 useStyles = () => { | |
| const colorScheme = useColorScheme(); | |
| return useMemo( | |
| () => | |
| StyleSheet.create({ | |
| groupSenderWrapper: { | |
| flexDirection: "row", | |
| flexBasis: "100%", | |
| }, | |
| groupSender: { | |
| fontSize: 12, | |
| color: textSecondaryColor(colorScheme), | |
| marginLeft: 10, | |
| marginVertical: 4, | |
| }, | |
| }), | |
| [colorScheme] | |
| ); | |
| }; | |
| import { ResponsiveValue, useResponsiveValue } from '@utils/responsive'; | |
| const useStyles = () => { | |
| const colorScheme = useColorScheme(); | |
| const baseFontSize = useResponsiveValue({ | |
| default: 12, | |
| tablet: 14, | |
| desktop: 16, | |
| }); | |
| return useMemo( | |
| () => | |
| StyleSheet.create({ | |
| groupSenderWrapper: { | |
| flexDirection: "row", | |
| flexBasis: "100%", | |
| }, | |
| groupSender: { | |
| fontSize: baseFontSize, | |
| color: textSecondaryColor(colorScheme), | |
| marginLeft: 10, | |
| marginVertical: 4, | |
| }, | |
| }), | |
| [colorScheme, baseFontSize] | |
| ); | |
| }; |
| const onLongPress = useCallback(() => { | ||
| runOnJS(triggerHapticFeedback)(); | ||
| runOnJS(showContextMenu)(); | ||
| }, [triggerHapticFeedback, showContextMenu]); |
There was a problem hiding this comment.
Remove unnecessary runOnJS calls in onLongPress handler
The runOnJS function is intended for use within Reanimated worklets to call JavaScript functions on the JavaScript thread. In this context, onLongPress is a regular React callback, so wrapping triggerHapticFeedback and showContextMenu with runOnJS is unnecessary.
Apply this diff to fix the issue:
const onLongPress = useCallback(() => {
- runOnJS(triggerHapticFeedback)();
- runOnJS(showContextMenu)();
+ triggerHapticFeedback();
+ showContextMenu();
}, [triggerHapticFeedback, showContextMenu]);📝 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 onLongPress = useCallback(() => { | |
| runOnJS(triggerHapticFeedback)(); | |
| runOnJS(showContextMenu)(); | |
| }, [triggerHapticFeedback, showContextMenu]); | |
| const onLongPress = useCallback(() => { | |
| triggerHapticFeedback(); | |
| showContextMenu(); | |
| }, [triggerHapticFeedback, showContextMenu]); |
| if (member.addresses[0].toLowerCase() !== currentAccount?.toLowerCase()) { | ||
| addresses.push(member.addresses[0]); | ||
| } |
There was a problem hiding this comment.
Handle potential undefined member.addresses[0] to prevent runtime errors
The code assumes that member.addresses[0] exists, but if member.addresses is undefined or an empty array, accessing member.addresses[0] will cause a runtime error.
Apply this diff to add a safety check:
- if (member.addresses[0].toLowerCase() !== currentAccount?.toLowerCase()) {
+ if (member.addresses[0] && member.addresses[0].toLowerCase() !== currentAccount?.toLowerCase()) {
addresses.push(member.addresses[0]);
}📝 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 (member.addresses[0].toLowerCase() !== currentAccount?.toLowerCase()) { | |
| addresses.push(member.addresses[0]); | |
| } | |
| if (member.addresses[0] && member.addresses[0].toLowerCase() !== currentAccount?.toLowerCase()) { | |
| addresses.push(member.addresses[0]); | |
| } |
| subtitle={`${timeToShow} ⋅ Message exam`} | ||
| contextMenuComponent={contextMenuComponent} | ||
| isUnread={isUnread} | ||
| rightIsDestructive={isBlockedChatView} |
There was a problem hiding this comment.
Add missing isBlockedChatView prop to ConversationListItemDumb
The ConversationListItemDumb component requires the isBlockedChatView prop, but it's not being provided. Instead, rightIsDestructive is set to isBlockedChatView, which may not have the intended effect.
Apply this diff to pass the required prop:
isUnread={isUnread}
- rightIsDestructive={isBlockedChatView}
+ isBlockedChatView={isBlockedChatView}Ensure that rightIsDestructive is set appropriately if needed by the component.
Committable suggestion was skipped due to low confidence.
| import { SendButton } from "./SendButton"; | ||
| import { MediaPreview } from "../../../data/store/chatStore"; | ||
| import { useConversationContext } from "../../../utils/conversation"; | ||
| import { isDesktop } from "../../../utils/device"; |
There was a problem hiding this comment.
Fix import path or provide module for '../../../utils/device'
The module '../../../utils/device' cannot be found. Ensure that the module exists and that the import path is correct to resolve the TypeScript error.
🧰 Tools
🪛 GitHub Check: tsc
[failure] 36-36:
Cannot find module '../../../utils/device' or its corresponding type declarations.
| await sendMessage(preparedAttachmentMessageRef.current); | ||
| preparedAttachmentMessageRef.current = null; | ||
| } |
There was a problem hiding this comment.
Add error handling for sendMessage function calls
The sendMessage calls lack error handling. If an error occurs during message sending, it may go unnoticed, leading to potential issues. Consider using try...catch blocks to handle possible errors.
Apply this diff to include error handling:
335 if (preparedAttachmentMessageRef.current) {
+ try {
336 await sendMessage(preparedAttachmentMessageRef.current);
337 preparedAttachmentMessageRef.current = null;
+ } catch (error) {
+ console.error('Error sending attachment message:', error);
+ // Optionally, notify the user about the error
+ }
338 }And for sending text messages:
+ try {
+ await sendMessage(messageToSend);
+ } catch (error) {
+ console.error('Error sending message:', error);
+ // Optionally, notify the user about the error
+ }
- sendMessage(messageToSend);Also applies to: 354-354
What this PR does not do
This is not expected to be perfectly coded and 100% exact functionality
It's not expected to use the new design system or have finalized approaches for V3 specific workings
What this PR does do
Introduces a new way to work with only V3 Conversations
Introduces dumb components to allow V2 & V3 logic to live together with 90+% same functionality as what the app offers today and provide easier areas for devs to make changes to more distinct layers of code
I am anticipating that this will go into a 2.0.9 release, but maybe I can get it done earlier
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Chores