From c7a4796b25a4ceeffeef626b96a9656c1a6f8ab5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 19 May 2026 22:04:14 +0000 Subject: [PATCH] refactor(marmot): audit follow-ups on MLS reply paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-audit of the previous MLS reply commit surfaced four concrete improvements: 1. Push-notification cold-start replies now thread reliably. The receiver previously rebuilt the parent inner event by looking it up in LocalCache; in a cold-process broadcast that cache hasn't been re-hydrated yet (Account.restoreAll runs async on init), so the q-tag silently dropped. Carry the parent's inner event id AND author pubkey through the Intent extras and feed them straight to MarmotManager.buildTextMessage, which now takes (eventId, author) instead of a full Event. The cold-start reply is now always threaded, not just the warm-cache case. 2. marmotGroupRelays() is no longer duplicated. The receiver was reimplementing what AccountViewModel had as a private fun (which itself was used 9× inside the VM). Lifted to Account so headless callers can reach it without spinning up a ViewModel; both sites now share one implementation. 3. Dropped the dead editFromDraft / draftId plumbing. Added for route symmetry with NIP-17 but Marmot has no draft persistence, so the parameter rode all the way through MarmotGroupChatView only to be ignored under @Suppress("UNUSED_PARAMETER"). Per CLAUDE.md, don't pre-emptively abstract; reinstate when drafts actually land. 4. MarmotGroupMessageComposer no longer has default-param `remember` blocks. There's exactly one caller and it always passes both messageState and replyTo — the defaults were just noise. The in-chat send path also captures (id, pubKey) under the replyTo guard before launching the send coroutine, so a slow send + a user- cleared reply state can't race into a partially-threaded message. No protocol or behavioral change for the warm-cache happy path; the threading improvement is observable only on cold-start push-reply. --- .../vitorpamplona/amethyst/model/Account.kt | 22 ++++++++++ .../EventNotificationConsumer.kt | 1 + .../NotificationReplyReceiver.kt | 36 +++++----------- .../notifications/NotificationUtils.kt | 7 ++++ .../amethyst/ui/navigation/AppNavigation.kt | 1 - .../amethyst/ui/navigation/routes/Routes.kt | 1 - .../ui/screen/loggedIn/AccountViewModel.kt | 41 ++++++++----------- .../marmotGroup/MarmotGroupChatScreen.kt | 2 - .../chats/marmotGroup/MarmotGroupChatView.kt | 19 ++++----- .../amethyst/commons/marmot/MarmotManager.kt | 17 +++++--- 10 files changed, 76 insertions(+), 71 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt index 9cd1d901b2..96eaab07b2 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt @@ -2132,6 +2132,28 @@ class Account( // --- Marmot Group Messaging --- + /** + * Resolve the relay set for a Marmot group. Prefer the relays carried in + * the MLS GroupContext metadata so every member converges on the same + * canonical set; fall back to the account's outbox relays if the group + * has none (e.g. a group joined before MIP-01 metadata existed). + * + * Lives on Account (not AccountViewModel) so that headless callers — + * notifications' BroadcastReceiver, background workers — can resolve + * relays without spinning up a ViewModel. + */ + fun marmotGroupRelays(nostrGroupId: HexKey): Set { + val groupRelays = + marmotManager + ?.groupMetadata(nostrGroupId) + ?.relays + ?.mapNotNull { + com.vitorpamplona.quartz.nip01Core.relay.normalizer.RelayUrlNormalizer + .normalizeOrNull(it) + }?.toSet() + return if (!groupRelays.isNullOrEmpty()) groupRelays else outboxRelays.flow.value + } + /** * Send a message to a Marmot MLS group. * Encrypts the inner event and publishes the GroupEvent to group relays. diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/notifications/EventNotificationConsumer.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/notifications/EventNotificationConsumer.kt index d1e5c15c57..3a3b6bd37a 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/notifications/EventNotificationConsumer.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/notifications/EventNotificationConsumer.kt @@ -588,6 +588,7 @@ class EventNotificationConsumer( chatroomMembers = null, marmotNostrGroupId = nostrGroupId, marmotReplyToInnerEventId = innerEvent.id, + marmotReplyToInnerAuthor = innerEvent.pubKey, ) } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/notifications/NotificationReplyReceiver.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/notifications/NotificationReplyReceiver.kt index 5dc14e635f..cb9b0377c6 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/notifications/NotificationReplyReceiver.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/notifications/NotificationReplyReceiver.kt @@ -29,10 +29,7 @@ import androidx.core.content.ContextCompat import com.vitorpamplona.amethyst.Amethyst import com.vitorpamplona.amethyst.LocalPreferences import com.vitorpamplona.amethyst.model.LocalCache -import com.vitorpamplona.quartz.nip01Core.core.Event import com.vitorpamplona.quartz.nip01Core.hints.EventHintBundle -import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl -import com.vitorpamplona.quartz.nip01Core.relay.normalizer.RelayUrlNormalizer import com.vitorpamplona.quartz.nip01Core.tags.people.PTag import com.vitorpamplona.quartz.nip10Notes.TextNoteEvent import com.vitorpamplona.quartz.nip17Dm.messages.ChatMessageEvent @@ -110,9 +107,10 @@ class NotificationReplyReceiver : BroadcastReceiver() { val accountNpub = intent.getStringExtra(NotificationUtils.KEY_ACCOUNT_NPUB) ?: return val nostrGroupId = intent.getStringExtra(NotificationUtils.KEY_MARMOT_GROUP_ID) ?: return val replyToInnerId = intent.getStringExtra(NotificationUtils.KEY_MARMOT_REPLY_TO_INNER_ID) + val replyToInnerAuthor = intent.getStringExtra(NotificationUtils.KEY_MARMOT_REPLY_TO_INNER_AUTHOR) runOnRelay(notificationManager, notificationId) { - sendMarmotReply(accountNpub, nostrGroupId, replyToInnerId, replyText) + sendMarmotReply(accountNpub, nostrGroupId, replyToInnerId, replyToInnerAuthor, replyText) } } } @@ -165,6 +163,7 @@ class NotificationReplyReceiver : BroadcastReceiver() { accountNpub: String, nostrGroupId: String, replyToInnerEventId: String?, + replyToInnerAuthor: String?, replyText: String, ) { val accountSettings = LocalPreferences.loadAccountConfigFromEncryptedStorage(accountNpub) ?: return @@ -172,35 +171,20 @@ class NotificationReplyReceiver : BroadcastReceiver() { val manager = account.marmotManager ?: return - // Recover the parent inner event so the kind:9 reply carries the - // proper q-tag. Inner events live in LocalCache keyed by the inner - // id; if we somehow miss it (e.g. cache was pruned) the reply still - // goes through unthreaded — better than dropping the user's message. - val replyToInnerEvent: Event? = - replyToInnerEventId?.let { LocalCache.getNoteIfExists(it)?.event } - + // Use id+author from the Intent so the reply is threaded even when + // LocalCache hasn't been rehydrated yet (cold-process broadcast + // receiver: Account.restoreAll runs async on init and may not have + // finished by the time we get here). val bundle = manager.buildTextMessage( nostrGroupId = nostrGroupId, text = replyText, - replyTo = replyToInnerEvent, + replyToEventId = replyToInnerEventId, + replyToAuthorPubKey = replyToInnerAuthor, persistOwn = false, ) - // Mirror AccountViewModel.marmotGroupRelays(): prefer the group's - // configured relays from MLS GroupContext metadata, fall back to - // the account's outbox set so a misconfigured group doesn't silently - // drop the reply. - val groupRelays: Set = - manager - .groupMetadata(nostrGroupId) - ?.relays - ?.mapNotNull { RelayUrlNormalizer.normalizeOrNull(it) } - ?.toSet() - ?.takeIf { it.isNotEmpty() } - ?: account.outboxRelays.flow.value - - account.sendMarmotGroupMessage(nostrGroupId, bundle.innerEvent, groupRelays) + account.sendMarmotGroupMessage(nostrGroupId, bundle.innerEvent, account.marmotGroupRelays(nostrGroupId)) } private suspend fun sendPublicReply( diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/notifications/NotificationUtils.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/notifications/NotificationUtils.kt index c0bef4688a..27b56d05bd 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/notifications/NotificationUtils.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/notifications/NotificationUtils.kt @@ -69,6 +69,7 @@ object NotificationUtils { const val KEY_TARGET_EVENT_ID = "key_target_event_id" const val KEY_MARMOT_GROUP_ID = "key_marmot_group_id" const val KEY_MARMOT_REPLY_TO_INNER_ID = "key_marmot_reply_to_inner_id" + const val KEY_MARMOT_REPLY_TO_INNER_AUTHOR = "key_marmot_reply_to_inner_author" private const val DM_SUMMARY_ID = 0x10000 private const val ZAP_SUMMARY_ID = 0x20000 @@ -379,6 +380,7 @@ object NotificationUtils { chatroomMembers: String? = null, marmotNostrGroupId: String? = null, marmotReplyToInnerEventId: String? = null, + marmotReplyToInnerAuthor: String? = null, ) { getOrCreateDMChannel(applicationContext) val channelId = stringRes(applicationContext, R.string.app_notification_dms_channel_id) @@ -397,6 +399,7 @@ object NotificationUtils { chatroomMembers = chatroomMembers, marmotNostrGroupId = marmotNostrGroupId, marmotReplyToInnerEventId = marmotReplyToInnerEventId, + marmotReplyToInnerAuthor = marmotReplyToInnerAuthor, ) } @@ -434,6 +437,7 @@ object NotificationUtils { chatroomMembers: String?, marmotNostrGroupId: String? = null, marmotReplyToInnerEventId: String? = null, + marmotReplyToInnerAuthor: String? = null, ) { val notId = id.hashCode() @@ -554,6 +558,9 @@ object NotificationUtils { if (marmotReplyToInnerEventId != null) { putExtra(KEY_MARMOT_REPLY_TO_INNER_ID, marmotReplyToInnerEventId) } + if (marmotReplyToInnerAuthor != null) { + putExtra(KEY_MARMOT_REPLY_TO_INNER_AUTHOR, marmotReplyToInnerAuthor) + } } val replyPendingIntent = diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/AppNavigation.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/AppNavigation.kt index ca035bcc65..7ad6288f6b 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/AppNavigation.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/AppNavigation.kt @@ -373,7 +373,6 @@ fun BuildNavigation( nostrGroupId = it.nostrGroupId, draftMessage = it.message, replyToInnerNote = it.replyId, - editFromDraft = it.draftId, accountViewModel = accountViewModel, nav = nav, ) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/routes/Routes.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/routes/Routes.kt index fef37c8762..bc0c08c42b 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/routes/Routes.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/routes/Routes.kt @@ -406,7 +406,6 @@ sealed class Route { val nostrGroupId: String, val message: String? = null, val replyId: HexKey? = null, - val draftId: HexKey? = null, ) : Route() @Serializable data class MarmotGroupInfo( diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountViewModel.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountViewModel.kt index 92d3067bfc..b695b85191 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountViewModel.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountViewModel.kt @@ -1541,16 +1541,23 @@ class AccountViewModel( suspend fun sendMarmotGroupMessage( nostrGroupId: String, text: String, - replyToInnerEvent: Event? = null, + replyToInnerEventId: HexKey? = null, + replyToInnerAuthorPubKey: HexKey? = null, ) { // Inner event construction lives on MarmotManager so CLI and UI don't drift. // persistOwn=false because Account.sendMarmotGroupMessage routes the outer // event through LocalCache which already handles own-message display. val bundle = account.marmotManager - ?.buildTextMessage(nostrGroupId, text, replyTo = replyToInnerEvent, persistOwn = false) + ?.buildTextMessage( + nostrGroupId = nostrGroupId, + text = text, + replyToEventId = replyToInnerEventId, + replyToAuthorPubKey = replyToInnerAuthorPubKey, + persistOwn = false, + ) ?: return - val relays = marmotGroupRelays(nostrGroupId) + val relays = account.marmotGroupRelays(nostrGroupId) account.sendMarmotGroupMessage(nostrGroupId, bundle.innerEvent, relays) } @@ -1580,7 +1587,7 @@ class AccountViewModel( account.signer.pubKey, template, ) - val relays = marmotGroupRelays(nostrGroupId) + val relays = account.marmotGroupRelays(nostrGroupId) account.sendMarmotGroupMessage(nostrGroupId, innerEvent, relays) } @@ -1618,7 +1625,7 @@ class AccountViewModel( } suspend fun leaveMarmotGroup(nostrGroupId: String) { - val relays = marmotGroupRelays(nostrGroupId) + val relays = account.marmotGroupRelays(nostrGroupId) account.leaveMarmotGroup(nostrGroupId, relays) } @@ -1626,22 +1633,6 @@ class AccountViewModel( account.resetMarmotState() } - /** - * Get the relay set for a Marmot group from MLS GroupContext metadata. - * Falls back to outbox relays if the group has no configured relays. - */ - private fun marmotGroupRelays(nostrGroupId: String): Set { - val metadata = account.marmotManager?.groupMetadata(nostrGroupId) - val groupRelays = - metadata - ?.relays - ?.mapNotNull { - com.vitorpamplona.quartz.nip01Core.relay.normalizer.RelayUrlNormalizer - .normalizeOrNull(it) - }?.toSet() - return if (!groupRelays.isNullOrEmpty()) groupRelays else account.outboxRelays.flow.value - } - fun marmotGroupMembers(nostrGroupId: String): List = account.marmotManager?.memberPubkeys(nostrGroupId) ?: emptyList() suspend fun addMarmotGroupMember( @@ -1653,7 +1644,7 @@ class AccountViewModel( nostrGroupId: String, targetLeafIndex: Int, ) { - val relays = marmotGroupRelays(nostrGroupId) + val relays = account.marmotGroupRelays(nostrGroupId) account.removeMarmotGroupMember(nostrGroupId, targetLeafIndex, relays) } @@ -1661,7 +1652,7 @@ class AccountViewModel( nostrGroupId: String, targetPubKey: String, ) { - val relays = marmotGroupRelays(nostrGroupId) + val relays = account.marmotGroupRelays(nostrGroupId) account.grantMarmotGroupAdmin(nostrGroupId, targetPubKey, relays) } @@ -1669,7 +1660,7 @@ class AccountViewModel( nostrGroupId: String, targetPubKey: String, ) { - val relays = marmotGroupRelays(nostrGroupId) + val relays = account.marmotGroupRelays(nostrGroupId) account.revokeMarmotGroupAdmin(nostrGroupId, targetPubKey, relays) } @@ -1701,7 +1692,7 @@ class AccountViewModel( name = name, description = description, ) - val relays = marmotGroupRelays(nostrGroupId) + val relays = account.marmotGroupRelays(nostrGroupId) account.updateMarmotGroupMetadata(nostrGroupId, updatedMetadata, relays) } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/marmotGroup/MarmotGroupChatScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/marmotGroup/MarmotGroupChatScreen.kt index c3da81de90..fe3413d65e 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/marmotGroup/MarmotGroupChatScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/marmotGroup/MarmotGroupChatScreen.kt @@ -53,7 +53,6 @@ fun MarmotGroupChatScreen( nostrGroupId: HexKey, draftMessage: String? = null, replyToInnerNote: HexKey? = null, - editFromDraft: HexKey? = null, accountViewModel: AccountViewModel, nav: INav, ) { @@ -132,7 +131,6 @@ fun MarmotGroupChatScreen( nostrGroupId = nostrGroupId, draftMessage = draftMessage, replyToInnerNote = replyToInnerNote, - editFromDraft = editFromDraft, accountViewModel = accountViewModel, nav = nav, ) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/marmotGroup/MarmotGroupChatView.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/marmotGroup/MarmotGroupChatView.kt index 5777b41605..b5936e8f2c 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/marmotGroup/MarmotGroupChatView.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/marmotGroup/MarmotGroupChatView.kt @@ -81,7 +81,6 @@ fun MarmotGroupChatView( nostrGroupId: HexKey, draftMessage: String? = null, replyToInnerNote: HexKey? = null, - @Suppress("UNUSED_PARAMETER") editFromDraft: HexKey? = null, accountViewModel: AccountViewModel, nav: INav, ) { @@ -128,11 +127,6 @@ fun MarmotGroupChatView( } } - // editFromDraft is accepted for route symmetry with NIP-17's - // Route.Room, but Marmot doesn't yet persist drafts, so it's currently - // unused. Suppressed at the parameter rather than via a fake binding - // so ktlint stays happy. - Column(Modifier.fillMaxHeight()) { Column( modifier = @@ -168,8 +162,8 @@ fun MarmotGroupChatView( @Composable fun MarmotGroupMessageComposer( nostrGroupId: HexKey, - messageState: TextFieldState = remember { TextFieldState() }, - replyTo: MutableState = remember { mutableStateOf(null) }, + messageState: TextFieldState, + replyTo: MutableState, accountViewModel: AccountViewModel, nav: INav, onMessageSent: suspend () -> Unit, @@ -234,13 +228,18 @@ fun MarmotGroupMessageComposer( ) { val text = messageState.text.toString().trim() if (text.isNotEmpty()) { - val replyParent = replyTo.value?.event + // Capture id+pubKey snapshot under the value? guard so + // a slow send doesn't race a user-cleared reply state. + val parentEvent = replyTo.value?.event + val replyId = parentEvent?.id + val replyAuthor = parentEvent?.pubKey scope.launch(Dispatchers.IO) { try { accountViewModel.sendMarmotGroupMessage( nostrGroupId = nostrGroupId, text = text, - replyToInnerEvent = replyParent, + replyToInnerEventId = replyId, + replyToInnerAuthorPubKey = replyAuthor, ) messageState.clearText() replyTo.value = null diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt index cccb5b83f8..b4a0634e1f 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt @@ -184,21 +184,26 @@ class MarmotManager( suspend fun buildTextMessage( nostrGroupId: HexKey, text: String, - replyTo: Event? = null, + replyToEventId: HexKey? = null, + replyToAuthorPubKey: HexKey? = null, persistOwn: Boolean = true, ): TextMessageBundle { val template = com.vitorpamplona.quartz.nip01Core.signers .eventTemplate(kind = 9, description = text) { - if (replyTo != null) { + if (replyToEventId != null) { // Mirror ChatEvent.reply(): NIP-18 q-tag references the - // parent inner kind:9 by id (+ author, no relay hint — - // the inner rumor never hits a relay directly). + // parent inner kind:9 by id (+ optional author, no + // relay hint — the inner rumor never hits a relay + // directly). Taking id+pubKey separately (rather than + // the full parent Event) lets the push-notification + // reply path produce a threaded reply from cold start, + // when LocalCache hasn't been re-hydrated yet. quote( QEventTag( - eventId = replyTo.id, + eventId = replyToEventId, relayHint = null, - authorPubKeyHex = replyTo.pubKey, + authorPubKeyHex = replyToAuthorPubKey, ), ) }