feat: Remove XMTP DB drop and clean XMTP stuff #1550
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThis pull request involves a comprehensive restructuring of the XMTP client-related modules and types. The primary changes include removing the Changes
Sequence DiagramsequenceDiagram
participant App
participant ClientManager
participant XMTPClient
App->>ClientManager: Request XMTP Client
ClientManager->>XMTPClient: Create Client
XMTPClient-->>ClientManager: Client Instance
ClientManager-->>App: Provide Client
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
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: 0
🧹 Nitpick comments (4)
utils/xmtpRN/client/client-installations.ts (1)
Line range hint
26-37: Add error handling forsignWithInstallationKeyConsider adding error handling around
client.signWithInstallationKey(message)to manage potential exceptions during the signing process and prevent unhandled promise rejections.Apply this diff to include error handling:
export async function getInstallationKeySignature( account: string, message: string ): Promise<InstallationSignature> { const client = (await getXmtpClient(account)) as ConverseXmtpClientType; if (!client) throw new Error("Client not found"); + let raw; + try { raw = await client.signWithInstallationKey(message); + } catch (error) { + throw new Error(`Failed to sign with installation key: ${error.message}`); + } return { installationPublicKey: client.installationId, installationKeySignature: Buffer.from(raw).toString("hex"), }; }utils/xmtpRN/client/client.ts (2)
33-43: Add error handling forClient.buildWrapping the
Client.buildcall in a try-catch block will handle any potential errors during client instantiation, enhancing robustness.Apply this diff to implement error handling:
export const getXmtpClientFromAddress = async (address: string) => { const dbDirectory = await getDbDirectory(); const dbEncryptionKey = await getDbEncryptionKey(); + try { return Client.build(address, { env, codecs, dbDirectory, dbEncryptionKey, }); + } catch (error) { + throw new Error(`Failed to build XMTP client: ${error.message}`); + } };
49-53: Handle errors inreconnectXmtpClientsDbConnectionsErrors from
c.reconnectLocalDatabase()might go unhandled. Adding error handling ensures any issues are logged and managed appropriately.Apply this diff to handle potential errors:
export const reconnectXmtpClientsDbConnections = async () => { await Promise.all( Object.values(xmtpClientByAccount).map(async (c) => { + try { await c.reconnectLocalDatabase(); + } catch (error) { + logger.error(`Failed to reconnect client ${c.address}: ${error.message}`); + } }) ); };utils/xmtpRN/sync.ts (1)
Line range hint
15-34: Prevent potential recursion ingetXmtpClientThe recursive call to
getXmtpClient(account)could lead to stack overflows in extreme cases. Refactor to use iteration or await the existing promises without recursion.Apply this diff to modify the control flow:
export const getXmtpClient = async ( account: string ): Promise<ConverseXmtpClientType> => { const lowerCaseAccount = account.toLowerCase(); if (account && xmtpClientByAccount[lowerCaseAccount]) { return xmtpClientByAccount[lowerCaseAccount]; } const alreadyInstantiating = instantiatingClientForAccount[lowerCaseAccount]; if (alreadyInstantiating) { return alreadyInstantiating; } if (Object.keys(instantiatingClientForAccount).length > 0) { - await new Promise((r) => setTimeout(r, 200)); - return getXmtpClient(account); + await Promise.all(Object.values(instantiatingClientForAccount)); + return xmtpClientByAccount[lowerCaseAccount]; } instantiatingClientForAccount[lowerCaseAccount] = (async () => { try { logger.debug("[XmtpRN] Getting client from address"); const client = await getXmtpClientFromAddress(account); logger.info(`[XmtpRN] Instantiated client for ${client.address}`); xmtpClientByAccount[lowerCaseAccount] = client; return client; } catch (e: unknown) { throw e; } finally { delete instantiatingClientForAccount[lowerCaseAccount]; } })(); return instantiatingClientForAccount[ lowerCaseAccount ] as Promise<ConverseXmtpClientType>; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (73)
App.tsx(0 hunks)components/XmtpEngine.tsx(0 hunks)containers/GroupScreenConsentTable.tsx(1 hunks)containers/GroupScreenMembersTable.tsx(2 hunks)data/store/chatStore.ts(0 hunks)features/GroupInvites/joinGroup/JoinGroup.client.ts(1 hunks)features/GroupInvites/joinGroup/joinGroup.machine.ts(1 hunks)features/consent/account-can-message-peer.ts(1 hunks)features/consent/update-consent-for-addresses-for-account.ts(1 hunks)features/consent/update-consent-for-groups-for-account.ts(1 hunks)features/consent/update-inbox-ids-consent-for-account.ts(1 hunks)features/consent/use-allow-group.mutation.ts(1 hunks)features/consent/use-dm-consent.tsx(1 hunks)features/conversation-list/conversation-list-item/conversation-list-item-dm.tsx(1 hunks)features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation-dm.tsx(1 hunks)features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation-group.tsx(1 hunks)features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation-message-preview.tsx(1 hunks)features/conversation-list/conversation-list.screen.tsx(1 hunks)features/conversation-list/conversation-list.tsx(1 hunks)features/conversation-list/hooks/use-delete-dm.ts(1 hunks)features/conversation-list/hooks/useMessagePlainText.ts(1 hunks)features/conversation-requests-list/use-conversation-requests-list-items.tsx(1 hunks)features/conversation/conversation-composer/conversation-composer-reply-preview.tsx(1 hunks)features/conversation/conversation-message/conversation-message-content-types/conversation-message-reply.tsx(1 hunks)features/conversation/conversation-message/conversation-message-status/conversation-message-status.utils.ts(1 hunks)features/conversation/conversation-message/conversation-message.store-context.tsx(1 hunks)features/conversation/conversation-message/conversation-message.tsx(1 hunks)features/conversation/conversation-message/conversation-message.utils.tsx(1 hunks)features/conversation/conversation-messages-list.tsx(1 hunks)features/conversation/conversation.tsx(1 hunks)features/conversation/conversation.utils.ts(1 hunks)features/conversation/hooks/use-send-message.ts(1 hunks)features/conversation/utils/__tests__/search.test.ts(1 hunks)features/conversation/utils/has-next-message-in-serie.ts(1 hunks)features/conversation/utils/has-previous-message-in-serie.ts(1 hunks)features/conversation/utils/is-conversation-allowed.ts(1 hunks)features/conversation/utils/is-conversation-consent-unknown.ts(1 hunks)features/conversation/utils/is-conversation-denied.ts(1 hunks)features/conversation/utils/is-conversation-dm.ts(1 hunks)features/conversation/utils/is-conversation-group.ts(1 hunks)features/conversation/utils/is-conversation-message-from-eth-address.ts(1 hunks)features/conversation/utils/message-is-from-current-user.ts(1 hunks)features/conversation/utils/message-should-show-date-change.ts(1 hunks)features/conversation/utils/search.ts(1 hunks)features/notifications/utils/background/converseNotification.ts(1 hunks)features/notifications/utils/background/groupJoinRequestNotification.ts(1 hunks)features/notifications/utils/background/groupMessageNotification.ts(1 hunks)features/notifications/utils/background/groupWelcomeNotification.ts(1 hunks)features/notifications/utils/background/notificationContent.ts(1 hunks)features/notifications/utils/background/notificationSpamScore.ts(1 hunks)features/notifications/utils/background/protocolNotification.ts(1 hunks)features/notifications/utils/subscribeToNotifications.ts(1 hunks)queries/conversations-query.ts(1 hunks)queries/use-conversation-messages-query.ts(1 hunks)queries/useConversationMessage.ts(1 hunks)queries/useGroupMembersQuery.ts(1 hunks)queries/useGroupPermissionsQuery.ts(1 hunks)queries/useGroupQuery.ts(1 hunks)screens/Main.tsx(1 hunks)utils/api/auth.ts(1 hunks)utils/events.ts(1 hunks)utils/evm/xmtp.ts(1 hunks)utils/frames.ts(2 hunks)utils/logout/index.tsx(1 hunks)utils/xmtpRN/attachments.ts(1 hunks)utils/xmtpRN/client/client-installations.ts(1 hunks)utils/xmtpRN/client/client.ts(1 hunks)utils/xmtpRN/client/client.types.ts(1 hunks)utils/xmtpRN/conversations.ts(1 hunks)utils/xmtpRN/signIn.ts(1 hunks)utils/xmtpRN/sync.ts(1 hunks)utils/xmtpRN/xmtp-messages/xmtp-messages-stream.ts(1 hunks)utils/xmtpRN/xmtp-messages/xmtp-messages.ts(1 hunks)
💤 Files with no reviewable changes (3)
- components/XmtpEngine.tsx
- data/store/chatStore.ts
- App.tsx
✅ Files skipped from review due to trivial changes (66)
- utils/xmtpRN/signIn.ts
- utils/api/auth.ts
- features/conversation/conversation-message/conversation-message-content-types/conversation-message-reply.tsx
- features/conversation/conversation-message/conversation-message.utils.tsx
- features/conversation/conversation-messages-list.tsx
- features/conversation-list/conversation-list.tsx
- features/conversation/utils/has-previous-message-in-serie.ts
- queries/useGroupPermissionsQuery.ts
- utils/xmtpRN/xmtp-messages/xmtp-messages-stream.ts
- features/conversation/utils/is-conversation-dm.ts
- utils/events.ts
- queries/conversations-query.ts
- features/conversation/conversation.tsx
- utils/xmtpRN/conversations.ts
- features/conversation/hooks/use-send-message.ts
- features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation-message-preview.tsx
- features/conversation/utils/message-should-show-date-change.ts
- queries/useConversationMessage.ts
- features/consent/account-can-message-peer.ts
- features/GroupInvites/joinGroup/joinGroup.machine.ts
- features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation-group.tsx
- features/conversation-list/conversation-list.screen.tsx
- features/notifications/utils/subscribeToNotifications.ts
- features/conversation/utils/tests/search.test.ts
- features/consent/update-consent-for-groups-for-account.ts
- queries/useGroupMembersQuery.ts
- features/conversation/conversation-message/conversation-message.store-context.tsx
- features/conversation-list/hooks/useMessagePlainText.ts
- features/conversation/utils/is-conversation-allowed.ts
- features/conversation-list/hooks/use-delete-dm.ts
- features/conversation/conversation-message/conversation-message.tsx
- utils/xmtpRN/client/client.types.ts
- utils/evm/xmtp.ts
- features/conversation/utils/search.ts
- features/consent/update-inbox-ids-consent-for-account.ts
- features/conversation/utils/is-conversation-denied.ts
- features/conversation/utils/is-conversation-message-from-eth-address.ts
- containers/GroupScreenConsentTable.tsx
- utils/logout/index.tsx
- features/consent/update-consent-for-addresses-for-account.ts
- features/consent/use-dm-consent.tsx
- features/conversation/utils/has-next-message-in-serie.ts
- features/conversation-requests-list/use-conversation-requests-list-items.tsx
- features/conversation/utils/is-conversation-consent-unknown.ts
- queries/useGroupQuery.ts
- features/conversation-list/conversation-list-pinned-conversations/conversation-list-pinned-conversation-dm.tsx
- features/conversation/conversation-composer/conversation-composer-reply-preview.tsx
- features/notifications/utils/background/converseNotification.ts
- features/conversation/conversation-message/conversation-message-status/conversation-message-status.utils.ts
- features/notifications/utils/background/notificationContent.ts
- features/conversation/utils/message-is-from-current-user.ts
- features/notifications/utils/background/groupWelcomeNotification.ts
- containers/GroupScreenMembersTable.tsx
- features/conversation/utils/is-conversation-group.ts
- queries/use-conversation-messages-query.ts
- features/GroupInvites/joinGroup/JoinGroup.client.ts
- features/notifications/utils/background/notificationSpamScore.ts
- features/conversation/conversation.utils.ts
- features/notifications/utils/background/protocolNotification.ts
- features/conversation-list/conversation-list-item/conversation-list-item-dm.tsx
- features/consent/use-allow-group.mutation.ts
- features/notifications/utils/background/groupJoinRequestNotification.ts
- features/notifications/utils/background/groupMessageNotification.ts
- utils/frames.ts
- utils/xmtpRN/attachments.ts
- utils/xmtpRN/xmtp-messages/xmtp-messages.ts
🔇 Additional comments (6)
utils/xmtpRN/client/client-installations.ts (3)
6-6: ImportingClientfrom "@xmtp/react-native-sdk"The import of
Clientis necessary for type annotations and client operations within this file.
8-8: Correct import ofgetXmtpClientImporting
getXmtpClientfrom"../sync"ensures the client retrieval function is accurately referenced.
10-10: Updated import path forConverseXmtpClientTypeAdjusting the import path to
@/utils/xmtpRN/client/client.typesreflects the new project structure and improves code organization.utils/xmtpRN/sync.ts (2)
11-11: Updated import paths for client modulesThe imports now correctly reflect the reorganized module structure, improving maintainability.
Line range hint
26-39: Verify the impact of removing reconnecting state updatesThe removal of
chat.setReconnecting(true)may affect how the app indicates connectivity issues to the user. Ensure that the user interface still accurately reflects the connection status.Run the following script to check for any remaining references to
chat.setReconnecting:✅ Verification successful
Connection status UI remains properly maintained
The removal of
chat.setReconnecting(true)does not impact the app's ability to indicate connectivity issues. The connection status is still properly handled through:
useShouldShowConnectinghook that shows appropriate status messages- Combined monitoring of network reachability and reconnection state
- Proper user feedback through "Waiting for network" and "Reconnecting" messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of 'setReconnecting' method in the codebase rg 'setReconnecting\('Length of output: 24
Script:
#!/bin/bash # Search for any references to 'reconnecting' state or property rg -i 'reconnecting' -A 2 -B 2 # Search for connection status related patterns rg -i '(connection.*status|isConnected|connected)' -A 2 -B 2 # Look for UI components that might show connection status ast-grep --pattern 'const $_ = $$$; return (<$$$>connection$$$</$$>);'Length of output: 11397
screens/Main.tsx (1)
4-4: LGTM! The import path change aligns with the XMTP cleanup objectives.The new path structure is more organized and specific, improving code maintainability.
Let's verify the consistency of XMTP-related changes across the codebase:
✅ Verification successful
Import path change is valid and consistent with codebase structure
The new import path
@/utils/xmtpRN/client/client-installationsis part of a well-organized XMTP module structure, and no files are using the old import path. The change aligns with the codebase organization and the PR's XMTP cleanup objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining old XMTP import paths and verify the new structure # Check for any remaining imports from the old path echo "Checking for old import paths..." rg -l "@utils/xmtpRN/client['\"]" # Verify the new client-installations structure echo "Verifying new structure..." rg -l "@/utils/xmtpRN/client/client-installations"Length of output: 243
Script:
#!/bin/bash # Description: Check for any other XMTP-related imports and files # Search for any XMTP-related imports echo "Checking for any XMTP-related imports..." rg -l "xmtpRN" # List XMTP-related files in utils directory echo -e "\nListing XMTP-related files..." fd -p "utils/.*xmtp.*" -t fLength of output: 5792
technoplato
approved these changes
Jan 19, 2025
| @@ -20,7 +20,7 @@ import { | |||
| } from "@/features/conversation/conversation.store-context"; | |||
| import { usePreferredInboxName } from "@/hooks/usePreferredInboxName"; | |||
| import { captureError } from "@/utils/capture-error"; | |||
| import { DecodedMessageWithCodecsType } from "@/utils/xmtpRN/client.types"; | |||
| import { DecodedMessageWithCodecsType } from "@/utils/xmtpRN/client/client.types"; | |||
There was a problem hiding this comment.
I'm starting to think it might be a good idea if we have a very thin layer that wraps all of our messaging stuff before messaging stuff touches the application
The reason being we don't need all these complicated types littering our code base and we could just have a message that has content available on it with a content type that is a Concrete property that we can determine at that layer and so instead of these complicated decoded message with Kodex types, etc., we could simply have message conversation, etc.
There was a problem hiding this comment.
I think we should delete frames stuff for now. We're definitely going to be redoing it.
| [account: string]: ConverseXmtpClientType; | ||
| } = {}; | ||
|
|
||
| export const reconnectXmtpClientsDbConnections = async () => { |
There was a problem hiding this comment.
Sorry, I thought the PR said we were getting rid of this
| @@ -88,8 +86,6 @@ export const onSyncLost = async (account: string, error: any) => { | |||
| } | |||
| }; | |||
|
|
|||
| const streamingAccounts: { [account: string]: boolean } = {}; | |||
There was a problem hiding this comment.
Curious, if there was anywhere else, this was used other than where you deleted it below
|
I really think we should keep this PR small to easily verify the change being done |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
Code Organization
clientsubdirectoryFeature Changes
XmtpEngineclass and related synchronization logicreconnectingstate management in chat storeDependency Management
Performance
The changes primarily focus on internal code organization and cleanup, with minimal user-facing impact.