From eaba40f2a6f320768906fa8536454549d82fbda9 Mon Sep 17 00:00:00 2001 From: nrobi144 Date: Mon, 10 Aug 2026 14:51:19 +0300 Subject: [PATCH] =?UTF-8?q?fix:=20address=20PR=20review=20=E2=80=94=20narr?= =?UTF-8?q?ow=20search-relay=20seam,=20Local=20providers=20on=20Android,?= =?UTF-8?q?=20bug=20+=20test=20fixes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings from @davotoula: 1. Behaviour drift (real): the per-note event finder reused Account.searchRelays() (trusted + own search list), widening every missing-event REQ to trusted relays. Add a narrow UserFinderAccount.searchOnlyRelays() (= the pre-extraction account.searchRelayList read) and use it in FilterMissingEvents. Implemented on Account and DesktopIAccount. 2. Runtime trap: LocalUserFinder/LocalUserFinderAccount/LocalEventFinder error() when unprovided and were only provided on Desktop. Provide them on Android too, at the logged-in root (AppNavigation) from accountViewModel.dataSources() + .account, so any shared composable using the no-arg observeUser*/EventFinderFilterAssemblerSubscription overloads is safe on Android (the :napplet process never renders these). 3. Removed the redundant `.ifEmpty { DefaultSearchRelayList }` in Account.searchRelays() (SearchRelayListState.flow already applies that fallback) + its now-unused import. 4. Fixed a pre-existing shadowing bug on lines this PR touches: FilterByEvent's `note.replyTo?.forEach { parentNote -> }` used `note` in the body, so parent notes were never fetched — now uses `parentNote`. 5. Added a test at the layer where #1 lived: filterMissingEvents(cache, keys) fans a missing event to searchOnlyRelays + the follow/mine/search default, NOT the trusted relays that searchRelays would add. 6. Replaced an inline fully-qualified name in the test with an import (CLAUDE.md style). Green: commons jvmTest + verifyKmpPurity, :amethyst compilePlayDebugKotlin, :desktopApp compile, spotless. Co-Authored-By: Claude Opus 4.8 --- .../vitorpamplona/amethyst/model/Account.kt | 7 +- .../subassemblies/FilterByEvent.kt | 6 +- .../amethyst/ui/navigation/AppNavigation.kt | 12 ++++ .../event/loaders/FilterMissingEvents.kt | 2 +- .../relayClient/user/UserFinderAccount.kt | 11 ++- .../event/EventFinderFilterAssemblyTest.kt | 68 ++++++++++++++++++- .../amethyst/desktop/model/DesktopIAccount.kt | 3 + 7 files changed, 101 insertions(+), 8 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 2260fb2b95..4cfab50126 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt @@ -32,7 +32,6 @@ import com.vitorpamplona.amethyst.commons.connectedApps.signers.NostrSignerPermi import com.vitorpamplona.amethyst.commons.connectedApps.signers.NostrSignerPermissionStore import com.vitorpamplona.amethyst.commons.defaults.Constants import com.vitorpamplona.amethyst.commons.defaults.DefaultIndexerRelayList -import com.vitorpamplona.amethyst.commons.defaults.DefaultSearchRelayList import com.vitorpamplona.amethyst.commons.marmot.MarmotManager import com.vitorpamplona.amethyst.commons.model.IAccount import com.vitorpamplona.amethyst.commons.model.buzz.BuzzRelayDialect @@ -382,7 +381,11 @@ class Account( override fun outboxHomeRelays(): Set = nip65RelayList.allFlowNoDefaults.value + privateStorageRelayList.flow.value + localRelayList.flow.value - override fun searchRelays(): Set = (trustedRelayList.flow.value + searchRelayList.flow.value.ifEmpty { DefaultSearchRelayList }).toSet() + // searchRelayList.flow already applies the DefaultSearchRelayList fallback internally + // (SearchRelayListState.normalizeSearchRelayListWithBackup), so no ifEmpty needed here. + override fun searchRelays(): Set = (trustedRelayList.flow.value + searchRelayList.flow.value).toSet() + + override fun searchOnlyRelays(): Set = searchRelayList.flow.value override fun followPlusAllMineWithSearchRelays(): Set = followPlusAllMineWithSearch.flow.value diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/searchCommand/subassemblies/FilterByEvent.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/searchCommand/subassemblies/FilterByEvent.kt index ae225e1b27..1bab42b2ef 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/searchCommand/subassemblies/FilterByEvent.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/searchCommand/subassemblies/FilterByEvent.kt @@ -45,9 +45,9 @@ fun filterByEvent( // loads threading that is event-based note.replyTo?.forEach { parentNote -> - if (parentNote !is AddressableNote && note.event == null) { - potentialRelaysToFindEvent(LocalCache, note).ifEmpty { default }.forEach { relayUrl -> - add(relayUrl, note.idHex) + if (parentNote !is AddressableNote && parentNote.event == null) { + potentialRelaysToFindEvent(LocalCache, parentNote).ifEmpty { default }.forEach { relayUrl -> + add(relayUrl, parentNote.idHex) } } } 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 ee1ace2d8d..2f1287512d 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 @@ -50,6 +50,9 @@ import androidx.navigation.compose.composable import com.vitorpamplona.amethyst.Amethyst import com.vitorpamplona.amethyst.R import com.vitorpamplona.amethyst.commons.nipACWebRtcCalls.CallState +import com.vitorpamplona.amethyst.commons.relayClient.event.LocalEventFinder +import com.vitorpamplona.amethyst.commons.relayClient.user.LocalUserFinder +import com.vitorpamplona.amethyst.commons.relayClient.user.LocalUserFinderAccount import com.vitorpamplona.amethyst.service.crashreports.DisplayCrashMessages import com.vitorpamplona.amethyst.service.relayClient.notifyCommand.compose.DisplayNotifyMessages import com.vitorpamplona.amethyst.service.resourceusage.DisplayResourceUsageAlert @@ -341,6 +344,15 @@ fun AppNavigation( CompositionLocalProvider( LocalScreenLayout provides screenLayout, LocalTabReselectCoordinator provides tabReselectCoordinator, + // Provide the shared finder CompositionLocals so any commons composable that + // uses the no-arg observeUser*/EventFinderFilterAssemblerSubscription(note) + // overloads works when rendered on Android (they error() if unprovided). Android's + // own UI uses the AccountViewModel overloads and doesn't strictly need these, but + // providing them removes the runtime trap for shared composables reaching the + // logged-in tree. (The :napplet process never renders these composables.) + LocalUserFinder provides accountViewModel.dataSources().userFinder, + LocalUserFinderAccount provides accountViewModel.account, + LocalEventFinder provides accountViewModel.dataSources().eventFinder, ) { AccountSwitcherAndLeftDrawerLayout(accountViewModel, accountSessionManager, nav) { Box(Modifier.fillMaxSize()) { diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/relayClient/event/loaders/FilterMissingEvents.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/relayClient/event/loaders/FilterMissingEvents.kt index 65cac46200..6675cf7538 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/relayClient/event/loaders/FilterMissingEvents.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/relayClient/event/loaders/FilterMissingEvents.kt @@ -115,7 +115,7 @@ fun filterMissingEvents( add(relayUrl, key.note.idHex) } - key.account.searchRelays().forEach { relayUrl -> + key.account.searchOnlyRelays().forEach { relayUrl -> add(relayUrl, key.note.idHex) } } diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/relayClient/user/UserFinderAccount.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/relayClient/user/UserFinderAccount.kt index 9f625e2839..d1ecaae910 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/relayClient/user/UserFinderAccount.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/relayClient/user/UserFinderAccount.kt @@ -56,9 +56,18 @@ interface UserFinderAccount { /** Home/write relays used for outbox discovery (nip65 + private storage + local). */ fun outboxHomeRelays(): Set - /** Search relays (trusted + search), with the default fallback applied. */ + /** Search relays (trusted + own search list), for the user-finder's search-tier fallback. */ fun searchRelays(): Set + /** + * Just this account's own NIP-51 search relay list — WITHOUT the trusted-relay + * union that [searchRelays] adds. This is the narrow set the per-note event + * finder fans "missing event" REQs to, matching the pre-extraction + * `account.searchRelayList` read (reusing [searchRelays] there would have + * unintentionally widened the fan-out to trusted relays). + */ + fun searchOnlyRelays(): Set + /** * Follow + all-mine + search relays, used by the per-note event-finder to * place "missing event" / "missing addressable" REQs (reactions, zaps, diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/relayClient/event/EventFinderFilterAssemblyTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/relayClient/event/EventFinderFilterAssemblyTest.kt index 034565a5d5..cfb3c42909 100644 --- a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/relayClient/event/EventFinderFilterAssemblyTest.kt +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/relayClient/event/EventFinderFilterAssemblyTest.kt @@ -20,18 +20,22 @@ */ package com.vitorpamplona.amethyst.commons.relayClient.event +import com.vitorpamplona.amethyst.commons.model.AddressableNote import com.vitorpamplona.amethyst.commons.model.Channel import com.vitorpamplona.amethyst.commons.model.Note import com.vitorpamplona.amethyst.commons.model.User import com.vitorpamplona.amethyst.commons.model.cache.ICacheEventStream import com.vitorpamplona.amethyst.commons.model.cache.ICacheProvider import com.vitorpamplona.amethyst.commons.relayClient.event.loaders.filterMissingEvents +import com.vitorpamplona.amethyst.commons.relayClient.user.UserFinderAccount import com.vitorpamplona.quartz.nip01Core.core.Address import com.vitorpamplona.quartz.nip01Core.core.Event import com.vitorpamplona.quartz.nip01Core.core.HexKey import com.vitorpamplona.quartz.nip01Core.hints.HintIndexer import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl +import com.vitorpamplona.quartz.nip85TrustedAssertions.list.tags.ServiceProviderTag import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Test @@ -73,6 +77,43 @@ class EventFinderFilterAssemblyTest { assertTrue(filterMissingEvents(mapOf(relay1 to emptySet())).isEmpty()) } + /** + * The per-note event finder fans a missing event out to the account's own + * search relays ([UserFinderAccount.searchOnlyRelays]) plus the + * follow/mine/search default — NOT the trusted-relay union that + * [UserFinderAccount.searchRelays] adds. Regression guard for reusing the + * wrong getter here (which would silently widen every missing-event REQ to + * trusted relays). + */ + @Test + fun `filterMissingEvents(keys) uses searchOnlyRelays, not the trusted searchRelays union`() { + val searchOnly = NormalizedRelayUrl("wss://search.test/") + val trustedExtra = NormalizedRelayUrl("wss://trusted.test/") // only in searchRelays() + val default = NormalizedRelayUrl("wss://default.test/") // followPlusAllMineWithSearchRelays() + + val account = + object : StubAccount() { + override fun searchOnlyRelays() = setOf(searchOnly) + + override fun searchRelays() = setOf(searchOnly, trustedExtra) + + override fun followPlusAllMineWithSearchRelays() = setOf(default) + } + + // event == null and not addressable → a "missing event"; no author/replies, + // and the stub cache has empty relay hints, so potentialRelaysToFindEvent is + // empty and the code falls back to the default relays + searchOnlyRelays. + val note = Note("a".repeat(64)) + + val filters = filterMissingEvents(StubCache(), listOf(EventFinderQueryState(note, account))) + val relays = filters.map { it.relay }.toSet() + + assertTrue("search-only relay carries the missing-event REQ", searchOnly in relays) + assertTrue("follow/mine/search default carries it", default in relays) + assertFalse("trusted relay (only in searchRelays) must NOT be fanned out to", trustedExtra in relays) + filters.forEach { assertEquals(listOf(note.idHex), it.filter.ids) } + } + /** * The new default [ICacheProvider.checkGetOrCreateUser] must swallow a * malformed-key throw and return null (the event-finder follows pubkey hints @@ -106,7 +147,7 @@ class EventFinderFilterAssemblyTest { override fun checkGetOrCreateNote(hexKey: HexKey): Note? = null - override fun getOrCreateAddressableNote(key: Address): com.vitorpamplona.amethyst.commons.model.AddressableNote = error("unused") + override fun getOrCreateAddressableNote(key: Address): AddressableNote = error("unused") override fun getEventStream(): ICacheEventStream = error("unused") @@ -116,4 +157,29 @@ class EventFinderFilterAssemblyTest { override fun justConsumeMyOwnEvent(event: Event): Boolean = false } + + /** Minimal [UserFinderAccount] returning nothing; override the relevant getters per test. */ + private open class StubAccount : UserFinderAccount { + override val userFinderPubkeyHex: HexKey = "00".repeat(32) + + override fun indexRelays(): Set = emptySet() + + override fun outboxHomeRelays(): Set = emptySet() + + override fun searchRelays(): Set = emptySet() + + override fun searchOnlyRelays(): Set = emptySet() + + override fun followPlusAllMineWithSearchRelays(): Set = emptySet() + + override fun commonRelays(): Set = emptySet() + + override fun cardHomeRelays(): Set = emptySet() + + override fun trustProvider(): ServiceProviderTag? = null + + override fun followerCountProvider(): ServiceProviderTag? = null + + override fun declaredFollowsByOutboxRelay(): Map> = emptyMap() + } } diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/model/DesktopIAccount.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/model/DesktopIAccount.kt index c045e74df5..e68f95c233 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/model/DesktopIAccount.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/model/DesktopIAccount.kt @@ -114,6 +114,9 @@ class DesktopIAccount( override fun searchRelays(): Set = relayManager.connectedRelays.value + // Desktop has no separate NIP-51 search relay list; degrade to connected relays. + override fun searchOnlyRelays(): Set = relayManager.connectedRelays.value + // Desktop has no merged follow/mine/search relay-list subsystem; route // missing-event discovery through the connected relays (same degrade path // as the other hints above).