Skip to content
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

Bug streaming groups multi client #496

Merged
merged 5 commits into from
Sep 20, 2024

Conversation

nplasterer
Copy link
Contributor

@nplasterer nplasterer commented Sep 19, 2024

Fixes #495

@nplasterer nplasterer self-assigned this Sep 19, 2024
@nplasterer
Copy link
Contributor Author

Does not reproduce in Android proper. Only reproduces in iOS and Android inside of ReactNative. Makes me think it's an event listener issue.

@nplasterer nplasterer marked this pull request as ready for review September 20, 2024 02:04
@nplasterer nplasterer requested a review from a team as a code owner September 20, 2024 02:04
@nplasterer
Copy link
Contributor Author

All the other methods already had this check so everything should work now. 🙏

}

return true
})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nmalzieu I think this fix also fixed this weird bug? Can you confirm?

@nplasterer
Copy link
Contributor Author

Screenshot 2024-09-19 at 10 37 10 PM

@@ -146,7 +146,7 @@ export default class Conversations<
const groupsSubscription = XMTPModule.emitter.addListener(
EventTypes.Group,
async ({ inboxId, group }: { inboxId: string; group: GroupParams }) => {
if (this.known[group.id]) {
if (this.known[group.id] || this.client.inboxId !== inboxId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good fix to merge now.

long term, I wonder if it would be a worthwhile performance gain to programmatically generate these Expo Module events to include the inbox-id in their name, so we dont send copies of all clients' events to each client event listener and then need to filter them out.

Events(
// Auth
"sign",
"authed",
"preCreateIdentityCallback",
"preEnableIdentityCallback",
"preAuthenticateToInboxCallback",
// Conversations
"conversation",
"group",
"conversationContainer",
"message",
"allGroupMessage",
// Conversation
"conversationMessage",
// Group
"groupMessage",
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into that and it didn't seem straight forward. But something we should consider looking into again for sure.

Copy link
Contributor

@cameronvoell cameronvoell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice fix! 🚢

@nplasterer nplasterer merged commit 83a352d into main Sep 20, 2024
5 of 6 checks passed
@nplasterer nplasterer deleted the np/group-streaming-every-persons-group branch September 20, 2024 19:14
Copy link

🎉 This PR is included in version 2.5.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 Clients logged in on the same app streaming all messages
2 participants