From 65e30c0acd32f77c8cd8783fe9e5bee76dee30c2 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 18:03:26 +0000 Subject: [PATCH] fix: keep observeNotes list sorted while deduping addressables by idHex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback: keep the incrementally-maintained, created_at-sorted structure (a feed must stay sorted like a relay) instead of re-sorting a hash map on every emission. The root cause is unchanged: there is one Note instance per id/address (LocalCache owns creation), but a note's sort key is mutable — a newer replaceable event swaps the event on the SAME AddressableNote instance, changing created_at in place. A sorted set ordered on that live value corrupts: the moved node leaves the add()/remove() search path, so the same instance is inserted twice and the emitted list carries a duplicate idHex, crashing the App Recommendations LazyColumn (keyed on idHex). Fix: snapshot the sort key into an immutable Entry when the note first enters, order a ConcurrentSkipListSet on that snapshot (never read live again), and index entries by the stable idHex (ConcurrentHashMap + putIfAbsent) so membership stays unique and removal is reliable regardless of later created_at changes. Ordering and "new versions do not update the list" are preserved. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Ah1aCniyjnzc27x4pwq2Df --- .../observables/NoteListMatchingFilter.kt | 104 ++++++++++++------ .../observables/NoteListMatchingFilterTest.kt | 21 ++++ 2 files changed, 89 insertions(+), 36 deletions(-) diff --git a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/observables/NoteListMatchingFilter.kt b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/observables/NoteListMatchingFilter.kt index 07c82c1dc1..29bd6f3147 100644 --- a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/observables/NoteListMatchingFilter.kt +++ b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/observables/NoteListMatchingFilter.kt @@ -28,30 +28,56 @@ import com.vitorpamplona.quartz.nip01Core.core.HexKey import com.vitorpamplona.quartz.nip01Core.relay.filters.Filter import java.util.SortedSet import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.ConcurrentSkipListSet /** - * Creates a list of notes (regular and addressable) - * that only gets updated when a new note appears. + * Creates a list of notes (regular and addressable), sorted by created_at like a + * relay, that only grows when a new note appears. * * New versions of addressables do not update the list. * - * Membership is keyed by the immutable [Note.idHex] rather than kept in a - * sorted set ordered by createdAt. AddressableNotes are mutable: when a newer - * version of a replaceable event arrives, LocalCache swaps the event on the - * SAME note instance, changing its createdAt in place. A - * ConcurrentSkipListSet ordered on that createdAt cannot survive the change — - * the moved node is no longer found by add()/remove(), so the note ends up - * inserted twice and the emitted list carries a duplicate idHex, crashing any - * LazyColumn keyed on it. Deduping by idHex keeps membership correct - * regardless of createdAt changes; the display order is computed fresh on each - * emission. + * There is exactly one [Note] instance per id/address (LocalCache owns their + * creation), so uniqueness is a non-issue in principle — except a note's sort + * key is mutable: a newer replaceable event swaps the event on the SAME + * [AddressableNote] instance, changing its created_at in place. A sorted set + * ordered on that live value cannot survive it — the moved node is no longer on + * the search path of add()/remove(), so the same instance gets inserted twice + * and the emitted list carries a duplicate idHex, crashing any LazyColumn keyed + * on it. + * + * So the sort key is snapshotted into an immutable [Entry] when the note first + * enters and never read live again; the ordered set is keyed on that snapshot + * (stable), and an idHex index keeps membership unique and makes removal reliable + * regardless of later created_at changes. */ class NoteListMatchingFilter( private val filter: Filter, private val atOnce: (filter: Filter) -> SortedSet, private val update: (List) -> Unit, ) : Observable { - val currentResults: ConcurrentHashMap = ConcurrentHashMap() + /** A note plus the sort key captured at insertion time, so ordering never depends on mutable state. */ + private class Entry( + val note: Note, + val createdAt: Long, + val id: HexKey, + ) + + // created_at descending, id ascending as a stable tiebreak. Both fields are + // immutable snapshots, so an Entry never moves once inserted. + private val order = + Comparator { a, b -> + val byCreatedAt = b.createdAt.compareTo(a.createdAt) + if (byCreatedAt != 0) byCreatedAt else a.id.compareTo(b.id) + } + + private val sorted = ConcurrentSkipListSet(order) + private val byId = ConcurrentHashMap() + + private fun entryFor(note: Note): Entry { + // A null event (unresolved note) sorts last, matching CreatedAtIdHexComparator. + val event = note.event + return Entry(note, note.createdAt() ?: Long.MIN_VALUE, event?.id ?: note.idHex) + } override fun new( event: Event, @@ -59,35 +85,41 @@ class NoteListMatchingFilter( ) { if (event is AddressableEvent && note !is AddressableNote) return - // New versions of addressables do not update the list. - if (currentResults.containsKey(note.idHex)) return + if (!filter.match(event)) return - if (filter.match(event)) { - currentResults[note.idHex] = note + val entry = entryFor(note) - val limit = filter.limit - if (limit != null && currentResults.size > limit) { - // Drop the oldest (sorts last under CreatedAtIdHexComparator). - currentResults.values.maxWithOrNull(CreatedAtIdHexComparator)?.let { - currentResults.remove(it.idHex) - } - } + // putIfAbsent gates uniqueness atomically: new versions of an already + // listed addressable return here without touching the sorted set. + if (byId.putIfAbsent(note.idHex, entry) != null) return - update(snapshot()) + sorted.add(entry) + + val limit = filter.limit + if (limit != null && sorted.size > limit) { + sorted.pollLast()?.let { byId.remove(it.note.idHex, it) } } - } - override fun remove(note: Note) { - if (currentResults.remove(note.idHex) != null) { - update(snapshot()) - } - } - - fun init() { - currentResults.clear() - atOnce(filter).forEach { currentResults[it.idHex] = it } update(snapshot()) } - private fun snapshot(): List = currentResults.values.sortedWith(CreatedAtIdHexComparator) + override fun remove(note: Note) { + val entry = byId.remove(note.idHex) ?: return + sorted.remove(entry) + update(snapshot()) + } + + fun init() { + sorted.clear() + byId.clear() + atOnce(filter).forEach { note -> + val entry = entryFor(note) + if (byId.putIfAbsent(note.idHex, entry) == null) { + sorted.add(entry) + } + } + update(snapshot()) + } + + private fun snapshot(): List = sorted.map { it.note } } diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/model/observables/NoteListMatchingFilterTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/model/observables/NoteListMatchingFilterTest.kt index cbdfdd6bfc..91457f08df 100644 --- a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/model/observables/NoteListMatchingFilterTest.kt +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/model/observables/NoteListMatchingFilterTest.kt @@ -101,6 +101,27 @@ class NoteListMatchingFilterTest { assertEquals(last.size, last.map { it.idHex }.toSet().size, "no duplicate keys") } + @Test + fun listStaysSortedByCreatedAtDescendingAsNotesArriveOutOfOrder() { + var last: List = emptyList() + val subject = newFilter { last = it } + subject.init() + + val a = noteFor("app-a") + val b = noteFor("app-b") + val c = noteFor("app-c") + + // Arrive out of order; the emitted list must always be newest-first. + a.load(2000) + subject.new(a.event!!, a) + b.load(4000) + subject.new(b.event!!, b) + c.load(1000) + subject.new(c.event!!, c) + + assertEquals(listOf(b.idHex, a.idHex, c.idHex), last.map { it.idHex }) + } + @Test fun removeDropsTheNoteEvenAfterCreatedAtChanged() { var last: List = emptyList()