fix: Streaming New Messages on Created Conversations #1336
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several modifications across various components and utilities. The Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
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 (
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
utils/xmtpRN/conversations.ts
Outdated
| // if (groupName) { | ||
| // setGroupNameQueryData(account, group.topic, groupName); | ||
| // } | ||
| // }s |
There was a problem hiding this comment.
There appears to be a stray 's' character after the comment closing. The line should be // } instead of // }s.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
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: 2
🧹 Outside diff range and nitpick comments (1)
utils/str.ts (1)
68-70: LGTM! Consider adding type annotations for better maintainability.The simplification of
useAccountsProfilesusingusePreferredNamesis a good improvement. Consider adding explicit return type annotation for better type safety and documentation.Here's a suggested improvement:
-export const useAccountsProfiles = () => { +export const useAccountsProfiles = (): string[] => { const accounts = useAccountsList(); const accountNames = usePreferredNames(accounts); return accountNames; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
features/conversation-requests-list/useRequestItems.tsx(1 hunks)screens/Accounts/Accounts.tsx(1 hunks)screens/Navigation/ConversationRequestsListNav.ios.tsx(2 hunks)screens/Navigation/ConversationRequestsListNav.tsx(2 hunks)utils/str.ts(2 hunks)utils/xmtpRN/conversations.ts(6 hunks)utils/xmtpRN/sync.ts(2 hunks)
🔇 Additional comments (8)
features/conversation-requests-list/useRequestItems.tsx (1)
4-11: LGTM! Hook enhancement supports pull-to-refresh functionality.
The addition of isRefetching and refetch properties aligns with the PR objective of implementing pull-to-refresh for the request list.
Let's verify the hook usage in consuming components:
✅ Verification successful
Hook usage verified and properly implemented in consuming components
The verification confirms that both ConversationRequestsListNav.tsx and its iOS variant correctly destructure and utilize the new isRefetching and refetch properties from useRequestItems hook, ensuring proper implementation of the pull-to-refresh functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using useRequestItems to ensure they handle the new properties
rg -l "useRequestItems" | xargs ast-grep --pattern 'const { $$$, isRefetching, refetch } = useRequestItems()'
Length of output: 505
utils/xmtpRN/sync.ts (1)
33-35: Ensure consistent case handling for account identifiers.
The changes improve robustness by enforcing case-insensitive account handling. However, let's verify that this change doesn't affect existing account lookups.
Consider extracting the account normalization logic into a utility function for consistency:
+const normalizeAccount = (account: string): string => account.toLowerCase();
+
export const getXmtpClient = async (
account: string
): Promise<ConverseXmtpClientType | Client> => {
- const lowerCaseAccount = account.toLowerCase();
+ const lowerCaseAccount = normalizeAccount(account);Also applies to: 38-38, 48-48, 55-55, 61-61, 64-64
✅ Verification successful
Case-insensitive account handling is correctly implemented and consistent.
The verification shows that:
- All direct accesses to
xmtpClientByAccountinsync.tsuse thelowerCaseAccountvariable, which is consistently derived fromaccount.toLowerCase(). - The only place where the original
accountis used withxmtpClientByAccountis indeleteXmtpClient, but this is safe as it's a cleanup operation where missing a key wouldn't cause issues. - The case-insensitive handling is properly synchronized with the instantiation tracking in
instantiatingClientForAccount.
The refactoring suggestion to extract the normalization function would be a good enhancement, but the current implementation is correct and safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any mixed-case account references that might be affected
rg -i "xmtpClientByAccount\[(?!\w+\.toLowerCase\(\))" -A 2
Length of output: 330
Script:
#!/bin/bash
# Let's try a simpler search pattern to find any direct usage of xmtpClientByAccount
rg "xmtpClientByAccount\[" -A 2
# Also search for any other references to this object
rg "xmtpClientByAccount" -A 2
# Let's also check the full file content to understand the context better
cat utils/xmtpRN/sync.ts
Length of output: 8277
screens/Navigation/ConversationRequestsListNav.tsx (1)
39-40: LGTM: Pull-to-refresh implementation looks good.
The addition of isRefetching and refetch props enhances the user experience by providing feedback during data refresh operations.
Also applies to: 164-169
screens/Navigation/ConversationRequestsListNav.ios.tsx (1)
36-37: LGTM: iOS implementation maintains consistency.
The changes mirror the non-iOS version, ensuring consistent behavior across platforms.
Also applies to: 175-180
✅ Verification successful
Verified: iOS implementation maintains consistency with non-iOS version
The verification confirms that both implementations:
- Use identical
useRequestItemshook with the same destructured properties:likelySpam,likelyNotSpam,isRefetching,refetch - Pass the same props to
ConversationFlashList:isRefetching,refetch, and items (though nameditemsToShowin iOS anditemsin non-iOS)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistency between iOS and non-iOS implementations
diff <(rg -A 5 "ConversationFlashList" screens/Navigation/ConversationRequestsListNav.tsx) <(rg -A 5 "ConversationFlashList" screens/Navigation/ConversationRequestsListNav.ios.tsx)
Length of output: 288
Script:
#!/bin/bash
# Let's first check if these files exist and their content
fd "ConversationRequestsListNav" --type f
# Then let's examine the content of both files focusing on the relevant sections
echo "=== iOS Version ==="
rg -B 2 -A 5 "useRequestItems|ConversationFlashList" "screens/Navigation/ConversationRequestsListNav.ios.tsx" || true
echo -e "\n=== Non-iOS Version ==="
rg -B 2 -A 5 "useRequestItems|ConversationFlashList" "screens/Navigation/ConversationRequestsListNav.tsx" || true
Length of output: 3099
utils/xmtpRN/conversations.ts (4)
17-17: LGTM: Import addition aligns with streaming functionality
The addition of streamAllMessages import supports the PR's objective of fixing message streaming in new conversations.
314-314: LGTM: Conversation creation now properly handles message streaming
The addition of handleNewConversationCreation ensures that message streaming is restarted after creating a new conversation, addressing the core issue mentioned in PR objectives.
Line range hint 346-360: LGTM: Group creation now properly handles message streaming
The refactored implementation properly stores the group creation result, adds logging, and ensures message streaming is restarted.
544-554: 🛠️ Refactor suggestion
Add error handling and consider parameter usage
While the implementation addresses the streaming issue, consider these improvements:
- Add error handling for
streamAllMessagescall - Either use or remove the unused
_conversationparameter - Add more context about when this workaround can be removed
Consider this implementation:
const handleNewConversationCreation = async (
client: ConverseXmtpClientType,
- _conversation: ConversationWithCodecsType
+ conversation: ConversationWithCodecsType
) => {
logger.info(
- "[XMTPRN Conversations] Restarting message stream to handle new conversation"
+ `[XMTPRN Conversations] Restarting message stream for conversation ${conversation.topic}`
);
- await streamAllMessages(client.address);
+ try {
+ await streamAllMessages(client.address);
+ } catch (error) {
+ logger.error(
+ `[XMTPRN Conversations] Failed to restart message stream: ${error}`
+ );
+ throw error;
+ }
};✅ Verification successful
Add error handling and use retryWithBackoff for message streaming
The review comment is valid but can be improved. Based on the codebase analysis:
- The
streamAllMessagesfunction is already wrapped withretryWithBackoffin the sync process (utils/xmtpRN/sync.ts), providing robust error handling with retries. - The unused
_conversationparameter should be utilized as suggested in the review. - The TODO comment is appropriate as it references an open GitHub issue (Fix restore group swipe action icon invisible in iOS dark mode #560) for tracking the temporary nature of this workaround.
const handleNewConversationCreation = async (
client: ConverseXmtpClientType,
- _conversation: ConversationWithCodecsType
+ conversation: ConversationWithCodecsType
) => {
logger.info(
- "[XMTPRN Conversations] Restarting message stream to handle new conversation"
+ `[XMTPRN Conversations] Restarting message stream for conversation ${conversation.topic}`
);
- await streamAllMessages(client.address);
+ await retryWithBackoff({
+ fn: () => streamAllMessages(client.address),
+ retries: 5,
+ delay: 1000,
+ });
};🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the referenced GitHub issue status
gh issue view 560 --repo xmtp/xmtp-react-native --json state,title,body
Length of output: 570
Script:
#!/bin/bash
# Check for error handling patterns in similar async functions
ast-grep --pattern 'try {
$$$
await streamAllMessages($$$)
$$$
} catch'
# Check for other usages of streamAllMessages to understand error handling patterns
rg "streamAllMessages" -A 2 -B 2
Length of output: 2816
Script:
#!/bin/bash
# Check how retryWithBackoff is implemented for error handling
rg "retryWithBackoff" -A 5 -B 5
# Check the implementation of streamAllMessages in messages.ts
cat utils/xmtpRN/messages.ts
Length of output: 13459
| items={accountsProfiles.map((a) => ({ | ||
| id: a, | ||
| title: accountsProfiles[a] || shortAddress(a), | ||
| title: a, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving account address readability in the UI.
The account address is now displayed directly without formatting, which might impact readability. Consider using the existing shortAddress or getReadableProfile utility to format the address.
Here's a suggested improvement:
items={accountsProfiles.map((a) => ({
id: a,
- title: a,
+ title: getReadableProfile(a, a), // or shortAddress(a) for a simpler approach
action: () => {Committable suggestion skipped: line range outside the PR's diff.
utils/xmtpRN/conversations.ts
Outdated
|
|
||
| // if (groupName) { | ||
| // setGroupNameQueryData(account, group.topic, groupName); | ||
| // } | ||
| // }s |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered evidence, I can now provide a final response:
Remove commented metadata setting code as it's handled elsewhere
The commented code block can be safely removed because:
- The metadata setting functionality is actively used in other parts of the codebase (utils/groupUtils/handle*Update.ts files)
- The group metadata is already being set during group creation via the
createGroupfunction parameters - The code has been replaced with
handleNewConversationCreationwhich handles the post-creation workflow
🔗 Analysis chain
Clean up or restore commented metadata setting code
There's commented out code for setting group metadata. This should either be:
- Removed if it's no longer needed
- Restored if the functionality is still required
- Documented with a TODO if it's temporarily disabled
Please clarify the intended handling of this code.
Let me gather more context about this specific code section to understand if this commented code is still needed or can be safely removed.
Let me check one more thing to see if this metadata setting functionality is handled elsewhere in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these metadata functions are used elsewhere in the codebase
rg "setGroup(Name|Photo|Description)QueryData"
Length of output: 2941
Script:
#!/bin/bash
# Check the git history of this specific section to understand when and why it was commented out
git log -L389,392:utils/xmtpRN/conversations.ts
# Look for any related group metadata setting code in the same file
rg -A 5 "groupName|groupPhoto|groupDescription" utils/xmtpRN/conversations.ts
Length of output: 1376
Script:
#!/bin/bash
# Check for handleNewConversation usage and implementation
rg -A 10 "handleNewConversation"
# Check if there's any other place where group metadata is set after creation
ast-grep --pattern 'createGroup($$$).then($$$setGroup$$$)'
ast-grep --pattern 'createGroupByAccount($$$).then($$$setGroup$$$)'
Length of output: 6419
Restart message stream when creating a new conversation Updated Accounts list to use query Added pull to refresh for request list
alexrisch
force-pushed
the
ar/stream-messages-new-dm
branch
from
927766c to
2e8e535
Compare
Restart message stream when creating a new conversation Updated Accounts list to use query
Added pull to refresh for request list
Workaround for xmtp/xmtp-react-native#560
Fixes #1335
Fixes #1334
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores