-
Notifications
You must be signed in to change notification settings - Fork 30
deleteMessage, prepareMessage noSend, streamMessageDeletions (libxmtp 1.9.0 updates) #765
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
| dbEncryptionKey = encryptionKeyBytes, | ||
| dbDirectory = authOptions.dbDirectory, | ||
| historySyncUrl = historySyncUrl, | ||
| deviceSyncEnabled = authOptions.deviceSyncEnabled, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debugEventsEnabled is parsed from authParams but no longer passed to ClientOptions, so the user's setting is silently ignored. Consider re-adding it.
🚀 Want me to fix this? Reply ex: "fix it for me".
| ): Promise<() => void> { | ||
| await XMTPModule.subscribeToMessageDeletions(this.client.installationId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider removing existing subscriptions before assigning new ones. If streamMessageDeletions is called multiple times without cleanup, previous subscriptions remain active but become unreachable, causing memory leaks and duplicate callbacks.
- ): Promise<() => void> {
+ ): Promise<() => void> {
+ this.subscriptions[EventTypes.MessageDeletion]?.remove()
+ this.subscriptions[EventTypes.MessageDeletionClosed]?.remove()
await XMTPModule.subscribeToMessageDeletions(this.client.installationId)🚀 Want me to fix this? Reply ex: "fix it for me".
| } | ||
| } catch (e: Exception) { | ||
| Log.e("XMTPModule", "Error in message deletions subscription: $e") | ||
| subscriptions[getMessageDeletionsCacheKey(installationId)]?.cancel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cancel() call in the catch block is redundant since the coroutine is already completing. It could also cancel a newer subscription if a race condition occurs where another call replaced this entry in the map. Consider removing this line.
| subscriptions[getMessageDeletionsCacheKey(installationId)]?.cancel() |
🚀 Want me to fix this? Reply ex: "fix it for me".
| updateGroupDescriptionPolicy = createPermissionOptionFromString(jsonObj.get("updateGroupDescriptionPolicy").asString), | ||
| updateGroupImagePolicy = createPermissionOptionFromString(jsonObj.get("updateGroupImagePolicy").asString), | ||
| updateMessageDisappearingPolicy = createPermissionOptionFromString(jsonObj.get("updateMessageDisappearingPolicy").asString), | ||
| updateAppDataPolicy = createPermissionOptionFromString(jsonObj.get("updateAppDataPolicy").asString), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parsing updateAppDataPolicy with jsonObj.get() can throw NullPointerException when the field is absent (e.g., older clients). Suggest guarding with has() and a default, consistent with CreateGroupParamsWrapper.
| updateAppDataPolicy = createPermissionOptionFromString(jsonObj.get("updateAppDataPolicy").asString), | |
| updateAppDataPolicy = if (jsonObj.has("updateAppDataPolicy")) createPermissionOptionFromString(jsonObj.get("updateAppDataPolicy").asString) else PermissionOption.Unknown, |
🚀 Want me to fix this? Reply ex: "fix it for me".
Add
Conversations.streamMessageDeletions,Conversation.deleteMessage, andConversation.prepareMessage(noSend)for libxmtp 1.9.0 updatesAdd message deletion streaming and events, support deleting messages, and allow preparing messages without sending; expose
publishMessageandnoSendacross DM/Group, propagateappDatain group creation, and expand permission policies.📍Where to Start
Start with the native event stream and handlers in XMTPModule.kt, then review the JS wiring in Conversations.ts and the new DM/Group methods in Dm.ts and Group.ts.
📊 Macroscope summarized e060fca. 10 files reviewed, 15 issues evaluated, 2 issues filtered, 4 comments posted
🗂️ Filtered Issues
android/src/main/java/expo/modules/xmtpreactnativesdk/XMTPModule.kt — 2 comments posted, 5 evaluated, 2 filtered
prepareMessagefunction signature changed from 3 parameters to 4 parameters with the addition ofnoSend: Boolean. If JavaScript callers have not been updated to pass this required parameter, the Expo module framework will likely throw a runtime error due to argument count mismatch. [ Low confidence ]prepareEncodedMessagefunction signature changed from 4 parameters to 5 parameters with the addition ofnoSend: Boolean. Existing JavaScript callers not passing this parameter will experience a runtime argument mismatch error. [ Low confidence ]