Skip to content

add delete message codec#530

Merged
mchenani merged 7 commits intomainfrom
mc/add-delete-message-codecs
Jan 28, 2026
Merged

add delete message codec#530
mchenani merged 7 commits intomainfrom
mc/add-delete-message-codecs

Conversation

@mchenani
Copy link
Contributor

@mchenani mchenani commented Jan 26, 2026

Add delete message codec by introducing DeleteMessageCodec for DeleteMessageRequest content in org.xmtp.android.library

Add DeleteMessageRequest data class, register DeleteMessageCodec with contentType version 1.0, implement encode/decode via UniFFI bindings, set fallback to null and shouldPush to false, and add tests. Update UniFFI Kotlin bindings and native libraries. Add KDoc to Conversation, Dm, and Group message retrieval methods.

📍Where to Start

Start with the codec implementation in DeleteMessageCodec.kt, then review the encode/decode bindings in xmtpv3.kt, and validate behavior with DeleteMessageCodecTest.kt.


Macroscope summarized 9c25aae.

@mchenani mchenani requested a review from a team as a code owner January 26, 2026 22:01
@claude
Copy link

claude bot commented Jan 26, 2026

Claude finished @mchenani's task —— View job


Code Review Complete ✅

I've reviewed the PR and here's my detailed feedback:


Overall Assessment

This is a well-implemented feature that adds delete message functionality with proper codec implementation, FFI bindings, and comprehensive test coverage. The code follows Kotlin/Android best practices and integrates cleanly with the existing codebase.


✅ Strengths

1. Excellent Test Coverage (library/src/androidTest/java/org/xmtp/android/library/DeleteMessageCodecTest.kt)

  • ✅ Unit tests for encode/decode functionality
  • ✅ Tests for fallback behavior (correctly returns null)
  • ✅ Tests for push behavior (correctly returns false)
  • ✅ Content type registration tests
  • ✅ End-to-end integration tests with both DM and Group conversations
  • ✅ Cross-client decoding tests

2. Clean Architecture (library/src/main/java/org/xmtp/android/library/codecs/DeleteMessageCodec.kt)

  • ✅ Simple, focused data class with a single property
  • ✅ Proper content type registration following existing patterns
  • ✅ Codec implementation correctly delegates to FFI layer
  • ✅ Good documentation explaining the feature and permissions

3. Consistent API Integration (Conversation.kt, Dm.kt, Group.kt)

  • deleteMessage() method properly added to all conversation types
  • ✅ Consistent error handling across implementations
  • ✅ Good use of withContext(Dispatchers.IO) for coroutine dispatching
  • ✅ Clear documentation explaining permissions (users can delete their own messages, super admins can delete any message in groups)

4. Proper FFI Bindings (xmtpv3.kt)

  • ✅ Auto-generated code with proper type conversions
  • ✅ Error handling with GenericException

🔍 Detailed Analysis

Code Quality & Kotlin Best Practices

  • Idiomatic Kotlin: Uses data classes, extension functions, and proper null safety
  • Naming conventions: Clear, descriptive names (DeleteMessageRequest, ContentTypeDeleteMessageRequest)
  • Documentation: KDoc comments present on public APIs
  • Code organization: Follows existing patterns in the codebase

Null Safety

  • fallback() returns String? (nullable) and correctly returns null - this is appropriate for this codec
  • ✅ No force unwraps (!!) or unsafe casts
  • ✅ Proper handling of nullable types throughout

Coroutine Usage

  • ✅ All suspend functions properly use withContext(Dispatchers.IO) for I/O operations
  • ✅ Consistent with existing coroutine patterns in Group.kt and Dm.kt (lines 209-216 in Dm.kt, 253-260 in Group.kt)
  • ✅ No blocking operations on main thread

Performance Considerations

  • Lightweight data structure: DeleteMessageRequest only contains a single String field
  • Efficient serialization: Uses Protocol Buffers via FFI
  • No unnecessary allocations: Direct delegation to FFI layer
  • shouldPush = false: Correctly avoids unnecessary push notifications for delete operations (line 47 in DeleteMessageCodec.kt)

Security Concerns

  • Authorization handled at FFI layer: The FFI deleteMessage() call enforces permissions (users can only delete their own messages, super admins can delete any)
  • Error wrapping: Proper exception handling that doesn't leak sensitive information (lines 211-215 in Dm.kt, 254-259 in Group.kt)
  • Message ID validation: Uses hex-encoded IDs with hexToByteArray() conversion
  • ⚠️ Minor consideration: The error message wraps the underlying exception message, which could potentially leak some internal details, but this is acceptable for debugging purposes

Test Coverage

Comprehensive testing at multiple levels:

  1. Unit tests: Encode/decode, fallback, push behavior (lines 62-97)
  2. Integration tests: Message sending and receiving (lines 30-59)
  3. Cross-conversation tests: Both DM and Group support (lines 100-142)
  4. Content type verification: Ensures proper codec registration (lines 144-177)

💡 Suggestions for Improvement

1. Consider Adding MessageId Type Safety (Optional Enhancement)

Currently, messageId is a plain String. Consider creating a type alias or inline class for better type safety:

@JvmInline
value class MessageId(val value: String)

data class DeleteMessageRequest(
    val messageId: MessageId,
)

This would prevent accidental mixing of message IDs with other string types. However, this is a minor enhancement and not critical.

2. Test Edge Cases (Nice to Have)

Consider adding tests for:

  • Empty or invalid message IDs
  • Attempting to delete non-existent messages
  • Attempting to delete already-deleted messages
  • Permission-denied scenarios

These might already be covered at the FFI level, but explicit Android-side tests would provide additional confidence.

3. Documentation Enhancement (Minor)

Consider adding a usage example in the KDoc for DeleteMessageRequest:

/**
 * Represents a request to delete a message.
 *
 * This content type is used to request deletion of a specific message in a conversation.
 * The message will be deleted for all participants.
 *
 * @property messageId The ID of the message to delete
 *
 * Example:
 * ```kotlin
 * val messageId = conversation.send("Hello!")
 * conversation.deleteMessage(messageId)
 * ```
 */

⚠️ Potential Issues (Minor)

1. No Public Codec Registration in Tests

The tests manually register the codec with Client.register(codec = DeleteMessageCodec()), but there's no indication if this codec should be registered by default. If it's a core feature, consider auto-registering it in the SDK initialization.

Location: DeleteMessageCodecTest.kt lines 31, 101, 146

Recommendation: Document whether developers need to manually register this codec or if it will be registered automatically.


📊 Summary

Category Rating Notes
Code Quality ⭐⭐⭐⭐⭐ Clean, idiomatic Kotlin following existing patterns
Test Coverage ⭐⭐⭐⭐⭐ Comprehensive unit and integration tests
Null Safety ⭐⭐⭐⭐⭐ Proper nullable types, no unsafe operations
Coroutine Usage ⭐⭐⭐⭐⭐ Correct dispatcher usage, consistent patterns
Performance ⭐⭐⭐⭐⭐ Lightweight, efficient implementation
Security ⭐⭐⭐⭐☆ Good authorization, minor consideration on error messages
Documentation ⭐⭐⭐⭐☆ Good KDocs, could use more examples

✅ Recommendation

This PR is ready to merge. The implementation is solid, well-tested, and follows Android/Kotlin best practices. The suggestions above are minor enhancements that could be addressed in follow-up PRs if desired.

Great work on this feature! 🎉


Review completed by Claude Code

@mchenani mchenani requested a review from cameronvoell January 26, 2026 22:12
@mchenani mchenani merged commit 6e9d4e5 into main Jan 28, 2026
8 of 9 checks passed
@mchenani mchenani deleted the mc/add-delete-message-codecs branch January 28, 2026 22:57
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