fix(nip29): audit fixes — subgroup edit safety, subscription load, roles

Bugs: - Metadata edit could re-root a subgroup or drop its children on a load race: the edit ViewModel snapshotted parent/children at prefill and overrode the Account-level live-read defaults. Children are no longer snapshotted (Account reads the live child list at save time), and the parent is only overridden when the user actually re-parents (parentTouched) — a plain rename can't re-root or orphan children anymore, even if metadata hadn't loaded yet. - Parent selector card cached a null channel via remember() and never refreshed, so the parent's name/picture never loaded and the warm-up never mounted. Now get-or-create + warm + observe the metadata flow. - previousEventRefs could let a note with an unresolved author slip past the self-exclusion and reference the sender's own event. Now requires a resolved author. - Assigning a relay-defined role replaced the member's whole role set while the menu implied additive; now keeps existing roles (entry.roles + role.name). - GroupNAddrInvite now also accepts a bare `invite=<code>` remainder if the `?` is stripped upstream (+ test). Performance: - Subgroups bar mounted a full warm-up (metadata + content) subscription per child chip — up to ~21 relay subscriptions per open group. Replaced with one relay-directory subscription; chips read from cache. - Parent picker recomputed the whole candidate scan on every recomposition (each search keystroke) via a produceState initial-value argument; the scan now lives only in the producer with a cheap empty initial. spotless clean; quartz tests green; amethyst compiles. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Qst2JsmNYMvXitv2vxo4S
2026-08-12 09:13:23 +00:00 · 2026-07-16 04:19:25 +00:00
parent c280c14b3c
commit 0e8239b22f
7 changed files with 56 additions and 37 deletions
@@ -273,7 +273,9 @@ private fun RelayGroupMemberRow(
                                    },
                                    onClick = {
                                        menuOpen = false
-                                        accountViewModel.putRelayGroupUser(channel, entry.pubkey, listOf(role.name))
+                                        // Additive: NIP-29 allows multiple roles per member and the menu only
+                                        // offers roles they lack, so keep the ones they already hold.
+                                        accountViewModel.putRelayGroupUser(channel, entry.pubkey, entry.roles + role.name)
                                    },
                                )
                            }
@@ -94,11 +94,11 @@ class RelayGroupMetadataViewModel : ViewModel() {
        private set

    /**
-     * Subgroups: the group's current children, carried through untouched on save. NIP-29
-     * requires a kind-9002 to re-list every child or the relay drops them, so we preserve
-     * whatever the relay last advertised rather than letting a metadata edit clear the tree.
+     * True once the user actually picks a parent in this session. Until then we let the save
+     * read the group's live parent rather than the (possibly not-yet-loaded) prefilled value, so
+     * a plain rename can never re-root a subgroup just because its metadata hadn't arrived yet.
     */
-    private var childrenGroupIds: List<String> = emptyList()
+    private var parentTouched by mutableStateOf(false)

    var pickedMedia by mutableStateOf<SelectedMedia?>(null)
        private set
@@ -150,12 +150,12 @@ class RelayGroupMetadataViewModel : ViewModel() {
        // Stored geohashes are mip-mapped into every prefix; the last (longest) is the real one.
        geohash.value = TextFieldValue(event?.geohashes()?.maxByOrNull { it.length } ?: "")
        parentGroupId = channel.parentGroupId()
-        childrenGroupIds = channel.childGroupIds()
    }

    /** Set (or clear, with null) the group's parent from the picker; marks the form touched. */
    fun setParent(groupId: String?) {
        parentGroupId = groupId
+        parentTouched = true
        markTouched()
    }

@@ -258,8 +258,11 @@ class RelayGroupMetadataViewModel : ViewModel() {
                isRestricted = isRestricted,
                hashtags = hashtags,
                geohashes = geohashes,
-                parent = parentGroupId,
-                children = childrenGroupIds,
+                // Only override the parent when the user actually re-parented; otherwise let
+                // Account read the group's live parent so a rename can't accidentally re-root it.
+                // children likewise defaults to the live child list, so a concurrently-added
+                // subgroup isn't dropped by this metadata edit.
+                parent = if (parentTouched) parentGroupId else existing.parentGroupId(),
            )
        }
    }
@@ -60,6 +60,7 @@ import androidx.compose.ui.res.pluralStringResource
 import androidx.compose.ui.text.font.FontWeight
 import androidx.compose.ui.text.style.TextOverflow
 import androidx.compose.ui.unit.dp
+import androidx.lifecycle.compose.collectAsStateWithLifecycle
 import com.vitorpamplona.amethyst.R
 import com.vitorpamplona.amethyst.commons.icons.symbols.Icon
 import com.vitorpamplona.amethyst.commons.icons.symbols.MaterialSymbols
@@ -136,14 +137,18 @@ private fun ParentSelectorCard(
    accountViewModel: AccountViewModel,
    onClick: () -> Unit,
 ) {
-    val parentChannel =
-        remember(parentId, relay) {
-            parentId?.let { accountViewModel.getRelayGroupChannelIfExists(GroupId(it, relay)) }
+    // Resolve the parent (get-or-create so it's never stuck null when the metadata isn't cached
+    // yet), warm its single 39000, and observe it so the name/picture fill in as they arrive.
+    val liveParent: RelayGroupChannel? =
+        parentId?.let { id ->
+            val channel = remember(id, relay) { accountViewModel.checkGetOrCreateRelayGroupChannel(GroupId(id, relay)) }
+            RelayGroupWarmupSubscription(channel, accountViewModel.dataSources().relayGroupWarmup, accountViewModel)
+            val state by channel
+                .flow()
+                .metadata.stateFlow
+                .collectAsStateWithLifecycle()
+            state.channel as? RelayGroupChannel ?: channel
        }
-    // Warm the parent's metadata so its name/picture fills the card while the form is open.
-    parentChannel?.let {
-        RelayGroupWarmupSubscription(it, accountViewModel.dataSources().relayGroupWarmup, accountViewModel)
-    }

    Surface(
        shape = RoundedCornerShape(18.dp),
@@ -160,11 +165,11 @@ private fun ParentSelectorCard(
            horizontalArrangement = Arrangement.spacedBy(14.dp),
        ) {
            GradientBadge {
-                if (parentId != null && parentChannel != null) {
+                if (liveParent != null) {
                    RobohashFallbackAsyncImage(
-                        robot = parentChannel.groupId.id,
-                        model = parentChannel.profilePicture(),
-                        contentDescription = parentChannel.toBestDisplayName(),
+                        robot = liveParent.groupId.id,
+                        model = liveParent.profilePicture(),
+                        contentDescription = liveParent.toBestDisplayName(),
                        modifier = Modifier.size(46.dp).clip(CircleShape),
                        loadProfilePicture = accountViewModel.settings.showProfilePictures(),
                        loadRobohash = accountViewModel.settings.isNotPerformanceMode(),
@@ -172,7 +177,7 @@ private fun ParentSelectorCard(
                    )
                } else {
                    Icon(
-                        symbol = if (parentId == null) MaterialSymbols.Home else MaterialSymbols.Group,
+                        symbol = MaterialSymbols.Home,
                        contentDescription = null,
                        tint = MaterialTheme.colorScheme.onPrimary,
                        modifier = Modifier.size(24.dp),
@@ -187,12 +192,7 @@ private fun ParentSelectorCard(
                    color = MaterialTheme.colorScheme.onSurfaceVariant,
                )
                Text(
-                    text =
-                        when {
-                            parentId == null -> stringRes(R.string.relay_group_parent_none)
-                            parentChannel != null -> parentChannel.toBestDisplayName()
-                            else -> parentId
-                        },
+                    text = liveParent?.toBestDisplayName() ?: stringRes(R.string.relay_group_parent_none),
                    style = MaterialTheme.typography.titleMedium,
                    fontWeight = FontWeight.SemiBold,
                    maxLines = 1,
@@ -255,9 +255,11 @@ private fun ParentGroupPickerSheet(
            descendantIdsOf(accountViewModel, selfGroupId, relay) + selfGroupId
        }

-    // Re-read the relay's genuine, relay-signed groups whenever a kind-39000 lands.
+    // Re-read the relay's genuine, relay-signed groups whenever a kind-39000 lands. The initial
+    // value is empty (cheap) rather than an eager scan — a produceState initial arg is evaluated
+    // on every recomposition (e.g. each search keystroke), so the scan lives only in the producer.
    val candidates by produceState(
-        initialValue = pickCandidates(accountViewModel, relay, relayInfo, forbidden),
+        initialValue = emptyList<RelayGroupChannel>(),
        relay,
        relayInfo,
        forbidden,
@@ -49,7 +49,7 @@ import com.vitorpamplona.amethyst.commons.model.nip29RelayGroups.RelayGroupChann
 import com.vitorpamplona.amethyst.ui.navigation.navs.INav
 import com.vitorpamplona.amethyst.ui.navigation.routes.Route
 import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel
-import com.vitorpamplona.amethyst.ui.screen.loggedIn.chats.publicChannels.relayGroup.datasource.RelayGroupWarmupSubscription
+import com.vitorpamplona.amethyst.ui.screen.loggedIn.chats.publicChannels.relayGroup.datasource.RelayGroupsOnRelaySubscription
 import com.vitorpamplona.amethyst.ui.theme.DividerThickness
 import com.vitorpamplona.quartz.nip29RelayGroups.GroupId

@@ -75,6 +75,11 @@ fun RelayGroupSubgroupsBar(

    val relay = channel.groupId.relayUrl

+    // Load the parent's + every child's kind-39000 (for their names) with ONE relay-directory
+    // subscription rather than a warm-up per chip — a single REQ that streams metadata for all
+    // of the relay's groups, versus up to N subscriptions that would each also pull content.
+    RelayGroupsOnRelaySubscription(relay, accountViewModel.dataSources().relayGroupsOnRelay, accountViewModel)
+
    Surface(
        color = MaterialTheme.colorScheme.surfaceColorAtElevation(2.dp),
        modifier = Modifier.fillMaxWidth(),
@@ -120,8 +125,8 @@ fun RelayGroupSubgroupsBar(
 }

 /**
- * A single tappable group chip. Resolves the group by id on the shared host relay and warms
- * its metadata so the name fills in, then navigates to that group when tapped.
+ * A single tappable group chip. Resolves the group by id on the shared host relay (its metadata
+ * is streamed by the bar's one directory subscription) and navigates to that group when tapped.
 */
@Composable
 private fun SubgroupChip(
@@ -131,9 +136,6 @@ private fun SubgroupChip(
    nav: INav,
 ) {
    LoadRelayGroupChannel(groupId, accountViewModel) { child ->
-        // Fetch the child's 39000 (name/picture) while this bar is visible.
-        RelayGroupWarmupSubscription(child, accountViewModel.dataSources().relayGroupWarmup, accountViewModel)
-
        val childState by child
            .flow()
            .metadata.stateFlow
@@ -225,9 +225,11 @@ class RelayGroupChannel(
        notes
            .mapNotNull { _, note ->
                val createdAt = note.createdAt()
-                if (createdAt != null && note.author?.pubkeyHex != selfPubkey) note to createdAt else null
+                val author = note.author
+                // Require a resolved author so an unlinked note can't slip past the self-exclusion
+                // and make us reference our own event (the very thing `previous` guards against).
+                if (createdAt != null && author != null && author.pubkeyHex != selfPubkey) note to createdAt else null
            }.sortedByDescending { it.second }
-            .take(50)
            .take(max)
            .map { it.first.idHex.take(8) }

@@ -43,7 +43,9 @@ object GroupNAddrInvite {
     */
    fun parse(suffix: String?): String? {
        if (suffix.isNullOrEmpty()) return null
-        val query = suffix.substringAfter('?', "")
+        // Normally the code trails the naddr as `?invite=<code>`. Fall back to a bare
+        // `invite=<code>` remainder too, in case an upstream parser strips the `?`.
+        val query = if ('?' in suffix) suffix.substringAfter('?') else suffix
        if (query.isEmpty()) return null

        return query
@@ -54,6 +54,12 @@ class GroupNAddrInviteTest {
        assertNull(GroupNAddrInvite.parse("?invite="))
    }

+    @Test
+    fun acceptsBareInviteWithoutQuestionMark() {
+        // Defensive: if an upstream parser drops the `?`, a bare `invite=<code>` still resolves.
+        assertEquals("abc123", GroupNAddrInvite.parse("invite=abc123"))
+    }
+
    @Test
    fun ignoresLeadingBech32RemainderWithoutQuery() {
        // A plain trailing word (not a query) carries no invite.