Skip to content

Conversation

@neekolas
Copy link
Contributor

@neekolas neekolas commented Nov 19, 2025

Add inserted_at time filtering and sorting to Conversation.messages, Dm.messages, and Group.messages, and expose DecodedMessage.insertedAtNs for retrieval consistency

Introduce optional insertedAfterNs, insertedBeforeNs, and sortBy parameters across message fetch/count methods, map SortBy to FFI, and add initialization-time contract and checksum validation with a message deletion callback in library/src/main/java/xmtpv3.kt.

📍Where to Start

Start with the FFI contract/checksum and callback registration in xmtpv3.kt, then review parameter threading in Conversation.messages, Dm.messages, and Group.messages.


Macroscope summarized 252eebc.

Copy link
Contributor Author


How to use the Graphite Merge Queue

Add the label mergequeue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Nov 19, 2025

Claude finished @neekolas's task —— View job


Code Review Complete ✅

I've reviewed this PR that adds support for filtering and sorting messages by inserted_at time. Overall, this is a well-implemented feature with good test coverage. Here are my findings:


Strengths

  1. Consistent API Design: The new parameters (insertedAfterNs, insertedBeforeNs, sortBy) are added consistently across all message query methods in Conversation.kt, Dm.kt, and Group.kt.

  2. Good Test Coverage: Both DmTest.kt and GroupTest.kt include new test cases (testCanQueryMessagesByInsertedTime) that verify:

    • The insertedAtNs field is populated
    • Filtering by insertedAfterNs works correctly
    • The sortBy parameter accepts both SENT_TIME and INSERTED_TIME
    • countMessages respects the insertedAfterNs filter
  3. Proper Null Safety: All new parameters are nullable with sensible defaults, maintaining backward compatibility.

  4. Correct Coroutine Usage: All new async methods properly use withContext(Dispatchers.IO) for background execution.

  5. FFI Integration: The mapping between Kotlin enums and FFI types is clean and straightforward:

    sortBy = when (sortBy) {
        SortBy.SENT_TIME -> FfiSortBy.SENT_AT
        SortBy.INSERTED_TIME -> FfiSortBy.INSERTED_AT
    }

🔍 Observations & Minor Suggestions

1. DecodedMessage.kt (lines 55-56)

The new insertedAtNs property is well-placed. Consider adding KDoc to clarify the difference between sentAtNs and insertedAtNs for developers:

/**
 * Timestamp in nanoseconds when the message was inserted into the local database.
 * This may differ from sentAtNs due to sync delays.
 */
val insertedAtNs: Long
    get() = libXMTPMessage.insertedAtNs

2. Parameter Order (Dm.kt:191-201, Group.kt:224-234, etc.)

The new parameters are consistently placed at the end of the parameter lists. This is good for backward compatibility. However, grouping related parameters might improve readability:

Current:

excludeSenderInboxIds: List<String>? = null,
insertedAfterNs: Long? = null,
insertedBeforeNs: Long? = null,
sortBy: SortBy = SortBy.SENT_TIME,

Suggestion (minor): Keep as-is for now, but consider future refactoring to a data class for complex filter options if more parameters are added.

3. Enum Naming (DecodedMessage.kt:35-38)

The SortBy enum uses SENT_TIME and INSERTED_TIME. Consider if SENT_AT and INSERTED_AT would be more consistent with the property names (sentAtNs, insertedAtNs). This is very minor and current naming is acceptable.

4. Test Assertions (DmTest.kt:732, GroupTest.kt:1254)

The test verifies insertedAtNs > 0, which is good. Consider also verifying that insertedAtNs >= sentAtNs (since insertion can't happen before sending):

assert(firstMessage.insertedAtNs >= firstMessage.sentAtNs)

🔐 Security Considerations

No security issues identified. The changes:

  • Don't expose any sensitive data
  • Maintain proper access control (no new public APIs for crypto operations)
  • Use safe type conversions for timestamps

Performance Considerations

Appropriate for mobile devices:

  1. Database Queries: The new filtering/sorting happens at the FFI/database layer, not in memory—this is efficient.
  2. No N+1 Queries: All message operations use batch queries via findMessages().
  3. Nullable Parameters: Optional parameters mean no performance penalty for existing code paths.

Minor consideration: When using sortBy = INSERTED_TIME, ensure the underlying database has an index on the inserted_at column for optimal query performance. (This would be in the native layer, not visible in this PR.)


🧪 Test Coverage Assessment

Good coverage for the new functionality:

  • ✅ Basic filtering with insertedAfterNs
  • ✅ Sorting with both SortBy.SENT_TIME and SortBy.INSERTED_TIME
  • ✅ Integration with countMessages()
  • ✅ Tests in both DM and Group contexts

Potential additional tests (nice to have, not blocking):

  • Test insertedBeforeNs filter explicitly
  • Test combining insertedAfterNs + insertedBeforeNs (range query)
  • Test edge case: insertedAfterNs = insertedBeforeNs (should return empty or single message)
  • Test ordering differences when sortBy changes (verify actual order, not just count)

📋 Code Quality & Best Practices

Excellent Kotlin/Android practices:

  • Proper use of suspend functions
  • Consistent null safety with ? and default values
  • Clean when-expression exhaustiveness for enum mapping
  • No blocking calls in suspend functions
  • Proper use of Dispatchers.IO for potentially long-running operations

📦 Native Library Changes

The PR includes updates to:

  • xmtpv3.kt (auto-generated FFI bindings) - Massive diff (8375 additions, 7071 deletions)
  • Native .so binaries for all Android architectures

These are expected for FFI updates and cannot be meaningfully reviewed in this PR. Trust that the corresponding native library changes are correct.


Summary

Recommendation: ✅ APPROVE

This PR successfully adds inserted-time filtering and sorting with:

  • ✅ Clean, consistent API design
  • ✅ Good test coverage (with room for expansion)
  • ✅ Proper null safety and coroutine usage
  • ✅ No security or performance concerns
  • ✅ Backward compatible (all new parameters are optional)

Minor improvements suggested above are optional enhancements, not blockers.


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