laantungir/amethyst
1
0
Fork 0
mirror of https://github.com/vitorpamplona/amethyst.git synced 2026-08-08 23:54:39 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(marmot): audit follow-ups on MLS reply paths

Browse Source 
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.
This commit is contained in:
Claude
2026-05-19 22:04:14 +00:00
parent 2326738b84
commit c7a4796b25
10 changed files with 76 additions and 71 deletions
Download Patch File Download Diff File Expand all files Collapse all files
amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt
+22
View File
@@ -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<NormalizedRelayUrl> {
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.
amethyst/src/main/java/com/vitorpamplona/amethyst/service/notifications/EventNotificationConsumer.kt
+1
View File
@@ -588,6 +588,7 @@ class EventNotificationConsumer(
chatroomMembers = null,
marmotNostrGroupId = nostrGroupId,
marmotReplyToInnerEventId = innerEvent.id,
marmotReplyToInnerAuthor = innerEvent.pubKey,
)
}
amethyst/src/main/java/com/vitorpamplona/amethyst/service/notifications/NotificationReplyReceiver.kt
+10 -26
View File
@@ -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<NormalizedRelayUrl> =
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(
amethyst/src/main/java/com/vitorpamplona/amethyst/service/notifications/NotificationUtils.kt
+7
View File
@@ -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 =
amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/AppNavigation.kt
-1
View File
@@ -373,7 +373,6 @@ fun BuildNavigation(
nostrGroupId = it.nostrGroupId,
draftMessage = it.message,
replyToInnerNote = it.replyId,
editFromDraft = it.draftId,
accountViewModel = accountViewModel,
nav = nav,
)
amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/routes/Routes.kt
-1
View File
@@ -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(
amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountViewModel.kt
+16 -25
View File
@@ -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<NormalizedRelayUrl> {
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<com.vitorpamplona.amethyst.commons.marmot.GroupMemberInfo> = 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)
}
amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/marmotGroup/MarmotGroupChatScreen.kt
-2
View File
@@ -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,
)
amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/marmotGroup/MarmotGroupChatView.kt
+9 -10
View File
@@ -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<Note?> = remember { mutableStateOf(null) },
messageState: TextFieldState,
replyTo: MutableState<Note?>,
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
commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt
+11 -6
View File
@@ -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<Event>(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,
),
)
}