mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-08-08 23:54:39 +00:00
fix: keep observeNotes list sorted while deduping addressables by idHex
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ah1aCniyjnzc27x4pwq2Df
This commit is contained in:
+68
-36
@@ -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<Note>,
|
||||
private val update: (List<Note>) -> Unit,
|
||||
) : Observable {
|
||||
val currentResults: ConcurrentHashMap<HexKey, Note> = 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<Entry> { 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<HexKey, Entry>()
|
||||
|
||||
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<Note> = 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<Note> = sorted.map { it.note }
|
||||
}
|
||||
|
||||
+21
@@ -101,6 +101,27 @@ class NoteListMatchingFilterTest {
|
||||
assertEquals(last.size, last.map { it.idHex }.toSet().size, "no duplicate keys")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun listStaysSortedByCreatedAtDescendingAsNotesArriveOutOfOrder() {
|
||||
var last: List<Note> = 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<Note> = emptyList()
|
||||
|
||||
Reference in New Issue
Block a user