From 707b22878d5be1cb3c66d20788b7c39fbcb44811 Mon Sep 17 00:00:00 2001 From: jack Date: Wed, 10 Jun 2026 21:26:21 +0200 Subject: [PATCH] Convert shared coordinator state to owner-side intent operations nostrKeyMapping, sentGeoDeliveryAcks/sentReadReceipts dedupe, isBatchingPublic, geo subscription lifecycle, and private-chat selection hand-off now mutate through single intent operations on ChatViewModel, with backing storage locked down via private(set) so the single-writer property is compiler-enforced. Context protocols downgrade to read-only access where reads remain. 8 new contract tests. Known remaining writers outside the protocols: sentReadReceipts is passed inout to PrivateChatManager.syncReadReceiptsForSentMessages and un-marked by ChatTransportEventCoordinator on disconnect. Co-Authored-By: Claude Fable 5 --- .../ViewModels/ChatDeliveryCoordinator.swift | 10 +- bitchat/ViewModels/ChatNostrCoordinator.swift | 54 +++++--- .../ChatPeerIdentityCoordinator.swift | 4 +- .../ChatPrivateConversationCoordinator.swift | 40 +++--- .../ChatPublicConversationCoordinator.swift | 17 ++- bitchat/ViewModels/ChatViewModel.swift | 105 +++++++++++++- .../ChatNostrCoordinatorContextTests.swift | 11 ++ ...eConversationCoordinatorContextTests.swift | 15 ++ ...cConversationCoordinatorContextTests.swift | 12 +- .../ChatViewModelDeliveryStatusTests.swift | 6 + .../ChatViewModelExtensionsTests.swift | 131 +++++++++++++++++- .../PerformanceBaselineTests.swift | 17 +++ 12 files changed, 358 insertions(+), 64 deletions(-) diff --git a/bitchat/ViewModels/ChatDeliveryCoordinator.swift b/bitchat/ViewModels/ChatDeliveryCoordinator.swift index 62492aa2..61a4c8e3 100644 --- a/bitchat/ViewModels/ChatDeliveryCoordinator.swift +++ b/bitchat/ViewModels/ChatDeliveryCoordinator.swift @@ -14,8 +14,11 @@ import Foundation protocol ChatDeliveryContext: AnyObject { var messages: [BitchatMessage] { get set } var privateChats: [PeerID: [BitchatMessage]] { get set } - var sentReadReceipts: Set { get set } var isStartupPhase: Bool { get } + /// Drops every recorded read receipt whose message ID is not in `validMessageIDs`. + /// Returns the number of receipts removed. (Single mutation path for the + /// owner's `sentReadReceipts`; this coordinator never reads the raw set.) + func pruneSentReadReceipts(keeping validMessageIDs: Set) -> Int /// Signals that message state changed so observers refresh (e.g. `objectWillChange.send()`). func notifyUIChanged() /// Confirms receipt so the message router stops retaining the message for resend. @@ -57,10 +60,7 @@ final class ChatDeliveryCoordinator { } ) - let oldCount = context.sentReadReceipts.count - context.sentReadReceipts = context.sentReadReceipts.intersection(validMessageIDs) - - let removedCount = oldCount - context.sentReadReceipts.count + let removedCount = context.pruneSentReadReceipts(keeping: validMessageIDs) if removedCount > 0 { SecureLogger.debug("🧹 Cleaned up \(removedCount) old read receipts", category: .session) } diff --git a/bitchat/ViewModels/ChatNostrCoordinator.swift b/bitchat/ViewModels/ChatNostrCoordinator.swift index cf9b24b9..96b35783 100644 --- a/bitchat/ViewModels/ChatNostrCoordinator.swift +++ b/bitchat/ViewModels/ChatNostrCoordinator.swift @@ -18,10 +18,17 @@ protocol ChatNostrContext: AnyObject { // MARK: Channel & subscription state var activeChannel: ChannelID { get set } var currentGeohash: String? { get set } - var geoSubscriptionID: String? { get set } - var geoDmSubscriptionID: String? { get set } + var geoSubscriptionID: String? { get } + var geoDmSubscriptionID: String? { get } + func setGeoChatSubscriptionID(_ id: String?) + func setGeoDmSubscriptionID(_ id: String?) /// Geohash sampling subscriptions: subscription ID -> geohash. - var geoSamplingSubs: [String: String] { get set } + var geoSamplingSubs: [String: String] { get } + func addGeoSamplingSub(_ subID: String, forGeohash geohash: String) + func removeGeoSamplingSub(_ subID: String) + /// Clears all sampling subscriptions and returns the removed subscription IDs + /// so the caller can unsubscribe them from the relay manager. + func clearGeoSamplingSubs() -> [String] /// Per-geohash notification cooldown: geohash -> last notify time. var lastGeoNotificationAt: [String: Date] { get set } var nostrRelayManager: NostrRelayManager? { get } @@ -44,7 +51,9 @@ protocol ChatNostrContext: AnyObject { // MARK: Inbound private (geohash DM) payloads var selectedPrivateChatPeer: PeerID? { get } - var nostrKeyMapping: [PeerID: String] { get set } + var nostrKeyMapping: [PeerID: String] { get } + /// Records the Nostr pubkey behind a (possibly virtual) peer ID. + func registerNostrKeyMapping(_ pubkey: String, for peerID: PeerID) func handlePrivateMessage( _ payload: NoisePayload, senderPubkey: String, @@ -225,12 +234,12 @@ final class ChatNostrCoordinator { if let dmSub = context.geoDmSubscriptionID { NostrRelayManager.shared.unsubscribe(id: dmSub) - context.geoDmSubscriptionID = nil + context.setGeoDmSubscriptionID(nil) } if let identity = try? context.deriveNostrIdentity(forGeohash: channel.geohash) { let dmSub = "geo-dm-\(channel.geohash)" - context.geoDmSubscriptionID = dmSub + context.setGeoDmSubscriptionID(dmSub) let dmFilter = NostrFilter.giftWrapsFor( pubkey: identity.publicKeyHex, since: Date().addingTimeInterval(-TransportConfig.nostrDMSubscribeLookbackSeconds) @@ -274,8 +283,8 @@ final class ChatNostrCoordinator { context.setGeoNickname(nick, forPubkey: event.pubkey) } - context.nostrKeyMapping[PeerID(nostr_: event.pubkey)] = event.pubkey - context.nostrKeyMapping[PeerID(nostr: event.pubkey)] = event.pubkey + context.registerNostrKeyMapping(event.pubkey, for: PeerID(nostr_: event.pubkey)) + context.registerNostrKeyMapping(event.pubkey, for: PeerID(nostr: event.pubkey)) context.recordGeoParticipant(pubkeyHex: event.pubkey) if event.kind == NostrProtocol.EventKind.geohashPresence.rawValue { @@ -349,7 +358,7 @@ final class ChatNostrCoordinator { let messageTimestamp = Date(timeIntervalSince1970: TimeInterval(rumorTs)) let convKey = PeerID(nostr_: senderPubkey) - context.nostrKeyMapping[convKey] = senderPubkey + context.registerNostrKeyMapping(senderPubkey, for: convKey) switch noisePayload.type { case .privateMessage: @@ -400,11 +409,11 @@ final class ChatNostrCoordinator { if let sub = context.geoSubscriptionID { NostrRelayManager.shared.unsubscribe(id: sub) - context.geoSubscriptionID = nil + context.setGeoChatSubscriptionID(nil) } if let dmSub = context.geoDmSubscriptionID { NostrRelayManager.shared.unsubscribe(id: dmSub) - context.geoDmSubscriptionID = nil + context.setGeoDmSubscriptionID(nil) } context.currentGeohash = nil context.setActiveParticipantGeohash(nil) @@ -429,7 +438,7 @@ final class ChatNostrCoordinator { } let subID = "geo-\(channel.geohash)" - context.geoSubscriptionID = subID + context.setGeoChatSubscriptionID(subID) context.startGeoParticipantRefreshTimer() let ts = Date().addingTimeInterval(-TransportConfig.nostrGeohashInitialLookbackSeconds) let filter = NostrFilter.geohashEphemeral(channel.geohash, since: ts, limit: TransportConfig.nostrGeohashInitialLimit) @@ -504,8 +513,8 @@ final class ChatNostrCoordinator { context.setGeoNickname(nickTag[1].trimmed, forPubkey: event.pubkey) } - context.nostrKeyMapping[PeerID(nostr_: event.pubkey)] = event.pubkey - context.nostrKeyMapping[PeerID(nostr: event.pubkey)] = event.pubkey + context.registerNostrKeyMapping(event.pubkey, for: PeerID(nostr_: event.pubkey)) + context.registerNostrKeyMapping(event.pubkey, for: PeerID(nostr: event.pubkey)) if event.kind == NostrProtocol.EventKind.geohashPresence.rawValue { return @@ -547,7 +556,7 @@ final class ChatNostrCoordinator { guard let identity = try? context.deriveNostrIdentity(forGeohash: channel.geohash) else { return } let dmSub = "geo-dm-\(channel.geohash)" - context.geoDmSubscriptionID = dmSub + context.setGeoDmSubscriptionID(dmSub) if TorManager.shared.isReady { SecureLogger.debug("GeoDM: subscribing DMs pub=\(identity.publicKeyHex.prefix(8))… sub=\(dmSub)", category: .session) } @@ -593,7 +602,7 @@ final class ChatNostrCoordinator { } let convKey = PeerID(nostr_: senderPubkey) - context.nostrKeyMapping[convKey] = senderPubkey + context.registerNostrKeyMapping(senderPubkey, for: convKey) switch payload.type { case .privateMessage: @@ -633,7 +642,7 @@ final class ChatNostrCoordinator { } context.recordGeoParticipant(pubkeyHex: identity.publicKeyHex) - context.nostrKeyMapping[PeerID(nostr: identity.publicKeyHex)] = identity.publicKeyHex + context.registerNostrKeyMapping(identity.publicKeyHex, for: PeerID(nostr: identity.publicKeyHex)) SecureLogger.debug( "GeoTeleport: sent geo message pub=\(identity.publicKeyHex.prefix(8))… teleported=\(geoContext.teleported)", category: .session @@ -666,7 +675,7 @@ final class ChatNostrCoordinator { for (subID, gh) in context.geoSamplingSubs where toRemove.contains(gh) { NostrRelayManager.shared.unsubscribe(id: subID) - context.geoSamplingSubs.removeValue(forKey: subID) + context.removeGeoSamplingSub(subID) } for gh in toAdd { @@ -678,7 +687,7 @@ final class ChatNostrCoordinator { func subscribe(_ gh: String) { guard let context else { return } let subID = "geo-sample-\(gh)" - context.geoSamplingSubs[subID] = gh + context.addGeoSamplingSub(subID, forGeohash: gh) let filter = NostrFilter.geohashEphemeral( gh, since: Date().addingTimeInterval(-TransportConfig.nostrGeohashSampleLookbackSeconds), @@ -771,10 +780,9 @@ final class ChatNostrCoordinator { @MainActor func endGeohashSampling() { guard let context else { return } - for subID in context.geoSamplingSubs.keys { + for subID in context.clearGeoSamplingSubs() { NostrRelayManager.shared.unsubscribe(id: subID) } - context.geoSamplingSubs.removeAll() clearGeoSamplingEventDedup() } @@ -885,7 +893,7 @@ final class ChatNostrCoordinator { let payload = NoisePayload.decode(packet.payload) { let messageTimestamp = Date(timeIntervalSince1970: TimeInterval(rumorTimestamp)) await MainActor.run { - context.nostrKeyMapping[targetPeerID] = senderPubkey + context.registerNostrKeyMapping(senderPubkey, for: targetPeerID) switch payload.type { case .privateMessage: @@ -1063,7 +1071,7 @@ final class ChatNostrCoordinator { func startGeohashDM(withPubkeyHex hex: String) { guard let context else { return } let convKey = PeerID(nostr_: hex) - context.nostrKeyMapping[convKey] = hex + context.registerNostrKeyMapping(hex, for: convKey) context.startPrivateChat(with: convKey) } diff --git a/bitchat/ViewModels/ChatPeerIdentityCoordinator.swift b/bitchat/ViewModels/ChatPeerIdentityCoordinator.swift index 6a3047cb..372c7956 100644 --- a/bitchat/ViewModels/ChatPeerIdentityCoordinator.swift +++ b/bitchat/ViewModels/ChatPeerIdentityCoordinator.swift @@ -301,7 +301,7 @@ final class ChatPeerIdentityCoordinator { .visibleGeohashPeople() .first(where: { $0.displayName == nickname }) { let conversationKey = PeerID(nostr_: person.id) - viewModel.nostrKeyMapping[conversationKey] = person.id + viewModel.registerNostrKeyMapping(person.id, for: conversationKey) return conversationKey } @@ -312,7 +312,7 @@ final class ChatPeerIdentityCoordinator { .lowercased() ?? nickname.lowercased() if let pubkey = viewModel.geoNicknames.first(where: { $0.value.lowercased() == base })?.key { let conversationKey = PeerID(nostr_: pubkey) - viewModel.nostrKeyMapping[conversationKey] = pubkey + viewModel.registerNostrKeyMapping(pubkey, for: conversationKey) return conversationKey } diff --git a/bitchat/ViewModels/ChatPrivateConversationCoordinator.swift b/bitchat/ViewModels/ChatPrivateConversationCoordinator.swift index 66dc766b..ea1d28e0 100644 --- a/bitchat/ViewModels/ChatPrivateConversationCoordinator.swift +++ b/bitchat/ViewModels/ChatPrivateConversationCoordinator.swift @@ -15,13 +15,23 @@ import Foundation protocol ChatPrivateConversationContext: AnyObject { // MARK: Conversation state var privateChats: [PeerID: [BitchatMessage]] { get set } - var sentReadReceipts: Set { get set } - var sentGeoDeliveryAcks: Set { get set } + var sentReadReceipts: Set { get } var unreadPrivateMessages: Set { get set } - var selectedPrivateChatPeer: PeerID? { get set } + var selectedPrivateChatPeer: PeerID? { get } var nickname: String { get } var activeChannel: ChannelID { get } var nostrKeyMapping: [PeerID: String] { get } + /// Records that a read receipt is being sent for `messageID`. + /// Returns `false` when one was already recorded — the caller must skip sending. + @discardableResult + func markReadReceiptSent(_ messageID: String) -> Bool + /// Records that a GeoDM delivery ACK is being sent for `messageID`. + /// Returns `false` when one was already recorded — the caller must skip sending. + @discardableResult + func markGeoDeliveryAckSent(_ messageID: String) -> Bool + /// Moves the open private chat to `newPeerID` when the current selection is + /// one of the peer IDs being migrated away. + func handOffSelectedPrivateChat(from oldPeerIDs: [PeerID], to newPeerID: PeerID) /// Signals that message state changed so observers refresh (e.g. `objectWillChange.send()`). func notifyUIChanged() @@ -62,8 +72,10 @@ protocol ChatPrivateConversationContext: AnyObject { } extension ChatViewModel: ChatPrivateConversationContext { - // `privateChats`, `sentReadReceipts`, and `notifyUIChanged()` are shared - // requirements with `ChatDeliveryContext`; the remaining state members are + // `privateChats` and `notifyUIChanged()` are shared requirements with + // `ChatDeliveryContext`; the single-writer intent ops (`markReadReceiptSent`, + // `markGeoDeliveryAckSent`, `handOffSelectedPrivateChat`) live next to their + // backing state in `ChatViewModel`. The remaining state members are // satisfied by existing `ChatViewModel` properties and methods. var myPeerID: PeerID { meshService.myPeerID } @@ -425,15 +437,13 @@ final class ChatPrivateConversationCoordinator { } func sendDeliveryAckIfNeeded(to messageId: String, senderPubKey: String, from id: NostrIdentity) { - guard !context.sentGeoDeliveryAcks.contains(messageId) else { return } + guard context.markGeoDeliveryAckSent(messageId) else { return } context.sendGeohashDeliveryAck(for: messageId, toRecipientHex: senderPubKey, from: id) - context.sentGeoDeliveryAcks.insert(messageId) } func sendReadReceiptIfNeeded(to messageId: String, senderPubKey: String, from id: NostrIdentity) { - guard !context.sentReadReceipts.contains(messageId) else { return } + guard context.markReadReceiptSent(messageId) else { return } context.sendGeohashReadReceipt(messageId, toRecipientHex: senderPubKey, from: id) - context.sentReadReceipts.insert(messageId) } func handlePrivateMessage( @@ -579,7 +589,7 @@ final class ChatPrivateConversationCoordinator { readerNickname: context.nickname ) context.sendMeshReadReceipt(receipt, to: peerID) - context.sentReadReceipts.insert(message.id) + context.markReadReceiptSent(message.id) } else { context.unreadPrivateMessages.insert(peerID) NotificationService.shared.sendPrivateMessageNotification( @@ -654,10 +664,10 @@ final class ChatPrivateConversationCoordinator { ) SecureLogger.debug("Viewing chat; sending READ ack for \(message.id.prefix(8))… via router", category: .session) context.routeReadReceipt(receipt, to: PeerID(hexData: key)) - context.sentReadReceipts.insert(message.id) + context.markReadReceiptSent(message.id) } else if let identity = context.currentNostrIdentity() { context.sendGeohashReadReceipt(message.id, toRecipientHex: senderPubkey, from: identity) - context.sentReadReceipts.insert(message.id) + context.markReadReceiptSent(message.id) SecureLogger.debug( "Viewing chat; sent READ ack directly to Nostr pub=\(senderPubkey.prefix(8))… for mid=\(message.id.prefix(8))…", category: .session @@ -802,17 +812,13 @@ final class ChatPrivateConversationCoordinator { } if !oldPeerIDsToRemove.isEmpty { - let needsSelectedUpdate = oldPeerIDsToRemove.contains { context.selectedPrivateChatPeer == $0 } - for oldID in oldPeerIDsToRemove { context.privateChats.removeValue(forKey: oldID) context.unreadPrivateMessages.remove(oldID) context.clearStoredFingerprint(for: oldID) } - if needsSelectedUpdate { - context.selectedPrivateChatPeer = peerID - } + context.handOffSelectedPrivateChat(from: oldPeerIDsToRemove, to: peerID) } if !migratedMessages.isEmpty { diff --git a/bitchat/ViewModels/ChatPublicConversationCoordinator.swift b/bitchat/ViewModels/ChatPublicConversationCoordinator.swift index f140922e..c7627ca3 100644 --- a/bitchat/ViewModels/ChatPublicConversationCoordinator.swift +++ b/bitchat/ViewModels/ChatPublicConversationCoordinator.swift @@ -25,7 +25,10 @@ protocol ChatPublicConversationContext: AnyObject { var currentGeohash: String? { get } var nickname: String { get } var myPeerID: PeerID { get } - var isBatchingPublic: Bool { get set } + /// Publishes the public-timeline batching state (UI animation suppression). + /// (Single mutation path for the owner's `isBatchingPublic`; this + /// coordinator never reads it.) + func setPublicBatching(_ isBatching: Bool) /// Signals that message state changed so observers refresh (e.g. `objectWillChange.send()`). func notifyUIChanged() func trimMessagesIfNeeded() @@ -56,7 +59,9 @@ protocol ChatPublicConversationContext: AnyObject { // MARK: Geohash participants & presence var geoNicknames: [String: String] { get } var isTeleported: Bool { get } - var nostrKeyMapping: [PeerID: String] { get set } + var nostrKeyMapping: [PeerID: String] { get } + /// Drops every key mapping that resolves to the given (lowercased) Nostr pubkey. + func removeNostrKeyMappings(matchingPubkeyHexLowercased hex: String) func visibleGeoPeople() -> [GeoPerson] func geoParticipantCount(for geohash: String) -> Int func removeGeoParticipant(pubkeyHex: String) @@ -88,7 +93,7 @@ protocol ChatPublicConversationContext: AnyObject { extension ChatViewModel: ChatPublicConversationContext { // `messages`, `privateChats`, `unreadPrivateMessages`, `nostrKeyMapping`, // `nickname`, `activeChannel`, `currentGeohash`, `geoNicknames`, - // `myPeerID`, `isTeleported`, `isBatchingPublic`, `notifyUIChanged()`, + // `myPeerID`, `isTeleported`, `notifyUIChanged()`, // `geoParticipantCount(for:)`, `isNostrBlocked(pubkeyHexLowercased:)`, // `deriveNostrIdentity(forGeohash:)`, and // `appendGeohashMessageIfAbsent(_:toGeohash:)` are shared requirements @@ -247,9 +252,7 @@ final class ChatPublicConversationCoordinator: PublicMessagePipelineDelegate { context.unreadPrivateMessages = unread } - for (key, value) in context.nostrKeyMapping where value.lowercased() == hex { - context.nostrKeyMapping.removeValue(forKey: key) - } + context.removeNostrKeyMappings(matchingPubkeyHexLowercased: hex) addSystemMessage( String( @@ -616,7 +619,7 @@ final class ChatPublicConversationCoordinator: PublicMessagePipelineDelegate { } func pipelineSetBatchingState(_ pipeline: PublicMessagePipeline, isBatching: Bool) { - context.isBatchingPublic = isBatching + context.setPublicBatching(isBatching) } } diff --git a/bitchat/ViewModels/ChatViewModel.swift b/bitchat/ViewModels/ChatViewModel.swift index 090a8afd..23667420 100644 --- a/bitchat/ViewModels/ChatViewModel.swift +++ b/bitchat/ViewModels/ChatViewModel.swift @@ -292,8 +292,9 @@ final class ChatViewModel: ObservableObject, BitchatDelegate, TransportEventDele objectWillChange.send() } } - var geoSubscriptionID: String? = nil - var geoDmSubscriptionID: String? = nil + // Single-writer: mutate only via `setGeoChatSubscriptionID(_:)` / `setGeoDmSubscriptionID(_:)` below. + private(set) var geoSubscriptionID: String? = nil + private(set) var geoDmSubscriptionID: String? = nil var currentGeohash: String? { get { locationPresenceStore.currentGeohash } set { locationPresenceStore.setCurrentGeohash(newValue) } @@ -366,7 +367,8 @@ final class ChatViewModel: ObservableObject, BitchatDelegate, TransportEventDele set { locationPresenceStore.replaceTeleportedGeo(newValue) } } // lowercased pubkey hex // Sampling subscriptions for multiple geohashes (when channel sheet is open) - var geoSamplingSubs: [String: String] = [:] // subID -> geohash + // Single-writer: mutate only via `addGeoSamplingSub` / `removeGeoSamplingSub` / `clearGeoSamplingSubs` below. + private(set) var geoSamplingSubs: [String: String] = [:] // subID -> geohash var lastGeoNotificationAt: [String: Date] = [:] // geohash -> last notify time // MARK: - Message Delivery Tracking @@ -384,7 +386,8 @@ final class ChatViewModel: ObservableObject, BitchatDelegate, TransportEventDele // MARK: - Public message batching (UI perf) let publicMessagePipeline: PublicMessagePipeline - @Published var isBatchingPublic: Bool = false + // Single-writer: mutate only via `setPublicBatching(_:)` below. + @Published private(set) var isBatchingPublic: Bool = false // Track sent read receipts to avoid duplicates (persisted across launches) // Note: Persistence happens automatically in didSet, no lifecycle observers needed @@ -403,7 +406,8 @@ final class ChatViewModel: ObservableObject, BitchatDelegate, TransportEventDele } // Track which GeoDM messages we've already sent a delivery ACK for (by messageID) - var sentGeoDeliveryAcks: Set = [] + // Single-writer: mutate only via `markGeoDeliveryAckSent(_:)` below. + private(set) var sentGeoDeliveryAcks: Set = [] // Track app startup phase to prevent marking old messages as unread var isStartupPhase = true @@ -411,7 +415,96 @@ final class ChatViewModel: ObservableObject, BitchatDelegate, TransportEventDele var torInitialReadyAnnounced: Bool = false // Track Nostr pubkey mappings for unknown senders - var nostrKeyMapping: [PeerID: String] = [:] // senderPeerID -> nostrPubkey + // Single-writer: mutate only via `registerNostrKeyMapping` / `removeNostrKeyMappings` below. + private(set) var nostrKeyMapping: [PeerID: String] = [:] // senderPeerID -> nostrPubkey + + // MARK: - Single-Writer Intent Operations + // Owner-side mutation paths for state the coordinator contexts may read + // but not write directly. Each op is the sole way to mutate its backing + // state, so check-then-mutate races between coordinators cannot occur. + + /// Records the Nostr pubkey behind a (possibly virtual) peer ID. + @MainActor + func registerNostrKeyMapping(_ pubkey: String, for peerID: PeerID) { + nostrKeyMapping[peerID] = pubkey + } + + /// Drops every key mapping that resolves to the given (lowercased) Nostr pubkey. + @MainActor + func removeNostrKeyMappings(matchingPubkeyHexLowercased hex: String) { + for (key, value) in nostrKeyMapping where value.lowercased() == hex { + nostrKeyMapping.removeValue(forKey: key) + } + } + + /// Records that a read receipt is being sent for `messageID`. + /// Returns `false` when one was already recorded — the caller must skip sending. + @MainActor + @discardableResult + func markReadReceiptSent(_ messageID: String) -> Bool { + sentReadReceipts.insert(messageID).inserted + } + + /// Records that a GeoDM delivery ACK is being sent for `messageID`. + /// Returns `false` when one was already recorded — the caller must skip sending. + @MainActor + @discardableResult + func markGeoDeliveryAckSent(_ messageID: String) -> Bool { + sentGeoDeliveryAcks.insert(messageID).inserted + } + + /// Drops every recorded read receipt whose message ID is no longer valid. + /// Returns the number of receipts removed. + @MainActor + func pruneSentReadReceipts(keeping validMessageIDs: Set) -> Int { + let oldCount = sentReadReceipts.count + sentReadReceipts = sentReadReceipts.intersection(validMessageIDs) + return oldCount - sentReadReceipts.count + } + + /// Publishes the public-timeline batching state (UI animation suppression). + @MainActor + func setPublicBatching(_ isBatching: Bool) { + isBatchingPublic = isBatching + } + + @MainActor + func setGeoChatSubscriptionID(_ id: String?) { + geoSubscriptionID = id + } + + @MainActor + func setGeoDmSubscriptionID(_ id: String?) { + geoDmSubscriptionID = id + } + + @MainActor + func addGeoSamplingSub(_ subID: String, forGeohash geohash: String) { + geoSamplingSubs[subID] = geohash + } + + @MainActor + func removeGeoSamplingSub(_ subID: String) { + geoSamplingSubs.removeValue(forKey: subID) + } + + /// Clears all sampling subscriptions and returns the removed subscription IDs + /// so the caller can unsubscribe them from the relay manager. + @MainActor + func clearGeoSamplingSubs() -> [String] { + let subIDs = Array(geoSamplingSubs.keys) + geoSamplingSubs.removeAll() + return subIDs + } + + /// Moves the open private chat to `newPeerID` when the current selection is + /// one of the peer IDs being migrated away (side-effectful: re-targets the + /// private chat session and resyncs the conversation stores). + @MainActor + func handOffSelectedPrivateChat(from oldPeerIDs: [PeerID], to newPeerID: PeerID) { + guard oldPeerIDs.contains(where: { selectedPrivateChatPeer == $0 }) else { return } + selectedPrivateChatPeer = newPeerID + } // MARK: - Initialization diff --git a/bitchatTests/ChatNostrCoordinatorContextTests.swift b/bitchatTests/ChatNostrCoordinatorContextTests.swift index 73ad3b96..30d208e4 100644 --- a/bitchatTests/ChatNostrCoordinatorContextTests.swift +++ b/bitchatTests/ChatNostrCoordinatorContextTests.swift @@ -28,6 +28,16 @@ private final class MockChatNostrContext: ChatNostrContext { var lastGeoNotificationAt: [String: Date] = [:] var nostrRelayManager: NostrRelayManager? { nil } + func setGeoChatSubscriptionID(_ id: String?) { geoSubscriptionID = id } + func setGeoDmSubscriptionID(_ id: String?) { geoDmSubscriptionID = id } + func addGeoSamplingSub(_ subID: String, forGeohash geohash: String) { geoSamplingSubs[subID] = geohash } + func removeGeoSamplingSub(_ subID: String) { geoSamplingSubs.removeValue(forKey: subID) } + + func clearGeoSamplingSubs() -> [String] { + defer { geoSamplingSubs.removeAll() } + return Array(geoSamplingSubs.keys) + } + // Public timeline & pipeline var messages: [BitchatMessage] = [] private(set) var pipelineResetCount = 0 @@ -71,6 +81,7 @@ private final class MockChatNostrContext: ChatNostrContext { // Inbound private (geohash DM) payloads var selectedPrivateChatPeer: PeerID? var nostrKeyMapping: [PeerID: String] = [:] + func registerNostrKeyMapping(_ pubkey: String, for peerID: PeerID) { nostrKeyMapping[peerID] = pubkey } private(set) var handledPrivateMessages: [(payload: NoisePayload, senderPubkey: String, convKey: PeerID, timestamp: Date)] = [] private(set) var handledDelivered: [(senderPubkey: String, convKey: PeerID)] = [] private(set) var handledReadReceipts: [(senderPubkey: String, convKey: PeerID)] = [] diff --git a/bitchatTests/ChatPrivateConversationCoordinatorContextTests.swift b/bitchatTests/ChatPrivateConversationCoordinatorContextTests.swift index dc55fdca..e8f9493e 100644 --- a/bitchatTests/ChatPrivateConversationCoordinatorContextTests.swift +++ b/bitchatTests/ChatPrivateConversationCoordinatorContextTests.swift @@ -29,6 +29,21 @@ private final class MockChatPrivateConversationContext: ChatPrivateConversationC var nostrKeyMapping: [PeerID: String] = [:] private(set) var notifyUIChangedCount = 0 + @discardableResult + func markReadReceiptSent(_ messageID: String) -> Bool { + sentReadReceipts.insert(messageID).inserted + } + + @discardableResult + func markGeoDeliveryAckSent(_ messageID: String) -> Bool { + sentGeoDeliveryAcks.insert(messageID).inserted + } + + func handOffSelectedPrivateChat(from oldPeerIDs: [PeerID], to newPeerID: PeerID) { + guard oldPeerIDs.contains(where: { selectedPrivateChatPeer == $0 }) else { return } + selectedPrivateChatPeer = newPeerID + } + func notifyUIChanged() { notifyUIChangedCount += 1 } diff --git a/bitchatTests/ChatPublicConversationCoordinatorContextTests.swift b/bitchatTests/ChatPublicConversationCoordinatorContextTests.swift index 68322fd3..6051ccca 100644 --- a/bitchatTests/ChatPublicConversationCoordinatorContextTests.swift +++ b/bitchatTests/ChatPublicConversationCoordinatorContextTests.swift @@ -31,10 +31,14 @@ private final class MockChatPublicConversationContext: ChatPublicConversationCon var currentGeohash: String? var nickname = "me" var myPeerID = PeerID(str: "0011223344556677") - var isBatchingPublic = false + private(set) var isBatchingPublic = false private(set) var notifyUIChangedCount = 0 private(set) var trimMessagesCount = 0 + func setPublicBatching(_ isBatching: Bool) { + isBatchingPublic = isBatching + } + func notifyUIChanged() { notifyUIChangedCount += 1 } @@ -147,6 +151,12 @@ private final class MockChatPublicConversationContext: ChatPublicConversationCon var geoParticipantCounts: [String: Int] = [:] private(set) var removedGeoParticipants: [String] = [] + func removeNostrKeyMappings(matchingPubkeyHexLowercased hex: String) { + for (key, value) in nostrKeyMapping where value.lowercased() == hex { + nostrKeyMapping.removeValue(forKey: key) + } + } + func visibleGeoPeople() -> [GeoPerson] { geoPeople } diff --git a/bitchatTests/ChatViewModelDeliveryStatusTests.swift b/bitchatTests/ChatViewModelDeliveryStatusTests.swift index 60d0b86d..9730c053 100644 --- a/bitchatTests/ChatViewModelDeliveryStatusTests.swift +++ b/bitchatTests/ChatViewModelDeliveryStatusTests.swift @@ -388,6 +388,12 @@ private final class MockChatDeliveryContext: ChatDeliveryContext { private(set) var notifyUIChangedCount = 0 private(set) var markedDeliveredMessageIDs: [String] = [] + func pruneSentReadReceipts(keeping validMessageIDs: Set) -> Int { + let oldCount = sentReadReceipts.count + sentReadReceipts = sentReadReceipts.intersection(validMessageIDs) + return oldCount - sentReadReceipts.count + } + func notifyUIChanged() { notifyUIChangedCount += 1 } diff --git a/bitchatTests/ChatViewModelExtensionsTests.swift b/bitchatTests/ChatViewModelExtensionsTests.swift index b446000a..666745f2 100644 --- a/bitchatTests/ChatViewModelExtensionsTests.swift +++ b/bitchatTests/ChatViewModelExtensionsTests.swift @@ -225,7 +225,7 @@ struct ChatViewModelPrivateChatExtensionTests { // "Check geohash (Nostr) blocks using mapping to full pubkey" let hexPubkey = "0000000000000000000000000000000000000000000000000000000000000001" - viewModel.nostrKeyMapping[blockedPeerID] = hexPubkey + viewModel.registerNostrKeyMapping(hexPubkey, for: blockedPeerID) viewModel.identityManager.setNostrBlocked(hexPubkey, isBlocked: true) // Force isGeoChat/isGeoDM check to be true by setting prefix? @@ -235,7 +235,7 @@ struct ChatViewModelPrivateChatExtensionTests { // We need a peerID that looks like geo. let geoPeerID = PeerID(nostr_: hexPubkey) - viewModel.nostrKeyMapping[geoPeerID] = hexPubkey + viewModel.registerNostrKeyMapping(hexPubkey, for: geoPeerID) let geoMessage = BitchatMessage( id: "msg-geo-blocked", @@ -789,7 +789,7 @@ struct ChatViewModelGeoDMTests { let convKey = PeerID(nostr_: recipientHex) viewModel.switchLocationChannel(to: .location(GeohashChannel(level: .city, geohash: geohash))) - viewModel.nostrKeyMapping[convKey] = recipientHex + viewModel.registerNostrKeyMapping(recipientHex, for: convKey) viewModel.identityManager.setNostrBlocked(recipientHex, isBlocked: true) viewModel.sendGeohashDM("hello", to: convKey) @@ -829,6 +829,131 @@ struct ChatViewModelGeoDMTests { } } +// MARK: - Single-Writer Intent Operation Tests + +/// Contracts for the owner-side intent ops that are the sole mutation paths +/// for `ChatViewModel`'s shared coordinator state (`nostrKeyMapping`, +/// `sentReadReceipts`, `sentGeoDeliveryAcks`, `isBatchingPublic`, the geo +/// subscription IDs, and the selected private chat hand-off). +struct ChatViewModelIntentOperationTests { + + @Test @MainActor + func markGeoDeliveryAckSent_returnsFalseOnSecondCall() async { + let (viewModel, _) = makeTestableViewModel() + + #expect(viewModel.markGeoDeliveryAckSent("geo-ack-1")) + #expect(!viewModel.markGeoDeliveryAckSent("geo-ack-1")) + #expect(viewModel.markGeoDeliveryAckSent("geo-ack-2")) + #expect(viewModel.sentGeoDeliveryAcks == ["geo-ack-1", "geo-ack-2"]) + } + + @Test @MainActor + func markReadReceiptSent_returnsFalseOnSecondCall() async { + let (viewModel, _) = makeTestableViewModel() + + #expect(viewModel.markReadReceiptSent("read-1")) + #expect(!viewModel.markReadReceiptSent("read-1")) + #expect(viewModel.sentReadReceipts.contains("read-1")) + } + + @Test @MainActor + func pruneSentReadReceipts_dropsStaleIDsAndReturnsRemovedCount() async { + let (viewModel, _) = makeTestableViewModel() + viewModel.sentReadReceipts = ["keep-1", "keep-2", "drop-1", "drop-2"] + + let removed = viewModel.pruneSentReadReceipts(keeping: ["keep-1", "keep-2", "unrelated"]) + + #expect(removed == 2) + #expect(viewModel.sentReadReceipts == ["keep-1", "keep-2"]) + // Nothing stale left: a second prune removes nothing. + #expect(viewModel.pruneSentReadReceipts(keeping: ["keep-1", "keep-2"]) == 0) + } + + @Test @MainActor + func registerNostrKeyMapping_isVisibleToNostrCoordinatorLookups() async { + let (viewModel, _) = makeTestableViewModel() + let hex = "00112233445566778899aabbccddeeff00112233445566778899aabbccddeeff" + let convKey = PeerID(nostr_: hex) + + // Registered through the owner intent op (as e.g. the private + // conversation flow does) and resolved through the Nostr coordinator, + // which reads the same backing dictionary via `ChatNostrContext`. + viewModel.registerNostrKeyMapping(hex, for: convKey) + + #expect(viewModel.nostrKeyMapping[convKey] == hex) + #expect(viewModel.nostrCoordinator.fullNostrHex(forSenderPeerID: convKey) == hex) + } + + @Test @MainActor + func removeNostrKeyMappings_dropsEveryMappingForThePubkeyCaseInsensitively() async { + let (viewModel, _) = makeTestableViewModel() + let hex = "aabbccddeeff00112233445566778899aabbccddeeff00112233445566778899" + let otherHex = "ffeeddccbbaa99887766554433221100ffeeddccbbaa99887766554433221100" + viewModel.registerNostrKeyMapping(hex.uppercased(), for: PeerID(nostr: hex)) + viewModel.registerNostrKeyMapping(hex, for: PeerID(nostr_: hex)) + viewModel.registerNostrKeyMapping(otherHex, for: PeerID(nostr_: otherHex)) + + viewModel.removeNostrKeyMappings(matchingPubkeyHexLowercased: hex) + + #expect(viewModel.nostrKeyMapping[PeerID(nostr: hex)] == nil) + #expect(viewModel.nostrKeyMapping[PeerID(nostr_: hex)] == nil) + #expect(viewModel.nostrKeyMapping[PeerID(nostr_: otherHex)] == otherHex) + } + + @Test @MainActor + func setPublicBatching_publishesBatchingState() async { + let (viewModel, _) = makeTestableViewModel() + + #expect(!viewModel.isBatchingPublic) + viewModel.setPublicBatching(true) + #expect(viewModel.isBatchingPublic) + viewModel.setPublicBatching(false) + #expect(!viewModel.isBatchingPublic) + } + + @Test @MainActor + func geoSubscriptionIntentOps_setClearAndDrainSubscriptions() async { + let (viewModel, _) = makeTestableViewModel() + + viewModel.setGeoChatSubscriptionID("geo-u4pruy") + viewModel.setGeoDmSubscriptionID("geo-dm-u4pruy") + #expect(viewModel.geoSubscriptionID == "geo-u4pruy") + #expect(viewModel.geoDmSubscriptionID == "geo-dm-u4pruy") + + viewModel.setGeoChatSubscriptionID(nil) + viewModel.setGeoDmSubscriptionID(nil) + #expect(viewModel.geoSubscriptionID == nil) + #expect(viewModel.geoDmSubscriptionID == nil) + + viewModel.addGeoSamplingSub("geo-sample-aaaa", forGeohash: "aaaa") + viewModel.addGeoSamplingSub("geo-sample-bbbb", forGeohash: "bbbb") + viewModel.removeGeoSamplingSub("geo-sample-aaaa") + #expect(viewModel.geoSamplingSubs == ["geo-sample-bbbb": "bbbb"]) + + let cleared = viewModel.clearGeoSamplingSubs() + #expect(cleared == ["geo-sample-bbbb"]) + #expect(viewModel.geoSamplingSubs.isEmpty) + } + + @Test @MainActor + func handOffSelectedPrivateChat_movesSelectionOnlyWhenSelectedPeerIsMigrated() async { + let (viewModel, _) = makeTestableViewModel() + let oldPeer = PeerID(str: "aaaaaaaaaaaaaaaa") + let unrelatedPeer = PeerID(str: "cccccccccccccccc") + let newPeer = PeerID(str: "bbbbbbbbbbbbbbbb") + + // Selection not among the migrated peers: untouched. + viewModel.selectedPrivateChatPeer = unrelatedPeer + viewModel.handOffSelectedPrivateChat(from: [oldPeer], to: newPeer) + #expect(viewModel.selectedPrivateChatPeer == unrelatedPeer) + + // Selection being migrated away: handed off to the new peer. + viewModel.selectedPrivateChatPeer = oldPeer + viewModel.handOffSelectedPrivateChat(from: [oldPeer], to: newPeer) + #expect(viewModel.selectedPrivateChatPeer == newPeer) + } +} + @Suite(.serialized) struct ChatViewModelMediaTransferTests { diff --git a/bitchatTests/Performance/PerformanceBaselineTests.swift b/bitchatTests/Performance/PerformanceBaselineTests.swift index 14b08f10..34ef1918 100644 --- a/bitchatTests/Performance/PerformanceBaselineTests.swift +++ b/bitchatTests/Performance/PerformanceBaselineTests.swift @@ -384,6 +384,16 @@ private final class PerfNostrContext: ChatNostrContext { var lastGeoNotificationAt: [String: Date] = [:] var nostrRelayManager: NostrRelayManager? { nil } + func setGeoChatSubscriptionID(_ id: String?) { geoSubscriptionID = id } + func setGeoDmSubscriptionID(_ id: String?) { geoDmSubscriptionID = id } + func addGeoSamplingSub(_ subID: String, forGeohash geohash: String) { geoSamplingSubs[subID] = geohash } + func removeGeoSamplingSub(_ subID: String) { geoSamplingSubs.removeValue(forKey: subID) } + + func clearGeoSamplingSubs() -> [String] { + defer { geoSamplingSubs.removeAll() } + return Array(geoSamplingSubs.keys) + } + var messages: [BitchatMessage] = [] func resetPublicMessagePipeline() {} func updatePublicMessagePipelineChannel(_ channel: ChannelID) {} @@ -403,6 +413,7 @@ private final class PerfNostrContext: ChatNostrContext { var selectedPrivateChatPeer: PeerID? var nostrKeyMapping: [PeerID: String] = [:] + func registerNostrKeyMapping(_ pubkey: String, for peerID: PeerID) { nostrKeyMapping[peerID] = pubkey } func handlePrivateMessage(_ payload: NoisePayload, senderPubkey: String, convKey: PeerID, id: NostrIdentity, messageTimestamp: Date) {} func handleDelivered(_ payload: NoisePayload, senderPubkey: String, convKey: PeerID) {} func handleReadReceipt(_ payload: NoisePayload, senderPubkey: String, convKey: PeerID) {} @@ -460,6 +471,12 @@ private final class PerfDeliveryContext: ChatDeliveryContext { func notifyUIChanged() {} func markMessageDelivered(_ messageID: String) {} + func pruneSentReadReceipts(keeping validMessageIDs: Set) -> Int { + let oldCount = sentReadReceipts.count + sentReadReceipts = sentReadReceipts.intersection(validMessageIDs) + return oldCount - sentReadReceipts.count + } + /// 2000 public + `peerCount` x `messagesPerPeer` private messages with /// deterministic IDs and timestamps. static func makeCorpus(publicCount: Int, peerCount: Int, messagesPerPeer: Int) -> PerfDeliveryContext {