-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: Bump XMTP Version #1496
feat: Bump XMTP Version #1496
Conversation
Updated XMTP Version Migrated from DecodedMessage Handle as inboxIds rather than address
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 SwiftLint (0.57.0)ios/ConverseNotificationExtension/Xmtp/Messages.swift[ WalkthroughThis pull request introduces updates across multiple files related to XMTP message handling and dependencies. The primary changes involve modifying message type handling from Changes
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 |
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.
Actionable comments posted: 0
🧹 Nitpick comments (4)
utils/xmtpRN/conversations.ts (1)
49-49
: Update function documentationThe removal of the order parameter should be documented in the function's JSDoc comments to prevent confusion.
Add a comment explaining that conversation ordering is now handled internally by the XMTP SDK.
ios/ConverseNotificationExtension/Xmtp/Messages.swift (3)
81-84
: LGTM! Consider enhancing the inline comment.The transition from
senderAddress
tosenderInboxId
is well implemented. The inline comment explains the temporary nature of the address conversion, but could be more specific about the migration timeline.Consider updating the comment to include:
-// We replaced decodedMessage.senderAddress from inboxId to actual address -// so it appears well in the app until inboxId is a first class citizen +// TODO(XMTP-Migration): Temporarily converting inboxId to address for backwards +// compatibility until inboxId becomes a first-class citizen in version X.Y.Z
Line range hint
140-152
: LGTM! Enhanced error tracking with Sentry breadcrumbs.The migration from
DecodedMessage
toMessage
is well implemented with improved error tracking. However, the force unwrap ontry!
could be handled more gracefully.Consider handling the potential error case:
-if let conversation = try! await xmtpClient.findConversationByTopic(topic: envelope.contentTopic) { +if let conversation = try? await xmtpClient.findConversationByTopic(topic: envelope.contentTopic) {
166-176
: LGTM! Improved type safety and content type handling.The migration to
Message
type and the enhanced content type checking usingtypeID.starts(with:)
improves code robustness. Consider extracting content type prefixes as constants for better maintainability.Consider defining content type constants:
private enum ContentTypePrefix { static let text = "text" static let reply = "reply" static let remoteAttachment = "remoteStaticAttachment" static let transaction = "transactionReference" static let payment = "coinbase-messaging-payment-activity" // ... other types }Also applies to: 179-206
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ios/Podfile.lock
is excluded by!**/*.lock
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (6)
ios/ConverseNotificationExtension/Spam.swift
(2 hunks)ios/ConverseNotificationExtension/Xmtp/Messages.swift
(9 hunks)ios/Podfile
(1 hunks)package.json
(1 hunks)queries/useConversationListQuery.ts
(1 hunks)utils/xmtpRN/conversations.ts
(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (7)
ios/Podfile (1)
17-17
: Verify XMTP version compatibilityThe XMTP version has been bumped from 3.0.21 to 3.0.22.
Run this script to check the changelog and ensure compatibility:
✅ Verification successful
XMTP version 3.0.22 is valid
Version 3.0.22 exists as an official tag in the XMTP iOS repository, following the proper version sequence. The minor version bump from 3.0.21 appears safe to use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check XMTP changelog for version 3.0.22 # Check if there's a changelog file fd -t f -e md -e txt "CHANGELOG|changelog" | xargs rg -i "3\.0\.22" # Check commit history for version bump commits gh api graphql -f query=' { repository(owner: "xmtp", name: "xmtp-ios") { releases(first: 1, orderBy: {field: CREATED_AT, direction: DESC}) { nodes { name description tagName } } } }'Length of output: 765
Script:
#!/bin/bash # Check for version 3.0.22 related information gh api graphql -f query=' { repository(owner: "xmtp", name: "xmtp-ios") { refs(refPrefix: "refs/tags/", first: 5, orderBy: {field: TAG_COMMIT_DATE, direction: DESC}) { nodes { name target { ... on Tag { message } } } } issues(first: 5, states: OPEN, orderBy: {field: CREATED_AT, direction: DESC}) { nodes { title url } } } }' # Check if this version exists in any branch gh api repos/xmtp/xmtp-ios/branches | jq '.[].name' | grep -i "3\.0\.22"Length of output: 1330
queries/useConversationListQuery.ts (1)
182-190
: LGTM! API usage updated correctlyThe conversation list API call has been updated to use the new options object pattern, maintaining all required properties while aligning with the latest XMTP SDK version.
ios/ConverseNotificationExtension/Spam.swift (1)
155-155
: Good improvements to error handlingThe changes include:
- Updated parameter type from DecodedMessage to Message
- Added proper null checks for message content and type
Also applies to: 213-224
utils/xmtpRN/conversations.ts (1)
4-4
: Verify impact of removing conversation orderingThe ConversationOrder parameter has been removed from the XMTP SDK. This could affect how conversations are displayed in the UI.
Run this script to check for UI components that might depend on conversation ordering:
Also applies to: 35-35, 49-49, 63-63
ios/ConverseNotificationExtension/Xmtp/Messages.swift (3)
92-95
: LGTM! Consistent implementation across message types.The profile lookup implementation is consistently applied for both group messages and DMs, maintaining a uniform approach to handling inboxIds.
248-251
: LGTM! Enhanced error tracking with content type information.The error tracking now includes the specific content type, making debugging easier.
288-288
: LGTM! Consistent use of inboxId.The change to use
senderInboxId
is consistent with the overall migration strategy.
Updated XMTP Version
Migrated from DecodedMessage
Handle as inboxIds rather than address
Summary by CodeRabbit
Release Notes
Dependency Updates
Improvements
Technical Updates