feat: decouple dm, group and new conversation #1331
Conversation
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 54 files out of 156 files are above the max files limit of 75. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the WalkthroughThis pull request encompasses significant changes to the chat and conversation components within the application. Key modifications include the complete removal of several components related to message handling, such as Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
thierryskoda
force-pushed
the
ts/decouple-dm-group-conversations
branch
from
48b60d2 to
6fad49c
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
features/conversation/dm-conversation/existing-dm-conversation-content.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (12)
components/Chat/Message/message-gestures.tsx (1)
77-96: 🛠️ Refactor suggestion
Add error handling for gesture measurement failures
The
measurecall could fail silently if the ref is not properly attached. Consider adding proper error handling and logging.const longPress = Gesture.LongPress() .onStart((e) => { Haptics.softImpactAsyncAnimated(); scaleAV.value = withTiming(MESSAGE_GESTURE_LONG_PRESS_SCALE, { duration: MESSAGE_GESTURE_LONG_PRESS_MIN_DURATION * 2, easing: Easing.bezier(0.31, 0.04, 0.03, 1.04), }); const measured = measure(containerRef); - if (!measured) return; + if (!measured) { + console.warn('Failed to measure message container'); + return; + } if (onLongPress) { runOnJS(onLongPress)({ ...e, ...measured }); } })📝 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 longPress = Gesture.LongPress() .onStart((e) => { Haptics.softImpactAsyncAnimated(); scaleAV.value = withTiming(MESSAGE_GESTURE_LONG_PRESS_SCALE, { duration: MESSAGE_GESTURE_LONG_PRESS_MIN_DURATION * 2, easing: Easing.bezier(0.31, 0.04, 0.03, 1.04), }); const measured = measure(containerRef); if (!measured) { console.warn('Failed to measure message container'); return; } if (onLongPress) { runOnJS(onLongPress)({ ...e, ...measured }); } }) .onFinalize(() => { scaleAV.value = withTiming(1, { duration: MESSAGE_GESTURE_LONG_PRESS_MIN_DURATION * 2, easing: Easing.bezier(0.82, 0.06, 0.42, 1.01), }); }) .minDuration(MESSAGE_GESTURE_LONG_PRESS_MIN_DURATION);components/Chat/Message/message-context-menu/message-context-menu-reactors.tsx (1)
81-84:
⚠️ Potential issuePotential Key Collision in
FlatListThe
keyExtractorusesitem[0](the reaction content) as the key. If multiple users react with the same content, this could cause key collisions.Consider modifying the
keyExtractorto ensure uniqueness:keyExtractor={(item) => `${item[0]}-${index}`}Or use a combination of content and addresses:
keyExtractor={(item) => `${item[0]}-${item[1].join('-')}`}Committable suggestion skipped: line range outside the PR's diff.
components/Chat/Message/message-context-menu/message-context-menu.tsx (1)
90-94:
⚠️ Potential issueSafe Access of
bySenderfor ReactionsThe expression
Boolean(bySender && Object.keys(bySender).length > 0)could result in an error ifbySenderis undefined.Modify the condition to safely handle undefined
bySender:const hasReactions = Boolean(Object.keys(bySender ?? {}).length > 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.const handleChooseMoreEmojis = useCallback(() => { openMessageContextMenuEmojiPicker(); }, []); const hasReactions = Boolean(Object.keys(bySender ?? {}).length > 0);components/Conversation/V3Conversation.tsx (1)
97-98:
⚠️ Potential issueFix usage of undefined variables
isNewConversationandconversationVersionThe variables
isNewConversationandconversationVersionare commented out but are still used in the render logic. This will result inReferenceErrorat runtime.Apply this diff to reintroduce the variables:
- // const isNewConversation = useConversationContext("isNewConversation"); + const isNewConversation = useConversationContext("isNewConversation"); - // const conversationVersion = useConversationContext("conversationVersion"); + const conversationVersion = useConversationContext("conversationVersion");Alternatively, if these variables are no longer needed, update the rendering logic to remove their usage accordingly.
📝 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 isNewConversation = useConversationContext("isNewConversation"); const conversationVersion = useConversationContext("conversationVersion");features/conversation/group-conversation.screen.tsx (1)
244-245: 🛠️ Refactor suggestion
Avoid using
@ts-ignoreto suppress TypeScript errorsUsing
@ts-ignoresuppresses TypeScript's type checking and can mask underlying issues. It's recommended to address the root cause of the type error to maintain type safety.Consider resolving the TypeScript error by correctly typing the component or adjusting the types accordingly.
theme/layout.ts (1)
3-3: 🛠️ Refactor suggestion
Consider using useWindowDimensions for better reactivity
The current implementation using
Dimensions.get()won't update if the screen size changes (e.g., rotation, split-screen). Consider using theuseWindowDimensionshook instead.-import { Dimensions } from "react-native"; +import { useWindowDimensions } from "react-native"; +// Create a hook to get dynamic dimensions +export const useLayout = () => { + const { width, height } = useWindowDimensions(); + return { + ...layout, + screen: { width, height } + }; +}; export const layout = { onboardingScreenPadding: spacing.lg, chat: { messageSenderAvatar: { width: avatarSize.sm, height: avatarSize.sm, }, - }, - screen: { - width: Dimensions.get("window").width, - height: Dimensions.get("window").height, }, };Also applies to: 14-17
design-system/BlurView.tsx (1)
3-3: 💡 Codebase verification
FadeIn import appears unused - consider removing or documenting intended usage
The
FadeInanimation is imported but not used anywhere in the BlurView component or its consumers. While the component usesAnimated.createAnimatedComponent, there's no implementation of theFadeInanimation.
design-system/BlurView.tsx: Remove unusedFadeInimport or implement the intended animation functionality🔗 Analysis chain
Consider documenting animation usage patterns
The addition of
FadeInanimation capabilities is good, but it would be helpful to document recommended usage patterns in comments or component props, especially since this is a design system component.Let's check current usage patterns:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find components using BlurView with FadeIn animation rg "BlurView.*FadeIn|FadeIn.*BlurView" --type tsx # Check for any animation-related props being passed to BlurView ast-grep --pattern 'BlurView $$$entering={FadeIn}$$$'Length of output: 135
Script:
#!/bin/bash # Search for BlurView usage patterns with proper file extensions rg "BlurView.*FadeIn|FadeIn.*BlurView" -t ts -t tsx -t jsx # Find all files that import and use BlurView rg "import.*BlurView.*from" -t ts -t tsx -t jsx -A 5 # Look for animation-related props or documentation in BlurView.tsx rg "entering|animation|transition" design-system/BlurView.tsx # Check for any existing documentation or comments about animation rg -B 2 -A 2 "\/\*\*|\*\/|\/\/.*animation" design-system/BlurView.tsxLength of output: 401
Script:
#!/bin/bash # Search for BlurView usage with simpler patterns rg -l "BlurView" # Look for animation-related content in BlurView.tsx cat design-system/BlurView.tsx # Search for any comments or documentation in BlurView rg "\/\*|\/\/" design-system/BlurView.tsx # Find files importing FadeIn rg "FadeIn.*react-native-reanimated"Length of output: 1870
features/conversation/dm-conversation/dm-conversation.screen.tsx (1)
29-32:
⚠️ Potential issueAddress the TODO comment for error handling.
The current implementation returns null when
peerAddressis missing, which could lead to a poor user experience. Consider implementing proper error handling and user feedback.Would you like me to help implement an error state component that provides better user feedback?
components/Chat/Message/message-context-menu/message-context-menu-utils.tsx (1)
61-71: 🛠️ Refactor suggestion
Improve copy functionality error handling
The copy functionality could be improved by:
- Using a more descriptive error message from translations
- Adding success feedback
- Handling clipboard errors
action: () => { const messageStringContent = getMessageStringContent(message); - if (!!messageStringContent) { - Clipboard.setString(messageStringContent); - } else { - showSnackbar({ - message: `Couldn't copy message content`, - }); - } + try { + if (messageStringContent) { + Clipboard.setString(messageStringContent); + showSnackbar({ + type: "success", + message: translate("message_copied"), + }); + } else { + showSnackbar({ + type: "error", + message: translate("error_empty_message"), + }); + } + } catch (error) { + showSnackbar({ + type: "error", + message: translate("error_copying_message"), + }); + } resetMessageContextMenuData(); },Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 62-62: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
features/conversation/dm-conversation/existing-dm-conversation-content.tsx (1)
38-38:
⚠️ Potential issueRemove suspicious semicolon in JSX
There's an erroneous semicolon after the
<Messages />component which should be removed.- <Messages />; + <Messages />📝 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.<Messages />🧰 Tools
🪛 Biome (1.9.4)
[error] 38-39: There is a suspicious semicolon in the JSX element.
This is usually the result of a typo or some refactor gone wrong.
Remove the semicolon, or move it inside a JSX element.(lint/suspicious/noSuspiciousSemicolonInJsx)
features/conversation/conversation-service.ts (1)
207-209: 💡 Codebase verification
getCurrentConversationTopiccannot be safely removed - it's still being usedThe function is still actively used in two places:
- In
getCurrentConversationMessages()where it's called to get the topic- Imported in
components/Chat/Message/message-utils.tsxThe commented-out function needs to be uncommented or the code needs to be refactored to use
useConversationCurrentTopicinstead.🔗 Analysis chain
Verify if getCurrentConversationTopic can be safely removed.
The function is commented out but still referenced in
getCurrentConversationMessages. This could lead to runtime errors.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if getCurrentConversationTopic is still used in the codebase # Search for direct usage of the function rg "getCurrentConversationTopic\(" --type ts --type tsx # Search for any string containing the function name (might catch comments/strings) rg "getCurrentConversationTopic" --type ts --type tsxLength of output: 167
Script:
#!/bin/bash # Search for direct usage of the function without file type restrictions rg "getCurrentConversationTopic\(" # Search for any string containing the function name rg "getCurrentConversationTopic"Length of output: 522
Script:
#!/bin/bash # Get context around the usage in message-utils.tsx rg -B 5 -A 5 "getCurrentConversationTopic" components/Chat/Message/message-utils.tsx # Get context around the usage in conversation-service.ts rg -B 5 -A 5 "getCurrentConversationTopic" features/conversation/conversation-service.tsLength of output: 1116
screens/Navigation/Navigation.tsx (1)
81-83: 💡 Codebase verification
The removal of
topicfrom DmConversation params requires additional changesBased on the verification results, there are still components and files that depend on the
topicprop:
DmConversationTitlecomponent still expects atopicprop in its props type definition- Several components are rendering
DmConversationTitlewith atopicprop:
ExistingDmConversationContentGroupConversation.screenV3ConversationThese components need to be updated to align with the navigation parameter changes.
🔗 Analysis chain
Verify the impact of removing topic from DmConversation params.
The simplification of navigation params is good, but we should ensure all navigation calls have been updated to use only peerAddress.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for navigation calls to DmConversation # Search for navigation calls using topic rg "navigate\(['\"]DmConversation['\"].*topic" --type ts --type tsx # Search for navigation calls to verify they use peerAddress rg "navigate\(['\"]DmConversation['\"]" -A 2 --type ts --type tsxLength of output: 201
Script:
#!/bin/bash # Description: Check for navigation calls to DmConversation with corrected file type flags # Search for navigation calls using topic rg "navigate\(['\"]DmConversation['\"].*topic" -t typescript -t tsx # Search for navigation calls to verify they use peerAddress rg "navigate\(['\"]DmConversation['\"]" -A 2 -t typescript -t tsx # Also search for any remaining references to topic in relation to DmConversation rg "DmConversation.*topic|topic.*DmConversation" -t typescript -t tsxLength of output: 322
Script:
#!/bin/bash # Description: Check for navigation calls to DmConversation with correct ripgrep type flags # Search for navigation calls using topic rg "navigate\(['\"]DmConversation['\"].*topic" --type-add 'tsx:*.{tsx,ts}' -t tsx # Search for navigation calls to verify they use peerAddress rg "navigate\(['\"]DmConversation['\"]" -A 2 --type-add 'tsx:*.{tsx,ts}' -t tsx # Also search for any remaining references to topic in relation to DmConversation rg "DmConversation.*topic|topic.*DmConversation" --type-add 'tsx:*.{tsx,ts}' -t tsxLength of output: 1051
Script:
#!/bin/bash # Let's check the DmConversationTitle component and navigation params ast-grep --pattern 'type DmConversationTitleProps = { $$$ }' # Also check the actual navigation calls rg "navigation\.(navigate|push|replace)\s*\(\s*['\"]DmConversation['\"]" --type-add 'tsx:*.{tsx,ts}' -t tsx -B 2 -A 2Length of output: 436
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 |
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Outside diff range and nitpick comments (92)
features/conversation/use-mark-as-read-on-enter.tsx (1)
12-19: Consider handling cleanup and conversation changes.While the current implementation works well for marking messages as read, consider these improvements:
- Add cleanup to handle component unmounting
- Reset the state when conversation changes
Here's a suggested implementation:
-export const useMarkAsReadOnEnter = ({ +export const useMarkAsReadOnEnter = ({ messagesLoading, isUnread, toggleReadStatus, + conversationId, // Add this prop to track conversation changes }: { messagesLoading: boolean; isUnread: boolean; toggleReadStatus: () => void; + conversationId: string; }) => { const hasMarkedAsRead = useRef(false); useEffect(() => { + // Reset state when conversation changes + hasMarkedAsRead.current = false; + }, [conversationId]); + + useEffect(() => { if (isUnread && !messagesLoading && !hasMarkedAsRead.current) { toggleReadStatus(); hasMarkedAsRead.current = true; } + + // Cleanup if component unmounts while loading + return () => { + if (messagesLoading) { + hasMarkedAsRead.current = false; + } + }; }, [isUnread, messagesLoading, toggleReadStatus]); };features/conversation-list/useV3ConversationItems.ts (1)
Line range hint
32-36: LGTM! Consider memoizing the Sets.The filtering logic is clean and well-organized. However, since
pinnedTopicsanddeletedTopicsSets are recreated on every render, consider memoizing them separately if thetopicsDataobject is large.const conversationsFiltered = useMemo(() => { - const pinnedTopics = new Set(pinnedConversationTopics); - const deletedTopics = new Set( - Object.entries(topicsData) - .filter(([_, data]) => data?.status === "deleted") - .map(([topic]) => topic) - ); + const pinnedTopics = useMemo( + () => new Set(pinnedConversationTopics), + [pinnedConversationTopics] + ); + const deletedTopics = useMemo( + () => new Set( + Object.entries(topicsData) + .filter(([_, data]) => data?.status === "deleted") + .map(([topic]) => topic) + ), + [topicsData] + );features/conversation-list/hooks/useMessageText.ts (1)
Line range hint
35-38: Address TODO and improve group update message handling.The hardcoded message "conversation updated" and the TODO comment suggest incomplete implementation of group update notifications.
Would you like me to help create a more descriptive implementation for group update messages? This could include:
- Detailed update messages based on the type of group change
- Proper localization support
- Type-safe handling of different group update scenarios
features/conversation/conversation-message/conversation-message-content-container.tsx (1)
13-14: Consider adding error boundary for context usageWhile using the context store is a good approach, the component might break if used outside of the MessageContextStoreContext provider.
Consider adding a safety check:
+ const messageContext = useMessageContextStoreContext(useSelect(["fromMe"])); + if (!messageContext) { + console.warn('MessageContentContainer must be used within a MessageContextStoreContext provider'); + return null; + } - const { fromMe } = useMessageContextStoreContext(useSelect(["fromMe"])); + const { fromMe } = messageContext;features/conversation/conversation-message/conversation-message-content-types/conversation-message-static-attachment.tsx (1)
34-34: LGTM! Consider adding documentation about the layout change.The removal of the MessageLayout wrapper simplifies the component structure while maintaining the core functionality. The Content component continues to handle attachment loading, error states, and proper image rendering effectively.
Consider adding a comment explaining why MessageLayout was removed and how the layout responsibilities are now handled, to help future maintainers understand the architectural decision.
+// Note: MessageLayout wrapper was removed as part of decoupling conversation components. +// Layout responsibilities are now handled by the parent conversation container. return <Content messageId={message.id} staticAttachment={content} />;features/conversation/conversation-message/conversation-message-reactions/conversation-message-reactions.tsx (3)
24-30: Consider adding prop types for better maintainability.While the component implementation is clean, adding TypeScript interface/type definitions for the component's props would improve maintainability and provide better documentation.
interface ConversationMessageReactionsProps { // Add any future props here } export const ConversationMessageReactions = memo<ConversationMessageReactionsProps>( function ConversationMessageReactions() { // ... existing implementation } );
40-78: Consider documenting the MAX_REACTION_EMOJIS_SHOWN constant.The render logic is well-implemented with good accessibility support. However, the
MAX_REACTION_EMOJIS_SHOWNconstant would benefit from a JSDoc comment explaining why 3 was chosen as the limit./** * Maximum number of emoji reactions to display in the preview. * Limited to 3 to maintain a clean UI while providing sufficient context. */ const MAX_REACTION_EMOJIS_SHOWN = 3;
Line range hint
82-183: Consider breaking down the useMessageReactionsRolledUp hook for better maintainability.While the hook implementation is functionally correct, its complexity suggests it could benefit from being broken down into smaller, more focused functions. This would improve readability and testability.
Consider extracting these operations into separate functions:
- Reaction flattening
- Social details mapping
- Preview counting
- Detailed array processing
Example refactor:
function flattenReactions(reactions: Record<string, any[]>) { return Object.entries(reactions ?? {}).flatMap( ([senderAddress, senderReactions]) => senderReactions.map((reaction) => ({ senderAddress, ...reaction })) ); } function createSocialsMap(membersSocials: any[]) { return new Map( membersSocials.map((social) => [social.address, social]) ); } // ... rest of the refactored functions function useMessageReactionsRolledUp() { // ... existing setup code return useMemo((): RolledUpReactions => { const flatReactions = flattenReactions(reactions); const socialsMap = createSocialsMap(membersSocials); // ... use other extracted functions return { totalCount, userReacted, preview, detailed, }; }, [reactions, userAddress, membersSocials]); }features/conversation/conversation-keyboard-filler.tsx (3)
6-8: Consider debouncing keyboard height changes.While the current implementation is solid, rapid keyboard height changes (e.g., during split-screen mode changes or external keyboard connections) might cause unnecessary re-renders.
Consider adding debounce logic:
+import { useDebouncedCallback } from 'use-debounce'; + export const KeyboardFiller = memo(function KeyboardFiller() { - const { height: keyboardHeightAV } = useAnimatedKeyboard(); + const { height: keyboardHeightAV } = useAnimatedKeyboard({ + onKeyboardDidChangeFrame: useDebouncedCallback( + (e) => { + // Additional handling if needed + }, + 100 + ), + }); const insets = useSafeAreaInsets();
14-14: Consider adding accessibility attributes.The AnimatedVStack might affect screen reader behavior when the keyboard appears/disappears.
Add accessibility attributes:
- return <AnimatedVStack style={as} />; + return <AnimatedVStack + style={as} + accessibilityRole="none" + accessible={false} + />;
1-15: Well-structured keyboard handling component.This component follows the single responsibility principle well by focusing solely on keyboard height adjustment. Its decoupled nature aligns with the PR's objective of separating conversation-related functionalities.
Consider documenting the following in the component or README:
- The component's role in the conversation feature
- Examples of where and how to use it
- Any limitations or considerations when using with other keyboard-aware components
features/conversation/conversation-message/conversation-message-content-types/conversation-message-chat-group-update.tsx (2)
40-44: Remove commented out debug code and document style change
- The commented out
debugBorderprop should be removed as it appears to be debugging code.- Consider documenting the width style change in a comment to explain why full width is necessary.
<VStack - // {...debugBorder()} style={{ width: "100%", // Ensures message content spans full width of container paddingVertical: theme.spacing.sm, }}
Line range hint
1-1: Consider consistent naming for related componentsFor future improvements, consider renaming the other components in this file to follow the same naming pattern:
ChatGroupMemberLeft→MessageGroupMemberLeftChatGroupMemberJoined→MessageGroupMemberJoinedChatGroupMetadataUpdate→MessageGroupMetadataUpdateChatGroupUpdateText→MessageGroupUpdateTextThis would maintain consistency with the new naming convention.
features/conversation/conversation-message/conversation-message-content-types/conversation-message-reply.tsx (6)
61-62: Consider extracting magic numbers into theme constantsThe spacing values like 'xxxs' are used directly in styles. Consider defining these in the theme system for better maintainability.
- marginTop: theme.spacing.xxxs, + marginTop: theme.spacing.message.replyBubbleSpacing,
86-87: Extract complex border radius calculation into a helperThe border radius calculation is complex and repeated throughout the component. Consider extracting it into a helper function or theme constant.
+ const getAdjustedBorderRadius = (baseRadius: number, padding: number) => + baseRadius - padding / 2; - borderRadius: - theme.borderRadius.message.attachment - - theme.spacing.message.replyMessage.horizontalPadding / 2, + borderRadius: getAdjustedBorderRadius( + theme.borderRadius.message.attachment, + theme.spacing.message.replyMessage.horizontalPadding + ),
Line range hint
42-49: Improve error handling for invalid message contentThe current error handling uses console.error and returns null. Consider implementing proper error boundaries or fallback UI.
if (typeof replyMessageContent === "string") { - // TODO. Render simple bubble message with the content? - console.error("reply message is a string"); - return null; + return ( + <BubbleContainer fromMe={fromMe}> + <BubbleContentContainer fromMe={fromMe} hasNextMessageInSeries={hasNextMessageInSeries}> + <MessageText inverted={fromMe}>{replyMessageContent}</MessageText> + </BubbleContentContainer> + </BubbleContainer> + ); }
Line range hint
102-185: Add error boundary for MessageReplyReferenceThe component should gracefully handle cases where the reference message fails to load.
+ const MessageReplyReferenceErrorBoundary = ({ children }: { children: React.ReactNode }) => { + const { theme } = useAppTheme(); + return ( + <ErrorBoundary + fallback={ + <Text preset="smaller" color="error"> + Unable to load referenced message + </Text> + } + > + {children} + </ErrorBoundary> + ); + }; const MessageReplyReference = memo(function MessageReplyReference(props: { referenceMessageId: MessageId; }) { // ... existing code ... return ( + <MessageReplyReferenceErrorBoundary> <VStack> {/* ... existing JSX ... */} </VStack> + </MessageReplyReferenceErrorBoundary> ); });
Line range hint
205-211: Improve error tracking for unhandled content typesThe current error tracking could provide more context about the unhandled content type.
- sentryTrackError( - `Reply message reference message content is not handled with default text ${replyMessageSafeText}` - ); + sentryTrackError( + new Error('Unhandled reply message content type'), { + extra: { + messageId: replyMessage.id, + contentType: typeof content, + fallbackText: replyMessageSafeText, + } + } + );
Line range hint
231-251: Optimize message fetching hook with memoizationThe hook could benefit from memoizing the query options and cached message lookup.
function useConversationMessageForReplyMessage( messageId: MessageId ): DecodedMessage<ReplyCodec> | undefined { const currentAccount = useCurrentAccount()!; const topic = useCurrentConversationTopic(); - const messages = getConversationMessages(currentAccount, topic); + const messages = useMemo( + () => getConversationMessages(currentAccount, topic), + [currentAccount, topic] + ); - const cachedReplyMessage = messages?.byId[messageId] as + const cachedReplyMessage = useMemo( + () => messages?.byId[messageId] as | DecodedMessage<ReplyCodec> - | undefined; + | undefined, + [messages, messageId] + ); + const queryOptions = useMemo( + () => getConversationMessageQueryOptions({ + account: currentAccount, + messageId, + }), + [currentAccount, messageId] + ); const { data: replyMessage } = useQuery({ - ...getConversationMessageQueryOptions({ - account: currentAccount, - messageId, - }), + ...queryOptions, enabled: !cachedReplyMessage, });features/conversation/conversation-message-timestamp.tsx (2)
Line range hint
21-73: Consider optimizing the animation configuration.The component handles animations well, but there's room for improvement in terms of performance and maintainability:
- Consider extracting animation constants to a configuration object
- The interpolation values could be memoized to prevent recalculations
Here's a suggested optimization:
+ const ANIMATION_CONFIG = { + scale: { + input: [0, 1], + output: [0, 1], + }, + opacity: { + input: [0, 1], + output: [0, 1], + }, + height: (textHeight: number) => ({ + input: [0, 1], + output: [0, textHeight || 14], + }), + } as const; const timeAnimatedStyle = useAnimatedStyle(() => { return { - height: interpolate( - showTimeProgressAV.value, - [0, 1], - [0, textHeight || 14] - ), + height: interpolate( + showTimeProgressAV.value, + ANIMATION_CONFIG.height(textHeight).input, + ANIMATION_CONFIG.height(textHeight).output + ), opacity: interpolate(showTimeProgressAV.value, [0, 1], [0, 1]), // ... rest of the style }; }, [textHeight]);
Line range hint
25-35: Consider using a custom hook for store subscription.The store subscription logic could be extracted into a custom hook to improve reusability and testing.
Here's a suggested refactor:
const useMessageTimeVisibility = (showTimeAV: SharedValue<number>) => { const messageStore = useMessageContextStore(); useEffect(() => { const unsubscribe = messageStore.subscribe( (state) => state.isShowingTime, (isShowingTime) => { showTimeAV.value = isShowingTime ? 1 : 0; } ); return () => unsubscribe(); }, [messageStore, showTimeAV]); };Then use it in the component:
const MessageTimestamp = memo(function MessageTimestamp() { const showTimeAV = useSharedValue(0); useMessageTimeVisibility(showTimeAV); // ... rest of the component });features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.store.tsx (3)
43-54: Consider enhancing type safety and documentation.While the implementation is functional, consider these improvements:
- Add JSDoc comments to document the store's purpose and usage
- Make the setter function more explicit about when null is acceptable
+/** + * Creates a Zustand store for managing message context menu state + * @param initProps Initial store properties + * @returns A configured Zustand store instance + */ const createMessageContextMenuStore = ( initProps: IMessageContextMenuStoreProps ) => { return createStore<IMessageContextMenuStoreState>()( subscribeWithSelector((set) => ({ messageContextMenuData: null, - setMessageContextMenuData: (data) => - set({ messageContextMenuData: data }), + setMessageContextMenuData: (data: IMessageContextMenuStoreState["messageContextMenuData"]) => { + set({ messageContextMenuData: data }); + }, ...initProps, })) ); };
56-73: Consider consolidating error messages.The implementation follows React hooks best practices, but there's a minor opportunity for improvement:
+const MISSING_PROVIDER_ERROR = "Missing MessageContextMenuStore.Provider in the tree"; const MessageContextMenuStoreContext = createContext<IMessageContextMenuStore | null>(null); export function useMessageContextMenuStoreContext<T>( selector: (state: IMessageContextMenuStoreState) => T ): T { const store = useContext(MessageContextMenuStoreContext); if (!store) - throw new Error("Missing MessageContextMenuStore.Provider in the tree"); + throw new Error(MISSING_PROVIDER_ERROR); return useStore(store, selector); } export function useMessageContextMenuStore() { const store = useContext(MessageContextMenuStoreContext); if (!store) - throw new Error(`Missing MessageContextMenuStore.Provider in the tree`); + throw new Error(MISSING_PROVIDER_ERROR); return store; }
8-20: Consider adding cleanup functionality.The store manages message context menu state but lacks cleanup functionality. Consider adding a reset method to clear the state when the context menu is dismissed or the component unmounts.
export type IMessageContextMenuStoreState = IMessageContextMenuStoreProps & { messageContextMenuData: { messageId: MessageId; itemRectX: number; itemRectY: number; itemRectHeight: number; itemRectWidth: number; messageComponent: React.ReactNode; } | null; setMessageContextMenuData: ( data: IMessageContextMenuStoreState["messageContextMenuData"] ) => void; + reset: () => void; }; const createMessageContextMenuStore = ( initProps: IMessageContextMenuStoreProps ) => { return createStore<IMessageContextMenuStoreState>()( subscribeWithSelector((set) => ({ messageContextMenuData: null, setMessageContextMenuData: (data) => set({ messageContextMenuData: data }), + reset: () => set({ messageContextMenuData: null }), ...initProps, })) ); };Also applies to: 46-53
features/conversation/conversation-title.tsx (1)
Line range hint
11-15: Update type name to match component nameThe type
ConversationTitleDumbPropsshould be renamed toConversationTitlePropsto maintain consistency with the new component name.-type ConversationTitleDumbProps = { +type ConversationTitleProps = { title?: string; avatarComponent?: React.ReactNode; onLongPress?: () => void; onPress?: () => void; };features/conversations/components/NewConversationTitle.tsx (1)
Line range hint
59-71: LGTM! Consider memoizing the avatar componentThe implementation is clean and follows best practices. However, consider memoizing the avatar component to prevent unnecessary re-renders.
+const MemoizedAvatar = memo(({ uri, name, style }: { uri?: string, name?: string, style: any }) => ( + <Avatar + uri={uri} + size={AvatarSizes.conversationTitle} + style={style} + name={name} + /> +)); return ( <ConversationTitle title={preferredName} onPress={onPress} avatarComponent={ displayAvatar && ( - <Avatar - uri={preferredAvatarUri ?? undefined} - size={AvatarSizes.conversationTitle} - style={themed($avatar)} - /> + <MemoizedAvatar + uri={preferredAvatarUri ?? undefined} + style={themed($avatar)} + /> ) } /> );features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-emoji-picker/conversation-message-context-menu-emoji-picker-list.tsx (2)
Line range hint
46-72: Consider optimizing list performanceThe list rendering implementation could benefit from additional optimizations:
- Consider memoizing the
EmojiRowcomponent if it's not already memoized- The animation timing could be extracted to a constant to maintain consistency
Here's a suggested optimization:
const renderItem: ListRenderItem<CategorizedEmojisRecord> & FlashListRenderItem<CategorizedEmojisRecord> = useCallback( ({ item }) => <EmojiRow onPress={onPress} item={item} />, [onPress] ); +const ANIMATION_DURATION = 400; + useEffect(() => { height.value = withTiming( Math.min(emojis.length * 50, windowHeight * 0.75), { - duration: 400, + duration: ANIMATION_DURATION, } ); }, [emojis.length, height, windowHeight]);
Line range hint
31-33: Consider adding a comment explaining the platform-specific rendererThe platform-specific list renderer choice would benefit from a brief explanation of the Android scroll issue it addresses.
-// Works around issue with Android not picking up scrolls +// BottomSheetFlashList has scroll detection issues on Android, so we fall back to BottomSheetFlatList +// See: [Insert issue/PR reference if available] const ListRenderer = Platform.OS === "ios" ? BottomSheetFlashList : BottomSheetFlatList;features/conversation/conversation-message/conversation-message-layout.tsx (3)
11-13: Add JSDoc comments for better documentation.Consider adding JSDoc comments to document the purpose of the component props interface.
+/** + * Props for the ConversationMessageLayout component + */ type IConversationMessageLayoutProps = { + /** The message content to be rendered within the layout */ children: ReactNode; };
35-38: Consider extracting spacing constants.The spacing values ("xxs", "4xs") could be extracted into named constants for better maintainability and type safety.
+const AVATAR_SPACING = "xxs" as const; +const MESSAGE_SPACING = "4xs" as const; + <VStack style={{ - rowGap: theme.spacing["4xs"], + rowGap: theme.spacing[MESSAGE_SPACING], alignItems: fromMe ? "flex-end" : "flex-start", }} />
26-43: Enhance accessibility with ARIA labels.Consider adding appropriate ARIA labels to improve screen reader support.
-<MessageContainer> +<MessageContainer aria-label={fromMe ? "Your message" : "Message from " + senderAddress}> <MessageContentContainer> {!fromMe && ( <> - <V3MessageSenderAvatar inboxId={senderAddress} /> + <V3MessageSenderAvatar inboxId={senderAddress} aria-label="Sender's avatar" /> <VStack style={{ width: theme.spacing.xxs }} /> </> )} <VStack + role="presentation" style={{ rowGap: theme.spacing["4xs"], alignItems: fromMe ? "flex-end" : "flex-start", }} > {children} </VStack> </MessageContentContainer> </MessageContainer>components/Recommendations/Recommendations.tsx (3)
82-85: Consider adding error handling for navigation.The navigation code uses a timeout which could potentially fail silently. Consider adding error handling and possibly a loading state.
contactPol = useCallback(() => { + const navigateWithRetry = (retries = 3) => { navigation.popToTop(); - setTimeout(() => { + const timeoutId = setTimeout(() => { navigation.navigate("Conversation", { peer: config.contactAddress, - }); + }).catch(error => { + if (retries > 0) { + console.warn('Navigation failed, retrying...', error); + navigateWithRetry(retries - 1); + } else { + console.error('Navigation failed after retries', error); + } + }); + return () => clearTimeout(timeoutId); }, 300); + }; + navigateWithRetry(); }, [navigation]);
Line range hint
14-24: Consider optimizing imports.The component imports several React Native components that could be destructured in a single import statement to improve code organization.
-import { - FlatList, - Keyboard, - Platform, - StyleSheet, - Text, - View, - useColorScheme, -} from "react-native"; +import type { ViewToken } from 'react-native'; +import { + FlatList, + Keyboard, + Platform, + StyleSheet, + Text, + View, + useColorScheme, +} from 'react-native';
Line range hint
93-103: Add type safety to viewableItems callback.The
onViewableItemsChangedcallback usesanytype which could be made type-safe.const onViewableItemsChanged = useCallback( - ({ viewableItems: items }: any) => { + ({ viewableItems: items }: { viewableItems: Array<ViewToken> }) => { const viewable: { [key: string]: true } = {}; - items.forEach((item: any) => { - viewable[item.item] = true; + items.forEach((viewToken) => { + if (viewToken.item && typeof viewToken.item === 'string') { + viewable[viewToken.item] = true; + } }); setViewableItems(viewable); }, [] );components/ConversationList/ChatNullState.tsx (1)
Line range hint
63-79: Remove commented out AnimatedBanner code.Consider removing the commented out
AnimatedBannersection to maintain code cleanliness. If this feature might be needed later, it should be tracked in a separate issue.features/conversation-list/hooks/useMessageIsUnread.ts (1)
Line range hint
25-41: Consider adding input validation for timestampNsWhile the implementation is sound, consider adding validation for the
timestampNsparameter to handle edge cases like negative values or invalid timestamps.return useMemo(() => { // No message means no unread status if (!lastMessage) return false; + + // Validate timestamp + if (timestampNs < 0) { + console.warn('Invalid negative timestamp provided to useConversationIsUnread'); + return false; + } // Check if status is unread if (topicsData[topic]?.status === "unread") return true;features/conversation/conversation-message/conversation-message-content-types/conversation-message-simple-text.tsx (1)
11-13: Consider enhancing type safety and memoizationThe component could benefit from the following improvements:
- Extract the props interface for reusability
- Add a custom comparison function to
memofor optimal performance+interface MessageSimpleTextProps { + message: DecodedMessage<TextCodec>; +} + +const arePropsEqual = ( + prevProps: MessageSimpleTextProps, + nextProps: MessageSimpleTextProps +) => prevProps.message.id === nextProps.message.id; + -export const MessageSimpleText = memo(function MessageSimpleText(props: { - message: DecodedMessage<TextCodec>; -}) { +export const MessageSimpleText = memo(function MessageSimpleText( + props: MessageSimpleTextProps +) {features/conversation/conversation-message-sender-avatar.tsx (4)
10-12: Consider using absolute imports for better maintainabilityThe recent changes introduced relative imports (
../../) which can make the codebase harder to maintain. Consider using absolute imports (e.g.,@store/accountsStore,@utils/navigation,@utils/profile) to align with the centralized directory structure pattern used in other imports.-import { useCurrentAccount } from "../../data/store/accountsStore"; -import { navigate } from "../../utils/navigation"; -import { getPreferredInboxAvatar } from "../../utils/profile"; +import { useCurrentAccount } from "@store/accountsStore"; +import { navigate } from "@utils/navigation"; +import { getPreferredInboxAvatar } from "@utils/profile";
Line range hint
21-44: Clean up commented code or add TODO commentsThe component contains commented code related to message series handling. If this functionality is planned for future implementation, consider adding a TODO comment with the rationale. Otherwise, remove the commented code to improve readability.
- // hasNextMessageInSeries: boolean; + // TODO: Add message series handling when implementing conversation threading + // hasNextMessageInSeries?: boolean;
Line range hint
82-98: Add error handling for profile navigationThe
openProfilecallback assumesaddresswill be available when needed. Consider adding error handling or user feedback when the address is unavailable.const openProfile = useCallback(() => { if (address) { navigate("Profile", { address }); + } else { + // Consider showing a toast or alert + console.warn('Profile address not available'); } }, [address]);
Line range hint
101-113: Clean up commented stylesThe
groupSenderAvatarWrapperstyle contains a commented margin property. Either remove it or document why it might be needed in the future.groupSenderAvatarWrapper: { - // marginRight: 6, },components/Chat/Transaction/TransactionPreview.tsx (2)
Line range hint
1-28: Cleanup unused importsSeveral imports appear to be unused in the active code:
- Clipboard
- Linking
- useCallback, useEffect
- SVG components (Clock, Checkmark, Exclamationmark)
Remove unused imports to maintain code cleanliness:
import { actionSheetColors, messageInnerBubbleColor, myMessageInnerBubbleColor, textPrimaryColor, textSecondaryColor, } from "@styles/colors"; import { ColorSchemeName, Platform, StyleSheet, Text, useColorScheme, View, } from "react-native"; import { MessageToDisplay } from "../../../features/conversation/Message";
Line range hint
30-53: TransactionView component lacks proper documentationThe TransactionView component appears to be a reusable UI component but lacks proper documentation about its purpose and usage.
Add JSDoc documentation to improve maintainability:
+/** + * Renders a transaction bubble with consistent styling + * @param fromMe - Boolean indicating if the transaction is from the current user + * @param children - Content to be rendered inside the transaction bubble + */ const TransactionView = ({ fromMe, children, }: { fromMe: boolean; children: React.ReactNode; }) => {components/PinnedConversations/PinnedV3DMConversation.tsx (1)
21-21: LGTM! Changes maintain consistent decoupling pattern.The changes in this file mirror those in the group conversation component, maintaining a clear separation between DM and group conversation handling. The consistent patterns across these components support the PR's objective of decoupling different conversation types.
The clear separation between DM and group conversation components, while sharing common utilities through the features directory, provides a good foundation for:
- Independent evolution of DM and group features
- Easier testing and maintenance
- Potential code splitting optimizations
Also applies to: 58-58
components/V3GroupConversationListItem.tsx (3)
Line range hint
89-146: Add error handling for async operationsThe delete operations involve multiple async calls (updateConsent, consentToInboxIdsOnProtocolByAccount) but lack error handling. Consider wrapping these in try-catch blocks to handle potential failures gracefully.
async () => { + try { saveTopicsData(currentAccount, { [topic]: { status: "deleted" }, }); setTopicsData({ [topic]: { status: "deleted", timestamp: new Date().getTime(), }, }); await group.updateConsent("denied"); await consentToInboxIdsOnProtocolByAccount({ account: currentAccount, inboxIds: [group.addedByInboxId], consent: "deny", }); setInboxIdPeerStatus({ [group.addedByInboxId]: "denied", }); + } catch (error) { + console.error('Failed to delete and block group:', error); + // TODO: Show error toast/notification to user + } }
Line range hint
147-189: Add error handling for restore operationsSimilar to delete operations, restore operations should include error handling for the async calls.
async () => { + try { await group.updateConsent("allowed"); await consentToInboxIdsOnProtocolByAccount({ account: currentAccount, inboxIds: [group.addedByInboxId], consent: "allow", }); setInboxIdPeerStatus({ [group.addedByInboxId]: "allowed", }); + } catch (error) { + console.error('Failed to restore and unblock group:', error); + // TODO: Show error toast/notification to user + } }
Line range hint
190-235: Consider memoizing action handlersThe action handlers for the context menu items are recreated on every render. Consider moving them to separate useCallback hooks to optimize performance.
+ const handlePin = useCallback(() => { + setPinnedConversations([topic]); + closeContextMenu(); + }, [setPinnedConversations, topic]); + + const handleMarkReadUnread = useCallback(() => { + toggleReadStatus(); + closeContextMenu(); + }, [toggleReadStatus]); const contextMenuItems: ContextMenuItem[] = useMemo( () => [ { title: translate("pin"), - action: () => { - setPinnedConversations([topic]); - closeContextMenu(); - }, + action: handlePin, id: "pin", rightView: <ContextMenuIcon icon="pin" />, }, { title: isUnread ? translate("mark_as_read") : translate("mark_as_unread"), - action: () => { - toggleReadStatus(); - closeContextMenu(); - }, + action: handleMarkReadUnread, id: "markAsUnread", rightView: ( <ContextMenuIcon icon={isUnread ? "checkmark.message" : "message.badge"} /> ), },data/store/chatStore.ts (4)
2-2: Document migration path for deprecated typesSeveral types are marked as @deprecated but lack documentation about:
- What replaces these types
- How to migrate existing code
- Timeline for removal
This could lead to maintenance issues and technical debt.
Consider adding migration guides in the comments:
/** * @deprecated - Use the conversation from query instead + * @migration - Migrate to NewConversationType from @xmtp/conversation-store + * @timeline - Planned for removal in v2.0.0 */Also applies to: 39-41, 73-75, 89-91
Line range hint
252-267: Clean up commented implementation in setInitialLoadDoneThe commented-out code block suggests incomplete refactoring. Either:
- Remove if no longer needed
- Implement if still required
- Document why it's temporarily disabled
This helps maintain code clarity and prevents confusion.
Line range hint
365-386: Enhance migration error handlingThe migration logic lacks error handling for edge cases:
- Corrupted state
- Missing or malformed data
- Type conversion failures
This could lead to runtime errors and data loss.
Consider wrapping migrations in try-catch:
migrate: (persistedState: any, version: number): ChatStoreType => { + try { logger.debug("Zustand migration version:", version); if (version < 1 && persistedState.deletedTopics) { // ... migration logic ... } return persistedState as ChatStoreType; + } catch (error) { + logger.error("Migration failed:", error); + // Return safe default state + return { + conversations: {}, + pinnedConversationTopics: [], + // ... other required fields + } as ChatStoreType; + } }
Line range hint
1-386: Document architectural transition strategyThe codebase shows signs of transitioning from local state management to a query-based approach, but lacks:
- Documentation of the new architecture
- Migration timeline
- Impact on dependent components
- Performance implications
This could make it difficult for team members to understand and contribute to the transition.
Consider adding an Architecture Decision Record (ADR) that covers:
- Motivation for the change
- New architecture overview
- Migration strategy
- Timeline and milestones
features/conversation/conversation-consent-popup/conversation-consent-popup-group.tsx (2)
Line range hint
28-45: Handle potential race conditions in consent operationsThe consent operations (
blockGroupandallowGroup) could have race conditions if triggered multiple times rapidly.Consider adding loading state and disabling buttons during operations:
export function GroupConsentPopup() { + const [isLoading, setIsLoading] = useState(false); // ... const onBlock = useCallback(async () => { + if (isLoading) return; + setIsLoading(true); groupRemoveRestoreHandler( "unknown", colorScheme, groupName, allowGroup, blockGroup )((success: boolean) => { if (success) { navigation.pop(); } + setIsLoading(false); }); - }, [groupName, colorScheme, allowGroup, blockGroup, navigation]); + }, [groupName, colorScheme, allowGroup, blockGroup, navigation, isLoading]);
Line range hint
52-71: Add loading state to buttonsThe buttons should reflect the loading state to prevent multiple submissions.
<ConsentPopupButton variant="text" action="danger" icon="xmark" text={translate("decline")} onPress={onBlock} + disabled={isLoading} /> <ConsentPopupButton variant="fill" icon="checkmark" text={translate("join_this_group")} onPress={onAccept} + disabled={isLoading} />features/conversation/conversation-message/conversation-message-content-types/conversation-message-remote-attachment.tsx (2)
25-25: Remove commented debug code.Remove the commented debug border as it's not needed in production code.
- // {...debugBorder("green")}
26-28: Consider using design tokens for max width.Instead of directly calculating width based on screen dimensions, consider using design tokens or predefined constants for consistent layout across the app.
- style={{ - maxWidth: theme.layout.screen.width * 0.7, - }} + style={{ + maxWidth: theme.layout.content.maxWidth, + }}features/conversation/conversation-attachment/conversation-attachment-remote-image.tsx (3)
Line range hint
77-83: Implement the openInWebview handler.The click handler for unsupported attachments is incomplete. This could lead to a confusing user experience where clicking the "View in browser" link does nothing.
Would you like me to help implement the openInWebview handler with proper security considerations?
Line range hint
115-117: Remove commented debug border code.Remove the commented debugBorder code as it's not needed in production.
- // ...debugBorder(),
Based on the code analysis, I can now generate the final response:
Remote attachment validation needs improvement despite having basic size limit.
While there is a maximum size limit defined (
MAX_AUTOMATIC_DOWNLOAD_ATTACHMENT_SIZE = 10000000; // 10MB), thefetchAttachmentfunction lacks:
- Content type validation before download
- Enforcement of the size limit during download
- Malicious content scanning
Specific issues:
- The
fetchAndDecodeRemoteAttachmentfunction downloads content without checking the content type- No validation of attachment size before initiating download
- No validation of file extensions or content types against an allowlist
- Direct file operations without sanitization
🔗 Analysis chain
Line range hint
134-146: Add content validation for remote attachments.The fetchAttachment function downloads remote content without validating the content type or implementing size limits for different content types. This could be a security risk if malicious content is downloaded.
Let's verify the content validation across the codebase:
Let me analyze the content validation patterns found in the codebase and generate additional verification scripts to check for size limits and content type validation specifically for remote attachments.
Let me check the configuration files found and look for any attachment-related validation in the codebase.
Let me check the remote attachment codec and message sending implementation for any validation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for content validation patterns in attachment handling rg -A 5 "contentType|validateContent|sanitize" # Check for size limits implementation ast-grep --pattern 'const $_ = $NUM * 1024 * 1024' # Looking for MB definitionsLength of output: 35212
Script:
#!/bin/bash # Search for remote attachment validation and size limits rg -A 10 "fetchAttachment|uploadFile|remoteAttachment" features/conversation/conversation-attachment/ # Search for any file size or content type validation in attachment handling rg -A 5 "maxSize|fileSize|validateAttachment|allowedTypes" # Look for attachment-related configuration fd -t f "config|constants" -x rg -l "attachment|size|limit"Length of output: 5567
Script:
#!/bin/bash # Check configuration files for attachment limits cat app.config.ts design-system/ContextMenu/ContextMenu.constants.ts # Look for attachment validation in utils rg -A 10 "attachment" utils/attachment/ rg -A 10 "attachment" utils/xmtpRN/ # Check for any validation in the remote attachment codec fd -t f -e ts -x rg -l "RemoteAttachmentCodec|RemoteAttachment"Length of output: 18554
Script:
#!/bin/bash # Check remote attachment implementation cat utils/xmtpRN/attachments.ts # Check message sending with attachments cat features/conversations/hooks/use-send-message.ts # Look for any size or type validation in the XMTP client setup cat utils/xmtpRN/client.tsLength of output: 8555
features/conversation/conversation-new-dm.tsx (8)
35-39: Consider simplifying theuseEffectdependency arrayIncluding
navigationin the dependency array may cause unnecessary re-executions of theuseEffect, asnavigationis generally stable. You can omit it to prevent unintended side effects.Apply this diff to adjust the dependency array:
useEffect(() => { navigation.setOptions({ headerTitle: () => <NewConversationTitle peerAddress={peerAddress} />, }); - }, [peerAddress, navigation]); + }, [peerAddress]);
43-44: Avoid unnecessary type assertionThe type assertion
"new-conversation" as ConversationTopicmay indicate a mismatch between the expected type and the provided value. Ensure that"new-conversation"is a validConversationTopicor adjust the type definitions to accept this value without casting.Consider adjusting the code as follows:
<ConversationComposerStoreProvider - storeName={"new-conversation" as ConversationTopic} + storeName={"new-conversation"} inputValue={textPrefill} >
69-71: Remove unnecessary non-null assertion onpeerAddressSince
peerAddressis already typed as a non-nullablestring, the non-null assertionpeerAddress!is unnecessary.Apply this diff to remove the non-null assertion:
return createConversationByAccount(currentAccount, peerAddress!); +return createConversationByAccount(currentAccount, peerAddress);
97-106: Implement optimistic updates for better user experienceThe commented-out
onMutatefunction suggests plans to implement optimistic updates. Enabling this feature can improve the user experience by providing immediate feedback when a new conversation is initiated.Consider uncommenting and completing the
onMutatefunction:}, + onMutate: (peerAddress) => { + const currentAccount = getCurrentAccount()!; + // Implement optimistic update logic here + }, });
150-163: Implement optimistic updates for message sendingSimilarly, in the
sendMessageAsyncmutation, implementing theonMutatefunction for optimistic updates can provide immediate feedback to the user when sending messages.Consider uncommenting and completing the
onMutatefunction:}); + // onMutate: (args) => { + // const { conversation } = args; + // const currentAccount = getCurrentAccount()!; + // // Implement optimistic update logic here + // },
120-138: Optimize message sending by combining text and attachments in repliesWhen both
textandremoteAttachmentare provided with areferencedMessageId, the current implementation sends them as separate messages. If the API supports it, consider sending both in a single message to reduce the number of messages and improve efficiency.Adjust the logic as follows:
if (referencedMessageId) { - if (remoteAttachment) { - await conversation.send({ - reply: { - reference: referencedMessageId, - content: { remoteAttachment }, - }, - }); - } - if (text) { - await conversation.send({ - reply: { - reference: referencedMessageId, - content: { text }, - }, - }); - } + const content: any = {}; + if (text) { + content.text = text; + } + if (remoteAttachment) { + content.remoteAttachment = remoteAttachment; + } + await conversation.send({ + reply: { + reference: referencedMessageId, + content, + }, + }); return; }
140-149: Optimize message sending by combining text and attachmentsWhen sending a message without a
referencedMessageId, and bothtextandremoteAttachmentare provided, the current implementation sends them separately. Consider combining them into a single message to enhance efficiency.Adjust the code as follows:
- if (remoteAttachment) { - await conversation.send({ - remoteAttachment, - }); - } - - if (text) { - await conversation.send(text); - } + const content: any = {}; + if (text) { + content.text = text; + } + if (remoteAttachment) { + content.remoteAttachment = remoteAttachment; + } + await conversation.send(content);
182-184: Enhance error handling by providing detailed user feedbackCurrently, the error message shown to the user is generic. Consider customizing the error message based on the type of error to provide more informative feedback, improving the user experience.
For example:
} catch (error) { - showSnackbar({ message: "Failed to send message" }); + showSnackbar({ message: error instanceof NetworkError ? "Network error: Please check your connection." : "Failed to send message." }); sentryTrackError(error); }Ensure that sensitive error details are not exposed to the user.
features/conversation/conversation-composer/conversation-composer.tsx (5)
38-58: Consider using event-based updates instead of polling inwaitUntilMediaPreviewIsUploadedThe current implementation polls the media upload status every 200ms with a timeout of 10 seconds. Using event listeners or promises that resolve upon upload completion can improve efficiency and reduce unnecessary function calls.
111-114: Address the TODO: Fix scrolling functionalityThere's a commented-out TODO regarding the
converseEventEmitter.emitfunction for scrolling the chat to the new message. Please implement this to ensure users are scrolled to the latest message after sending.Would you like assistance in implementing this functionality or opening a GitHub issue to track this task?
181-183: VerifySendButtonenablement during media uploadThe
SendButtonis enabled when there's text input or the media has been uploaded. If media is still uploading, the button remains disabled. Please confirm that this behavior aligns with the intended user experience.
280-283: Use store updater functions instead of directly mutating stateInstead of directly setting the state with
setState, consider using a defined updater function in your store, such assetInputValue(text). This approach enhances code readability and aligns with best practices for state management in Zustand.
319-323: Improve platform checks inonKeyPresshandlerThe current platform check
Platform.OS !== "web"might need refinement, especially if web or tablet support becomes a focus in the future. Consider using a more robust method to detect the platform or input method to ensure consistent behavior across all devices.features/conversation/conversation.store-context.tsx (2)
10-11: Remove unnecessary intersection with empty object in type definitionThe intersection with an empty object
{}intype IConversationStoreState = IConversationStoreProps & {};is redundant and can be omitted for simplicity.Apply this diff to remove the unnecessary intersection:
- type IConversationStoreState = IConversationStoreProps & {}; + type IConversationStoreState = IConversationStoreProps;
48-48: Correct error messages to reflect the provider's actual nameThe error messages in
useConversationStoreContextanduseConversationStoreshould referenceConversationStoreProviderinstead ofConversationStore.Providerto match the actual component name and avoid confusion.Apply this diff to update the error messages:
// In useConversationStoreContext - if (!store) throw new Error("Missing ConversationStore.Provider in the tree"); + if (!store) throw new Error("Missing ConversationStoreProvider in the tree"); // In useConversationStore - if (!store) throw new Error("Missing ConversationStore.Provider in the tree"); + if (!store) throw new Error("Missing ConversationStoreProvider in the tree");Also applies to: 54-54
features/conversation/conversation-message.store-context.tsx (1)
48-52: Optimize useEffect dependencies to prevent unnecessary re-rendersIn the
useEffecthook, you are using[props]as the dependency array. Sincepropsis an object that may be recreated on each render, this could cause theuseEffectto execute unnecessarily, leading to performance issues.Consider destructuring the specific properties from
propsthat are relevant to the effect, and include them individually in the dependency array. This will prevent unnecessary updates when unrelated props change.useEffect(() => { storeRef.current?.setState({ ...getStoreStateBasedOnProps(props), }); - }, [props]); + }, [props.message, props.previousMessage, props.nextMessage]);features/conversation/conversation-composer/conversation-composer.store-context.tsx (1)
61-65: Ensure store state updates when props changeIn
ConversationComposerStoreProvider, the store is created once and does not update whenpropschange. Ifprops.inputValueor other props change over time, the store will not reflect these updates, potentially causing inconsistencies.Consider adding a
useEffecthook to update the store's state when the relevantpropschange, similar to how it's handled inMessageContextStoreProvider.export const ConversationComposerStoreProvider = memo( ({ children, ...props }: IConversationComposerStoreProviderProps) => { const storeRef = useRef<IConversationComposerStore>(); if (!storeRef.current) { storeRef.current = createConversationComposerStore(props); } + // Update the store when props change + useEffect(() => { + storeRef.current?.setState((state) => ({ + ...state, + ...props, + })); + }, [props.inputValue]); return ( <ConversationComposerStoreContext.Provider value={storeRef.current}> {children} </ConversationComposerStoreContext.Provider> ); } );features/conversation/conversation-composer/conversation-composer-add-attachment-button.tsx (1)
49-49: Simplify condition using optional chainingYou can simplify the conditional check
if (match && match[1])by using optional chaining and nullish coalescing. This makes the code more concise and readable.- if (match && match[1]) { + if (match?.[1]) {🧰 Tools
🪛 Biome (1.9.4)
[error] 49-49: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
features/conversation/conversation-composer/conversation-composer-reply-preview.tsx (1)
245-319: Refactor Message Type Handling for Improved MaintainabilityThe
ReplyPreviewMessageContentcomponent contains multiple conditional checks for different message types, resulting in a lengthy function. Consider refactoring this logic using a mapping or switch-case structure to enhance readability and make it easier to maintain and extend in the future.features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx (1)
92-105: Optimize Reaction Handling LogicIn the
handleSelectReactionfunction, the logic for adding or removing reactions can be simplified for better readability. Consider refactoring the condition to make the code more concise.Apply this diff to simplify the logic:
const handleSelectReaction = useCallback( (emoji: string) => { const alreadyReacted = bySender?.[currentUserInboxId!]?.some( (reaction) => reaction.content === emoji ); - if (currentUserAlreadyReacted) { + if (alreadyReacted) { removeReactionOnMessage({ messageId: messageId, emoji, }); } else { reactOnMessage({ messageId: messageId, emoji }); } messageContextMenuStore.getState().setMessageContextMenuData(null); },design-system/touchable-without-feedback.tsx (1)
7-11: Consider Removing Memoization for SimplicityThe
TouchableWithoutFeedbackcomponent is a direct wrapper aroundRNTouchableWithoutFeedbackwithout additional state or props transformation. Memoizing this component might not provide performance benefits and could be unnecessary. Consider removingmemofor simplicity.Apply this diff to remove memoization:
-import { memo } from "react"; +import React from "react"; -export const TouchableWithoutFeedback = memo(function TouchableWithoutFeedback( +export function TouchableWithoutFeedback( props: TouchableWithoutFeedbackProps ) { return <RNTouchableWithoutFeedback {...props} />; -}); +}features/conversation/conversation-message/conversation-message-container.tsx (1)
1-2: Good architectural change moving to context-based state management.The shift from prop-based to context-based state management for
fromMealigns well with the decoupling objective. This reduces prop drilling and centralizes the message ownership state.Consider memoizing the context selector to prevent unnecessary re-renders:
- const { fromMe } = useMessageContextStoreContext(useSelect(["fromMe"])); + const { fromMe } = useMessageContextStoreContext( + useCallback(useSelect(["fromMe"]), []) + );Also applies to: 11-11
features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.service..ts (1)
Line range hint
13-26: Good addition of error handling for bottomSheetModalRef.The error handling for uninitialized bottomSheetModalRef is a good defensive programming practice. However, consider adding type safety for the rolledUpReactions parameter.
Add type validation:
export function openMessageReactionsDrawer( rolledUpReactions: RolledUpReactions ) { try { + if (!rolledUpReactions || typeof rolledUpReactions !== 'object') { + throw new Error('Invalid rolledUpReactions parameter'); + } if (!bottomSheetModalRef.current) { throw new Error( "Bottom sheet modal reference is not initialized. Ensure the component is mounted."components/StateHandlers/HydrationStateHandler.tsx (1)
28-28: Track technical debt with issue.The TODO comment indicates known technical debt regarding account logic refactoring.
Would you like me to create a GitHub issue to track this technical debt and the planned refactoring of account logic?
features/conversation/conversation-messages-list.tsx (1)
43-48: Document or remove commented codeThe commented
maintainVisibleContentPositionandestimatedListSizeconfigurations should either be:
- Implemented if needed
- Removed if not needed
- Documented with a clear TODO if planned for future
features/conversation/conversation-message-date-change.tsx (2)
Line range hint
71-73: Remove commented debugBorder styleThe commented debugBorder style should be removed if not needed for development.
Line range hint
21-22: Add null check for sentAtThe
sentAtvalue is used directly ingetLocalizedTimeandgetRelativeDatewithout null checking, which could cause runtime errors.Apply this diff to add null checks:
const [sentAt, showDateChange] = useMessageContextStoreContext((s) => [ s.sentAt, s.showDateChange, ]); - const messageTime = sentAt ? getLocalizedTime(sentAt) : ""; + const messageTime = sentAt != null ? getLocalizedTime(sentAt) : "";Also applies to: 31-31
features/conversation/conversation-message/conversation-message.tsx (2)
51-64: Track unimplemented message typesThere are three message types (ReadReceipt, TransactionReference, CoinbasePayment) marked with TODO comments but no tracking.
Would you like me to create GitHub issues to track the implementation of these message types?
66-66: Remove unused variableThe
_ensureNevervariable is unused and can be removed. If it's intended for exhaustiveness checking, consider using a proper TypeScript exhaustiveness check.Apply this diff:
- const _ensureNever = message; + // Use TypeScript's never type for exhaustiveness checking + const assertNever = (x: never): never => { + throw new Error('Unexpected message type: ' + x); + }; + // This will cause a compile-time error if a message type is not handled + // assertNever(message);features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.utils.tsx (1)
Line range hint
71-85: Consider implementing or removing the TODO comment about share frame.The commented code block contains a TODO for implementing share frame functionality. Consider either implementing this feature or removing the commented code if it's no longer needed.
Would you like me to help implement the share frame functionality or create a GitHub issue to track this task?
features/conversation/conversation-consent-popup/conversation-consent-popup-dm.tsx (1)
Line range hint
48-63: Consider sequential execution for consent operations.The parallel execution of consent operations using
Promise.allcould lead to inconsistent states if one operation fails. Consider executing these operations sequentially or implementing proper rollback mechanisms.- await Promise.all([ - consentToGroupsOnProtocolByAccount({ - account: currentAccount, - groupIds: [conversationId], - consent: args.consent, - }), - consentToInboxIdsOnProtocolByAccount({ - account: currentAccount, - inboxIds: [peerInboxId], - consent: args.consent, - }), - ]); + try { + await consentToGroupsOnProtocolByAccount({ + account: currentAccount, + groupIds: [conversationId], + consent: args.consent, + }); + await consentToInboxIdsOnProtocolByAccount({ + account: currentAccount, + inboxIds: [peerInboxId], + consent: args.consent, + }); + } catch (error) { + // TODO: Implement rollback mechanism + throw error; + }features/conversation/ChatPlaceholder.tsx (3)
90-90: Use translation for blocked user message.The string "This user is blocked" should use the translation system for consistency.
- This user is blocked + {translate("user_blocked")}
103-103: Use translation for action sheet options.The strings "Unblock" and "Cancel" should use the translation system.
- options: ["Unblock", "Cancel"], + options: [translate("unblock"), translate("cancel")],
159-159: Consider making the greeting emoji configurable.The wave emoji is hardcoded in the sendMessage call. Consider making it configurable or using a translation key for different cultural contexts.
+ const DEFAULT_GREETING = "👋"; // ... sendMessage({ conversation, - content: "👋", + content: translate("default_greeting", DEFAULT_GREETING), contentType: "xmtp.org/text:1.0", });
📜 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 (82)
components/Chat/Chat.tsx(0 hunks)components/Chat/ChatDumb.tsx(0 hunks)components/Chat/ChatPlaceholder/ChatPlaceholder.tsx(0 hunks)components/Chat/Message/Message.tsx(0 hunks)components/Chat/Message/MessageActions.tsx(0 hunks)components/Chat/Message/V3Message.tsx(0 hunks)components/Chat/Message/components/message-layout.tsx(0 hunks)components/Chat/Message/message-content-types/message-simple-text.tsx(0 hunks)components/Chat/Message/stores/message-store.tsx(0 hunks)components/Chat/Transaction/TransactionPreview.tsx(1 hunks)components/Conversation/V3Conversation.tsx(0 hunks)components/Conversation/V3ConversationHeader.tsx(0 hunks)components/ConversationList/ChatNullState.tsx(1 hunks)components/Onboarding/ConnectViaWallet/ConnectViaWallet.store.tsx(1 hunks)components/PinnedConversations/PinnedMessagePreview.tsx(1 hunks)components/PinnedConversations/PinnedV3DMConversation.tsx(2 hunks)components/PinnedConversations/PinnedV3GroupConversation.tsx(2 hunks)components/Recommendations/Recommendations.tsx(1 hunks)components/StateHandlers/HydrationStateHandler.tsx(2 hunks)components/V3DMListItem.tsx(1 hunks)components/V3GroupConversationListItem.tsx(1 hunks)data/store/chatStore.ts(1 hunks)design-system/touchable-without-feedback.tsx(1 hunks)features/conversation-list/hooks/useMessageIsUnread.ts(1 hunks)features/conversation-list/hooks/useMessageText.ts(1 hunks)features/conversation-list/useV3ConversationItems.ts(2 hunks)features/conversation/ChatPlaceholder.tsx(1 hunks)features/conversation/composer/composer.tsx(0 hunks)features/conversation/conversation-attachment/conversation-attachment-loading.tsx(0 hunks)features/conversation/conversation-attachment/conversation-attachment-remote-image.tsx(2 hunks)features/conversation/conversation-composer/conversation-composer-add-attachment-button.tsx(2 hunks)features/conversation/conversation-composer/conversation-composer-reply-preview.tsx(1 hunks)features/conversation/conversation-composer/conversation-composer.store-context.tsx(1 hunks)features/conversation/conversation-composer/conversation-composer.tsx(1 hunks)features/conversation/conversation-consent-popup/conversation-consent-popup-dm.tsx(3 hunks)features/conversation/conversation-consent-popup/conversation-consent-popup-group.tsx(1 hunks)features/conversation/conversation-context.tsx(0 hunks)features/conversation/conversation-group-context.tsx(0 hunks)features/conversation/conversation-keyboard-filler.tsx(1 hunks)features/conversation/conversation-message-date-change.tsx(1 hunks)features/conversation/conversation-message-sender-avatar.tsx(1 hunks)features/conversation/conversation-message-timestamp.tsx(1 hunks)features/conversation/conversation-message.store-context.tsx(1 hunks)features/conversation/conversation-message/conversation-message-container.tsx(1 hunks)features/conversation/conversation-message/conversation-message-content-container.tsx(1 hunks)features/conversation/conversation-message/conversation-message-content-types/conversation-message-chat-group-update.tsx(2 hunks)features/conversation/conversation-message/conversation-message-content-types/conversation-message-remote-attachment.tsx(2 hunks)features/conversation/conversation-message/conversation-message-content-types/conversation-message-reply.tsx(5 hunks)features/conversation/conversation-message/conversation-message-content-types/conversation-message-simple-text.tsx(1 hunks)features/conversation/conversation-message/conversation-message-content-types/conversation-message-static-attachment.tsx(2 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-above-message-reactions.tsx(2 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-container.tsx(1 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-emoji-picker/conversation-message-context-menu-emoji-picker-list.tsx(1 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-emoji-picker/conversation-message-context-menu-emoji-picker.tsx(1 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-items.tsx(1 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-reactors.tsx(1 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.store.tsx(1 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx(1 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.utils.tsx(1 hunks)features/conversation/conversation-message/conversation-message-layout.tsx(1 hunks)features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.service..ts(1 hunks)features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.store.tsx(1 hunks)features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.tsx(1 hunks)features/conversation/conversation-message/conversation-message-reactions/conversation-message-reactions.tsx(1 hunks)features/conversation/conversation-message/conversation-message-reactions/conversation-message-reactions.types.ts(1 hunks)features/conversation/conversation-message/conversation-message.tsx(1 hunks)features/conversation/conversation-message/conversation-message.utils.tsx(5 hunks)features/conversation/conversation-messages-list.tsx(1 hunks)features/conversation/conversation-new-dm.tsx(1 hunks)features/conversation/conversation-persisted-stores.ts(0 hunks)features/conversation/conversation-service.ts(0 hunks)features/conversation/conversation-store.ts(0 hunks)features/conversation/conversation-title.tsx(1 hunks)features/conversation/conversation.service.ts(1 hunks)features/conversation/conversation.store-context.tsx(1 hunks)features/conversation/conversation.tsx(1 hunks)features/conversation/dm-conversation.screen.tsx(0 hunks)features/conversation/use-mark-as-read-on-enter.tsx(1 hunks)features/conversations/components/DmConversationTitle.tsx(2 hunks)features/conversations/components/GroupConversationTitle.tsx(2 hunks)features/conversations/components/NewConversationTitle.tsx(2 hunks)features/conversations/components/V3ConversationFromPeer.tsx(0 hunks)
⛔ Files not processed due to max files limit (28)
- features/conversations/hooks/use-react-on-message.ts
- features/conversations/hooks/use-remove-reaction-on-message.ts
- features/conversations/hooks/use-send-message.ts
- features/conversations/utils/hasNextMessageInSeries.ts
- features/conversations/utils/isConversationAllowed.ts
- features/conversations/utils/isConversationDm.ts
- features/conversations/utils/isConversationGroup.ts
- features/conversations/utils/messageIsFromCurrentUser.ts
- features/search/components/NavigationChatButton.tsx
- hooks/useCurrentAccountXmtpClient.ts
- hooks/useGroupConsent.ts
- ios/Converse.xcodeproj/project.pbxproj
- queries/useConversationMessages.ts
- queries/useConversationQuery.ts
- queries/useConversationWithPeerQuery.ts
- screens/Conversation.tsx
- screens/ConversationList.tsx
- screens/ConversationReadOnly.tsx
- screens/Navigation/ConversationNav.tsx
- screens/Navigation/Navigation.tsx
- screens/Navigation/navHelpers.test.ts
- screens/Navigation/navHelpers.ts
- screens/Profile.tsx
- utils/attachment/attachment.utils.ts
- utils/attachment/types.ts
- utils/xmtpRN/client.types.ts
- utils/xmtpRN/send.ts
- utils/xmtpRN/sync.ts
💤 Files with no reviewable changes (20)
- features/conversation/conversation-attachment/conversation-attachment-loading.tsx
- features/conversation/conversation-persisted-stores.ts
- features/conversation/conversation-context.tsx
- components/Chat/Message/V3Message.tsx
- features/conversation/conversation-group-context.tsx
- components/Chat/Message/message-content-types/message-simple-text.tsx
- components/Conversation/V3Conversation.tsx
- components/Chat/ChatPlaceholder/ChatPlaceholder.tsx
- components/Chat/Message/Message.tsx
- features/conversation/composer/composer.tsx
- features/conversation/dm-conversation.screen.tsx
- components/Chat/Message/stores/message-store.tsx
- components/Conversation/V3ConversationHeader.tsx
- components/Chat/Message/components/message-layout.tsx
- features/conversations/components/V3ConversationFromPeer.tsx
- features/conversation/conversation-service.ts
- components/Chat/Message/MessageActions.tsx
- features/conversation/conversation-store.ts
- components/Chat/Chat.tsx
- components/Chat/ChatDumb.tsx
✅ Files skipped from review due to trivial changes (8)
- features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-container.tsx
- features/conversation/conversation-message/conversation-message-reactions/conversation-message-reactions.types.ts
- features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.tsx
- features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-items.tsx
- features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-emoji-picker/conversation-message-context-menu-emoji-picker.tsx
- features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-reactors.tsx
- features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.store.tsx
- features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-above-message-reactions.tsx
🧰 Additional context used
🪛 eslint
features/conversation/ChatPlaceholder.tsx
[error] 63-63: 'Text' is not defined.
(react/jsx-no-undef)
[error] 82-82: 'Text' is not defined.
(react/jsx-no-undef)
[error] 135-135: 'Text' is not defined.
(react/jsx-no-undef)
🪛 Biome (1.9.4)
features/conversation/conversation-composer/conversation-composer-add-attachment-button.tsx
[error] 49-49: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.store.tsx
[error] 6-6: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (54)
features/conversation/use-mark-as-read-on-enter.tsx (1)
3-11: LGTM! Clean and well-typed interface.
The hook's interface is well-designed with proper TypeScript types and clear, descriptive prop names.
features/conversation-list/useV3ConversationItems.ts (1)
1-1: LGTM! Verify the utility function implementation.
Good separation of concerns by moving the conversation allowance check to a dedicated utility function.
Let's verify the implementation of the utility function:
✅ Verification successful
Utility function implementation is correct and consistently used
The verification shows that isConversationAllowed is a simple, focused utility that checks if a conversation's state is "allowed". It's used consistently across the codebase in two main places:
- Conversation list filtering in
useV3ConversationItems.ts - Conditional rendering in
conversation.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of isConversationAllowed utility
# Find and display the implementation
ast-grep --pattern 'export function isConversationAllowed($_) {
$$$
}'
# Find all usages to ensure consistent behavior
rg -A 3 'isConversationAllowed'
Length of output: 2594
features/conversation-list/hooks/useMessageText.ts (1)
1-4: LGTM! Import path change aligns with decoupling objective.
The relocation of utility functions from @/components/Chat/Message/message-utils to @/features/conversation/conversation-message/conversation-message.utils better organizes the code by grouping conversation-related utilities together.
Let's verify that all other files have been updated to use the new import path:
✅ Verification successful
Import path change is correctly implemented across the codebase
The new import path @/features/conversation/conversation-message/conversation-message.utils is consistently used across multiple files in the codebase, and there are no remaining references to the old import paths. The refactoring has been thoroughly applied.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old import path
# Expect: No results, indicating all imports have been updated
rg -l "@/components/Chat/Message/message-utils"
Length of output: 47
Script:
#!/bin/bash
# Let's verify the new import path is being used consistently
rg -l "@/features/conversation/conversation-message/conversation-message.utils"
# Also check if there are any other variations of the old path that might have been missed
rg -l "Chat/Message/message-utils"
rg -l "Message/message-utils"
Length of output: 1090
features/conversation/conversation-message/conversation-message-content-container.tsx (2)
1-2: LGTM: Clean import structure for store integration
The addition of store-related imports aligns well with the shift towards centralized state management. The imports are properly organized and specific to their purpose.
8-9: Verify the impact of removing the fromMe prop
The component signature has been simplified by removing the fromMe prop. While this is a cleaner approach that leverages context, we should verify that all consumers of this component are aware of this breaking change.
✅ Verification successful
Based on the search results, I can see that MessageContentContainer is only used in one place - the conversation-message-layout.tsx file, and it's being used correctly with just the children prop. The removal of the fromMe prop hasn't caused any breaking changes as the component is only used in this single location, where the fromMe logic is now handled by the parent component directly.
No breaking changes detected - component usage is consistent with new signature
The fromMe prop removal is safe as:
- The component is only used in
conversation-message-layout.tsx - The parent component already has access to the
fromMevalue and handles the conditional rendering directly - No other usages of
MessageContentContainerwithfromMeprop were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct usage of fromMe prop with MessageContentContainer
rg -l "MessageContentContainer.*fromMe" --type tsx
# Search for all instances of MessageContentContainer to verify proper usage
rg -A 2 "MessageContentContainer" --type tsx
Length of output: 153
Script:
#!/bin/bash
# Search for any remaining direct usage of fromMe prop with MessageContentContainer
rg -l "MessageContentContainer.*fromMe"
# Search for all instances of MessageContentContainer to verify proper usage
rg -A 2 "MessageContentContainer"
# Find all TypeScript/React files that might use MessageContentContainer
fd "\.(tsx?|jsx?)$"
Length of output: 30450
features/conversation/conversation-message/conversation-message-content-types/conversation-message-static-attachment.tsx (1)
2-3: Verify the impact of import path changes across the codebase.
The import paths have been updated to reflect the new feature-based architecture, moving from @/components/Chat/Attachment/* to @/features/conversation/conversation-attachment/*. This change aligns with decoupling conversations.
Let's verify if all imports have been consistently updated:
✅ Verification successful
Import paths have been correctly updated across the codebase
The verification shows that:
- The new import paths
@/features/conversation/conversation-attachment/*are consistently used in both files that reference these components - No instances of the old import path
@/components/Chat/Attachment/*were found in the codebase - The import changes are part of a clean architectural shift from a generic "Chat" feature to a more specific "conversation" feature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining old import paths that might have been missed
rg -l "@/components/Chat/Attachment/(attachment-container|attachment-loading)"
# Search for new import paths to ensure consistent adoption
rg -l "@/features/conversation/conversation-attachment/(conversation-attachment-container|conversation-attachment-loading)"
Length of output: 414
Script:
#!/bin/bash
# Let's check the content of these files to understand the import usage
echo "=== conversation-message-static-attachment.tsx ==="
cat features/conversation/conversation-message/conversation-message-content-types/conversation-message-static-attachment.tsx
echo -e "\n=== conversation-attachment-remote-image.tsx ==="
cat features/conversation/conversation-attachment/conversation-attachment-remote-image.tsx
# Also search for any other potential old paths that might have been missed
rg -l "@/components/Chat/Attachment"
Length of output: 8125
features/conversation/conversation-message/conversation-message-reactions/conversation-message-reactions.tsx (1)
13-20: LGTM! Clean architectural reorganization.
The move towards a feature-based directory structure and separation of types improves code organization and maintainability.
features/conversation/conversation-keyboard-filler.tsx (2)
1-4: LGTM! Clean and well-organized imports.
The imports are properly structured and include all necessary dependencies for the component's functionality.
10-12: Verify keyboard behavior across different devices and orientations.
The height calculation looks correct, but keyboard behavior can vary across different devices and orientations.
Run the following script to check for potential device-specific keyboard handling:
✅ Verification successful
Based on the search results, I can now generate the final response:
Keyboard height calculation is correctly implemented with platform-specific handling
The implementation is robust because:
- Uses
useAnimatedKeyboardfrom Reanimated for optimal performance - Correctly handles safe area insets to prevent layout issues
- Has proper platform-specific handling as evidenced by the Drawer component
- Math.max safeguard prevents negative heights
- Follows the same pattern as other keyboard-aware components in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for keyboard-related edge cases and device-specific handling
# Check for other keyboard-related components and potential conflicts
rg -A 5 "useAnimatedKeyboard|KeyboardAvoidingView|keyboardHeight"
# Look for device-specific keyboard handling
rg -A 5 "Platform.select.*keyboard|Platform.OS.*keyboard"
Length of output: 2764
features/conversation/conversation-message/conversation-message-content-types/conversation-message-chat-group-update.tsx (3)
22-24: LGTM: Type rename aligns with new naming convention
The type rename from IChatGroupUpdatedMessageProps to IMessageChatGroupUpdateProps follows a more consistent naming pattern, aligning with the broader refactoring effort.
26-28: LGTM: Component rename maintains consistency
The component rename from ChatGroupUpdatedMessage to MessageChatGroupUpdate maintains consistency with the type changes and aligns with the broader refactoring effort.
20-20: Verify the MessageDateChange import path
The import path has been updated to use a relative path going up two directories. Let's ensure this path is correct and the component is accessible.
✅ Verification successful
Let me verify the export of MessageDateChange component with a different pattern since the previous one didn't yield results.
Import path for MessageDateChange is correct
The component exists at the expected location features/conversation/conversation-message-date-change.tsx and is properly exported as a named constant. The relative import path ../../conversation-message-date-change from conversation-message-content-types/conversation-message-chat-group-update.tsx correctly resolves to this file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and location of MessageDateChange component
# Expected: Find the component at the new location
# Search for the file
fd "conversation-message-date-change.tsx"
# Verify the exports
ast-grep --pattern 'export { MessageDateChange }'
Length of output: 152
Script:
#!/bin/bash
# Check the content of the file to verify the export
cat features/conversation/conversation-message-date-change.tsx
# Alternative pattern to find export
ast-grep --pattern 'export const MessageDateChange'
Length of output: 8000
features/conversation/conversation-message-timestamp.tsx (1)
1-4: Verify the new store location aligns with feature-based architecture.
The import path change from Chat/Message to features/conversation aligns well with the PR objective of decoupling conversation components. This moves towards a more feature-based organization.
Let's verify the new structure is consistent across the codebase:
✅ Verification successful
Let me gather more information to verify if this is a consistent architectural change.
Import path change is consistent with feature-based architecture
The new store location @/features/conversation/conversation-message.store-context is being consistently used across multiple conversation-related components. The only reference to the old path structure is in a commented-out import, which appears to be a work-in-progress migration. The store is properly integrated into the feature-based architecture, with all active imports using the new path.
- The store is used in 10 different files within the conversation feature
- The old store file (
message-store) is not present in the codebase - Components are properly organized under the features directory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of the new feature-based structure
# Check if other components are also moved to the features directory
rg -g '!*.test.*' -g '!*.spec.*' "from.*@/components/Chat/Message"
# Verify the new store location is properly structured
fd conversation-message.store-context
Length of output: 370
Script:
#!/bin/bash
# Check the content of the commented import to understand if it's a leftover comment
rg -A 5 "// import.*MessageContextMenu" features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx
# Check if there are any other imports from the old path structure
rg "from.*@/components/Chat/Message" --type ts --type tsx
# Check if there are any other components using the new store context
rg "from.*conversation-message\.store-context" --type ts --type tsx
Length of output: 568
Script:
#!/bin/bash
# Check imports without file type restrictions
rg "from.*@/components/Chat/Message"
# Check store context usage without file type restrictions
rg "from.*conversation-message\.store-context"
# Check if the old store file still exists
fd "message-store" --type f
Length of output: 2037
features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.store.tsx (2)
8-20: LGTM! Well-structured state interface.
The state interface is well-defined with clear types and includes all necessary positioning data for precise context menu placement.
29-41: LGTM! Well-implemented store provider.
The implementation follows React best practices:
- Uses memo for performance optimization
- Maintains singleton store instance with useRef
- Implements lazy initialization
features/conversation/conversation-title.tsx (1)
Line range hint 20-42: LGTM! Clean implementation of the conversation title component
The component follows React Native best practices:
- Proper use of
AnimatedHStackfor animations - Efficient touch handling with both
onPressandonLongPress - Text truncation with
numberOfLines - Disabled font scaling for consistent UI
features/conversations/components/DmConversationTitle.tsx (1)
Line range hint 69-83: LGTM! Consider consistent memoization pattern
The implementation is clean. Similar to NewConversationTitle, consider memoizing the avatar component for consistency across conversation types.
Let's verify the XMTP SDK usage:
✅ Verification successful
Based on the extensive search results, I can now generate the final response:
XMTP SDK usage is consistent and well-integrated
The implementation shows proper and consistent XMTP SDK usage across conversation components:
- Uses
@xmtp/react-native-sdkversion 3.1.1 consistently - Correctly handles conversation topics and message decoding
- Properly integrates with XMTP's content types and codecs
- Follows the SDK's patterns for DM conversations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent XMTP SDK usage across conversation components
rg -A 2 "from '@xmtp/react-native-sdk'" --type typescript
Length of output: 100
Script:
#!/bin/bash
# Let's try different approaches to verify XMTP SDK usage
# 1. Search for XMTP imports without specifying file type
rg "from '@xmtp/"
# 2. Search for XMTP-related code patterns
rg -A 3 "xmtp" -i
# 3. Find files that might be related to XMTP conversations
fd -e ts -e tsx | xargs rg "conversation.*xmtp" -i
Length of output: 65629
features/conversations/components/GroupConversationTitle.tsx (2)
16-16: Verify architectural consistency across the codebase.
The import path structure has changed from conversations (plural) to conversation (singular), which might indicate an architectural change. Let's verify the consistency of this pattern across the codebase.
✅ Verification successful
Import path changes are part of a consistent architectural update
Based on the results, the codebase shows a clear and intentional architectural organization:
- The singular
@/features/conversation/contains core conversation components and functionality - The plural
@/features/conversations/contains higher-level components and utilities @/features/conversation-list/handles conversation listing features
The import change in GroupConversationTitle.tsx aligns with this architecture, where core UI components like ConversationTitle are correctly placed under the singular conversation feature folder.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential inconsistencies in feature folder naming
# and import patterns across the codebase
# Check for both singular and plural versions of the conversation feature folder
echo "Checking feature folder structure:"
fd -t d "conversation[s]?" features/
# Check import patterns to identify any mixed usage
echo -e "\nChecking import patterns:"
rg -g "*.{ts,tsx}" "@/features/conversation[s]?"
Length of output: 22643
Line range hint 82-87: LGTM! Clean implementation of the new ConversationTitle component.
The migration from ConversationTitleDumb to ConversationTitle is well-structured, maintaining the necessary props while potentially enabling additional features. The implementation follows React best practices.
features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-emoji-picker/conversation-message-context-menu-emoji-picker-list.tsx (1)
18-18: LGTM! Verify naming consistency across the codebase.
The import path change aligns with the feature-based architecture pattern.
Let's verify the naming consistency across related files:
✅ Verification successful
Import path change is consistent with the codebase structure
The verification confirms that:
- The new import path
./conversation-message-context-menu-emoji-picker-rowis correctly used - All related files follow the new naming convention with the
conversation-message-context-menu-emoji-pickerprefix - The EmojiRow component is properly referenced across the emoji picker feature files
- No old-style naming patterns remain in the imports
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old-style naming patterns and verify new pattern adoption
# Search for old naming pattern
rg -l "message-context-menu-emoji-picker"
# Search for new naming pattern to confirm consistency
rg -l "conversation-message-context-menu-emoji-picker"
Length of output: 1036
Script:
#!/bin/bash
# Let's check the actual content of these files to verify the import statements
# and ensure there are no remaining old-style references
# Check imports and usage in the context menu file
rg "emoji-picker" features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx -A 2
# Check imports in the emoji picker component
rg "import.*emoji" features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-emoji-picker/conversation-message-context-menu-emoji-picker.tsx
# Check if there are any other potential old references
rg "message-context-menu-emoji" --type ts --type tsx
Length of output: 1772
Script:
#!/bin/bash
# Let's try again with correct file type specification and check both old and new patterns
# to ensure complete verification
rg "message-context-menu-emoji" "*.tsx" "*.ts"
# Also check the specific row component import that was changed
rg -l "EmojiRow" "*.tsx" "*.ts"
Length of output: 260
Script:
#!/bin/bash
# Let's fix the command syntax for file type matching
# Check for EmojiRow component references
fd -e tsx -e ts -x rg -l "EmojiRow" {}
# Check imports in the list component to verify the change
rg "import.*EmojiRow" features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-emoji-picker/conversation-message-context-menu-emoji-picker-list.tsx
Length of output: 865
features/conversation/conversation-message/conversation-message-layout.tsx (4)
1-9: LGTM! Well-organized imports.
The imports are properly structured and all seem necessary for the component's functionality.
15-18: LGTM! Good use of memo for performance optimization.
The component is properly memoized and the props interface is clean.
21-23: LGTM! Efficient state selection.
Good use of useSelect to pick only the required fields from the store.
15-46: Verify integration with different message types.
Let's ensure this layout component works correctly with all message types mentioned in the summary (MessageSimpleText, MessageChatGroupUpdate, MessageRemoteAttachment).
✅ Verification successful
From the search results, I can see that the ConversationMessageLayout is properly integrated with different message types. The ConversationMessage component in conversation-message.tsx shows it handles different message types (text messages via MessageSimpleText and group updates via MessageChatGroupUpdate) and wraps them all in the ConversationMessageLayout component. The layout is used consistently in both the main Conversation component and ConversationReadOnly screen.
Layout component is correctly integrated with message types
The ConversationMessageLayout serves as a wrapper that provides consistent layout structure (avatar, spacing, alignment) regardless of the message type, while the actual message content rendering is handled by type-specific components. This is a proper implementation of the separation of concerns principle.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of ConversationMessageLayout across different message components
rg -l "ConversationMessageLayout" --type typescript
Length of output: 86
Script:
#!/bin/bash
# Search for usage of ConversationMessageLayout with ripgrep, without type filter
rg "ConversationMessageLayout" -A 3
Length of output: 3509
Script:
#!/bin/bash
# Let's check the ConversationMessage component to see what message types it handles
rg "ConversationMessage = " -A 10
# Also search for message type definitions
rg "type.*Message.*=" -A 3
Length of output: 22491
components/Recommendations/Recommendations.tsx (1)
83-83: Parameter rename aligns with decoupling effort.
The rename from mainConversationWithPeer to peer reflects a simpler, more focused parameter name that better aligns with the decoupling of conversation types.
Let's verify this change is consistent across the codebase:
✅ Verification successful
Parameter rename is consistent across the codebase
The verification shows that the peer parameter is consistently used across the codebase for navigation to conversations. The parameter is used in:
Recommendations.tsxNavigationChatButton.tsxNewConversationNav.tsx(URL parsing)navHelpers.ts(deep linking)
There are no remaining instances of mainConversationWithPeer, confirming the rename was complete and consistent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of old parameter name
rg "mainConversationWithPeer" --type ts --type tsx
# Search for the new navigation pattern to ensure consistency
rg 'navigate\(["\']Conversation["\'],\s*\{[^}]*peer:' --type ts --type tsx
Length of output: 195
Script:
#!/bin/bash
# Search for any remaining usage of old parameter name, using ripgrep with proper file extensions
rg "mainConversationWithPeer" -t typescript -t tsx
# Search for the new navigation pattern to ensure consistency
rg 'navigate\(["\']Conversation["\'].*peer:' -t typescript -t tsx
Length of output: 202
Script:
#!/bin/bash
# Search for any remaining usage of old parameter name
rg "mainConversationWithPeer" --type-add 'typescript:*.{ts,tsx}'
# Search for navigation patterns with peer parameter
rg 'navigate.*Conversation.*peer:' --type-add 'typescript:*.{ts,tsx}'
Length of output: 136
Script:
#!/bin/bash
# First, let's find all TypeScript/TSX files that might contain navigation code
fd "\.(ts|tsx)$" | xargs grep -l "navigation\|navigate" || true
# Then search for the specific parameter in these files
fd "\.(ts|tsx)$" | xargs grep -l "contactAddress\|mainConversationWithPeer\|peer:" || true
Length of output: 4869
Script:
#!/bin/bash
# Let's examine the specific files that might be related to navigation with peer parameter
echo "=== Checking Recommendations.tsx ==="
grep -A 5 -B 5 "peer:" components/Recommendations/Recommendations.tsx || true
echo -e "\n=== Checking NavigationChatButton.tsx ==="
grep -A 5 -B 5 "peer:" features/search/components/NavigationChatButton.tsx || true
echo -e "\n=== Checking NewConversationNav.tsx ==="
grep -A 5 -B 5 "peer:" screens/Navigation/NewConversationNav.tsx || true
echo -e "\n=== Checking navHelpers.ts ==="
grep -A 5 -B 5 "peer:" screens/Navigation/navHelpers.ts || true
Length of output: 2187
components/ConversationList/ChatNullState.tsx (2)
28-28: LGTM! Import path change aligns with decoupling effort.
The change to import NewConversationButton as a sibling component aligns with the PR's objective to decouple conversation-related components.
Let's verify the new component location and its usage:
✅ Verification successful
Import path change is correctly implemented and consistent across the codebase
The verification confirms:
NewConversationButtoncomponent exists at the correct locationcomponents/ConversationList/NewConversationButton.tsx- The import in
ChatNullState.tsxuses the correct relative path./NewConversationButton - Other imports of this component (in
ConversationListNav.ios.tsxandConversationList.tsx) use the appropriate absolute paths - The component is properly integrated and used conditionally for Android platform in
ChatNullState.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify NewConversationButton component exists and is properly referenced
# Check if the component exists in the expected location
fd "NewConversationButton.tsx" --type f
# Check for any other references to ensure consistent import paths
rg "import.*NewConversationButton" --type ts --type tsx
Length of output: 179
Script:
#!/bin/bash
# Let's try with just the typescript extension and a more specific ripgrep command
# Check for imports of NewConversationButton
rg "import.*NewConversationButton" -t ts
# Also check the content of both files to verify the component structure
echo "=== Content of ChatNullState.tsx ==="
cat "components/ConversationList/ChatNullState.tsx"
echo -e "\n=== Content of NewConversationButton.tsx ==="
cat "components/ConversationList/NewConversationButton.tsx"
Length of output: 7921
Line range hint 134-134: Verify platform-specific NewConversationButton behavior.
The NewConversationButton is only rendered on Android. Ensure this platform-specific behavior is intentional and documented.
Let's check for platform-specific patterns:
✅ Verification successful
Platform-specific behavior for NewConversationButton is intentional and justified
The platform-specific rendering of NewConversationButton is part of a coherent design pattern where Android and iOS have different UI treatments:
- Android has no top border (
borderTopWidth: 0) and includes theNewConversationButton - iOS has a top border (
borderTopWidth: 1) and omits theNewConversationButton
These differences align with platform-specific UI/UX guidelines and appear to be intentionally designed as part of the component's layout strategy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check platform-specific patterns for NewConversationButton
# Look for Platform.OS checks related to NewConversationButton
rg "Platform\.OS.*NewConversationButton" --type ts --type tsx -B 2 -A 2
# Check for any documentation about platform-specific behavior
fd "README|CONTRIBUTING" --type f --exec rg -i "platform.*(android|ios).*conversation"
Length of output: 190
Script:
#!/bin/bash
# Let's try a different approach to find platform-specific patterns
# Search for NewConversationButton usage and surrounding context
rg "NewConversationButton" -t ts -t tsx -B 5 -A 5
# Search for Platform.OS checks in the same file
rg "Platform\.OS" "components/ConversationList/ChatNullState.tsx" -B 2 -A 2
Length of output: 607
Script:
#!/bin/bash
# Let's check the entire file content to understand the component structure better
cat "components/ConversationList/ChatNullState.tsx"
# Also search for any related test files that might document the behavior
fd "ChatNullState.test|ChatNullState.spec"
Length of output: 5878
features/conversation-list/hooks/useMessageIsUnread.ts (3)
20-20: LGTM! Parameter rename is consistent with type changes
The parameter rename maintains consistency with the type definition changes.
7-7: Verify the consistency of the new import path across the codebase
The import path for getCurrentUserAccountInboxId has been updated to a more specific location. Let's ensure this change is consistent across all files.
✅ Verification successful
Let me try a different search strategy to verify the import paths more effectively.
Import path is consistent across the codebase
The verification shows that:
- The function is defined in
conversation-message.utils.tsx - All imports consistently use the new path
@/features/conversation/conversation-message/conversation-message.utils - All usages of the function are properly imported
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old import paths and verify new import path usage
# Expected: All imports should use the new path pattern
# Search for any remaining old imports of getCurrentUserAccountInboxId
rg "import.*getCurrentUserAccountInboxId.*(?<!conversation-message\.utils)"
# Verify the new import is used consistently
rg "import.*getCurrentUserAccountInboxId.*conversation-message\.utils"
Length of output: 815
Script:
#!/bin/bash
# Search for all imports of getCurrentUserAccountInboxId
rg "import.*getCurrentUserAccountInboxId" -A 1
# Search for any usage of getCurrentUserAccountInboxId without import
rg "getCurrentUserAccountInboxId" --type ts
Length of output: 1343
12-12: Approve the type property rename for better clarity
The rename from timestamp to timestampNs better indicates that the value is in nanoseconds, improving code clarity.
✅ Verification successful
Property rename has been properly implemented across the codebase
The search results confirm that all usages of the useConversationIsUnread hook have been updated to use timestampNs property. Found in:
components/V3GroupConversationListItem.tsxcomponents/V3DMListItem.tsxcomponents/PinnedConversations/PinnedV3GroupConversation.tsxcomponents/PinnedConversations/PinnedV3DMConversation.tsxfeatures/conversation/conversation.tsx
All implementations correctly pass the nanosecond timestamp using the timestampNs prop, and there are no remaining instances of the old timestamp property usage with this hook.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all usages of the type have been updated
# Expected: No remaining uses of the old property name
# Search for any remaining uses of the old property name with UseConversationIsUnreadProps
ast-grep --pattern 'type UseConversationIsUnreadProps = {
$$$
timestamp: $_
$$$
}'
# Search for components using the hook to verify they've been updated
rg "useConversationIsUnread.*timestamp[^N]"
Length of output: 136
Script:
#!/bin/bash
# Let's try a broader search to find all usages of the hook and the timestamp property
# Find all files that import or use useConversationIsUnread
rg "useConversationIsUnread" -l
# Find any usage of timestamp property in the context of conversations
rg "timestamp\W" -A 2 -B 2
# Find TypeScript files containing timestamp-related properties
ast-grep --pattern 'timestamp: $_'
# Search specifically for the hook's usage with props
ast-grep --pattern 'useConversationIsUnread({
$$$
})'
Length of output: 17669
features/conversation/conversation-message/conversation-message-content-types/conversation-message-simple-text.tsx (1)
1-9: Verify the existence and implementation of custom components
The component relies on several custom UI components. Let's verify their implementation to ensure consistency.
✅ Verification successful
Let me generate one more verification script to check the store context implementation since it wasn't found in the previous results.
All custom components and hooks are properly implemented
The verification confirms that all imported components and hooks exist and are properly implemented:
BubbleContainerandBubbleContentContainerare implemented inconversation-message-bubble.tsxwith proper typing and stylingMessageTextis implemented inconversation-message-text.tsxas a memoized component with theme supportuseMessageContextStoreContextis implemented inconversation-message.store-context.tsxand used consistently across multiple components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and implementation of custom components
# Check BubbleContainer and BubbleContentContainer
ast-grep --pattern 'export const BubbleContainer = $$$'
ast-grep --pattern 'export const BubbleContentContainer = $$$'
# Check MessageText implementation
ast-grep --pattern 'export const MessageText = $$$'
# Verify store context implementation
ast-grep --pattern 'export const useMessageContextStoreContext = $$$'
Length of output: 7163
Script:
#!/bin/bash
# Search for the store context implementation with ripgrep for better context
rg "useMessageContextStoreContext" -A 5
Length of output: 15811
features/conversation/conversation-message-sender-avatar.tsx (1)
Line range hint 1-113: Verify component integration after broader refactoring
The AI summary indicates significant changes in related components (MessageLayout, V3Message removal). Let's verify that this component still integrates correctly with its parent components.
✅ Verification successful
Component integration is properly maintained
The verification shows that V3MessageSenderAvatar is correctly integrated within the ConversationMessageLayout component, which uses it appropriately for non-sender messages. The component's usage pattern and integration remain valid after the refactoring:
- Used in
conversation-message-layout.tsxwith proper imports - Correctly receives the required
inboxIdprop - Properly integrated within the message layout structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of V3MessageSenderAvatar to ensure proper integration
rg -l "V3MessageSenderAvatar" --type tsx
# Check for any potential circular dependencies
ast-grep --pattern 'import { V3MessageSenderAvatar } from $path'
Length of output: 135
Script:
#!/bin/bash
# Search for usage of V3MessageSenderAvatar with proper file extension
rg -l "V3MessageSenderAvatar" --type ts --type tsx --type-add 'tsx:*.tsx'
# Look for imports of this component
rg "import.*V3MessageSenderAvatar" --type ts --type tsx --type-add 'tsx:*.tsx'
# Search for direct usage of the component in JSX
rg "<V3MessageSenderAvatar" --type ts --type tsx --type-add 'tsx:*.tsx'
# Check for any message-related components that might use this
rg -l "Message.*Avatar" --type ts --type tsx --type-add 'tsx:*.tsx'
Length of output: 884
Script:
#!/bin/bash
# Let's check the conversation-message-layout.tsx to understand the integration
cat features/conversation/conversation-message/conversation-message-layout.tsx
# Also check if there are any other message-related components that might need this avatar
rg -l "Message" --type ts --type tsx --type-add 'tsx:*.tsx'
Length of output: 9764
components/Chat/Transaction/TransactionPreview.tsx (3)
Line range hint 54-252: Remove commented code
A large portion of the file contains commented-out code, including the TransactionStatusView and main TransactionPreview components. This commented code:
- Makes the file harder to maintain
- Creates confusion about which implementation is current
- Increases cognitive load when reading the file
If this code is no longer needed, remove it entirely. If it's needed for reference, consider:
- Moving it to a separate reference file
- Creating a documentation ticket
- Preserving it in the git history
Let's verify if this code has been moved elsewhere:
✅ Verification successful
Remove commented-out code as it's been moved to a new location
The commented code in components/Chat/Transaction/TransactionPreview.tsx should be removed as the functionality has been moved to a dedicated location under components/TransactionPreview/. The active implementation exists in components/TransactionPreview/TransactionPreview.tsx and related files in the same directory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential new locations of the transaction preview implementation
rg -l "TransactionStatusView|TransactionPreview" --type typescript
Length of output: 103
Script:
#!/bin/bash
# Search for potential new locations of the transaction preview implementation
# Using ripgrep without file type restriction
rg -l "TransactionStatusView|TransactionPreview"
Length of output: 424
Script:
#!/bin/bash
# Let's check the content of the active TransactionPreview implementations
rg "TransactionStatusView|export.*TransactionPreview" -A 5 components/TransactionPreview/TransactionPreview.tsx components/Chat/Transaction/TransactionPreview.tsx
Length of output: 2227
Line range hint 254-379: Review the styling implementation
The useStyles hook contains styles for components that are no longer in use (based on the commented-out code). This suggests that:
- The styling code needs cleanup
- There might be a new styling implementation elsewhere
Let's verify if these styles are still needed:
Consider the following improvements:
- Extract common styles into a shared styles file
- Use a design system for consistent styling
- Consider using styled-components or a similar styling solution for better maintainability
✅ Verification successful
Let me try to find any references to these styles or the component itself to determine if they are still in use. If the styles are unused, we should see no matches for the style classes, and the component's implementation would help us understand the current styling approach.
The search results show that there's a newer implementation of TransactionPreview in a different location (components/TransactionPreview/TransactionPreview.tsx), and the file we're reviewing seems to be an old version. Let's verify if these styles have been migrated to the new implementation or if they're truly unused.
Remove unused styles and outdated code
The verification shows that:
- The component has been completely reimplemented in
components/TransactionPreview/TransactionPreview.tsxusing a different styling approach with@design-system/VStack - The old styles in the file under review are not used anywhere else in the codebase
- The entire file appears to be legacy code that was commented out but not removed
- Remove the entire commented-out implementation and unused styles from
components/Chat/Transaction/TransactionPreview.tsx - Consider removing the file entirely if it contains no other active code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of these style classes
rg -A 2 "transactionDetailsContainer|statusContainer|centeredStatusContainer" --type typescript
Length of output: 132
Script:
#!/bin/bash
# Search for usage of these style classes without file type restriction
rg -A 2 "transactionDetailsContainer|statusContainer|centeredStatusContainer"
# Also search for the component name to understand its usage
rg "TransactionPreview"
Length of output: 2950
Script:
#!/bin/bash
# Let's check the actual implementation in the new location
cat components/TransactionPreview/TransactionPreview.tsx
# And check if these styles are used in any other transaction-related components
rg -A 2 "styles\.(text|textMe|amount|bold|small|innerBubble|innerBubbleMe|metadataContainer|statusIcon)" components/TransactionPreview/
Length of output: 7170
Line range hint 1-379: Architectural Concerns: Incomplete Refactoring
The current state of this file raises several architectural concerns:
- The file retains only basic UI components while core transaction functionality is removed
- There's no clear indication of where the transaction logic has moved
- The relationship between this component and the new implementation is unclear
Consider the following recommendations:
- Document the new architecture in the PR description
- Update or create ADR (Architecture Decision Record) explaining the decoupling strategy
- Ensure proper migration guide exists for other developers
Let's verify the architectural changes:
components/PinnedConversations/PinnedMessagePreview.tsx (1)
7-7: LGTM! Import path change aligns with feature organization.
The relocation of isTextMessage utility to the conversation feature directory improves code organization. However, let's verify if there are other message-related utilities that should be moved similarly.
✅ Verification successful
LGTM! All message-related utilities are properly organized in the conversation feature directory.
The import path change for isTextMessage is part of a well-organized structure where all message-related utilities are centralized in features/conversation/conversation-message/conversation-message.utils.tsx. This file contains a comprehensive set of message type checking and utility functions, making it the correct location for isTextMessage. No other message utilities need to be moved.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other message-related utilities that might need relocation
rg -l "message-utils" --type ts --type tsx
Length of output: 70
Script:
#!/bin/bash
# Search for message-related utility imports across the codebase
rg "from.*message.*utils" --type typescript
Length of output: 80
Script:
#!/bin/bash
# Search for files containing isTextMessage usage
rg "isTextMessage" --type typescript
Length of output: 71
Script:
#!/bin/bash
# List all typescript files in the repository to ensure we're using correct file extensions
fd -e ts -e tsx
Length of output: 28201
Script:
#!/bin/bash
# Search for isTextMessage usage and imports with correct file extension
rg "isTextMessage" -g "*.ts" -g "*.tsx"
Length of output: 1263
Script:
#!/bin/bash
# Search for other message utility functions in the conversation feature directory
rg "export function" -g "*message.utils.ts*" -g "*message.utils.tsx*"
Length of output: 2213
components/PinnedConversations/PinnedV3GroupConversation.tsx (2)
20-20: LGTM! Import path change maintains consistency.
The relocation of isTextMessage utility matches the pattern seen in other files, maintaining consistency in the codebase.
55-55: Parameter rename improves clarity.
Renaming timestamp to timestampNs better indicates that the value is in nanoseconds. Let's verify this change is consistent across the codebase.
✅ Verification successful
Parameter rename is consistent with codebase usage patterns
The codebase consistently uses:
timestampNsfor nanosecond timestamps (e.g., in message sent times, conversation data)timestampfor millisecond timestamps (e.g., in notifications, debug utils, store data)
The rename in PinnedV3GroupConversation.tsx correctly aligns with this pattern as it's handling message sent times in nanoseconds.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining 'timestamp' parameters in conversation-related files
rg "timestamp(?!Ns)" --type ts --type tsx
Length of output: 271
Script:
#!/bin/bash
# Search for timestamp-related parameters and variables in TypeScript files
rg "timestamp" --type ts --type tsx -A 2
Length of output: 68
Script:
#!/bin/bash
# Search for timestamp-related parameters and variables
rg "timestamp" -t ts
Length of output: 5688
components/V3DMListItem.tsx (1)
72-72: LGTM! Parameter rename improves clarity
The rename from timestamp to timestampNs better reflects that the value is in nanoseconds, making the code more self-documenting.
components/V3GroupConversationListItem.tsx (1)
72-72: LGTM! Consistent parameter naming
The rename from timestamp to timestampNs maintains consistency with other components.
data/store/chatStore.ts (1)
Line range hint 268-280: Verify thread safety in topic data updates
The setTopicsData implementation might have race conditions when multiple updates occur simultaneously due to the async nature of state updates.
features/conversation/conversation.service.ts (1)
3-9: 🛠️ Refactor suggestion
Add type safety and error handling to the hooks
The hooks could benefit from TypeScript types and proper error handling for undefined states.
Consider applying these improvements:
+import { ConversationTopic } from './types';
-export function useCurrentConversationTopic() {
+export function useCurrentConversationTopic(): ConversationTopic | undefined {
return useConversationStoreContext((state) => state.topic);
}
-export function useConversationCurrentConversationId() {
+export function useConversationCurrentConversationId(): string | undefined {
return useConversationStoreContext((state) => state.conversationId);
}Let's verify the store implementation and usage:
features/conversation/conversation-consent-popup/conversation-consent-popup-group.tsx (1)
6-6: Verify the new import path structure
The import path has been changed to a feature-based structure. Let's ensure this aligns with the new architecture.
✅ Verification successful
Import path structure is consistent with the new architecture
The new feature-based import path structure is correctly implemented:
- All imports consistently use the
@/features/conversation/conversation-consent-popuppattern - No legacy imports using
Chat/consent-popupfound - The directory structure matches the imports with appropriate files:
- conversation-consent-popup-dm.tsx
- conversation-consent-popup-group.tsx
- conversation-consent-popup.design-system.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent import patterns across the codebase
rg --type typescript "from.*features/conversation/conversation-consent-popup"
# Verify no old import paths are still in use
rg --type typescript "from.*Chat.*consent-popup"
Length of output: 200
Script:
#!/bin/bash
# Check for consistent import patterns across the codebase using .ts and .tsx extensions
rg "from.*features/conversation/conversation-consent-popup" -g "*.{ts,tsx}"
# Verify no old import paths are still in use
rg "from.*Chat.*consent-popup" -g "*.{ts,tsx}"
# List all files in the conversation-consent-popup directory to understand the structure
fd . "features/conversation/conversation-consent-popup"
Length of output: 947
features/conversation/conversation-attachment/conversation-attachment-remote-image.tsx (1)
Line range hint 28-156: Excellent error handling and loading states.
The component properly handles all possible states (loading, error, download prompt, unsupported) and provides appropriate user feedback. The use of React Query for data fetching is also well implemented.
features/conversation/conversation-composer/conversation-composer.tsx (1)
28-167: Well-structured implementation of the Composer component
The Composer component is effectively designed, utilizing hooks and state management appropriately. The separation of concerns and modularization improve readability and maintainability.
features/conversation/conversation.store-context.tsx (2)
44-50: Custom hook useConversationStoreContext is well implemented
The custom hook correctly provides access to the store state with a selector function, ensuring efficient updates and state management.
52-56: Custom hook useConversationStore provides direct access to the store
The useConversationStore hook properly retrieves the store from context and returns it, allowing components to access the store directly when needed.
features/conversation/conversation-message/conversation-message.utils.tsx (1)
218-218:
Ensure 'currentAccount' is defined in hooks to avoid runtime errors
In useConversationMessageById, you're asserting that currentAccount is non-null with useCurrentAccount()!. This can cause runtime errors if useCurrentAccount() returns undefined, especially during the initial render before the account is set.
Consider checking if currentAccount is defined before proceeding. This will make your hook safer and prevent potential crashes.
export function useConversationMessageById({
messageId,
topic,
}: {
messageId: MessageId;
topic: ConversationTopic;
}) {
- const currentAccount = useCurrentAccount()!;
+ const currentAccount = useCurrentAccount();
+ if (!currentAccount) {
+ // Handle the undefined case appropriately
+ return { message: null, isLoading: false };
+ }
const { data: messages } = useConversationMessages(currentAccount, topic);
// ...
}Likely invalid or redundant comment.
features/conversation/conversation-composer/conversation-composer-reply-preview.tsx (1)
1-319: Overall Implementation is Well-Structured
The ReplyPreview and associated components are well-implemented, correctly handling various message types and providing a smooth user experience when displaying the reply preview.
features/conversation/conversation.tsx (1)
58-334: Overall Structure and Logic are Sound
The Conversation component and its nested structures are well-designed, effectively managing state and rendering the conversation interface with proper handling of messages and user interactions.
features/conversation/conversation-message/conversation-message-container.tsx (1)
1-11: Verify MessageContextStoreContext provider placement.
The component now depends on MessageContextStoreContext. Ensure the context provider is placed appropriately in the component tree to prevent undefined context errors.
✅ Verification successful
Let's analyze the context provider implementation and usage to verify its placement.
The MessageContextStoreContext provider is properly placed in the component tree.
The verification shows that MessageContainer is used within ConversationMessageLayout, which is wrapped by MessageContextStoreProvider in both the main conversation view (conversation.tsx) and read-only view (ConversationReadOnly.tsx). The provider is correctly placed higher in the component tree before any components that consume the context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for MessageContextStoreContext provider usage
ast-grep --pattern 'MessageContextStoreContext.Provider value={$_}'
Length of output: 69
Script:
#!/bin/bash
# Search for MessageContextStoreContext provider and its usage patterns
rg -l "MessageContextStoreContext" --type tsx --type ts
# Then let's look at the store context definition
rg "export.*MessageContextStoreContext" -A 5
# And search for Provider usage with more flexible pattern
rg "MessageContextStoreContext.*Provider"
Length of output: 1028
Script:
#!/bin/bash
# Let's look at the full context of the provider implementation
rg -B 5 -A 5 "<MessageContextStoreContext.Provider"
# And let's see where MessageContainer is being used
rg "MessageContainer" --type-add 'tsx:*.{tsx,ts}' --type tsx
Length of output: 1556
Script:
#!/bin/bash
# Let's see the implementation of conversation-message-layout.tsx to understand the hierarchy
rg -B 10 -A 10 "MessageContainer" features/conversation/conversation-message/conversation-message-layout.tsx
# And let's check if MessageContextStoreProvider wraps this layout
rg -B 5 -A 5 "MessageContextStore" features/conversation/conversation-message/conversation-message-layout.tsx
Length of output: 3059
Script:
#!/bin/bash
# Let's check where ConversationMessageLayout is being used to find the provider
rg "ConversationMessageLayout" --type-add 'tsx:*.{tsx,ts}' --type tsx -B 5 -A 5
# And let's look at the full implementation of the store context file
cat features/conversation/conversation-message.store-context.tsx
Length of output: 10530
features/conversation/conversation-messages-list.tsx (1)
38-38: Consider cross-platform scroll indicator behavior
The comment indicates a size glitch on Android, but hiding the scroll indicator might impact usability. Consider:
- Documenting the specific Android glitch
- Finding an alternative solution for Android
Let's check if this is a known issue:
✅ Verification successful
Scroll indicator issue appears to be isolated
The codebase search reveals this is the only instance where scroll indicators are specifically disabled for Android due to a size glitch. While there are numerous Platform.OS checks for iOS-specific styling, no other scroll indicator issues are documented. Since this is an isolated case and the comment clearly documents the reason (size glitch), the current implementation appears to be an intentional workaround for a specific Android UI issue.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar Platform.OS checks
rg "Platform\.OS.*===.*('ios'|\"ios\")" --type typescript
Length of output: 98
Script:
#!/bin/bash
# Search for Platform.OS checks with proper file type
rg "Platform\.OS.*===.*('ios'|\"ios\")"
# Also search for any mentions of scroll indicators and Android
rg -i "scroll.*indicator.*android|android.*scroll.*indicator"
Length of output: 7284
features/conversation/conversation-message/conversation-message.tsx (1)
38-44: Consider adding loading states for attachments
The MessageRemoteAttachment and MessageStaticAttachment components likely involve async operations, but there's no visible loading state handling.
Let's check how attachments are handled:
features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.utils.tsx (1)
23-29: Verify missing imports for message context menu actions.
The function uses setCurrentConversationReplyToMessageId and resetMessageContextMenuData but their imports are not visible in the import section.
| if (!store) | ||
| throw new Error(`Missing ConnectViaWalletStore.Provider in the tree`); |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent error message formats found across store providers
The search results reveal inconsistency in error message formats across store providers:
- Some use double quotes:
"Missing ConversationStore.Provider in the tree" - Others use template literals:
`Missing ConversationStore.Provider in the tree` - Same stores have duplicate implementations with different formats
Key files needing standardization:
features/conversation/conversation.store-context.tsxfeatures/conversation/conversation-message.store-context.tsxfeatures/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.store.tsxfeatures/onboarding/Privy/privyAuthStore.tsxfeatures/conversation/conversation-composer/conversation-composer.store-context.tsx
🔗 Analysis chain
LGTM! Consider standardizing error messages across similar hooks.
The improved error message provides better context for debugging. The consistent use of template literals aligns with modern JavaScript practices.
Let's check if similar hooks in the codebase could benefit from standardized error messages:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find similar store hooks that might need consistent error messaging
# Look for store hooks that might throw errors without descriptive messages
# Search for store hooks with basic error throws
rg "throw new Error\(\);" --type ts --type tsx
# Search for store hooks with missing provider errors
rg "Missing.*Provider" --type ts --type tsx
Length of output: 150
Script:
#!/bin/bash
# Let's try without file type filters since tsx is not recognized
# Search for store hooks with basic error throws
rg "throw new Error"
# Search for store hooks with missing provider errors
rg "Missing.*Provider"
Length of output: 9637
| import { createStore, useStore } from "zustand"; | ||
| import { subscribeWithSelector } from "zustand/middleware"; | ||
|
|
||
| type IMessageContextMenuStoreProps = {}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace empty object type with explicit interface.
The empty object type {} should be replaced with an explicit interface, even if empty, to maintain clarity and follow TypeScript best practices.
-type IMessageContextMenuStoreProps = {};
+interface IMessageContextMenuStoreProps {
+ // Add props here as needed
+}📝 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.
| type IMessageContextMenuStoreProps = {}; | |
| interface IMessageContextMenuStoreProps { | |
| // Add props here as needed | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
| const { hasNextMessageInSeries, fromMe } = useMessageContextStoreContext( | ||
| useSelect(["hasNextMessageInSeries", "fromMe"]) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize state selection and document context requirements
The component relies on context values but lacks documentation about required providers.
- Memoize the state selection:
+ const stateSelector = React.useMemo(
+ () => ["hasNextMessageInSeries", "fromMe"] as const,
+ []
+ );
const { hasNextMessageInSeries, fromMe } = useMessageContextStoreContext(
- useSelect(["hasNextMessageInSeries", "fromMe"])
+ useSelect(stateSelector)
);- Add JSDoc comment to document context requirements:
/**
* Renders a simple text message in a conversation.
* @requires MessageContextStoreProvider - Must be wrapped in a MessageContextStoreProvider
*/
export const MessageSimpleText = memo(...)| const { message } = props; | ||
|
|
||
| const textContent = message.content(); |
There was a problem hiding this comment.
Add error handling and content validation
The message content extraction lacks error handling and validation. Consider:
- Adding null/undefined checks
- Implementing content sanitization for security
- Adding error boundaries for runtime errors
- const textContent = message.content();
+ const textContent = React.useMemo(() => {
+ try {
+ const content = message.content();
+ return content ?? '';
+ } catch (error) {
+ console.error('Failed to extract message content:', error);
+ return '';
+ }
+ }, [message]);📝 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 { message } = props; | |
| const textContent = message.content(); | |
| const { message } = props; | |
| const textContent = React.useMemo(() => { | |
| try { | |
| const content = message.content(); | |
| return content ?? ''; | |
| } catch (error) { | |
| console.error('Failed to extract message content:', error); | |
| return ''; | |
| } | |
| }, [message]); |
| return ( | ||
| <BubbleContainer fromMe={fromMe}> | ||
| <BubbleContentContainer | ||
| fromMe={fromMe} | ||
| hasNextMessageInSeries={hasNextMessageInSeries} | ||
| > | ||
| <MessageText inverted={fromMe}>{textContent}</MessageText> | ||
| </BubbleContentContainer> | ||
| </BubbleContainer> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and user experience
The message component should be more accessible to screen readers and keyboard navigation.
return (
- <BubbleContainer fromMe={fromMe}>
+ <BubbleContainer
+ fromMe={fromMe}
+ role="article"
+ aria-label={`Message ${fromMe ? 'sent' : 'received'}`}
+ >
<BubbleContentContainer
fromMe={fromMe}
hasNextMessageInSeries={hasNextMessageInSeries}
>
- <MessageText inverted={fromMe}>{textContent}</MessageText>
+ <MessageText
+ inverted={fromMe}
+ aria-live="polite"
+ >
+ {textContent}
+ </MessageText>
</BubbleContentContainer>
</BubbleContainer>
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <BubbleContainer fromMe={fromMe}> | |
| <BubbleContentContainer | |
| fromMe={fromMe} | |
| hasNextMessageInSeries={hasNextMessageInSeries} | |
| > | |
| <MessageText inverted={fromMe}>{textContent}</MessageText> | |
| </BubbleContentContainer> | |
| </BubbleContainer> | |
| ); | |
| return ( | |
| <BubbleContainer | |
| fromMe={fromMe} | |
| role="article" | |
| aria-label={`Message ${fromMe ? 'sent' : 'received'}`} | |
| > | |
| <BubbleContentContainer | |
| fromMe={fromMe} | |
| hasNextMessageInSeries={hasNextMessageInSeries} | |
| > | |
| <MessageText | |
| inverted={fromMe} | |
| aria-live="polite" | |
| > | |
| {textContent} | |
| </MessageText> | |
| </BubbleContentContainer> | |
| </BubbleContainer> | |
| ); |
| @@ -1,10 +1,10 @@ | |||
| import { createBottomSheetModalRef } from "@design-system/BottomSheet/BottomSheet.utils"; | |||
There was a problem hiding this comment.
Fix the double dot in the filename.
The filename contains a double dot: conversation-message-reaction-drawer.service..ts
Rename the file to remove the extra dot:
- conversation-message-reaction-drawer.service..ts
+ conversation-message-reaction-drawer.service.ts| // TODO: Move else where once we refactored the accounts logics | ||
| await prefetchCurrentUserAccountInboxId(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider performance impact of synchronous prefetch during hydration.
The addition of prefetchCurrentUserAccountInboxId as a blocking operation during hydration could delay app startup. Consider making this operation non-blocking or moving it to a later stage in the app lifecycle.
Consider this alternative approach:
- // TODO: Move else where once we refactored the accounts logics
- await prefetchCurrentUserAccountInboxId();
+ // Start prefetch but don't block hydration
+ prefetchCurrentUserAccountInboxId().catch(error =>
+ logger.error('[Hydration] Failed to prefetch inbox ID:', error)
+ );Committable suggestion skipped: line range outside the PR's diff.
| // @ts-ignore | ||
| <Animated.FlatList |
There was a problem hiding this comment.
💡 Codebase verification
Remove @ts-ignore as it's masking a potential type issue with Animated.FlatList
The component is using react-native-reanimated for the Animated.FlatList, but the type issue is being suppressed. Since this is the only usage of Animated.FlatList in the codebase and the proper imports are in place (including AnimatedProps), the type error should be addressed by:
- Using proper typing with AnimatedProps from react-native-reanimated
- Ensuring FlatList props are correctly typed with the message data structure
🔗 Analysis chain
Remove @ts-ignore and fix type issues properly
The @ts-ignore comment suppresses TypeScript errors which could hide potential issues. Consider fixing the underlying type issue instead.
Let's check if there are similar patterns in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for @ts-ignore usage
rg "@ts-ignore" --type typescript
Length of output: 68
Script:
#!/bin/bash
# Search for @ts-ignore usage with ripgrep, trying different file extensions
rg "@ts-ignore" -g "*.ts" -g "*.tsx"
# Also check if Animated.FlatList is used elsewhere
rg "Animated\.FlatList" -g "*.ts" -g "*.tsx" -A 2
Length of output: 678
Script:
#!/bin/bash
# Let's check the context around the @ts-ignore in conversation-messages-list.tsx
rg "@ts-ignore" features/conversation/conversation-messages-list.tsx -B 2 -A 2
# Also check the imports to see if Animated is properly imported
rg "import.*Animated" features/conversation/conversation-messages-list.tsx
Length of output: 290
| type IProps = { | ||
| messagesCount: number; | ||
| conversation: DmWithCodecsType | undefined | null; | ||
| isBlockedPeer: boolean; | ||
| recommendationData: any; // TODO: Add proper type | ||
| conversationName: (conversation: DmWithCodecsType) => string; | ||
| sendMessage: (args: { | ||
| conversation: DmWithCodecsType; | ||
| content: string; | ||
| contentType: string; | ||
| }) => void; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define proper type for recommendationData.
The TODO comment indicates missing proper typing for recommendationData.
Consider defining a proper interface for the recommendation data:
interface RecommendationData {
// Add appropriate fields based on the actual data structure
// Example:
id: string;
type: string;
content: string;
// ... other fields
}
type IProps = {
// ... other props
recommendationData: RecommendationData;
// ... remaining props
};| import { VStack } from "@/design-system/VStack"; | ||
| import { TouchableWithoutFeedback } from "@/design-system/touchable-without-feedback"; | ||
| import { ActivityIndicator } from "@components/ActivityIndicator"; | ||
| import { Recommendation } from "@components/Recommendations/Recommendation"; | ||
| import { showActionSheetWithOptions } from "@components/StateHandlers/ActionSheetStateHandler"; | ||
| import { currentAccount, useProfilesStore } from "@data/store/accountsStore"; | ||
| import { Button } from "@design-system/Button/Button"; | ||
| import { useConversationContext } from "@features/conversation/conversation-context"; | ||
| import { translate } from "@i18n"; | ||
| import { actionSheetColors, textPrimaryColor } from "@styles/colors"; | ||
| import { isV3Topic } from "@utils/groupUtils/groupId"; | ||
| import { getProfile, getProfileData } from "@utils/profile"; | ||
| import { DmWithCodecsType } from "@utils/xmtpRN/client"; | ||
| import { consentToAddressesOnProtocolByAccount } from "@utils/xmtpRN/contacts"; | ||
| import { Keyboard, Platform, useColorScheme } from "react-native"; | ||
|
|
There was a problem hiding this comment.
Add missing Text import from React Native.
The Text component is used but not imported.
+import { Text, Keyboard, Platform, useColorScheme } from "react-native";
-import { Keyboard, Platform, useColorScheme } from "react-native";📝 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.
| import { VStack } from "@/design-system/VStack"; | |
| import { TouchableWithoutFeedback } from "@/design-system/touchable-without-feedback"; | |
| import { ActivityIndicator } from "@components/ActivityIndicator"; | |
| import { Recommendation } from "@components/Recommendations/Recommendation"; | |
| import { showActionSheetWithOptions } from "@components/StateHandlers/ActionSheetStateHandler"; | |
| import { currentAccount, useProfilesStore } from "@data/store/accountsStore"; | |
| import { Button } from "@design-system/Button/Button"; | |
| import { useConversationContext } from "@features/conversation/conversation-context"; | |
| import { translate } from "@i18n"; | |
| import { actionSheetColors, textPrimaryColor } from "@styles/colors"; | |
| import { isV3Topic } from "@utils/groupUtils/groupId"; | |
| import { getProfile, getProfileData } from "@utils/profile"; | |
| import { DmWithCodecsType } from "@utils/xmtpRN/client"; | |
| import { consentToAddressesOnProtocolByAccount } from "@utils/xmtpRN/contacts"; | |
| import { Keyboard, Platform, useColorScheme } from "react-native"; | |
| import { VStack } from "@/design-system/VStack"; | |
| import { TouchableWithoutFeedback } from "@/design-system/touchable-without-feedback"; | |
| import { ActivityIndicator } from "@components/ActivityIndicator"; | |
| import { Recommendation } from "@components/Recommendations/Recommendation"; | |
| import { showActionSheetWithOptions } from "@components/StateHandlers/ActionSheetStateHandler"; | |
| import { currentAccount, useProfilesStore } from "@data/store/accountsStore"; | |
| import { Button } from "@design-system/Button/Button"; | |
| import { useConversationContext } from "@features/conversation/conversation-context"; | |
| import { translate } from "@i18n"; | |
| import { actionSheetColors, textPrimaryColor } from "@styles/colors"; | |
| import { isV3Topic } from "@utils/groupUtils/groupId"; | |
| import { getProfile, getProfileData } from "@utils/profile"; | |
| import { DmWithCodecsType } from "@utils/xmtpRN/client"; | |
| import { consentToAddressesOnProtocolByAccount } from "@utils/xmtpRN/contacts"; | |
| import { Text, Keyboard, Platform, useColorScheme } from "react-native"; |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (52)
features/conversation/DmConversationTitle.tsx (2)
2-2: Consider standardizing import path conventionsThe file mixes relative paths (
./hooks/useConversationTitleLongPress) with absolute paths (@/features/conversation/conversation-title). Consider standardizing to absolute paths for better maintainability and consistency.Also applies to: 16-16
Line range hint
28-40: Well-structured component with clean separation of concernsThe implementation demonstrates good practices:
- Custom hook (useUserInteraction) for handling user interactions
- Clear type definitions
- Proper separation of concerns
utils/date.ts (2)
Line range hint
2-4: Address the locale import optimization commentThe comment "Long term we should only import used locales" indicates a known optimization opportunity.
Would you like me to help create a GitHub issue to track the locale import optimization? I can help generate a solution that:
- Analyzes actual locale usage
- Implements dynamic imports for locales
- Reduces the bundle size
Line range hint
47-53: Document timestamp conversion strategyWith the addition of
getTodayNs(), the codebase now has two functions handling timestamp conversions in opposite directions. Consider adding documentation to explain the timestamp strategy.Add JSDoc comments to clarify the timestamp handling approach:
+ /** + * Converts a timestamp from nanoseconds to milliseconds if needed. + * @param timestamp - The timestamp to normalize + * @returns The timestamp in milliseconds + */ export function normalizeTimestamp(timestamp: number) { // If the timestamp has more than 13 digits, assume it's in nanoseconds if (timestamp > 1e13) { return Math.floor(timestamp / 1e6); // Convert nanoseconds to milliseconds } return timestamp; // Already in milliseconds } + /** + * Gets the current timestamp in nanoseconds. + * @returns The current timestamp in nanoseconds + * @see normalizeTimestamp for conversion back to milliseconds + */ export function getTodayNs(): number { return Date.now() * 1000000; }Also applies to: 101-103
components/ConversationContextMenu.tsx (2)
Line range hint
31-89: Consider architectural improvements for better maintainabilityThe gesture and animation logic is quite complex. Consider these improvements:
- Extract gesture handling to a custom hook:
const useContextMenuGestures = (height: number, closeMenu: () => void) => { return Gesture.Simultaneous( Gesture.Pan() .onUpdate((event) => { translateY.value = Math.max(0, event.translationY); }) .onEnd((event) => { if (event.velocityY > 500 || event.translationY > height * 0.2) { runOnJS(closeMenu)(); } else { translateY.value = withTiming(0); } }), Gesture.Tap().onEnd(() => { runOnJS(closeMenu)(); }) ); };
- Consolidate animation values into a single shared value object:
const animationState = useSharedValue({ active: false, opacity: 0, intensity: 0, translateY: height });
- Extract magic numbers in styles to constants:
const MENU_CONSTANTS = { MIN_PREVIEW_HEIGHT: 210, MIN_MENU_HEIGHT: 300, MENU_WIDTH: 180, BORDER_RADIUS: 16 } as const;
Line range hint
90-157: Add accessibility support and error handlingThe component handles complex user interactions but lacks accessibility support and proper error handling.
- Add accessibility props to interactive elements:
<GestureDetector gesture={gesture}> <AnimatedBlurView + accessible={true} + accessibilityRole="button" + accessibilityLabel="Close menu" + accessibilityHint="Double tap to close the conversation menu" tint="default" style={[styles.flex, Platform.OS === "android" ? backDropContainerStyle : undefined]} animatedProps={animatedContainerProps} >
- Add error boundary:
const handleAnimationError = (error: Error) => { console.error('Animation failed:', error); resetConversationListContextMenuStore(); }; // Use in animation translateY.value = withTiming( height, { duration: animation.contextMenuHoldDuration }, (finished) => { if (finished) { runOnJS(resetConversationListContextMenuStore)(); } else { runOnJS(handleAnimationError)(new Error('Animation cancelled')); } } );features/conversation-requests-list/useV3RequestItems.tsx (3)
Line range hint
53-54: Address TODO comment regarding default spam scoreThe current implementation uses a default spam score of 0 when content type is unavailable. This might lead to false negatives in spam detection.
Would you like me to help implement a more robust fallback mechanism for unknown content types?
Line range hint
55-61: Consider extracting spam threshold configurationThe spam score threshold (> 1) is hardcoded. Consider extracting this to a configuration constant or environment variable for easier maintenance and adjustment.
+const SPAM_SCORE_THRESHOLD = 1; // Consider moving to configuration file if (spamScore > 1) { - likelySpam.push(conversation); + likelySpam.push(conversation); } else { likelyNotSpam.push(conversation); }
Line range hint
34-42: Enhance error handling for content extractionThe current error handling silently falls back to default values. Consider logging these errors for debugging purposes.
try { const decodedContent = lastMessage?.content() ?? lastMessage?.fallback ?? ""; if (typeof decodedContent === "string") { content = decodedContent; } else { content = lastMessage?.fallback ?? ""; } -} catch { +} catch (error) { + console.error('Failed to decode message content:', error); content = lastMessage?.fallback ?? ""; }features/conversation/utils/messageIsFromCurrentUser.ts (1)
Line range hint
8-24: Consider consolidating duplicate message ownership checksThere are two similar functions (
messageIsFromCurrentUserandmessageIsFromCurrentUserV3) that serve the same purpose but use different implementations. Consider consolidating these into a single function to improve maintainability.- export const messageIsFromCurrentUser = ({ - message, - }: MessageFromCurrentUserPayload) => { - if (!message) return false; - const currentAccount = getCurrentAccount(); - if (!currentAccount) return false; - return message.senderAddress.toLowerCase() === currentAccount.toLowerCase(); - }; - - export function messageIsFromCurrentUserV3({ - message, - }: MessageFromCurrentUserPayload) { - return ( - message?.senderAddress.toLowerCase() === getCurrentUserAccountInboxId() - ); - } + export const messageIsFromCurrentUser = ({ + message, + }: MessageFromCurrentUserPayload): boolean => { + if (!message?.senderAddress) return false; + const currentInboxId = getCurrentUserAccountInboxId(); + return message.senderAddress.toLowerCase() === currentInboxId; + };features/notifications/utils/background/notificationSpamScore.ts (3)
Line range hint
11-63: Inconsistent error handling in spam score computationThe
computeSpamScoreGroupWelcomefunction returns 0 on error, whilecomputeSpamScoreGroupMessagesilently ignores errors. Consider standardizing the error handling approach.- } catch (error) { - return 0; - } + } catch (error) { + console.error('Error computing welcome spam score:', error); + return 0; + }
Line range hint
65-134: Extract duplicate address consent checking logicBoth functions contain similar code for checking address consent states. Consider extracting this into a shared utility function.
async function checkAddressConsent( addresses: string[], xmtpClient: ConverseXmtpClientType ): Promise<number> { for (const address of addresses) { const addressConsentState = await xmtpClient.preferences.addressConsentState(address); if (addressConsentState === "denied") return 1; } for (const address of addresses) { const addressConsentState = await xmtpClient.preferences.addressConsentState(address); if (addressConsentState === "allowed") return -1; } return 0; }This would simplify both functions and reduce code duplication.
Line range hint
65-134: Consider simplifying nested conditionalsThe function contains multiple nested conditionals that could be simplified using early returns or guard clauses.
For example:
export const computeSpamScoreGroupMessage = async ( xmtpClient: ConverseXmtpClientType, group: GroupWithCodecsType, decodedMessage: DecodedMessageWithCodecsType ): Promise<number> => { try { await xmtpClient.preferences.syncConsent(); // Early returns for consent states const groupConsentState = await xmtpClient.preferences.conversationConsentState(group.id); if (groupConsentState === "denied") return 1; if (groupConsentState === "allowed") return -1; const senderInboxId = decodedMessage.senderAddress as InboxId; const senderConsentState = await xmtpClient.preferences.inboxIdConsentState(senderInboxId); if (senderConsentState === "denied") return 1; if (senderConsentState === "allowed") return -1; // Rest of the function... } catch (error) { console.error('Error computing message spam score:', error); return 0; } };theme/animations.ts (1)
112-139: Consider these improvements to the new animation functionWhile the implementation is solid, here are some suggestions to enhance maintainability and type safety:
+ const FINAL_OPACITY = 1; + const FINAL_SCALE = 1; + const INITIAL_OPACITY = 0; + const INITIAL_SCALE = 0; + + type AnimationResult = { + initialValues: { + opacity: number; + transform: Array<{ scale: number }>; + }; + animations: { + opacity: number; + transform: Array<{ scale: number }>; + }; + }; + - reanimatedFadeInScaleIn: () => { + reanimatedFadeInScaleIn: (): AnimationResult => { "worklet"; + const springConfig = { + damping: SICK_DAMPING, + stiffness: SICK_STIFFNESS, + }; + const animations = { - opacity: withSpring(1, { - damping: SICK_DAMPING, - stiffness: SICK_STIFFNESS, - }), + opacity: withSpring(FINAL_OPACITY, springConfig), transform: [ { - scale: withSpring(1, { - damping: SICK_DAMPING, - stiffness: SICK_STIFFNESS, - }), + scale: withSpring(FINAL_SCALE, springConfig), }, ], }; const initialValues = { - opacity: 0, - transform: [{ scale: 0 }], + opacity: INITIAL_OPACITY, + transform: [{ scale: INITIAL_SCALE }], };These changes:
- Add TypeScript types for better type safety
- Extract magic numbers into named constants
- Reduce duplication by sharing spring configuration
features/conversation/conversation-message/conversation-message-layout.tsx (4)
10-12: Consider extending the props interface for better flexibility.The current props interface only accepts
children. Consider adding optional props for customization:
className?: string- For custom styling overridestestId?: string- For testing purposescustomAvatar?: ReactNode- For custom avatar renderingtype IConversationMessageLayoutProps = { children: ReactNode; + className?: string; + testId?: string; + customAvatar?: ReactNode; };
20-22: Remove unused field from useSelect.The
messageIdfield is selected but never used in the component.const { senderAddress, fromMe } = useMessageContextStoreContext( - useSelect(["senderAddress", "fromMe", "messageId"]) + useSelect(["senderAddress", "fromMe"]) );
30-30: Consider making the avatar spacing configurable.The spacing between avatar and message content is hardcoded to
theme.spacing.xxs. Consider making this configurable through props for better flexibility.+type IConversationMessageLayoutProps = { + children: ReactNode; + avatarSpacing?: keyof typeof theme.spacing; +}; export const ConversationMessageLayout = memo( function ConversationMessageLayout({ children, + avatarSpacing = "xxs", }: IConversationMessageLayoutProps) { // ... - <VStack style={{ width: theme.spacing.xxs }} /> + <VStack style={{ width: theme.spacing[avatarSpacing] }} />
14-44: Consider splitting the layout component for better separation of concerns.The component currently handles both sent and received message layouts. Consider splitting it into separate components:
SentMessageLayoutReceivedMessageLayoutThis would:
- Make the code more maintainable
- Allow for easier customization of different message types
- Follow the Single Responsibility Principle
screens/Profile.tsx (2)
Line range hint
770-785: Add error handling for notification permission requestThe notification permission request lacks error handling for potential promise rejections. Consider adding a try-catch block and handling the error appropriately.
- requestPushNotificationsPermissions().then( - (newStatus: NotificationPermissionStatus | undefined) => { - if (!newStatus) return; - setNotificationsPermissionStatus(newStatus); - } - ); + try { + const newStatus = await requestPushNotificationsPermissions(); + if (newStatus) { + setNotificationsPermissionStatus(newStatus); + } + } catch (error) { + console.error('Failed to request notification permissions:', error); + // Handle the error appropriately + }
Line range hint
115-900: Consider breaking down the ProfileScreenImpl componentThe
ProfileScreenImplcomponent is quite large and handles multiple responsibilities. Consider breaking it down into smaller, more focused components for better maintainability and readability:
- Extract profile header (avatar, title, error message)
- Extract action items generation logic
- Extract social items section
- Extract settings section for the user's own profile
This would improve code organization and make it easier to test individual pieces of functionality.
features/conversation-list/hooks/useToggleReadStatus.ts (1)
Line range hint
13-35: Consider adding error handling and race condition protectionThe implementation looks good but could benefit from some improvements:
- Add error handling around
saveTopicsDatato handle potential storage failures- Consider debouncing or using optimistic updates to handle rapid toggles
- Use a single timestamp for consistency
Here's a suggested improvement:
export const useToggleReadStatus = ({ topic, isUnread, currentAccount, }: UseToggleReadStatusProps) => { const { setTopicsData } = useChatStore(useSelect(["setTopicsData"])); return useCallback(() => { const newStatus = isUnread ? "read" : "unread"; const timestamp = new Date().getTime(); + const topicData = { + [topic]: { + status: newStatus, + readUntil: isUnread ? timestamp : 0, + timestamp, + }, + }; setTopicsData({ - [topic]: { - status: newStatus, - readUntil: isUnread ? timestamp : 0, - timestamp, - }, + ...topicData }); - saveTopicsData(currentAccount, { - [topic]: { - status: newStatus, - readUntil: isUnread ? timestamp : 0, - timestamp, - }, - }); + try { + await saveTopicsData(currentAccount, topicData); + } catch (error) { + console.error('Failed to save topic data:', error); + // Optionally revert the local state + setTopicsData({ + [topic]: { + status: isUnread ? "unread" : "read", + readUntil: isUnread ? 0 : timestamp, + timestamp, + }, + }); + } }, [setTopicsData, topic, isUnread, currentAccount]); };features/conversation/conversation-message-timestamp.tsx (2)
30-30: Remove unnecessary commented-out codeThe commented-out line can be removed to maintain code cleanliness.
Apply this diff:
- // const { showTimeAV } = useMessageContext();
94-94: Remove unnecessary commented-out codeRemoving the commented-out code improves readability.
Apply this diff:
- // ...debugBorder("yellow"),features/conversation/conversation-message.store-context.tsx (3)
48-52: OptimizeuseEffectdependency array to prevent unnecessary store updatesThe
useEffecthook currently depends on the entirepropsobject. Sincepropsis an object and may change reference on each render, this can cause the effect to run more often than necessary, leading to unnecessary calls tostoreRef.current?.setState. Consider specifying the specific properties ofpropsthat should trigger the effect or memoizingpropsto prevent unnecessary updates.Apply this diff to optimize the dependency array:
useEffect(() => { storeRef.current?.setState({ ...getStoreStateBasedOnProps(props), }); -}, [props]); +}, [props.message, props.previousMessage, props.nextMessage]);
109-110: Ensure consistent use of quotes in error messagesThe error message in
useMessageContextStoreuses backticks, while the one inuseMessageContextStoreContextuses double quotes. For consistency and readability, consider using the same quote style in both error messages.Apply this diff to unify the quote style:
if (!store) - throw new Error(`Missing MessageContextStore.Provider in the tree`); + throw new Error("Missing MessageContextStore.Provider in the tree");
39-60: Re-evaluate the use ofReact.memoonMessageContextStoreProviderWrapping
MessageContextStoreProviderwithmemomay not be necessary since the component's state updates wheneverpropschange via theuseEffecthook. This could potentially lead to unexpected behavior if props are updated but the component does not re-render as intended. Consider removingmemoto ensure proper updates.Apply this diff to remove
memo:-export const MessageContextStoreProvider = memo( +export const MessageContextStoreProvider = ( ({ children, ...props }: IMessageContextStoreProviderProps) => { const storeRef = useRef<IMessageContextStore>();features/conversation/conversation-message/conversation-message-repliable.tsx (2)
12-12: Correct the spelling of 'threshold'The word 'threshold' is misspelled as 'treshold' in comments and variable names.
Apply the following changes:
Line 12:
-// TODO: When we'll use ReanimatedSwipeable, we'll be able to listen to progress and trigger haptic once the treshold to reply is reached +// TODO: When we'll use ReanimatedSwipeable, we'll be able to listen to progress and trigger haptic once the threshold to reply is reachedLine 25:
-const xTresholdToReply = theme.spacing["3xl"]; +const xThresholdToReply = theme.spacing["3xl"];Line 77:
-if (translation && v > xTresholdToReply) { +if (translation && v > xThresholdToReply) {Also applies to: 25-25, 77-77
8-13: Offer assistance with planned enhancementsThere are TODO comments indicating plans to switch to
ReanimatedSwipeableand optimize performance after upgrading to Expo SDK 52.Would you like me to help prepare these changes or open GitHub issues to track these future enhancements?
features/conversation/conversation.screen.tsx (1)
24-31: Provide a User-Friendly Message When No Peer or Topic Is FoundInstead of rendering an empty screen when neither
peernortopicis provided, consider displaying a user-friendly message to inform the user.Apply the following diff to enhance the user experience:
captureError(new Error("No peer or topic found in ConversationScreen")); return ( <Screen contentContainerStyle={{ flex: 1 }}> - <VStack /> + <Center style={{ flex: 1 }}> + <Text>No conversation available.</Text> + </Center> </Screen> );Ensure to import the
CenterandTextcomponents:+ import { Center } from "@/design-system/Center"; + import { Text } from "@/design-system/Text";features/conversation/hooks/use-remove-reaction-on-message.ts (1)
20-85: Consider Refactoring to Avoid Code DuplicationThe
useRemoveReactionOnMessagehook shares similar logic withuseReactOnMessage. Consider abstracting the common functionality into a shared helper function or higher-order hook to reduce code duplication and improve maintainability.features/conversation/conversation.tsx (1)
71-84: Simplify Redundant Conditional Logic inuseEffectHookIn the
useEffecthook, both conditions forisConversationDmandisConversationGroupexecute the same code:if (isConversationDm(conversation)) { navigation.setOptions({ headerTitle: () => <GroupConversationTitle topic={topic} />, }); } else if (isConversationGroup(conversation)) { navigation.setOptions({ headerTitle: () => <GroupConversationTitle topic={topic} />, }); }Since both conditions result in the same outcome, consider simplifying the logic to remove redundancy.
Apply this diff to simplify the code:
useEffect(() => { if (!conversation) { return; } - if (isConversationDm(conversation)) { - navigation.setOptions({ - headerTitle: () => <GroupConversationTitle topic={topic} />, - }); - } else if (isConversationGroup(conversation)) { - navigation.setOptions({ - headerTitle: () => <GroupConversationTitle topic={topic} />, - }); - } + navigation.setOptions({ + headerTitle: () => <GroupConversationTitle topic={topic} />, + }); }, [topic, navigation, conversation]);features/conversation/utils/isConversationDm.ts (1)
4-6: Add Type Guard Return Type for Better Type InferenceConsider specifying the return type as a type guard to improve TypeScript type inference when using this function. This enhances code readability and provides better type safety.
Apply this diff to specify the return type:
-export function isConversationDm(conversation: ConversationWithCodecsType) { +export function isConversationDm( + conversation: ConversationWithCodecsType +): conversation is ConversationWithCodecsType & { version: ConversationVersion.DM } { return conversation.version === ConversationVersion.DM; }features/conversation/utils/isConversationGroup.ts (1)
4-6: Add Type Guard Return Type for Better Type InferenceSimilar to
isConversationDm, specify the return type as a type guard to improve TypeScript type inference when using this function.Apply this diff to specify the return type:
-export function isConversationGroup(conversation: ConversationWithCodecsType) { +export function isConversationGroup( + conversation: ConversationWithCodecsType +): conversation is ConversationWithCodecsType & { version: ConversationVersion.GROUP } { return conversation.version === ConversationVersion.GROUP; }features/conversation/conversation-message/conversation-message-container.tsx (2)
12-12: Remove Unused Destructured VariablechildrenThe
childrenvariable is already accessed fromprops. Destructuring it again inside the function is unnecessary.Apply this diff to remove the redundant destructuring:
- const { children } = props;
Line range hint
16-24: Simplify Alignment Logic in Style ObjectThe alignment logic in the
styleprop can be simplified by directly setting thejustifyContentproperty based onfromMe.Apply this diff to simplify the style object:
... style={{ width: "100%", alignItems: "flex-end", - ...(fromMe - ? { justifyContent: "flex-end" } - : { justifyContent: "flex-start" }), + justifyContent: fromMe ? "flex-end" : "flex-start", }} ...features/conversation/hooks/use-send-message.ts (1)
5-14: Consider adding metadata to message content typeThe current type definition could be enhanced to support additional message metadata that might be needed in the future (e.g., message priority, expiration).
export type MessageContent = { text?: string; remoteAttachment?: RemoteAttachmentContent; metadata?: Record<string, unknown>; }; export type ISendMessageParams = { content: MessageContent; referencedMessageId?: MessageId; } & ( | { content: { text: string } & Partial<MessageContent> } | { content: { remoteAttachment: RemoteAttachmentContent } & Partial<MessageContent> } );features/conversation/conversation.nav.tsx (1)
11-19: Consider adding validation for topic parameterThe screen configuration handles encoding/decoding but lacks validation for the topic parameter.
export const ConversationScreenConfig = { path: "/conversation", parse: { - topic: decodeURIComponent, + topic: (value: string) => { + const decoded = decodeURIComponent(value); + // Add validation logic here + return decoded; + }, }, stringify: { topic: encodeURIComponent, }, };features/conversation/NewConversationTitle.tsx (2)
Line range hint
59-67: Improve loading state and error handlingThe current implementation returns null during loading, which might cause layout shifts. Consider showing a skeleton loader instead.
- if (!displayAvatar) return null; + if (!displayAvatar) { + return ( + <ConversationTitle + title={<SkeletonLoader width={100} />} + avatarComponent={<SkeletonLoader circle size={AvatarSizes.conversationTitle} />} + /> + ); + }
Line range hint
33-46: Optimize data fetching with combined hookThe component uses multiple hooks for fetching user data, which could lead to multiple network requests.
Consider combining the data fetching hooks into a single hook to optimize performance:
const useUserData = (address: string) => { const { isLoading, error } = useProfileSocials(address); const preferredName = usePreferredName(address); const preferredAvatarUri = usePreferredAvatarUri(address); return { isLoading, error, userData: { name: preferredName, avatarUri: preferredAvatarUri } }; };features/conversation/conversation-message/conversation-message.tsx (3)
67-67: Remove unused variable_ensureNeverThe variable
_ensureNeveris declared but never used. It appears to be intended for exhaustive type checking but isn't serving that purpose currently.- const _ensureNever = message;
52-65: Track TODOs for unimplemented message typesThere are several message types (ReadReceipt, TransactionReference, CoinbasePayment) that currently return null with TODO comments. These should be tracked and implemented.
Would you like me to create GitHub issues to track the implementation of these message types? I can help create detailed issues with acceptance criteria for each type.
69-71: Consider enhancing error capture with message detailsWhile error capturing is implemented, it could be more informative by including additional message details in the error context.
- captureError(new Error(`Unknown message type ${message.contentTypeId}`)); + captureError( + new Error(`Unknown message type ${message.contentTypeId}`), + { + extra: { + messageId: message.id, + contentType: message.contentTypeId, + sender: message.senderAddress, + }, + } + );features/conversation/conversation-new-dm.tsx (3)
45-45: Track empty state implementationThe TODO comment for adding an empty state should be tracked.
Would you like me to create a GitHub issue to track the implementation of the empty state UI component?
94-104: Consider implementing optimistic updatesThere are two commented-out sections for optimistic updates that could improve the UX by making the app feel more responsive.
Consider implementing these optimistic updates to enhance the user experience. The implementation looks well-thought-out but needs proper error handling for rollback scenarios.
Also applies to: 147-160
178-181: Enhance error handlingThe error handling could be more specific about what failed (conversation creation or message sending).
- } catch (error) { - showSnackbar({ message: "Failed to send message" }); - sentryTrackError(error); + } catch (error) { + const errorMessage = + error instanceof Error && error.message.includes("conversation") + ? "Failed to create conversation" + : "Failed to send message"; + showSnackbar({ message: errorMessage }); + sentryTrackError(error, { + extra: { + peerAddress, + hasText: !!text, + hasAttachment: !!remoteAttachment, + }, + });queries/useConversationMessages.ts (2)
53-79: Enhance error handling in processMessages functionThe function should validate the input messages array and handle potential edge cases.
function processMessages(args: { messages: DecodedMessageWithCodecsType[]; existingData?: IMessageAccumulator; prependNewMessages?: boolean; }): IMessageAccumulator { const { messages, existingData, prependNewMessages = false } = args; + if (!Array.isArray(messages)) { + throw new Error('Messages must be an array'); + } const result: IMessageAccumulator = existingData ? { ...existingData } : { ids: [], byId: {}, reactions: {}, };
81-138: Consider optimizing reaction processingThe reaction processing logic could be optimized by:
- Using a Map instead of an object for better lookup performance
- Considering batch processing for large message sets
Consider implementing batch processing if the message volume is high:
const BATCH_SIZE = 100; const processInBatches = (messages: DecodedMessageWithCodecsType[]) => { for (let i = 0; i < messages.length; i += BATCH_SIZE) { const batch = messages.slice(i, i + BATCH_SIZE); // Process batch } };screens/Navigation/Navigation.tsx (1)
50-50: Consider grouping related importsThe ConversationTopic import could be grouped with other conversation-related imports for better organization.
-import { ConversationTopic } from "@xmtp/react-native-sdk"; +import { + ConversationNavParams, + ConversationNav, + ConversationTopic, +} from "@xmtp/react-native-sdk";features/conversation/conversation-message-date-change.tsx (1)
31-39: Refactor subscription logic usinguseDerivedValuefor cleaner codeConsider replacing the
useEffecthook and manual subscription with Reanimated'suseDerivedValueto deriveshowTimeAVdirectly fromisShowingTime. This simplifies the code and improves performance.Apply this diff to refactor the code:
- useEffect(() => { - const unsubscribe = messageStore.subscribe( - (state) => state.isShowingTime, - (isShowingTime) => { - showTimeAV.value = isShowingTime ? 1 : 0; - } - ); - return () => unsubscribe(); - }, [messageStore, showTimeAV]); + const showTimeAV = useDerivedValue(() => { + return messageStore.getState().isShowingTime ? 1 : 0; + });features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx (1)
183-273: Remove commented implementation codeLarge blocks of commented code make the file harder to maintain and understand. If this code represents alternative implementations that might be useful later, consider moving it to a separate documentation file or creating a GitHub issue to track the exploration of different context menu libraries.
features/conversation/conversation-composer/conversation-composer.tsx (2)
318-333: Improve event handling type safety and platform-specific logicThe event handling uses
anytype and mixes platform-specific logic within the component.Consider these improvements:
+type KeyPressEvent = { + nativeEvent: { + key: string; + altKey?: boolean; + metaKey?: boolean; + shiftKey?: boolean; + }; +}; + +const handleKeyPress = (event: KeyPressEvent, onSubmit: () => void) => { + if (Platform.OS !== "web") return; + + const { key, altKey, metaKey, shiftKey } = event.nativeEvent; + + if (key === "Enter" && !altKey && !metaKey && !shiftKey) { + event.preventDefault(); + onSubmit(); + } +}; - onKeyPress={(event: any) => { - if (Platform.OS !== "web") { - return; - } - - if ( - event.nativeEvent.key === "Enter" && - !event.altKey && - !event.metaKey && - !event.shiftKey - ) { - event.preventDefault(); - onSubmitEditing(); - } - }} + onKeyPress={(event) => handleKeyPress(event, onSubmitEditing)}
111-114: Address TODO comment for scroll functionalityThe TODO comment indicates that scroll functionality needs to be fixed with a function in context.
Would you like me to help implement the scroll functionality using the context API or create a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (48)
components/ConversationContextMenu.tsx(1 hunks)data/helpers/conversations/spamScore.ts(1 hunks)features/conversation-list/hooks/useMessageText.ts(1 hunks)features/conversation-list/hooks/useToggleReadStatus.ts(1 hunks)features/conversation-list/useV3ConversationItems.ts(2 hunks)features/conversation-requests-list/useV3RequestItems.tsx(1 hunks)features/conversation/DmConversationTitle.tsx(3 hunks)features/conversation/GroupConversationTitle.tsx(2 hunks)features/conversation/NewConversationTitle.tsx(2 hunks)features/conversation/conversation-composer/conversation-composer.tsx(1 hunks)features/conversation/conversation-message-date-change.tsx(1 hunks)features/conversation/conversation-message-timestamp.tsx(1 hunks)features/conversation/conversation-message.store-context.tsx(1 hunks)features/conversation/conversation-message/conversation-message-container.tsx(1 hunks)features/conversation/conversation-message/conversation-message-content-types/conversation-message-chat-group-update.tsx(2 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-above-message-reactions.tsx(2 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx(1 hunks)features/conversation/conversation-message/conversation-message-layout.tsx(1 hunks)features/conversation/conversation-message/conversation-message-reactions/conversation-message-reactions.tsx(2 hunks)features/conversation/conversation-message/conversation-message-repliable.tsx(1 hunks)features/conversation/conversation-message/conversation-message.tsx(1 hunks)features/conversation/conversation-message/conversation-message.utils.tsx(5 hunks)features/conversation/conversation-messages-list.tsx(1 hunks)features/conversation/conversation-new-dm.tsx(1 hunks)features/conversation/conversation.nav.tsx(1 hunks)features/conversation/conversation.screen.tsx(1 hunks)features/conversation/conversation.tsx(1 hunks)features/conversation/hooks/use-react-on-message.ts(1 hunks)features/conversation/hooks/use-remove-reaction-on-message.ts(1 hunks)features/conversation/hooks/use-send-message.ts(1 hunks)features/conversation/utils/hasNextMessageInSeries.ts(1 hunks)features/conversation/utils/isConversationAllowed.ts(1 hunks)features/conversation/utils/isConversationDm.ts(1 hunks)features/conversation/utils/isConversationGroup.ts(1 hunks)features/conversation/utils/messageIsFromCurrentUser.ts(1 hunks)features/conversations/Messages.types.ts(0 hunks)features/notifications/utils/background/notificationContent.ts(1 hunks)features/notifications/utils/background/notificationSpamScore.ts(1 hunks)queries/useConversationMessages.ts(4 hunks)screens/Conversation.tsx(0 hunks)screens/ConversationList.tsx(2 hunks)screens/ConversationReadOnly.tsx(1 hunks)screens/Main.tsx(1 hunks)screens/Navigation/Navigation.tsx(2 hunks)screens/Profile.tsx(5 hunks)theme/animations.ts(3 hunks)utils/date.ts(1 hunks)utils/xmtpRN/client.ts(1 hunks)
💤 Files with no reviewable changes (2)
- features/conversations/Messages.types.ts
- screens/Conversation.tsx
✅ Files skipped from review due to trivial changes (4)
- utils/xmtpRN/client.ts
- data/helpers/conversations/spamScore.ts
- features/notifications/utils/background/notificationContent.ts
- screens/ConversationList.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- features/conversation-list/useV3ConversationItems.ts
- features/conversation-list/hooks/useMessageText.ts
- features/conversation/conversation-messages-list.tsx
- features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-above-message-reactions.tsx
- features/conversation/conversation-message/conversation-message-content-types/conversation-message-chat-group-update.tsx
- features/conversation/conversation-message/conversation-message-reactions/conversation-message-reactions.tsx
🔇 Additional comments (31)
features/conversation/utils/hasNextMessageInSeries.ts (2)
5-5: LGTM! Type definition accurately reflects nullable nature.
The type definition change properly expresses that nextMessage can be undefined, improving type safety and making the API contract more explicit.
12-13: LGTM! Robust null check implementation.
The guard clause properly handles the now-optional nextMessage parameter, making the function more defensive and maintainable. The implementation is clean and follows a clear logical flow.
features/conversation/DmConversationTitle.tsx (1)
Line range hint 69-83: Verify ConversationTitle component integration
The switch from ConversationTitleDumb to ConversationTitle looks good, but let's verify its usage across the codebase.
✅ Verification successful
Let me gather more information about the ConversationTitle component usage and implementation.
Component change is properly integrated and consistent
The switch from ConversationTitleDumb to ConversationTitle is correctly implemented across the codebase. The component is:
- Used consistently in all conversation title variants (DM, Group, New)
- Maintains the same prop structure (title, avatarComponent, onPress, onLongPress)
- Properly styled and animated with consistent avatar positioning
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of ConversationTitleDumb
rg "ConversationTitleDumb"
# Search for all usages of the new ConversationTitle component
ast-grep --pattern 'import { ConversationTitle } from "@/features/conversation/conversation-title"'
Length of output: 678
Script:
#!/bin/bash
# Check the implementation of ConversationTitle in conversation-title.tsx
cat features/conversation/conversation-title.tsx
# Check the implementation of the files using ConversationTitle
cat features/conversation/NewConversationTitle.tsx
cat features/conversation/GroupConversationTitle.tsx
cat features/conversation/DmConversationTitle.tsx
Length of output: 10193
features/conversation/utils/isConversationAllowed.ts (1)
1-7: Verify the exhaustive handling of conversation states
Let's verify if there are other possible states that need to be handled.
✅ Verification successful
Based on the codebase analysis, I can now generate the final response:
The conversation state handling is correctly implemented
The code correctly handles all possible conversation states from the XMTP SDK. From the codebase, we can see that a conversation can have the following states:
- "allowed": For accepted/consented conversations
- "denied": For blocked conversations
- "unknown": For pending/new conversations
The isConversationAllowed function's implementation is correct as it specifically checks for the "allowed" state, which is the appropriate way to verify if a conversation is permitted.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for conversation state assignments or comparisons
rg -t typescript -t javascript "conversation.*state.*=|state.*===" --no-heading
# Search for state type definitions
ast-grep --pattern 'type $_ = "allowed" | $_'
Length of output: 164
Script:
#!/bin/bash
# Search for conversation state assignments or comparisons with ripgrep
rg "conversation.*state.*=|state.*===" --type-add 'ts:*.{ts,tsx}' --type=ts
# Search for type definitions containing "state" in TypeScript files
rg "type.*state" --type-add 'ts:*.{ts,tsx}' --type=ts -i
# Search for enum or type definitions that might contain conversation states
ast-grep --pattern 'type ConversationState = $_'
ast-grep --pattern 'enum ConversationState { $$$ }'
Length of output: 3575
Script:
#!/bin/bash
# Let's check the ConsentState type definition from the SDK
rg "ConsentState" -A 5 -B 5 --type-add 'ts:*.{ts,tsx}' --type=ts
# Let's also check the ConversationWithCodecsType to see its state property type
rg "ConversationWithCodecsType" -A 10 -B 2 --type-add 'ts:*.{ts,tsx}' --type=ts
Length of output: 67620
features/conversation/GroupConversationTitle.tsx (2)
14-16: LGTM! Import organization follows React best practices
The reorganization of hooks into a dedicated directory and the use of absolute imports with aliases improves code organization and maintainability.
Line range hint 82-87: Verify ConversationTitle component API compatibility
The replacement of ConversationTitleDumb with ConversationTitle looks correct, but let's ensure the new component maintains API compatibility.
✅ Verification successful
ConversationTitle component API is fully compatible
The component maintains the exact same props interface and implementation. The props being passed in GroupConversationTitle (title, onLongPress, onPress, avatarComponent) match perfectly with the component's type definition ConversationTitleDumbProps and its implementation. The only change appears to be the component's exported name, while maintaining backward compatibility by keeping the props type named as ConversationTitleDumbProps.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ConversationTitle component's props interface
# Search for the ConversationTitle component definition
ast-grep --pattern 'export const ConversationTitle = $_' -A 20
# Search for any other usages of ConversationTitle to verify consistent prop usage
rg -t typescript -t tsx "ConversationTitle.*" -g '!GroupConversationTitle.tsx'
Length of output: 180
Script:
#!/bin/bash
# Let's try different approaches to find the component definition and its usages
# Search for ConversationTitle component definition with .tsx extension
ast-grep --pattern 'const ConversationTitle = $_'
# Search for ConversationTitle type/interface definition
ast-grep --pattern 'interface ConversationTitleProps {
$$$
}'
# Search for ConversationTitle usages with proper file extensions
rg "ConversationTitle" --type-add 'ts:*.{ts,tsx}' -t ts
Length of output: 2947
Script:
#!/bin/bash
# Let's examine the ConversationTitle component definition and its props
cat features/conversation/conversation-title.tsx
Length of output: 1822
utils/date.ts (1)
101-103: 🛠️ Refactor suggestion
Add TypeScript return type and consider precision limitations
The implementation has several concerns:
- Missing return type annotation
- Potential precision loss due to JavaScript number limitations
- Inconsistency with existing timestamp handling in
normalizeTimestamp
Consider applying these improvements:
- export function getTodayNs() {
+ export function getTodayNs(): number {
- return Date.now() * 1000000;
+ // Use BigInt to prevent precision loss
+ return Number(BigInt(Date.now()) * BigInt(1000000));
}Let's verify the usage of timestamps across the codebase:
features/conversation-requests-list/useV3RequestItems.tsx (3)
Line range hint 70-70: LGTM!
The return statement is clean and provides all necessary data for the component consumers.
1-1: Verify consistent usage of the new import path across the codebase
The import path for getMessageContentType has been updated. Let's ensure this change is consistently applied throughout the codebase.
✅ Verification successful
Import path change has been consistently applied
The verification shows that:
- No instances of the old import path
@/utils/xmtpRN/contentTypeswere found - The new import path
@/utils/xmtpRN/content-types/content-typesis consistently used across 8 files - All imports are correctly using the new path structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old import paths and verify new import usage
echo "Checking for old import path usage..."
rg "from ['\"]@/utils/xmtpRN/contentTypes['\"]"
echo "Verifying new import path usage..."
rg "from ['\"]@/utils/xmtpRN/content-types/content-types['\"]"
Length of output: 1351
Line range hint 10-15: Consider the implications of disabled query refetching
The query options refetchOnWindowFocus: false and refetchOnMount: false might prevent users from seeing real-time updates to conversation requests. Consider if this is the intended behavior, especially for a messaging application where timely updates are crucial.
features/conversation/utils/messageIsFromCurrentUser.ts (1)
1-1: Verify the new import path usage across the codebase
The import path has been updated to reflect the new architecture. Let's verify this change is consistent across the codebase.
✅ Verification successful
Based on the search results, I can now generate the final response:
Import path change is consistent across the codebase
The new import path @/features/conversation/conversation-message/conversation-message.utils is being used consistently across the codebase, and there are no remaining references to the old import path @/components/Chat/Message/message-utils. The change aligns with the new architectural organization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining old import paths
rg "@/components/Chat/Message/message-utils"
# Search for usage of the new import path
rg "@/features/conversation/conversation-message/conversation-message.utils"
Length of output: 2633
features/notifications/utils/background/notificationSpamScore.ts (1)
6-6: Verify the new import path usage
The import path has been updated. Let's verify this change is consistent across the codebase.
✅ Verification successful
Import path change is consistent across the codebase
The search results show that all imports of content-type related functions are using the new path @/utils/xmtpRN/content-types/content-types, and there are no remaining instances of the old import path @utils/xmtpRN/contentTypes. The change is consistent throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining old import paths
rg "@utils/xmtpRN/contentTypes"
# Search for usage of the new import path
rg "@/utils/xmtpRN/content-types/content-types"
Length of output: 1144
theme/animations.ts (3)
10-10: LGTM: Import addition is appropriate
The withSpring import is correctly added and necessary for the new animation implementation.
76-78: LGTM: Improved naming consistency
The rename to reanimatedLayoutSpringTransition better reflects its purpose and maintains consistent naming patterns.
Line range hint 76-139: Verify the impact of animation changes
Let's ensure that components using these animations have been updated accordingly.
✅ Verification successful
Let me try another search strategy to find the animation usages.
Let me try one final search strategy without file type restrictions.
Animation changes are being used correctly across the codebase
The search results confirm that the animations are being used correctly:
reanimatedLayoutSpringTransitionis properly used in conversation components for layout animationsreanimatedFadeInScaleInis correctly used in conversation message reactions- All imports of animations from the theme are consistent with the changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of the renamed transition and new animation
echo "Checking usages of animations..."
rg "reanimatedSpringLayoutTransition|reanimatedLayoutSpringTransition|reanimatedFadeInScaleIn" --type ts --type tsx
# Look for potential Keyframe-based implementations that might need updating
echo "Checking for other Keyframe implementations..."
ast-grep --pattern 'new Keyframe({
$$$
})'
Length of output: 810
Script:
#!/bin/bash
# Search for usages of animations with proper file type
echo "Checking usages of animations..."
rg "reanimatedSpringLayoutTransition|reanimatedLayoutSpringTransition|reanimatedFadeInScaleIn" -t typescript -t javascriptreact -t typescriptreact
# Search for other animation-related imports and usages
echo "Checking animation imports and usages..."
rg "import.*animations|animations\." -t typescript -t javascriptreact -t typescriptreact
Length of output: 475
Script:
#!/bin/bash
# Search for any animation usages without file type restrictions
echo "Checking animation usages..."
rg "reanimatedLayoutSpringTransition|reanimatedFadeInScaleIn|FadeInDown\.easing|FadeInUp\.easing|FadeIn\.springify|FadeOut\.springify"
# Search for animation imports
echo "Checking animation imports..."
rg "from ['\"].*animations['\"]"
Length of output: 2647
features/conversation/conversation-message/conversation-message-layout.tsx (1)
1-45: Verify migration to new message layout component.
Since this is a new component that likely replaces older message layout implementations, we should verify that all message rendering code has been updated accordingly.
✅ Verification successful
Migration to ConversationMessageLayout is properly implemented
The new ConversationMessageLayout component is correctly used in both conversation views (conversation.tsx and ConversationReadOnly.tsx). The remaining V3Message references in the codebase are only in supporting components and iOS files, not in actual message layout implementations. The migration appears to be complete and consistent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of old message layout components
rg -l "MessageLayout|V3Message" --type tsx
# Search for usage of the new ConversationMessageLayout
rg -l "ConversationMessageLayout" --type tsx
Length of output: 145
Script:
#!/bin/bash
# Search for any remaining usage of old message layout components without file type restriction
rg -l "MessageLayout|V3Message"
# Search for usage of the new ConversationMessageLayout
rg -l "ConversationMessageLayout"
Length of output: 687
Script:
#!/bin/bash
# Check the actual usage context of both old and new components
rg "MessageLayout|V3Message" features/conversation/conversation.tsx -A 2 -B 2
rg "ConversationMessageLayout" features/conversation/conversation.tsx -A 2 -B 2
rg "MessageLayout|V3Message" screens/ConversationReadOnly.tsx -A 2 -B 2
rg "ConversationMessageLayout" screens/ConversationReadOnly.tsx -A 2 -B 2
Length of output: 4197
screens/Profile.tsx (1)
361-361: Verify navigation parameter changes across the codebase
The navigation parameter has been renamed from mainConversationWithPeer to peer, which improves clarity. Let's verify that this change has been applied consistently across the codebase.
Also applies to: 368-368, 375-375, 696-696
✅ Verification successful
Based on the gathered information, I can now provide the final response:
Navigation parameter change is consistent and correct
The change from mainConversationWithPeer to peer has been properly implemented:
- No instances of the old
mainConversationWithPeerparameter were found in the codebase - The new
peerparameter is consistently used in navigation calls - The change aligns with the navigation parameter handling in
navHelpers.tsand its tests - The parameter rename is part of a clean navigation parameter standardization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of the old parameter name
rg "mainConversationWithPeer" -A 2
# Search for the new parameter usage to ensure consistency
rg "params: \{ peer:" -A 2
Length of output: 63
Script:
#!/bin/bash
# Let's try a broader search for both old and new parameter patterns
# Search for the old parameter pattern with more variations
rg -i "mainconversationwithpeer" --type ts --type tsx
# Search for peer parameter in navigation or route contexts
rg "peer.*params|params.*peer" --type ts --type tsx
# Search for navigation-related files to check parameter usage
fd "\.(tsx?|js)$" | grep -i "navigation\|route"
Length of output: 1046
Script:
#!/bin/bash
# Let's check the navigation files for parameter usage patterns
cat screens/Navigation/navHelpers.ts
cat utils/navigation.ts
# Let's also check the Profile.tsx file directly to see the context of these changes
cat screens/Profile.tsx
# And check navigation-related test files for parameter usage
cat screens/Navigation/navHelpers.test.ts
Length of output: 44983
Script:
#!/bin/bash
# Let's search for navigation-related files and their usage of peer parameters
ast-grep --pattern 'navigation.navigate($_, { peer: $_ })'
# Let's also check for any remaining instances of mainConversationWithPeer in the codebase
ast-grep --pattern 'mainConversationWithPeer'
# And check for the current peer parameter usage
ast-grep --pattern 'params.peer'
Length of output: 578
features/conversation-list/hooks/useToggleReadStatus.ts (1)
4-4: Type safety improvement with ConversationTopic
Good improvement switching from string to the more specific ConversationTopic type from XMTP SDK. This provides better type safety and aligns with the SDK's type system.
Let's verify that all usages of this hook have been updated to pass the correct type:
Also applies to: 8-8
features/conversation/conversation-message-timestamp.tsx (1)
21-108: Component implementation is correct and efficient
The ConversationMessageTimestamp component is well-implemented. The use of animated values and effects is appropriate and follows best practices.
features/conversation/hooks/use-react-on-message.ts (1)
20-83: Implementation of useReactOnMessage Hook Looks Good
The hook is properly implemented with correct handling of optimistic updates and error management.
screens/ConversationReadOnly.tsx (1)
28-87: Refactoring Enhances Data Fetching and Error Handling
The updated implementation improves the component by utilizing useConversationQuery and ConversationMessagesList, enhancing readability and maintainability.
features/conversation/conversation.tsx (2)
159-163:
Add Null Checks to Prevent Accessing Undefined Message Properties
When determining the unread status, the code accesses messages?.ids[0] and messages?.byId[messages?.ids[0]]. If messages.ids is empty or undefined, this could lead to accessing undefined properties. Consider adding checks to ensure that messages.ids[0] exists before accessing it.
Apply this diff to add necessary checks:
const isUnread = useConversationIsUnread({
topic,
- lastMessage: messages?.byId[messages?.ids[0]], // Get latest message
- timestampNs: messages?.byId[messages?.ids[0]]?.sentNs ?? 0,
+ lastMessage: messages?.ids?.length ? messages.byId[messages.ids[0]] : undefined,
+ timestampNs: messages?.ids?.length ? messages.byId[messages.ids[0]]?.sentNs ?? 0 : 0,
});204-205:
Avoid Non-Null Assertions on Potentially Undefined Values
In the renderMessage function, the code uses a non-null assertion on messages?.byId[messageId]!. To prevent possible runtime errors, consider adding a null check before accessing the message.
Apply this diff to add a null check:
renderMessage={({ messageId, index }) => {
- const message = messages?.byId[messageId]!;
+ const message = messages?.byId[messageId];
+ if (!message) return null;screens/Main.tsx (1)
25-25: Verify the new import path
The ConversationScreenConfig import path has been updated as part of the conversation feature reorganization.
✅ Verification successful
Let me check the content of these files to verify if the old import path is still being used or if they're just referencing the component name.
Import path change is valid and consistent
The new import path ../features/conversation/conversation.nav is correct as:
- The target file exists at
features/conversation/conversation.nav.tsx - It properly exports
ConversationScreenConfig - Other files are using the new feature-based path structure for conversation-related imports
- References to "ConversationNav" in other files are either using the new path or referring to different components (NewConversationNav)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the new import path exists and check for any other references
# that might need updating
# Check if the new path exists
fd -e ts -e tsx "conversation.nav"
# Look for any potentially missed references to the old path
rg -l "ConversationNav"
Length of output: 474
Script:
#!/bin/bash
# Check content of files that reference ConversationNav
rg -A 2 -B 2 "ConversationNav" screens/Main.tsx
rg -A 2 -B 2 "ConversationNav" screens/Navigation/Navigation.tsx
rg -A 2 -B 2 "ConversationNav" screens/Profile.tsx
rg -A 2 -B 2 "ConversationNav" screens/NewConversation/NewConversationModal.tsx
# Also verify the new conversation.nav.tsx exports ConversationScreenConfig
rg "export.*ConversationScreenConfig" features/conversation/conversation.nav.tsx
Length of output: 2386
queries/useConversationMessages.ts (2)
30-41: LGTM: Well-structured type definitions for message accumulation
The types are well-defined with clear separation of concerns for messages and reactions tracking.
43-51: Consider uncommenting the utility function
The commented-out getReactionSignature function appears to be useful for generating consistent reaction signatures. Consider uncommenting it if it's being used elsewhere in the codebase.
features/conversation/conversation-message/conversation-message.utils.tsx (3)
101-103:
Handle potential undefined currentAccount
The non-null assertion on currentAccount could lead to runtime errors.
This issue was previously identified in past reviews. The same concerns apply here.
122-126:
Remove non-null assertion in getCurrentUserAccountInboxId
Similar to the previous issue, the non-null assertion on currentAccount should be handled safely.
export function getCurrentUserAccountInboxId() {
- const currentAccount = getCurrentAccount()!;
+ const currentAccount = getCurrentAccount();
+ if (!currentAccount) {
+ return null;
+ }
return queryClient.getQueryData<IGetCurrentAccountInboxIdQueryData>(
getCurrentAccountInboxIdQueryOptions(currentAccount).queryKey
);
}Likely invalid or redundant comment.
128-132:
Handle edge case in prefetchCurrentUserAccountInboxId
The function should handle the case where getCurrentAccount returns undefined.
export function prefetchCurrentUserAccountInboxId() {
- const currentAccount = getCurrentAccount()!;
+ const currentAccount = getCurrentAccount();
+ if (!currentAccount) {
+ return Promise.reject(new Error('No current account found'));
+ }
return queryClient.prefetchQuery(
getCurrentAccountInboxIdQueryOptions(currentAccount)
);
}Likely invalid or redundant comment.
screens/Navigation/Navigation.tsx (1)
30-33: LGTM: Clean import restructuring
The consolidation of conversation-related imports improves code organization.
features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx (1)
61-71:
Replace non-null assertions with proper null checks
The code uses multiple non-null assertions (!) which could lead to runtime errors if the values are undefined. Consider adding proper null checks for better type safety.
Apply this diff to add proper null checks:
-const account = useCurrentAccount()!;
+const account = useCurrentAccount();
+if (!account) return null;
-const { data: conversation } = useConversationQuery(account, topic);
+const { data: conversation } = useConversationQuery(account, topic);
+if (!conversation) return null;
-const message = getMessageById({
+const message = getMessageById({
messageId,
topic,
-})!;
+});
+if (!message) return null;Likely invalid or redundant comment.
| export function isConversationAllowed( | ||
| conversation: ConversationWithCodecsType | ||
| ) { | ||
| return conversation.state === "allowed"; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety and null checking
While the implementation is straightforward, it could benefit from additional type safety and null checking.
Consider applying these improvements:
+// Define possible conversation states as a union type
+type ConversationState = "allowed" | "blocked" | "pending";
+
+// Define the state constant
+const ALLOWED_STATE: ConversationState = "allowed";
+
export function isConversationAllowed(
conversation: ConversationWithCodecsType
) {
- return conversation.state === "allowed";
+ return conversation?.state === ALLOWED_STATE;
}This refactor:
- Adds type safety for possible conversation states
- Prevents typos in state comparison
- Adds null-safety check
- Makes it easier to update state values if needed
📝 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 isConversationAllowed( | |
| conversation: ConversationWithCodecsType | |
| ) { | |
| return conversation.state === "allowed"; | |
| } | |
| // Define possible conversation states as a union type | |
| type ConversationState = "allowed" | "blocked" | "pending"; | |
| // Define the state constant | |
| const ALLOWED_STATE: ConversationState = "allowed"; | |
| export function isConversationAllowed( | |
| conversation: ConversationWithCodecsType | |
| ) { | |
| return conversation?.state === ALLOWED_STATE; | |
| } |
| @@ -139,6 +139,7 @@ const ConversationContextMenuComponent: FC = () => { | |||
| > | |||
| <ConversationReadOnly | |||
| topic={conversationTopic as ConversationTopic} | |||
| conversationId={conversationId} | |||
There was a problem hiding this comment.
Fix undefined conversationId variable
The conversationId prop is being passed to ConversationReadOnly but is not defined anywhere in the component. This will cause runtime errors.
Consider one of these fixes:
- conversationId={conversationId}
+ conversationId={conversationTopic.conversationId}Or add it to the component's state management:
const conversationTopic = useConversationListContextMenuConversationTopic();
+ const conversationId = useConversationListContextMenuConversationId();
const contextMenuItems = useConversationListContextMenuItems();Committable suggestion skipped: line range outside the PR's diff.
| const translation = swipeableRef.current?.state.rowTranslation; | ||
| const translationValue = (translation as any)._value; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid accessing internal state properties of Swipeable
Accessing swipeableRef.current?.state.rowTranslation and using its _value directly relies on internal properties of the Swipeable component. This can lead to instability and potential breakage with future updates, as these internal APIs are not guaranteed to remain consistent.
Consider refactoring to avoid relying on private properties. You might use public methods or event handlers provided by Swipeable. Alternatively, once upgraded to Expo SDK 52, you can utilize ReanimatedSwipeable and SharedValue for a more performant and stable implementation, as noted in your TODO comments.
| const { data: conversation } = useConversationQuery(currentAccount, topic); | ||
|
|
||
| const sendMessage = useSendMessage({ | ||
| conversation: conversation!, | ||
| }); |
There was a problem hiding this comment.
Add Null Checks for conversation Before Usage
In the GroupConversationEmpty component, the conversation object is assumed to be non-null with the non-null assertion !. If conversation is undefined, this could lead to runtime errors. Consider adding a null check to ensure conversation is defined before using it.
Apply this diff to ensure conversation is defined:
const { data: conversation } = useConversationQuery(currentAccount, topic);
-const sendMessage = useSendMessage({
- conversation: conversation!,
-});
+if (!conversation) {
+ return null; // or handle the undefined state appropriately
+}
+
+const sendMessage = useSendMessage({
+ conversation,
+});📝 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 { data: conversation } = useConversationQuery(currentAccount, topic); | |
| const sendMessage = useSendMessage({ | |
| conversation: conversation!, | |
| }); | |
| const { data: conversation } = useConversationQuery(currentAccount, topic); | |
| if (!conversation) { | |
| return null; // or handle the undefined state appropriately | |
| } | |
| const sendMessage = useSendMessage({ | |
| conversation, | |
| }); |
| const sendMessage = useCallback( | ||
| async ({ referencedMessageId, content }: ISendMessageParams) => { | ||
| if (!conversation) { | ||
| return; | ||
| } | ||
|
|
||
| if (referencedMessageId) { | ||
| if (content.remoteAttachment) { | ||
| await conversation.send({ | ||
| reply: { | ||
| reference: referencedMessageId, | ||
| content: { remoteAttachment: content.remoteAttachment }, | ||
| }, | ||
| }); | ||
| } | ||
| if (content.text) { | ||
| await conversation.send({ | ||
| reply: { | ||
| reference: referencedMessageId, | ||
| content: { text: content.text }, | ||
| }, | ||
| }); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| if (content.remoteAttachment) { | ||
| await conversation.send({ | ||
| remoteAttachment: content.remoteAttachment, | ||
| }); | ||
| } | ||
|
|
||
| if (content.text) { | ||
| await conversation?.send({ | ||
| text: content.text, | ||
| }); | ||
| } | ||
| }, | ||
| [conversation] | ||
| ); |
There was a problem hiding this comment.
Add error handling and consider potential race conditions
The sendMessage implementation has several areas for improvement:
- Error handling is missing for the conversation.send() calls
- When sending both text and attachment, parallel sends could cause message ordering issues
- Code duplication in send logic between reply and regular messages
Consider this refactored implementation:
const sendMessage = useCallback(
async ({ referencedMessageId, content }: ISendMessageParams) => {
if (!conversation) {
- return;
+ throw new Error("Conversation is not defined");
}
+ try {
+ const sendContent = async (messageContent: { text?: string; remoteAttachment?: RemoteAttachmentContent }) => {
+ const payload = referencedMessageId
+ ? { reply: { reference: referencedMessageId, content: messageContent } }
+ : messageContent;
+ await conversation.send(payload);
+ };
+ // Send attachment first, then text to maintain order
+ if (content.remoteAttachment) {
+ await sendContent({ remoteAttachment: content.remoteAttachment });
+ }
+ if (content.text) {
+ await sendContent({ text: content.text });
+ }
+ } catch (error) {
+ console.error('Failed to send message:', error);
+ throw error;
+ }
},
[conversation]
);📝 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 sendMessage = useCallback( | |
| async ({ referencedMessageId, content }: ISendMessageParams) => { | |
| if (!conversation) { | |
| return; | |
| } | |
| if (referencedMessageId) { | |
| if (content.remoteAttachment) { | |
| await conversation.send({ | |
| reply: { | |
| reference: referencedMessageId, | |
| content: { remoteAttachment: content.remoteAttachment }, | |
| }, | |
| }); | |
| } | |
| if (content.text) { | |
| await conversation.send({ | |
| reply: { | |
| reference: referencedMessageId, | |
| content: { text: content.text }, | |
| }, | |
| }); | |
| } | |
| return; | |
| } | |
| if (content.remoteAttachment) { | |
| await conversation.send({ | |
| remoteAttachment: content.remoteAttachment, | |
| }); | |
| } | |
| if (content.text) { | |
| await conversation?.send({ | |
| text: content.text, | |
| }); | |
| } | |
| }, | |
| [conversation] | |
| ); | |
| const sendMessage = useCallback( | |
| async ({ referencedMessageId, content }: ISendMessageParams) => { | |
| if (!conversation) { | |
| throw new Error("Conversation is not defined"); | |
| } | |
| try { | |
| const sendContent = async (messageContent: { text?: string; remoteAttachment?: RemoteAttachmentContent }) => { | |
| const payload = referencedMessageId | |
| ? { reply: { reference: referencedMessageId, content: messageContent } } | |
| : messageContent; | |
| await conversation.send(payload); | |
| }; | |
| // Send attachment first, then text to maintain order | |
| if (content.remoteAttachment) { | |
| await sendContent({ remoteAttachment: content.remoteAttachment }); | |
| } | |
| if (content.text) { | |
| await sendContent({ text: content.text }); | |
| } | |
| } catch (error) { | |
| console.error('Failed to send message:', error); | |
| throw error; | |
| } | |
| }, | |
| [conversation] | |
| ); |
| const { mutateAsync: sendMessageAsync, status: sendMessageStatus } = | ||
| useMutation({ | ||
| mutationFn: async (args: { | ||
| conversation: ConversationWithCodecsType; | ||
| text?: string; | ||
| remoteAttachment?: RemoteAttachmentContent; | ||
| referencedMessageId?: MessageId; | ||
| }) => { | ||
| const { conversation, text, remoteAttachment, referencedMessageId } = | ||
| args; | ||
|
|
||
| if (referencedMessageId) { | ||
| if (remoteAttachment) { | ||
| await conversation.send({ | ||
| reply: { | ||
| reference: referencedMessageId, | ||
| content: { remoteAttachment }, | ||
| }, | ||
| }); | ||
| } | ||
| if (text) { | ||
| await conversation.send({ | ||
| reply: { | ||
| reference: referencedMessageId, | ||
| content: { text }, | ||
| }, | ||
| }); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| if (remoteAttachment) { | ||
| await conversation.send({ | ||
| remoteAttachment, | ||
| }); | ||
| } | ||
|
|
||
| if (text) { | ||
| await conversation.send(text); | ||
| } | ||
| }, | ||
| // TODO: Add this for optimistic update and faster UX | ||
| // onMutate: (args) => { | ||
| // try { | ||
| // const { conversation } = args; | ||
| // const currentAccount = getCurrentAccount()!; | ||
| // addConversationMessage(currentAccount, conversation.topic!, { | ||
| // id: "RANDOM_MESSAGE_ID", | ||
| // content: { text: "RANDOM_MESSAGE_TEXT" }, | ||
| // }); | ||
| // } catch (error) { | ||
| // console.log("error:", error); | ||
| // } | ||
| // }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify message sending logic
The message sending logic is complex with multiple nested conditions. Consider refactoring to improve readability and maintainability.
- mutationFn: async (args: {
- conversation: ConversationWithCodecsType;
- text?: string;
- remoteAttachment?: RemoteAttachmentContent;
- referencedMessageId?: MessageId;
- }) => {
- const { conversation, text, remoteAttachment, referencedMessageId } =
- args;
-
- if (referencedMessageId) {
- if (remoteAttachment) {
- await conversation.send({
- reply: {
- reference: referencedMessageId,
- content: { remoteAttachment },
- },
- });
- }
- if (text) {
- await conversation.send({
- reply: {
- reference: referencedMessageId,
- content: { text },
- },
- });
- }
- return;
- }
-
- if (remoteAttachment) {
- await conversation.send({
- remoteAttachment,
- });
- }
-
- if (text) {
- await conversation.send(text);
- }
+ mutationFn: async ({
+ conversation,
+ text,
+ remoteAttachment,
+ referencedMessageId,
+ }: {
+ conversation: ConversationWithCodecsType;
+ text?: string;
+ remoteAttachment?: RemoteAttachmentContent;
+ referencedMessageId?: MessageId;
+ }) => {
+ const content = remoteAttachment
+ ? { remoteAttachment }
+ : text
+ ? { text }
+ : null;
+
+ if (!content) return;
+
+ await conversation.send(
+ referencedMessageId
+ ? {
+ reply: {
+ reference: referencedMessageId,
+ content,
+ },
+ }
+ : content
+ );📝 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 { mutateAsync: sendMessageAsync, status: sendMessageStatus } = | |
| useMutation({ | |
| mutationFn: async (args: { | |
| conversation: ConversationWithCodecsType; | |
| text?: string; | |
| remoteAttachment?: RemoteAttachmentContent; | |
| referencedMessageId?: MessageId; | |
| }) => { | |
| const { conversation, text, remoteAttachment, referencedMessageId } = | |
| args; | |
| if (referencedMessageId) { | |
| if (remoteAttachment) { | |
| await conversation.send({ | |
| reply: { | |
| reference: referencedMessageId, | |
| content: { remoteAttachment }, | |
| }, | |
| }); | |
| } | |
| if (text) { | |
| await conversation.send({ | |
| reply: { | |
| reference: referencedMessageId, | |
| content: { text }, | |
| }, | |
| }); | |
| } | |
| return; | |
| } | |
| if (remoteAttachment) { | |
| await conversation.send({ | |
| remoteAttachment, | |
| }); | |
| } | |
| if (text) { | |
| await conversation.send(text); | |
| } | |
| }, | |
| // TODO: Add this for optimistic update and faster UX | |
| // onMutate: (args) => { | |
| // try { | |
| // const { conversation } = args; | |
| // const currentAccount = getCurrentAccount()!; | |
| // addConversationMessage(currentAccount, conversation.topic!, { | |
| // id: "RANDOM_MESSAGE_ID", | |
| // content: { text: "RANDOM_MESSAGE_TEXT" }, | |
| // }); | |
| // } catch (error) { | |
| // console.log("error:", error); | |
| // } | |
| // }, | |
| }); | |
| const { mutateAsync: sendMessageAsync, status: sendMessageStatus } = | |
| useMutation({ | |
| mutationFn: async ({ | |
| conversation, | |
| text, | |
| remoteAttachment, | |
| referencedMessageId, | |
| }: { | |
| conversation: ConversationWithCodecsType; | |
| text?: string; | |
| remoteAttachment?: RemoteAttachmentContent; | |
| referencedMessageId?: MessageId; | |
| }) => { | |
| const content = remoteAttachment | |
| ? { remoteAttachment } | |
| : text | |
| ? { text } | |
| : null; | |
| if (!content) return; | |
| await conversation.send( | |
| referencedMessageId | |
| ? { | |
| reply: { | |
| reference: referencedMessageId, | |
| content, | |
| }, | |
| } | |
| : content | |
| ); | |
| }, | |
| // TODO: Add this for optimistic update and faster UX | |
| // onMutate: (args) => { | |
| // try { | |
| // const { conversation } = args; | |
| // const currentAccount = getCurrentAccount()!; | |
| // addConversationMessage(currentAccount, conversation.topic!, { | |
| // id: "RANDOM_MESSAGE_ID", | |
| // content: { text: "RANDOM_MESSAGE_TEXT" }, | |
| // }); | |
| // } catch (error) { | |
| // console.log("error:", error); | |
| // } | |
| // }, | |
| }); |
| const timeInlineAnimatedStyle = useAnimatedStyle(() => { | ||
| return { | ||
| display: showTimeAV.value ? "flex" : "none", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid toggling the display property in animated styles
The display property is not animatable in React Native. Toggling it can cause layout reflows and unexpected behavior. Instead, control visibility using opacity alone.
Apply this diff to adjust the animated style:
return {
- display: showTimeAV.value ? "flex" : "none",
opacity: withSpring(showTimeAV.value ? 1 : 0, {
damping: SICK_DAMPING,
stiffness: SICK_STIFFNESS,
}),
};Committable suggestion skipped: line range outside the PR's diff.
| const handleSelectReaction = useCallback( | ||
| (emoji: string) => { | ||
| const currentUserAlreadyReacted = bySender?.[currentUserInboxId!]?.some( | ||
| (reaction) => reaction.content === emoji | ||
| ); | ||
|
|
||
| if (currentUserAlreadyReacted) { | ||
| removeReactionOnMessage({ | ||
| messageId: messageId, | ||
| emoji, | ||
| }); | ||
| } else { | ||
| reactOnMessage({ messageId: messageId, emoji }); | ||
| } | ||
| messageContextMenuStore.getState().setMessageContextMenuData(null); | ||
| }, | ||
| [ | ||
| reactOnMessage, | ||
| messageId, | ||
| currentUserInboxId, | ||
| bySender, | ||
| removeReactionOnMessage, | ||
| messageContextMenuStore, | ||
| ] | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve reaction handling safety and readability
The reaction handling logic uses a non-null assertion on currentUserInboxId which could cause runtime errors. Additionally, the function could be simplified.
Apply this diff to improve the code:
const handleSelectReaction = useCallback(
(emoji: string) => {
- const currentUserAlreadyReacted = bySender?.[currentUserInboxId!]?.some(
+ if (!currentUserInboxId) return;
+
+ const currentUserAlreadyReacted = bySender?.[currentUserInboxId]?.some(
(reaction) => reaction.content === emoji
);
if (currentUserAlreadyReacted) {
removeReactionOnMessage({
messageId: messageId,
emoji,
});
} else {
reactOnMessage({ messageId: messageId, emoji });
}
messageContextMenuStore.getState().setMessageContextMenuData(null);
},
[
reactOnMessage,
messageId,
currentUserInboxId,
bySender,
removeReactionOnMessage,
messageContextMenuStore,
]
);📝 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 handleSelectReaction = useCallback( | |
| (emoji: string) => { | |
| const currentUserAlreadyReacted = bySender?.[currentUserInboxId!]?.some( | |
| (reaction) => reaction.content === emoji | |
| ); | |
| if (currentUserAlreadyReacted) { | |
| removeReactionOnMessage({ | |
| messageId: messageId, | |
| emoji, | |
| }); | |
| } else { | |
| reactOnMessage({ messageId: messageId, emoji }); | |
| } | |
| messageContextMenuStore.getState().setMessageContextMenuData(null); | |
| }, | |
| [ | |
| reactOnMessage, | |
| messageId, | |
| currentUserInboxId, | |
| bySender, | |
| removeReactionOnMessage, | |
| messageContextMenuStore, | |
| ] | |
| ); | |
| const handleSelectReaction = useCallback( | |
| (emoji: string) => { | |
| if (!currentUserInboxId) return; | |
| const currentUserAlreadyReacted = bySender?.[currentUserInboxId]?.some( | |
| (reaction) => reaction.content === emoji | |
| ); | |
| if (currentUserAlreadyReacted) { | |
| removeReactionOnMessage({ | |
| messageId: messageId, | |
| emoji, | |
| }); | |
| } else { | |
| reactOnMessage({ messageId: messageId, emoji }); | |
| } | |
| messageContextMenuStore.getState().setMessageContextMenuData(null); | |
| }, | |
| [ | |
| reactOnMessage, | |
| messageId, | |
| currentUserInboxId, | |
| bySender, | |
| removeReactionOnMessage, | |
| messageContextMenuStore, | |
| ] | |
| ); |
| function waitUntilMediaPreviewIsUploaded() { | ||
| return new Promise((resolve, reject) => { | ||
| const startTime = Date.now(); | ||
| const checkStatus = () => { | ||
| if (!mediaPreview?.status) { | ||
| resolve(true); | ||
| return; | ||
| } | ||
| if (mediaPreview.status === "uploaded") { | ||
| resolve(true); | ||
| return; | ||
| } | ||
| if (Date.now() - startTime > 10000) { | ||
| reject(new Error("Media upload timeout after 10 seconds")); | ||
| return; | ||
| } | ||
| setTimeout(checkStatus, 200); | ||
| }; | ||
| checkStatus(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve media upload status checking mechanism
The current implementation has several potential issues:
- No cleanup if component unmounts during upload
- Polling with setTimeout could lead to memory leaks
- Hard-coded timeout value should be configurable
Consider using this improved implementation:
+const MEDIA_UPLOAD_TIMEOUT = 10000;
+
function waitUntilMediaPreviewIsUploaded() {
return new Promise((resolve, reject) => {
const startTime = Date.now();
+ let timeoutId: NodeJS.Timeout;
+
const checkStatus = () => {
if (!mediaPreview?.status) {
resolve(true);
return;
}
if (mediaPreview.status === "uploaded") {
resolve(true);
return;
}
- if (Date.now() - startTime > 10000) {
+ if (Date.now() - startTime > MEDIA_UPLOAD_TIMEOUT) {
reject(new Error("Media upload timeout after 10 seconds"));
return;
}
- setTimeout(checkStatus, 200);
+ timeoutId = setTimeout(checkStatus, 200);
};
+
checkStatus();
+
+ return () => {
+ if (timeoutId) clearTimeout(timeoutId);
+ };
});
}📝 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.
| function waitUntilMediaPreviewIsUploaded() { | |
| return new Promise((resolve, reject) => { | |
| const startTime = Date.now(); | |
| const checkStatus = () => { | |
| if (!mediaPreview?.status) { | |
| resolve(true); | |
| return; | |
| } | |
| if (mediaPreview.status === "uploaded") { | |
| resolve(true); | |
| return; | |
| } | |
| if (Date.now() - startTime > 10000) { | |
| reject(new Error("Media upload timeout after 10 seconds")); | |
| return; | |
| } | |
| setTimeout(checkStatus, 200); | |
| }; | |
| checkStatus(); | |
| }); | |
| } | |
| const MEDIA_UPLOAD_TIMEOUT = 10000; | |
| function waitUntilMediaPreviewIsUploaded() { | |
| return new Promise((resolve, reject) => { | |
| const startTime = Date.now(); | |
| let timeoutId: NodeJS.Timeout; | |
| const checkStatus = () => { | |
| if (!mediaPreview?.status) { | |
| resolve(true); | |
| return; | |
| } | |
| if (mediaPreview.status === "uploaded") { | |
| resolve(true); | |
| return; | |
| } | |
| if (Date.now() - startTime > MEDIA_UPLOAD_TIMEOUT) { | |
| reject(new Error("Media upload timeout after 10 seconds")); | |
| return; | |
| } | |
| timeoutId = setTimeout(checkStatus, 200); | |
| }; | |
| checkStatus(); | |
| return () => { | |
| if (timeoutId) clearTimeout(timeoutId); | |
| }; | |
| }); | |
| } |
| const send = useCallback(async () => { | ||
| const mediaPreview = store.getState().composerMediaPreview; | ||
| const replyingToMessageId = store.getState().replyingToMessageId; | ||
|
|
||
| function waitUntilMediaPreviewIsUploaded() { | ||
| return new Promise((resolve, reject) => { | ||
| const startTime = Date.now(); | ||
| const checkStatus = () => { | ||
| if (!mediaPreview?.status) { | ||
| resolve(true); | ||
| return; | ||
| } | ||
| if (mediaPreview.status === "uploaded") { | ||
| resolve(true); | ||
| return; | ||
| } | ||
| if (Date.now() - startTime > 10000) { | ||
| reject(new Error("Media upload timeout after 10 seconds")); | ||
| return; | ||
| } | ||
| setTimeout(checkStatus, 200); | ||
| }; | ||
| checkStatus(); | ||
| }); | ||
| } | ||
|
|
||
| if (mediaPreview) { | ||
| if (mediaPreview?.status === "uploading") { | ||
| await waitUntilMediaPreviewIsUploaded(); | ||
| } | ||
|
|
||
| store.getState().updateMediaPreviewStatus("sending"); | ||
|
|
||
| try { | ||
| const mediaPreview = store.getState().composerMediaPreview; | ||
| if (mediaPreview) { | ||
| await saveAttachmentLocally(mediaPreview); | ||
| } | ||
| } catch (error) { | ||
| sentryTrackError(error); | ||
| } | ||
|
|
||
| const uploadedRemoteAttachment = | ||
| store.getState().composerUploadedAttachment; | ||
|
|
||
| if (!uploadedRemoteAttachment) { | ||
| throw new Error("Something went wrong while uploading attachment"); | ||
| } | ||
|
|
||
| await onSend({ | ||
| content: { | ||
| remoteAttachment: uploadedRemoteAttachment, | ||
| }, | ||
| ...(replyingToMessageId && { | ||
| referencedMessageId: replyingToMessageId, | ||
| }), | ||
| }); | ||
|
|
||
| store.getState().setComposerMediaPreview(null); | ||
| store.getState().setComposerUploadedAttachment(null); | ||
| } | ||
|
|
||
| const inputValue = store.getState().inputValue; | ||
|
|
||
| if (inputValue.length > 0) { | ||
| await onSend({ | ||
| content: { | ||
| text: inputValue, | ||
| }, | ||
| ...(replyingToMessageId && { | ||
| referencedMessageId: replyingToMessageId, | ||
| }), | ||
| }); | ||
| } | ||
|
|
||
| store.getState().reset(); | ||
|
|
||
| // TODO: Fix with function in context | ||
| // converseEventEmitter.emit("scrollChatToMessage", { | ||
| // index: 0, | ||
| // }); | ||
| }, [onSend, store]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in send function
The current error handling is minimal and might lead to silent failures. Consider adding proper error handling with user feedback.
Consider these improvements:
const send = useCallback(async () => {
+ try {
const mediaPreview = store.getState().composerMediaPreview;
const replyingToMessageId = store.getState().replyingToMessageId;
if (mediaPreview) {
if (mediaPreview?.status === "uploading") {
- await waitUntilMediaPreviewIsUploaded();
+ try {
+ await waitUntilMediaPreviewIsUploaded();
+ } catch (error) {
+ store.getState().updateMediaPreviewStatus("error");
+ throw new Error("Failed to upload media: " + error.message);
+ }
}
store.getState().updateMediaPreviewStatus("sending");
try {
const mediaPreview = store.getState().composerMediaPreview;
if (mediaPreview) {
await saveAttachmentLocally(mediaPreview);
}
} catch (error) {
sentryTrackError(error);
+ store.getState().updateMediaPreviewStatus("error");
+ throw new Error("Failed to save attachment locally");
}
const uploadedRemoteAttachment =
store.getState().composerUploadedAttachment;
if (!uploadedRemoteAttachment) {
+ store.getState().updateMediaPreviewStatus("error");
throw new Error("Something went wrong while uploading attachment");
}
await onSend({
content: {
remoteAttachment: uploadedRemoteAttachment,
},
...(replyingToMessageId && {
referencedMessageId: replyingToMessageId,
}),
});
store.getState().setComposerMediaPreview(null);
store.getState().setComposerUploadedAttachment(null);
}
const inputValue = store.getState().inputValue;
if (inputValue.length > 0) {
await onSend({
content: {
text: inputValue,
},
...(replyingToMessageId && {
referencedMessageId: replyingToMessageId,
}),
});
}
store.getState().reset();
+ } catch (error) {
+ sentryTrackError(error);
+ // TODO: Show error toast to user
+ console.error('Failed to send message:', error);
+ }
}, [onSend, store]);📝 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 send = useCallback(async () => { | |
| const mediaPreview = store.getState().composerMediaPreview; | |
| const replyingToMessageId = store.getState().replyingToMessageId; | |
| function waitUntilMediaPreviewIsUploaded() { | |
| return new Promise((resolve, reject) => { | |
| const startTime = Date.now(); | |
| const checkStatus = () => { | |
| if (!mediaPreview?.status) { | |
| resolve(true); | |
| return; | |
| } | |
| if (mediaPreview.status === "uploaded") { | |
| resolve(true); | |
| return; | |
| } | |
| if (Date.now() - startTime > 10000) { | |
| reject(new Error("Media upload timeout after 10 seconds")); | |
| return; | |
| } | |
| setTimeout(checkStatus, 200); | |
| }; | |
| checkStatus(); | |
| }); | |
| } | |
| if (mediaPreview) { | |
| if (mediaPreview?.status === "uploading") { | |
| await waitUntilMediaPreviewIsUploaded(); | |
| } | |
| store.getState().updateMediaPreviewStatus("sending"); | |
| try { | |
| const mediaPreview = store.getState().composerMediaPreview; | |
| if (mediaPreview) { | |
| await saveAttachmentLocally(mediaPreview); | |
| } | |
| } catch (error) { | |
| sentryTrackError(error); | |
| } | |
| const uploadedRemoteAttachment = | |
| store.getState().composerUploadedAttachment; | |
| if (!uploadedRemoteAttachment) { | |
| throw new Error("Something went wrong while uploading attachment"); | |
| } | |
| await onSend({ | |
| content: { | |
| remoteAttachment: uploadedRemoteAttachment, | |
| }, | |
| ...(replyingToMessageId && { | |
| referencedMessageId: replyingToMessageId, | |
| }), | |
| }); | |
| store.getState().setComposerMediaPreview(null); | |
| store.getState().setComposerUploadedAttachment(null); | |
| } | |
| const inputValue = store.getState().inputValue; | |
| if (inputValue.length > 0) { | |
| await onSend({ | |
| content: { | |
| text: inputValue, | |
| }, | |
| ...(replyingToMessageId && { | |
| referencedMessageId: replyingToMessageId, | |
| }), | |
| }); | |
| } | |
| store.getState().reset(); | |
| // TODO: Fix with function in context | |
| // converseEventEmitter.emit("scrollChatToMessage", { | |
| // index: 0, | |
| // }); | |
| }, [onSend, store]); | |
| const send = useCallback(async () => { | |
| try { | |
| const mediaPreview = store.getState().composerMediaPreview; | |
| const replyingToMessageId = store.getState().replyingToMessageId; | |
| function waitUntilMediaPreviewIsUploaded() { | |
| return new Promise((resolve, reject) => { | |
| const startTime = Date.now(); | |
| const checkStatus = () => { | |
| if (!mediaPreview?.status) { | |
| resolve(true); | |
| return; | |
| } | |
| if (mediaPreview.status === "uploaded") { | |
| resolve(true); | |
| return; | |
| } | |
| if (Date.now() - startTime > 10000) { | |
| reject(new Error("Media upload timeout after 10 seconds")); | |
| return; | |
| } | |
| setTimeout(checkStatus, 200); | |
| }; | |
| checkStatus(); | |
| }); | |
| } | |
| if (mediaPreview) { | |
| if (mediaPreview?.status === "uploading") { | |
| try { | |
| await waitUntilMediaPreviewIsUploaded(); | |
| } catch (error) { | |
| store.getState().updateMediaPreviewStatus("error"); | |
| throw new Error("Failed to upload media: " + error.message); | |
| } | |
| } | |
| store.getState().updateMediaPreviewStatus("sending"); | |
| try { | |
| const mediaPreview = store.getState().composerMediaPreview; | |
| if (mediaPreview) { | |
| await saveAttachmentLocally(mediaPreview); | |
| } | |
| } catch (error) { | |
| sentryTrackError(error); | |
| store.getState().updateMediaPreviewStatus("error"); | |
| throw new Error("Failed to save attachment locally"); | |
| } | |
| const uploadedRemoteAttachment = | |
| store.getState().composerUploadedAttachment; | |
| if (!uploadedRemoteAttachment) { | |
| store.getState().updateMediaPreviewStatus("error"); | |
| throw new Error("Something went wrong while uploading attachment"); | |
| } | |
| await onSend({ | |
| content: { | |
| remoteAttachment: uploadedRemoteAttachment, | |
| }, | |
| ...(replyingToMessageId && { | |
| referencedMessageId: replyingToMessageId, | |
| }), | |
| }); | |
| store.getState().setComposerMediaPreview(null); | |
| store.getState().setComposerUploadedAttachment(null); | |
| } | |
| const inputValue = store.getState().inputValue; | |
| if (inputValue.length > 0) { | |
| await onSend({ | |
| content: { | |
| text: inputValue, | |
| }, | |
| ...(replyingToMessageId && { | |
| referencedMessageId: replyingToMessageId, | |
| }), | |
| }); | |
| } | |
| store.getState().reset(); | |
| } catch (error) { | |
| sentryTrackError(error); | |
| // TODO: Show error toast to user | |
| console.error('Failed to send message:', error); | |
| } | |
| }, [onSend, store]); |
thierryskoda
force-pushed
the
ts/decouple-dm-group-conversations
branch
from
5d9b50a to
1773684
Compare
thierryskoda
force-pushed
the
ts/decouple-dm-group-conversations
branch
from
64f5847 to
63c58c8
Compare
* wip * wip * wip * wip * performance improvment * remove service + small fixes * rebase and fix tsc
Loom to explain some of the changes and thoughts: https://www.loom.com/share/105af117a9ca4861967357110fbf9446
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Documentation