From 1a97982668888c73a2c8c88dd8950cdd6e044c7e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 15:41:16 +0000 Subject: [PATCH] test: put the two front ends' REQs side by side MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The claim this whole pass rests on is that the same query reaches a relay as the same REQ from either front end. Nothing checked it — every bug it started from was a violation of it, and each one was invisible because the two answers were never compared. `desktopApp` is the only module that can see both paths, so the test lives there: 16 query shapes, each asked through Android's `searchPostsByText` and Desktop's `SearchFilterFactory`, compared arm by arm on kinds, authors, tags, search, since and until. One assumption of mine was wrong and the test said so: a bare `from:` does build filters on both sides. It is bounded by its author, so unlike a bare `kind:` it is a real search rather than an unbounded feed. The plan is updated to what shipped, including the two findings left for someone else: twelve event classes missing from `EventFactory`, and the searchable-kinds reference table now nine kinds behind the code. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DWTxEzzvD3mKkkgE4N7H66 --- .../2026-09-10-search-state-unification.md | 88 ++++++++++----- .../subscriptions/SearchFilterParityTest.kt | 104 ++++++++++++++++++ 2 files changed, 167 insertions(+), 25 deletions(-) create mode 100644 desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/subscriptions/SearchFilterParityTest.kt diff --git a/commons/plans/2026-09-10-search-state-unification.md b/commons/plans/2026-09-10-search-state-unification.md index b29a663de7..6c076f36c4 100644 --- a/commons/plans/2026-09-10-search-state-unification.md +++ b/commons/plans/2026-09-10-search-state-unification.md @@ -1,6 +1,7 @@ # Search state unification -*Status: proposal — needs maintainer input on sequencing (see §3 and §9).* +*Status: done, by order (b). Phases 1 and 2 both landed; §9's questions are answered +below from what shipped rather than left open.* ## 1. Why @@ -146,25 +147,42 @@ Do not attempt to unify these; they are real differences, not drift: nothing ever indexes them. That last one is a quartz bug, filed here rather than fixed: it is not search's to make. -4. One debounce policy, stated once. +4. **One debounce policy, stated once.** *(Done — two windows on `SearchState`, + named for what they protect: 100 ms before a cache scan, 300 ms before a REQ. + Eight collectors had been declaring the 100 separately.)* *Size: ~400 lines moved/added, ~200 deleted. No module boundaries crossed.* *Risk: low. Both callers keep their current shape.* -### Phase 2 — one state holder *(gated on §3)* +### Phase 2 — one state holder *(done, by order (b))* -5. Extend `ICacheProvider` with the five search entry points `CacheSearch` - already implements: `findNotesMatching`, `findNotesStartingWith`, - `findPublicChatChannelsStartingWith`, `findEphemeralChatChannelsStartingWith`, - `findLiveActivityChannelsStartingWith`. *(Skip entirely under order (a).)* -6. `SearchState` in commons; `SearchBarViewModel` and `AdvancedSearchBarState` - become shells over it. -7. Collapse the 7 scope guards into one filter over the shared result set. -8. Port history + saved searches to Android for free — they become a property of - `SearchState`, not of desktop. +5. **The five search entry points on `ICacheProvider`.** *(Done.)* They default + to returning nothing rather than being abstract: Desktop's cache holds notes + and live channels but no public-chat or ephemeral store, and a port that + forced it to implement those would be asking it to lie. `CacheSearch` now + takes `LiveHiddenUsers` rather than the `HiddenUsersState` holder, which + also fixed `findNotesStartingWith` re-reading `.flow.value` five times down + one scan. +6. **`SearchState` in commons.** *(Done.)* `SearchBarViewModel` 615 → 508 lines; + `AdvancedSearchBarState` delegates its text, parse, debounce and sort orders. + `SearchInput` carries the text and its parse as one value, so a collector + cannot pair one keystroke's characters with another's parse — and carries + `nameTerms`, the most-repeated parse of all. +7. **One scope table.** *(Done — `SearchScope.shows(SearchResultKind)`.)* The + seven guards disagreed: the note flow let hashtags through under Notes, the + channel flows did not, and nothing said which was intended. The pinned test + is written from the old guards, not the new enum. +8. **History and saved searches shared.** *(Done, and Android has both now.)* + `SearchHistory` + a two-method `SearchHistoryStorage`; Desktop keeps its + `Preferences` node, Android gets a DataStore file and a recent-searches list + in what used to be a blank screen. -*Size: ~600 lines net reduction across the three files.* -*Risk: medium — this is where behaviour can shift.* +Two bugs surfaced only once the behaviour was under test: a saved search whose +label contained a tab lost the query it named, and re-running a query typed in a +different token order made a second history entry. + +*Risk realised: none observed. Both front ends compile and the parity test below +passes; the behaviour changes are the ones named above.* ## 6. Non-goals @@ -191,18 +209,38 @@ The pure layer is already well covered (`SearchFilterBuilderTest`, `SearchResultSorterTest`, `SearchSeedTest`, `SearchTokenizerTest`). Add before refactoring, not after: -- **A parity table**: one query × both front ends → same filters, same kept set, - same order. This is the test that would have caught all four bugs. -- **Scope × result-type**: the current 7 guards, pinned, before they collapse. +- **A parity table**: one query × both front ends → same filters. *(Done: + `desktopApp`'s `SearchFilterParityTest`, 16 query shapes. It lives there + because that is the only module that can see both paths.)* This is the test + that would have caught all four bugs. +- **Scope × result-type**: the 7 guards, pinned. *(Done: `SearchScopeTest`.)* - **Kind-set parity**: relay allowlist == local allowlist, and every kind in it is one `SearchableEvent` covers. *(Done: `RenderableKindsTest`.)* +- Also added: `SearchPipelineTest` (one case per shipped bug), `SearchStateTest`, + `SearchHistoryTest`, and quartz's `SearchableKindsTest`. -## 9. Open questions for the maintainer +## 9. Questions, and what shipped -1. Sequencing — order (a) or (b) in §3? (b) is additive to the sweep and - unblocks now; (a) is less total work but waits on step 3. -2. Should `AdvancedSearchBarState` be deleted outright in Phase 2, or kept as a - desktop-side shell? It is in `commons/` but desktop-only today, which is - itself a naming problem. -3. Is Android's 7-result-type surface wanted on desktop eventually? If yes, the - shared result map is the right shape; if no, it stays a front-end concern. +1. **Sequencing — (a) or (b)?** (b). The five entry points are additive to the + sweep rather than parallel to it, and they survive the `LocalCache` move + unchanged: when step 3 lands, `DesktopLocalCache` starts answering the same + methods instead of inheriting the empty defaults, and nothing above the port + changes. +2. **Delete `AdvancedSearchBarState` or keep it?** Kept, as Desktop's shell. What + is left in it is genuinely Desktop's — relay callbacks, raw `Event` results, + per-relay sync status, the form panel — so deleting it would only move that + code, not remove it. It is still in `commons/` while being desktop-only, + which remains a naming problem worth fixing on its own. +3. **Is Android's 7-result-type surface wanted on Desktop?** Left open — it is a + product question, not a structural one. `SearchResultKind` now names the seven + in commons, so the answer can be acted on without another refactor. + +## 10. What is left + +- Pagination past the 100/200 cap (§6, still a non-goal here). +- `FeedDefinitionEvent` and eleven other event classes are missing from + `EventFactory`, so their events parse as a plain `Event` and nothing indexes + them. A quartz bug, found by the kind sweep, not fixed here. +- `.claude/skills/searchable-events/references/searchable-kinds.md` is stale + (133 kinds recorded, 142 reachable) and now has a machine-checked list to be + reconciled against. diff --git a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/subscriptions/SearchFilterParityTest.kt b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/subscriptions/SearchFilterParityTest.kt new file mode 100644 index 0000000000..c0526f0c63 --- /dev/null +++ b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/subscriptions/SearchFilterParityTest.kt @@ -0,0 +1,104 @@ +/* + * 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.desktop.subscriptions + +import com.vitorpamplona.amethyst.commons.relayClient.search.searchPostsByText +import com.vitorpamplona.amethyst.commons.search.QueryParser +import com.vitorpamplona.quartz.nip01Core.relay.normalizer.RelayUrlNormalizer +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The one test that would have caught all four of them. + * + * The same query, asked by both front ends, must reach a relay as the same REQ. This is the only + * place in the repo where both paths can be called from — `desktopApp` sees `commons`, and + * `commons` is where Android's subscription builder lives — so it is the only place the claim can + * be checked rather than asserted in a comment. + * + * Every bug this refactor started from was a violation of it: a kind window that had drifted on + * one side, a `kind:` chip that reached the REQ on one platform and not the other, a hashtag + * fan-out that differed by an arm. They were invisible because nothing ever put the two answers + * side by side. + */ +class SearchFilterParityTest { + private val relay = RelayUrlNormalizer.normalizeOrNull("wss://relay.example.com")!! + + private fun androidAsks(text: String) = searchPostsByText(text, relay).map { it.filter } + + private fun desktopAsks(text: String) = SearchFilterFactory.createFilters(QueryParser.parse(text)) + + private fun assertSameREQ(text: String) { + val android = androidAsks(text) + val desktop = desktopAsks(text) + assertEquals("filter count differs for \"$text\"", desktop.size, android.size) + android.zip(desktop).forEachIndexed { i, (a, d) -> + assertEquals("kinds differ at arm $i of \"$text\"", d.kinds, a.kinds) + assertEquals("authors differ at arm $i of \"$text\"", d.authors, a.authors) + assertEquals("tags differ at arm $i of \"$text\"", d.tags, a.tags) + assertEquals("search differs at arm $i of \"$text\"", d.search, a.search) + assertEquals("since differs at arm $i of \"$text\"", d.since, a.since) + assertEquals("until differs at arm $i of \"$text\"", d.until, a.until) + } + } + + @Test + fun bothFrontEndsAskTheSameThing() { + listOf( + "bitcoin", + "kind:article bitcoin", + "kind:picture", + "kind:video nostr", + "#nostr", + "#nostr bitcoin", + "from:npub180cvv07tjdrrgpa0j7j7tmnyl2yr6yr7l8j4s3evf6u64th6gkwsyjh6w6 bitcoin", + "to:npub180cvv07tjdrrgpa0j7j7tmnyl2yr6yr7l8j4s3evf6u64th6gkwsyjh6w6 hello", + "geo:9q8yy coffee", + "bitcoin -scam", + "kind:reply bitcoin", + "lang:en bitcoin", + "domain:example.com bitcoin", + "\"exact phrase\"", + "bitcoin since:2024-01-01 until:2024-12-31", + "from:npub180cvv07tjdrrgpa0j7j7tmnyl2yr6yr7l8j4s3evf6u64th6gkwsyjh6w6", + ).forEach(::assertSameREQ) + } + + @Test + fun aQueryNamingItsOwnKindCollapsesToOneArmOnBothSides() { + // The fan-out exists for the default window; a named kind is one group, and a platform + // that kept fanning out would be asking for kinds the chip on screen excluded. + assertTrue(androidAsks("kind:article bitcoin").all { it.kinds == listOf(30023) }) + assertTrue(desktopAsks("kind:article bitcoin").all { it.kinds == listOf(30023) }) + } + + @Test + fun neitherSideAsksAnythingForAQueryThatSaysNothing() { + // A bare `kind:` is not one of these on a technicality: "every recent article" is an + // unbounded REQ. A bare `from:` is bounded by its author, so it is a real search and both + // sides do ask for it — which is why it is in the parity list above rather than here. + listOf("", " ", "kind:article").forEach { + assertEquals("android asked for \"$it\"", emptyList(), androidAsks(it)) + assertEquals("desktop asked for \"$it\"", emptyList(), desktopAsks(it)) + } + } +}