mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-08-10 08:27:04 +00:00
fix(relays): count filters once per filter, not once per entity it names
The Active Subscriptions screen reported two different things as "filters".
The header counted filters; a purpose card summed its per-entity rows. Those
only agree when every filter names exactly one entity — and the busiest ones
name many, because batching is the whole point of them: one `#e` filter per
relay carrying every followed chat, one `#d` filter per host relay carrying
every joined group.
So six chats on six relays read as 144 filters where 24 were on the wire, and
"8% of all" divided the inflated number by the real one. The screen exists to
answer "why do I have this many subscriptions", and it was overstating its
loudest purposes by exactly their batching factor — the opposite of the job.
A purpose now tallies each filter once as it is scanned, before the fan-out.
The per-entity rows stay, because "which chats is this relay serving" is worth
seeing, but they are a breakdown rather than a total: the field is renamed to
namedInFilters and says in its docs that it is not summable.
The aggregation moves out of the ViewModel into a pure aggregateSubscriptions()
so the invariant is testable without a relay pool. AggregateSubscriptionsTest
pins it on the reported shape, and was mutation-checked: restoring the old
`entityRows.sumOf { … }` fails three of its four cases, the fourth being the
relay count, which never depended on it.
On device, Public Chat goes from 144 filters over 6 relays to 18 over the same
6, and Relay Groups from 116 to 96. No filter changed; only the arithmetic.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
8e019a2d59
commit
f2895b2d29
+129
-80
@@ -28,6 +28,7 @@ import com.vitorpamplona.amethyst.commons.model.topNavFeeds.IFeedTopNavPerRelayF
|
||||
import com.vitorpamplona.amethyst.commons.relayClient.subscriptions.ExplainedFilter
|
||||
import com.vitorpamplona.amethyst.commons.relayClient.subscriptions.SubPurpose
|
||||
import com.vitorpamplona.quartz.nip01Core.core.HexKey
|
||||
import com.vitorpamplona.quartz.nip01Core.relay.filters.Filter
|
||||
import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl
|
||||
import kotlinx.coroutines.Dispatchers
|
||||
import kotlinx.coroutines.flow.MutableStateFlow
|
||||
@@ -64,7 +65,15 @@ data class SubscriptionEntityRow(
|
||||
val scope: IFeedTopNavPerRelayFilter?,
|
||||
val detail: String?,
|
||||
val relays: List<NormalizedRelayUrl>,
|
||||
val filterCount: Int,
|
||||
/**
|
||||
* How many in-flight filters name this entity.
|
||||
*
|
||||
* **Not summable across entities.** One batched filter names every chat its relay serves, so it
|
||||
* counts once for each of them — summing these to get a purpose's total is how "6 chats on 6
|
||||
* relays" once read as 144 filters when 24 were on the wire. [SubscriptionPurposeRow.filterCount]
|
||||
* counts filters, and is the only number that belongs next to a total.
|
||||
*/
|
||||
val namedInFilters: Int,
|
||||
)
|
||||
|
||||
/**
|
||||
@@ -83,11 +92,25 @@ private data class EntityKey(
|
||||
@Immutable
|
||||
data class SubscriptionPurposeRow(
|
||||
val purpose: SubPurpose,
|
||||
/** Filters actually in flight for this purpose — the same unit as [ActiveSubscriptionsState.totalFilters]. */
|
||||
val filterCount: Int,
|
||||
val relays: List<NormalizedRelayUrl>,
|
||||
val entities: List<SubscriptionEntityRow>,
|
||||
)
|
||||
|
||||
/**
|
||||
* One purpose's tally for one account, accumulated in a single pass.
|
||||
*
|
||||
* [filters] is incremented once per filter; [entities] records the same filter against every entity
|
||||
* it names. Keeping both means the card can report a real filter count while still breaking down
|
||||
* which chats or communities that filter is for.
|
||||
*/
|
||||
private class PurposeTally {
|
||||
var filters = 0
|
||||
val relays = mutableSetOf<NormalizedRelayUrl>()
|
||||
val entities = mutableMapOf<EntityKey, MutableList<NormalizedRelayUrl>>()
|
||||
}
|
||||
|
||||
@Immutable
|
||||
data class SubscriptionAccountRow(
|
||||
/** Null groups everything not yet attributed to an account. Shown last, never hidden. */
|
||||
@@ -126,88 +149,114 @@ class ActiveSubscriptionsViewModel : ViewModel() {
|
||||
private suspend fun snapshot(): ActiveSubscriptionsState =
|
||||
withContext(Dispatchers.Default) {
|
||||
val client = Amethyst.instance.client
|
||||
|
||||
// account -> purpose -> entity -> relays / count
|
||||
val byAccount = mutableMapOf<HexKey?, MutableMap<SubPurpose, MutableMap<EntityKey, MutableList<NormalizedRelayUrl>>>>()
|
||||
val detailOf = mutableMapOf<Pair<SubPurpose, EntityKey>, String?>()
|
||||
val scopeOf = mutableMapOf<Pair<SubPurpose, EntityKey>, IFeedTopNavPerRelayFilter>()
|
||||
var total = 0
|
||||
var untagged = 0
|
||||
val allRelays = mutableSetOf<NormalizedRelayUrl>()
|
||||
|
||||
client.connectedRelaysFlow().value.forEach { relay ->
|
||||
client.activeRequests(relay).values.flatten().forEach { filter ->
|
||||
total++
|
||||
val explained = filter as? ExplainedFilter
|
||||
if (explained == null) {
|
||||
untagged++
|
||||
return@forEach
|
||||
}
|
||||
allRelays.add(relay)
|
||||
// Discovery filters name no entity — they go looking for things rather than
|
||||
// serving known ones — but they do carry the selection they search within, which
|
||||
// is what keeps them from collapsing into one nameless row per purpose.
|
||||
val scopeKey = explained.scope?.let { it::class.simpleName }
|
||||
// A batched filter serves several entities at once — relay-group state is one #d
|
||||
// filter per host relay carrying every joined group on it — so it contributes a
|
||||
// row to each of them rather than collapsing to "All".
|
||||
val entities: List<HexKey?> = explained.entityIds?.takeIf { it.isNotEmpty() } ?: listOf(null)
|
||||
entities.forEach { entityId ->
|
||||
val key = EntityKey(entityId, scopeKey)
|
||||
byAccount
|
||||
.getOrPut(explained.accountPubKey) { mutableMapOf() }
|
||||
.getOrPut(explained.purpose) { mutableMapOf() }
|
||||
.getOrPut(key) { mutableListOf() }
|
||||
.add(relay)
|
||||
detailOf[explained.purpose to key] = explained.purposeDetail
|
||||
explained.scope?.let { scopeOf.getOrPut(explained.purpose to key) { it } }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
val accounts =
|
||||
byAccount
|
||||
.map { (account, purposes) ->
|
||||
val purposeRows =
|
||||
purposes
|
||||
.map { (purpose, entities) ->
|
||||
val entityRows =
|
||||
entities
|
||||
.map { (key, relays) ->
|
||||
SubscriptionEntityRow(
|
||||
entityId = key.entityId,
|
||||
scope = scopeOf[purpose to key],
|
||||
detail = detailOf[purpose to key],
|
||||
relays = relays.distinct().sortedBy { it.url },
|
||||
filterCount = relays.size,
|
||||
)
|
||||
}.sortedByDescending { it.filterCount }
|
||||
SubscriptionPurposeRow(
|
||||
purpose = purpose,
|
||||
filterCount = entityRows.sumOf { it.filterCount },
|
||||
relays = entityRows.flatMap { it.relays }.distinct(),
|
||||
entities = entityRows,
|
||||
)
|
||||
}.sortedByDescending { it.filterCount }
|
||||
SubscriptionAccountRow(
|
||||
accountPubKey = account,
|
||||
filterCount = purposeRows.sumOf { it.filterCount },
|
||||
relays = purposeRows.flatMap { it.relays }.distinct(),
|
||||
purposes = purposeRows,
|
||||
)
|
||||
}
|
||||
// unattributed group last, so it reads as a remainder rather than a headline
|
||||
.sortedWith(compareBy<SubscriptionAccountRow> { it.accountPubKey == null }.thenByDescending { it.filterCount })
|
||||
|
||||
ActiveSubscriptionsState(
|
||||
accounts = accounts,
|
||||
totalFilters = total,
|
||||
totalRelays = allRelays.size,
|
||||
untaggedFilters = untagged,
|
||||
aggregateSubscriptions(
|
||||
client.connectedRelaysFlow().value.associateWith { relay ->
|
||||
client.activeRequests(relay).values.flatten()
|
||||
},
|
||||
)
|
||||
}
|
||||
|
||||
companion object {
|
||||
const val REFRESH_MS = 2_000L
|
||||
private const val REFRESH_MS = 2000L
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Folds the in-flight filters into the screen's rows.
|
||||
*
|
||||
* Pure and separate from the ViewModel so the invariant it exists to keep — every tagged filter
|
||||
* counted **exactly once**, so a purpose's count shares a unit with the total it is drawn against —
|
||||
* is testable without a relay pool. It did not hold before: purposes summed their per-entity rows,
|
||||
* and a batched filter naming six chats counted six times.
|
||||
*/
|
||||
fun aggregateSubscriptions(filtersByRelay: Map<NormalizedRelayUrl, List<Filter>>): ActiveSubscriptionsState {
|
||||
// account -> purpose -> tally (real filter count + relays + per-entity breakdown)
|
||||
val byAccount = mutableMapOf<HexKey?, MutableMap<SubPurpose, PurposeTally>>()
|
||||
val detailOf = mutableMapOf<Pair<SubPurpose, EntityKey>, String?>()
|
||||
val scopeOf = mutableMapOf<Pair<SubPurpose, EntityKey>, IFeedTopNavPerRelayFilter>()
|
||||
var total = 0
|
||||
var untagged = 0
|
||||
val allRelays = mutableSetOf<NormalizedRelayUrl>()
|
||||
|
||||
filtersByRelay.forEach { (relay, filters) ->
|
||||
filters.forEach { filter ->
|
||||
total++
|
||||
val explained = filter as? ExplainedFilter
|
||||
if (explained == null) {
|
||||
untagged++
|
||||
return@forEach
|
||||
}
|
||||
allRelays.add(relay)
|
||||
// Discovery filters name no entity — they go looking for things rather than
|
||||
// serving known ones — but they do carry the selection they search within, which
|
||||
// is what keeps them from collapsing into one nameless row per purpose.
|
||||
val scopeKey = explained.scope?.let { it::class.simpleName }
|
||||
|
||||
val tally =
|
||||
byAccount
|
||||
.getOrPut(explained.accountPubKey) { mutableMapOf() }
|
||||
.getOrPut(explained.purpose) { PurposeTally() }
|
||||
|
||||
// The filter counts ONCE, here — before it is fanned out below. This is the
|
||||
// number that shares a unit with `total`, so a card's share of the whole is a
|
||||
// comparison of like with like.
|
||||
tally.filters++
|
||||
tally.relays.add(relay)
|
||||
|
||||
// A batched filter serves several entities at once — relay-group state is one #d
|
||||
// filter per host relay carrying every joined group on it — so it contributes a
|
||||
// row to each of them rather than collapsing to "All". These rows are a
|
||||
// breakdown, never a total: see [SubscriptionEntityRow.namedInFilters].
|
||||
val entities: List<HexKey?> = explained.entityIds?.takeIf { it.isNotEmpty() } ?: listOf(null)
|
||||
entities.forEach { entityId ->
|
||||
val key = EntityKey(entityId, scopeKey)
|
||||
tally.entities.getOrPut(key) { mutableListOf() }.add(relay)
|
||||
detailOf[explained.purpose to key] = explained.purposeDetail
|
||||
explained.scope?.let { scopeOf.getOrPut(explained.purpose to key) { it } }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
val accounts =
|
||||
byAccount
|
||||
.map { (account, purposes) ->
|
||||
val purposeRows =
|
||||
purposes
|
||||
.map { (purpose, tally) ->
|
||||
val entityRows =
|
||||
tally.entities
|
||||
.map { (key, relays) ->
|
||||
SubscriptionEntityRow(
|
||||
entityId = key.entityId,
|
||||
scope = scopeOf[purpose to key],
|
||||
detail = detailOf[purpose to key],
|
||||
relays = relays.distinct().sortedBy { it.url },
|
||||
namedInFilters = relays.size,
|
||||
)
|
||||
}.sortedByDescending { it.namedInFilters }
|
||||
SubscriptionPurposeRow(
|
||||
purpose = purpose,
|
||||
// The tally, NOT the sum of the entity rows — a batched filter
|
||||
// appears in one row per entity it names.
|
||||
filterCount = tally.filters,
|
||||
relays = tally.relays.sortedBy { it.url },
|
||||
entities = entityRows,
|
||||
)
|
||||
}.sortedByDescending { it.filterCount }
|
||||
SubscriptionAccountRow(
|
||||
accountPubKey = account,
|
||||
filterCount = purposeRows.sumOf { it.filterCount },
|
||||
relays = purposeRows.flatMap { it.relays }.distinct(),
|
||||
purposes = purposeRows,
|
||||
)
|
||||
}
|
||||
// unattributed group last, so it reads as a remainder rather than a headline
|
||||
.sortedWith(compareBy<SubscriptionAccountRow> { it.accountPubKey == null }.thenByDescending { it.filterCount })
|
||||
|
||||
return ActiveSubscriptionsState(
|
||||
accounts = accounts,
|
||||
totalFilters = total,
|
||||
totalRelays = allRelays.size,
|
||||
untaggedFilters = untagged,
|
||||
)
|
||||
}
|
||||
|
||||
+115
@@ -0,0 +1,115 @@
|
||||
/*
|
||||
* Copyright (c) 2025 Vitor Pamplona
|
||||
*
|
||||
* Permission is hereby granted, free of charge, to any person obtaining a copy of
|
||||
* this software and associated documentation files (the "Software"), to deal in
|
||||
* the Software without restriction, including without limitation the rights to use,
|
||||
* copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the
|
||||
* Software, and to permit persons to whom the Software is furnished to do so,
|
||||
* subject to the following conditions:
|
||||
*
|
||||
* The above copyright notice and this permission notice shall be included in all
|
||||
* copies or substantial portions of the Software.
|
||||
*
|
||||
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
|
||||
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
|
||||
* FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
|
||||
* COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
|
||||
* AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
|
||||
* WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
|
||||
*/
|
||||
package com.vitorpamplona.amethyst.ui.screen.loggedIn.relays.subscriptions
|
||||
|
||||
import com.vitorpamplona.amethyst.commons.relayClient.subscriptions.ExplainedFilter
|
||||
import com.vitorpamplona.amethyst.commons.relayClient.subscriptions.SubPurpose
|
||||
import com.vitorpamplona.quartz.nip01Core.relay.filters.Filter
|
||||
import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Test
|
||||
|
||||
/**
|
||||
* The unit invariant behind the Active Subscriptions screen: a purpose's filter count and the
|
||||
* screen's total must be the *same thing counted the same way*, because the card draws one as a
|
||||
* share of the other.
|
||||
*
|
||||
* It was broken by batching. A relay-group state filter carries every joined group on its host
|
||||
* relay, and a public-chat filter carries every followed chat — so summing the per-entity rows
|
||||
* counted one filter once per entity it named. Six chats over six relays reported 144 filters where
|
||||
* 24 were on the wire, and the resulting "8% of all" divided an inflated number by a real one.
|
||||
*/
|
||||
class AggregateSubscriptionsTest {
|
||||
private val account = "aa9047325603dacd4f8142093567973566de3b1e20a89557b728c3be4c6a844b"
|
||||
private val chats = (1..6).map { "cafe$it".padEnd(64, '0') }
|
||||
|
||||
private fun relay(n: Int) = NormalizedRelayUrl("wss://relay$n.example/")
|
||||
|
||||
/** One batched filter naming every chat the relay serves — what the real builders emit. */
|
||||
private fun batched(kind: Int) =
|
||||
ExplainedFilter(
|
||||
purpose = SubPurpose.PUBLIC_CHATS,
|
||||
entityIds = chats,
|
||||
kinds = listOf(kind),
|
||||
tags = mapOf("e" to chats),
|
||||
accountPubKey = account,
|
||||
)
|
||||
|
||||
/** Six relays, each carrying four batched filters that each name all six chats. */
|
||||
private fun sixChatsOnSixRelays(): Map<NormalizedRelayUrl, List<Filter>> = (1..6).associate { r -> relay(r) to listOf(batched(40), batched(41), batched(42), batched(43)) }
|
||||
|
||||
@Test
|
||||
fun `a batched filter counts once, not once per entity it names`() {
|
||||
val state = aggregateSubscriptions(sixChatsOnSixRelays())
|
||||
|
||||
// 6 relays x 4 filters = 24 actually in flight. The bug reported 144 (24 x 6 entities).
|
||||
assertEquals(24, state.totalFilters)
|
||||
|
||||
val purpose =
|
||||
state.accounts
|
||||
.single()
|
||||
.purposes
|
||||
.single()
|
||||
assertEquals(24, purpose.filterCount)
|
||||
|
||||
// The per-entity breakdown is still there, and still says each chat is named by 24 filters.
|
||||
assertEquals(6, purpose.entities.size)
|
||||
purpose.entities.forEach { assertEquals(24, it.namedInFilters) }
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `purpose counts sum to the total, so a share of the whole is a like-for-like ratio`() {
|
||||
val state = aggregateSubscriptions(sixChatsOnSixRelays())
|
||||
|
||||
val summed = state.accounts.sumOf { acct -> acct.purposes.sumOf { it.filterCount } }
|
||||
assertEquals(state.totalFilters, summed)
|
||||
assertEquals(state.totalFilters, state.accounts.sumOf { it.filterCount })
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `relays are counted distinctly, not once per filter that reaches them`() {
|
||||
val state = aggregateSubscriptions(sixChatsOnSixRelays())
|
||||
|
||||
assertEquals(6, state.totalRelays)
|
||||
assertEquals(
|
||||
6,
|
||||
state.accounts
|
||||
.single()
|
||||
.purposes
|
||||
.single()
|
||||
.relays.size,
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `untagged filters are reported but never attributed to a purpose`() {
|
||||
val state =
|
||||
aggregateSubscriptions(
|
||||
mapOf(relay(1) to listOf(batched(42), Filter(kinds = listOf(1)))),
|
||||
)
|
||||
|
||||
assertEquals(2, state.totalFilters)
|
||||
assertEquals(1, state.untaggedFilters)
|
||||
// Only the tagged one reaches a card, so the cards no longer add up to the total here —
|
||||
// which is exactly what the untagged line under the header exists to explain.
|
||||
assertEquals(1, state.accounts.sumOf { it.filterCount })
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user