add message deletion and overhaul the example app#526
Conversation
|
Claude finished @mchenani's task —— View job Code Review Complete ✅I've completed a comprehensive review of PR #526. This is a significant overhaul adding message deletion, encrypted key storage, and a complete UI redesign with Jetpack Compose. Here are my findings: 🔴 Critical Issues1. Thread Safety Issues in ClientManager (
|
example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailViewModel.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailActivity.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailViewModel.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailViewModel.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailActivity.kt
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/connect/ConnectWalletViewModel.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailViewModel.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailActivity.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailActivity.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailActivity.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/conversation/AttachmentPreviewActivity.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/conversation/AttachmentPreviewActivity.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailActivity.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailActivity.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailActivity.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailActivity.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/MainViewModel.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailActivity.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/ClientManager.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/ClientManager.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailActivity.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailActivity.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailActivity.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailActivity.kt
Outdated
Show resolved
Hide resolved
example/src/main/java/org/xmtp/android/example/ClientManager.kt
Outdated
Show resolved
Hide resolved
| myExistingReaction != null -> { | ||
| viewModel.sendReaction(message.id, myExistingReaction!!, isRemoving = true) | ||
| viewModel.sendReaction(message.id, emoji, isRemoving = false) | ||
| } |
There was a problem hiding this comment.
When replacing a reaction, the result of removing the old reaction is discarded. If removal fails, the error is silently ignored. Consider checking the first sendReaction result before proceeding with the second.
- myExistingReaction != null -> {
- viewModel.sendReaction(message.id, myExistingReaction!!, isRemoving = true)
- viewModel.sendReaction(message.id, emoji, isRemoving = false)
- }
+ myExistingReaction != null -> {
+ val removeResult = viewModel.sendReaction(message.id, myExistingReaction!!, isRemoving = true)
+ if (removeResult is ConversationDetailViewModel.ReactionState.Success) {
+ viewModel.sendReaction(message.id, emoji, isRemoving = false)
+ } else {
+ removeResult
+ }
+ }🚀 Want me to fix this? Reply ex: "fix it for me".
| onBackClick: () -> Unit, | ||
| onNavigateToGroupManagement: (String) -> Unit, | ||
| onNavigateToUserProfile: (String, String?) -> Unit, | ||
| onNavigateToAttachmentPreview: (List<Uri>) -> Unit, |
There was a problem hiding this comment.
onNavigateToAttachmentPreview is unused. Suggest either remove it or route navigation through it instead of attachmentPreviewLauncher for consistency.
🚀 Want me to fix this? Reply ex: "fix it for me".
| MainActivityContent( | ||
| viewModel = viewModel, | ||
| onConversationClick = { conversationId, topic -> | ||
| openConversation(topic, conversationId) |
There was a problem hiding this comment.
Using the topic for both conversationId and peerAddress is incorrect and creates redundant mapping. Suggest pass the real peerAddress from ConversationItem to onConversationClick/openConversation and drop the topic→topic map.
🚀 Want me to fix this? Reply ex: "fix it for me".
| now.get(Calendar.DAY_OF_YEAR) == messageCalendar.get(Calendar.DAY_OF_YEAR) -> { | ||
| SimpleDateFormat("HH:mm", Locale.getDefault()).format(messageDate) | ||
| } | ||
| now.get(Calendar.YEAR) == messageCalendar.get(Calendar.YEAR) && |
There was a problem hiding this comment.
The "Yesterday" check fails at year boundaries. On January 1st, a message from December 31st yields a difference of -364, not 1. Consider comparing dates properly using calendar day manipulation instead of day-of-year arithmetic.
🚀 Want me to fix this? Reply ex: "fix it for me".
| if (showDeleteConfirmation && selectedMessage != null) { | ||
| DeleteConfirmationDialog( | ||
| onConfirm = { | ||
| scope.launch { | ||
| viewModel.deleteMessage(selectedMessage!!.id).collect { state -> |
There was a problem hiding this comment.
Consider capturing selectedMessage in a local variable before using it in the onConfirm lambda. Since the lambda executes later, another callback could set selectedMessage = null before confirm is clicked, causing a NullPointerException at selectedMessage!!.id.
- if (showDeleteConfirmation && selectedMessage != null) {
+ val messageToDelete = selectedMessage
+ if (showDeleteConfirmation && messageToDelete != null) {
DeleteConfirmationDialog(
onConfirm = {
scope.launch {
- viewModel.deleteMessage(selectedMessage!!.id).collect { state ->
+ viewModel.deleteMessage(messageToDelete.id).collect { state ->🚀 Want me to fix this? Reply ex: "fix it for me".
|
|
||
| private val attachmentUris = mutableListOf<Uri>() | ||
| private val captions = mutableMapOf<Int, String>() | ||
| private val previewBitmaps = mutableMapOf<Int, Bitmap>() |
There was a problem hiding this comment.
Manual Bitmap.recycle() here conflicts with Compose state holding the same instances and can crash during teardown. Suggest removing the manual recycling and letting GC handle it, or manage bitmap lifecycle in Compose (e.g., DisposableEffect) and avoid storing shared bitmaps at the activity level.
🚀 Want me to fix this? Reply ex: "fix it for me".
| var showLogViewerSheet by remember { mutableStateOf(false) } | ||
|
|
||
| // Track log and hide deleted states for UI updates | ||
| var logsEnabled by remember { mutableStateOf(isLogsEnabled) } |
There was a problem hiding this comment.
Local state logsEnabled and hideDeleted won't update when isLogsEnabled or hideDeletedMessages parameters change after recomposition. Consider using rememberUpdatedState or a LaunchedEffect to sync with parameter changes.
🚀 Want me to fix this? Reply ex: "fix it for me".
| Intent(context, AttachmentPreviewActivity::class.java).apply { | ||
| putParcelableArrayListExtra(EXTRA_ATTACHMENT_URIS, ArrayList(uris)) | ||
| // Grant read permission for URIs so they can be accessed in the preview activity | ||
| addFlags(Intent.FLAG_GRANT_READ_URI_PERMISSION) | ||
| } |
There was a problem hiding this comment.
FLAG_GRANT_READ_URI_PERMISSION only grants access for URIs passed via setData() or setClipData(), not putParcelableArrayListExtra(). Consider using ClipData to grant permissions for multiple URIs.
- Intent(context, AttachmentPreviewActivity::class.java).apply {
- putParcelableArrayListExtra(EXTRA_ATTACHMENT_URIS, ArrayList(uris))
- // Grant read permission for URIs so they can be accessed in the preview activity
- addFlags(Intent.FLAG_GRANT_READ_URI_PERMISSION)
- }
+ Intent(context, AttachmentPreviewActivity::class.java).apply {
+ putParcelableArrayListExtra(EXTRA_ATTACHMENT_URIS, ArrayList(uris))
+ addFlags(Intent.FLAG_GRANT_READ_URI_PERMISSION)
+ // Use ClipData to grant URI permissions for all URIs
+ if (uris.isNotEmpty()) {
+ val clipData = android.content.ClipData.newRawUri(null, uris.first())
+ uris.drop(1).forEach { clipData.addItem(android.content.ClipData.Item(it)) }
+ setClipData(clipData)
+ }
+ }🚀 Want me to fix this? Reply ex: "fix it for me".
| onGenerateWallet = { environment, logLevel -> | ||
| viewModel.generateWallet(environment, logLevel) | ||
| }, | ||
| onConnectSuccess = { address -> |
There was a problem hiding this comment.
On configuration changes, LaunchedEffect re-executes while uiState remains Success, causing duplicate signIn calls. Consider resetting the UI state after handling success, or using a one-shot event pattern to prevent re-triggering.
🚀 Want me to fix this? Reply ex: "fix it for me".
Add message deletion and overhaul the Android example app to a Compose-based UI with synchronized client creation in
example.ClientManagerIntroduce
DeletedMessagehandling and delete operations in conversation flows, migrate activities and screens to Jetpack Compose, and refactorexample.ClientManagerto synchronize client lifecycle, rebuild wallet from stored keys, and register codecs. Secure key storage moves toEncryptedSharedPreferencesin KeyUtil.kt, and navigation, messaging, and attachments are implemented via new Compose components and view models.📍Where to Start
Start with the client lifecycle changes in
example.ClientManagerin ClientManager.kt, then review message classification and delete handling in ConversationDetailViewModel.kt.📊 Macroscope summarized 0259338. 9 files reviewed, 52 issues evaluated, 36 issues filtered, 9 comments posted
🗂️ Filtered Issues
example/src/main/java/org/xmtp/android/example/ClientManager.kt — 0 comments posted, 7 evaluated, 7 filtered
lockobject is used inconsistently for protecting_clientStateand_client. IncreateClient(), the state is checked and set toCreatinginsidesynchronized(lock)(lines 93-97), but the final state updates toReadyorError(lines 140, 143) happen outside any synchronization. This means concurrent calls tosetClient()orclearClient()could interleave with these state updates, leading to inconsistent state. [ Already posted ]clearClient()and the coroutine launched increateClient(). WhenclearClient()is called, it cancelsmanagerScopeat line 167 and sets_client = nullat line 165. However, if a coroutine fromcreateClient()is still running (lines 99-145), it may complete afterclearClient()finishes, setting_clientto a new value and_clientStatetoReady(line 140) orError(line 143), overwriting theUnknownstate set byclearClient(). The coroutine's state updates at lines 140 and 143 are not synchronized with thelock. [ Already posted ]createClientfunction signature expects anaddressparameter to look up the private key, but the example usage inMainActivitypasses the result ofKeyUtil.loadKeys()which returns the account password/key data fromAccountManager, not the wallet address. This mismatch will causekeyUtil.retrievePrivateKey(address)to look up with an incorrect key, returningnulland throwingIllegalStateException("No wallet key found. Please sign in again.")for all existing users. [ Already posted ]SharedPreferenceswithcontext.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE), butmigrateLegacyPrefsIfNeeded()tries to read the legacy prefs asEncryptedSharedPreferences. This will throw an exception when trying to decrypt the unencrypted data, causing the catch block to mark migration as complete without actually migrating the keys. Existing users will lose their DB encryption keys stored under thexmtp-dev-prefix. [ Already posted ]rebuiltAddressfromwallet.publicIdentity.identifierforclientOptions, but the originaladdressparameter was used forkeyUtil.retrievePrivateKey(address). If these addresses differ in case or format, the DB encryption key lookup inclientOptionscould create a new key instead of retrieving the existing one, potentially causing data access issues. [ Already posted ]clearClient()function cancelsmanagerScopewhilecreateClient()may have an active coroutine running. IfclearClient()is called immediately aftercreateClient(), the state transitions toClientState.Creatingbut the coroutine is cancelled before it can transition toReadyorError. Subsequent calls tocreateClient()will return early at line 95 due to theClientState.Creatingcheck, leaving the client permanently stuck in theCreatingstate. [ Already posted ]clearClient(): CancellingmanagerScopedoes not immediately stop non-suspending code in thecreateClient()coroutine. AfterclearClient()sets_client = nulland_clientState.value = ClientState.Unknown, the already-running coroutine fromcreateClient()can continue executing lines 135-140, setting_clientState.value = ClientState.Readywhile_clientremainsnull. This leaves the system in an inconsistent state whereclientStateisReadybut_clientisnull, causing theclientgetter to throwNullPointerExceptionwhen it executes_client!!. [ Already posted ]example/src/main/java/org/xmtp/android/example/MainActivity.kt — 2 comments posted, 9 evaluated, 5 filtered
migrateLegacyPrefsIfNeededattempts to read legacy data usingEncryptedSharedPreferences, but the original implementation stored keys in plainSharedPreferences(viacontext.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE)). WhenEncryptedSharedPreferences.create()tries to open the unencrypted legacy file, it will throw an exception because the data format is incompatible. The exception is caught and silently ignored, marking migration as complete without actually migrating any data. This causes data loss for existing users - their DB encryption keys stored in plain prefs will not be migrated andretrieveKey()will returnnull. [ Low confidence ]KeyUtilattempts to read legacy SharedPreferences asEncryptedSharedPreferences, but the old code used regularSharedPreferences(viacontext.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE)). WhenmigrateLegacyPrefsIfNeededtries to open the legacy file withEncryptedSharedPreferences.create(), it will fail because the file format is incompatible. The exception is caught and migration is marked complete without actually migrating data. This causes existing users to lose their DB encryption keys whenClientManager.createClient()is called, resulting in data loss as their encrypted database becomes unreadable with the newly generated key. [ Low confidence ]keysreturned byKeyUtil(this).loadKeys()(which retrievesaccountManager.getPassword(account)) is passed toClientManager.createClient(keys, this)where the first parameter is namedaddressand is used to look up private keys viakeyUtil.retrievePrivateKey(address). If the value stored as the account password is not the Ethereum address used when storing keys viastorePrivateKey(), the lookup will fail and throwIllegalStateException("No wallet key found"), forcing users to re-authenticate even if their data exists. [ Low confidence ]clientState.message.contains("No wallet key found")relies on matching a specific substring in the error message. If the error message text is changed or localized inClientManager.createClient(), this check will fail to detect the condition, and users will see a generic error toast instead of being redirected to sign-in to re-authenticate. [ Already posted ]now.get(Calendar.DAY_OF_YEAR) - messageCalendar.get(Calendar.DAY_OF_YEAR) == 1) which fails at year boundaries. If today is January 1st (day 1) and the message is from December 31st (day 365), the calculation yields-364not1, so "Yesterday" won't display. The same issue affects the "within last week" calculation where negative differences would incorrectly be< 7. However, since the year check (now.get(Calendar.YEAR) == messageCalendar.get(Calendar.YEAR)) filters these out, cross-year messages fall through to show the date format, which is acceptable but semantically incorrect for recent messages. [ Low confidence ]example/src/main/java/org/xmtp/android/example/MainViewModel.kt — 0 comments posted, 3 evaluated, 2 filtered
.map { }block starting at line 57,ClientManager.client.conversations.getHmacKeys()is called for every topic returned byallPushTopics(). This means the potentially expensivegetHmacKeys()network/database call is executed N times (once per topic) instead of once. This should be moved outside the map to call it once and reuse the result, improving performance and avoiding potential rate limiting or timeout issues with many conversations. [ Out of scope ]streamandmessageStreamflows (lines 152-155 and 170-173), when an exception occurs during processing (e.g., ingetConversationDisplayInfoorfetchMostRecentMessage), the.catchblock emitsnulland the inner flow terminates. SinceclientStatemay not emit anotherReadystate, a single transient error (like a network failure while fetching group name or members) could cause the conversation/message stream to stop permanently until the app is restarted or client state cycles. [ Already posted ]example/src/main/java/org/xmtp/android/example/conversation/AttachmentPreviewActivity.kt — 2 comments posted, 10 evaluated, 5 filtered
attachmentsinside therememberblock performs synchronous bitmap loading vialoadBitmap(uri)on the main/UI thread during composition. For large images,BitmapFactory.decodeStream()can take significant time, potentially causing ANR (Application Not Responding) if it exceeds 5 seconds. [ Already posted ]BitmapFactory.decodeStream(). For high-resolution images (e.g., camera photos), this could causeOutOfMemoryErrorwhen multiple large attachments are loaded simultaneously. [ Already posted ]loadBitmapis called during Compose'srememberblock initialization, which runs on the main thread. Loading multiple images synchronously blocks the UI thread and can cause ANR (Application Not Responding) if the images are large or numerous. [ Already posted ]loadBitmaploads full-resolution images without downsampling. For large images (e.g., high-resolution camera photos), this can causeOutOfMemoryErrorsinceBitmapFactory.decodeStreamwill attempt to allocate memory for the entire unscaled bitmap. [ Already posted ]BitmapFactory.decodeStream()withoutBitmapFactory.Options.inSampleSizecan causeOutOfMemoryErrorwhen processing large images. There is no downsampling or size limiting applied before decoding. [ Already posted ]example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailActivity.kt — 4 comments posted, 11 evaluated, 6 filtered
cameraImageUristate is stored usingremember { mutableStateOf<Uri?>(null) }which does not survive configuration changes. If the device rotates while the camera app is open,cameraImageUriwill be reset tonull. When the camera returns successfully at line 352-355, the checkif (success && cameraImageUri != null)will fail and the captured photo will be silently discarded. [ Already posted ]pendingAttachmentUrisandpendingCaptionsstate variables are declared at lines 243-244 but are never used anywhere in the function, suggesting incomplete implementation of attachment handling logic. [ Low confidence ]pendingAttachmentUrisandpendingCaptionsstate variables are declared but never used. The attachment sending logic inattachmentPreviewLauncherreads URIs and captions directly from the result intent rather than using these state variables. This appears to be dead code from an incomplete refactor. [ Low confidence ]galleryLauncherandfileLauncher, the code catchesSecurityExceptionwhen callingtakePersistableUriPermission()and continues silently. If permissions cannot be persisted and the process is killed whileAttachmentPreviewActivityis showing, the URIs may become unreadable when the result returns, causingopenInputStream(uri)to return null. While the null case is handled, the user receives no indication of why the attachment failed. [ Low confidence ]avatarColorsis passed as an empty list, the expressionavatarColors[abs(headerTitle.hashCode()) % avatarColors.size]at line 486 will throwArithmeticException(division by zero) orIndexOutOfBoundsException. The parameter typeList<Color>allows empty lists but the implementation assumes it's non-empty. [ Low confidence ]example/src/main/java/org/xmtp/android/example/conversation/ConversationDetailViewModel.kt — 0 comments posted, 11 evaluated, 11 filtered
message.encodedContent.type?.typeIdonDecodedMessage) andclassifyMessagefiltering (line 411 usingmessage.contentTypeId.typeIdonDecodedMessageV2). If these properties represent content types differently between the two message formats, hidden content types could slip through the stream-level check and causeclassifyMessageto return null, resulting in unexpected null emissions to the StateFlow. [ Low confidence ]findEnrichedMessagereturns null orclassifyMessagereturns null (lines 182-188), themapLatestblock implicitly returns null, which is emitted to the StateFlow. This could lead to unexpected null values being observed by collectors when messages exist but cannot be enriched or classified, rather than clearly indicating why the message was filtered. [ Already posted ]streamMessagesat lines 182-188, whenfindEnrichedMessagereturns null orclassifyMessagereturns null, themapLatestblock evaluates to null. This null emission is indistinguishable from the null emitted on errors in the catch block at line 192, potentially making it difficult for consumers to differentiate between "message couldn't be processed" and "an error occurred". [ Already posted ]sendMessage, ifconversationis null, theconversation?.send()calls on lines 228-231 and 235 will silently do nothing due to the safe call operator, butSendMessageState.Successwill still be emitted on line 237. This reports success to the UI even though no message was actually sent. [ Already posted ]sendMessage, whenconversationis null and either a reply or regular message is being sent (lines 228-235), theconversation?.send()call silently does nothing due to the safe call operator, butSendMessageState.Successis still emitted at line 237. This causes the UI to report success when the message was never actually sent. [ Already posted ]deleteMessagefunction silently succeeds whenconversationis null. The safe callconversation?.deleteMessage(messageId)skips the operation ifconversationis null, but the code still setsflow.value = DeleteMessageState.Success. This misleads callers into thinking the deletion succeeded when nothing actually happened, causing the UI to show "Message deleted" while the message remains. [ Already posted ]deleteMessage, whenconversationis null,conversation?.deleteMessage(messageId)at line 250 silently does nothing due to the safe call operator, butDeleteMessageState.Successis still set at line 251. This reports successful deletion when nothing was actually deleted. [ Already posted ]sendReaction, ifconversationis null,conversation?.send()on line 273 will silently do nothing, butReactionState.Successis returned on line 284. This reports success even though no reaction was actually sent. [ Already posted ]sendReaction, whenconversationis null,conversation?.send()at line 273 silently does nothing, andReactionState.Successis returned at line 284. This reports success when the reaction was never sent. [ Already posted ]sendAttachment, after the explicit null check at lines 305-307 that returns an error, the code at line 309 still usesconversation?.send()with a safe call. Sinceconversationis avar, Kotlin cannot smart-cast it, meaning ifconversationis set to null by another thread between the check and the send, the attachment won't be sent butSendAttachmentState.Successwill still be returned at line 314. [ Already posted ]sendAttachment, after the explicit null check on lines 305-307 returns an error ifconversationis null, line 309 still usesconversation?.send()with a safe call operator. Ifconversationwere to become null between the check and the send (race condition), the send would silently fail butSendAttachmentState.Successwould still be returned on line 314. [ Already posted ]