Fix disappearing events on thread view

This commit fixes an issue where events in threads would occasionally
disappear.

Previously, the computation of parent events and reply events depended
on EventCache and had to be manually computed upon event selection
change. This may lead to inconsistencies if the computation is not
re-done after a new event that leads to a change in the model, or if certain
events are not yet on the cache. Instead, these are now computed
properties inside ThreadModel, and relies exclusively on the events
already in the ThreadModel.

Several other smaller improvements were made around the affected class,
including:
- Removing unused code for simplicity
- Configuring the class external interface with more intent, avoiding
  misusage
- Adding more documentation on the usage of things, as well as
  implementation notes on why certain design decisions were taken.
- Moving things to explicit actors, to integrate more structured concurrency
- Improving code efficiency to lower computational overhead on the main
  actor
- Splitting concerns between objects with more intent and thoughful
  design.

Changelog-Fixed: Fixed an issue where events on a thread view would occasionally disappear
Closes: https://github.com/damus-io/damus/issues/2791
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This commit is contained in:
Daniel D’Aquino
2025-01-10 13:34:45 +09:00
parent 8066fa1bf8
commit 74d5bee1f6
8 changed files with 258 additions and 231 deletions

View File

@@ -7,42 +7,75 @@
import Foundation
/// manages the lifetime of a thread
/// manages the lifetime of a thread in a thread view such as `ChatroomThreadView`
/// Makes a subscription to the relay pool to get events related to the thread
/// It also keeps track of a selected event in the thread, and can pinpoint all of its parents and reply chain
@MainActor
class ThreadModel: ObservableObject {
@Published var event: NostrEvent
/// The original event where this thread was loaded from
/// We use this to know the starting point from which we try to load the rest of the thread
/// This is immutable because this is our starting point of the thread, and we don't expect this to ever change during the lifetime of a thread view
let original_event: NostrEvent
let highlight: String?
var event_map: Set<NostrEvent>
/// A map of events, the reply chain, etc
/// This can be read by the view, but it can only be updated internally, because it is this classes' responsibility to ensure we load the proper events
@Published private(set) var event_map: ThreadEventMap
/// The currently selected event
/// Can only be directly changed internally. Views should set this via the `select` methods
@Published private(set) var selected_event: NostrEvent
init(event: NostrEvent, damus_state: DamusState, highlight: String? = nil) {
self.damus_state = damus_state
self.event_map = Set()
self.event = event
self.original_event = event
self.highlight = highlight
add_event(event, keypair: damus_state.keypair)
/// All of the parent events of `selected_event` in the thread, sorted from the highest level in the thread (The root of the thread), down to the direct parent
///
/// ## Implementation notes
///
/// This is a computed property because we then don't need to worry about keeping things in sync
var parent_events: [NostrEvent] {
return event_map.parent_events(of: selected_event)
}
func events() -> [NostrEvent] {
return Array(event_map).sorted(by: { a, b in
return a.created_at < b.created_at
/// All of the direct and indirect replies of `selected_event` in the thread. sorted chronologically
///
/// ## Implementation notes
///
/// This is a computed property because we then don't need to worry about keeping things in sync
var sorted_child_events: [NostrEvent] {
event_map.sorted_recursive_child_events(of: selected_event).filter({
should_show_event(event: $0, damus_state: damus_state) // Hide muted events from chatroom conversation
})
}
var is_original: Bool {
return original_event.id == event.id
}
/// The damus state, needed to access the relay pool and load the thread events
let damus_state: DamusState
let profiles_subid = UUID().description
let base_subid = UUID().description
let meta_subid = UUID().description
var subids: [String] {
private let profiles_subid = UUID().description
private let base_subid = UUID().description
private let meta_subid = UUID().description
private var subids: [String] {
return [profiles_subid, base_subid, meta_subid]
}
// MARK: Initialization
/// Initialize this model
///
/// You should also call `subscribe()` to start loading thread events from the relay pool.
/// This is done manually to ensure we only load stuff when needed (e.g. when a view appears)
init(event: NostrEvent, damus_state: DamusState) {
self.damus_state = damus_state
self.event_map = ThreadEventMap()
self.original_event = event
self.selected_event = event
add_event(event, keypair: damus_state.keypair)
}
/// All events in the thread, sorted in chronological order
var events: [NostrEvent] {
return event_map.sorted_events
}
// MARK: Relay pool subscription management
/// Unsubscribe from events in the relay pool. Call this when unloading the view
func unsubscribe() {
self.damus_state.pool.remove_handler(sub_id: base_subid)
self.damus_state.pool.remove_handler(sub_id: meta_subid)
@@ -50,33 +83,25 @@ class ThreadModel: ObservableObject {
self.damus_state.pool.unsubscribe(sub_id: base_subid)
self.damus_state.pool.unsubscribe(sub_id: meta_subid)
self.damus_state.pool.unsubscribe(sub_id: profiles_subid)
print("unsubscribing from thread \(event.id) with sub_id \(base_subid)")
}
@discardableResult
func set_active_event(_ ev: NostrEvent, keypair: Keypair) -> Bool {
self.event = ev
add_event(ev, keypair: keypair)
//self.objectWillChange.send()
return false
Log.info("unsubscribing to thread %s with sub_id %s", for: .render, original_event.id.hex(), base_subid)
}
/// Subscribe to events in this thread. Call this when loading the view.
func subscribe() {
var meta_events = NostrFilter()
var quote_events = NostrFilter()
var event_filter = NostrFilter()
var ref_events = NostrFilter()
let thread_id = event.thread_id()
let thread_id = original_event.thread_id()
ref_events.referenced_ids = [thread_id, event.id]
ref_events.referenced_ids = [thread_id, original_event.id]
ref_events.kinds = [.text]
ref_events.limit = 1000
event_filter.ids = [thread_id, event.id]
event_filter.ids = [thread_id, original_event.id]
meta_events.referenced_ids = [event.id]
meta_events.referenced_ids = [original_event.id]
var kinds: [NostrKind] = [.zap, .text, .boost]
if !damus_state.settings.onlyzaps_mode {
@@ -86,33 +111,40 @@ class ThreadModel: ObservableObject {
meta_events.limit = 1000
quote_events.kinds = [.text]
quote_events.quotes = [event.id]
quote_events.quotes = [original_event.id]
quote_events.limit = 1000
let base_filters = [event_filter, ref_events]
let meta_filters = [meta_events, quote_events]
print("subscribing to thread \(event.id) with sub_id \(base_subid)")
Log.info("subscribing to thread %s with sub_id %s", for: .render, original_event.id.hex(), base_subid)
damus_state.pool.subscribe(sub_id: base_subid, filters: base_filters, handler: handle_event)
damus_state.pool.subscribe(sub_id: meta_subid, filters: meta_filters, handler: handle_event)
}
/// Adds an event to this thread.
/// Normally this does not need to be called externally because it is the responsibility of this class to load the events, not the view's.
/// However, this can be called externally for testing purposes (e.g. injecting events for testing)
func add_event(_ ev: NostrEvent, keypair: Keypair) {
if event_map.contains(ev) {
if event_map.contains(id: ev.id) {
return
}
damus_state.events.upsert(ev)
_ = damus_state.events.upsert(ev)
damus_state.replies.count_replies(ev, keypair: keypair)
damus_state.events.add_replies(ev: ev, keypair: keypair)
event_map.insert(ev)
event_map.add(event: ev)
// Publish changes
objectWillChange.send()
}
/// Handles an incoming event from a relay pool
///
/// Marked as private because it is this class' responsibility to load events, not the view's. Simplify the interface
@MainActor
func handle_event(relay_id: RelayURL, ev: NostrConnectionEvent) {
private func handle_event(relay_id: RelayURL, ev: NostrConnectionEvent) {
let (sub_id, done) = handle_subid_event(pool: damus_state.pool, relay_id: relay_id, ev: ev) { sid, ev in
guard subids.contains(sid) else {
return
@@ -125,7 +157,7 @@ class ThreadModel: ObservableObject {
} else if ev.is_textlike {
// handle thread quote reposts, we just count them instead of
// adding them to the thread
if let target = ev.is_quote_repost, target == self.event.id {
if let target = ev.is_quote_repost, target == self.selected_event.id {
//let _ = self.damus_state.quote_reposts.add_event(ev, target: target)
} else {
self.add_event(ev, keypair: damus_state.keypair)
@@ -139,10 +171,167 @@ class ThreadModel: ObservableObject {
if sub_id == self.base_subid {
guard let txn = NdbTxn(ndb: damus_state.ndb) else { return }
load_profiles(context: "thread", profiles_subid: self.profiles_subid, relay_id: relay_id, load: .from_events(Array(event_map)), damus_state: damus_state, txn: txn)
load_profiles(context: "thread", profiles_subid: self.profiles_subid, relay_id: relay_id, load: .from_events(Array(event_map.events)), damus_state: damus_state, txn: txn)
}
}
// MARK: External control interface
// Control methods created for the thread view
/// Change the currently selected event
///
/// - Parameter event: Event to select
func select(event: NostrEvent) {
self.selected_event = event
add_event(event, keypair: damus_state.keypair)
}
}
/// A thread event map, a model that holds events, indexes them, and can efficiently answer questions about a thread.
///
/// Add events that are part of a thread to this model, and use one of its many convenience functions to get answers about the hierarchy of the thread.
///
/// This does NOT perform any event loading, networking, or storage operations. This is simply a convenient/efficient way to keep and query about a thread
struct ThreadEventMap {
/// A map for keeping nostr events, and efficiently querying them by note id
///
/// Marked as `private` because:
/// - We want to hide this complexity from the user of this struct
/// - It is this struct's responsibility to keep this in sync with `event_reply_index`
private var event_map: [NoteId: NostrEvent] = [:]
/// An index of the reply hierarchy, which allows replies to be found in O(1) efficiency
///
/// ## Implementation notes
///
/// Marked as `private` because:
/// - We want to hide this complexity from the user of this struct
/// - It is this struct's responsibility to keep this in sync with `event_map`
///
/// We only store note ids to save space, as we can easily get them from `event_map`
private var event_reply_index: [NoteId: Set<NoteId>] = [:]
// MARK: External interface
/// Events in the thread, in no particular order
/// Use this when the order does not matter
var events: Set<NostrEvent> {
return Set(event_map.values)
}
/// Events in the thread, sorted chronologically. Use this when the order matters.
/// Use `.events` when the order doesn't matter, as it is more computationally efficient.
var sorted_events: [NostrEvent] {
return events.sorted(by: { a, b in
return a.created_at < b.created_at
})
}
/// Add an event to this map
///
/// Efficiency: O(1)
///
/// - Parameter event: The event to be added
mutating func add(event: NostrEvent) {
self.event_map[event.id] = event
// Update our efficient reply index
if let note_id_replied_to = event.direct_replies() {
if event_reply_index[note_id_replied_to] == nil {
event_reply_index[note_id_replied_to] = [event.id]
}
else {
event_reply_index[note_id_replied_to]?.insert(event.id)
}
}
}
/// Whether the thread map contains a given note, referenced by ID
///
/// Efficiency: O(1)
///
/// - Parameter id: The ID to look for
/// - Returns: True if it does, false otherwise
func contains(id: NoteId) -> Bool {
return self.event_map[id] != nil
}
/// Gets a note from the thread by its id
///
/// Efficiency: O(1)
///
/// - Parameter id: The note id
/// - Returns: The note, if it exists in the thread map.
func get(id: NoteId) -> NostrEvent? {
return self.event_map[id]
}
/// Returns all the parent events in a thread, relative to a given event
///
/// Efficiency: O(N) in the worst case
///
/// - Parameter query_event: The event for which to find the parents for
/// - Returns: An array of parent events, sorted from the highest level in the thread (The root of the thread), down to the direct parent of the query event. If query event is not found, this will return an empty array
func parent_events(of query_event: NostrEvent) -> [NostrEvent] {
var parents: [NostrEvent] = []
var event = query_event
while true {
guard let direct_reply = event.direct_replies(),
let parent_event = self.get(id: direct_reply), parent_event != event
else {
break
}
parents.append(parent_event)
event = parent_event
}
return parents.reversed()
}
/// All of the replies in a thread for a given event, including indirect replies (reply of a reply), sorted in chronological order
///
/// Efficiency: O(Nlog(N)) in the worst case scenario, coming from Swift's built-in sorting algorithm "Timsort"
///
/// - Parameter query_event: The event for which to find the children for
/// - Returns: All of the direct and indirect replies for an event, sorted in chronological order. If query event is not present, this will be an empty array.
func sorted_recursive_child_events(of query_event: NostrEvent) -> [NostrEvent] {
let all_recursive_child_events = self.recursive_child_events(of: query_event)
return all_recursive_child_events.sorted(by: { a, b in
return a.created_at < b.created_at
})
}
/// All of the replies in a thread for a given event, including indirect replies (reply of a reply), in any order
///
/// Use this when the order does not matter, as it is more efficient
///
/// Efficiency: O(N) in the worst case scenario.
///
/// - Parameter query_event: The event for which to find the children for
/// - Returns: All of the direct and indirect replies for an event, sorted in chronological order. If query event is not present, this will be an empty array.
func recursive_child_events(of query_event: NostrEvent) -> Set<NostrEvent> {
let immediate_children_ids = self.event_reply_index[query_event.id] ?? []
var immediate_children: Set<NostrEvent> = []
for immediate_child_id in immediate_children_ids {
guard let immediate_child = self.event_map[immediate_child_id] else {
// This is an internal inconsistency.
// Crash the app in debug mode to increase awareness, but let it go in production mode (not mission critical)
assertionFailure("Desync between `event_map` and `event_reply_index` should never happen in `ThreadEventMap`!")
continue
}
immediate_children.insert(immediate_child)
}
var indirect_children: Set<NdbNote> = []
for immediate_child in immediate_children {
let recursive_children = self.recursive_child_events(of: immediate_child)
indirect_children = indirect_children.union(recursive_children)
}
return immediate_children.union(indirect_children)
}
}

View File

@@ -31,7 +31,7 @@ struct DamusURLHandler {
let search = SearchModel(state: damus_state, search: nostrFilter)
return .route(.Search(search: search))
case .event(let nostrEvent):
let thread = ThreadModel(event: nostrEvent, damus_state: damus_state)
let thread = await ThreadModel(event: nostrEvent, damus_state: damus_state)
return .route(.Thread(thread: thread))
case .event_reference(let event_reference):
return .route(.ThreadFromReference(note_reference: event_reference))

View File

@@ -189,7 +189,7 @@ enum Route: Hashable {
hasher.combine("firstAidSettings")
case .Thread(let threadModel):
hasher.combine("thread")
hasher.combine(threadModel.event.id)
hasher.combine(threadModel.original_event.id)
case .ThreadFromReference(note_reference: let note_reference):
hasher.combine("thread_from_reference")
hasher.combine(note_reference)

View File

@@ -13,47 +13,24 @@ struct ChatroomThreadView: View {
@State var once: Bool = false
let damus: DamusState
@ObservedObject var thread: ThreadModel
@State var selected_note_id: NoteId? = nil
@State var highlighted_note_id: NoteId? = nil
@State var user_just_posted_flag: Bool = false
@Namespace private var animation
@State var parent_events: [NostrEvent] = []
@State var sorted_child_events: [NostrEvent] = []
func compute_events(selected_event: NostrEvent? = nil) {
let selected_event = selected_event ?? thread.event
self.parent_events = damus.events.parent_events(event: selected_event, keypair: damus.keypair)
let all_recursive_child_events = self.recursive_child_events(event: selected_event)
self.sorted_child_events = all_recursive_child_events.filter({
should_show_event(event: $0, damus_state: damus) // Hide muted events from chatroom conversation
}).sorted(by: { a, b in
return a.created_at < b.created_at
})
}
func recursive_child_events(event: NdbNote) -> [NdbNote] {
let immediate_children = damus.events.child_events(event: event)
var indirect_children: [NdbNote] = []
for immediate_child in immediate_children {
indirect_children.append(contentsOf: self.recursive_child_events(event: immediate_child))
}
return immediate_children + indirect_children
}
func go_to_event(scroller: ScrollViewProxy, note_id: NoteId) {
scroll_to_event(scroller: scroller, id: note_id, delay: 0, animate: true, anchor: .top)
selected_note_id = note_id
highlighted_note_id = note_id
DispatchQueue.main.asyncAfter(deadline: .now() + 0.5, execute: {
withAnimation {
selected_note_id = nil
highlighted_note_id = nil
}
})
}
func set_active_event(scroller: ScrollViewProxy, ev: NdbNote) {
withAnimation {
self.compute_events(selected_event: ev)
thread.set_active_event(ev, keypair: self.damus.keypair)
self.thread.select(event: ev)
self.go_to_event(scroller: scroller, note_id: ev.id)
}
}
@@ -63,7 +40,7 @@ struct ChatroomThreadView: View {
ScrollView(.vertical) {
LazyVStack(alignment: .leading, spacing: 8) {
// MARK: - Parents events view
ForEach(parent_events, id: \.id) { parent_event in
ForEach(thread.parent_events, id: \.id) { parent_event in
EventMutingContainerView(damus_state: damus, event: parent_event) {
EventView(damus: damus, event: parent_event)
.matchedGeometryEffect(id: parent_event.id.hex(), in: animation, anchor: .center)
@@ -93,7 +70,7 @@ struct ChatroomThreadView: View {
// MARK: - Actual event view
EventMutingContainerView(
damus_state: damus,
event: self.thread.event,
event: self.thread.selected_event,
muteBox: { event_shown, muted_reason in
AnyView(
EventMutedBoxView(shown: event_shown, reason: muted_reason)
@@ -101,19 +78,19 @@ struct ChatroomThreadView: View {
)
}
) {
SelectedEventView(damus: damus, event: self.thread.event, size: .selected)
.matchedGeometryEffect(id: self.thread.event.id.hex(), in: animation, anchor: .center)
SelectedEventView(damus: damus, event: self.thread.selected_event, size: .selected)
.matchedGeometryEffect(id: self.thread.selected_event.id.hex(), in: animation, anchor: .center)
}
.id(self.thread.event.id)
.id(self.thread.selected_event.id)
// MARK: - Children view
let events = sorted_child_events
let events = thread.sorted_child_events
let count = events.count
SwipeViewGroup {
ForEach(Array(zip(events, events.indices)), id: \.0.id) { (ev, ind) in
ChatEventView(event: events[ind],
selected_event: self.thread.event,
selected_event: self.thread.selected_event,
prev_ev: ind > 0 ? events[ind-1] : nil,
next_ev: ind == count-1 ? nil : events[ind+1],
damus_state: damus,
@@ -124,7 +101,7 @@ struct ChatroomThreadView: View {
focus_event: {
self.set_active_event(scroller: scroller, ev: ev)
},
highlight_bubble: selected_note_id == ev.id,
highlight_bubble: highlighted_note_id == ev.id,
bar: make_actionbar_model(ev: ev.id, damus: damus)
)
.padding(.horizontal)
@@ -148,16 +125,14 @@ struct ChatroomThreadView: View {
}
})
.onReceive(thread.objectWillChange) {
self.compute_events()
if let last_event = thread.events().last, last_event.pubkey == damus.pubkey, user_just_posted_flag {
if let last_event = thread.events.last, last_event.pubkey == damus.pubkey, user_just_posted_flag {
self.go_to_event(scroller: scroller, note_id: last_event.id)
user_just_posted_flag = false
}
}
.onAppear() {
thread.subscribe()
self.compute_events()
scroll_to_event(scroller: scroller, id: thread.event.id, delay: 0.1, animate: false)
scroll_to_event(scroller: scroller, id: thread.selected_event.id, delay: 0.1, animate: false)
}
.onDisappear() {
thread.unsubscribe()
@@ -193,6 +168,7 @@ struct ChatroomView_Previews: PreviewProvider {
}
}
@MainActor
func scroll_after_load(thread: ThreadModel, proxy: ScrollViewProxy) {
scroll_to_event(scroller: proxy, id: thread.event.id, delay: 0.1, animate: false)
scroll_to_event(scroller: proxy, id: thread.selected_event.id, delay: 0.1, animate: false)
}

View File

@@ -39,7 +39,7 @@ struct EventBody: View {
HighlightBodyView(state: damus_state, ev: event, options: options)
.onTapGesture {
if let highlighted_note = event.highlighted_note_id().flatMap({ damus_state.events.lookup($0) }) {
let thread = ThreadModel(event: highlighted_note, damus_state: damus_state, highlight: event.content)
let thread = ThreadModel(event: highlighted_note, damus_state: damus_state)
damus_state.nav.push(route: Route.Thread(thread: thread))
}
}

View File

@@ -49,10 +49,10 @@ class LoadableThreadModel: ObservableObject {
case .note_id(let note_id):
let res = await find_event(state: damus_state, query: .event(evid: note_id))
guard let res, case .event(let ev) = res else { return .not_found }
return .loaded(model: ThreadModel(event: ev, damus_state: damus_state))
return .loaded(model: await ThreadModel(event: ev, damus_state: damus_state))
case .naddr(let naddr):
guard let event = await naddrLookup(damus_state: damus_state, naddr: naddr) else { return .not_found }
return .loaded(model: ThreadModel(event: event, damus_state: damus_state))
return .loaded(model: await ThreadModel(event: event, damus_state: damus_state))
}
}

View File

@@ -1,130 +0,0 @@
//
// ThreadV2View.swift
// damus
//
// Created by Thomas Tastet on 25/12/2022.
//
import SwiftUI
struct ThreadView: View {
let state: DamusState
@ObservedObject var thread: ThreadModel
@Environment(\.dismiss) var dismiss
var parent_events: [NostrEvent] {
state.events.parent_events(event: thread.event, keypair: state.keypair)
}
var sorted_child_events: [NostrEvent] {
state.events.child_events(event: thread.event).sorted(by: { a, b in
let a_is_muted = !should_show_event(event: a, damus_state: state)
let b_is_muted = !should_show_event(event: b, damus_state: state)
if a_is_muted == b_is_muted {
// If both are muted or unmuted, sort them based on their creation date.
return a.created_at < b.created_at
}
else {
// Muting status is different
// Prioritize the replies that are not muted
return !a_is_muted && b_is_muted
}
})
}
var body: some View {
//let top_zap = get_top_zap(events: state.events, evid: thread.event.id)
ScrollViewReader { reader in
ScrollView {
LazyVStack {
// MARK: - Parents events view
ForEach(parent_events, id: \.id) { parent_event in
EventMutingContainerView(damus_state: state, event: parent_event) {
EventView(damus: state, event: parent_event)
}
.padding(.horizontal)
.onTapGesture {
thread.set_active_event(parent_event, keypair: self.state.keypair)
scroll_to_event(scroller: reader, id: parent_event.id, delay: 0.1, animate: false)
}
Divider()
.padding(.top, 4)
.padding(.leading, 25 * 2)
}.background(GeometryReader { geometry in
// get the height and width of the EventView view
let eventHeight = geometry.frame(in: .global).height
// let eventWidth = geometry.frame(in: .global).width
// vertical gray line in the background
Rectangle()
.fill(Color.gray.opacity(0.25))
.frame(width: 2, height: eventHeight)
.offset(x: 40, y: 40)
})
// MARK: - Actual event view
EventMutingContainerView(
damus_state: state,
event: self.thread.event,
muteBox: { event_shown, muted_reason in
AnyView(
EventMutedBoxView(shown: event_shown, reason: muted_reason)
.padding(5)
)
}
) {
SelectedEventView(damus: state, event: self.thread.event, size: .selected)
}
.id(self.thread.event.id)
/*
if let top_zap {
ZapEvent(damus: state, zap: top_zap, is_top_zap: true)
.padding(.horizontal)
}
*/
ForEach(sorted_child_events, id: \.id) { child_event in
EventMutingContainerView(
damus_state: state,
event: child_event
) {
EventView(damus: state, event: child_event)
}
.padding(.horizontal)
.onTapGesture {
thread.set_active_event(child_event, keypair: state.keypair)
scroll_to_event(scroller: reader, id: child_event.id, delay: 0.1, animate: false)
}
Divider()
.padding([.top], 4)
}
}
}.navigationBarTitle(NSLocalizedString("Thread", comment: "Navigation bar title for note thread."))
.onAppear {
thread.subscribe()
let anchor: UnitPoint = self.thread.event.known_kind == .longform ? .top : .bottom
scroll_to_event(scroller: reader, id: self.thread.event.id, delay: 0.0, animate: false, anchor: anchor)
}
.onDisappear {
thread.unsubscribe()
}
.onReceive(handle_notify(.switched_timeline)) { notif in
dismiss()
}
}
}
}
struct ThreadView_Previews: PreviewProvider {
static var previews: some View {
let state = test_damus_state
let thread = ThreadModel(event: test_note, damus_state: state)
ThreadView(state: state, thread: thread)
}
}