Skip to content

Conversation

cameronvoell
Copy link
Contributor

@cameronvoell cameronvoell commented Aug 27, 2025

Fix initiatedByInboxId nesting for ContentTypeGroupUpdated in Android expo.modules.xmtpreactnativesdk.wrappers.ContentJson and add tests for the group-updated message upon joining a group or DM

  • Move initiatedByInboxId under the groupUpdated object for ContentTypeGroupUpdated in ContentJson.kt.
  • Add a test in conversationTests.ts that registers GroupUpdatedCodec, creates a group and a DM, and asserts the first message is xmtp.org/group_updated:1.0 with the expected initiatedByInboxId using GroupUpdatedContent.
  • Update Podfile.lock to reference XMTPReactNative 4.4.0.

📍Where to Start

Start with the ContentTypeGroupUpdated mapping in ContentJson.kt, then review the new test in conversationTests.ts.


Macroscope summarized ab5b96b.

@cameronvoell cameronvoell requested a review from a team as a code owner August 27, 2025 01:08
Copy link

changeset-bot bot commented Aug 27, 2025

⚠️ No Changeset found

Latest commit: ab5b96b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

claude bot commented Aug 29, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

Copy link

claude bot commented Aug 29, 2025

Claude finished @cameronvoell's task —— View job


Code Review for PR #712

✅ Summary

This PR correctly fixes the initiatedByInboxId nesting issue for ContentTypeGroupUpdated on Android and adds comprehensive tests to verify the group-updated message functionality when joining groups and DMs.

🔍 Review Findings

1. Android ContentJson.kt Fix

The change correctly moves initiatedByInboxId from being a top-level field to being nested under the groupUpdated object (lines 263-264):

// Before: "initiatedByInboxId" was at the wrong level
// After: Properly nested under "groupUpdated"
ContentTypeGroupUpdated.id -> mapOf(
    "groupUpdated" to mapOf(
        "initiatedByInboxId" to (content as GroupUpdated).initiatedByInboxId,
        // ... other fields
    )
)

This aligns with the expected JSON structure and matches the iOS implementation.

2. New Test Coverage

The test new groups and dms contain a message including who added the user (conversationTests.ts:1334-1385) is well-structured:

  • Properly registers the GroupUpdatedCodec before testing
  • Tests both group and DM scenarios
  • Verifies the message type is xmtp.org/group_updated:1.0
  • Correctly validates that initiatedByInboxId identifies who added the user
  • Uses appropriate assertions with descriptive error messages

3. Podfile.lock Changes

The update to XMTPReactNative version 4.4.0 appears to be a routine dependency update that aligns with the SDK version.

💡 Suggestions for Improvement

  1. Remove console.log statements: The test includes two console.log statements (lines 1353 and 1375) that should be removed before merging:

    console.log(message.contentTypeId)  // Line 1353
    console.log(dmMessage.contentTypeId) // Line 1375
  2. Consider edge case testing: While the test covers the happy path well, consider adding tests for:

    • What happens when initiatedByInboxId is missing or null
    • Behavior when multiple members are added simultaneously

✅ Security & Performance

  • No security concerns identified
  • The change is a simple data structure fix with minimal performance impact
  • Tests are appropriately scoped and shouldn't affect test suite performance

✅ Best Practices

  • Follows project conventions for test structure
  • Uses existing test utilities appropriately
  • Maintains consistency with other conversation tests

📝 Verdict

LGTM with minor suggestion to remove the console.log statements. The fix correctly addresses the Android parsing issue and the test provides good coverage for the functionality.

@cameronvoell cameronvoell merged commit 34d4d81 into main Aug 29, 2025
8 of 9 checks passed
@cameronvoell cameronvoell deleted the cv/add-test-for-group-updated-upon-joining-conversation branch August 29, 2025 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants