fix: double tap reaction + keyboard staying in place when long press on message + fix for new SDK senderInboxId + many other stuff #1401
Conversation
|
Warning Rate limit exceeded@thierryskoda has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 42 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (42)
WalkthroughThis pull request introduces significant refactoring of conversation management across multiple files in the project. The changes primarily focus on simplifying conversation-related types, updating component structures, and streamlining message interaction logic. Key modifications include removing complex conversation types, updating gesture handling for messages, and enhancing hooks for group names and message reactions. The refactoring aims to create a more straightforward and maintainable approach to managing conversations and message interactions. Changes
Possibly related PRs
Suggested labels
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 (
|
| @@ -191,15 +74,12 @@ export const initChatStore = (account: string) => { | |||
| persist( | |||
| (set) => | |||
| ({ | |||
| conversations: {}, | |||
There was a problem hiding this comment.
just so i am tracking this was moved to be handled by react query?
There was a problem hiding this comment.
Exactly! the goal would be to erase ChatStore I think. So as soon as I'm moving stuff away I'm trying to clean from ChatStore
There was a problem hiding this comment.
Yes! A chat store is fine, but the previous chat store was resulting in many things updating too often or requiring manual usage
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 |
...conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx
Show resolved
Hide resolved
features/conversation/conversation-message/conversation-message-gestures.tsx
Show resolved
Hide resolved
features/conversation/conversation-message/conversation-message-gestures.tsx
Show resolved
Hide resolved
features/conversation/conversation-message/conversation-message.utils.tsx
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
utils/conversation.ts (1)
Line range hint
52-71: Critical: Empty function implementation could cause runtime issues.The
conversationShouldBeDisplayedfunction appears to be exported but has no implementation. The commented-out code suggests important validation logic for:
- Group conversation readiness
- Pending status
- Message existence
- Deletion status
- Version compatibility
- Forbidden characters
- Pinned status
This could lead to incorrect conversation display behavior.
Consider restoring the implementation:
export const conversationShouldBeDisplayed = ( conversation: ConversationWithCodecsType, topicsData: TopicsData, pinnedConversations?: string[] ) => { + const isNotReady = + (conversation.isGroup && !conversation.groupMembers) || + (!conversation.isGroup && !conversation.peerAddress); + if (isNotReady) return false; + + const isPending = !!conversation.pending; + const isNotEmpty = conversation.messages.size > 0; + const isDeleted = topicsData[conversation.topic]?.status === "deleted"; + const isActive = conversation.isGroup ? conversation.isActive : true; + const isV1 = conversation.version === "v1"; + const isForbidden = conversation.topic.includes("\x00"); + const isPinned = pinnedConversations?.find( + (convo) => convo === conversation.topic + ); + + return ( + (!isPending || isNotEmpty) && + !isDeleted && + !isV1 && + !isForbidden && + !isPinned && + isActive + ); };
🧹 Nitpick comments (16)
features/conversation/conversation-composer/conversation-composer-reply-preview.tsx (2)
Line range hint
214-252: Consider extracting message type handling into a separate component.The
ReplyPreviewMessageContentcomponent contains multiple message type checks and renderings. Consider extracting this into a separateMessageTypeRenderercomponent to improve maintainability and reusability.Example refactor:
+ const MessageTypeRenderer = memo(({ message }: { message: DecodedMessageWithCodecsType }) => { + if (isStaticAttachmentMessage(message)) { + return <Text>Static attachment</Text>; + } + if (isTransactionReferenceMessage(message)) { + return <Text>Transaction</Text>; + } + // ... other type checks + }); const ReplyPreviewMessageContent = memo(function ReplyPreviewMessageContent({ replyMessage }: { replyMessage: DecodedMessageWithCodecsType; }) { const messageText = useMessageText(replyMessage); const clearedMessage = messageText?.replace(/(\n)/gm, " "); - if (isStaticAttachmentMessage(replyMessage)) { - // ... - } - // ... other type checks + return <MessageTypeRenderer message={replyMessage} />; });
Line range hint
232-234: Add a more descriptive TODO comment.The TODO comment for static attachment handling should be more descriptive about what needs to be implemented.
- // TODO + // TODO: Implement proper static attachment preview renderingfeatures/conversation/conversation-new-dm.tsx (1)
155-197: "ConversationNewDmNoMessagesPlaceholder" logic is clear and user-friendly.
The fallback for blocked peers returning null is concise, and the welcome message flow is straightforward. Consider making the greeting emoji configurable or localized if product requirements evolve.features/conversation/hooks/use-react-on-message.ts (2)
39-61: Ensure conversation state is current
The function calls getCurrentAccountConversation(topics) again in onMutate. Confirm that additional calls to retrieve the conversation are valid if the conversation might have changed in between.You could capture the conversation reference earlier and reuse it across the entire function to prevent multiple lookups:
export function useReactOnMessage(props: { topic: ConversationTopic }) { const { topic } = props; const { mutateAsync: reactOnMessageMutationAsync } = useMutation({ mutationFn: async (variables) => { ... }, onMutate: (variables) => { const currentAccount = getCurrentAccount()!; - const conversation = getCurrentAccountConversation(topic); + const conversation = cachedConversation ?? getCurrentAccountConversation(topic); ... }, onError: ... }); ... }
88-88: Dependency array might be incomplete
Only referencing reactOnMessageMutationAsync in the dependency array is fine, but confirm that no other dependencies need to be declared (e.g. Haptics). If Haptics is an imported enum or typed constant, it’s safe. Otherwise, consider adding it to avoid potential stale references.features/conversation/hooks/use-remove-reaction-on-message.ts (2)
40-62: Duplicate conversation fetch
As with the other hook, re-fetching the conversation within onMutate is functional but might lead to inconsistencies if the conversation changes between calls. Consider caching it if performance is a concern or calls are frequent.
88-88: Dependency array completeness
As with useReactOnMessage, confirm that only removeReactionMutationAsync is needed. If more dependencies become relevant, add them to ensure a stable reference.queries/useConversationMessages.ts (2)
Line range hint
251-254: Address WIP comment for IOptimisticMessage type.The "WIP" comment suggests incomplete implementation. This should be addressed to ensure the optimistic message handling is fully implemented.
Would you like help completing the implementation of the optimistic message handling? I can help design a more comprehensive type definition and related functionality.
Line range hint
183-250: Consider optimizing reaction processing for large message sets.The reaction processing logic is well-structured but could benefit from performance optimizations when dealing with large sets of messages:
- Consider using Map instead of plain objects for O(1) lookups
- Consider batching reaction updates to reduce re-renders
Here's a suggested optimization:
type IMessageAccumulator = { ids: MessageId[]; byId: Record<MessageId, DecodedMessageWithCodecsType>; reactions: Record< MessageId, { - bySender: Record<InboxId, ReactionContent[]>; - byReactionContent: Record<string, InboxId[]>; + bySender: Map<InboxId, ReactionContent[]>; + byReactionContent: Map<string, InboxId[]>; } >; };features/conversation/conversation.tsx (1)
313-379: Double-tap reaction logic
- The double-tap gesture toggles a “❤️” reaction if not already present. This is user-friendly.
- Consider edge cases: If the message store is empty or momentarily undefined, ensure graceful handling so that accessing messageId doesn’t raise errors.
- The inline arrow callbacks are concise, but be mindful of potential re-renders if dependencies are large.
hooks/useGroupName.ts (2)
6-6: Preferred names integration
Bringing in usePreferredNames integrates user-friendly references for group participants. This feature can significantly enhance the UI for group participants.
29-29: Performance consideration
Using usePreferredNames for each member is helpful. If the group is large, consider performance optimizations or caching.features/conversation/conversation-message/conversation-message-gestures.tsx (1)
62-69: Consider adding haptic feedback for double tap.For consistency in user experience, consider adding haptic feedback to the double tap gesture similar to the long press.
const doubleTap = Gesture.Tap() .numberOfTaps(2) .onEnd(() => { if (onDoubleTap) { + Haptics.softImpactAsyncAnimated(); onDoubleTap(); } })features/conversation/conversation-message/conversation-message.utils.tsx (1)
227-241: Consider adding null checks for better error handlingThe new
getCurrentUserAlreadyReactedOnMessagefunction could benefit from additional null checks to prevent potential runtime errors.Consider updating the implementation:
export function getCurrentUserAlreadyReactedOnMessage(args: { messageId: MessageId; topic: ConversationTopic; emoji: string | undefined; }) { const { messageId, topic, emoji } = args; const currentUserInboxId = getCurrentUserAccountInboxId(); + if (!currentUserInboxId) return false; const currentAccount = getCurrentAccount()!; const messages = getConversationMessagesQueryData(currentAccount, topic); + if (!messages?.reactions) return false; const reactions = messages?.reactions[messageId]; const bySender = reactions?.bySender; return bySender?.[currentUserInboxId]?.some( (reaction) => !emoji || reaction.content === emoji ); }utils/conversation.ts (2)
Line range hint
73-98: Refactor suggestion: Reduce code duplication and address TODO.
- There's a TODO comment about checking inboxId that needs to be addressed
- The group/non-group conversation handling is duplicated between functions
Consider refactoring to reduce duplication:
+ const getConversationState = (conversation: ConversationWithCodecsType) => { + const isGroup = conversation.version === ConversationVersion.GROUP; + return { isGroup, state: conversation.state }; + }; export const conversationShouldBeInInbox = ( conversation: ConversationWithCodecsType ) => { - if (conversation.version === ConversationVersion.GROUP) { - const isGroupBlocked = conversation.state === "denied"; - const isGroupAllowed = conversation.state === "allowed"; - if (isGroupBlocked) { - return false; - } - return isGroupAllowed; - } else { - const isPeerConsented = conversation.state === "allowed"; - return isPeerConsented; - } + const { state } = getConversationState(conversation); + return state === "allowed"; }; export const isConversationBlocked = ( conversation: ConversationWithCodecsType ) => { - if (conversation.version === ConversationVersion.GROUP) { - // TODO: Check if inboxId is blocked as well - return conversation.state === "denied"; - } else { - return conversation.state === "denied"; - } + const { state } = getConversationState(conversation); + return state === "denied"; };Would you like me to help implement the inboxId blocking check mentioned in the TODO comment?
Line range hint
1-98: Consider documenting the conversation state management approach.Given the significant refactoring and simplification of conversation handling:
- The removal of ConversationContext suggests a shift in state management
- The conversation display logic has been significantly altered
- The state handling is now more streamlined
Consider adding documentation to explain:
- The new approach to conversation state management
- The relationship between conversation states ("allowed", "denied")
- The impact of these changes on the UI layer
This will help maintain consistency as the codebase evolves and make it easier for other developers to understand the design decisions.
📜 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 (27)
data/store/chatStore.ts(0 hunks)features/conversation/conversation-composer/conversation-composer-reply-preview.tsx(2 hunks)features/conversation/conversation-header/conversation-dm-header-title.tsx(2 hunks)features/conversation/conversation-header/conversation-group-header-title.tsx(3 hunks)features/conversation/conversation-header/conversation-header-title.tsx(1 hunks)features/conversation/conversation-header/conversation-new-dm-header-title.tsx(2 hunks)features/conversation/conversation-keyboard-filler.tsx(3 hunks)features/conversation/conversation-message-gestures.tsx(0 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx(4 hunks)features/conversation/conversation-message/conversation-message-gestures.tsx(1 hunks)features/conversation/conversation-message/conversation-message-reactions/conversation-message-reactions.types.ts(0 hunks)features/conversation/conversation-message/conversation-message-status/conversation-message-status.tsx(1 hunks)features/conversation/conversation-message/conversation-message.utils.tsx(3 hunks)features/conversation/conversation-messages-list.tsx(0 hunks)features/conversation/conversation-new-dm-no-messages-placeholder.tsx(0 hunks)features/conversation/conversation-new-dm.tsx(2 hunks)features/conversation/conversation.tsx(4 hunks)features/conversation/conversation.utils.ts(1 hunks)features/conversation/hooks/use-group-name-convos.ts(0 hunks)features/conversation/hooks/use-react-on-message.ts(3 hunks)features/conversation/hooks/use-remove-reaction-on-message.ts(3 hunks)features/conversation/hooks/use-send-message.ts(2 hunks)features/notifications/utils/background/notificationSpamScore.ts(1 hunks)hooks/useGroupName.ts(1 hunks)queries/useConversationMessages.ts(1 hunks)utils/conversation.ts(1 hunks)utils/events.ts(0 hunks)
💤 Files with no reviewable changes (7)
- features/conversation/conversation-message-gestures.tsx
- features/conversation/hooks/use-group-name-convos.ts
- features/conversation/conversation-message/conversation-message-reactions/conversation-message-reactions.types.ts
- features/conversation/conversation-new-dm-no-messages-placeholder.tsx
- features/conversation/conversation-messages-list.tsx
- data/store/chatStore.ts
- utils/events.ts
🔇 Additional comments (52)
features/conversation/conversation-composer/conversation-composer-reply-preview.tsx (2)
15-15: LGTM! Clean import organization.
The import of DecodedMessageWithCodecsType is properly placed and removes duplication.
60-60: LGTM! Type inference improvement.
Good cleanup by removing the redundant type assertion. The type is correctly inferred from the usePreferredInboxName hook.
features/conversation/conversation-new-dm.tsx (6)
3-6: Imports from the new design system modules look good.
These imports are straightforward and match the usage in the code below.
10-10: Confirmed correct import path for "NewConversationTitle".
Ensures that the correct header title is used for the new DM flow.
16-17: Imports for name resolution and internationalization appear correct.
Using "usePreferredName" and "translate" is appropriate for dynamic naming and localized text.
21-21: Usage of "useAppTheme" for dynamic styling is on point.
This allows consistent theme-based design across components.
28-28: "Keyboard" import supports dismissing the keyboard on screen tap.
This helps provide a cleaner user experience when dismissing the keyboard.
149-153: Well-defined prop type for "ConversationNewDmNoMessagesPlaceholder".
Defining a clear interface for props improves type safety and code maintainability.
features/conversation/hooks/use-react-on-message.ts (3)
28-31: Robust error handling
Good check for nonexistent conversation before sending a reaction. This helps avoid null pointer or undefined object errors.
66-66: Error and refetch logic
Refetching messages upon error is a good fallback strategy. Ensure that any partially updated state does not cause duplicates or conflict.
22-23: Function signature changes raise potential impact on downstream calls
By switching from passing in an entire conversation to just a topic, any dependent code that expects a conversation might break. Make sure all callers have updated references so they invoke this hook with the new signature.
Please verify with the following command to ensure all calls adhere to the new signature:
✅ Verification successful
Based on the rg results, I can see all usages of useReactOnMessage in the codebase. The hook is imported and used in two files:
features/conversation/conversation.tsxfeatures/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx
In both cases, the hook is called with the correct new signature, passing an object with just the topic prop:
const reactOnMessage = useReactOnMessage({
topic,
});Let me generate the final response:
All usages of useReactOnMessage are already aligned with the new signature
The hook is used in two locations and both correctly pass just the topic prop, matching the new function signature. No changes are needed in the dependent code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify all calls to `useReactOnMessage` have been updated to accept `{ topic }` props
rg -A 2 "useReactOnMessage"
Length of output: 1926
features/conversation/hooks/use-remove-reaction-on-message.ts (3)
22-24: Similar shift to topic-based signature
Make sure all references to this hook have updated to pass { topic } to avoid runtime failures.
29-32: Conversation existence check
Excellent safeguard to prevent errors when the conversation is missing. This keeps the operation from silently failing.
67-67: Refetch logic
Refetching conversation messages on error is consistent with the approach in useReactOnMessage. Verify that partial state or earlier optimistic updates aren’t causing message duplication.
queries/useConversationMessages.ts (1)
Line range hint 76-83: LGTM! Verify rename impact across codebase.
The renamed function better reflects its purpose of retrieving cached query data. The implementation is correct.
Let's verify that all references to the old function name have been updated:
✅ Verification successful
Function rename is properly implemented and used consistently
The verification shows that:
- The function is correctly imported and used in
conversation-message.utils.tsx - Both usages in the utils file follow the correct pattern with proper parameters
- No other files are using the old function name
- The function is properly exported and imported where needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old function name
rg "getConversationMessages\(" --type ts
# Search for usages of the new function name to ensure proper adoption
rg "getConversationMessagesQueryData\(" --type ts
Length of output: 394
Script:
#!/bin/bash
# Let's check the git history to see if there are any recent changes related to this rename
git log -p --grep="getConversationMessages" --since="1 month ago"
# Let's also check the implementation in conversation-message.utils.tsx to ensure correct usage
ast-grep --pattern 'const messages = getConversationMessagesQueryData($account, $topic)'
Length of output: 90
Script:
#!/bin/bash
# Let's check the actual content of the utils file to understand the usage context
rg "getConversationMessagesQueryData" features/conversation/conversation-message/conversation-message.utils.tsx -B 2 -A 2
# Let's also verify there are no other files importing or using this function
rg "import.*getConversationMessagesQueryData" --type ts
Length of output: 828
features/notifications/utils/background/notificationSpamScore.ts (3)
91-91: LGTM: Type safety improvement with SDK update
The removal of type assertion for senderInboxId aligns with the SDK update, allowing for proper type inference. This change reduces the risk of runtime type errors.
Line range hint 1-144: Verify error handling for security-critical paths
Since this code handles spam detection, it's crucial to ensure that errors don't lead to spam messages being incorrectly scored as safe. Currently, errors result in either 0 or undefined scores, which might allow spam to bypass detection.
Consider:
- Logging errors for monitoring
- Using a more conservative default score on error
- Adding proper error types and handling specific error cases
Line range hint 91-144: Verify the impact of commented out syncConsent call
The syncConsent() call is commented out in computeSpamScoreGroupMessage but remains active in computeSpamScoreGroupWelcome. This inconsistency might affect consent state accuracy.
Consider refactoring duplicated consent checking logic
Both functions share similar patterns for checking consent states (inboxId, address, group). Consider extracting these checks into reusable utility functions to improve maintainability.
Example refactor:
async function checkConsentStates(
xmtpClient: ConverseXmtpClientType,
inboxId: InboxId,
groupId: string
): Promise<number> {
const [groupConsent, inboxConsent] = await Promise.all([
xmtpClient.preferences.conversationConsentState(groupId),
xmtpClient.preferences.inboxIdConsentState(inboxId)
]);
if (groupConsent === "allowed" || inboxConsent === "allowed") return -1;
if (inboxConsent === "denied") return 1;
return 0;
}Standardize error handling approach
The error handling differs between the two functions:
computeSpamScoreGroupWelcomereturns 0 on errorcomputeSpamScoreGroupMessagesilently ignores errors
Consider standardizing the error handling approach across both functions for consistent behavior.
features/conversation/conversation-message/conversation-message-status/conversation-message-status.tsx (1)
10-10: Verify the consistency of the new import path.
The import path for IConversationMessageStatus has been moved from conversation-message/conversation-message.types to conversation-message.types. Let's ensure this change is consistent across the codebase.
✅ Verification successful
Import path change is correctly implemented
The import path change is consistent with the codebase structure. The type IConversationMessageStatus is defined in conversation-message.types.ts and correctly imported in conversation-message-status.tsx using a relative path. No other files in the codebase are importing this type, so there are no inconsistencies to address.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining imports from the old path
rg "from.*conversation-message/conversation-message\.types.*IConversationMessageStatus"
# Search for imports from the new path to confirm adoption
rg "from.*conversation-message\.types.*IConversationMessageStatus"
Length of output: 158
Script:
#!/bin/bash
# Let's first find all files that might contain IConversationMessageStatus
rg "IConversationMessageStatus" --type-list
rg "IConversationMessageStatus" -l
# Then let's check the actual import statements more broadly
rg "from.*conversation-message.*types.*IConversationMessageStatus" -A 1
Length of output: 718
Script:
#!/bin/bash
# Let's examine the content of both files to understand the type definition and its usage
echo "=== Content of conversation-message.types.ts ==="
cat features/conversation/conversation-message/conversation-message.types.ts
echo -e "\n=== Content of conversation-message-status.tsx ==="
cat features/conversation/conversation-message/conversation-message-status/conversation-message-status.tsx
Length of output: 3077
features/conversation/hooks/use-send-message.ts (3)
84-84: LGTM: Consistent rename to senderInboxId.
The rename from senderAddress to senderInboxId aligns with the PR objectives for the new SDK integration.
Line range hint 31-49: LGTM: Robust message content handling.
The function properly handles both text and remote attachment content with appropriate type checks and null safety.
Line range hint 124-137: LGTM: Comprehensive error handling.
The error handling implementation properly captures errors, shows user-friendly toasts, and ensures data consistency through refetching.
features/conversation/conversation.tsx (9)
15-16: Consistent naming convention regarding header components
Imports for DmConversationTitle and GroupConversationTitle appear consistent with the new naming convention for header titles. Ensure all usage in the codebase follows the same pattern (e.g., ConversationHeaderTitle) to maintain clarity and consistency.
22-22: No concerns
Importing 'useMessageContextMenuStore' is straightforward; the usage context will determine whether any additional error handling is necessary.
25-28: Ensure that the replaced gestures component is fully deprecated
The re-import of ConversationMessageGestures likely replaces older references. Confirm that legacy references to these gestures are removed or updated to avoid confusion in the codebase.
33-33: Clear naming
Importing ConversationMessageStatus helps clarify UX for message acknowledgment or read receipts. This naming aligns well with the file's refactored logic.
35-38: Logical grouping for context
The updated imports for MessageContextStoreProvider and related hooks keep context usage modular. Make sure this store's lifecycle is well-scoped to avoid unexpected global states or memory leaks.
41-41: Reusable logic for reaction
Importing getCurrentUserAlreadyReactedOnMessage centralizes reaction-checking logic. This fosters maintainability and reusability.
45-46: Solid approach for adding & removing reactions
These new hooks for reacting and removing reactions on messages are intuitive. Ensure that any concurrency around their usage (e.g., quickly toggling the reaction multiple times) is well-handled in the underlying implementation.
72-72: Sensible highlight logic
ConversationMessageHighlighted is a good addition to visually emphasize messages. Confirm that highlighting doesn't interfere with gesture events in some edge scenario.
295-299: Improved gestures integration
Wrapping message content in ConversationMessageGesturesWrapper is a clean approach to unify multiple gesture handlers. This keeps the core message component simpler.
features/conversation/conversation-header/conversation-header-title.tsx (1)
15-15: Rename to reflect usage
Renaming from ConversationTitle to ConversationHeaderTitle clarifies the component’s scope. Confirm all references in the codebase have updated imports to avoid referencing a stale component name.
hooks/useGroupName.ts (6)
3-3: Query usage
Importing useGroupMembersQuery broadens the hook’s approach to building dynamic group names. This is a good step toward robust group-specific logic.
10-10: Ensure account readiness
Before usage, confirm that currentAccount() never returns an undefined or invalid account. Potentially handle fallback logic if the user is not fully logged in or if the account data is not yet ready.
11-15: Loading and error states
Renaming data → groupName and isLoading → groupNameLoading is more expressive. Confirm that any UI references are updated to use these refined names for clarity.
20-23: Parallel queries for group data
Running group members and group name queries in parallel is efficient but ensure that any new error handling merges them adequately if either fails.
25-27: Filter out self from addresses
Filtering out the current account’s address is logical when building a user-centric name. This keeps group naming distinct and relevant.
37-37: Fallback logic
Using groupName || names.join(", ") ensures at least a minimal fallback for unnamed groups. Be aware that an empty string for groupName also triggers the fallback, which might be desired or might be an unintended override.
features/conversation/conversation-header/conversation-new-dm-header-title.tsx (2)
1-1: Consistent usage
Updated import of ConversationHeaderTitle unifies the new naming convention. This boosts consistency across components.
38-38: Improved approach to DM header
Switching to ConversationHeaderTitle ensures compliance with the new naming standard and seamlessly integrates the avatar. The approach to conditionally rendering the avatar (based on isLoading) is sound.
features/conversation/conversation-header/conversation-dm-header-title.tsx (1)
Line range hint 50-64: LGTM! Clean component renaming.
The change from ConversationTitle to ConversationHeaderTitle maintains the same functionality while improving naming consistency across conversation components.
features/conversation/conversation-keyboard-filler.tsx (2)
28-42: Consider potential race condition in keyboard state management.
The keyboard state management could have a race condition if the context menu state changes rapidly. Consider debouncing the keyboard show/hide operations or adding a small delay before re-focusing.
Line range hint 64-72: LGTM! Improved variable naming.
Renaming animatedStyle to fillerAnimatedStyle better describes the variable's purpose and improves code readability.
features/conversation/conversation-message/conversation-message-gestures.tsx (1)
92-92: LGTM! Well-structured gesture composition.
The use of Gesture.Exclusive ensures proper gesture priority handling between double tap, single tap, and long press.
features/conversation/conversation-header/conversation-group-header-title.tsx (2)
40-40: LGTM: Hook usage simplified
The migration from useGroupNameConvos to useGroupName simplifies the hook usage by removing the unnecessary account parameter, making the code more maintainable.
Line range hint 75-93: LGTM: Component update maintains functionality
The switch to ConversationHeaderTitle maintains all necessary props and functionality while improving component organization.
features/conversation/conversation-message/conversation-message.utils.tsx (1)
111-111: LGTM: Function rename improves clarity
The rename from getConversationMessages to getConversationMessagesQueryData better reflects the function's purpose and return type.
features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx (3)
84-88: LGTM: Simplified hook props
The removal of unnecessary props in useReactOnMessage and useRemoveReactionOnMessage hooks improves code maintainability.
96-100: LGTM: Improved reaction check logic
The use of getCurrentUserAlreadyReactedOnMessage centralizes the reaction check logic, making it more maintainable and consistent across the application.
Line range hint 109-117: Verify dependency array completeness
Ensure that all dependencies used within the callback are included in the dependency array.
utils/conversation.ts (2)
1-3: LGTM! Import cleanup aligns with type simplification.
Line range hint 4-11: Consider expanding ConversationFlatListItem type.
The ConversationFlatListItem type is currently just an alias for ConversationFlatListHiddenRequestItem. This seems overly simplified - should it include other possible item types for the conversation list?
| import { getCurrentAccount } from "@/data/store/accountsStore"; | ||
| import { getConversationQueryData } from "@/queries/useConversationQuery"; | ||
| import { ConversationTopic, MessageId } from "@xmtp/react-native-sdk"; | ||
|
|
||
| export function getCurrentAccountConversation(topic: ConversationTopic) { | ||
| const currentAccount = getCurrentAccount()!; | ||
| return getConversationQueryData({ | ||
| account: currentAccount, | ||
| topic: topic, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure safe usage of non-null assertion
Using getCurrentAccount()! might throw if there is no current account. Although the code suggests user must be logged in, add an early check or error message to handle the edge case.
Here's a potential approach:
export function getCurrentAccountConversation(topic: ConversationTopic) {
- const currentAccount = getCurrentAccount()!;
- return getConversationQueryData({ account: currentAccount, topic });
+ const currentAccount = getCurrentAccount();
+ if (!currentAccount) {
+ throw new Error("No current account set");
+ }
+ return getConversationQueryData({ account: currentAccount, 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.
| import { getCurrentAccount } from "@/data/store/accountsStore"; | |
| import { getConversationQueryData } from "@/queries/useConversationQuery"; | |
| import { ConversationTopic, MessageId } from "@xmtp/react-native-sdk"; | |
| export function getCurrentAccountConversation(topic: ConversationTopic) { | |
| const currentAccount = getCurrentAccount()!; | |
| return getConversationQueryData({ | |
| account: currentAccount, | |
| topic: topic, | |
| }); | |
| } | |
| import { getCurrentAccount } from "@/data/store/accountsStore"; | |
| import { getConversationQueryData } from "@/queries/useConversationQuery"; | |
| import { ConversationTopic, MessageId } from "@xmtp/react-native-sdk"; | |
| export function getCurrentAccountConversation(topic: ConversationTopic) { | |
| const currentAccount = getCurrentAccount(); | |
| if (!currentAccount) { | |
| throw new Error("No current account set"); | |
| } | |
| return getConversationQueryData({ account: currentAccount, topic }); | |
| } |
| @@ -79,10 +79,9 @@ export function useSendMessage(props: { | |||
| : contentTypesPrefixes.remoteAttachment, | |||
| sentNs: getTodayNs(), | |||
| fallback: "new-message", | |||
| // @ts-ignore we're adding our "own" delivery status because we want to display it in the UI | |||
| deliveryStatus: "sending" satisfies IConversationMessageStatus, | |||
| deliveryStatus: "sending" as MessageDeliveryStatus, // NOT GOOD but tmp | |||
There was a problem hiding this comment.
Replace type assertion with type validation.
The change from satisfies to as type assertion reduces type safety. The comment "NOT GOOD but tmp" acknowledges this issue. Consider using satisfies or proper type validation instead.
- deliveryStatus: "sending" as MessageDeliveryStatus, // NOT GOOD but tmp
+ deliveryStatus: "sending" satisfies MessageDeliveryStatus,📝 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.
| deliveryStatus: "sending" as MessageDeliveryStatus, // NOT GOOD but tmp | |
| deliveryStatus: "sending" satisfies MessageDeliveryStatus, |
| @@ -1,58 +0,0 @@ | |||
| import { Center } from "@/design-system/Center"; | |||
There was a problem hiding this comment.
is this not needed any longer?
There was a problem hiding this comment.
I've put it inside conversation-new-dm for now since everything this file needed was also in that one and I felt it was more clear than having this one only external file
|
You will need to import import { wait } from "@/utils/wait"; to fix the TSC issues |
thierryskoda
force-pushed
the
ts/chat-fixes-19-12-2024-2
branch
from
830e586 to
6bb4853
Compare
Summary by CodeRabbit
Release Notes
New Features
ConversationNewDmcomponent with improved UI and state management for new conversations.Improvements
Bug Fixes
Deprecated
Refactor