From 304460ee8376bc3bec8524ff6c2872a5a4eccb66 Mon Sep 17 00:00:00 2001 From: jack <212554440+jackjackbits@users.noreply.github.com> Date: Thu, 9 Jul 2026 16:47:08 +0200 Subject: [PATCH] Nearby notes: tap-to-reveal (consent-gate the location subscription) + subscription hygiene (#1422) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Nearby notes: tap-to-reveal before any relay REQ, plus shared subscription pool The nearby-notes counter used to open a live building-precision (precision-8) geohash REQ to the closest geo relays whenever the mesh public timeline was visible — a passive location side-channel with no opt-in. Now nothing subscribes until one explicit act reveals the counter for the session: tapping the new "check for notes left here" line on the empty mesh timeline (static, no network), opening the notices sheet's geo tab, or a successful /drop. The app-info setting stays the default-ON kill switch and still gates /drop. Subscription hygiene alongside: - LocationNotesManager.deinit now unsubscribes its live REQ (hopping to the main actor like the timer teardown) instead of only invalidating timers. - New refcounted LocationNotesPool dedupes the counter's and the notices sheet's identical 9-cell kind-1 REQs into one shared manager per geohash; both callers release-and-reacquire instead of retargeting in place, and the sheet releases its ref on dismissal. The reveal affordance is localized across all 29 catalog locales, and new NearbyNotesCounterTests cover the no-REQ-before-reveal contract, the 9-cell filter, NIP-40 expiry handling, single unsubscribe on deactivate, and pool refcounting. Co-Authored-By: Claude Opus 4.8 * Review fixes: permission-gate the hint, exclude building cell from pre-reveal sampling, explicit-act reveal only Fixes the verified review findings on the tap-to-reveal PR: - Permission dead-end: the "check for notes left here" hint now renders only when location permission is already authorized (it never prompts). Previously a location-denied install could tap it, flip the sticky revealed flag, and get nothing for the rest of the session because retarget() guards on authorization. Predicate lives on NearbyNotesCounter.offersRevealHint(permissionState:) and is reactive to the published permission state. - Building-cell sampling: background geohash sampling subscribed geo-sample- for every regional level including the precision-8 building cell, pre-reveal — contradicting the claim that nothing building-precision hits a relay before the explicit act. GeoChannelCoordinator now excludes the building level until NearbyNotesCounter.revealed (injectable publisher for tests); coarser levels keep the nearby-conversation hint and participant counts working, and bookmarks stay exempt (bookmarking is explicit). - Implicit reveal: opening the notices sheet no longer reveals — the sheet auto-lands on the geo tab whenever a location channel is selected, so browsing a remote geohash and opening notices revealed the LOCAL building subscription. reveal() now fires only on the person actively picking the geo segment, and only when the sheet has a geo scope (the empty-mesh hint tap and /drop stay as before). - Subscription hygiene: switching the sheet geo → mesh releases the pooled notes manager (the REQ was left streaming behind the mesh board); switching back re-acquires from the pool. The dismissal release stays balanced — liveGeoManager is nil after the tab-switch release. - VoiceOver: the hint button exposes the plain localized action text instead of the decorated "* 📍 … *" label. - Removed LocationNotesManager.setGeohash: zero callers, and calling it on a pooled instance would corrupt the pool's keying and refcounts. New tests: hint permission gate, explicit-geo-tab reveal contract, building-cell sampling exclusion before/after reveal, pool release/re-acquire round trip. Full suite (1467) green; iOS simulator build green. Co-Authored-By: Claude Fable 5 * Deflake peer-snapshot binding waits: longTimeout for the multi-hop pipeline CI failed once on initialization_bindsPeerSnapshotsIntoAllPeers: the snapshot -> allPeers binding crosses the mock transport's unstructured Task, UnifiedPeerService.updatePeers, a receive(on: main), and another Task { @MainActor } in bindPeerService — all contending with every parallel worker. On the failing runner the whole suite took 10.1s (usually ~4.4s locally), so the positive 5s defaultTimeout wait lost the race. That's exactly the case TestConstants.longTimeout documents; passing runs return as soon as the condition holds and never pay it. Not caused by the tap-to-reveal changes: nothing on that pipeline was touched, and 20 full parallel-suite loops each on the branch and on origin/main reproduce zero failures locally. The two sibling waits on the same pipeline in this file get the same timeout. Co-Authored-By: Claude Fable 5 --------- Co-authored-by: jack Co-authored-by: Claude Opus 4.8 --- bitchat/App/NearbyNotesCounter.swift | 52 ++- bitchat/Localizable.xcstrings | 180 ++++++++++ bitchat/Services/CommandProcessor.swift | 4 + bitchat/Services/LocationNotesManager.swift | 34 +- bitchat/Services/LocationNotesPool.swift | 61 ++++ .../ViewModels/GeoChannelCoordinator.swift | 41 ++- .../Views/Components/MeshEmptyStateView.swift | 33 ++ bitchat/Views/MessageListView.swift | 4 +- bitchat/Views/NoticesView.swift | 48 ++- bitchatTests/ChatViewModelTests.swift | 12 +- .../GeoChannelCoordinatorContextTests.swift | 39 +++ bitchatTests/LocationNotesManagerTests.swift | 21 -- bitchatTests/NearbyNotesCounterTests.swift | 309 ++++++++++++++++++ 13 files changed, 771 insertions(+), 67 deletions(-) create mode 100644 bitchat/Services/LocationNotesPool.swift create mode 100644 bitchatTests/NearbyNotesCounterTests.swift diff --git a/bitchat/App/NearbyNotesCounter.swift b/bitchat/App/NearbyNotesCounter.swift index 3c1f630a..abe9c18b 100644 --- a/bitchat/App/NearbyNotesCounter.swift +++ b/bitchat/App/NearbyNotesCounter.swift @@ -17,6 +17,12 @@ final class NearbyNotesCounter: ObservableObject { static let shared = NearbyNotesCounter() @Published private(set) var noteCount = 0 + /// Whether an explicit notes act (the empty-timeline "check for notes" + /// tap, opening the notices sheet's geo tab, or a successful /drop) has + /// unlocked the counter this session. Until then nothing subscribes: + /// merely looking at the mesh timeline must not open a building-precision + /// relay REQ that leaks location passively. + @Published private(set) var revealed = false private var manager: LocationNotesManager? private var managerCancellable: AnyCancellable? @@ -24,9 +30,38 @@ final class NearbyNotesCounter: ObservableObject { private var settingCancellable: AnyCancellable? private var activeHolders = 0 private let locationManager: LocationChannelManager + private let managerFactory: @MainActor (String) -> LocationNotesManager + private let releaseManager: @MainActor (LocationNotesManager?) -> Void - init(locationManager: LocationChannelManager = .shared) { + init( + locationManager: LocationChannelManager = .shared, + managerFactory: @escaping @MainActor (String) -> LocationNotesManager = { LocationNotesPool.shared.acquire($0) }, + releaseManager: @escaping @MainActor (LocationNotesManager?) -> Void = { LocationNotesPool.shared.release($0) } + ) { self.locationManager = locationManager + self.managerFactory = managerFactory + self.releaseManager = releaseManager + } + + /// Whether the empty-timeline "check for notes" hint should render. + /// The permission gate matters: `retarget()` never subscribes without + /// location authorization, so offering the hint to an unauthorized + /// install would be a silent dead-end — tap, `revealed` flips, the hint + /// vanishes, and nothing else happens for the session. The hint never + /// prompts; it simply stays hidden until permission exists. The caller + /// passes its own observed permission state so the hint re-renders when + /// authorization changes. + func offersRevealHint(permissionState: LocationChannelManager.PermissionState) -> Bool { + !revealed && LocationNotesSettings.enabled && permissionState == .authorized + } + + /// Marks the one explicit act that lets the counter subscribe. Sticky for + /// the rest of the session (the singleton's lifetime); `deactivate()` + /// deliberately does not reset it. + func reveal() { + guard !revealed else { return } + revealed = true + retarget() } /// Begins (or keeps) the notes subscription for the current building @@ -53,31 +88,36 @@ final class NearbyNotesCounter: ObservableObject { channelsCancellable = nil settingCancellable = nil managerCancellable = nil - manager?.cancel() + releaseManager(manager) manager = nil noteCount = 0 } private func retarget() { guard activeHolders > 0, + revealed, LocationNotesSettings.enabled, locationManager.permissionState == .authorized, let geohash = locationManager.availableChannels .first(where: { $0.level == .building })?.geohash else { managerCancellable = nil - manager?.cancel() + releaseManager(manager) manager = nil noteCount = 0 return } if let manager { - manager.setGeohash(geohash) - return + guard manager.geohash != geohash.lowercased() else { return } + // Pooled managers are shared; never retarget one in place — + // release the old cell and acquire the new one. + managerCancellable = nil + releaseManager(manager) + self.manager = nil } - let fresh = LocationNotesManager(geohash: geohash) + let fresh = managerFactory(geohash) manager = fresh managerCancellable = fresh.$notes .receive(on: DispatchQueue.main) diff --git a/bitchat/Localizable.xcstrings b/bitchat/Localizable.xcstrings index 797af0ef..80c3c11d 100644 --- a/bitchat/Localizable.xcstrings +++ b/bitchat/Localizable.xcstrings @@ -31595,6 +31595,186 @@ } } }, + "content.empty.check_notes" : { + "comment" : "Empty mesh timeline action that starts looking for notes left at this place; before tapping, no lookup runs", + "extractionState" : "manual", + "localizations" : { + "ar" : { + "stringUnit" : { + "state" : "translated", + "value" : "تحقّق من الملاحظات المتروكة هنا" + } + }, + "bn" : { + "stringUnit" : { + "state" : "translated", + "value" : "এখানে রাখা নোট আছে কি না দেখুন" + } + }, + "de" : { + "stringUnit" : { + "state" : "translated", + "value" : "nachsehen, ob hier notizen hinterlassen wurden" + } + }, + "en" : { + "stringUnit" : { + "state" : "translated", + "value" : "check for notes left here" + } + }, + "es" : { + "stringUnit" : { + "state" : "translated", + "value" : "buscar notas dejadas aquí" + } + }, + "fil" : { + "stringUnit" : { + "state" : "translated", + "value" : "tingnan kung may mga note na naiwan dito" + } + }, + "fr" : { + "stringUnit" : { + "state" : "translated", + "value" : "vérifier s'il y a des notes laissées ici" + } + }, + "he" : { + "stringUnit" : { + "state" : "translated", + "value" : "בדיקה אם הושארו כאן פתקים" + } + }, + "hi" : { + "stringUnit" : { + "state" : "translated", + "value" : "देखें कि यहाँ नोट छोड़े गए हैं या नहीं" + } + }, + "id" : { + "stringUnit" : { + "state" : "translated", + "value" : "periksa catatan yang ditinggalkan di sini" + } + }, + "it" : { + "stringUnit" : { + "state" : "translated", + "value" : "controlla se ci sono note lasciate qui" + } + }, + "ja" : { + "stringUnit" : { + "state" : "translated", + "value" : "ここに残されたメモを確認" + } + }, + "ko" : { + "stringUnit" : { + "state" : "translated", + "value" : "여기 남겨진 쪽지 확인" + } + }, + "ms" : { + "stringUnit" : { + "state" : "translated", + "value" : "semak nota yang ditinggalkan di sini" + } + }, + "ne" : { + "stringUnit" : { + "state" : "translated", + "value" : "यहाँ छोडिएका नोटहरू छन् कि हेर्नुहोस्" + } + }, + "nl" : { + "stringUnit" : { + "state" : "translated", + "value" : "kijk of hier notities zijn achtergelaten" + } + }, + "pl" : { + "stringUnit" : { + "state" : "translated", + "value" : "sprawdź, czy zostawiono tutaj notatki" + } + }, + "pt" : { + "stringUnit" : { + "state" : "translated", + "value" : "ver se há notas deixadas aqui" + } + }, + "pt-BR" : { + "stringUnit" : { + "state" : "translated", + "value" : "ver se há notas deixadas aqui" + } + }, + "ru" : { + "stringUnit" : { + "state" : "translated", + "value" : "проверить, есть ли здесь заметки" + } + }, + "sv" : { + "stringUnit" : { + "state" : "translated", + "value" : "kolla om anteckningar lämnats här" + } + }, + "ta" : { + "stringUnit" : { + "state" : "translated", + "value" : "இங்கே விடப்பட்ட குறிப்புகள் உள்ளதா எனப் பார்க்கவும்" + } + }, + "th" : { + "stringUnit" : { + "state" : "translated", + "value" : "ดูว่ามีโน้ตทิ้งไว้ที่นี่หรือไม่" + } + }, + "tr" : { + "stringUnit" : { + "state" : "translated", + "value" : "buraya bırakılan notlara bak" + } + }, + "uk" : { + "stringUnit" : { + "state" : "translated", + "value" : "перевірити, чи залишено тут нотатки" + } + }, + "ur" : { + "stringUnit" : { + "state" : "translated", + "value" : "دیکھیں کہ یہاں نوٹ چھوڑے گئے ہیں یا نہیں" + } + }, + "vi" : { + "stringUnit" : { + "state" : "translated", + "value" : "kiểm tra ghi chú để lại ở đây" + } + }, + "zh-Hans" : { + "stringUnit" : { + "state" : "translated", + "value" : "查看这里留下的留言" + } + }, + "zh-Hant" : { + "stringUnit" : { + "state" : "translated", + "value" : "查看這裡留下的留言" + } + } + } + }, "content.empty.location_intro" : { "comment" : "First line of an empty geohash timeline naming the channel", "extractionState" : "manual", diff --git a/bitchat/Services/CommandProcessor.swift b/bitchat/Services/CommandProcessor.swift index f2b007dd..a68bf65b 100644 --- a/bitchat/Services/CommandProcessor.swift +++ b/bitchat/Services/CommandProcessor.swift @@ -200,6 +200,10 @@ final class CommandProcessor { LocationNotesManager.postDrop(content: content, nickname: nickname, geohash: geohash) else { return .error(message: "no geo relays reachable — note not left") } + // Leaving a note is an explicit notes act: it unlocks the passive + // nearby-notes counter (tap-to-reveal) so the sender sees their own + // drop counted on the timeline. + NearbyNotesCounter.shared.reveal() return .success(message: "📍 note left here — it fades in 24h") } diff --git a/bitchat/Services/LocationNotesManager.swift b/bitchat/Services/LocationNotesManager.swift index a85a32bc..4657a7b3 100644 --- a/bitchat/Services/LocationNotesManager.swift +++ b/bitchat/Services/LocationNotesManager.swift @@ -173,6 +173,15 @@ final class LocationNotesManager: ObservableObject { deinit { expiryPruneTimer?.invalidate() connectivityRetryTimer?.invalidate() + // A live REQ must not outlive its manager: relays would keep + // streaming events nobody consumes. deinit is nonisolated, so hop to + // the main actor with just the captured closure and id. + if let sub = subscriptionID { + let unsubscribe = dependencies.unsubscribe + Task { @MainActor in + unsubscribe(sub) + } + } } /// Drops notes whose NIP-40 expiry has passed. Their ids stay in @@ -190,27 +199,10 @@ final class LocationNotesManager: ObservableObject { } } - func setGeohash(_ newGeohash: String) { - let norm = newGeohash.lowercased() - guard norm != geohash else { return } - guard Geohash.isValidGeohash(norm) else { - SecureLogger.warning("LocationNotesManager: rejecting invalid geohash '\(norm)' (expected 1-12 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 - ownPubkey = (try? dependencies.deriveIdentity(norm))?.publicKeyHex - notes.removeAll() - noteIDs.removeAll() - subscribe() - } + // A manager's geohash is fixed for its lifetime: instances are pooled + // per geohash (`LocationNotesPool`), so retargeting one in place would + // corrupt the pool's keying and refcounts. Release the manager and + // acquire the new cell instead. func refresh() { if let sub = subscriptionID { diff --git a/bitchat/Services/LocationNotesPool.swift b/bitchat/Services/LocationNotesPool.swift new file mode 100644 index 00000000..9a6f2ec5 --- /dev/null +++ b/bitchat/Services/LocationNotesPool.swift @@ -0,0 +1,61 @@ +// +// LocationNotesPool.swift +// bitchat +// +// Refcounted pool of LocationNotesManager instances keyed by geohash, so +// surfaces watching the same place (the nearby-notes counter and the notices +// sheet's geo tab) share one relay subscription instead of opening two +// identical 9-cell REQs. +// This is free and unencumbered software released into the public domain. +// + +import Foundation + +@MainActor +final class LocationNotesPool { + static let shared = LocationNotesPool() + + private var entries: [String: (manager: LocationNotesManager, refs: Int)] = [:] + private let makeManager: @MainActor (String) -> LocationNotesManager + + /// The factory is injectable so tests can pool managers built over stub + /// dependencies; live use derives one real manager per geohash. + init(makeManager: @escaping @MainActor (String) -> LocationNotesManager = { LocationNotesManager(geohash: $0) }) { + self.makeManager = makeManager + } + + /// Returns the shared manager for `geohash` (case-insensitive), creating + /// it on first acquire and reviving a cancelled one on re-acquire. + /// Callers must never `cancel` a pooled manager — release it and acquire + /// the new geohash instead. + func acquire(_ geohash: String) -> LocationNotesManager { + let key = geohash.lowercased() + if let entry = entries[key] { + entries[key] = (entry.manager, entry.refs + 1) + if entry.manager.state == .idle { + entry.manager.refresh() + } + return entry.manager + } + let manager = makeManager(key) + entries[key] = (manager, 1) + return manager + } + + /// Balances `acquire`: the last release cancels the subscription and + /// drops the entry. Releasing an instance the pool doesn't own (a + /// test-injected manager) degrades to a plain `cancel()`. + func release(_ manager: LocationNotesManager?) { + guard let manager else { return } + guard let entry = entries[manager.geohash], entry.manager === manager else { + manager.cancel() + return + } + if entry.refs <= 1 { + entries[manager.geohash] = nil + manager.cancel() + } else { + entries[manager.geohash] = (entry.manager, entry.refs - 1) + } + } +} diff --git a/bitchat/ViewModels/GeoChannelCoordinator.swift b/bitchat/ViewModels/GeoChannelCoordinator.swift index 51af663a..68536d01 100644 --- a/bitchat/ViewModels/GeoChannelCoordinator.swift +++ b/bitchat/ViewModels/GeoChannelCoordinator.swift @@ -37,25 +37,34 @@ final class GeoChannelCoordinator { private weak var context: (any GeoChannelContext)? private var cancellables = Set() - private var regionalGeohashes: [String] = [] + private var regionalChannels: [GeohashChannel] = [] private var bookmarkedGeohashes: [String] = [] + /// Mirrors `NearbyNotesCounter.revealed` (injectable for tests): the + /// session's one explicit notes act. Until it happens, background + /// sampling must not include the building-precision cell — see + /// `sampledRegionalGeohashes`. + private var notesRevealed = false + private let notesRevealedPublisher: AnyPublisher init( locationManager: LocationChannelManager? = nil, bookmarksStore: GeohashBookmarksStore? = nil, torManager: TorManager? = nil, + notesRevealed: AnyPublisher? = nil, context: any GeoChannelContext ) { self.locationManager = locationManager ?? Self.defaultLocationManager() self.bookmarksStore = bookmarksStore ?? GeohashBookmarksStore.shared self.torManager = torManager ?? Self.defaultTorManager() + self.notesRevealedPublisher = notesRevealed + ?? NearbyNotesCounter.shared.$revealed.eraseToAnyPublisher() self.context = context start() } func start() { - regionalGeohashes = locationManager.availableChannels.map { $0.geohash } + regionalChannels = locationManager.availableChannels bookmarkedGeohashes = bookmarksStore.bookmarks locationManager.$selectedChannel @@ -72,7 +81,18 @@ final class GeoChannelCoordinator { .receive(on: DispatchQueue.main) .sink { [weak self] channels in guard let self else { return } - self.regionalGeohashes = channels.map { $0.geohash } + self.regionalChannels = channels + self.updateSampling() + } + .store(in: &cancellables) + + // Revealing the nearby-notes counter is the session's explicit notes + // act; it widens sampling to include the building cell (below). + notesRevealedPublisher + .receive(on: DispatchQueue.main) + .sink { [weak self] revealed in + guard let self, self.notesRevealed != revealed else { return } + self.notesRevealed = revealed self.updateSampling() } .store(in: &cancellables) @@ -102,8 +122,21 @@ final class GeoChannelCoordinator { updateSampling() } + /// Regional geohashes eligible for background sampling. The + /// building-precision (precision-8) cell identifies a single address, so + /// sampling it passively would leak the same location signal the + /// nearby-notes tap-to-reveal exists to gate — it joins only after the + /// session's explicit notes act. The coarser levels (block and up) keep + /// the nearby-conversation hint and channel participant counts working. + /// Bookmarks are exempt: bookmarking a geohash is itself explicit. + private var sampledRegionalGeohashes: [String] { + regionalChannels + .filter { notesRevealed || $0.level != .building } + .map { $0.geohash } + } + private func updateSampling() { - let union = Array(Set(regionalGeohashes).union(bookmarkedGeohashes)) + let union = Array(Set(sampledRegionalGeohashes).union(bookmarkedGeohashes)) Task { @MainActor in guard !union.isEmpty else { context?.endGeohashSampling() diff --git a/bitchat/Views/Components/MeshEmptyStateView.swift b/bitchat/Views/Components/MeshEmptyStateView.swift index ef6e6219..a244d5f1 100644 --- a/bitchat/Views/Components/MeshEmptyStateView.swift +++ b/bitchat/Views/Components/MeshEmptyStateView.swift @@ -24,6 +24,7 @@ struct MeshEmptyStateView: View { @EnvironmentObject private var peerListModel: PeerListModel @ObservedObject private var activityTracker = GeohashChatActivityTracker.shared @ObservedObject private var sightingsTracker = MeshSightingsTracker.shared + @ObservedObject private var nearbyNotes = NearbyNotesCounter.shared @ThemedPalette private var palette @@ -38,6 +39,7 @@ struct MeshEmptyStateView: View { static let meshIntro = String(localized: "content.empty.mesh_intro", comment: "First line of the empty mesh timeline explaining what the mesh channel is") static let switchHint = String(localized: "content.empty.switch_hint", comment: "Empty timeline hint pointing at the channel switcher and the help screen") static let sightingsOne = String(localized: "content.empty.sightings_one", comment: "Empty mesh timeline stat when exactly one device came within range today") + static let checkNotes = String(localized: "content.empty.check_notes", comment: "Empty mesh timeline action that starts looking for notes left at this place; before tapping, no lookup runs") static func sightingsMany(_ count: Int) -> String { String( @@ -80,6 +82,9 @@ struct MeshEmptyStateView: View { if let conversation = nearbyConversation { conversationHint(conversation) } + if showsCheckNotesHint { + checkNotesHint + } } else { // The radar + tally already say "scanning, nobody yet", so // the narration stays to two lines with the live hint after @@ -89,6 +94,9 @@ struct MeshEmptyStateView: View { if let conversation = nearbyConversation { conversationHint(conversation) } + if showsCheckNotesHint { + checkNotesHint + } // The radar centers in whatever space is left below the // text — the flexible spacers split it evenly. @@ -127,6 +135,31 @@ private extension MeshEmptyStateView { activityTracker.mostActiveConversation(among: locationChannelsModel.availableChannels) } + /// Tap-to-reveal: the nearby-notes counter never subscribes on its own — + /// looking at the mesh timeline must not open a building-precision relay + /// REQ (a passive location side-channel). This static line is the one + /// explicit act that unlocks it; nothing touches the network until the + /// tap. It only renders when location permission is already granted + /// (the tap never prompts, so without permission it would dead-end + /// silently). Once revealed it yields to today's live strip and count, + /// and the app-info setting stays the kill switch. + var showsCheckNotesHint: Bool { + nearbyNotes.offersRevealHint(permissionState: locationChannelsModel.permissionState) + } + + var checkNotesHint: some View { + Button { + NearbyNotesCounter.shared.reveal() + } label: { + actionLine("📍 \(Strings.checkNotes)") + .contentShape(Rectangle()) + } + .buttonStyle(.plain) + // The visual label carries decorative asterisks and an emoji; expose + // just the localized action text to assistive tech. + .accessibilityLabel(Strings.checkNotes) + } + var sightingsText: String { sightingsTracker.todayCount == 1 ? Strings.sightingsOne diff --git a/bitchat/Views/MessageListView.swift b/bitchat/Views/MessageListView.swift index 605599ea..2742aebd 100644 --- a/bitchat/Views/MessageListView.swift +++ b/bitchat/Views/MessageListView.swift @@ -334,8 +334,10 @@ private extension MessageListView { .buttonStyle(.plain) } - /// The nearby-notes counter runs whenever the mesh public timeline is + /// The nearby-notes counter is held whenever the mesh public timeline is /// showing — the strip needs a live count before it can decide to exist. + /// Holding is not subscribing: nothing hits the relays until an explicit + /// act reveals the counter (tap-to-reveal). func updateNotesCounterHold() { let shouldHold = privatePeer == nil && locationChannelsModel.selectedChannel.isMesh guard shouldHold != holdsNotesCounter else { return } diff --git a/bitchat/Views/NoticesView.swift b/bitchat/Views/NoticesView.swift index 9093e171..8b3e7d90 100644 --- a/bitchat/Views/NoticesView.swift +++ b/bitchat/Views/NoticesView.swift @@ -40,8 +40,10 @@ struct NoticesView: View { /// Injected notes manager for tests; live use derives one per geohash. private let notesManager: LocationNotesManager? - /// Live manager owned by the sheet so the composer can post pure Nostr + /// Pooled manager held by the sheet so the composer can post pure Nostr /// notes (∞ expiry has no mesh-board copy) and the list can render them. + /// Acquired from `LocationNotesPool` (shared with the nearby-notes + /// counter, one REQ per geohash) and released on dismissal. @State private var liveGeoManager: LocationNotesManager? init( @@ -61,19 +63,33 @@ struct NoticesView: View { notesManager ?? liveGeoManager } - /// Creates (or retargets/revives) the sheet-owned notes manager for the + /// The one explicit act inside the sheet that unlocks the passive + /// nearby-notes counter: the person actively picking the geo segment + /// while the sheet has a geo scope. Landing on the geo tab via the + /// sheet's initial selection (auto-derived from the current channel — + /// e.g. browsing a remote geohash) is not an act toward the LOCAL + /// building cell and must not reveal it. + static func revealsNearbyNotes(onSwitchingTo tab: Tab, geoGeohash: String?) -> Bool { + tab == .geo && geoGeohash != nil + } + + /// Acquires (or retargets/revives) the pooled notes manager for the /// current geo scope. private func ensureGeoNotesManager() { - guard notesManager == nil, tab == .geo, let geohash = geoGeohash else { return } + guard tab == .geo else { return } + guard notesManager == nil, let geohash = geoGeohash else { return } if let manager = liveGeoManager { if manager.geohash != geohash.lowercased() { - manager.setGeohash(geohash) + // Pooled managers are shared; never retarget one in place — + // release the old cell and acquire the new one. + LocationNotesPool.shared.release(manager) + liveGeoManager = LocationNotesPool.shared.acquire(geohash) } else if manager.state == .idle { - // Cancelled on a tab switch; returning re-subscribes. + // Revived after a cancel elsewhere; returning re-subscribes. manager.refresh() } } else { - liveGeoManager = LocationNotesManager(geohash: geohash) + liveGeoManager = LocationNotesPool.shared.acquire(geohash) } } @@ -180,9 +196,19 @@ struct NoticesView: View { .onChange(of: tab) { newTab in if newTab == .geo { beginGeoLocationIfNeeded() + if Self.revealsNearbyNotes(onSwitchingTo: newTab, geoGeohash: geoGeohash) { + NearbyNotesCounter.shared.reveal() + } ensureGeoNotesManager() } else { locationChannelsModel.endLiveRefresh() + // Leaving the geo tab must take its REQ down with it, not + // leave the geohash subscription streaming behind the mesh + // board. The pool makes the return trip cheap (re-acquire + // revives the manager), and the dismissal release below is + // safe: `liveGeoManager` is nil until re-acquired. + LocationNotesPool.shared.release(liveGeoManager) + liveGeoManager = nil } // Each tab keeps its natural default: geo notes stay until // deleted (∞), mesh board posts fade within a week. @@ -196,7 +222,11 @@ struct NoticesView: View { .onChange(of: geoGeohash) { _ in ensureGeoNotesManager() } - .onDisappear { locationChannelsModel.endLiveRefresh() } + .onDisappear { + locationChannelsModel.endLiveRefresh() + LocationNotesPool.shared.release(liveGeoManager) + liveGeoManager = nil + } } /// The geo tab tracks the device's building geohash while on mesh; keep @@ -417,10 +447,6 @@ private struct GeoNoticesList: View { board: board, notesManager: notesManager ) - .onChange(of: geohash) { newValue in - notesManager.setGeohash(newValue) - } - .onDisappear { notesManager.cancel() } } } diff --git a/bitchatTests/ChatViewModelTests.swift b/bitchatTests/ChatViewModelTests.swift index 5a50f0ac..8b9afcef 100644 --- a/bitchatTests/ChatViewModelTests.swift +++ b/bitchatTests/ChatViewModelTests.swift @@ -85,9 +85,14 @@ struct ChatViewModelInitializationTests { ) ]) + // The snapshot → allPeers binding hops the transport's unstructured + // Task, UnifiedPeerService, a receive(on: main), and another Task — + // all contending with every parallel worker, so a loaded CI runner + // can exceed defaultTimeout (observed: one 5s miss on a run where + // the whole suite took 10s instead of the usual ~4s). let updated = await TestHelpers.waitUntil({ viewModel.allPeers.contains { $0.peerID == peerID && $0.nickname == "Alice" } - }, timeout: TestConstants.defaultTimeout) + }, timeout: TestConstants.longTimeout) #expect(updated) } @@ -119,9 +124,10 @@ struct ChatViewModelIdentityTests { ) ]) + // Same multi-hop snapshot pipeline as above: longTimeout for load. let oldPeerBound = await TestHelpers.waitUntil({ viewModel.connectedPeers.contains(oldPeerID) - }, timeout: TestConstants.defaultTimeout) + }, timeout: TestConstants.longTimeout) #expect(oldPeerBound) let existingMessage = BitchatMessage( @@ -155,7 +161,7 @@ struct ChatViewModelIdentityTests { let newPeerBound = await TestHelpers.waitUntil({ viewModel.connectedPeers.contains(newPeerID) && !viewModel.connectedPeers.contains(oldPeerID) - }, timeout: TestConstants.defaultTimeout) + }, timeout: TestConstants.longTimeout) #expect(newPeerBound) viewModel.updatePrivateChatPeerIfNeeded() diff --git a/bitchatTests/GeoChannelCoordinatorContextTests.swift b/bitchatTests/GeoChannelCoordinatorContextTests.swift index 9345b341..271ff402 100644 --- a/bitchatTests/GeoChannelCoordinatorContextTests.swift +++ b/bitchatTests/GeoChannelCoordinatorContextTests.swift @@ -14,6 +14,7 @@ // import Testing +import Combine import Foundation import CoreLocation import Tor @@ -168,6 +169,44 @@ struct GeoChannelCoordinatorContextTests { #expect(await waitUntil { context.endSamplingCount > endCountBeforeRefresh }) } + @Test @MainActor + func buildingCellJoinsSamplingOnlyAfterNotesReveal() async { + TorManager.shared.setAppForeground(true) + let locationManager = makeLocationManager() + let context = MockGeoChannelContext() + let revealed = CurrentValueSubject(false) + let coordinator = GeoChannelCoordinator( + locationManager: locationManager, + bookmarksStore: locationManager, + torManager: TorManager.shared, + notesRevealed: revealed.eraseToAnyPublisher(), + context: context + ) + defer { withExtendedLifetime(coordinator) {} } + + // A location fix yields all six channel levels… + locationManager.locationManager( + CLLocationManager(), + didUpdateLocations: [CLLocation(latitude: 21.2850, longitude: -157.8357)] + ) + #expect(await waitUntil { + locationManager.availableChannels.contains { $0.level == .building } + }) + let building = locationManager.availableChannels.first { $0.level == .building }!.geohash + + // …but pre-reveal sampling must exclude the building-precision cell: + // a passive precision-8 REQ identifies a single address. + #expect(await waitUntil { + (context.beginSamplingCalls.last?.count ?? 0) == GeohashChannelLevel.allCases.count - 1 + }) + #expect(context.beginSamplingCalls.allSatisfy { !$0.contains(building) }) + + // The explicit notes act widens sampling to include it. + revealed.send(true) + #expect(await waitUntil { context.beginSamplingCalls.last?.contains(building) == true }) + #expect(context.beginSamplingCalls.last?.count == GeohashChannelLevel.allCases.count) + } + @Test @MainActor func releasedContext_isHeldWeaklyAndSafelyIgnored() async { let locationManager = makeLocationManager() diff --git a/bitchatTests/LocationNotesManagerTests.swift b/bitchatTests/LocationNotesManagerTests.swift index 80b8713d..abd16e3d 100644 --- a/bitchatTests/LocationNotesManagerTests.swift +++ b/bitchatTests/LocationNotesManagerTests.swift @@ -122,27 +122,6 @@ struct LocationNotesManagerTests { #expect(manager.notes.first?.content == "hi") } - @Test - func setGeohash_invalidValueIsIgnored() { - var subscribeCount = 0 - let deps = LocationNotesDependencies( - relayLookup: { _, _ in ["wss://relay.one"] }, - subscribe: { _, _, _, _, _ in - subscribeCount += 1 - }, - unsubscribe: { _ in }, - sendEvent: { _, _ in }, - deriveIdentity: { _ in try NostrIdentity.generate() }, - now: { Date() } - ) - - let manager = LocationNotesManager(geohash: "u4pruydq", dependencies: deps) - manager.setGeohash("not-valid") - - #expect(manager.geohash == "u4pruydq") - #expect(subscribeCount == 1) - } - @Test func refreshAndCancel_manageSubscriptions() { var subscribeIDs: [String] = [] diff --git a/bitchatTests/NearbyNotesCounterTests.swift b/bitchatTests/NearbyNotesCounterTests.swift new file mode 100644 index 00000000..36a31a61 --- /dev/null +++ b/bitchatTests/NearbyNotesCounterTests.swift @@ -0,0 +1,309 @@ +import Combine +import CoreLocation +import XCTest +@testable import bitchat + +/// Tap-to-reveal privacy contract: the nearby-notes counter must not open a +/// building-precision relay REQ until one explicit act calls `reveal()`, and +/// the pooled subscription must come up exactly once and go down exactly once. +@MainActor +final class NearbyNotesCounterTests: XCTestCase { + private var previousNotesEnabled: Any? + + override func setUp() { + super.setUp() + previousNotesEnabled = UserDefaults.standard.object(forKey: "locationNotes.enabled") + UserDefaults.standard.set(true, forKey: "locationNotes.enabled") + } + + override func tearDown() { + if let previous = previousNotesEnabled as? Bool { + UserDefaults.standard.set(previous, forKey: "locationNotes.enabled") + } else { + UserDefaults.standard.removeObject(forKey: "locationNotes.enabled") + } + super.tearDown() + } + + func test_counterOnlySubscribesAfterReveal_countsUnexpiredNotes_andUnsubscribesOnDeactivate() async throws { + let relays = SubscriptionRecorder() + let locationManager = try await makeAuthorizedLocationManager() + let buildingGeohash = try XCTUnwrap( + locationManager.availableChannels.first(where: { $0.level == .building })?.geohash + ) + + let counter = NearbyNotesCounter( + locationManager: locationManager, + managerFactory: { LocationNotesManager(geohash: $0, dependencies: relays.dependencies) }, + releaseManager: { $0?.cancel() } + ) + + counter.activate() + // Let the availableChannels replay and any queued retargets land: + // being active is not consent, so still no REQ. + try await Task.sleep(nanoseconds: 50_000_000) + XCTAssertEqual(relays.subscribeCount, 0) + XCTAssertFalse(counter.revealed) + + counter.reveal() + + XCTAssertTrue(counter.revealed) + XCTAssertEqual(relays.subscribeCount, 1) + let gTags = try XCTUnwrap(geohashTagFilter(of: XCTUnwrap(relays.lastFilter))) + XCTAssertEqual(gTags.count, 9) + XCTAssertEqual( + Set(gTags), + Set([buildingGeohash] + Geohash.neighbors(of: buildingGeohash)), + "REQ must cover the building geohash plus its 8 neighbors" + ) + + // An expired NIP-40 note must never count. + let identity = try NostrIdentity.generate() + let now = Date() + let expired = NostrEvent( + pubkey: identity.publicKeyHex, + createdAt: now.addingTimeInterval(-3600), + kind: .textNote, + tags: [["g", buildingGeohash], ["expiration", String(Int(now.timeIntervalSince1970) - 60)]], + content: "gone" + ) + relays.lastHandler?(try expired.sign(with: identity.schnorrSigningKey())) + try await Task.sleep(nanoseconds: 50_000_000) + XCTAssertEqual(counter.noteCount, 0) + + // A live matching kind-1 note drives the count to 1. + let live = NostrEvent( + pubkey: identity.publicKeyHex, + createdAt: now, + kind: .textNote, + tags: [["g", buildingGeohash]], + content: "still here" + ) + relays.lastHandler?(try live.sign(with: identity.schnorrSigningKey())) + let counted = await waitUntil { counter.noteCount == 1 } + XCTAssertTrue(counted) + // Still exactly one REQ after all the async retarget re-entries. + XCTAssertEqual(relays.subscribeCount, 1) + + counter.deactivate() + + XCTAssertEqual(relays.unsubscribeCount, 1) + XCTAssertEqual(counter.noteCount, 0) + } + + func test_checkNotesHint_requiresAuthorizedLocationPermission() { + let relays = SubscriptionRecorder() + let counter = NearbyNotesCounter( + locationManager: makeBareLocationManager(), + managerFactory: { LocationNotesManager(geohash: $0, dependencies: relays.dependencies) }, + releaseManager: { $0?.cancel() } + ) + + // An unauthorized install must never see the hint: the tap can't + // subscribe (retarget requires authorization) and must not prompt, + // so offering it would be a silent dead-end for the session. + XCTAssertFalse(counter.offersRevealHint(permissionState: .notDetermined)) + XCTAssertFalse(counter.offersRevealHint(permissionState: .denied)) + XCTAssertFalse(counter.offersRevealHint(permissionState: .restricted)) + XCTAssertTrue(counter.offersRevealHint(permissionState: .authorized)) + + // The app-info kill switch hides it too. + LocationNotesSettings.enabled = false + XCTAssertFalse(counter.offersRevealHint(permissionState: .authorized)) + LocationNotesSettings.enabled = true + + // Once revealed, the hint yields to the live strip and count. + counter.reveal() + XCTAssertFalse(counter.offersRevealHint(permissionState: .authorized)) + XCTAssertEqual(relays.subscribeCount, 0, "reveal without authorization must not open a REQ") + } + + func test_noticesSheet_revealsOnlyOnExplicitGeoTabSelectionWithScope() { + // The sheet reveals the local counter only when the person actively + // picks the geo tab and the sheet actually has a geo scope — + // auto-landing on geo (initial tab) never calls this path at all. + XCTAssertTrue(NoticesView.revealsNearbyNotes(onSwitchingTo: .geo, geoGeohash: "u4pruydq")) + XCTAssertFalse(NoticesView.revealsNearbyNotes(onSwitchingTo: .geo, geoGeohash: nil)) + XCTAssertFalse(NoticesView.revealsNearbyNotes(onSwitchingTo: .mesh, geoGeohash: "u4pruydq")) + } + + func test_pool_sharesOneManagerPerGeohash_andCancelsOnLastRelease() { + let relays = SubscriptionRecorder() + let pool = LocationNotesPool( + makeManager: { LocationNotesManager(geohash: $0, dependencies: relays.dependencies) } + ) + + let first = pool.acquire("u4pruydq") + let second = pool.acquire("U4PRUYDQ") + + XCTAssertTrue(first === second, "same geohash (case-insensitive) must share one manager") + XCTAssertEqual(relays.subscribeCount, 1) + + pool.release(first) + + XCTAssertEqual(relays.unsubscribeCount, 0, "first release keeps the shared REQ live") + XCTAssertNotEqual(first.state, .idle) + + pool.release(second) + + XCTAssertEqual(relays.unsubscribeCount, 1) + XCTAssertEqual(first.state, .idle) + + // An instance the pool never owned (test-injected) degrades to cancel. + let stray = LocationNotesManager(geohash: "u4pruydp", dependencies: relays.dependencies) + pool.release(stray) + XCTAssertEqual(relays.unsubscribeCount, 2) + XCTAssertEqual(stray.state, .idle) + } + + func test_pool_reacquireAfterFullRelease_bringsSubscriptionBackUp() { + let relays = SubscriptionRecorder() + let pool = LocationNotesPool( + makeManager: { LocationNotesManager(geohash: $0, dependencies: relays.dependencies) } + ) + + // The notices sheet's geo → mesh → geo cycle: switching to mesh + // releases (REQ goes down), switching back re-acquires (fresh REQ), + // with exactly one live REQ at any point. + let first = pool.acquire("u4pruydq") + XCTAssertEqual(relays.subscribeCount, 1) + + pool.release(first) + XCTAssertEqual(relays.unsubscribeCount, 1) + XCTAssertEqual(first.state, .idle) + + let second = pool.acquire("u4pruydq") + XCTAssertEqual(relays.subscribeCount, 2) + XCTAssertNotEqual(second.state, .idle) + + // Dismissal after the round trip releases once more — no double + // unsubscribe from the earlier tab-switch release. + pool.release(second) + XCTAssertEqual(relays.unsubscribeCount, 2) + } + + // MARK: - Helpers + + /// A LocationStateManager that never touches CoreLocation; for tests + /// that don't need channels or authorization. + private func makeBareLocationManager() -> LocationStateManager { + let suiteName = "NearbyNotesCounterTests-\(UUID().uuidString)" + let storage = UserDefaults(suiteName: suiteName)! + storage.removePersistentDomain(forName: suiteName) + addTeardownBlock { + storage.removePersistentDomain(forName: suiteName) + } + return LocationStateManager( + storage: storage, + locationManager: MockLocationManager(authorizationStatus: .denied), + geocoder: MockLocationGeocoder(), + shouldInitializeCoreLocation: false + ) + } + + /// An authorized LocationStateManager whose availableChannels carry a + /// real building-level geohash (Honolulu), built over CoreLocation mocks. + private func makeAuthorizedLocationManager() async throws -> LocationStateManager { + let suiteName = "NearbyNotesCounterTests-\(UUID().uuidString)" + let storage = UserDefaults(suiteName: suiteName)! + storage.removePersistentDomain(forName: suiteName) + addTeardownBlock { + storage.removePersistentDomain(forName: suiteName) + } + + let manager = LocationStateManager( + storage: storage, + locationManager: MockLocationManager(authorizationStatus: .authorizedAlways), + geocoder: MockLocationGeocoder(), + shouldInitializeCoreLocation: true + ) + let authorized = await waitUntil { manager.permissionState == .authorized } + XCTAssertTrue(authorized) + + manager.locationManager( + CLLocationManager(), + didUpdateLocations: [CLLocation(latitude: 21.2850, longitude: -157.8357)] + ) + let channelsLoaded = await waitUntil { + manager.availableChannels.contains { $0.level == .building } + } + XCTAssertTrue(channelsLoaded) + return manager + } + + /// The filter's `g` tag values, read through its NIP-01 wire encoding + /// (the stored tag filters aren't visible outside the Nostr layer). + private func geohashTagFilter(of filter: NostrFilter) throws -> [String]? { + let data = try JSONEncoder().encode(filter) + let json = try XCTUnwrap(JSONSerialization.jsonObject(with: data) as? [String: Any]) + return json["#g"] as? [String] + } + + private func waitUntil( + timeout: TimeInterval = 1.0, + condition: @escaping @MainActor () -> Bool + ) async -> Bool { + let deadline = Date().addingTimeInterval(timeout) + while Date() < deadline { + if condition() { + return true + } + try? await Task.sleep(nanoseconds: 10_000_000) + } + return condition() + } +} + +/// Stub relay layer: counts REQs, captures the last filter/handler, and never +/// touches the network. +@MainActor +private final class SubscriptionRecorder { + private(set) var subscribeCount = 0 + private(set) var unsubscribeCount = 0 + private(set) var lastFilter: NostrFilter? + private(set) var lastHandler: ((NostrEvent) -> Void)? + + var dependencies: LocationNotesDependencies { + LocationNotesDependencies( + relayLookup: { _, _ in ["wss://relay.one"] }, + subscribe: { [weak self] filter, _, _, handler, _ in + self?.subscribeCount += 1 + self?.lastFilter = filter + self?.lastHandler = handler + }, + unsubscribe: { [weak self] _ in + self?.unsubscribeCount += 1 + }, + sendEvent: { _, _ in }, + deriveIdentity: { _ in try NostrIdentity.generate() }, + now: { Date() } + ) + } +} + +private final class MockLocationManager: LocationStateManaging { + weak var delegate: CLLocationManagerDelegate? + var desiredAccuracy: CLLocationAccuracy = 0 + var distanceFilter: CLLocationDistance = 0 + var authorizationStatus: CLAuthorizationStatus + + init(authorizationStatus: CLAuthorizationStatus) { + self.authorizationStatus = authorizationStatus + } + + func requestWhenInUseAuthorization() {} + func requestLocation() {} + func startUpdatingLocation() {} + func stopUpdatingLocation() {} +} + +private final class MockLocationGeocoder: LocationStateGeocoding { + func cancelGeocode() {} + + func reverseGeocodeLocation( + _ location: CLLocation, + completionHandler: @escaping ([CLPlacemark]?, Error?) -> Void + ) { + completionHandler(nil, nil) + } +}