From cfa875459d02bf61f64658bfd36baeeb60fadea2 Mon Sep 17 00:00:00 2001 From: jack Date: Tue, 11 Aug 2026 10:03:01 +0200 Subject: [PATCH] Review fix: record withheld receipts in both tracking sets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P1: the manager-path withheld claim landed only in PrivateChatManager.sentReadReceipts, while the lifecycle read pass dedups against ChatViewModel's persisted set — enabling receipts before the next lifecycle pass could send a receipt for a message read while the setting was off. The withheld branch now records into the owner's persisted set too (markReceiptHandled, wired in the bootstrapper); test strengthened to require both sets. Co-Authored-By: Claude Fable 5 --- bitchat/Services/PrivateChatManager.swift | 12 ++++++++++-- bitchat/ViewModels/ChatViewModelBootstrapper.swift | 3 +++ bitchatTests/ChatViewModelTests.swift | 9 ++++++--- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/bitchat/Services/PrivateChatManager.swift b/bitchat/Services/PrivateChatManager.swift index 3141ec0b..a243d570 100644 --- a/bitchat/Services/PrivateChatManager.swift +++ b/bitchat/Services/PrivateChatManager.swift @@ -252,15 +252,23 @@ final class PrivateChatManager: ObservableObject { /// suites through the shared UserDefaults-backed setting. var sendsReadReceipts: () -> Bool = { ReadReceiptSettings.sendReadReceipts } + /// Records a withheld receipt in the owner's persisted set too: the + /// lifecycle read pass dedups against ChatViewModel.sentReadReceipts, + /// not this manager's set, so claiming only locally would let a receipt + /// for a message read while the setting was OFF fire after re-enabling. + var markReceiptHandled: ((String) -> Void)? + private func sendReadReceipt(for message: BitchatMessage) { guard !sentReadReceipts.contains(message.id), let senderPeerID = message.senderPeerID else { return } - // Withheld receipts are still claimed below as sent: re-enabling the - // setting must never fire a retroactive burst disclosing past reads. + // Withheld receipts are still claimed as sent — in BOTH tracking + // sets: re-enabling the setting must never fire a retroactive burst + // disclosing past reads, from this manager or the lifecycle pass. guard sendsReadReceipts() else { sentReadReceipts.insert(message.id) + markReceiptHandled?(message.id) return } diff --git a/bitchat/ViewModels/ChatViewModelBootstrapper.swift b/bitchat/ViewModels/ChatViewModelBootstrapper.swift index 3ccb7d12..ca769b05 100644 --- a/bitchat/ViewModels/ChatViewModelBootstrapper.swift +++ b/bitchat/ViewModels/ChatViewModelBootstrapper.swift @@ -90,6 +90,9 @@ private extension ChatViewModelBootstrapper { viewModel.privateChatManager.conversationStore = viewModel.conversations viewModel.privateChatManager.messageRouter = viewModel.messageRouter viewModel.privateChatManager.unifiedPeerService = viewModel.unifiedPeerService + viewModel.privateChatManager.markReceiptHandled = { [weak viewModel] messageID in + viewModel?.markReadReceiptSent(messageID) + } viewModel.unifiedPeerService.messageRouter = viewModel.messageRouter // Surface silent outbox drops (attempt cap, TTL expiry, overflow // eviction) as a visible failure. The store's no-downgrade rule does diff --git a/bitchatTests/ChatViewModelTests.swift b/bitchatTests/ChatViewModelTests.swift index 1bd66497..b4e9109e 100644 --- a/bitchatTests/ChatViewModelTests.swift +++ b/bitchatTests/ChatViewModelTests.swift @@ -462,10 +462,13 @@ struct ChatViewModelServiceLifecycleTests { #expect(!sentReadReceipt) // ...while the chat is still marked read locally and the receipt is - // recorded as handled, so re-enabling the setting never fires a - // retroactive burst disclosing past reading activity. + // recorded as handled in BOTH tracking sets (the lifecycle pass + // dedups against the owner's persisted set, the manager against its + // own), so re-enabling the setting never fires a retroactive burst + // disclosing past reading activity from either path. #expect(!viewModel.unreadPrivateMessages.contains(peerID)) - #expect(viewModel.sentReadReceipts.contains("read-2") || viewModel.privateChatManager.sentReadReceipts.contains("read-2")) + #expect(viewModel.sentReadReceipts.contains("read-2")) + #expect(viewModel.privateChatManager.sentReadReceipts.contains("read-2")) } @Test @MainActor