-
Notifications
You must be signed in to change notification settings - Fork 22
Add support for filtering and sorting by inserted_at time #500
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
base: main
Are you sure you want to change the base?
Add support for filtering and sorting by inserted_at time #500
Conversation
How to use the Graphite Merge QueueAdd 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 finished @neekolas's task —— View job Code Review Complete ✅I've reviewed this PR that adds support for filtering and sorting messages by ✅ Strengths
🔍 Observations & Minor Suggestions1. DecodedMessage.kt (lines 55-56)The new /**
* 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.insertedAtNs2. 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 4. Test Assertions (DmTest.kt:732, GroupTest.kt:1254)The test verifies assert(firstMessage.insertedAtNs >= firstMessage.sentAtNs)🔐 Security Considerations✅ No security issues identified. The changes:
⚡ Performance Considerations✅ Appropriate for mobile devices:
Minor consideration: When using 🧪 Test Coverage Assessment✅ Good coverage for the new functionality:
Potential additional tests (nice to have, not blocking):
📋 Code Quality & Best Practices✅ Excellent Kotlin/Android practices:
📦 Native Library ChangesThe PR includes updates to:
These are expected for FFI updates and cannot be meaningfully reviewed in this PR. Trust that the corresponding native library changes are correct. ✅ SummaryRecommendation: ✅ APPROVE This PR successfully adds inserted-time filtering and sorting with:
Minor improvements suggested above are optional enhancements, not blockers. |

Add inserted_at time filtering and sorting to
Conversation.messages,Dm.messages, andGroup.messages, and exposeDecodedMessage.insertedAtNsfor retrieval consistencyIntroduce optional
insertedAfterNs,insertedBeforeNs, andsortByparameters across message fetch/count methods, mapSortByto 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, andGroup.messages.Macroscope summarized 252eebc.