From 0f23ed0a99c9065141b5dd92aafc3d70c8dc93d1 Mon Sep 17 00:00:00 2001 From: jack <212554440+jackjackbits@users.noreply.github.com> Date: Tue, 7 Oct 2025 12:36:54 +0200 Subject: [PATCH] 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 --- bitchat/Protocols/Geohash.swift | 8 ++++ bitchat/Services/LocationNotesCounter.swift | 7 +++- bitchat/Services/LocationNotesManager.swift | 41 ++++++++++++++++++-- bitchat/Views/ContentView.swift | 7 ++-- bitchat/Views/LocationNotesView.swift | 6 +-- bitchatTests/LocationNotesManagerTests.swift | 12 +++--- 6 files changed, 63 insertions(+), 18 deletions(-) diff --git a/bitchat/Protocols/Geohash.swift b/bitchat/Protocols/Geohash.swift index bdd0a299..e5576775 100644 --- a/bitchat/Protocols/Geohash.swift +++ b/bitchat/Protocols/Geohash.swift @@ -10,6 +10,14 @@ enum Geohash { 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. /// - Parameters: /// - latitude: Latitude in degrees (-90...90) diff --git a/bitchat/Services/LocationNotesCounter.swift b/bitchat/Services/LocationNotesCounter.swift index b82d75fa..b3c32118 100644 --- a/bitchat/Services/LocationNotesCounter.swift +++ b/bitchat/Services/LocationNotesCounter.swift @@ -54,6 +54,11 @@ final class LocationNotesCounter: ObservableObject { func subscribe(geohash gh: String) { let norm = gh.lowercased() 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 if let sub = subscriptionID { dependencies.unsubscribe(sub) } subscriptionID = nil @@ -74,7 +79,7 @@ final class LocationNotesCounter: ObservableObject { } 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 guard let self = self else { return } guard event.kind == NostrProtocol.EventKind.textNote.rawValue else { return } diff --git a/bitchat/Services/LocationNotesManager.swift b/bitchat/Services/LocationNotesManager.swift index ca57a511..fba6f993 100644 --- a/bitchat/Services/LocationNotesManager.swift +++ b/bitchat/Services/LocationNotesManager.swift @@ -74,7 +74,9 @@ final class LocationNotesManager: ObservableObject { @Published private(set) var state: State = .loading @Published private(set) var errorMessage: String? private var subscriptionID: String? + private var noteIDs = Set() // O(1) duplicate detection private let dependencies: LocationNotesDependencies + private let maxNotesInMemory = 500 // Defensive cap (relay limit is 200) 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") @@ -89,20 +91,35 @@ final class LocationNotesManager: ObservableObject { } init(geohash: String, dependencies: LocationNotesDependencies = .live) { - self.geohash = geohash.lowercased() + let norm = geohash.lowercased() + self.geohash = norm 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() } func setGeohash(_ newGeohash: String) { let norm = newGeohash.lowercased() 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 { dependencies.unsubscribe(sub) subscriptionID = nil } + // Set loading state before clearing to prevent empty state flicker + state = .loading + initialLoadComplete = false + errorMessage = nil geohash = norm notes.removeAll() + noteIDs.removeAll() subscribe() } @@ -111,7 +128,12 @@ final class LocationNotesManager: ObservableObject { dependencies.unsubscribe(sub) subscriptionID = nil } + // Set loading state before clearing to prevent empty state flicker + state = .loading + initialLoadComplete = false + errorMessage = nil notes.removeAll() + noteIDs.removeAll() subscribe() } @@ -147,12 +169,14 @@ final class LocationNotesManager: ObservableObject { guard event.kind == NostrProtocol.EventKind.textNote.rawValue else { return } // Ensure matching tag 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 ts = Date(timeIntervalSince1970: TimeInterval(event.created_at)) let note = Note(id: event.id, pubkey: event.pubkey, content: event.content, createdAt: ts, nickname: nick) self.notes.append(note) self.notes.sort { $0.createdAt > $1.createdAt } + self.enforceMemoryCap() self.state = .ready }, { [weak self] in guard let self = self else { return } @@ -188,10 +212,12 @@ final class LocationNotesManager: ObservableObject { id: event.id, pubkey: id.publicKeyHex, content: trimmed, - createdAt: dependencies.now(), + createdAt: Date(timeIntervalSince1970: TimeInterval(event.created_at)), nickname: nickname ) + self.noteIDs.insert(event.id) self.notes.insert(echo, at: 0) + self.enforceMemoryCap() self.state = .ready self.errorMessage = nil } 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. func cancel() { if let sub = subscriptionID { diff --git a/bitchat/Views/ContentView.swift b/bitchat/Views/ContentView.swift index f57e2176..3bd6f302 100644 --- a/bitchat/Views/ContentView.swift +++ b/bitchat/Views/ContentView.swift @@ -1188,8 +1188,7 @@ struct ContentView: View { showLocationNotes = true }) { HStack(alignment: .center, spacing: 4) { - let currentCount = (notesCounter.count ?? 0) - let hasNotes = (!notesCounter.initialLoadComplete ? max(currentCount, sheetNotesCount) : currentCount) > 0 + let hasNotes = (notesCounter.count ?? 0) > 0 Image(systemName: "long.text.page.and.pencil") .font(.bitchatSystem(size: 12)) .foregroundColor(hasNotes ? textColor : Color.gray) @@ -1290,7 +1289,9 @@ struct ContentView: View { .onAppear { viewModel.isLocationChannelsSheetPresented = true } .onDisappear { viewModel.isLocationChannelsSheetPresented = false } } - .sheet(isPresented: $showLocationNotes) { + .sheet(isPresented: $showLocationNotes, onDismiss: { + notesGeohash = nil + }) { Group { if let gh = notesGeohash ?? LocationChannelManager.shared.availableChannels.first(where: { $0.level == .building })?.geohash { LocationNotesView(geohash: gh, onNotesCountChanged: { cnt in sheetNotesCount = cnt }) diff --git a/bitchat/Views/LocationNotesView.swift b/bitchat/Views/LocationNotesView.swift index a6c3c88f..8dbe901d 100644 --- a/bitchat/Views/LocationNotesView.swift +++ b/bitchat/Views/LocationNotesView.swift @@ -125,11 +125,7 @@ struct LocationNotesView: View { .font(.bitchatSystem(size: 12, design: .monospaced)) .foregroundColor(.secondary) .fixedSize(horizontal: false, vertical: true) - if manager.state == .loading && !manager.initialLoadComplete { - Text(Strings.loadingRecent) - .font(.bitchatSystem(size: 11, design: .monospaced)) - .foregroundColor(.secondary) - } else if manager.state == .noRelays { + if manager.state == .noRelays { Text(Strings.relaysPaused) .font(.bitchatSystem(size: 11, design: .monospaced)) .foregroundColor(.secondary) diff --git a/bitchatTests/LocationNotesManagerTests.swift b/bitchatTests/LocationNotesManagerTests.swift index 9170fcfa..e34fd023 100644 --- a/bitchatTests/LocationNotesManagerTests.swift +++ b/bitchatTests/LocationNotesManagerTests.swift @@ -16,7 +16,7 @@ final class LocationNotesManagerTests: XCTestCase { // now: { Date() } // ) // -// let manager = LocationNotesManager(geohash: "abcd1234", dependencies: deps) +// let manager = LocationNotesManager(geohash: "u4pruydq", dependencies: deps) // // XCTAssertFalse(subscribeCalled) // XCTAssertEqual(manager.state, .noRelays) @@ -66,7 +66,7 @@ final class LocationNotesManagerTests: XCTestCase { now: { Date() } ) - let manager = LocationNotesManager(geohash: "abcd1234", dependencies: deps) + let manager = LocationNotesManager(geohash: "u4pruydq", dependencies: deps) XCTAssertEqual(relaysCaptured, ["wss://relay.one"]) XCTAssertEqual(manager.state, .loading) @@ -74,7 +74,7 @@ final class LocationNotesManagerTests: XCTestCase { pubkey: "pub", createdAt: Date(), kind: .textNote, - tags: [["g", "abcd1234"]], + tags: [["g", "u4pruydq"]], content: "hi" ) event.id = "event1" @@ -102,7 +102,7 @@ final class LocationNotesCounterTests: XCTestCase { ) let counter = LocationNotesCounter(testDependencies: deps) - counter.subscribe(geohash: "abcdefgh") + counter.subscribe(geohash: "u4pruydq") XCTAssertFalse(subscribeCalled) XCTAssertFalse(counter.relayAvailable) @@ -126,13 +126,13 @@ final class LocationNotesCounterTests: XCTestCase { ) let counter = LocationNotesCounter(testDependencies: deps) - counter.subscribe(geohash: "abcdefgh") + counter.subscribe(geohash: "u4pruydq") var first = NostrEvent( pubkey: "pub", createdAt: Date(), kind: .textNote, - tags: [["g", "abcdefgh"]], + tags: [["g", "u4pruydq"]], content: "a" ) first.id = "eventA"