fix(ui): pass FeedDefinition through FeedFilterSpinner.onSelect

The dialog used to hand the caller an integer index into the latest options list, but the indexes were captured from a snapshot taken at remember time. If options changed (a new community/list arrived) between dialog open and tap, the user could pick "Community A" and have an unrelated entry selected. Pass the resolved FeedDefinition directly so the picked item can never drift. Other audit fixes folded into the same composable: - Match the placeholder by both subclass and code string so TopFilter variants that share an Address-derived code (PeopleList vs MuteList) no longer collide. - Drop the local mutableStateOf for `selected` and the derivedStateOf-in- remember for `currentText` — both were redundant with the StateFlow round-trip and caused an extra recomposition per pick. - De-duplicate RenderOption with Name.name(context) (also fixes the accessibility text disagreeing with the visible label for Geohash). - Pre-compute the ordered (group, items) list once per options change. - Drop IndexedFeedDefinition (no longer needed), use Spacer.width instead of a Spacer with start padding.
2026-04-26 18:42:12 +00:00
parent 49f769f92a
commit 9a0eee3414
18 changed files with 101 additions and 156 deletions
@@ -34,15 +34,14 @@ import androidx.compose.foundation.layout.Spacer
 import androidx.compose.foundation.layout.fillMaxWidth
 import androidx.compose.foundation.layout.height
 import androidx.compose.foundation.layout.padding
+import androidx.compose.foundation.layout.width
 import androidx.compose.foundation.lazy.LazyColumn
 import androidx.compose.foundation.shape.RoundedCornerShape
 import androidx.compose.material3.MaterialTheme
 import androidx.compose.material3.Surface
 import androidx.compose.material3.Text
 import androidx.compose.runtime.Composable
-import androidx.compose.runtime.Immutable
 import androidx.compose.runtime.LaunchedEffect
-import androidx.compose.runtime.derivedStateOf
 import androidx.compose.runtime.getValue
 import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.remember
@@ -69,7 +68,6 @@ import com.vitorpamplona.amethyst.Amethyst
 import com.vitorpamplona.amethyst.R
 import com.vitorpamplona.amethyst.commons.icons.symbols.Icon
 import com.vitorpamplona.amethyst.commons.icons.symbols.MaterialSymbols
-import com.vitorpamplona.amethyst.model.AddressableNote
 import com.vitorpamplona.amethyst.model.TopFilter
 import com.vitorpamplona.amethyst.service.location.LocationState
 import com.vitorpamplona.amethyst.service.relayClient.reqCommand.event.observeNote
@@ -92,10 +90,6 @@ import com.vitorpamplona.amethyst.ui.theme.Font14SP
 import com.vitorpamplona.amethyst.ui.theme.Size20Modifier
 import com.vitorpamplona.amethyst.ui.theme.StdHorzSpacer
 import com.vitorpamplona.amethyst.ui.theme.placeholderText
-import com.vitorpamplona.quartz.nip51Lists.followList.FollowListEvent
-import com.vitorpamplona.quartz.nip51Lists.peopleList.PeopleListEvent
-import com.vitorpamplona.quartz.nip72ModCommunities.definition.CommunityDefinitionEvent
-import com.vitorpamplona.quartz.nip89AppHandlers.definition.AppDefinitionEvent
 import kotlinx.collections.immutable.ImmutableList

@OptIn(ExperimentalPermissionsApi::class)
@@ -104,32 +98,27 @@ fun FeedFilterSpinner(
    placeholderCode: TopFilter,
    explainer: String,
    options: ImmutableList<FeedDefinition>,
-    onSelect: (Int) -> Unit,
+    onSelect: (FeedDefinition) -> Unit,
    modifier: Modifier = Modifier,
    accountViewModel: AccountViewModel,
 ) {
    var optionsShowing by remember { mutableStateOf(false) }

    val context = LocalContext.current
-    val selectAnOption =
-        stringRes(
-            id = R.string.select_an_option,
-        )
+    val selectAnOption = stringRes(id = R.string.select_an_option)

-    var selected by
+    val selected =
        remember(placeholderCode, options) {
-            mutableStateOf(
-                options.firstOrNull { it.code.code == placeholderCode.code },
-            )
-        }
-
-    val currentText by
-        remember(placeholderCode, options) {
-            derivedStateOf {
-                selected?.name?.name(context) ?: selectAnOption
+            // Match by both subclass and code string to avoid collisions between
+            // TopFilter variants that derive `code` from the same Address (e.g.
+            // PeopleList vs MuteList).
+            options.firstOrNull {
+                it.code::class == placeholderCode::class && it.code.code == placeholderCode.code
            }
        }

+    val currentText = selected?.name?.name(context) ?: selectAnOption
+
    val accessibilityDescription =
        if (selected != null) {
            stringRes(R.string.feed_filter_selected, currentText)
@@ -282,10 +271,9 @@ fun FeedFilterSpinner(
            title = explainer,
            options = options,
            onDismiss = { optionsShowing = false },
-            onSelect = {
-                selected = options[it]
+            onSelect = { definition ->
                optionsShowing = false
-                onSelect(it)
+                onSelect(definition)
            },
        ) {
            RenderOption(it.name, accountViewModel)
@@ -298,6 +286,7 @@ fun RenderOption(
    option: Name,
    accountViewModel: AccountViewModel,
 ) {
+    val context = LocalContext.current
    when (option) {
        is GeoHashName -> {
            LoadCityName(option.geoHashTag) {
@@ -305,74 +294,35 @@ fun RenderOption(
            }
        }

-        is HashtagName -> {
-            Text(text = option.name(), fontSize = Font14SP, color = MaterialTheme.colorScheme.onSurface)
-        }
-
-        is ResourceName -> {
-            Text(
-                text = stringRes(id = option.resourceId),
-                fontSize = Font14SP,
-                color = MaterialTheme.colorScheme.onSurface,
-            )
-        }
-
+        // Note-backed names: subscribe to the note so the displayed title updates as
+        // the corresponding event arrives from relays. The displayed string itself is
+        // produced by Name.name(), which already has the right precedence rules.
        is PeopleListName -> {
            val noteState by observeNote(option.note, accountViewModel)
-
-            val noteEvent = noteState.note.event
-            val name =
-                when (noteEvent) {
-                    is PeopleListEvent -> {
-                        noteEvent.titleOrName() ?: option.note.dTag()
-                    }
-
-                    is FollowListEvent -> {
-                        noteEvent.title() ?: option.note.dTag()
-                    }
-
-                    else -> {
-                        option.note.dTag()
-                    }
-                }
-
+            val name = remember(noteState) { option.name(context) }
            Text(text = name, fontSize = Font14SP, color = MaterialTheme.colorScheme.onSurface)
        }

        is CommunityName -> {
-            val it by observeNote(option.note, accountViewModel)
-
-            val addressable = it.note as? AddressableNote
-            val definition = addressable?.event as? CommunityDefinitionEvent
-            val label = definition?.name()?.ifBlank { null } ?: addressable?.dTag() ?: ""
-            Text(text = "/n/$label", fontSize = Font14SP, color = MaterialTheme.colorScheme.onSurface)
-        }
-
-        is RelayName -> {
-            Text(
-                text = option.name(),
-                fontSize = Font14SP,
-                color = MaterialTheme.colorScheme.onSurface,
-            )
+            val noteState by observeNote(option.note, accountViewModel)
+            val name = remember(noteState) { option.name(context) }
+            Text(text = name, fontSize = Font14SP, color = MaterialTheme.colorScheme.onSurface)
        }

        is FavoriteAlgoFeedName -> {
            val noteState by observeNote(option.note, accountViewModel)
-            val name =
-                (noteState.note.event as? AppDefinitionEvent)
-                    ?.appMetaData()
-                    ?.name
-                    ?.takeIf { it.isNotBlank() } ?: option.note.dTag()
-            Text(
-                text = name,
-                fontSize = Font14SP,
-                color = MaterialTheme.colorScheme.onSurface,
-            )
+            val name = remember(noteState) { option.name(context) }
+            Text(text = name, fontSize = Font14SP, color = MaterialTheme.colorScheme.onSurface)
        }

-        is InterestSetName -> {
+        // Pure names: no relay subscription needed.
+        is HashtagName,
+        is ResourceName,
+        is RelayName,
+        is InterestSetName,
+        -> {
            Text(
-                text = option.name(),
+                text = option.name(context),
                fontSize = Font14SP,
                color = MaterialTheme.colorScheme.onSurface,
            )
@@ -380,12 +330,6 @@ fun RenderOption(
    }
 }

-@Immutable
-private data class IndexedFeedDefinition(
-    val originalIndex: Int,
-    val item: FeedDefinition,
-)
-
 private enum class FeedGroup(
    @param:androidx.annotation.StringRes val labelRes: Int,
 ) {
@@ -399,48 +343,51 @@ private enum class FeedGroup(
    RELAYS(R.string.feed_group_relays),
 }

-private fun groupFeedDefinitions(options: ImmutableList<FeedDefinition>): Map<FeedGroup, List<IndexedFeedDefinition>> {
-    val indexed = options.mapIndexed { index, item -> IndexedFeedDefinition(index, item) }
-    return indexed.groupBy { entry ->
-        when (entry.item.name) {
-            is HashtagName -> {
-                FeedGroup.HASHTAGS
-            }
+private fun FeedDefinition.group(): FeedGroup =
+    when (name) {
+        is HashtagName -> {
+            FeedGroup.HASHTAGS
+        }

-            is CommunityName -> {
-                FeedGroup.COMMUNITIES
-            }
+        is CommunityName -> {
+            FeedGroup.COMMUNITIES
+        }

-            is PeopleListName -> {
-                FeedGroup.LISTS
-            }
+        is PeopleListName -> {
+            FeedGroup.LISTS
+        }

-            is RelayName -> {
-                FeedGroup.RELAYS
-            }
+        is RelayName -> {
+            FeedGroup.RELAYS
+        }

-            is GeoHashName -> {
-                FeedGroup.LOCATIONS
-            }
+        is GeoHashName -> {
+            FeedGroup.LOCATIONS
+        }

-            is FavoriteAlgoFeedName -> {
-                FeedGroup.DVMS
-            }
+        is FavoriteAlgoFeedName -> {
+            FeedGroup.DVMS
+        }

-            is InterestSetName -> {
-                FeedGroup.INTEREST_SETS
-            }
+        is InterestSetName -> {
+            FeedGroup.INTEREST_SETS
+        }

-            is ResourceName -> {
-                when (entry.item.code) {
-                    is TopFilter.AroundMe -> FeedGroup.LOCATIONS
-                    is TopFilter.Global -> FeedGroup.RELAYS
-                    is TopFilter.AllFavoriteAlgoFeeds -> FeedGroup.DVMS
-                    else -> FeedGroup.FEEDS
-                }
+        is ResourceName -> {
+            when (code) {
+                is TopFilter.AroundMe -> FeedGroup.LOCATIONS
+                is TopFilter.Global -> FeedGroup.RELAYS
+                is TopFilter.AllFavoriteAlgoFeeds -> FeedGroup.DVMS
+                else -> FeedGroup.FEEDS
            }
        }
    }
+
+private fun groupFeedDefinitions(options: ImmutableList<FeedDefinition>): List<Pair<FeedGroup, List<FeedDefinition>>> {
+    val grouped = options.groupBy { it.group() }
+    return FeedGroup.entries.mapNotNull { group ->
+        grouped[group]?.takeIf { it.isNotEmpty() }?.let { group to it }
+    }
 }

@OptIn(ExperimentalLayoutApi::class)
@@ -448,7 +395,7 @@ private fun groupFeedDefinitions(options: ImmutableList<FeedDefinition>): Map<Fe
 private fun GroupedFeedFilterDialog(
    title: String,
    options: ImmutableList<FeedDefinition>,
-    onSelect: (Int) -> Unit,
+    onSelect: (FeedDefinition) -> Unit,
    onDismiss: () -> Unit,
    onRenderItem: @Composable (FeedDefinition) -> Unit,
 ) {
@@ -472,18 +419,15 @@ private fun GroupedFeedFilterDialog(
                    )
                }

-                FeedGroup.entries.forEach { group ->
-                    val items = grouped[group]
-                    if (!items.isNullOrEmpty()) {
-                        item {
-                            GroupSection(
-                                label = stringRes(group.labelRes),
-                                items = items,
-                                isChipLayout = group == FeedGroup.HASHTAGS,
-                                onSelect = onSelect,
-                                onRenderItem = onRenderItem,
-                            )
-                        }
+                grouped.forEach { (group, items) ->
+                    item(key = group) {
+                        GroupSection(
+                            label = stringRes(group.labelRes),
+                            items = items,
+                            isChipLayout = group == FeedGroup.HASHTAGS,
+                            onSelect = onSelect,
+                            onRenderItem = onRenderItem,
+                        )
                    }
                }
            }
@@ -495,11 +439,12 @@ private fun GroupedFeedFilterDialog(
@Composable
 private fun GroupSection(
    label: String,
-    items: List<IndexedFeedDefinition>,
+    items: List<FeedDefinition>,
    isChipLayout: Boolean,
-    onSelect: (Int) -> Unit,
+    onSelect: (FeedDefinition) -> Unit,
    onRenderItem: @Composable (FeedDefinition) -> Unit,
 ) {
+    val context = LocalContext.current
    Surface(
        modifier = Modifier.padding(horizontal = 12.dp, vertical = 4.dp),
        shape = RoundedCornerShape(16.dp),
@@ -523,13 +468,13 @@ private fun GroupSection(
                ) {
                    items.forEach { entry ->
                        Surface(
-                            modifier = Modifier.clickable { onSelect(entry.originalIndex) },
+                            modifier = Modifier.clickable { onSelect(entry) },
                            shape = RoundedCornerShape(18.dp),
                            border = BorderStroke(1.dp, MaterialTheme.colorScheme.outline),
                            color = Color.Transparent,
                        ) {
                            Text(
-                                text = entry.item.name.name(),
+                                text = entry.name.name(context),
                                fontSize = 13.sp,
                                color = MaterialTheme.colorScheme.onSurface,
                                modifier = Modifier.padding(horizontal = 14.dp, vertical = 7.dp),
@@ -545,15 +490,15 @@ private fun GroupSection(
                        modifier =
                            Modifier
                                .fillMaxWidth()
-                                .clickable { onSelect(entry.originalIndex) }
+                                .clickable { onSelect(entry) }
                                .padding(horizontal = 16.dp, vertical = 6.dp),
                    ) {
                        FeedIcon(
-                            item = entry.item,
+                            item = entry,
                            modifier = Size20Modifier,
                        )
-                        Spacer(modifier = Modifier.padding(start = 12.dp))
-                        Column(modifier = Modifier.weight(1f)) { onRenderItem(entry.item) }
+                        Spacer(modifier = Modifier.width(12.dp))
+                        Column(modifier = Modifier.weight(1f)) { onRenderItem(entry) }
                    }
                }
            }
@@ -64,7 +64,7 @@ private fun ArticlesTopNavFilterBar(
        placeholderCode = listName,
        explainer = stringRes(R.string.select_list_to_filter),
        options = allLists,
-        onSelect = { onChange(allLists.getOrNull(it) ?: followListsModel.allFollows) },
+        onSelect = onChange,
        accountViewModel = accountViewModel,
    )
 }
@@ -64,7 +64,7 @@ private fun AudioRoomsTopNavFilterBar(
        placeholderCode = listName,
        explainer = stringRes(R.string.select_list_to_filter),
        options = allLists,
-        onSelect = { onChange(allLists.getOrNull(it) ?: followListsModel.allFollows) },
+        onSelect = onChange,
        accountViewModel = accountViewModel,
    )
 }
@@ -64,7 +64,7 @@ private fun BadgesTopNavFilterBar(
        placeholderCode = listName,
        explainer = stringRes(R.string.select_list_to_filter),
        options = allLists,
-        onSelect = { onChange(allLists.getOrNull(it) ?: followListsModel.allFollows) },
+        onSelect = onChange,
        accountViewModel = accountViewModel,
    )
 }
@@ -64,7 +64,7 @@ private fun CommunitiesTopNavFilterBar(
        placeholderCode = listName,
        explainer = stringRes(R.string.select_list_to_filter),
        options = allLists,
-        onSelect = { onChange(allLists.getOrNull(it) ?: followListsModel.allFollows) },
+        onSelect = onChange,
        accountViewModel = accountViewModel,
    )
 }
@@ -64,7 +64,7 @@ private fun TopNavFilterBar(
        placeholderCode = listName,
        explainer = stringRes(R.string.select_list_to_filter),
        options = allLists,
-        onSelect = { onChange(allLists.getOrNull(it) ?: followListsModel.allFollows) },
+        onSelect = onChange,
        accountViewModel = accountViewModel,
    )
 }
@@ -64,7 +64,7 @@ private fun BrowseEmojiSetsTopNavFilterBar(
        placeholderCode = listName,
        explainer = stringRes(R.string.select_list_to_filter),
        options = allLists,
-        onSelect = { onChange(allLists.getOrNull(it) ?: followListsModel.allFollows) },
+        onSelect = onChange,
        accountViewModel = accountViewModel,
    )
 }
@@ -64,7 +64,7 @@ private fun FollowPacksTopNavFilterBar(
        placeholderCode = listName,
        explainer = stringRes(R.string.select_list_to_filter),
        options = allLists,
-        onSelect = { onChange(allLists.getOrNull(it) ?: followListsModel.allFollows) },
+        onSelect = onChange,
        accountViewModel = accountViewModel,
    )
 }
@@ -69,7 +69,7 @@ private fun TopNavFilterBar(
        placeholderCode = listName,
        explainer = stringRes(R.string.select_list_to_filter),
        options = allLists,
-        onSelect = { onChange(allLists.getOrNull(it) ?: followListsModel.allFollows) },
+        onSelect = onChange,
        accountViewModel = accountViewModel,
    )
 }
@@ -64,7 +64,7 @@ private fun LiveStreamsTopNavFilterBar(
        placeholderCode = listName,
        explainer = stringRes(R.string.select_list_to_filter),
        options = allLists,
-        onSelect = { onChange(allLists.getOrNull(it) ?: followListsModel.allFollows) },
+        onSelect = onChange,
        accountViewModel = accountViewModel,
    )
 }
@@ -64,7 +64,7 @@ private fun LongsTopNavFilterBar(
        placeholderCode = listName,
        explainer = stringRes(R.string.select_list_to_filter),
        options = allLists,
-        onSelect = { onChange(allLists.getOrNull(it) ?: followListsModel.allFollows) },
+        onSelect = onChange,
        accountViewModel = accountViewModel,
    )
 }
@@ -64,7 +64,7 @@ private fun TopNavFilterBar(
        placeholderCode = listName,
        explainer = stringRes(R.string.select_list_to_filter),
        options = allLists,
-        onSelect = { onChange(allLists.getOrNull(it) ?: followListsModel.allFollows) },
+        onSelect = onChange,
        accountViewModel = accountViewModel,
    )
 }
@@ -64,7 +64,7 @@ private fun PicturesTopNavFilterBar(
        placeholderCode = listName,
        explainer = stringRes(R.string.select_list_to_filter),
        options = allLists,
-        onSelect = { onChange(allLists.getOrNull(it) ?: followListsModel.allFollows) },
+        onSelect = onChange,
        accountViewModel = accountViewModel,
    )
 }
@@ -64,7 +64,7 @@ private fun PollsTopNavFilterBar(
        placeholderCode = listName,
        explainer = stringRes(R.string.select_list_to_filter),
        options = allLists,
-        onSelect = { onChange(allLists.getOrNull(it) ?: followListsModel.allFollows) },
+        onSelect = onChange,
        accountViewModel = accountViewModel,
    )
 }
@@ -64,7 +64,7 @@ private fun ProductsTopNavFilterBar(
        placeholderCode = listName,
        explainer = stringRes(R.string.select_list_to_filter),
        options = allLists,
-        onSelect = { onChange(allLists.getOrNull(it) ?: followListsModel.allFollows) },
+        onSelect = onChange,
        accountViewModel = accountViewModel,
    )
 }
@@ -64,7 +64,7 @@ private fun PublicChatsTopNavFilterBar(
        placeholderCode = listName,
        explainer = stringRes(R.string.select_list_to_filter),
        options = allLists,
-        onSelect = { onChange(allLists.getOrNull(it) ?: followListsModel.allFollows) },
+        onSelect = onChange,
        accountViewModel = accountViewModel,
    )
 }
@@ -64,7 +64,7 @@ private fun ShortsTopNavFilterBar(
        placeholderCode = listName,
        explainer = stringRes(R.string.select_list_to_filter),
        options = allLists,
-        onSelect = { onChange(allLists.getOrNull(it) ?: followListsModel.allFollows) },
+        onSelect = onChange,
        accountViewModel = accountViewModel,
    )
 }
@@ -65,7 +65,7 @@ private fun TopNavFilterBar(
        placeholderCode = listName,
        explainer = stringRes(R.string.select_list_to_filter),
        options = allLists,
-        onSelect = { onChange(allLists.getOrNull(it) ?: followListsModel.allFollows) },
+        onSelect = onChange,
        accountViewModel = accountViewModel,
    )
 }