feat: more chat fixes + message status + optimistic update for text message + refactor queries/mutations #1357
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces a comprehensive set of changes across multiple files in the conversation and messaging features of the application. The modifications focus on enhancing the conversation composer, message status tracking, keyboard interactions, and overall conversation management. Key updates include the introduction of new components like 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 (
|
Performance Comparison ReportSignificant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
Render Count ChangesThere are no entries Render IssuesThere are no entries Added ScenariosThere are no entries Removed ScenariosThere are no entries |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (22)
features/conversation/conversation-message/conversation-message-content-types/conversation-message-reply.tsx (2)
Line range hint
44-48: Enhance error handling for string type messages.Instead of just logging to console, consider:
- Using proper error tracking via
sentryTrackError- Implementing the TODO to render a simple bubble message
if (typeof replyMessageContent === "string") { - // TODO. Render simple bubble message with the content? - console.error("reply message is a string"); + sentryTrackError("Unexpected string type for reply message content"); + return ( + <BubbleContainer fromMe={fromMe}> + <BubbleContentContainer + fromMe={fromMe} + hasNextMessageInSeries={hasNextMessageInSeries} + > + <MessageText inverted={fromMe}>{replyMessageContent}</MessageText> + </BubbleContentContainer> + </BubbleContainer> + ); }
Line range hint
219-244: Optimize query handling in useConversationMessageForReplyMessage.Consider these improvements:
- Memoize the query options to prevent unnecessary object creation
- Move the query configuration outside the hook when not enabled
function useConversationMessageForReplyMessage( messageId: MessageId ): DecodedMessage<ReplyCodec> | undefined { const currentAccount = useCurrentAccount()!; const topic = useCurrentConversationTopic(); const messages = getConversationMessages(currentAccount, topic); const cachedReplyMessage = messages?.byId[messageId] as | DecodedMessage<ReplyCodec> | undefined; + const queryOptions = React.useMemo( + () => ({ + ...getConversationMessageQueryOptions({ + account: currentAccount, + messageId, + }), + enabled: !cachedReplyMessage, + }), + [currentAccount, messageId, cachedReplyMessage] + ); // Only fetch the message if it's in the list of messages of the conversation - const { data: replyMessage } = useQuery({ - ...getConversationMessageQueryOptions({ - account: currentAccount, - messageId, - }), - enabled: !cachedReplyMessage, - }); + const { data: replyMessage } = useQuery(queryOptions); return ( (replyMessage as DecodedMessage<ReplyCodec> | undefined) ?? cachedReplyMessage ); }features/conversation/conversation-message/conversation-message-reactions/conversation-message-reactions.tsx (1)
103-105: Simplify inbox ID extraction for clarity and efficiencyYou can simplify the extraction of
inboxIdsby usingObject.keys()instead ofObject.entries().map().Apply this diff to simplify the code:
const inboxIds = Array.from( new Set( - Object.entries(reactionsBySender ?? {}).map( - ([senderInboxId]) => senderInboxId - ) + Object.keys(reactionsBySender ?? {}) ) );features/conversation/conversation-keyboard-filler.tsx (4)
28-29: Remove unnecessaryconsole.logstatementsThe
console.logstatements on lines 28 and 29 are likely used for debugging purposes and should be removed to clean up the production code.
47-52: Review dependencies inuseEffectto avoid unnecessary re-rendersIncluding mutable
SharedValueobjects likekeyboardHeightandlastKeyboardHeightin the dependencies array may cause unintended re-executions of the effect. Consider removing them from the dependencies array as changes to their internal values won't trigger a re-render.
60-65: Reevaluate dependencies in the seconduseEffectSimilar to the previous
useEffect, consider removingkeyboardHeightandlastKeyboardHeightfrom the dependencies array to prevent unnecessary effect executions due to mutable shared values.
55-55: Correct typographical error in commentThere's a typo in the comment on line 55: "to that" should be "so that". Please correct it for clarity.
features/conversation/conversation.tsx (1)
294-296: Clean up commented-out code for better readabilityThere are blocks of commented-out code within
ConversationMessageGestures. Removing or refactoring this code will enhance readability and maintainability. If this code is needed for future reference, consider adding a TODO comment explaining its purpose.Also applies to: 308-316
features/conversation/utils/is-latest-message-settled-from-peer.ts (2)
4-7: Type definitions look good but could use documentation.The type changes make the contract clearer. Consider adding JSDoc to document the purpose and return value.
+/** + * Determines if the current message is the latest settled message from the peer + * @param message The current message being evaluated + * @param nextMessage The next message in the conversation, if any + * @returns boolean indicating if this is the latest settled peer message + */ type IsLatestMessageSettledFromPeerPayload = { message: DecodedMessageWithCodecsType; nextMessage: DecodedMessageWithCodecsType | undefined; };
Line range hint
12-19: Remove redundant null check.Since
messageis now required in the type definition, the initial null check is redundant and can be removed.export const isLatestMessageSettledFromPeer = ({ message, nextMessage, }: IsLatestMessageSettledFromPeerPayload) => { - if (!message) return false; if ( !messageIsFromCurrentUser({ message, }) ) return false;features/conversation/conversation-composer/conversation-composer-container.tsx (2)
5-7: Add explicit return type to memoized component.While TypeScript can infer the return type, it's better to be explicit for component definitions.
-export const ConversationComposerContainer = memo(function ( +export const ConversationComposerContainer = memo(function ConversationComposerContainer( props: IVStackProps -) { +): JSX.Element {
13-24: LGTM! Consider extracting default styles.The implementation correctly handles safe area insets and style merging. Consider extracting the default styles to a constant for better maintainability.
+const defaultStyles = { + justifyContent: "flex-end", + overflow: "hidden", +} as const; return ( <VStack style={[ { paddingBottom: insets.bottom, - justifyContent: "flex-end", - overflow: "hidden", + ...defaultStyles, }, style, ]} {...rest} /> );hooks/use-keyboard-handlers.ts (1)
13-26: Consider memoizing callbacks and adding cleanup.While the implementation is solid, consider these improvements:
- Wrap the callbacks with useCallback to prevent unnecessary re-renders
- Add cleanup logic using useEffect to handle component unmounting
Here's how you could improve it:
+import { useCallback, useEffect } from 'react'; import { runOnJS, KeyboardState, useAnimatedReaction, useAnimatedKeyboard, } from "react-native-reanimated"; export function useKeyboardHandlers(args: IKeyboardHandlersArgs) { const { state } = useAnimatedKeyboard(); + const handleKeyboardOpen = useCallback(args.onKeyboardOpen, [args.onKeyboardOpen]); + const handleKeyboardClose = useCallback(args.onKeyboardClose, [args.onKeyboardClose]); - useAnimatedReaction( + const reaction = useAnimatedReaction( () => state.value, (state) => { if (state === KeyboardState.OPEN) { - runOnJS(args.onKeyboardOpen)(); + runOnJS(handleKeyboardOpen)(); } else if (state === KeyboardState.CLOSED) { - runOnJS(args.onKeyboardClose)(); + runOnJS(handleKeyboardClose)(); } } ); + + useEffect(() => { + return () => { + reaction.cancel(); + }; + }, [reaction]); }hooks/use-keyboard-is-shown-ref.ts (2)
16-22: Remove console.log statements from production code.Console statements should be removed from production code.
(state) => { if (state === KeyboardState.OPEN) { - console.log("keyboard is open"); isKeyboardShown.value = true; } else if (state === KeyboardState.CLOSED) { - console.log("keyboard is closed"); isKeyboardShown.value = false; } }
1-27: Architecture: Consider consolidating keyboard-related hooks.There's significant code duplication across the three keyboard-related hooks. Consider creating a shared base hook and organizing these hooks into a more cohesive structure.
Suggested structure:
- Create a base
useKeyboardStatehook- Have all other keyboard hooks use this base hook
- Consider combining these hooks into a single file or a dedicated keyboard hooks module
- Add proper documentation explaining the differences and use cases for each hook
Would you like me to help create this consolidated structure?
queries/useConversationQuery.ts (1)
Line range hint
50-56: Consider returning query operation in setConversationQueryData for consistencyFor consistency with other query operations in this file, consider returning the query client operation here as well.
export const setConversationQueryData = ( account: string, topic: ConversationTopic, conversation: ConversationQueryData ) => { - queryClient.setQueryData<ConversationQueryData>( + return queryClient.setQueryData<ConversationQueryData>( conversationQueryKey(account, topic), conversation ); };features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-reactors.tsx (1)
108-113: Move hardcoded dimensions to constantsThe hardcoded width (76) should be moved to the constants file alongside MESSAGE_CONTEXT_REACTIONS_HEIGHT for better maintainability.
+ // In conversation-message-context-menu-constant.ts + export const MESSAGE_CONTEXT_REACTIONS_WIDTH = 76; // From iMessage - width: 76, // From iMessage + width: MESSAGE_CONTEXT_REACTIONS_WIDTH,features/conversation/conversation-new-dm.tsx (1)
Line range hint
89-106: Consider implementing the TODO for optimistic updatesThe commented code shows a plan for optimistic updates that would improve UX by making the conversation feel more responsive. This would be particularly beneficial for slower network conditions.
Would you like assistance in implementing the optimistic updates for both conversation creation and message sending?
features/conversation/conversation-composer/conversation-composer.tsx (3)
116-117: Consider extracting magic numbers to theme constantsThe margin and spacing values (6px, etc.) are hardcoded from Figma. Consider moving these to theme constants for better maintainability and consistency.
+ // In theme file + const composer = { + margin: 6, + containerSpacing: { + horizontal: 6, + vertical: 6, + }, + }; - margin: 6, // 6 in the Figma + margin: theme.composer.margin,Also applies to: 129-135
Line range hint
41-54: Consider improving media upload timeout handlingThe current implementation uses a simple polling mechanism with a hardcoded 10-second timeout. Consider:
- Making the timeout configurable
- Using exponential backoff for polling
- Adding proper cleanup on component unmount
+const MEDIA_UPLOAD_TIMEOUT = 10000; +const INITIAL_POLL_INTERVAL = 200; +const MAX_POLL_INTERVAL = 1000; function waitUntilMediaPreviewIsUploaded() { return new Promise((resolve, reject) => { const startTime = Date.now(); + let pollInterval = INITIAL_POLL_INTERVAL; 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); + pollInterval = Math.min(pollInterval * 1.5, MAX_POLL_INTERVAL); + setTimeout(checkStatus, pollInterval); }; checkStatus(); }); }
Line range hint
283-297: Improve keyboard event handling organizationThe keyboard event handling logic could be better organized and more robust:
- Extract platform-specific logic
- Add proper TypeScript types for the event
- Consider handling more keyboard combinations
+type KeyboardEvent = { + nativeEvent: { + key: string; + altKey?: boolean; + metaKey?: boolean; + shiftKey?: boolean; + }; +}; +const handleKeyPress = (event: KeyboardEvent, 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: KeyboardEvent) => handleKeyPress(event, onSubmitEditing)}features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx (1)
Line range hint
253-379: Consider moving experimental code to a separate fileWhile the commented code is well-documented explaining why the native context menu approach wasn't used, keeping large blocks of commented code in the main file can make it harder to maintain. Consider:
- Moving this experimental code to a separate file (e.g.,
conversation-message-context-menu.experimental.tsx)- Or documenting it in a technical design document/wiki
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
features/conversation/conversation-composer/conversation-composer-container.tsx(1 hunks)features/conversation/conversation-composer/conversation-composer-reply-preview.tsx(3 hunks)features/conversation/conversation-composer/conversation-composer.tsx(1 hunks)features/conversation/conversation-keyboard-filler.tsx(1 hunks)features/conversation/conversation-message-status/conversation-message-status.tsx(1 hunks)features/conversation/conversation-message-status/conversation-message-status.utils.ts(1 hunks)features/conversation/conversation-message/conversation-message-content-types/conversation-message-reply.tsx(1 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-reactors.tsx(4 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.store-context.tsx(0 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx(2 hunks)features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.service..ts(0 hunks)features/conversation/conversation-message/conversation-message-reactions/conversation-message-reactions.tsx(4 hunks)features/conversation/conversation-message/conversation-message-status-dumb.tsx(0 hunks)features/conversation/conversation-message/conversation-message-status.tsx(0 hunks)features/conversation/conversation-message/conversation-message.store-context.tsx(3 hunks)features/conversation/conversation-new-dm.tsx(2 hunks)features/conversation/conversation.tsx(8 hunks)features/conversation/utils/is-latest-message-settled-from-peer.ts(1 hunks)hooks/use-current-account-inbox-id.ts(1 hunks)hooks/use-keyboard-handlers.ts(1 hunks)hooks/use-keyboard-is-shown-ref.ts(1 hunks)hooks/use-keyboard-is-shown.ts(1 hunks)ios/Converse.xcodeproj/project.pbxproj(0 hunks)queries/useConversationMessages.ts(2 hunks)queries/useConversationQuery.ts(3 hunks)utils/xmtpRN/conversations.ts(0 hunks)
💤 Files with no reviewable changes (6)
- features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.store-context.tsx
- features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.service..ts
- features/conversation/conversation-message/conversation-message-status.tsx
- features/conversation/conversation-message/conversation-message-status-dumb.tsx
- utils/xmtpRN/conversations.ts
- ios/Converse.xcodeproj/project.pbxproj
🔇 Additional comments (9)
features/conversation/conversation-message/conversation-message-reactions/conversation-message-reactions.tsx (1)
4-7: Imports updated to utilize inbox-centric hooks
The import of isCurrentUserInboxId and useCurrentAccountInboxId aligns with the shift towards an inbox-centric approach and enhances code consistency.
hooks/use-current-account-inbox-id.ts (1)
26-29: Ensure robust handling when comparing inbox IDs
The function isCurrentUserInboxId assumes that inboxId is always defined. If there's a possibility that inboxId could be undefined or null, calling toLowerCase() might cause a runtime error. Similarly, ensure currentUserInboxId is properly handled.
Please verify that inboxId cannot be undefined or null. If there's a chance it might be, consider adding checks to handle these cases safely.
features/conversation/conversation-message-status/conversation-message-status.tsx (1)
116-118: Ensure all message.status keys are handled
When rendering {renderText && statusMapping[message.status]}, ensure that all possible message.status values are included in statusMapping to prevent rendering undefined. Consider providing a default value or handling unexpected statuses gracefully.
features/conversation/conversation-message-status/conversation-message-status.utils.ts (1)
4-6: LGTM! Verify SDK constant usage across the codebase.
The implementation is clean and type-safe. However, let's verify the consistency of MessageDeliveryStatus.PUBLISHED usage.
✅ Verification successful
Let me verify the usage of MessageDeliveryStatus.PUBLISHED more thoroughly by checking the type definitions and any potential enum declarations.
Let me try a different approach to check the imports and usage patterns.
MessageDeliveryStatus.PUBLISHED is used consistently across the codebase
The verification shows that:
- MessageDeliveryStatus is properly imported from "@xmtp/react-native-sdk"
- All occurrences consistently use MessageDeliveryStatus.PUBLISHED for delivery status checks
- The enum is used in related conversation features (reactions, message sending) with the same pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of MessageDeliveryStatus.PUBLISHED
# across the codebase to ensure we're handling message states uniformly
rg -A 2 "MessageDeliveryStatus\.PUBLISHED"
Length of output: 1216
Script:
#!/bin/bash
# Check for MessageDeliveryStatus type/enum definition
ast-grep --pattern 'enum MessageDeliveryStatus'
# Also search for any type imports or references
rg "MessageDeliveryStatus" --type typescript
Length of output: 129
Script:
#!/bin/bash
# Search for MessageDeliveryStatus imports and type definitions
rg "MessageDeliveryStatus"
# Check the specific file for imports
cat features/conversation/conversation-message-status/conversation-message-status.utils.ts
Length of output: 1209
queries/useConversationQuery.ts (1)
34-36: LGTM: Explicit return statements for query operations
The addition of return statements for query client operations is a good practice, allowing callers to properly handle the promises returned by these operations.
Also applies to: 44-47, 73-77
features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-reactors.tsx (1)
Line range hint 52-81: LGTM: Clean animation implementation using theme-based animations
The animation implementation using theme.animation.reanimatedFadeInScaleIn() is clean and consistent with the theme system.
features/conversation/conversation-message/conversation-message.store-context.tsx (1)
25-25: LGTM: Clean integration of message settlement status
The addition of isLatestSettledFromMe state and its computation using the isLatestMessageSettledFromPeer utility is well-structured and maintains type safety.
Also applies to: 68-71
features/conversation/conversation-new-dm.tsx (1)
4-4: LGTM! Clean integration of ConversationComposerContainer
The new container component is properly imported and integrated, providing a consistent wrapper for the composer interface.
Also applies to: 56-59
features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx (1)
157-157: Verify ConversationMessage styling in context menu
The replacement of the previous message component with ConversationMessage looks good, but please verify:
- That
ConversationMessageproperly handles all styling requirements within the context menu - Whether the removal of the commented VStack height styling affects the layout spacing
✅ Verification successful
Based on the shell script results, I can now generate the final response:
ConversationMessage component is properly integrated for context menu
The ConversationMessage component is a well-structured component that:
- Handles all message types through appropriate sub-components (MessageSimpleText, MessageReply, MessageRemoteAttachment, etc.)
- Is already used successfully in multiple contexts including the main conversation view and read-only views
- The removal of VStack height styling is safe as the component is wrapped in ConversationMessageLayout which handles proper spacing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ConversationMessage styling and usage patterns
echo "Searching for ConversationMessage styling..."
ast-grep --pattern 'const ConversationMessage = $$$'
echo "Searching for other ConversationMessage usages..."
rg 'ConversationMessage' -A 5
Length of output: 61530
...ation/conversation-message/conversation-message-reactions/conversation-message-reactions.tsx
Show resolved
Hide resolved
features/conversation/conversation-message-status/conversation-message-status.tsx
Outdated
Show resolved
Hide resolved
features/conversation/conversation-message-status/conversation-message-status.tsx
Outdated
Show resolved
Hide resolved
...ion-message/conversation-message-context-menu/conversation-message-context-menu-reactors.tsx
Outdated
Show resolved
Hide resolved
features/conversation/conversation-composer/conversation-composer-reply-preview.tsx
Show resolved
Hide resolved
features/conversation/conversation-composer/conversation-composer-reply-preview.tsx
Outdated
Show resolved
Hide resolved
...conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx
Outdated
Show resolved
Hide resolved
thierryskoda
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
features/conversation/hooks/use-send-message.ts (2)
Line range hint
223-223: Consistently handle message types without suppressing TypeScript errorsThe
@ts-ignoredirective at line 223 is used to accessmessage.deliveryStatus, which TypeScript doesn't recognize on theDecodedMessagetype. To maintain type safety and avoid suppressing errors, consider defining a custom message type that includes thedeliveryStatusproperty, similar to how you've handled it inuse-send-message.ts.Here's how you can modify the code:
+ interface DecodedMessageWithStatus<T> extends DecodedMessage<T> { + deliveryStatus?: IConversationMessageStatus; + } - export function getConvosMessageStatus(message: DecodedMessageWithCodecsType) { + export function getConvosMessageStatus(message: DecodedMessageWithStatus<SupportedCodecsType>) { if (message.deliveryStatus === "sending") { return "sending"; } // Rest of the function... }
Line range hint
235-235: Incorrect use ofsatisfiesoperator in return statementIn line 235, the
satisfiesoperator is used within a return statement, which is invalid syntax. Thesatisfiesoperator is a type-level construct and cannot be used in runtime expressions. To handle unhandled delivery statuses, consider throwing an error or returning a default value.Apply this diff to correct the syntax:
- return message.deliveryStatus satisfies never; + throw new Error(`Unhandled delivery status: ${message.deliveryStatus}`);
🧹 Nitpick comments (11)
hooks/use-previous-value.ts (1)
3-3: Add JSDoc documentation for better maintainability.Consider adding JSDoc documentation to describe the hook's purpose, parameters, and return value.
+/** + * Custom hook that returns the previous value of a variable from the last render. + * @template T The type of the value to track + * @param {T} value The value to track + * @returns {T | undefined} The previous value (undefined on first render) + * @example + * const prevCount = usePrevious(count); + */ export function usePrevious<T>(value: T): T | undefined {features/conversation/conversation-message/conversation-message-layout.tsx (3)
2-2: Consider removing unused debug importsThe
debugBorderutility is only used in a commented-out line. Consider removing both the import and the commented usage to keep the codebase clean, or document why this debugging capability needs to be retained.Also applies to: 8-8
39-48: Consider performance and accessibility optimizationsWhile the animation enhancement improves the UI, consider these suggestions:
- Add a check for
prefers-reduced-motionmedia query to respect user preferences- Consider performance implications when rendering large message lists with layout animations
Here's a suggested implementation:
<AnimatedVStack // {...debugBorder()} - layout={theme.animation.reanimatedLayoutSpringTransition} + layout={shouldReduceMotion ? undefined : theme.animation.reanimatedLayoutSpringTransition} style={{ rowGap: theme.spacing["4xs"], alignItems: fromMe ? "flex-end" : "flex-start", }} >Add this hook at the top of your component:
const shouldReduceMotion = useReducedMotion(); // Create this hook using react-native-reanimated
Line range hint
15-53: Consider splitting layout and animation concernsThe component currently handles both layout structure and animation logic. Consider extracting the animation logic into a separate component or hook for better separation of concerns and reusability across the conversation feature.
This would make it easier to:
- Test layout logic independently
- Reuse animation patterns
- Maintain consistent animations across the app
utils/xmtpRN/messages.ts (1)
40-45: Consider adding error handling for message processing.The message processing could benefit from error handling to ensure robustness.
Consider wrapping the message processing in a try-catch block:
if (isMessageFromOtherUser || isNonTextMessage) { + try { addConversationMessage({ account: client.address, topic: message.topic as ConversationTopic, message, }); + } catch (error) { + logger.error('[XmtpRN] Failed to add conversation message', { + error, + messageId: message.id, + topic: message.topic, + }); + } }features/conversation/conversation.tsx (3)
190-196: Consider optimizing latest message lookupThe current implementation uses
findto search through all messages. Since messages are typically ordered by time, consider optimizing this lookup by checking from the start of the array until the first match is found.const latestMessageIdByCurrentUser = useMemo(() => { if (!messages?.ids) return -1; - return messages.ids.find( - (messageId) => - messages.byId[messageId].senderAddress === currentAccountInboxId - ); + // Messages are typically ordered by time, so we can optimize by checking from start + for (const messageId of messages.ids) { + if (messages.byId[messageId].senderAddress === currentAccountInboxId) { + return messageId; + } + } + return -1; }, [messages?.ids, messages?.byId, currentAccountInboxId]);
Line range hint
318-340: Remove commented code and document refactoring plansThe commented code and TODOs suggest incomplete refactoring. Consider:
- Remove the commented code if it's no longer needed
- If there are plans for refactoring, document them in a separate issue/ticket
- // const message = messageStore.getState().message; - // const previousMessage = messageStore.getState().previousMessage; - // const nextMessage = messageStore.getState().nextMessage; messageContextMenuStore.getState().setMessageContextMenuData({ messageId, itemRectX: e.pageX, itemRectY: e.pageY, itemRectHeight: e.height, itemRectWidth: e.width, - // Need to have MessageContextStoreProvider here. - // Not the cleanest... - // Might want to find another solution later but works for now. - // Solution might be to remove the context and just pass props - // messageComponent: ( - // <MessageContextStoreProvider - // message={message} - // previousMessage={previousMessage} - // nextMessage={nextMessage} - // > - // {children} - // </MessageContextStoreProvider> - // ), });Would you like me to create a GitHub issue to track the planned refactoring of the MessageContextStoreProvider implementation?
Line range hint
1-405: LGTM: Well-architected conversation componentThe overall architecture demonstrates:
- Proper component composition and separation of concerns
- Effective use of React patterns (memo, context, hooks)
- Clean state management
Consider adding prop-types or TypeScript interfaces for better type safety and documentation of the component APIs.
queries/useConversationMessages.ts (3)
Line range hint
108-240: Consider performance optimizations for message processingThe message processing logic handles complex scenarios well but could benefit from performance optimizations when dealing with large message sets.
Consider these optimizations:
- Use
Mapinstead of plain objects forbyIdand reactions lookups:type IMessageAccumulator = { ids: MessageId[]; - byId: Record<MessageId, DecodedMessageWithCodecsType>; + byId: Map<MessageId, DecodedMessageWithCodecsType>; reactions: Map< MessageId, { - bySender: Record<InboxId, ReactionContent[]>; - byReactionContent: Record<string, InboxId[]>; + bySender: Map<InboxId, ReactionContent[]>; + byReactionContent: Map<string, InboxId[]>; } >; };
- Process messages in chunks for large datasets:
const CHUNK_SIZE = 100; function processMessagesInChunks(messages: DecodedMessageWithCodecsType[]) { return messages.reduce((acc, message, index) => { if (index % CHUNK_SIZE === 0) { // Process in the next tick to avoid blocking setTimeout(() => processChunk(messages.slice(index, index + CHUNK_SIZE)), 0); } return acc; }, initialAccumulator); }
241-244: Remove WIP comment and complete the interfaceThe
IOptimisticMessageinterface is marked as WIP and appears incomplete.Consider completing the interface:
-// WIP type IOptimisticMessage = { tempId: string; messageId?: MessageId; + status: 'pending' | 'sent' | 'failed'; + timestamp: number; };
247-298: Consider adding rollback handling for failed message replacementsThe optimistic update replacement is well implemented but could benefit from rollback handling for failed cases.
Consider adding rollback capability:
export function replaceOptimisticMessageWithReal(args: { tempId: string; topic: ConversationTopic; account: string; message: DecodedMessageWithCodecsType; + onError?: (error: Error) => void; }) { - const { tempId, topic, account, message } = args; + const { tempId, topic, account, message, onError } = args; logger.info( "[linkOptimisticMessageToReal] linking optimistic message to real", { tempId, messageId: message.id, } ); + const previousData = queryClient.getQueryData<ConversationMessagesQueryData>( + conversationMessagesQueryKey(account, topic) + ); + try { queryClient.setQueryData<ConversationMessagesQueryData>( conversationMessagesQueryKey(account, topic), (previousMessages) => { // ... existing logic ... } ); + } catch (error) { + logger.error("[replaceOptimisticMessageWithReal] Failed to replace message", { error, tempId }); + if (previousData) { + queryClient.setQueryData( + conversationMessagesQueryKey(account, topic), + previousData + ); + } + onError?.(error as Error); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
features/conversation/conversation-message-status/conversation-message-status.tsx(1 hunks)features/conversation/conversation-message/conversation-message-content-container.tsx(1 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx(4 hunks)features/conversation/conversation-message/conversation-message-layout.tsx(2 hunks)features/conversation/conversation-message/conversation-message.store-context.tsx(3 hunks)features/conversation/conversation-message/conversation-message.types.ts(1 hunks)features/conversation/conversation-message/conversation-message.utils.tsx(3 hunks)features/conversation/conversation.tsx(13 hunks)features/conversation/hooks/use-send-message.ts(2 hunks)hooks/use-current-account-inbox-id.ts(2 hunks)hooks/use-previous-value.ts(1 hunks)queries/useConversationMessage.ts(1 hunks)queries/useConversationMessages.ts(4 hunks)utils/xmtpRN/messages.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- features/conversation/conversation-message/conversation-message-content-container.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- features/conversation/conversation-message-status/conversation-message-status.tsx
- features/conversation/conversation-message/conversation-message.store-context.tsx
- hooks/use-current-account-inbox-id.ts
- features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx
🔇 Additional comments (11)
hooks/use-previous-value.ts (1)
3-11: LGTM! Clean and efficient implementation.
The hook follows React best practices with proper TypeScript typing, efficient use of useRef and useEffect, and minimal re-renders.
utils/xmtpRN/messages.ts (2)
33-39: Well-structured message filtering logic.
The new filtering logic is clear and well-documented. The use of descriptive boolean flags (isMessageFromOtherUser and isNonTextMessage) makes the code more maintainable and easier to understand.
9-12: Verify the necessity of both message verification utilities.
The code imports both messageIsFromCurrentUser and messageIsFromCurrentUserV3, but only uses the V3 version. This might indicate an incomplete migration.
features/conversation/conversation.tsx (4)
8-9: LGTM: New imports are well-organized
The new imports are properly structured and align with the feature additions for conversation composer, message status, and keyboard handling.
Also applies to: 19-19, 25-25, 27-30, 35-40, 46-46
151-156: LGTM: Clean keyboard filler implementation
The KeyboardFillerWrapper is well-implemented with proper memoization and context usage. It correctly handles keyboard behavior based on the message context menu state.
166-171: LGTM: Well-structured composer layout
The composition of ReplyPreview and Composer within ConversationComposerContainer provides a clean and maintainable structure.
Line range hint 250-297: LGTM: Well-implemented message status display
The conditional rendering of ConversationMessageStatus for the latest message is clean and properly implemented. The status is correctly derived from the message data.
features/conversation/hooks/use-send-message.ts (1)
83-83:
Incorrect use of satisfies operator in object literal
In line 83, the satisfies operator is used within an object literal, which is not valid syntax. The satisfies operator is a TypeScript type assertion used during variable assignments and cannot be used inside object property assignments. To ensure correct typing, you can remove the satisfies operator and rely on TypeScript's type inference.
Apply this diff to correct the syntax:
- deliveryStatus: "sending" satisfies IConversationMessageStatus,
+ deliveryStatus: "sending",Likely invalid or redundant comment.
features/conversation/conversation-message/conversation-message.types.ts (1)
1-1: Definition of IConversationMessageStatus is appropriate
The introduction of the IConversationMessageStatus type correctly defines the possible statuses of a conversation message as "sending", "sent", or "error". This enhances type safety and clarity in message status handling.
queries/useConversationMessage.ts (1)
49-51: Addition of fetchMessageByIdQuery function is correct
The new fetchMessageByIdQuery function properly utilizes queryClient.fetchQuery with the appropriate query options to fetch a message by its ID. This addition enhances the ability to retrieve messages when needed.
queries/useConversationMessages.ts (1)
53-77: LGTM! Well-structured query hook and utility functions
The hook and utility functions are well-implemented, following React Query patterns with proper typing and error handling.
features/conversation/conversation-message/conversation-message.utils.tsx
Outdated
Show resolved
Hide resolved
features/conversation/conversation-message/conversation-message.utils.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
features/conversation/conversation-message/conversation-message.utils.tsx (1)
227-227: Consider renaming for consistencyThe function name
getConvosMessageStatususes an abbreviated form ("Convos"), while other functions in this file use the full word "Conversation". Consider renaming togetConversationMessageStatusfor consistency.features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-reactors.tsx (2)
Line range hint
35-44: Consider enhancing type safety in the reactor mapping.The mapping logic is well-structured and efficient. However, the type assertion to
InboxIdcould be handled more safely.Consider this safer approach:
- reactionMap[getReactionContent(reaction)].push( - reactorInboxId as InboxId - ); + if (typeof reactorInboxId === 'string') { + reactionMap[getReactionContent(reaction)].push(reactorInboxId as InboxId); + }
107-112: Consider moving hardcoded dimensions to theme system.The hardcoded width value (76) should be moved to the theme system for better maintainability and consistency across the application.
Consider moving these values to your theme:
- width: 76, // From iMessage + width: theme.spacing.contextMenuItemWidth,queries/useConversationMessages.ts (4)
50-54: Enhance error message with contextThe error message should include the account and topic information to aid in debugging.
- throw new Error( - "Conversation not found in conversationMessagesByTopicQueryFn" - ); + throw new Error( + `Conversation not found for account ${account} and topic ${topic}` + );
246-249: Complete or remove WIP typeThe
IOptimisticMessagetype is marked as WIP and seems incomplete. Either complete the type definition or remove it if unused.
259-264: Fix inconsistent function name in log messageThe log message uses
linkOptimisticMessageToRealbut the function is namedreplaceOptimisticMessageWithReal.logger.info( - "[linkOptimisticMessageToReal] linking optimistic message to real", + "[replaceOptimisticMessageWithReal] replacing optimistic message with real", { tempId, messageId: message.id, } );
Line range hint
1-303: Consider implementing a message queue for reliabilityThe current implementation handles optimistic updates and message processing directly. Consider implementing a message queue pattern to handle message operations more reliably, especially for handling failures and retries in poor network conditions.
This would involve:
- Queuing messages before sending
- Handling retries automatically
- Managing message state transitions
- Providing better error recovery
Would you like me to provide a detailed implementation proposal for this pattern?
features/conversation/conversation-keyboard-filler.tsx (2)
27-50: Consider simplifying keyboard state management logicThe keyboard state management logic is complex and could be simplified to prevent potential race conditions between the two useEffect hooks.
Consider consolidating the keyboard state management into a single useEffect:
- useEffect(() => { - if (!messageContextMenuIsOpen) { - if (keyboardWasOpenRef.current) { - textInputRef.current?.focus(); - } - } else { - if (isKeyboardShown) { - Keyboard.dismiss(); - lastKeyboardHeight.value = keyboardHeight.value; - keyboardWasOpenRef.current = true; - } else { - keyboardWasOpenRef.current = false; - } - } - }, [messageContextMenuIsOpen, isKeyboardShown, keyboardHeight, lastKeyboardHeight]); + useEffect(() => { + const handleKeyboardState = () => { + if (messageContextMenuIsOpen) { + if (isKeyboardShown) { + lastKeyboardHeight.value = keyboardHeight.value; + keyboardWasOpenRef.current = true; + Keyboard.dismiss(); + } + } else if (keyboardWasOpenRef.current && !isKeyboardShown) { + textInputRef.current?.focus(); + keyboardWasOpenRef.current = false; + lastKeyboardHeight.value = 0; + } + }; + + handleKeyboardState(); + }, [messageContextMenuIsOpen, isKeyboardShown, keyboardHeight, lastKeyboardHeight]);
78-81: Add accessibility properties to hidden TextInputThe hidden TextInput should have proper accessibility properties to prevent screen readers from attempting to interact with it.
<TextInput ref={textInputRef} style={{ height: 0, width: 0, opacity: 0, position: "absolute" }} + accessibilityElementsHidden={true} + importantForAccessibility="no" />features/conversation/conversation.tsx (2)
Line range hint
322-344: Remove commented codeThere's a significant block of commented-out code that should be removed if it's no longer needed. If this code represents a potential future implementation, it should be tracked in a separate issue.
- // const message = messageStore.getState().message; - // const previousMessage = messageStore.getState().previousMessage; - // const nextMessage = messageStore.getState().nextMessage; messageContextMenuStore.getState().setMessageContextMenuData({ messageId, itemRectX: e.pageX, itemRectY: e.pageY, itemRectHeight: e.height, itemRectWidth: e.width, - // Need to have MessageContextStoreProvider here. - // Not the cleanest... - // Might want to find another solution later but works for now. - // Solution might be to remove the context and just pass props - // messageComponent: ( - // <MessageContextStoreProvider - // message={message} - // previousMessage={previousMessage} - // nextMessage={nextMessage} - // > - // {children} - // </MessageContextStoreProvider> - // ), });
289-304: Consider extracting message content layout to a separate componentThe message content layout logic is becoming complex with multiple conditional renders. Consider extracting it to a separate component for better maintainability.
const MessageContent = memo(({ message, isLatestMessageSentByCurrentUser, onReply }: MessageContentProps) => ( <VStack> <ConversationMessageTimestamp /> <ConversationMessageRepliable onReply={onReply}> <ConversationMessageLayout> <ConversationMessageGestures> <ConversationMessage message={message} /> </ConversationMessageGestures> <ConversationMessageReactions /> {isLatestMessageSentByCurrentUser && ( <ConversationMessageStatus status={getConvosMessageStatus(message)} /> )} </ConversationMessageLayout> </ConversationMessageRepliable> </VStack> ));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
features/conversation/conversation-composer/conversation-composer-reply-preview.tsx(2 hunks)features/conversation/conversation-keyboard-filler.tsx(1 hunks)features/conversation/conversation-message-status/conversation-message-status.tsx(1 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-reactors.tsx(5 hunks)features/conversation/conversation-message/conversation-message.utils.tsx(3 hunks)features/conversation/conversation.tsx(13 hunks)queries/useConversationMessages.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- features/conversation/conversation-message-status/conversation-message-status.tsx
- features/conversation/conversation-composer/conversation-composer-reply-preview.tsx
🔇 Additional comments (7)
features/conversation/conversation-message/conversation-message.utils.tsx (3)
23-23: LGTM: Import changes are appropriate
The addition of MessageDeliveryStatus and repositioning of imports maintain code organization while supporting the new functionality.
Also applies to: 36-36
228-229: Avoid using @ts-ignore; extend DecodedMessage type to include deliveryStatus
The @ts-ignore directive suppresses TypeScript errors when accessing message.deliveryStatus. This issue was previously raised and should be addressed for better type safety.
227-243: LGTM: Well-structured message status handling
The function implementation:
- Properly handles optimistic updates with the "sending" status
- Correctly maps all MessageDeliveryStatus enum values
- Includes appropriate error handling for unexpected status values
features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-reactors.tsx (2)
Line range hint 52-81: LGTM! Well-structured animation and layout implementation.
The AnimatedVStack implementation is clean, properly handles safe areas, and correctly implements event handling with pointerEvents.
97-103: LGTM! Correct implementation of membersSocials mapping.
The mapping now correctly uses the index from the map function, addressing the potential bug mentioned in previous reviews.
queries/useConversationMessages.ts (1)
33-35: 🛠️ Refactor suggestion
Remove redundant null check
This null check is redundant since conversationMessagesByTopicQueryFn already performs the check before calling this function.
- if (!conversation) {
- throw new Error("Conversation not found in conversationMessagesQueryFn");
- }Likely invalid or redundant comment.
features/conversation/conversation.tsx (1)
297-301: Verify message status visibility logic
The message status is only shown for the latest message sent by the current user. Verify if this is the intended behavior as users might want to see the status of all their sent messages.
✅ Verification successful
Message status visibility is correctly implemented
The current implementation showing message status only for the latest message is the correct behavior because:
- The message status types ("sending", "sent", "error") are transient states primarily relevant for the most recent message
- The implementation includes haptic feedback for status transitions, which would be disruptive if applied to all messages
- The status is tightly coupled with the message delivery system, focusing on real-time delivery confirmation for new messages
The code follows common messaging UI patterns where delivery status is most relevant for the latest sent message, similar to popular messaging apps like WhatsApp or iMessage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other implementations of message status display
rg -A 2 "ConversationMessageStatus|getConvosMessageStatus" --type ts --type tsx
# Check if there are any related configuration or feature flags
rg -l "messageStatus|MessageStatus" --type json
Length of output: 159
Script:
#!/bin/bash
# Search for message status implementations with correct file extensions
rg -A 2 "ConversationMessageStatus|getConvosMessageStatus" -t typescript -t tsx
# Look for message status related code patterns
ast-grep --pattern 'const ConversationMessageStatus = $_'
# Search for message status related configurations
rg -l "messageStatus|MessageStatus" --type-add 'typescript:*.{ts,tsx}' -t typescript
Length of output: 4465
Script:
#!/bin/bash
# Check the message status implementation in conversation message utils
rg -A 10 "getConvosMessageStatus" features/conversation/conversation-message/conversation-message.utils.tsx
# Check how message status is used in the conversation message component
rg "MessageStatus" features/conversation/conversation-message/conversation-message.types.ts
# Look for message sending logic to understand status handling
rg -A 5 "messageStatus|MessageStatus" features/conversation/hooks/use-send-message.ts
Length of output: 1391
| const { theme } = useAppTheme(); | ||
|
|
||
| const animatedValue = useSharedValue(0); | ||
| const currentAccount = useCurrentAccount()!; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential null currentAccount more safely.
The non-null assertion (!) on currentAccount could lead to runtime errors if the account is not available.
Consider this safer approach:
- const currentAccount = useCurrentAccount()!;
+ const currentAccount = useCurrentAccount();
+ if (!currentAccount) {
+ return null; // or a loading state
+ }Committable suggestion skipped: line range outside the PR's diff.
| export const getConversationMessages = ( | ||
| account: string, | ||
| topic: ConversationTopic | ||
| ) => { | ||
| return queryClient.getQueryData<ConversationMessagesQueryData>( | ||
| getConversationMessagesQueryOptions(account, topic).queryKey | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and type safety to getConversationMessages
The function should handle the case when query data is undefined and provide a clear return type.
export const getConversationMessages = (
account: string,
topic: ConversationTopic
-) => {
+): ConversationMessagesQueryData | undefined => {
+ if (!account || !topic) {
+ logger.warn("[getConversationMessages] Missing required parameters", { account, topic });
+ return undefined;
+ }
return queryClient.getQueryData<ConversationMessagesQueryData>(
getConversationMessagesQueryOptions(account, topic).queryKey
);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getConversationMessages = ( | |
| account: string, | |
| topic: ConversationTopic | |
| ) => { | |
| return queryClient.getQueryData<ConversationMessagesQueryData>( | |
| getConversationMessagesQueryOptions(account, topic).queryKey | |
| ); | |
| }; | |
| export const getConversationMessages = ( | |
| account: string, | |
| topic: ConversationTopic | |
| ): ConversationMessagesQueryData | undefined => { | |
| if (!account || !topic) { | |
| logger.warn("[getConversationMessages] Missing required parameters", { account, topic }); | |
| return undefined; | |
| } | |
| return queryClient.getQueryData<ConversationMessagesQueryData>( | |
| getConversationMessagesQueryOptions(account, topic).queryKey | |
| ); | |
| }; |
| export function refetchConversationMessages( | ||
| account: string, | ||
| topic: ConversationTopic | ||
| ) { | ||
| logger.info("[refetchConversationMessages] refetching messages"); | ||
| return queryClient.refetchQueries( | ||
| getConversationMessagesQueryOptions(account, topic) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to refetchConversationMessages
The function should handle potential errors during refetch operation.
export function refetchConversationMessages(
account: string,
topic: ConversationTopic
-) {
+): Promise<void> {
logger.info("[refetchConversationMessages] refetching messages");
- return queryClient.refetchQueries(
- getConversationMessagesQueryOptions(account, topic)
- );
+ try {
+ return queryClient.refetchQueries(
+ getConversationMessagesQueryOptions(account, topic)
+ );
+ } catch (error) {
+ logger.error("[refetchConversationMessages] Failed to refetch", { error, account, topic });
+ throw error;
+ }
}📝 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 refetchConversationMessages( | |
| account: string, | |
| topic: ConversationTopic | |
| ) { | |
| logger.info("[refetchConversationMessages] refetching messages"); | |
| return queryClient.refetchQueries( | |
| getConversationMessagesQueryOptions(account, topic) | |
| ); | |
| } | |
| export function refetchConversationMessages( | |
| account: string, | |
| topic: ConversationTopic | |
| ): Promise<void> { | |
| logger.info("[refetchConversationMessages] refetching messages"); | |
| try { | |
| return queryClient.refetchQueries( | |
| getConversationMessagesQueryOptions(account, topic) | |
| ); | |
| } catch (error) { | |
| logger.error("[refetchConversationMessages] Failed to refetch", { error, account, topic }); | |
| throw error; | |
| } | |
| } |
| export function replaceOptimisticMessageWithReal(args: { | ||
| tempId: string; | ||
| topic: ConversationTopic; | ||
| account: string; | ||
| message: DecodedMessageWithCodecsType; | ||
| }) { | ||
| const { tempId, topic, account, message } = args; | ||
| logger.info( | ||
| "[linkOptimisticMessageToReal] linking optimistic message to real", | ||
| { | ||
| tempId, | ||
| messageId: message.id, | ||
| } | ||
| ); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation
The function should validate its input parameters before processing.
export function replaceOptimisticMessageWithReal(args: {
tempId: string;
topic: ConversationTopic;
account: string;
message: DecodedMessageWithCodecsType;
}) {
const { tempId, topic, account, message } = args;
+
+ if (!tempId || !topic || !account || !message) {
+ logger.error("[replaceOptimisticMessageWithReal] Missing required parameters", args);
+ return;
+ }
+
logger.info(
"[replaceOptimisticMessageWithReal] replacing optimistic message with real",
{
tempId,
messageId: message.id,
}
);📝 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 replaceOptimisticMessageWithReal(args: { | |
| tempId: string; | |
| topic: ConversationTopic; | |
| account: string; | |
| message: DecodedMessageWithCodecsType; | |
| }) { | |
| const { tempId, topic, account, message } = args; | |
| logger.info( | |
| "[linkOptimisticMessageToReal] linking optimistic message to real", | |
| { | |
| tempId, | |
| messageId: message.id, | |
| } | |
| ); | |
| export function replaceOptimisticMessageWithReal(args: { | |
| tempId: string; | |
| topic: ConversationTopic; | |
| account: string; | |
| message: DecodedMessageWithCodecsType; | |
| }) { | |
| const { tempId, topic, account, message } = args; | |
| if (!tempId || !topic || !account || !message) { | |
| logger.error("[replaceOptimisticMessageWithReal] Missing required parameters", args); | |
| return; | |
| } | |
| logger.info( | |
| "[replaceOptimisticMessageWithReal] linking optimistic message to real", | |
| { | |
| tempId, | |
| messageId: message.id, | |
| } | |
| ); |
| const latestMessageIdByCurrentUser = useMemo(() => { | ||
| if (!messages?.ids) return -1; | ||
| return messages.ids.find( | ||
| (messageId) => | ||
| isAnActualMessage(messages.byId[messageId]) && | ||
| messages.byId[messageId].senderAddress === currentAccountInboxId | ||
| ); | ||
| }, [messages?.ids, messages?.byId, currentAccountInboxId]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize message filtering performance
The latestMessageIdByCurrentUser calculation could be performance intensive for large message lists as it searches through all messages on every render.
Consider using a reverse find or maintaining a separate tracker for the latest message:
const latestMessageIdByCurrentUser = useMemo(() => {
if (!messages?.ids) return -1;
- return messages.ids.find(
+ return messages.ids.findLast(
(messageId) =>
isAnActualMessage(messages.byId[messageId]) &&
messages.byId[messageId].senderAddress === currentAccountInboxId
);
}, [messages?.ids, messages?.byId, currentAccountInboxId]);📝 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 latestMessageIdByCurrentUser = useMemo(() => { | |
| if (!messages?.ids) return -1; | |
| return messages.ids.find( | |
| (messageId) => | |
| isAnActualMessage(messages.byId[messageId]) && | |
| messages.byId[messageId].senderAddress === currentAccountInboxId | |
| ); | |
| }, [messages?.ids, messages?.byId, currentAccountInboxId]); | |
| const latestMessageIdByCurrentUser = useMemo(() => { | |
| if (!messages?.ids) return -1; | |
| return messages.ids.findLast( | |
| (messageId) => | |
| isAnActualMessage(messages.byId[messageId]) && | |
| messages.byId[messageId].senderAddress === currentAccountInboxId | |
| ); | |
| }, [messages?.ids, messages?.byId, currentAccountInboxId]); |
There was a problem hiding this comment.
Actionable comments posted: 20
🔭 Outside diff range comments (10)
queries/useBlockGroupMutation.ts (2)
Line range hint
13-19: Consider improving type safety in the function signatureThe topic parameter is marked as required in the function signature but checked for nullity in mutationFn. This creates a disconnect between the API contract and implementation.
Consider this improvement:
-export const useBlockGroupMutation = ( - account: string, - topic: ConversationTopic -) => { +export const useBlockGroupMutation = (params: { + account: string; + topic: ConversationTopic; +}) => { + const { account, topic } = params;
Line range hint
20-28: Improve return type consistency in mutationFnThe mutation function returns either undefined or "denied", which could lead to unexpected behavior.
Consider making the return type explicit and consistent:
mutationFn: async () => { if (!topic || !account) { - return; + throw new Error('Missing required parameters: topic or account'); } await consentToGroupsOnProtocolByAccount({ account, groupIds: [getV3IdFromTopic(topic)], consent: "deny", }); return "denied"; },features/GroupInvites/joinGroup/JoinGroup.client.ts (2)
Line range hint
271-272: Consider implementing a fixture factory for type-safe test data.The current implementation has multiple TODOs and @ts-expect-error annotations related to fixture typing. This could be improved by:
- Creating a dedicated fixture factory
- Using TypeScript's utility types for partial implementations
- Implementing proper type guards
Here's a suggested approach:
// types/fixtures.ts import { ConversationDataEntity, Conversation } from '@/types'; export class ConversationFixtureFactory { static createConversation(override?: Partial<Conversation>): Conversation { return { id: 'default-id', createdAt: new Date().getTime(), members: async () => [], topic: 'default-topic' as ConversationTopic, // ... other required fields ...override }; } static createConversationDataEntity( conversations: Conversation[] ): ConversationDataEntity { return { ids: conversations.map(c => c.id), byId: Object.fromEntries( conversations.map(c => [c.id, c]) ) }; } }Also applies to: 365-366, 449-450
Dynamic import pattern needs to be updated for consistency
The verification reveals that most query imports in the codebase use static imports, while this file uses dynamic imports. Only a few instances of dynamic imports exist in the same file, suggesting an inconsistent pattern.
features/GroupInvites/joinGroup/JoinGroup.client.ts: Should be updated to use static import like other files:import { fetchConversationListQuery } from "@/queries/useConversationListQuery";🔗 Analysis chain
Line range hint
95-106: Implementation looks good, verify dynamic import consistency.The updated implementation:
- Correctly uses dynamic import pattern
- Properly transforms the response using entify
- Maintains type safety with ConversationDataEntity
Let's verify the consistency of dynamic import usage across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of dynamic imports for query functions # Expected: Similar dynamic import patterns for other query functions # Search for dynamic imports of query functions rg -A 2 "const \{ \w+Query \} = await import\("Length of output: 318
Script:
#!/bin/bash # Let's search for other query imports to verify the pattern consistency rg -A 2 "import.*Query.*from.*queries" # Also check for any other dynamic imports to understand the broader pattern rg -A 2 "const \{ \w+ \} = await import\("Length of output: 20586
features/notifications/utils/onInteractWithNotification.ts (2)
Line range hint
14-63: Add error handling for JSON parsing and type safety improvementsThe notification data processing could be more robust:
- JSON.parse could throw an error if the string is malformed
- Type casting to
Record<string, any>reduces type safetyConsider applying these improvements:
if ( notificationData && typeof notificationData === "object" && "body" in notificationData && typeof notificationData["body"] === "string" ) { - notificationData = JSON.parse(notificationData.body) as Record<string, any>; + try { + notificationData = JSON.parse(notificationData.body) as NotificationPayload; // Define a proper type + } catch (error) { + console.error('Failed to parse notification data:', error); + return; + } }
Line range hint
64-82: Consider handling race conditions in async operationsThe function updates global state synchronously before waiting for the async conversation list fetch to complete. This could lead to race conditions or inconsistent state.
Consider this safer approach:
if (conversationTopic) { const account = notificationData["account"] || useAccountsStore.getState().currentAccount; - // Fetch the conversation list to ensure we have the latest conversation list - // before navigating to the conversation - await fetchPersistedConversationListQuery(account); - useAccountsStore.getState().setCurrentAccount(account, false); + try { + // Set account first to ensure consistent state + useAccountsStore.getState().setCurrentAccount(account, false); + // Then fetch conversation list + await fetchPersistedConversationListQuery(account); + // Only navigate after both operations succeed + navigateToTopic(conversationTopic as ConversationTopic); + setTopicToNavigateTo(undefined); + resetNotifications(); + } catch (error) { + console.error('Failed to process notification:', error); + // Consider showing user feedback + } - navigateToTopic(conversationTopic as ConversationTopic); - setTopicToNavigateTo(undefined); - resetNotifications(); }queries/groupConsentMutationUtils.ts (1)
Line range hint
65-98: Refactor to reduce code duplicationThere's significant code duplication between
createGroupConsentMutationObserverandgetGroupConsentMutationOptions. Consider extracting shared logic into reusable functions.+const createMutationHandlers = (account: string, topic: ConversationTopic, action: GroupConsentAction) => { + const consentStatus = action === "allow" ? "allowed" : "denied"; + return { + onMutate: async () => { + const previousConsent = getGroupConsentQueryData(account, topic); + setGroupConsentQueryData(account, topic, consentStatus); + return { previousConsent }; + }, + onError: (error: unknown, _variables: unknown, context: { previousConsent?: ConsentState }) => { + logger.warn(`onError use${action.charAt(0).toUpperCase() + action.slice(1)}GroupMutation`); + sentryTrackError(error); + if (context?.previousConsent !== undefined) { + setGroupConsentQueryData(account, topic, context.previousConsent); + } + }, + onSuccess: () => { + logger.debug(`onSuccess use${action.charAt(0).toUpperCase() + action.slice(1)}GroupMutation`); + }, + }; +};hooks/useGroupDescription.ts (1)
Line range hint
1-1: Use useCurrentAccount hook instead of direct import.Using the store value directly makes the component less testable and harder to mock.
-import { currentAccount } from "../data/store/accountsStore"; +import { useCurrentAccount } from "../data/store/accountsStore"; export const useGroupDescription = (topic: ConversationTopic) => { - const account = currentAccount(); + const account = useCurrentAccount();Also applies to: 6-7
features/conversation/hooks/use-group-name-convos.ts (1)
Line range hint
26-28: Add null checks in member address mappingThe optional chaining could be improved to handle potential undefined values more safely.
const memberAddresses = members?.ids - .map((id) => members?.byId[id]?.addresses[0]) + .map((id) => members?.byId[id]?.addresses?.[0]) .filter((address) => address !== account);queries/useRevokeAdminMutation.ts (1)
Remove commented refreshGroup code
The
refreshGroupfunctionality appears to be intentionally disabled across all mutation handlers in the codebase. The implementation inconversations.tsshows that group refreshing is now handled through different mechanisms (refreshProtocolConversationandhandleNewConversationCreation). The commented code should be removed to avoid confusion.
queries/useRevokeAdminMutation.ts: Remove commented import andrefreshGroupcallqueries/useRevokeSuperAdminMutation.ts: Remove commented import andrefreshGroupcallqueries/useAddToGroupMutation.ts: Remove commented import andrefreshGroupcallqueries/useRemoveFromGroupMutation.ts: Remove commented import andrefreshGroupcallqueries/usePromoteToAdminMutation.ts: Remove commented import andrefreshGroupcallqueries/usePromoteToSuperAdminMutation.ts: Remove commented import andrefreshGroupcall🔗 Analysis chain
Line range hint
61-63: Consider implementing refreshGroupThe commented out refreshGroup call suggests incomplete implementation. Consider either implementing the refresh functionality or removing the comment if it's no longer needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if refreshGroup is used elsewhere in the codebase rg "refreshGroup.*account.*topic" --type typescriptLength of output: 88
Script:
#!/bin/bash # Search for refreshGroup function definition and usage rg "refreshGroup" -A 3 # Search for any similar refresh patterns in mutation success handlers rg "onSuccess.*refresh" -A 3Length of output: 6273
Script:
#!/bin/bash # Check if the refreshGroup implementation exists in the conversations utility fd "conversations" --exec cat {} \; # Look for any refresh-related implementations in the codebase rg "refresh.*group" -i --type tsLength of output: 18801
🧹 Nitpick comments (62)
screens/ConversationList.tsx (2)
21-24: Great addition for advanced search queries.
Importing these search utility functions (dmMatchesSearchQuery, groupMatchesSearchQuery) is a neat approach to separating search logic for direct messages and groups. Confirm that you’ve adequately tested all relevant conversation types (DM vs. group) so that unexpected search issues don’t slip through.
108-111: Simplify or encapsulate complex condition.
The combined condition (lines 108-111) might benefit from being extracted into a helper function for readability and testability.- items?.length === 0 && !searchQuery && !showInitialLoad && requestsCount === 0 + isEmptyState(items, searchQuery, showInitialLoad, requestsCount) function isEmptyState( items: FlatListItemType[] | undefined, searchQuery: string, showInitialLoad: boolean, requestsCount: number ) { return !items?.length && !searchQuery && !showInitialLoad && requestsCount === 0; }features/notifications/utils/accountTopicSubscription.ts (1)
17-20: Consider adding type safety for the context parameterThe observer creation has been updated to use an object parameter pattern, which is a good improvement for maintainability. However, the
contextparameter could benefit from type safety.Consider adding a type definition:
type ConversationListContext = "sync" | "other_valid_contexts"; interface ConversationListQueryParams { account: string; context: ConversationListContext; }This would prevent accidental misuse of the context parameter and provide better IDE support.
features/conversation/conversation-consent-popup/conversation-consent-popup-dm.tsx (2)
71-76: Optimistic update pattern recognized.
Using “onMutate” to retrieve conversation data is a valid approach for an optimistic update. Consider adding robust fallback logic if “conversation” is unexpectedly undefined, helping minimize inconsistent client states.
88-106: Effective rollback strategy.
You restore the previous “dm.state” upon errors, maintaining consistent UI states. One minor enhancement might be to wrap the entire revert process in a try-catch block in case of subsequent errors (e.g., ifdmis unexpectedly null).queries/useBlockGroupMutation.ts (2)
Line range hint
35-41: Enhance error handling specificityThe current error handling is generic and could benefit from more specific error types and handling.
Consider adding error type information and specific handling:
- onError: (error, _variables, context) => { - captureError(error); + onError: (error: Error, _variables, context: { previousConsent?: string }) => { + captureError(error, { + tags: { + operation: 'blockGroup', + account, + topic: topic.topic + } + });
Line range hint
29-45: Consider adding loading state handlingThe mutation could benefit from handling loading states more explicitly, especially for UI feedback.
Consider integrating with a loading state management system or adding loading callbacks to handle UI states during the mutation.
features/conversation/conversation-dm-header-title.tsx (1)
Line range hint
19-63: Consider memoizing the avatar componentTo optimize performance, consider memoizing the avatar component since it's recreated on every render:
+ const AvatarMemo = useCallback(() => ( + <Avatar + uri={preferredAvatarUri ?? undefined} + size={theme.avatarSize.md} + name={preferredName} + /> + ), [preferredAvatarUri, theme.avatarSize.md, preferredName]); return ( <ConversationTitle title={preferredName} onLongPress={onLongPress} onPress={onPress} avatarComponent={ - displayAvatar && ( - <Avatar - uri={preferredAvatarUri ?? undefined} - size={theme.avatarSize.md} - name={preferredName} - /> - ) + displayAvatar && <AvatarMemo /> } /> );queries/useGroupPinnedFrameQuery.ts (1)
Line range hint
12-21: Consider adding error handling for group pinned frame queriesThe current implementation silently returns undefined when group or topic is missing. Consider adding error handling or logging for better debugging.
Here's a suggested improvement:
return useQuery({ queryKey: groupPinnedFrameQueryKey(account, topic!), queryFn: async () => { if (!group || !topic) { - return; + throw new Error('Missing required group or topic for pinned frame query'); } return ""; // return group.groupPinnedFrame(); }, enabled: !!group && !!topic, + retry: false, });features/notifications/utils/onInteractWithNotification.ts (1)
Line range hint
14-82: Consider breaking down the function for better maintainabilityThe function handles multiple responsibilities: parsing notification data, handling different notification types, managing state, and navigation. This makes it harder to maintain and test.
Consider breaking it down into smaller, focused functions:
parseNotificationDatahandleGroupJoinRequesthandleConversationNavigationExample structure:
const parseNotificationData = (notification: Notifications.NotificationResponse) => { // parsing logic } const handleGroupJoinRequest = (payload: GroupJoinPayload) => { // group join handling } const handleConversationNavigation = async (topic: string, account: string) => { // navigation and state management } export const onInteractWithNotification = async ( event: Notifications.NotificationResponse ) => { const data = parseNotificationData(event); if (isGroupJoinRequest(data)) { return handleGroupJoinRequest(data); } if (isConversationNotification(data)) { return handleConversationNavigation(data.topic, data.account); } };queries/groupConsentMutationUtils.ts (2)
Line range hint
24-54: Consider adding mutation queue managementThe current implementation might be susceptible to race conditions when multiple consent mutations occur simultaneously. While the optimistic updates are handled correctly, concurrent mutations could lead to inconsistent states.
Consider implementing a mutation queue or adding a mechanism to prevent concurrent mutations for the same group/topic combination. This could be achieved by:
- Adding a pending state check before mutation
- Using a mutation queue manager
- Implementing a debounce mechanism
Would you like me to provide an example implementation of any of these approaches?
Line range hint
82-98: Improve error handling specificityThe error handling could be more specific about the types of errors that can occur during mutation.
- onError: ( - error: unknown, - _variables: unknown, - context: { previousConsent?: ConsentState } - ) => { + onError: ( + error: Error | { code: string; message: string }, + _variables: unknown, + context: { previousConsent?: ConsentState } + ) => { + const errorMessage = error instanceof Error ? error.message : error.message; logger.warn( - `onError use${ + `onError use${ action.charAt(0).toUpperCase() + action.slice(1) - }GroupMutation` + }GroupMutation: ${errorMessage}` ); sentryTrackError(error);features/conversation/conversation-consent-popup/conversation-consent-popup-group.tsx (2)
29-29: Consider handling loading and error statesThe
useGroupNameQueryhook might have loading and error states that should be handled to improve user experience.- const { data: groupName } = useGroupNameQuery({ account, topic }); + const { data: groupName, isLoading, error } = useGroupNameQuery({ account, topic }); + + if (isLoading) { + return <LoadingSpinner />; // or appropriate loading UI + } + + if (error) { + return <ErrorMessage error={error} />; // or appropriate error UI + }
Line range hint
32-45: Standardize error handling across callbacksWhile
onAcceptusescaptureErrorWithToast, theonBlockcallback doesn't have explicit error handling. Consider standardizing error handling across both callbacks for consistency.const onBlock = useCallback(async () => { - groupRemoveRestoreHandler( - "unknown", // To display "Remove & Block inviter" - colorScheme, - groupName, - allowGroup, - blockGroup - )((success: boolean) => { - if (success) { - navigation.pop(); - } - // If not successful, do nothing (user canceled) - }); + try { + await groupRemoveRestoreHandler( + "unknown", // To display "Remove & Block inviter" + colorScheme, + groupName, + allowGroup, + blockGroup + )((success: boolean) => { + if (success) { + navigation.pop(); + } + // If not successful, do nothing (user canceled) + }); + } catch (error) { + captureErrorWithToast(error); + } }, [groupName, colorScheme, allowGroup, blockGroup, navigation]);Also applies to: 47-54
queries/useGroupNameQuery.ts (1)
6-9: Refactored Function SignatureSwitching from two separate parameters (account/topic) to a single argument object is a cleaner approach, improving readability and flexibility when adding new arguments in the future.
utils/mutate-object-properties.ts (2)
7-13: Flexible Function SignatureAllowing a configurable recursion level is a thoughtful way to handle nested structures. However, consider adding a safeguard for very large levels to avoid potential stack overflow in extreme cases.
31-36: Return Mutated ObjectFinal return of the object is straightforward. Be mindful of the fact that the function mutates in place; some code might expect immutable operations.
queries/useGroupConsentQuery.ts (1)
15-19: Selection of Group StateSelecting group.state directly from the group object is simpler than performing additional checks or asynchronous calls. Verified that the “select” key usage aligns with React Query’s recommended patterns.
queries/useDmQuery.ts (1)
1-3: Reminder About the TODOThe TODO suggests possibly deleting this file in favor of conversation queries with a “peer” argument. Please confirm if that approach remains under consideration or if this file is still needed.
Do you want me to open a GitHub issue to track this TODO?
queries/useGroupPhotoQuery.ts (1)
13-14: Add optional chaining for safer property accessWhile the type guard ensures type safety, it's good practice to add optional chaining when accessing object properties that might be undefined.
- isConversationGroup(group) ? group.imageUrlSquare : undefined, + isConversationGroup(group) ? group?.imageUrlSquare : undefined,queries/useGroupPermissionPolicyQuery.ts (2)
Line range hint
6-9: Consider using object parameters for consistencyFor consistency with useGroupQuery and other hooks, consider consolidating the parameters into a single object.
-export const useGroupPermissionPolicyQuery = ( - account: string, - topic: ConversationTopic -) => { +export const useGroupPermissionPolicyQuery = (args: { + account: string; + topic: ConversationTopic; +}) => { + const { account, topic } = args;
Line range hint
12-18: Improve type safety and clarityA few suggestions to enhance the implementation:
- Remove the unnecessary non-null assertion on topic in queryKey
- Make the undefined return more explicit
- queryKey: groupPermissionPolicyQueryKey(account, topic!), + queryKey: groupPermissionPolicyQueryKey(account, topic), queryFn: () => { if (!group) { - return; + return undefined; } return group.permissionPolicySet(); },queries/useGroupIsActiveQuery.ts (1)
6-16: LGTM! Consider adding error handling documentation.The implementation is clean and follows React Query best practices. The select transformer properly handles the undefined case.
Consider adding JSDoc documentation to describe error scenarios and return types:
+/** + * Custom hook to check if a conversation group is active + * @param args.account - The account address + * @param args.topic - The conversation topic + * @returns Query result containing the group's active status + * @throws Will throw if the group query fails + */ export const useGroupIsActiveQuery = (args: {hooks/useGroupDescription.ts (1)
8-13: Standardize parameter passing style between query and mutation.The query uses object parameters while the mutation uses separate parameters, which is inconsistent.
const { data, isLoading, isError } = useGroupDescriptionQuery({ account, topic, }); - const { mutateAsync } = useGroupDescriptionMutation(account, topic); + const { mutateAsync } = useGroupDescriptionMutation({ account, topic });hooks/useGroupPhoto.ts (1)
6-6: Consider allowing undefined topic like useGroupNameFor consistency with
useGroupName, consider allowingundefinedtopic and handling it explicitly.- export const useGroupPhoto = (topic: ConversationTopic) => { + export const useGroupPhoto = (topic: ConversationTopic | undefined) => {queries/useGroupCreatorQuery.ts (1)
14-17: Consider adding error handling for group.creatorInboxId()The
creatorInboxId()call might throw an error, which should be handled gracefully.queryFn: () => { if (!group) return null; - return group.creatorInboxId(); + try { + return group.creatorInboxId(); + } catch (error) { + console.error('Failed to fetch creator inbox ID:', error); + return null; + } },features/conversation/hooks/use-group-name-convos.ts (1)
Line range hint
22-24: Consider updating useGroupMembersQuery to use object parametersFor consistency, useGroupMembersQuery should follow the same parameter pattern as useGroupNameQuery.
-const { data: members, isLoading: membersLoading } = useGroupMembersQuery( - account, - topic -); +const { data: members, isLoading: membersLoading } = useGroupMembersQuery({ + account, + topic +});features/conversation/conversation-title.tsx (1)
26-28: Consider extracting inline styles to a styled component or useStyles hookWhile inline styles work, they make the component less maintainable and reusable. Consider extracting these styles to maintain consistency across the codebase.
+ const useStyles = (theme: Theme) => ({ + container: { + flex: 1, + }, + pressable: { + flexDirection: "row", + alignItems: "center", + }, + avatarContainer: { + paddingRight: theme.spacing.xxs, + }, + }); const { theme } = useAppTheme(); + const styles = useStyles(theme); - <HStack style={{ flex: 1 }}> + <HStack style={styles.container}>Also applies to: 33-36, 39-41
features/conversation-list/useV3ConversationItems.ts (1)
12-15: Consider making query options configurableThe hardcoded query options might not be suitable for all use cases. Consider making these configurable through props or a configuration object.
+ type UseV3ConversationItemsOptions = { + refetchOnWindowFocus?: boolean; + refetchOnMount?: boolean; + }; + - export const useV3ConversationItems = () => { + export const useV3ConversationItems = (options?: UseV3ConversationItemsOptions) => { // ... queryOptions: { - refetchOnWindowFocus: false, - refetchOnMount: false, + refetchOnWindowFocus: options?.refetchOnWindowFocus ?? false, + refetchOnMount: options?.refetchOnMount ?? false, },queries/useGroupPhotoMutation.ts (1)
Line range hint
21-24: Add explicit error message for missing dependenciesFor consistency with other mutation functions, add an explicit error message when dependencies are missing.
mutationFn: async (groupPhoto: string) => { if (!group || !account || !topic) { - return; + throw new Error( + "Missing group, account, or topic in useGroupPhotoMutation" + ); }queries/useGroupNameMutation.ts (1)
29-39: Add error handling for failed optimistic updatesThe optimistic update doesn't handle the case where
updateGroupQueryDatamight fail.onMutate: async (groupName: string) => { const previousGroup = getGroupQueryData({ account, topic }); if (previousGroup) { + try { updateGroupQueryData({ account, topic, updates: { name: groupName, }, }); + } catch (error) { + captureError(error); + return { previousGroupName: previousGroup?.name }; + } } return { previousGroupName: previousGroup?.name }; },queries/useGroupDescriptionMutation.ts (1)
38-39: Remove unnecessary empty linesThere are several unnecessary empty lines that should be removed for better code formatting.
Also applies to: 44-44
queries/usePromoteToSuperAdminMutation.ts (1)
Line range hint
50-58: Consider error recovery strategyThe error handling looks good, but consider adding a retry strategy for transient failures in group member operations.
onError: (error, _variables, context) => { captureError(error); + // Add retry logic for transient failures + if (isTransientError(error)) { + return retry(); + } if (context?.previousGroupMembers === undefined) { return; } if (!topic) {queries/useRevokeSuperAdminMutation.ts (3)
Line range hint
52-59: Optimize error handling flowConsider reordering conditions to check for undefined context first, as it's a more fundamental precondition.
onError: (error, _variables, context) => { captureError(error); - if (!topic) { - return; - } if (context?.previousGroupMembers === undefined) { return; } + if (!topic) { + return; + } setGroupMembersQueryData(account, topic, context.previousGroupMembers); }
21-21: Consider adding group existence validationThe mutationFn checks for group existence, but consider adding early validation in the hook to prevent unnecessary mutation attempts.
const { data: group } = useGroupQuery({ account, topic }); + + if (!group) { + throw new Error('Group not found'); + } return useMutation({Also applies to: 24-24
Line range hint
1-1: Consider creating a shared mutation baseGiven the similar patterns across mutation files (error handling, group validation, refreshGroup todos), consider creating a shared base mutation factory or hooks to reduce duplication and ensure consistent behavior.
This could help:
- Standardize error handling
- Share common validation logic
- Implement consistent refresh strategies
- Reduce code duplication
queries/useConversationPreviewMessages.ts (1)
Line range hint
18-36: Consider environment-based loggingThe detailed logging in
conversationPreviewMessagesQueryFnis valuable for debugging but could impact performance. Consider conditionally enabling these logs based on the environment.logger.info( "[ConversationPreview Messages]conversationPreviewMessagesQueryFn", - { - topic: conversation?.topic, - } + process.env.NODE_ENV === 'development' ? { + topic: conversation?.topic, + } : undefined );features/conversation/conversation.screen.tsx (1)
55-59: Consider adding initial loading stateThe component assumes
currentAccountis always non-null. Consider adding a loading state for when the account is being fetched.- const currentAccount = useCurrentAccount()!; + const currentAccount = useCurrentAccount(); + + if (!currentAccount) { + return ( + <Center style={$globalStyles.flex1}> + <Loader /> + </Center> + ); + } const { data: dmConversation, isLoading } = useDmQuery({ account: currentAccount, peer: peerAddress, });queries/usePromoteToAdminMutation.ts (1)
Line range hint
68-69: Address commented codeThe commented
refreshGroupcall suggests incomplete changes. Either remove the comment if it's no longer needed or implement the refresh functionality if it's required.features/conversation-requests-list/useV3RequestItems.tsx (1)
Line range hint
27-69: Consider performance optimizationThe spam detection logic in useEffect could benefit from memoization to prevent unnecessary recalculations. Consider using useMemo for the spam score calculations.
+ const processConversation = useMemo(async (conversation: ConversationWithCodecsType) => { + if (conversation.state !== "unknown") return null; + // ... existing spam detection logic ... + return { conversation, isSpam: spamScore > 1 }; + }, []); useEffect(() => { const fetchGroups = async () => { const likelyNotSpam: ConversationWithCodecsType[] = []; const likelySpam: ConversationWithCodecsType[] = []; - await Promise.allSettled( + const results = await Promise.allSettled( (data ?? []).map(async (conversation) => { - // ... existing logic ... + const result = await processConversation(conversation); + if (result) { + result.isSpam ? likelySpam.push(result.conversation) : likelyNotSpam.push(result.conversation); + } }) ); setLikelyNotSpam(likelyNotSpam); setLikelySpam(likelySpam); }; fetchGroups(); }, [data, processConversation]);components/StateHandlers/HydrationStateHandler.tsx (1)
36-39: Enhance error handling for hydration failuresThe current error handling only logs warnings. Consider implementing a retry mechanism or user notification for critical hydration failures.
containers/GroupScreenName.tsx (1)
52-57: Enhance error handling and user feedback.The error handling could be more informative and should use the translation system for consistency.
try { setEditing(false); await updateGroupName(editedName); } catch (e) { logger.error(e); - Alert.alert("An error occurred"); + Alert.alert(translate("errors.group_name_update_failed"), translate("errors.please_try_again")); + setEditing(true); // Allow the user to try again }hooks/useGroupConsent.ts (2)
Line range hint
41-83: Consider refactoring blockGroup for better separation of concerns.The
blockGroupfunction handles multiple responsibilities: mutation, error handling, and inbox ID management. Consider breaking it down into smaller, focused functions.+const getDenyInboxIds = ( + group: Group, + groupCreator: string | undefined, + options: IGroupConsentOptions +): InboxId[] => { + const inboxIds: InboxId[] = []; + if (options.includeAddedBy && group.addedByInboxId) { + inboxIds.push(group.addedByInboxId); + } + if (options.includeCreator && groupCreator) { + inboxIds.push(groupCreator); + } + return inboxIds; +}; const blockGroup = useCallback( async (args: IGroupConsentOptions) => { if (!group) { showSnackbar({ type: "error", message: translate("group_not_found"), }); return; } try { await blockGroupMutation(); - const inboxIdsToDeny: InboxId[] = []; - if (includeAddedBy && group.addedByInboxId) { - inboxIdsToDeny.push(group.addedByInboxId); - } - if (includeCreator && groupCreator) { - inboxIdsToDeny.push(groupCreator); - } + const inboxIdsToDeny = getDenyInboxIds(group, groupCreator, args); if (inboxIdsToDeny.length > 0) { await consentToInboxIdsOnProtocolByAccount({ account, inboxIds: inboxIdsToDeny, consent: "deny", }); } + } catch (error) { + showSnackbar({ + type: "error", + message: translate("errors.block_group_failed"), + }); + logger.error(error); } }, [blockGroupMutation, groupCreator, account, group] );
Line range hint
41-50: Align error handling patterns between allowGroup and blockGroup.The error handling differs between
allowGroupandblockGroup. Consider using a consistent approach across both functions.const allowGroup = useCallback( async (args: IGroupConsentOptions) => { const { includeAddedBy, includeCreator } = args; if (!group) { - throw new Error("Group is required"); + showSnackbar({ + type: "error", + message: translate("group_not_found"), + }); + return; }queries/QueryKeys.ts (1)
75-89: Ensure consistent error handling in group-related query keysThe group-related query key functions (
groupPermissionPolicyQueryKey,groupCreatorQueryKey,groupPermissionsQueryKey) should have consistent error handling for invalid inputs.Consider adding input validation:
export const groupPermissionPolicyQueryKey = ( account: string, topic: ConversationTopic ) => { + if (!account || !topic) { + throw new Error('Invalid arguments: account and topic are required'); + } return [QueryKeys.GROUP_PERMISSION_POLICY, account.toLowerCase(), topic]; };queries/useGroupMembersQuery.ts (1)
Line range hint
33-37: Add type safety to groupMembersQueryConfigThe
groupMembersQueryConfigfunction could benefit from stronger type safety for its parameters.+ interface GroupMembersQueryConfig { + account: string; + group: GroupWithCodecsType | undefined | null; + enabled: boolean; + } const groupMembersQueryConfig = ( - account: string, - group: GroupWithCodecsType | undefined | null, - enabled: boolean + { account, group, enabled }: GroupMembersQueryConfig ): UseQueryOptions<GroupMembersSelectData> => ({ queryKey: groupMembersQueryKey(account, group?.topic!), queryFn: () => fetchGroupMembers(group!), enabled, });screens/ConversationReadOnly.tsx (1)
28-36: Consider memoizing query parametersThe query parameters object is recreated on every render. Consider memoizing it to prevent unnecessary re-renders.
+ const queryParams = React.useMemo( + () => ({ + account: currentAccount, + topic, + }), + [currentAccount, topic] + ); - const { data: conversation, isLoading: isLoadingConversation } = - useConversationQuery({ - account: currentAccount, - topic, - }); + const { data: conversation, isLoading: isLoadingConversation } = + useConversationQuery(queryParams);queries/useAllowGroupMutation.ts (1)
96-114: Consider adding retry logic for failed mutationsThe error handling is good with captureError, but consider adding retry logic for transient failures.
onError: ( error: unknown, variables: AllowGroupMutationVariables, context?: { previousConsent: ConsentState | undefined; } ) => { const { account, group } = variables; + // Add retry logic for transient failures + if (isTransientError(error)) { + return queryClient.invalidateQueries({ + queryKey: [MutationKeys.ALLOW_GROUP, account, group.topic], + }); + } captureError(error); if (!context) { return; } if (context.previousConsent) { setGroupConsentQueryData(account, group.topic, context.previousConsent); } },features/conversation/conversation-group-header-title.tsx (2)
67-69: Consider showing loading state instead of null.Returning null during loading might cause layout shifts. Consider showing a skeleton loader or placeholder instead.
- if (groupPhotoLoading || groupNameLoading) { - return null; - } + if (groupPhotoLoading || groupNameLoading) { + return ( + <ConversationTitle + title="Loading..." + avatarComponent={<Avatar size={theme.avatarSize.md} />} + /> + ); + }
118-131: Optimize member data processing.The current implementation creates a new array on each iteration. Consider using filter for better readability and performance.
- return members.ids.reduce<string[]>((addresses, memberId) => { - const memberAddress = members.byId[memberId]?.addresses[0]; - if ( - memberAddress && - memberAddress.toLowerCase() !== currentAccount?.toLowerCase() - ) { - addresses.push(memberAddress); - } - return addresses; - }, []); + return members.ids + .map(memberId => members.byId[memberId]?.addresses[0]) + .filter( + address => address && + address.toLowerCase() !== currentAccount?.toLowerCase() + );components/PinnedConversations/PinnedV3DMConversation.tsx (1)
Line range hint
69-108: Consider extracting context menu items to a separate hook.The context menu items logic is complex and could be moved to a custom hook for better maintainability and reusability.
+const useContextMenuItems = ({ + isUnread, + theme, + setPinnedConversations, + topic, + toggleReadStatus, + handleDelete, +}) => { + return useMemo( + () => [ + { + title: translate("unpin"), + action: () => { + setPinnedConversations([topic]); + closeContextMenu(); + }, + id: "pin", + rightView: <ContextMenuIcon icon="pin.slash" />, + }, + // ... other items + ], + [isUnread, theme.colors.global.caution, setPinnedConversations, topic, toggleReadStatus, handleDelete] + ); +};features/conversation/conversation-new-dm.tsx (1)
75-87: Consider adding specific error types for better error handling.While the error handling is good, consider creating specific error types for different failure scenarios to handle them more precisely.
try { addConversationToConversationListQuery({ account: currentAccount, conversation: newConversation, }); setDmQueryData({ account: currentAccount, peer: peerAddress, dm: newConversation, }); } catch (error) { - captureError(error); + if (error instanceof ConversationListError) { + captureError(error, 'Failed to update conversation list'); + } else if (error instanceof DMQueryError) { + captureError(error, 'Failed to update DM query data'); + } else { + captureError(error, 'Unknown error during conversation creation'); + } }containers/GroupScreenAddition.tsx (1)
78-81: Add loading state handling for group name query.Consider handling the loading state to improve user experience while the group name is being fetched.
- const { data: groupName } = useGroupNameQuery({ + const { data: groupName, isLoading } = useGroupNameQuery({ account: currentAccount, topic, }); + + if (isLoading) { + return <LoadingSpinner />; // Or any other loading indicator + }features/conversation/conversation.tsx (1)
Line range hint
310-347: Clean up commented code in handleLongPress.The function contains commented-out code that should be removed or documented if needed for future reference.
Apply this diff to clean up the code:
const handleLongPress = useCallback( (e: IMessageGesturesOnLongPressArgs) => { const messageId = messageStore.getState().messageId; - // const message = messageStore.getState().message; - // const previousMessage = messageStore.getState().previousMessage; - // const nextMessage = messageStore.getState().nextMessage; messageContextMenuStore.getState().setMessageContextMenuData({ messageId, itemRectX: e.pageX, itemRectY: e.pageY, itemRectHeight: e.height, itemRectWidth: e.width, - // Need to have MessageContextStoreProvider here. - // Not the cleanest... - // Might want to find another solution later but works for now. - // Solution might be to remove the context and just pass props - // messageComponent: ( - // <MessageContextStoreProvider - // message={message} - // previousMessage={previousMessage} - // nextMessage={nextMessage} - // > - // {children} - // </MessageContextStoreProvider> - // ), }); }, [messageContextMenuStore, messageStore] );screens/NewConversation/NewConversation.tsx (3)
Line range hint
165-238: Consider extracting the debounced search logic into a custom hookThe search functionality with debouncing could be moved to a custom hook to improve reusability and reduce component complexity.
Example implementation:
// hooks/useDebounceSearch.ts export const useDebounceSearch = ( value: string, searchFn: (value: string) => Promise<void>, delay: number = 500 ) => { const debounceTimer = useRef<ReturnType<typeof setTimeout> | null>(null); useEffect(() => { if (value.length < 3) return; if (debounceTimer.current !== null) { clearTimeout(debounceTimer.current); } debounceTimer.current = setTimeout(() => { searchFn(value); }, delay); return () => { if (debounceTimer.current !== null) { clearTimeout(debounceTimer.current); } }; }, [value, searchFn, delay]); };
Line range hint
239-267: Consider moving error messages to translation keysHard-coded error messages should be moved to translation keys for better internationalization support.
Example changes:
- error: "This handle does not exist. Please try again." + error: translate("errors.handle_not_exist") - error: "No address has been set for this domain." + error: translate("errors.no_address_for_domain") - error: `${value} does not use Converse or XMTP yet` + error: translate("errors.not_using_xmtp", { value })
Line range hint
1-500: Consider splitting the component for better maintainabilityThe component handles multiple responsibilities (search, group management, profile handling). Consider splitting it into smaller, focused components.
Suggested structure:
NewConversationSearch- Handle search functionalityGroupMembersList- Manage group members display and actionsSearchResults- Display and handle search resultsRecommendationsList- Handle recommendations displayThis would improve maintainability and make the code easier to test.
features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx (5)
Line range hint
89-107: Consider optimizing reaction handling performanceThe reaction handling logic is well-structured but could be optimized by:
- Memoizing the
currentUserAlreadyReactedcalculation- Moving the complex reaction logic to a custom hook
Example implementation:
const useReactionHandler = (messageId: string, currentUserInboxId: string, bySender: Record<string, any>) => { return useCallback((emoji: string) => { const currentUserAlreadyReacted = useMemo( () => bySender?.[currentUserInboxId]?.some( (reaction) => reaction.content === emoji ), [bySender, currentUserInboxId, emoji] ); if (currentUserAlreadyReacted) { removeReactionOnMessage({ messageId, emoji }); } else { reactOnMessage({ messageId, emoji }); } messageContextMenuStore.getState().setMessageContextMenuData(null); }, [messageId, currentUserInboxId, bySender]); };
167-169: Address TODO: Consider implementing presentational component patternThe TODO comment suggests making
ConversationMessagemore "dumb". This is a good architectural suggestion as it would improve reusability and testing.Consider splitting into:
- A presentational component handling only UI
- A container component handling logic and context
This would allow
ConversationMessageto be used without context in scenarios like this.
154-160: Extract spacing constants to a design system constants fileHardcoded spacing values should be maintained in a centralized location for consistency.
Consider creating a spacing constants file:
// constants/spacing.ts export const SPACING = { MESSAGE_CONTEXT_MENU: { REACTION_MESSAGE_GAP: 16, // ... other spacing values } } as const;
Line range hint
191-307: Document implementation attempts in a separate file instead of commented codeThe commented code provides valuable context about different approaches tried, but it would be better maintained in a separate documentation file.
Consider:
- Moving this to a
IMPLEMENTATION_NOTES.mdfile- Including pros and cons of each attempted approach
- Removing the commented code from the source file
128-182: Consider breaking down the MessageContextMenu into smaller componentsThe current implementation, while functional, handles multiple responsibilities. Consider splitting it into smaller, more focused components:
- MessageContextMenuReactions
- MessageContextMenuContent
- MessageContextMenuActions
This would improve:
- Testability
- Code reusability
- Maintenance
- Performance optimization opportunities
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
queries/__snapshots__/QueryKeys.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (82)
components/Connecting.tsx(2 hunks)components/PinnedConversations/PinnedConversations.tsx(2 hunks)components/PinnedConversations/PinnedV3DMConversation.tsx(2 hunks)components/StateHandlers/HydrationStateHandler.tsx(2 hunks)components/V3DMListItem.tsx(3 hunks)containers/GroupScreenAddition.tsx(3 hunks)containers/GroupScreenName.tsx(4 hunks)data/helpers/messages/handleGroupUpdatedMessage.test.ts(0 hunks)data/helpers/messages/handleGroupUpdatedMessage.ts(0 hunks)features/GroupInvites/joinGroup/JoinGroup.client.ts(2 hunks)features/blocked-chats/useV3BlockedChats.ts(1 hunks)features/conversation-list/useV3ConversationItems.ts(1 hunks)features/conversation-list/useV3RequestItemCount.ts(1 hunks)features/conversation-requests-list/useV3RequestItems.tsx(1 hunks)features/conversation/conversation-consent-popup/conversation-consent-popup-dm.tsx(4 hunks)features/conversation/conversation-consent-popup/conversation-consent-popup-group.tsx(1 hunks)features/conversation/conversation-dm-header-title.tsx(3 hunks)features/conversation/conversation-group-header-title.tsx(5 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx(4 hunks)features/conversation/conversation-message/conversation-message-layout.tsx(3 hunks)features/conversation/conversation-message/conversation-message.store-context.tsx(4 hunks)features/conversation/conversation-new-dm-header-title.tsx(3 hunks)features/conversation/conversation-new-dm.tsx(5 hunks)features/conversation/conversation-title.tsx(2 hunks)features/conversation/conversation.screen.tsx(2 hunks)features/conversation/conversation.tsx(14 hunks)features/conversation/hooks/use-group-name-convos.ts(1 hunks)features/conversation/utils/is-conversation-dm.ts(1 hunks)features/conversation/utils/is-conversation-group.ts(1 hunks)features/notifications/utils/accountTopicSubscription.ts(2 hunks)features/notifications/utils/onInteractWithNotification.ts(1 hunks)hooks/useConversationListGroupItem.ts(1 hunks)hooks/useGroupConsent.ts(2 hunks)hooks/useGroupCreator.ts(0 hunks)hooks/useGroupDescription.ts(1 hunks)hooks/useGroupName.ts(1 hunks)hooks/useGroupPhoto.ts(1 hunks)queries/QueryKeys.test.ts(2 hunks)queries/QueryKeys.ts(3 hunks)queries/groupConsentMutationUtils.ts(1 hunks)queries/useAddToGroupMutation.ts(2 hunks)queries/useAllowGroupMutation.ts(2 hunks)queries/useBlockGroupMutation.ts(2 hunks)queries/useConversationListQuery.ts(1 hunks)queries/useConversationMessages.ts(4 hunks)queries/useConversationPreviewMessages.ts(2 hunks)queries/useConversationQuery.ts(1 hunks)queries/useConversationWithPeerQuery.ts(0 hunks)queries/useDmConstentStateQuery.ts(0 hunks)queries/useDmPeerAddressQuery.ts(0 hunks)queries/useDmPeerInbox.ts(1 hunks)queries/useDmPeerInboxOnConversationList.ts(0 hunks)queries/useDmQuery.ts(1 hunks)queries/useGroupConsentQuery.ts(1 hunks)queries/useGroupCreatorQuery.ts(1 hunks)queries/useGroupDescriptionMutation.ts(1 hunks)queries/useGroupDescriptionQuery.ts(1 hunks)queries/useGroupIsActive.ts(0 hunks)queries/useGroupIsActiveQuery.ts(1 hunks)queries/useGroupMembersQuery.ts(2 hunks)queries/useGroupNameMutation.ts(1 hunks)queries/useGroupNameQuery.ts(1 hunks)queries/useGroupPermissionPolicyQuery.ts(1 hunks)queries/useGroupPermissionsQuery.ts(1 hunks)queries/useGroupPhotoMutation.ts(2 hunks)queries/useGroupPhotoQuery.ts(1 hunks)queries/useGroupPinnedFrameQuery.ts(1 hunks)queries/useGroupQuery.ts(1 hunks)queries/usePromoteToAdminMutation.ts(2 hunks)queries/usePromoteToSuperAdminMutation.ts(3 hunks)queries/useRemoveFromGroupMutation.ts(2 hunks)queries/useRevokeAdminMutation.ts(2 hunks)queries/useRevokeSuperAdminMutation.ts(2 hunks)queries/useV3ConversationListQuery.ts(0 hunks)screens/ConversationList.tsx(2 hunks)screens/ConversationReadOnly.tsx(1 hunks)screens/Group.tsx(1 hunks)screens/NewConversation/NewConversation.tsx(3 hunks)utils/groupUtils/handleGroupDescriptionUpdate.ts(0 hunks)utils/groupUtils/handleGroupImageUpdate.ts(0 hunks)utils/groupUtils/handleGroupNameUpdate.ts(0 hunks)utils/mutate-object-properties.ts(1 hunks)
⛔ Files not processed due to max files limit (3)
- utils/xmtpRN/conversations.ts
- utils/xmtpRN/messages.ts
- utils/xmtpRN/sync.ts
💤 Files with no reviewable changes (12)
- data/helpers/messages/handleGroupUpdatedMessage.test.ts
- utils/groupUtils/handleGroupDescriptionUpdate.ts
- utils/groupUtils/handleGroupImageUpdate.ts
- utils/groupUtils/handleGroupNameUpdate.ts
- hooks/useGroupCreator.ts
- data/helpers/messages/handleGroupUpdatedMessage.ts
- queries/useDmPeerInboxOnConversationList.ts
- queries/useDmConstentStateQuery.ts
- queries/useDmPeerAddressQuery.ts
- queries/useGroupIsActive.ts
- queries/useConversationWithPeerQuery.ts
- queries/useV3ConversationListQuery.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- features/conversation/conversation-message/conversation-message-layout.tsx
- features/conversation/conversation-message/conversation-message.store-context.tsx
🧰 Additional context used
📓 Learnings (3)
queries/useGroupQuery.ts (1)
Learnt from: alexrisch
PR: ephemeraHQ/converse-app#1313
File: queries/useV3ConversationListQuery.ts:196-268
Timestamp: 2024-12-05T18:33:24.472Z
Learning: In the TypeScript file `queries/useV3ConversationListQuery.ts`, helper functions like `updateGroupImageToConversationListQuery`, `updateGroupNameToConversationListQuery`, and `updateGroupDescriptionToConversationListQuery` are used to reduce dependency on exact types or creating objects that get passed unnecessarily, even though they internally use `updateGroupMetadataToConversationListQuery`.
features/GroupInvites/joinGroup/JoinGroup.client.ts (1)
Learnt from: alexrisch
PR: ephemeraHQ/converse-app#1313
File: queries/useV3ConversationListQuery.ts:196-268
Timestamp: 2024-12-05T18:33:24.472Z
Learning: In the TypeScript file `queries/useV3ConversationListQuery.ts`, helper functions like `updateGroupImageToConversationListQuery`, `updateGroupNameToConversationListQuery`, and `updateGroupDescriptionToConversationListQuery` are used to reduce dependency on exact types or creating objects that get passed unnecessarily, even though they internally use `updateGroupMetadataToConversationListQuery`.
queries/useConversationListQuery.ts (1)
Learnt from: alexrisch
PR: ephemeraHQ/converse-app#1313
File: queries/useV3ConversationListQuery.ts:196-268
Timestamp: 2024-12-05T18:33:24.472Z
Learning: In the TypeScript file `queries/useV3ConversationListQuery.ts`, helper functions like `updateGroupImageToConversationListQuery`, `updateGroupNameToConversationListQuery`, and `updateGroupDescriptionToConversationListQuery` are used to reduce dependency on exact types or creating objects that get passed unnecessarily, even though they internally use `updateGroupMetadataToConversationListQuery`.
🔇 Additional comments (118)
screens/ConversationList.tsx (4)
20-20: Looks good.
Importing the ConversationContextMenu seems consistent with the new context menu functionality. Ensure that users recognize when and how to open this menu so the feature is discoverable.
44-44: Ensure adequate loading/error handling.
By leveraging useConversationListItems, consider how loading states or potential errors from the custom hook are handled. If there is a failure or partial fetch, ensure that the UI handles this gracefully.
45-45: Good use of a dedicated hook.
useConversationListRequestCount centralizes request-count logic, which enhances code maintainability. Confirm that any changes in request states are synchronized or retried as needed to prevent stale counts.
46-46: Clear isSharingMode separation.
Extracting the sharing-mode state into its own hook is a clean abstraction. This ensures that the conversation-list component remains focused on rendering logic.
features/notifications/utils/accountTopicSubscription.ts (2)
Line range hint 1-43: LGTM! Clean implementation with proper subscription management
The overall implementation demonstrates good practices:
- Proper cleanup of subscriptions
- State tracking to prevent duplicate notifications
- Clear logging for debugging
- Clean separation of concerns
1-1: Verify removal of V3 query imports across the codebase
The import statement has been updated to use the new non-versioned query observer.
Let's verify that all V3 query imports have been consistently updated:
✅ Verification successful
V3 query imports have been successfully removed
The verification shows that all conversation list query imports are now using the new non-versioned pattern (useConversationListQuery, createConversationListQueryObserver). No instances of the old V3 imports (useV3ConversationListQuery, createV3ConversationListQueryObserver) were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining V3 conversation query imports
rg "useV3ConversationListQuery|createV3ConversationListQueryObserver"
# Check the new query usage pattern
rg "useConversationListQuery|createConversationListQueryObserver"
Length of output: 3346
features/conversation/conversation-consent-popup/conversation-consent-popup-dm.tsx (6)
8-10: Imports aligned with new data-query approach.
These imports replace refetch-based updates with direct data pull and setter functions, facilitating an optimistic UI pattern. Ensure that this new pattern is consistently applied across all modules to maintain coherence.
14-15: Utility imports look correct and consistent.
The introduced helper function and type are appropriately imported and used in the code. No issues spotted with the references.
24-27: Context-based hooks are consistent with usage.
These hooks are used for retrieving the current conversation topic and ID. No problems identified. Verify these hooks remain stable within the broader conversation store context.
40-43: Good defensive approach when peerInboxId is undefined.
You're checking for missing peer inbox ID using the “throw new Error”. This ensures you handle fallback scenarios gracefully.
77-80: State transition logic is straightforward.
Setting the conversation state to “allowed” or “denied” and immediately storing it is aligned with your optimistic updates. The usage looks solid.
81-85: Returning previous consent ensures easy rollback.
Storing previousDmConsent allows for a clean revert on error. This aligns well with React Query’s recommended pattern for rollback in “onError”.
queries/useBlockGroupMutation.ts (1)
1-6: LGTM: Clean import organization
The imports are well-organized and the migration to captureError aligns with the codebase's new error handling strategy.
features/conversation/conversation-dm-header-title.tsx (3)
2-3: LGTM: Import changes are well-structured
The new imports for inbox handling and theme management are properly organized and follow consistent patterns.
Also applies to: 11-11
24-24: LGTM: Improved theme destructuring
The change to use destructured theme object follows React best practices.
58-58: Verify consistent avatar size usage
The change to use themed avatar sizes is good for consistency, but we should verify this size is used consistently across other conversation components.
✅ Verification successful
Let me search for more avatar size usages across the codebase to ensure we have a complete picture.
Avatar size usage is consistent with theming system
The verification shows that avatar sizes are consistently using the theme system across conversation components and other relevant parts of the application. All production code (excluding tests) uses theme.avatarSize values, maintaining the design system's consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other avatar size usages in conversation components
rg -A 1 "Avatar.*size=" "features/conversation/"
Length of output: 440
Script:
#!/bin/bash
# Search for all avatar size usages in the codebase
rg -A 1 "Avatar.*size="
Length of output: 1388
features/GroupInvites/joinGroup/JoinGroup.client.ts (1)
24-26: LGTM! Import changes align with implementation requirements.
The new imports support the updated conversation list query implementation and type safety improvements.
queries/useGroupPinnedFrameQuery.ts (2)
11-11: LGTM: Parameter restructuring aligns with API changes
The update to use object parameters for useGroupQuery matches the changes mentioned in the summary, improving consistency across the codebase.
Line range hint 17-18: Critical: Incomplete implementation of group pinned frame
The implementation returns an empty string and has a commented-out line that seems to be the actual functionality. This could break features depending on pinned frames.
Questions:
- Is this intentional or work in progress?
- Why return an empty string instead of undefined/null?
Let's check for dependencies on this functionality:
features/notifications/utils/onInteractWithNotification.ts (1)
10-10: Verify consistent migration from V3 queries
The import has been updated from useV3ConversationListQuery to useConversationListQuery. Let's verify that all V3-related imports have been consistently updated across the codebase.
✅ Verification successful
No inconsistency found in query migration
The import change is correct and consistent with the codebase. The search results show that:
- There are no remaining references to
useV3ConversationListQuery - All files consistently use the new
useConversationListQueryfrom "@/queries/useConversationListQuery" - While some files have "V3" in their names, they are actually using the new query implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining V3 query references that might have been missed
# Search for any remaining V3 query references
echo "Checking for remaining V3 query references..."
rg -l "useV3.*Query"
# Search for mixed usage of V3 and non-V3 conversation list queries
echo "Checking for mixed usage of conversation list queries..."
rg -l "useConversationListQuery|useV3ConversationListQuery"
Length of output: 1065
Script:
#!/bin/bash
# Let's check the actual content of these files to see if they still contain V3 references
# and understand the context better
# Check for actual useV3 usage in the files
echo "Checking for actual V3 query usage..."
rg "useV3\w+Query" -A 2 -B 2
# Specifically check the conversation list related queries
echo "Checking conversation list query implementations..."
rg "useConversationListQuery|useV3ConversationListQuery" -A 2 -B 2
Length of output: 11404
queries/useAddToGroupMutation.ts (2)
4-6: LGTM! Clean import organization
The imports are well-organized and the transition to captureError aligns with modern error handling practices.
18-18: Great use of object destructuring!
The transition to object destructuring for query parameters improves code maintainability and makes the parameter usage more explicit. This pattern also makes it easier to add new parameters in the future.
queries/groupConsentMutationUtils.ts (1)
10-13: Verify the removal of cancel functionality
The removal of cancelGroupConsentQuery import might affect error recovery scenarios. Let's verify that this removal doesn't impact the application's ability to handle race conditions or concurrent mutations.
✅ Verification successful
Removal of cancelGroupConsentQuery is safe
The code inspection reveals that error handling is properly implemented without the need for explicit cancellation:
- The mutation observer and options both implement robust error handling with rollback capability
- On error, the code restores the previous consent state using
setGroupConsentQueryData - Error tracking is in place via
sentryTrackError - The implementation follows a proper optimistic update pattern with automatic rollback
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usages of cancelGroupConsentQuery to ensure complete removal
rg "cancelGroupConsentQuery" --type ts
# Look for alternative error handling mechanisms
ast-grep --pattern 'catch ($_) {
$$$
}'
Length of output: 82
Script:
#!/bin/bash
# Let's search for any error handling patterns in the mutation utils file
rg -A 5 "catch|error" queries/groupConsentMutationUtils.ts
# Check the implementation of the mutation utils to understand the error handling approach
cat queries/groupConsentMutationUtils.ts
# Look for any references to group consent cancellation in the codebase
rg -A 3 "groupConsent.*cancel|cancel.*groupConsent" --type ts
# Check the query hook implementation
cat queries/useGroupConsentQuery.ts
Length of output: 5210
features/conversation/conversation-consent-popup/conversation-consent-popup-group.tsx (2)
1-1: LGTM! Clean import organization
The imports are well-organized and the addition of useGroupNameQuery aligns with the new hook-based approach.
Also applies to: 9-9
27-27: 🛠️ Refactor suggestion
Consider adding null check for account
The non-null assertion operator (!) assumes useCurrentAccount() will never return null. Consider adding proper error handling or early return to handle cases where the account might not be available.
- const account = useCurrentAccount()!;
+ const account = useCurrentAccount();
+ if (!account) {
+ return null; // or handle the error case appropriately
+ }✅ Verification successful
Based on the search results, I can now provide a conclusive response about the safety of using the non-null assertion with useCurrentAccount():
Non-null assertion is appropriate in this context
The codebase shows consistent patterns where useCurrentAccount() is used with the non-null assertion operator (!) in similar group/conversation contexts where the user must be authenticated. Looking at the usage patterns:
-
The hook is used with non-null assertion in similar group-related components:
conversation-consent-popup-group.tsxconversation-consent-popup-dm.tsxconversation-group-header-title.tsx- Multiple other group and conversation components
-
The component is part of a conversation feature that requires authentication to access, meaning the account will always be available when this code runs.
-
The codebase shows proper handling in places where the account might be null (like in the message utils where there's an explicit null check), but uses non-null assertion in authenticated contexts like group conversations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if useCurrentAccount can return null in other parts of the codebase
ast-grep --pattern 'const $account = useCurrentAccount()'
Length of output: 59
Script:
#!/bin/bash
# Let's try a broader search for useCurrentAccount usage patterns
rg "useCurrentAccount" -A 2 -B 2
Length of output: 71502
Script:
#!/bin/bash
# Let's also check the implementation of useCurrentAccount
ast-grep --pattern 'function useCurrentAccount() {
$$$
}'
Length of output: 61
Script:
#!/bin/bash
# Search for the hook definition in case it's exported differently
fd useCurrentAccount
Length of output: 59
queries/useGroupNameQuery.ts (2)
1-2: Imports Look Good
These imports appear consistent with the refactor to unify query options and the isConversationGroup utility. No issues noted.
12-13: Query Behavior and Data Selection
The usage of “select” to derive the group name (if the group is indeed a conversation group) is correct and a concise approach. Make sure that any consuming components handle the possibility of receiving “undefined” properly.
queries/useGroupDescriptionQuery.ts (3)
1-2: Imports Are Properly Organized
Bringing in isConversationGroup and getGroupQueryOptions ensures you can rely on the same utility patterns as in useGroupNameQuery. Good job maintaining consistency.
6-9: Use of a Single Argument Object
Consistently passing an object with account and topic is a solid design choice for consistent extensibility.
12-14: Selector Logic for Description
Leveraging the “select” option to return the group description shows a parallel structure to the name query. Just confirm that any consuming components safely handle the “undefined” scenario.
utils/mutate-object-properties.ts (3)
1-5: Plain Object Detection
Using a separate utility function (isPlainObject) is a good pattern that keeps the main logic clean and helps avoid incorrectly mutating arrays or other objects.
14-16: Default Recursion Level
Level 0 by default is a smart choice. This prevents unintended deep merges and potential performance drawbacks by default.
19-30: Nested Mutation Logic
The approach to recursively mutate only if both the existing and new values are plain objects is correct and avoids inadvertently overwriting array or non-object structures. Ensure thorough test coverage for nested edge cases.
queries/useGroupConsentQuery.ts (4)
1-5: Refactor to Shared Query Functions
Adopting getGroupQueryData and getGroupQueryOptions consolidates logic and improves maintainability. Good to see the DRY principle in action.
10-13: Argument Object Extraction
Moving from multiple parameters to one object argument is consistent with the refactor across other group queries, improving uniformity.
26-29: Retrieving Group State
Fetching the group via getGroupQueryData, then returning the “.state” property is consistent with the approach used in the name and description queries. Just ensure that the undefined scenario is handled by consumers.
36-41: Set Group Consent
This update logic is direct and consistent with how you’re handling group data in setGroupQueryData. Confirm that other parts of the code (like event listeners or side effects) are aware of updated consent states.
queries/useGroupQuery.ts (7)
1-3: Docstring and Comments Look Good
Lines 1–3 provide a brief description referencing a derivation from useConversationQuery. No issues.
4-9: Imports are Consistent
The imports from "@/queries/useConversationQuery" and "@/utils/mutate-object-properties" are aligned with the usage in this file. No concerns here.
15-24: useGroupQuery Implementation
The function correctly defers to useConversationQuery and returns a typed UseQueryResult. This pattern is consistent with the rest of the codebase.
26-34: Potential Absence of Data Handling
The function returns undefined if the conversation data does not exist. Confirm if upstream logic gracefully handles this scenario to avoid null/undefined dereferences.
36-47: Data Consistency with setGroupQueryData
Here, conversation data is set by mapping the group object to the conversation property. No issues spotted, but ensure the group object structure is always valid to avoid partial or malformed data in the query cache.
49-55: getGroupQueryOptions Validity
This function simply forwards the arguments to getConversationQueryOptions. The current approach is DRY (Don’t Repeat Yourself). Looks good.
57-72: updateGroupQueryData Partial Updates
Uses mutateObjectProperties to perform partial updates. This is an efficient approach that aligns with existing patterns. It’s good to see that if no prior group data is found, the function returns early.
queries/useDmQuery.ts (8)
4-8: Imports and QueryClient Usage
Imports from queryClient, isConversationDm, etc., are standard. The approach to capturing errors via captureError is consistent.
15-17: Type Definitions for IDmQueryArgs
Defining these types is helpful for clarity. The typed approach ensures correct arguments pass to getDm.
25-42: getDm Function with setConversationQueryData
When a conversation is fetched, it also updates the main “conversation” query data. This ensures data consistency across queries. No issues noted.
44-53: useDmQuery Hook
Using enabled: !!peer to toggle the query is a common pattern. The query key uses dmQueryKey(account, peer), ensuring separation from group queries.
54-110: Synchronization with Conversation List
Lines 54–110 use a QueryObserver for the conversation list to keep the DM data in sync. This is a thoughtful approach, but it’s somewhat complex. Confirm thorough testing to ensure no race conditions or stale data.
112-113: Return the Query Result
Straightforward code that returns the useQuery result. Maintain this for consistent patterns.
115-123: setDmQueryData Approach
setDmQueryData ensures the DM is placed into the dmQueryKey as well as the conversation query. This is consistent with how the code merges the conversation. No issues.
125-128: getDmQueryData Usage
Retrieving data from the query client is straightforward. Confirm all DM references in the codebase are replaced with this utility for data consistency.
queries/useConversationListQuery.ts (14)
1-9: Imports and Basic Setup
Bringing in setConversationQueryData and mutateObjectProperties for local updates is standard. The usage of QueryKeys.CONVERSATIONS ensures a stable query key.
18-29: createConversationListQueryObserver
Enables creating custom observers for conversation lists. This approach is flexible for advanced usage.
30-40: useConversationListQuery Hook
Wraps conversationListQueryConfig with useQuery. The partial override of queryOptions is neat.
42-53: fetchPersistedConversationListQuery
Provides a specific context for “fetchPersistedConversationListQuery” and sets includeSync = false. This is appropriate if fetch from local storage or some persisted data is desired.
55-63: fetchConversationListQuery
Similar approach, but includes full sync by default. Clear naming convention.
65-73: prefetchConversationListQuery
Allows prefetching conversation data. This option is useful for background or lazy loading.
75-83: refetchConversationListQuery
Wrapper for refetching queries by the conversationListQueryConfig. Straightforward usage.
85-108: addConversationToConversationListQuery
Checks if conversation already exists via topic before appending. This is important to avoid duplicates in the list.
110-128: updateConversationInConversationListQuery
Uses mutateObjectProperties for partial updates. This approach retains existing methods on the conversation object. Looks good.
130-147: replaceConversationInConversationListQuery
Fully replaces conversation if the topic matches. Good design for scenarios requiring a fresh conversation object.
149-157: getConversationListQueryData
This function is an alias to queryClient.getQueryData for the conversation list. Straightforward.
159-171: setConversationListQueryData
Mirrors getConversationListQueryData but for storing data. Contributes to a consistent pattern for reading and writing.
173-230: getConversationList Function and Error Handling
Fetching conversation data from the XMTP client with optional sync. Thorough logging is included. Good for debugging. The try/catch block logs and rethrows, which is straightforward.
232-247: conversationListQueryConfig Setup
Generates a stable query key with optional sync. Setting staleTime ensures near-future queries remain fresh. This approach helps performance.
features/conversation/utils/is-conversation-dm.ts (2)
1-4: Added DmWithCodecsType Import
Importing DmWithCodecsType allows the function to act as a TypeScript type predicate. Good improvement for type safety.
7-9: Type Predicate on isConversationDm
Explicitly narrowing the return type to DmWithCodecsType ensures TypeScript can refine the object when the function returns true. This is a beneficial enhancement to type safety.
features/conversation/utils/is-conversation-group.ts (1)
5-8: Great type safety improvement!
The addition of the type predicate conversation is GroupWithCodecsType enhances type safety by allowing TypeScript to narrow the type when the function returns true. This is a well-implemented type guard that will help catch type-related issues at compile time.
hooks/useConversationListGroupItem.ts (1)
8-16: LGTM: Parameter restructuring improves maintainability
The change to use a single object parameter with queryOptions is a good improvement for maintainability and clarity.
hooks/useGroupName.ts (1)
17-22: LGTM: Improved naming and parameter structure
The rename from setGroupName to updateGroupName is more descriptive, and the parameter restructuring improves consistency.
hooks/useGroupPhoto.ts (1)
Line range hint 17-22: LGTM: Clean parameter restructuring
The consolidation of parameters into objects improves code consistency and maintainability.
queries/useGroupCreatorQuery.ts (1)
7-22: LGTM! Well-structured query hook with performance optimizations.
The implementation follows React Query best practices with proper caching strategy and performance optimizations.
features/blocked-chats/useV3BlockedChats.ts (1)
3-3: Verify the migration from V3 query
The change from useV3ConversationListQuery to useConversationListQuery might affect other components.
✅ Verification successful
No remaining uses of useV3ConversationListQuery found
Based on the search results, there are no remaining instances of useV3ConversationListQuery in the codebase, and the new useConversationListQuery is consistently used across all relevant components. 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 uses of the old V3 query
rg "useV3ConversationListQuery" --type ts
# Search for all uses of the new query to ensure consistent usage
rg "useConversationListQuery" --type ts -A 3
Length of output: 9635
features/conversation/hooks/use-group-name-convos.ts (1)
17-20: LGTM! Good migration to object parameters.
The change to use object parameters improves code clarity and maintainability.
queries/useDmPeerInbox.ts (2)
11-15: LGTM! Parameter refactoring improves maintainability.
The change to use an object parameter with destructuring improves code maintainability and follows React best practices. This makes the function more extensible for future parameters.
27-30: LGTM! Improved DM validation logic.
The switch to isConversationDm utility function improves code clarity and maintainability. The enabled condition correctly mirrors the validation logic.
Also applies to: 32-32
queries/useGroupPermissionsQuery.ts (2)
20-21: LGTM! Consistent parameter pattern.
The update to use object parameters for useGroupQuery maintains consistency with other query hooks in the codebase.
Line range hint 24-36: LGTM! Improved encapsulation.
Converting getGroupPermissionsQueryOptions to an internal function improves encapsulation while maintaining type safety and functionality.
components/PinnedConversations/PinnedConversations.tsx (2)
2-2: LGTM! Consistent query pattern.
The migration from useV3ConversationListQuery to useConversationListQuery and the use of object parameters aligns with the codebase's evolution towards a more consistent API.
Also applies to: 16-23
16-23: Verify the impact of disabled refetch options.
While the parameter structure looks good, disabling both refetchOnWindowFocus and refetchOnMount might lead to stale data. Please verify if this is intentional and won't impact the user experience.
✅ Verification successful
Disabled refetch options are consistent with application patterns
The disabled refetch options in PinnedConversations are aligned with the established patterns across the codebase. Multiple components and queries consistently use refetchOnWindowFocus: false and refetchOnMount: false, including:
- Conversation list features (useV3ConversationItems, useV3RequestItems)
- Profile queries (useProfileSocialsQuery, useInboxProfileSocialsQuery)
- Group-related queries (useGroupCreatorQuery)
- Blocked chats (useV3BlockedChats)
This appears to be an intentional optimization strategy to prevent unnecessary refetches and maintain consistent data loading behavior throughout the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of these refetch options to understand the pattern
rg -A 2 'refetchOnWindowFocus:|refetchOnMount:' --type typescript
Length of output: 102
Script:
#!/bin/bash
# Search for refetch options with proper file extension
rg -A 2 'refetchOnWindowFocus:|refetchOnMount:' --type ts --type tsx
# Also search for the query hook usage to understand the pattern
rg -A 2 'useConversationListQuery' --type ts --type tsx
Length of output: 182
Script:
#!/bin/bash
# Search without specifying file types
rg -A 2 'refetchOnWindowFocus:|refetchOnMount:'
# Search for the query hook usage pattern
rg -A 2 'useConversationListQuery'
# List all files containing these patterns to understand the context better
rg --files-with-matches 'useConversationListQuery'
Length of output: 11354
features/conversation/conversation-title.tsx (1)
46-48: LGTM: Good accessibility practices
The Text component correctly implements accessibility best practices with numberOfLines and allowFontScaling props.
features/conversation/conversation-new-dm-header-title.tsx (1)
45-45: Verify avatar size constant usage across the app
The switch to theme.avatarSize.md looks good, but let's ensure this size is used consistently across other avatar implementations.
✅ Verification successful
The previous regex had syntax issues. Let's try with a corrected pattern to find avatar size usages and theme avatar size references across the codebase.
Let's try without the file type restrictions since they seem to be unsupported in the environment.
Avatar size usage is consistent with theme system
The codebase shows consistent usage of theme-based avatar sizes:
- Most components use
theme.avatarSizewith appropriate size variants (md, sm, xs, xxl) - The change to
theme.avatarSize.mdin conversation-new-dm-header-title.tsx aligns with the pattern used in similar components:- conversation-dm-header-title.tsx
- conversation-group-header-title.tsx
- conversation-message-reaction-drawer.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other avatar size usages to ensure consistency
rg -A 1 "size={.*avatar" --type typescript --type typescript-react
Length of output: 171
Script:
#!/bin/bash
# Search for avatar size usage patterns
rg "size=\{.*\}" --type typescript --type typescript-react -A 1 | rg -A 1 "avatar|Avatar"
# Also search for direct avatar size references
rg "avatarSize" --type typescript --type typescript-react -A 1
Length of output: 224
Script:
#!/bin/bash
# Search for avatar size usage patterns without file type restrictions
rg "size=\{" -A 1 | rg -A 1 "avatar|Avatar"
# Search for theme avatar size references
rg "avatarSize" -A 1
Length of output: 12562
features/conversation-list/useV3ConversationItems.ts (1)
16-16: LGTM: Good use of context for query tracing
Adding a context identifier helps with debugging and tracing query usage throughout the application.
queries/useGroupPhotoMutation.ts (1)
33-37: Avoid type assertion and validate group structure
The type assertion to GroupWithCodecsType could be unsafe. Consider validating the group structure before the cast.
queries/useRevokeAdminMutation.ts (2)
20-20: LGTM: Parameter structure improvement
The updated parameter structure for useGroupQuery improves type safety and readability by using a named object parameter.
51-51: Verify error handling transition
The transition from sentryTrackError to captureError looks good, but ensure error tracking configuration is properly set up.
✅ Verification successful
Error handling transition is properly implemented
The transition from sentryTrackError to captureError is correctly implemented. The captureError function internally uses sentryTrackError for error tracking, maintaining the same error reporting functionality while adding additional features like dev environment logging. The implementation in useRevokeAdminMutation.ts is using this new error handling pattern appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining instances of sentryTrackError to ensure consistent error handling
rg "sentryTrackError" --type typescript
Length of output: 74
Script:
#!/bin/bash
# Check for any remaining instances of sentryTrackError using ripgrep without file type filter
rg "sentryTrackError"
# Also check for captureError usage to understand the error handling pattern
rg "captureError"
Length of output: 14959
Script:
#!/bin/bash
# Check the implementation of captureError to verify it's using sentryTrackError correctly
cat utils/capture-error.ts
Length of output: 775
queries/usePromoteToSuperAdminMutation.ts (1)
Line range hint 31-33: LGTM: Improved error handling
Good addition of explicit error message for missing topic requirement.
queries/useRemoveFromGroupMutation.ts (2)
21-21: LGTM: Parameter structure change aligns with codebase standards
The update to use an object parameter for useGroupQuery improves code clarity and maintains consistency with similar query hooks.
55-55: LGTM: Simplified error handling
The switch from sentryTrackError to captureError streamlines error reporting while maintaining tracking capabilities.
queries/useConversationPreviewMessages.ts (1)
45-49: LGTM: Improved query parameter structure
The update to use an object parameter for useConversationQuery enhances code clarity and maintains consistency with other query hooks.
features/conversation/conversation.screen.tsx (1)
69-71: LGTM: Clean conditional rendering
The conditional rendering with dmConversation is well-structured and maintains a clear flow between conversation states.
queries/usePromoteToAdminMutation.ts (1)
57-57: Verify error tracking migration
The change from sentryTrackError to captureError appears to be part of a broader error handling refactor. Ensure all error context is preserved in the new implementation.
✅ Verification successful
Let me analyze the error handling migration by checking the implementation of both functions.
Let me check the implementation of captureError with a different pattern since it might be defined as a function declaration.
Error handling change is safe and consistent
The change from sentryTrackError to captureError is part of a proper error handling refactor. The captureError function internally uses sentryTrackError, maintaining the same error tracking functionality while adding dev environment logging. All error context is preserved as captureError passes the error object directly to sentryTrackError.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if sentryTrackError is being consistently replaced across the codebase
rg "sentryTrackError" --type ts
rg "captureError" --type ts
Length of output: 12570
Script:
#!/bin/bash
# Check implementation of both error handling functions
ast-grep --pattern 'export const sentryTrackError = $_'
ast-grep --pattern 'export function captureError($_)'
Length of output: 403
Script:
#!/bin/bash
# Check the implementation of captureError in the capture-error.ts file
cat utils/capture-error.ts
Length of output: 775
features/conversation-requests-list/useV3RequestItems.tsx (1)
11-18: Review refetching strategy
Disabling both refetchOnWindowFocus and refetchOnMount might affect the freshness of conversation data. Consider if this could lead to stale data issues, especially for active conversations.
components/StateHandlers/HydrationStateHandler.tsx (1)
2-2: Verify V3 to new conversation list query migration
Ensure all V3 conversation list query references have been updated consistently across the codebase.
screens/Group.tsx (1)
35-38: LGTM! Consistent parameter pattern update.
The change to use an object parameter for useGroupQuery improves code maintainability and readability.
Let's verify the consistency of this pattern across the codebase:
✅ Verification successful
Let me gather more context about the actual implementation of useGroupQuery to understand the parameter pattern better.
Let me try a different approach to find the useGroupQuery implementation.
Let me check the implementation of useGroupQuery to verify its parameter pattern.
LGTM! The object parameter pattern is consistently used across the codebase
Looking at the implementation and all usages across the codebase, the object parameter pattern for useGroupQuery is consistently implemented and used. The hook is defined to accept an object parameter and all its usages across the codebase follow this pattern. This change aligns well with the codebase's conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct parameter usage of useGroupQuery
# to ensure consistent pattern adoption
ast-grep --pattern 'useGroupQuery($$$)' | grep -v '{' || echo "No direct parameter usage found"
Length of output: 502
Script:
#!/bin/bash
# Find the implementation of useGroupQuery
ast-grep --pattern 'export function useGroupQuery($_' -A 10
Length of output: 61
Script:
#!/bin/bash
# Search for useGroupQuery definition using ripgrep for better context
rg "useGroupQuery" -A 5 -B 5
Length of output: 30532
Script:
#!/bin/bash
# Find the implementation of useGroupQuery
cat queries/useGroupQuery.ts
Length of output: 1834
hooks/useGroupConsent.ts (1)
21-24: LGTM! Consistent parameter pattern update.
The change to use object parameters for queries improves code maintainability and readability.
Also applies to: 33-33
queries/QueryKeys.ts (2)
7-7: Verify the impact of renaming CONVERSATION_WITH_PEER to CONVERSATION_DM
The renaming of CONVERSATION_WITH_PEER to CONVERSATION_DM and corresponding function conversationWithPeerQueryKey to dmQueryKey could impact existing code that relies on these keys.
Also applies to: 41-44
✅ Verification successful
The query key renaming appears safe to proceed
The old conversationWithPeerQueryKey only appears in a commented-out code block in features/conversation/conversation-new-dm.tsx. The new CONVERSATION_DM key and its corresponding dmQueryKey function are properly defined and tested. No active usage of the old key was found, indicating this is a clean rename.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of old query key
rg -i "CONVERSATION_WITH_PEER|conversationWithPeerQueryKey" --type ts
Length of output: 187
Script:
#!/bin/bash
# Check the context of the commented out code and any DM related query key usage
rg -B 3 -A 3 "conversationWithPeerQueryKey|CONVERSATION_DM" --type ts
Length of output: 1779
19-19: Verify the implementation of new GROUP_CREATOR functionality
The addition of GROUP_CREATOR key and groupCreatorQueryKey suggests new group creator functionality. Ensure this is properly implemented across the codebase.
Also applies to: 80-83
✅ Verification successful
Let me gather more information about the implementation by checking the group creator functionality usage and related components.
GROUP_CREATOR functionality is properly implemented
The implementation is complete and well-structured:
GROUP_CREATORkey is used ingroupCreatorQueryKeyfor React Query cachinguseGroupCreatorQueryhook properly fetches the creator's inbox ID from the group- The functionality is integrated into the group consent system via
useGroupConsenthook - Test coverage exists in
QueryKeys.test.ts - The creator functionality is used in group management features, particularly in join/invite flows
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for group creator implementation
ast-grep --pattern 'GROUP_CREATOR'
rg "groupCreatorQueryKey" -A 5 --type ts
Length of output: 1932
Script:
#!/bin/bash
# Search for usage of useGroupCreatorQuery
rg "useGroupCreatorQuery" -A 5
# Search for any group creator related implementations
rg "creatorInboxId" -A 5
Length of output: 4272
screens/ConversationReadOnly.tsx (1)
30-33: Ensure consistent parameter structure across query hooks
The change to object parameter style for useConversationQuery is good, but ensure this pattern is consistent across all query hooks in the codebase.
✅ Verification successful
Parameter structure is already consistent across query hooks
Based on the verification results, the query hooks in the codebase consistently follow object parameter patterns. Most query hooks use a structured object parameter approach with named fields (typically including account, topic, and other relevant parameters). The useConversationQuery change aligns with this established pattern, as seen in similar hooks like useConversationListQuery, useGroupDescriptionQuery, and others.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other query hook usages
ast-grep --pattern 'use$_Query('
Length of output: 34
Script:
#!/bin/bash
# Search for query hook usages with ripgrep for better pattern matching
rg "use\w+Query" -t typescript -t tsx --context 2
Length of output: 86
Script:
#!/bin/bash
# Search for specific useConversationQuery usage pattern
rg "useConversationQuery" -t typescript -t tsx --context 2
Length of output: 93
Script:
#!/bin/bash
# Find all files that might contain query hooks
fd -e ts -e tsx | xargs grep -l "Query"
Length of output: 4603
Script:
#!/bin/bash
# Search for query hook usage patterns in the queries directory
grep -r "export.*use.*Query" queries/
Length of output: 1860
Script:
#!/bin/bash
# Look for specific parameter patterns in query hooks
grep -A 5 "export.*use.*Query.*=" queries/use*Query.ts
Length of output: 8291
components/Connecting.tsx (1)
70-70: LGTM: Query hook migration looks correct
The migration from useV3ConversationListQuery to useConversationListQuery with the new object-based argument structure is properly implemented.
queries/useConversationQuery.ts (2)
18-21: LGTM: Type-safe argument structure
Good use of TypeScript interface to enforce type safety for function arguments.
45-72: Verify potential race conditions in sync mechanism
The new sync mechanism using QueryObserver could potentially have race conditions if multiple updates occur simultaneously.
✅ Verification successful
Based on the analysis of the code, I can now generate the final response:
Race conditions are properly handled in the sync mechanism
The sync mechanism using QueryObserver is well-implemented with proper safeguards:
- The observer is properly scoped within a useEffect hook with correct dependencies
- The subscription cleanup is handled via the observer.destroy() in the effect cleanup
- The sync logic is unidirectional (list is source of truth) which prevents circular updates
- The setQueryData operation is only performed when listConversation exists, preventing invalid states
- The implementation follows React Query's recommended patterns for real-time synchronization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other QueryObserver usages and concurrent data updates
rg -A 3 "QueryObserver"
rg -A 3 "setQueryData.*conversationQueryKey"
Length of output: 2548
Script:
#!/bin/bash
# Let's search for all setQueryData usages and concurrent data modifications
rg -A 5 "setQueryData"
# Also search for any mutation or update logic related to conversations
rg -A 5 "useMutation.*conversation"
# Look for any error handling or race condition prevention
rg -A 5 "optimisticUpdate|onMutate|onError|onSettled"
Length of output: 45166
queries/useAllowGroupMutation.ts (2)
88-95: LGTM: Clean optimistic updates
The onMutate handler is well-implemented with proper optimistic updates and previous state tracking.
54-87: Verify concurrent operations handling
The mutation function now uses Promise.all for concurrent operations, which is more efficient but needs careful error handling.
✅ Verification successful
Concurrent operations are properly implemented
The Promise.all usage in the mutation is safe and well-structured because:
- The underlying consent functions (consentToGroupsOnProtocolByAccount and consentToInboxIdsOnProtocolByAccount) have proper error handling with specific error types.
- Each consent function already includes logging and proper client validation.
- The Promise.all array is conditionally constructed to avoid empty promises.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar concurrent operations patterns
rg -A 5 "Promise\.all.*consentTo"
Length of output: 35
Script:
#!/bin/bash
# Let's search for the consent-related function implementations
ast-grep --pattern 'consentToGroupsOnProtocolByAccount'
# Also search for any similar consent operations
rg "consentTo.*Protocol.*Account"
# Let's also check for any error handling patterns around Promise.all
rg -A 5 "Promise\.all.*catch"
Length of output: 5462
Script:
#!/bin/bash
# Let's check the implementation of these consent functions to understand their error handling
cat utils/xmtpRN/contacts.ts
# Let's also check for any try-catch blocks around Promise.all in the mutation file
rg -B 5 -A 5 "Promise\.all" queries/useAllowGroupMutation.ts
Length of output: 7238
queries/QueryKeys.test.ts (2)
34-149: LGTM! Well-organized test structure with comprehensive coverage.
The test suite is well-organized into logical sections (Conversations, Messages, Members, etc.) with consistent test patterns. Each query key function is properly tested for correct array structure and account handling.
150-156: Good addition of case sensitivity test.
The case sensitivity test ensures consistent lowercase handling of account names across all query keys, which is crucial for preventing case-related bugs.
features/conversation/conversation-group-header-title.tsx (1)
103-107: LGTM! Clear type definition.
The IMemberData interface is well-defined with clear optional properties.
components/PinnedConversations/PinnedV3DMConversation.tsx (1)
40-43:
Ensure non-null assertion is safe.
The non-null assertion on currentAccount could lead to runtime errors if the account is not available.
- const { data: peerInboxId } = useDmPeerInboxId({
- account: currentAccount!,
- topic,
- });
+ const { data: peerInboxId } = useDmPeerInboxId({
+ account: currentAccount ?? undefined,
+ topic,
+ });✅ Verification successful
Based on the extensive search results, I can now generate the final response:
Non-null assertion on currentAccount is safe in this context.
The non-null assertion on currentAccount in PinnedV3DMConversation.tsx is safe because:
- The component is only rendered within authenticated contexts where
currentAccountis guaranteed to be available. - The pattern is consistently used across the codebase in similar components and hooks.
- The
useCurrentAccounthook is used in conjunction with components that are only mounted after authentication.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for null checks on currentAccount in other files
rg "currentAccount\!" -A 2 -B 2
Length of output: 130
Script:
#!/bin/bash
# Check for null checks on currentAccount in the codebase
rg "currentAccount!" -A 2 -B 2
Length of output: 9103
Script:
#!/bin/bash
# Find the component file to understand the context
fd "PinnedV3DMConversation.tsx" --type f
# Check how currentAccount is defined/used in the component
ast-grep --pattern 'const { $$ } = $expr'
Length of output: 142
Script:
#!/bin/bash
# Look for similar DM conversation components
fd "DMConversation" --type f
# Check for any existing null checks or undefined handling patterns
rg "(\?\?|===|!==)\s*(undefined|null)" --type ts
Length of output: 5350
Script:
#!/bin/bash
# Check how currentAccount is obtained/defined in the component
rg "useCurrentAccount" -A 2 -B 2
# Look for the definition of useDmPeerInboxId hook
fd "useDmPeerInboxId" --type f
Length of output: 65658
features/conversation/conversation-new-dm.tsx (1)
56-59: LGTM! Good component composition.
The wrapping of Composer with ConversationComposerContainer and the addition of messageContextMenuIsOpen prop improve the component structure and keyboard handling.
components/V3DMListItem.tsx (1)
1-6: Verify the behavior of useDmPeerInboxId hook.
The replacement of useDmPeerInboxOnConversationList with useDmPeerInboxId might affect other components using this hook.
✅ Verification successful
No references to the old hook found, new hook consistently used across components
The verification shows that:
- The old hook
useDmPeerInboxOnConversationListhas no references in the codebase - The new hook
useDmPeerInboxIdis consistently used across 4 components:V3DMListItem.tsxconversation-dm-header-title.tsxconversation-consent-popup-dm.tsxPinnedV3DMConversation.tsx
Let's verify the implementation to ensure consistent behavior:
Hook migration is complete and implementation is correct
The new useDmPeerInboxId hook is properly implemented with:
- Correct dependency on
useConversationQuery - Proper type checking with
isConversationDm - Consistent error handling
- Appropriate query key management
- Safe enabling conditions
The old hook has been completely removed, and the new hook is being used consistently across all relevant components with the same parameter structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of the old hook to ensure complete migration
rg "useDmPeerInboxOnConversationList"
# Search for all usages of the new hook to verify consistent usage
rg "useDmPeerInboxId"
Length of output: 1041
Script:
#!/bin/bash
# Check the implementation of the new hook
cat queries/useDmPeerInbox.ts
Length of output: 1045
containers/GroupScreenAddition.tsx (1)
Line range hint 1-43: Verify the impact of replacing useGroupName with useGroupNameQuery.
The hook replacement might affect other components using the group name functionality.
✅ Verification successful
Based on the search results, I can now generate the final response:
The hook replacement is safe and follows the established pattern
The codebase shows that useGroupNameQuery is already being used consistently:
- It's the underlying implementation used by
useGroupNamehook inhooks/useGroupName.ts - Other components like
ConversationConsentPopupGroupandGroupNameConvosare already using it directly - The change in
GroupScreenAddition.tsxaligns with the existing usage pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of the old hook
rg "useGroupName"
# Search for all usages of the new hook
rg "useGroupNameQuery"
# Search for components that might be affected by this change
ast-grep --pattern 'const { data: groupName } = useGroupNameQuery'
Length of output: 3995
queries/useConversationMessages.ts (2)
27-39: LGTM! Proper error handling implemented.
The conversationMessagesQueryFn and conversationMessagesByTopicQueryFn functions now include appropriate null checks and error handling.
246-303: 🛠️ Refactor suggestion
Enhance error handling in replaceOptimisticMessageWithReal.
The function handles optimistic updates but could benefit from additional error handling and validation.
Apply this diff to improve error handling:
export function replaceOptimisticMessageWithReal(args: {
tempId: string;
topic: ConversationTopic;
account: string;
message: DecodedMessageWithCodecsType;
}) {
const { tempId, topic, account, message } = args;
+ if (!tempId || !topic || !account || !message) {
+ logger.error("[replaceOptimisticMessageWithReal] Missing required parameters", args);
+ return;
+ }
+
logger.info(
- "[linkOptimisticMessageToReal] linking optimistic message to real",
+ "[replaceOptimisticMessageWithReal] replacing optimistic message with real",
{
tempId,
messageId: message.id,
}
);
queryClient.setQueryData<ConversationMessagesQueryData>(
conversationMessagesQueryKey(account, topic),
(previousMessages) => {
if (!previousMessages) {
+ logger.info("[replaceOptimisticMessageWithReal] No previous messages found");
return {
ids: [message.id as MessageId],
byId: {
[message.id as MessageId]: message,
},
reactions: {},
};
}
const tempIndex = previousMessages.ids.indexOf(tempId as MessageId);
if (tempIndex === -1) {
+ logger.warn("[replaceOptimisticMessageWithReal] Temp message not found", { tempId });
return previousMessages;
}
const newIds = [...previousMessages.ids];
newIds[tempIndex] = message.id as MessageId;
const newById = { ...previousMessages.byId };
delete newById[tempId as MessageId];
newById[message.id as MessageId] = message;
+ logger.info("[replaceOptimisticMessageWithReal] Successfully replaced message");
return {
...previousMessages,
ids: newIds,
byId: newById,
};
}
);
}Likely invalid or redundant comment.
features/conversation/conversation.tsx (2)
297-301: LGTM! Clean implementation of message status.
The conditional rendering of ConversationMessageStatus is well implemented and properly optimized.
193-200: 🛠️ Refactor suggestion
Optimize message filtering performance.
The current implementation of latestMessageIdByCurrentUser could be performance intensive for large message lists.
Consider using findLast for better performance:
const latestMessageIdByCurrentUser = useMemo(() => {
if (!messages?.ids) return -1;
- return messages.ids.find(
+ return messages.ids.findLast(
(messageId) =>
isAnActualMessage(messages.byId[messageId]) &&
messages.byId[messageId].senderAddress === currentAccountInboxId
);
}, [messages?.ids, messages?.byId, currentAccountInboxId]);Likely invalid or redundant comment.
screens/NewConversation/NewConversation.tsx (2)
22-27: LGTM! Well-organized imports
The new imports are logically grouped and provide essential functionality for internationalization, address handling, and UI components.
58-61: Verify the useGroupQuery hook usage across the codebase
The change from positional parameters to an object parameter is a good practice for maintainability. However, this is a breaking change that needs verification across all usages.
✅ Verification successful
All useGroupQuery usages are consistent with the object parameter pattern
Based on the verification results:
- The hook definition in
useGroupQuery.tsuses object parameters - All 19 usages found across the codebase consistently use the object parameter pattern with
{ account, topic } - No instances of positional parameters were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all useGroupQuery usages to ensure they've been updated
rg "useGroupQuery\(" --type typescript
Length of output: 75
Script:
#!/bin/bash
# Search for all useGroupQuery usages with ripgrep, without file type filtering
rg "useGroupQuery\("
# Also try finding the hook definition
ast-grep --pattern 'const useGroupQuery = $_'
Length of output: 1832
Script:
#!/bin/bash
# Let's check the hook definition to confirm the parameter structure
cat queries/useGroupQuery.ts
Length of output: 1834
| const { data: peerInboxId } = useDmPeerInboxId({ account, topic }); | ||
|
|
||
| const peerAddress = usePreferredInboxAddress(peerInboxId!); |
There was a problem hiding this comment.
Add error handling for peer inbox ID
The non-null assertion (!) on peerInboxId could lead to runtime errors if the query fails or returns null. Consider adding proper error handling:
- const peerAddress = usePreferredInboxAddress(peerInboxId!);
+ const peerAddress = peerInboxId ? usePreferredInboxAddress(peerInboxId) : undefined;Also consider adding a loading state:
- const { data: peerInboxId } = useDmPeerInboxId({ account, topic });
+ const { data: peerInboxId, isLoading: isLoadingInbox } = useDmPeerInboxId({ account, topic });📝 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: peerInboxId } = useDmPeerInboxId({ account, topic }); | |
| const peerAddress = usePreferredInboxAddress(peerInboxId!); | |
| const { data: peerInboxId, isLoading: isLoadingInbox } = useDmPeerInboxId({ account, topic }); | |
| const peerAddress = peerInboxId ? usePreferredInboxAddress(peerInboxId) : undefined; |
| import { addMemberMutationKey } from "./MutationKeys"; | ||
| import { | ||
| cancelGroupMembersQuery, | ||
| invalidateGroupMembersQuery, | ||
| } from "./useGroupMembersQuery"; | ||
| import { useGroupQuery } from "@queries/useGroupQuery"; | ||
| import type { ConversationTopic } from "@xmtp/react-native-sdk"; | ||
| // import { refreshGroup } from "../utils/xmtpRN/conversations"; |
There was a problem hiding this comment.
💡 Codebase verification
Remove commented out refreshGroup code across mutation files
The refreshGroup functionality appears to have been systematically commented out across multiple mutation files, indicating an intentional refactoring:
- All mutation files have both the import and the function call commented out
- The functionality has been moved to the
joinGroupfeature with a different implementation - No active usage of the old
refreshGroupimport exists
Recommended actions:
- Remove the commented out import on line 12
- Remove the commented out function call on line 43
- Apply the same cleanup to similar commented code in:
useRemoveFromGroupMutation.tsusePromoteToAdminMutation.tsuseRevokeSuperAdminMutation.tsusePromoteToSuperAdminMutation.tsuseRevokeAdminMutation.ts
🔗 Analysis chain
Clean up or restore commented code
There are two instances of commented out code related to refreshGroup. This should either be removed if no longer needed or restored if the functionality is required. Could you clarify the intention?
Also applies to: 43-43
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if refreshGroup is used elsewhere in the codebase
# Look for other usages of refreshGroup
rg -t typescript "refreshGroup.*\(.*\)" "utils/" "queries/" "features/"
# Check if there are any related TODO comments
rg -t typescript "TODO.*refresh.*group" "utils/" "queries/" "features/"
Length of output: 205
Script:
#!/bin/bash
# Search for refreshGroup function usage without file type restriction
rg "refreshGroup.*\("
# Search for the refreshGroup function definition
ast-grep --pattern 'function refreshGroup($_) { $$$ }'
# Search for any commented out code related to refreshGroup
rg "//.*refreshGroup"
Length of output: 2484
| @@ -33,8 +33,7 @@ | |||
| await cancelGroupMembersQuery(account, topic); | |||
| }, | |||
| onError: (error, _variables, _context) => { | |||
| logger.warn("onError useAddToGroupMutation"); | |||
| sentryTrackError(error); | |||
| captureError(error); | |||
There was a problem hiding this comment.
💡 Codebase verification
Migration to captureError is in progress but incomplete
The codebase shows an inconsistent state of error handling:
- Several mutation files still have both old (
sentryTrackError,logger) and new (captureError) error handling imports - However, the actual error handling in the
onErrorcallbacks consistently uses the newcaptureErrorpattern - The old imports appear to be unused legacy code that wasn't cleaned up during the migration
Recommended actions:
- Remove unused imports for
loggerandsentryTrackErrorfrom:queries/useRevokeSuperAdminMutation.tsqueries/useRevokeAdminMutation.tsqueries/usePromoteToAdminMutation.tsqueries/useRemoveFromGroupMutation.ts
🔗 Analysis chain
Verify consistent error handling pattern
The simplified error handling using captureError looks good, but let's ensure this pattern is consistently applied across similar mutations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for inconsistent error handling patterns in mutation hooks
# Look for other mutation files that might still use the old error handling
rg -t typescript "sentryTrackError|logger\.warn.*error" "queries/" "features/"
# Check the usage pattern of captureError
rg -t typescript "captureError" "queries/" "features/"
Length of output: 197
Script:
#!/bin/bash
# Let's try without typescript file type flag since it's not recognized
# Search for old error handling patterns
rg "sentryTrackError|logger\.warn.*error" "queries/" "features/" -l
# Search for new captureError pattern
rg "captureError" "queries/" "features/" -l
Length of output: 1804
Script:
#!/bin/bash
# Let's examine the actual error handling implementations in mutation files
# to see if there's a mix of patterns in the same files
# Check files that showed up in both searches
for file in queries/useRevokeSuperAdminMutation.ts queries/useRevokeAdminMutation.ts queries/usePromoteToAdminMutation.ts queries/useRemoveFromGroupMutation.ts; do
echo "=== $file ==="
rg "captureError|sentryTrackError|logger\.warn.*error" "$file" -A 2 -B 2
done
Length of output: 3851
| currentAccount!, | ||
| { | ||
| const { data: groups } = useConversationListQuery({ | ||
| account: currentAccount!, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling null currentAccount case.
The non-null assertion (!) on currentAccount could lead to runtime errors if the account is null.
- account: currentAccount!,
+ account: currentAccount ?? throw new Error("No current account found"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| account: currentAccount!, | |
| account: currentAccount ?? throw new Error("No current account found"), |
| const { data: groups } = useConversationListQuery({ | ||
| account: currentAccount!, | ||
| queryOptions: { | ||
| refetchOnWindowFocus: false, | ||
| refetchOnMount: false, | ||
| }, | ||
| "useV3RequestItemCount" | ||
| ); | ||
| context: "useV3RequestItemCount", | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
V3 prefix removal appears to be inconsistent with the codebase
The codebase shows active usage of V3 prefixes across multiple files and features. The file under review useV3RequestItemCount.ts still maintains its V3 prefix in the filename while using the non-prefixed useConversationListQuery. This inconsistency suggests an incomplete migration:
useV3RequestItemCount.ts- Filename has V3 prefix but uses non-prefixed queryuseV3ConversationItems.ts- Still uses V3 prefixuseV3BlockedChats.ts- Still uses V3 prefixuseV3RequestItems.tsx- Still uses V3 prefix
The codebase appears to be in a transitional state where V3 functionality is still actively used, suggesting the V3 prefix removal in this specific case might be unintentional.
🔗 Analysis chain
Verify the migration from V3 prefix.
The change from useV3ConversationListQuery to useConversationListQuery suggests an API version change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other V3 prefixed conversation queries that might need migration
rg "useV3.*Conversation.*Query" -t ts
Length of output: 39
Script:
#!/bin/bash
# Let's search for both old and new query hook names to understand the migration pattern
rg "use(V3)?ConversationListQuery" -t ts
# Also search for any remaining V3 prefixes in the codebase
rg "V3" -t ts
Length of output: 16985
| @@ -48,7 +48,7 @@ export const useGroupMembersQuery = ( | |||
| account: string, | |||
| topic: ConversationTopic | |||
| ) => { | |||
| const { data: group } = useGroupQuery(account, topic); | |||
| const { data: group } = useGroupQuery({ account, topic }); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating duplicate query hooks
The useGroupMembersQuery and useGroupMembersConversationScreenQuery hooks are identical in implementation. Consider consolidating them to reduce code duplication.
- export const useGroupMembersConversationScreenQuery = (
- account: string,
- topic: ConversationTopic
- ) => {
- const { data: group } = useGroupQuery({ account, topic });
- const enabled = !!group && !!topic;
- return useQuery<GroupMembersSelectData>(
- groupMembersQueryConfig(account, group, enabled)
- );
- };Also applies to: 62-62
queries/useConversationQuery.ts
Outdated
| args: IArgs & { | ||
| conversation: ConversationQueryData; | ||
| } | ||
| ) => { | ||
| queryClient.setQueryData<ConversationQueryData>( | ||
| conversationQueryKey(account, topic), | ||
| conversation | ||
| ); | ||
| const { account, topic, conversation } = args; | ||
| // Source of truth for now | ||
| replaceConversationInConversationListQuery({ | ||
| account, | ||
| topic, | ||
| conversation, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for list updates
The setConversationQueryData function now updates the conversation list as the source of truth, but lacks error handling for failed updates.
export const setConversationQueryData = (
args: IArgs & {
conversation: ConversationQueryData;
}
) => {
const { account, topic, conversation } = args;
- // Source of truth for now
- replaceConversationInConversationListQuery({
- account,
- topic,
- conversation,
- });
+ try {
+ replaceConversationInConversationListQuery({
+ account,
+ topic,
+ conversation,
+ });
+ } catch (error) {
+ console.error('Failed to update conversation list:', error);
+ // Consider rolling back or retrying the update
+ }
};📝 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.
| args: IArgs & { | |
| conversation: ConversationQueryData; | |
| } | |
| ) => { | |
| queryClient.setQueryData<ConversationQueryData>( | |
| conversationQueryKey(account, topic), | |
| conversation | |
| ); | |
| const { account, topic, conversation } = args; | |
| // Source of truth for now | |
| replaceConversationInConversationListQuery({ | |
| account, | |
| topic, | |
| conversation, | |
| }); | |
| args: IArgs & { | |
| conversation: ConversationQueryData; | |
| } | |
| ) => { | |
| const { account, topic, conversation } = args; | |
| try { | |
| replaceConversationInConversationListQuery({ | |
| account, | |
| topic, | |
| conversation, | |
| }); | |
| } catch (error) { | |
| console.error('Failed to update conversation list:', error); | |
| // Consider rolling back or retrying the update | |
| } |
| try { | ||
| // Then, send the message | ||
| await sendMessageAsync({ | ||
| conversation, | ||
| params: args, | ||
| }); | ||
| } catch (error) { | ||
| showSnackbar({ message: "Failed to send message" }); | ||
| sentryTrackError(error); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify nested error handling structure.
The nested try-catch blocks make the code harder to maintain. Consider extracting the message sending logic into a separate function.
-try {
- // Then, send the message
- await sendMessageAsync({
- conversation,
- params: args,
- });
-} catch (error) {
- showSnackbar({ message: "Failed to send message" });
- sentryTrackError(error);
-}
+await sendInitialMessage(conversation, args).catch((error) => {
+ showSnackbar({ message: "Failed to send message" });
+ sentryTrackError(error);
+});
+async function sendInitialMessage(conversation, args) {
+ return sendMessageAsync({
+ conversation,
+ params: args,
+ });
+}| const { data: peerInboxId } = useDmPeerInboxId({ | ||
| account: currentAccount!, | ||
| topic, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for missing peerInboxId.
The hook usage should handle cases where peerInboxId data is undefined.
- const { data: peerInboxId } = useDmPeerInboxId({
+ const { data: peerInboxId, error } = useDmPeerInboxId({
account: currentAccount!,
topic,
});
+
+ if (error) {
+ console.error('Failed to fetch peer inbox ID:', error);
+ }
+
+ if (!peerInboxId) {
+ // Handle missing data case
+ return null;
+ }Committable suggestion skipped: line range outside the PR's diff.
| const { messageId, itemRectX, itemRectY, itemRectHeight, itemRectWidth } = | ||
| messageContextMenuData; | ||
|
|
||
| const account = useCurrentAccount()!; | ||
| const topic = useCurrentConversationTopic(); | ||
| const messageContextMenuStore = useMessageContextMenuStore(); | ||
| const { data: conversation } = useConversationQuery(account, topic); | ||
| const { data: conversation } = useConversationQuery({ | ||
| account, | ||
| topic, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving type safety by removing non-null assertions
The code uses multiple non-null assertions (!) which could lead to runtime errors. Consider adding proper null checks or using optional chaining.
Example safer approach:
- const account = useCurrentAccount()!;
+ const account = useCurrentAccount();
+ if (!account) {
+ return null;
+ }
- const { data: conversation } = useConversationQuery({
+ const { data: conversation, isLoading } = useConversationQuery({
account,
topic,
});
+ if (!conversation || isLoading) {
+ return null;
+ }Committable suggestion skipped: line range outside the PR's diff.
thierryskoda
changed the title
thierryskoda
changed the title
*** I will provide a Loom shorty to explain the changes
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
ConversationComposerContainercomponent for improved conversation composition.ConversationMessageStatusto display message statuses with animated transitions.useKeyboardHandlersanduseKeyboardIsShownhooks for better keyboard management.Enhancements
ReplyPreviewcomponent by integrating logic directly.KeyboardFillerto manage keyboard visibility based on context menu state.sendMessagefunctionality.maxWidthproperty to theBubbleContentContainerfor better message bubble responsiveness.Bug Fixes
openMessageReactionsDrawerfunction.Refactor
Chores