diff --git a/Sources/ColumbaApp/Services/AppServices.swift b/Sources/ColumbaApp/Services/AppServices.swift index 032b4bec..0d9ab315 100644 --- a/Sources/ColumbaApp/Services/AppServices.swift +++ b/Sources/ColumbaApp/Services/AppServices.swift @@ -2854,11 +2854,13 @@ public final class AppServices { // conversation title would otherwise stay stuck on the "Peer " // fallback even though the announce tells us the real name. This is // UPDATE-only (never creates a conversation for a bare announce) and - // only fills an empty/nil name (never clobbers one we already have). - if !displayName.isEmpty, let repo = self.messageRepository, - let convo = try? await repo.fetchConversation(data) { - if (convo.displayName ?? "").isEmpty { - try? await repo.updateDisplayName(data, displayName: displayName) + // only fills an empty/nil name or the exact generated hash fallback + // (never clobbers a custom or previously announced name). + if !displayName.isEmpty, let repo = self.messageRepository { + if (try? await repo.applyAnnouncedDisplayName( + data, + displayName: displayName + )) == true { DiagLog.log("[RNS] stamped display name onto convo \(data.map { String(format: "%02x", $0) }.joined().prefix(8))") } } diff --git a/Sources/ColumbaApp/Services/MessageRepository.swift b/Sources/ColumbaApp/Services/MessageRepository.swift index c48c3628..7f470b26 100644 --- a/Sources/ColumbaApp/Services/MessageRepository.swift +++ b/Sources/ColumbaApp/Services/MessageRepository.swift @@ -49,6 +49,9 @@ public actor MessageRepository { /// messages already use `IncomingMessageHandler.messageReceivedNotification`. public static let conversationActivityNotification = Notification.Name("network.columba.conversationActivity") + /// Posted after durable conversation metadata changes without a new message. + public static let conversationMetadataChangedNotification = + Notification.Name("network.columba.conversationMetadataChanged") public static let conversationHashUserInfoKey = "conversationHash" public static let stagedRetryMarker = "columba-app-retry-staged-v1" @@ -150,6 +153,50 @@ public actor MessageRepository { try await database.updateDisplayName(hash: conversationHash, displayName: displayName) } + /// Apply an announced peer name only while the durable row still contains + /// an empty name or its generated hash placeholder. The predicate and write + /// execute in one SQL statement so a concurrently saved custom nickname + /// cannot be overwritten between a read and a later update. + @discardableResult + public func applyAnnouncedDisplayName( + _ conversationHash: Data, + displayName: String + ) async throws -> Bool { + guard !displayName.isEmpty else { return false } + let generatedFallback = AppDataParser.generatedConversationName( + destinationHash: conversationHash + ) + let updated = try await replacementPool.write { db in + try db.execute( + sql: """ + UPDATE conversations + SET display_name = ?, updated_at = ? + WHERE destination_hash = ? + AND ( + display_name IS NULL + OR display_name = '' + OR display_name = ? COLLATE NOCASE + ) + """, + arguments: [ + displayName, + Date().timeIntervalSince1970, + conversationHash, + generatedFallback, + ] + ) + return db.changesCount == 1 + } + if updated { + NotificationCenter.default.post( + name: Self.conversationMetadataChangedNotification, + object: nil, + userInfo: [Self.conversationHashUserInfoKey: conversationHash] + ) + } + return updated + } + // MARK: - Icon Appearance /// Update peer icon appearance for a conversation. diff --git a/Sources/ColumbaApp/ViewModels/ChatsViewModel.swift b/Sources/ColumbaApp/ViewModels/ChatsViewModel.swift index 3b812306..6140de7c 100644 --- a/Sources/ColumbaApp/ViewModels/ChatsViewModel.swift +++ b/Sources/ColumbaApp/ViewModels/ChatsViewModel.swift @@ -151,6 +151,7 @@ public final class ChatsViewModel { private var inProcessObserver: NSObjectProtocol? private var conversationReadObserver: NSObjectProtocol? private var conversationActivityObserver: NSObjectProtocol? + private var conversationMetadataObserver: NSObjectProtocol? private var conversationLoadGeneration: UInt64 = 0 private var activeConversationLoadCount: Int = 0 private var activeConversationRefreshCount: Int = 0 @@ -207,6 +208,16 @@ public final class ChatsViewModel { await self?.loadConversations() } } + + conversationMetadataObserver = NotificationCenter.default.addObserver( + forName: MessageRepository.conversationMetadataChangedNotification, + object: nil, + queue: .main + ) { [weak self] _ in + Task { @MainActor in + await self?.loadConversations() + } + } } deinit { @@ -219,6 +230,9 @@ public final class ChatsViewModel { if let observer = conversationActivityObserver { NotificationCenter.default.removeObserver(observer) } + if let observer = conversationMetadataObserver { + NotificationCenter.default.removeObserver(observer) + } } // MARK: - Public Methods diff --git a/Sources/RNSAPI/Util/AppDataParser.swift b/Sources/RNSAPI/Util/AppDataParser.swift index bbeeab4b..20fd2d42 100644 --- a/Sources/RNSAPI/Util/AppDataParser.swift +++ b/Sources/RNSAPI/Util/AppDataParser.swift @@ -58,6 +58,30 @@ public enum AppDataParser { return "" } + /// Whether an announced name may replace a conversation's current name. + /// + /// Inbound messages can create a conversation before its peer announce is + /// observed. That row receives the generated `Peer ` placeholder, so + /// treating every non-empty name as user-owned leaves the placeholder stuck + /// forever. Replace only an empty value or the exact generated placeholder; + /// preserve custom nicknames and unrelated peer-like names. + public static func shouldReplaceConversationName( + _ existingName: String?, + destinationHash: Data + ) -> Bool { + guard let existingName, !existingName.isEmpty else { return true } + let generatedFallback = generatedConversationName(destinationHash: destinationHash) + return existingName.caseInsensitiveCompare(generatedFallback) == .orderedSame + } + + /// The placeholder used when a message arrives before its peer announce. + public static func generatedConversationName(destinationHash: Data) -> String { + let hashPrefix = destinationHash.prefix(4) + .map { String(format: "%02x", $0) } + .joined() + return "Peer \(hashPrefix)" + } + /// Pull a UTF-8 string out of a `.string` or `.binary` MessagePackValue. /// `.nil` / other cases → nil (so callers can fall back to ""). private static func string(_ value: MessagePackValue?) -> String? { diff --git a/Tests/ColumbaAppTests/AnnounceClassificationTests.swift b/Tests/ColumbaAppTests/AnnounceClassificationTests.swift index 4ff83282..8d2de9cd 100644 --- a/Tests/ColumbaAppTests/AnnounceClassificationTests.swift +++ b/Tests/ColumbaAppTests/AnnounceClassificationTests.swift @@ -303,6 +303,43 @@ final class MessageRepositoryAtomicReplacementTests: XCTestCase { } } + func testAnnouncedNameAtomicallyReplacesGeneratedFallback() async throws { + let databaseURL = temporaryDatabaseURL() + defer { removeDatabase(at: databaseURL) } + let repository = try MessageRepository(grdbPath: databaseURL.path) + let destination = Data([0x05, 0xc5, 0x7e, 0x42] + Array(repeating: 0xaa, count: 12)) + + try await repository.ensureConversation(destination, displayName: "Peer 05c57e42") + let applied = try await repository.applyAnnouncedDisplayName( + destination, + displayName: "Hermes Homelab" + ) + + let storedConversation = try await repository.fetchConversation(destination) + let conversation = try XCTUnwrap(storedConversation) + XCTAssertTrue(applied) + XCTAssertEqual(conversation.displayName, "Hermes Homelab") + } + + func testAnnouncedNameCompareAndSetPreservesCurrentCustomName() async throws { + let databaseURL = temporaryDatabaseURL() + defer { removeDatabase(at: databaseURL) } + let repository = try MessageRepository(grdbPath: databaseURL.path) + let destination = Data([0x05, 0xc5, 0x7e, 0x42] + Array(repeating: 0xaa, count: 12)) + + try await repository.ensureConversation(destination, displayName: "Peer 05c57e42") + try await repository.updateDisplayName(destination, displayName: "My Server") + let applied = try await repository.applyAnnouncedDisplayName( + destination, + displayName: "Hermes Homelab" + ) + + let storedConversation = try await repository.fetchConversation(destination) + let conversation = try XCTUnwrap(storedConversation) + XCTAssertFalse(applied) + XCTAssertEqual(conversation.displayName, "My Server") + } + func testRetryReplacementRekeysExactlyOneDurableRow() async throws { let databaseURL = temporaryDatabaseURL() defer { removeDatabase(at: databaseURL) } diff --git a/Tests/ColumbaAppTests/MicronParserTests.swift b/Tests/ColumbaAppTests/MicronParserTests.swift index c1a526c4..cf0f9cf4 100644 --- a/Tests/ColumbaAppTests/MicronParserTests.swift +++ b/Tests/ColumbaAppTests/MicronParserTests.swift @@ -772,6 +772,54 @@ final class MessageRepositoryAdapterTests: XCTestCase { XCTAssertEqual(viewModel.conversations.map(\.destinationHash), [newerHash, olderHash]) } + @MainActor + func testAnnouncedDisplayNameRefreshesVisibleChatList() async throws { + let databaseURL = FileManager.default.temporaryDirectory + .appendingPathComponent("columba-chat-announced-name-\(UUID().uuidString).sqlite") + defer { + try? FileManager.default.removeItem(at: databaseURL) + try? FileManager.default.removeItem(atPath: databaseURL.path + "-shm") + try? FileManager.default.removeItem(atPath: databaseURL.path + "-wal") + } + + let repository = try MessageRepository(grdbPath: databaseURL.path) + let viewModel = ChatsViewModel( + repository: repository, + notificationObserver: NotificationObserver() + ) + let destination = Data([0x05, 0xc5, 0x7e, 0x42] + Array(repeating: 0xaa, count: 12)) + let message = RNSAPI.LXMessage( + destinationHash: destination, + sourceIdentity: nil, + content: Data("hello".utf8) + ) + message.hash = Data(repeating: 0xbb, count: 32) + message.timestamp = 100 + message.incoming = true + message.sourceHash = destination + message.state = .received + message.method = .opportunistic + try await repository.saveMessage(message) + try await repository.ensureConversation(destination, displayName: "Peer 05c57e42") + await viewModel.loadConversations() + for _ in 0..<100 where viewModel.conversations.first?.displayName != "Peer 05c57e42" { + try await Task.sleep(for: .milliseconds(10)) + } + XCTAssertEqual(viewModel.conversations.first?.displayName, "Peer 05c57e42") + + let applied = try await repository.applyAnnouncedDisplayName( + destination, + displayName: "Hermes Homelab" + ) + XCTAssertTrue(applied) + + for _ in 0..<100 where viewModel.conversations.first?.displayName != "Hermes Homelab" { + try await Task.sleep(for: .milliseconds(10)) + } + + XCTAssertEqual(viewModel.conversations.first?.displayName, "Hermes Homelab") + } + @MainActor func testConversationReadNotificationClearsVisibleUnreadBadge() async throws { let databaseURL = FileManager.default.temporaryDirectory diff --git a/Tests/RNSAPITests/AppDataParserTests.swift b/Tests/RNSAPITests/AppDataParserTests.swift index fefffe3c..9965dd25 100644 --- a/Tests/RNSAPITests/AppDataParserTests.swift +++ b/Tests/RNSAPITests/AppDataParserTests.swift @@ -68,6 +68,19 @@ final class AppDataParserTests: XCTestCase { XCTAssertEqual(AppDataParser.displayName(from: Data("LegacyName".utf8), aspect: "lxmf.delivery"), "LegacyName") } + func testAnnouncedNameReplacesOnlyEmptyOrGeneratedHashFallback() { + let destinationHash = Data([0x05, 0xc5, 0x7e, 0x42] + Array(repeating: 0xaa, count: 12)) + + XCTAssertTrue(AppDataParser.shouldReplaceConversationName(nil, destinationHash: destinationHash)) + XCTAssertTrue(AppDataParser.shouldReplaceConversationName("", destinationHash: destinationHash)) + XCTAssertTrue(AppDataParser.shouldReplaceConversationName("Peer 05c57e42", destinationHash: destinationHash)) + XCTAssertTrue(AppDataParser.shouldReplaceConversationName("Peer 05C57E42", destinationHash: destinationHash)) + + XCTAssertFalse(AppDataParser.shouldReplaceConversationName("Hermes", destinationHash: destinationHash)) + XCTAssertFalse(AppDataParser.shouldReplaceConversationName("Peer Alice", destinationHash: destinationHash)) + XCTAssertFalse(AppDataParser.shouldReplaceConversationName("Peer deadbeef", destinationHash: destinationHash)) + } + // MARK: - Propagation (the regression) func testPropagationNoNameIsEmptyNotFalse() {