Location notes: fix performance and UI issues (#774)

* Location notes: fix performance and UI issues

Performance fixes:
- Add Set-based duplicate detection (O(1) vs O(n) lookup)
- Eliminates lag when receiving 200+ notes during EOSE

Correctness fixes:
- Fix optimistic echo timestamp to match signed event timestamp
- Add echo IDs to noteIDs set for consistency

UI improvements:
- Remove duplicate "loading recent notes" text in header
- Simplify toolbar icon color logic for immediate green indication
- Icon now reliably turns green when notes exist in geohash

Tests: All 3 LocationNotes tests passing

* Location notes: add remaining robustness fixes

Additional improvements:
- Align counter/manager limits to 200 (prevents showing count higher than displayable)
- Set loading state before clearing notes to eliminate UI flicker on geohash change
- Add geohash validation for building-level precision (8 valid base32 chars)
- Add defensive 500-note memory cap with automatic trimming
- Clear stale notesGeohash state on sheet dismiss

Tests:
- Fix test geohashes to use valid base32 characters
- All 3 LocationNotes tests passing

---------

Co-authored-by: jack <jackjackbits@users.noreply.github.com>
This commit is contained in:
jack
2025-10-07 12:36:54 +02:00
committed by GitHub
co-authored by jack
parent c583949031
commit 0f23ed0a99
6 changed files with 63 additions and 18 deletions
+8
View File
@@ -10,6 +10,14 @@ enum Geohash {
return map return map
}() }()
/// Validates a geohash string for building-level precision (8 characters).
/// - Parameter geohash: The geohash string to validate
/// - Returns: true if valid 8-character base32 geohash, false otherwise
static func isValidBuildingGeohash(_ geohash: String) -> Bool {
guard geohash.count == 8 else { return false }
return geohash.lowercased().allSatisfy { base32Map[$0] != nil }
}
/// Encodes the provided coordinates into a geohash string. /// Encodes the provided coordinates into a geohash string.
/// - Parameters: /// - Parameters:
/// - latitude: Latitude in degrees (-90...90) /// - latitude: Latitude in degrees (-90...90)
+6 -1
View File
@@ -54,6 +54,11 @@ final class LocationNotesCounter: ObservableObject {
func subscribe(geohash gh: String) { func subscribe(geohash gh: String) {
let norm = gh.lowercased() let norm = gh.lowercased()
if geohash == norm, subscriptionID != nil { return } if geohash == norm, subscriptionID != nil { return }
// Validate geohash (building-level precision: 8 chars)
guard Geohash.isValidBuildingGeohash(norm) else {
SecureLogger.warning("LocationNotesCounter: rejecting invalid geohash '\(norm)' (expected 8 valid base32 chars)", category: .session)
return
}
// Unsubscribe previous without clearing count to avoid flicker // Unsubscribe previous without clearing count to avoid flicker
if let sub = subscriptionID { dependencies.unsubscribe(sub) } if let sub = subscriptionID { dependencies.unsubscribe(sub) }
subscriptionID = nil subscriptionID = nil
@@ -74,7 +79,7 @@ final class LocationNotesCounter: ObservableObject {
} }
subscriptionID = subID subscriptionID = subID
let filter = NostrFilter.geohashNotes(norm, since: nil, limit: 500) let filter = NostrFilter.geohashNotes(norm, since: nil, limit: 200)
dependencies.subscribe(filter, subID, relays, { [weak self] event in dependencies.subscribe(filter, subID, relays, { [weak self] event in
guard let self = self else { return } guard let self = self else { return }
guard event.kind == NostrProtocol.EventKind.textNote.rawValue else { return } guard event.kind == NostrProtocol.EventKind.textNote.rawValue else { return }
+38 -3
View File
@@ -74,7 +74,9 @@ final class LocationNotesManager: ObservableObject {
@Published private(set) var state: State = .loading @Published private(set) var state: State = .loading
@Published private(set) var errorMessage: String? @Published private(set) var errorMessage: String?
private var subscriptionID: String? private var subscriptionID: String?
private var noteIDs = Set<String>() // O(1) duplicate detection
private let dependencies: LocationNotesDependencies private let dependencies: LocationNotesDependencies
private let maxNotesInMemory = 500 // Defensive cap (relay limit is 200)
private enum Strings { private enum Strings {
static let noRelays = String(localized: "location_notes.error.no_relays", comment: "Shown when no geo relays are available near the selected location") static let noRelays = String(localized: "location_notes.error.no_relays", comment: "Shown when no geo relays are available near the selected location")
@@ -89,20 +91,35 @@ final class LocationNotesManager: ObservableObject {
} }
init(geohash: String, dependencies: LocationNotesDependencies = .live) { init(geohash: String, dependencies: LocationNotesDependencies = .live) {
self.geohash = geohash.lowercased() let norm = geohash.lowercased()
self.geohash = norm
self.dependencies = dependencies self.dependencies = dependencies
// Validate geohash (building-level precision: 8 chars)
if !Geohash.isValidBuildingGeohash(norm) {
SecureLogger.warning("LocationNotesManager: invalid geohash '\(norm)' (expected 8 valid base32 chars)", category: .session)
}
subscribe() subscribe()
} }
func setGeohash(_ newGeohash: String) { func setGeohash(_ newGeohash: String) {
let norm = newGeohash.lowercased() let norm = newGeohash.lowercased()
guard norm != geohash else { return } guard norm != geohash else { return }
// Validate geohash (building-level precision: 8 chars)
guard Geohash.isValidBuildingGeohash(norm) else {
SecureLogger.warning("LocationNotesManager: rejecting invalid geohash '\(norm)' (expected 8 valid base32 chars)", category: .session)
return
}
if let sub = subscriptionID { if let sub = subscriptionID {
dependencies.unsubscribe(sub) dependencies.unsubscribe(sub)
subscriptionID = nil subscriptionID = nil
} }
// Set loading state before clearing to prevent empty state flicker
state = .loading
initialLoadComplete = false
errorMessage = nil
geohash = norm geohash = norm
notes.removeAll() notes.removeAll()
noteIDs.removeAll()
subscribe() subscribe()
} }
@@ -111,7 +128,12 @@ final class LocationNotesManager: ObservableObject {
dependencies.unsubscribe(sub) dependencies.unsubscribe(sub)
subscriptionID = nil subscriptionID = nil
} }
// Set loading state before clearing to prevent empty state flicker
state = .loading
initialLoadComplete = false
errorMessage = nil
notes.removeAll() notes.removeAll()
noteIDs.removeAll()
subscribe() subscribe()
} }
@@ -147,12 +169,14 @@ final class LocationNotesManager: ObservableObject {
guard event.kind == NostrProtocol.EventKind.textNote.rawValue else { return } guard event.kind == NostrProtocol.EventKind.textNote.rawValue else { return }
// Ensure matching tag // Ensure matching tag
guard event.tags.contains(where: { $0.count >= 2 && $0[0].lowercased() == "g" && $0[1].lowercased() == self.geohash }) else { return } guard event.tags.contains(where: { $0.count >= 2 && $0[0].lowercased() == "g" && $0[1].lowercased() == self.geohash }) else { return }
if self.notes.contains(where: { $0.id == event.id }) { return } guard !self.noteIDs.contains(event.id) else { return }
self.noteIDs.insert(event.id)
let nick = event.tags.first(where: { $0.first?.lowercased() == "n" && $0.count >= 2 })?.dropFirst().first let nick = event.tags.first(where: { $0.first?.lowercased() == "n" && $0.count >= 2 })?.dropFirst().first
let ts = Date(timeIntervalSince1970: TimeInterval(event.created_at)) let ts = Date(timeIntervalSince1970: TimeInterval(event.created_at))
let note = Note(id: event.id, pubkey: event.pubkey, content: event.content, createdAt: ts, nickname: nick) let note = Note(id: event.id, pubkey: event.pubkey, content: event.content, createdAt: ts, nickname: nick)
self.notes.append(note) self.notes.append(note)
self.notes.sort { $0.createdAt > $1.createdAt } self.notes.sort { $0.createdAt > $1.createdAt }
self.enforceMemoryCap()
self.state = .ready self.state = .ready
}, { [weak self] in }, { [weak self] in
guard let self = self else { return } guard let self = self else { return }
@@ -188,10 +212,12 @@ final class LocationNotesManager: ObservableObject {
id: event.id, id: event.id,
pubkey: id.publicKeyHex, pubkey: id.publicKeyHex,
content: trimmed, content: trimmed,
createdAt: dependencies.now(), createdAt: Date(timeIntervalSince1970: TimeInterval(event.created_at)),
nickname: nickname nickname: nickname
) )
self.noteIDs.insert(event.id)
self.notes.insert(echo, at: 0) self.notes.insert(echo, at: 0)
self.enforceMemoryCap()
self.state = .ready self.state = .ready
self.errorMessage = nil self.errorMessage = nil
} catch { } catch {
@@ -200,6 +226,15 @@ final class LocationNotesManager: ObservableObject {
} }
} }
/// Enforces defensive memory cap on notes array (keeps newest).
private func enforceMemoryCap() {
if notes.count > maxNotesInMemory {
let removed = notes.count - maxNotesInMemory
notes = Array(notes.prefix(maxNotesInMemory))
SecureLogger.debug("LocationNotesManager: trimmed \(removed) old notes (cap: \(maxNotesInMemory))", category: .session)
}
}
/// Explicitly cancel subscription and release resources. /// Explicitly cancel subscription and release resources.
func cancel() { func cancel() {
if let sub = subscriptionID { if let sub = subscriptionID {
+4 -3
View File
@@ -1188,8 +1188,7 @@ struct ContentView: View {
showLocationNotes = true showLocationNotes = true
}) { }) {
HStack(alignment: .center, spacing: 4) { HStack(alignment: .center, spacing: 4) {
let currentCount = (notesCounter.count ?? 0) let hasNotes = (notesCounter.count ?? 0) > 0
let hasNotes = (!notesCounter.initialLoadComplete ? max(currentCount, sheetNotesCount) : currentCount) > 0
Image(systemName: "long.text.page.and.pencil") Image(systemName: "long.text.page.and.pencil")
.font(.bitchatSystem(size: 12)) .font(.bitchatSystem(size: 12))
.foregroundColor(hasNotes ? textColor : Color.gray) .foregroundColor(hasNotes ? textColor : Color.gray)
@@ -1290,7 +1289,9 @@ struct ContentView: View {
.onAppear { viewModel.isLocationChannelsSheetPresented = true } .onAppear { viewModel.isLocationChannelsSheetPresented = true }
.onDisappear { viewModel.isLocationChannelsSheetPresented = false } .onDisappear { viewModel.isLocationChannelsSheetPresented = false }
} }
.sheet(isPresented: $showLocationNotes) { .sheet(isPresented: $showLocationNotes, onDismiss: {
notesGeohash = nil
}) {
Group { Group {
if let gh = notesGeohash ?? LocationChannelManager.shared.availableChannels.first(where: { $0.level == .building })?.geohash { if let gh = notesGeohash ?? LocationChannelManager.shared.availableChannels.first(where: { $0.level == .building })?.geohash {
LocationNotesView(geohash: gh, onNotesCountChanged: { cnt in sheetNotesCount = cnt }) LocationNotesView(geohash: gh, onNotesCountChanged: { cnt in sheetNotesCount = cnt })
+1 -5
View File
@@ -125,11 +125,7 @@ struct LocationNotesView: View {
.font(.bitchatSystem(size: 12, design: .monospaced)) .font(.bitchatSystem(size: 12, design: .monospaced))
.foregroundColor(.secondary) .foregroundColor(.secondary)
.fixedSize(horizontal: false, vertical: true) .fixedSize(horizontal: false, vertical: true)
if manager.state == .loading && !manager.initialLoadComplete { if manager.state == .noRelays {
Text(Strings.loadingRecent)
.font(.bitchatSystem(size: 11, design: .monospaced))
.foregroundColor(.secondary)
} else if manager.state == .noRelays {
Text(Strings.relaysPaused) Text(Strings.relaysPaused)
.font(.bitchatSystem(size: 11, design: .monospaced)) .font(.bitchatSystem(size: 11, design: .monospaced))
.foregroundColor(.secondary) .foregroundColor(.secondary)
+6 -6
View File
@@ -16,7 +16,7 @@ final class LocationNotesManagerTests: XCTestCase {
// now: { Date() } // now: { Date() }
// ) // )
// //
// let manager = LocationNotesManager(geohash: "abcd1234", dependencies: deps) // let manager = LocationNotesManager(geohash: "u4pruydq", dependencies: deps)
// //
// XCTAssertFalse(subscribeCalled) // XCTAssertFalse(subscribeCalled)
// XCTAssertEqual(manager.state, .noRelays) // XCTAssertEqual(manager.state, .noRelays)
@@ -66,7 +66,7 @@ final class LocationNotesManagerTests: XCTestCase {
now: { Date() } now: { Date() }
) )
let manager = LocationNotesManager(geohash: "abcd1234", dependencies: deps) let manager = LocationNotesManager(geohash: "u4pruydq", dependencies: deps)
XCTAssertEqual(relaysCaptured, ["wss://relay.one"]) XCTAssertEqual(relaysCaptured, ["wss://relay.one"])
XCTAssertEqual(manager.state, .loading) XCTAssertEqual(manager.state, .loading)
@@ -74,7 +74,7 @@ final class LocationNotesManagerTests: XCTestCase {
pubkey: "pub", pubkey: "pub",
createdAt: Date(), createdAt: Date(),
kind: .textNote, kind: .textNote,
tags: [["g", "abcd1234"]], tags: [["g", "u4pruydq"]],
content: "hi" content: "hi"
) )
event.id = "event1" event.id = "event1"
@@ -102,7 +102,7 @@ final class LocationNotesCounterTests: XCTestCase {
) )
let counter = LocationNotesCounter(testDependencies: deps) let counter = LocationNotesCounter(testDependencies: deps)
counter.subscribe(geohash: "abcdefgh") counter.subscribe(geohash: "u4pruydq")
XCTAssertFalse(subscribeCalled) XCTAssertFalse(subscribeCalled)
XCTAssertFalse(counter.relayAvailable) XCTAssertFalse(counter.relayAvailable)
@@ -126,13 +126,13 @@ final class LocationNotesCounterTests: XCTestCase {
) )
let counter = LocationNotesCounter(testDependencies: deps) let counter = LocationNotesCounter(testDependencies: deps)
counter.subscribe(geohash: "abcdefgh") counter.subscribe(geohash: "u4pruydq")
var first = NostrEvent( var first = NostrEvent(
pubkey: "pub", pubkey: "pub",
createdAt: Date(), createdAt: Date(),
kind: .textNote, kind: .textNote,
tags: [["g", "abcdefgh"]], tags: [["g", "u4pruydq"]],
content: "a" content: "a"
) )
first.id = "eventA" first.id = "eventA"