diff --git a/CHANGELOG.md b/CHANGELOG.md index 96bab93a3..3b832ba10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Wrong connection moved when dragging a list that holds a favorite or an active tag filter. (#1311) - Crash opening the connection list after two Macs moved two groups inside each other. (#1311) - Every saved group lost when one unreadable entry stopped the whole list decoding. (#1311) +- New tag discarded without a word when the name was already taken. +- Every saved tag replaced by the preset list when one unreadable entry stopped the library decoding. +- Tag created in one window missing from another until relaunch. +- Two Macs re-uploading the whole tag library to each other after a single tag changed. - Two Macs re-uploading the whole group list to each other after a single group changed. (#1311) - Deleting a group that a broken sync left in a loop also deleting the group it pointed at. (#1311) diff --git a/TablePro/Core/Services/Export/ConnectionExportService.swift b/TablePro/Core/Services/Export/ConnectionExportService.swift index ff3c8078b..6de73f80c 100644 --- a/TablePro/Core/Services/Export/ConnectionExportService.swift +++ b/TablePro/Core/Services/Export/ConnectionExportService.swift @@ -387,23 +387,23 @@ enum ConnectionExportService { } if let envelopeTags = preview.envelope.tags { - let existingTags = TagStorage.shared.loadTags() for exportTag in envelopeTags { - let alreadyExists = existingTags.contains { + /// Re-read per tag rather than once for the envelope: two tags sharing a name in + /// one file both passed a snapshot taken before either was added. + let alreadyExists = TagStorage.shared.loadTags().contains { $0.name.lowercased() == exportTag.name.lowercased() } - if !alreadyExists { - // Match preset tags by name - let preset = ConnectionTag.presets.first { - $0.name.lowercased() == exportTag.name.lowercased() - } - if let preset { - TagStorage.shared.addTag(preset) - } else { - let color = exportTag.color.flatMap { ConnectionColor(rawValue: $0) } ?? .gray - let tag = ConnectionTag(name: exportTag.name, color: color) - TagStorage.shared.addTag(tag) - } + guard !alreadyExists else { continue } + + let preset = ConnectionTag.presets.first { + $0.name.lowercased() == exportTag.name.lowercased() + } + let color = exportTag.color.flatMap { ConnectionColor(rawValue: $0) } ?? .gray + let tag = preset ?? ConnectionTag(name: exportTag.name, color: color) + do { + try TagStorage.shared.addTag(tag) + } catch { + Self.logger.error("Skipped importing tag: \(error.localizedDescription, privacy: .public)") } } } diff --git a/TablePro/Core/Storage/TagStorage.swift b/TablePro/Core/Storage/TagStorage.swift index 1d24c8689..dfd60edda 100644 --- a/TablePro/Core/Storage/TagStorage.swift +++ b/TablePro/Core/Storage/TagStorage.swift @@ -5,23 +5,51 @@ // Created by Claude on 20/12/25. // +import Combine import Foundation import os import TableProSyncTransport +internal enum TagStorageError: LocalizedError, Equatable { + case duplicateName(String) + case storeUnreadable + + internal var errorDescription: String? { + switch self { + case .duplicateName(let name): + return String(format: String(localized: "A tag named “%@” already exists."), name) + case .storeUnreadable: + return String(localized: "The saved tags could not be read. Nothing was changed.") + } + } +} + /// Service for persisting the global tag library @MainActor -final class TagStorage { - static let shared = TagStorage() +internal final class TagStorage { + internal static let shared = TagStorage() nonisolated private static let logger = Logger(subsystem: "com.TablePro", category: "TagStorage") private let tagsKey = "com.TablePro.tags" - private let defaults = AppStorageEnvironment.shared.defaults + private let defaults: UserDefaults + private let syncTracker: SyncChangeTracker + private let appEvents: AppEvents private let encoder = JSONEncoder() private let decoder = JSONDecoder() private var cachedTags: [ConnectionTag]? + /// Set when the stored payload could not be understood at all. Every mutation rewrites the + /// whole array, so continuing over an unreadable store would replace a user's own tags with + /// the preset list this falls back to for display. + private var storeIsUnreadable = false - private init() { + internal init( + userDefaults: UserDefaults = AppStorageEnvironment.shared.defaults, + syncTracker: SyncChangeTracker = .shared, + appEvents: AppEvents = .shared + ) { + self.defaults = userDefaults + self.syncTracker = syncTracker + self.appEvents = appEvents if loadTags().isEmpty { saveTags(ConnectionTag.presets) } @@ -30,74 +58,137 @@ final class TagStorage { // MARK: - Tag CRUD /// Load all tags (presets + custom) - func loadTags() -> [ConnectionTag] { + /// + /// A payload that decodes element by element keeps every tag it can read: one entry written by + /// a future version, or truncated on disk, used to take the whole library down with it and + /// leave the presets standing in its place. + internal func loadTags() -> [ConnectionTag] { if let cached = cachedTags { return cached } guard let data = defaults.data(forKey: tagsKey) else { + storeIsUnreadable = false let tags = ConnectionTag.presets cachedTags = tags return tags } - do { - let tags = try decoder.decode([ConnectionTag].self, from: data) - cachedTags = tags - return tags - } catch { - Self.logger.error("Failed to load tags: \(error)") - let tags = ConnectionTag.presets + if let tags = try? decoder.decode([ConnectionTag].self, from: data) { + storeIsUnreadable = false cachedTags = tags return tags } + + guard let salvaged = try? decoder.decode([SalvagedTag].self, from: data) else { + Self.logger.error("Tag store could not be read; leaving it untouched") + storeIsUnreadable = true + return ConnectionTag.presets + } + + let tags = salvaged.compactMap(\.tag) + Self.logger.error( + "Dropped \(salvaged.count - tags.count, privacy: .public) unreadable tag entries" + ) + storeIsUnreadable = false + cachedTags = tags + return tags } - /// Save all tags - func saveTags(_ tags: [ConnectionTag]) { + /// Save all tags. A save that failed leaves the store holding the previous set, so a caller + /// that goes on to write related state must check the result. + @discardableResult + internal func saveTags(_ tags: [ConnectionTag]) -> Bool { + guard !storeIsUnreadable else { + Self.logger.error("Refusing to overwrite an unreadable tag store") + return false + } + do { let data = try encoder.encode(tags) defaults.set(data, forKey: tagsKey) cachedTags = nil - SyncChangeTracker.shared.markDirty(.tag, ids: tags.map { $0.id.uuidString }) + syncTracker.markDirty(.tag, ids: tags.map { $0.id.uuidString }) + return true } catch { Self.logger.error("Failed to save tags: \(error)") + return false } } /// Add a new custom tag - func addTag(_ tag: ConnectionTag) { + internal func addTag(_ tag: ConnectionTag) throws { var tags = loadTags() guard !tags.contains(where: { $0.name.lowercased() == tag.name.lowercased() }) else { - return + throw TagStorageError.duplicateName(tag.name) } + tags.append(tag) - saveTags(tags) + guard saveTags(tags) else { throw TagStorageError.storeUnreadable } + notifyChanged() + } + + /// Apply a tag that arrived from another device. + /// + /// Written as it arrived, and skipped when it matches what is already stored: `saveTags` marks + /// every tag dirty and the push uploads every dirty tag, so writing an unchanged record + /// re-uploads the whole library to the device it came from, which writes it back. + @discardableResult + internal func applyRemoteTag(_ tag: ConnectionTag) -> Bool { + var tags = loadTags() + + if let index = tags.firstIndex(where: { $0.id == tag.id }) { + guard tags[index] != tag else { return false } + tags[index] = tag + } else { + tags.append(tag) + } + + return saveTags(tags) } /// Delete a custom tag (presets cannot be deleted) - func deleteTag(_ tag: ConnectionTag) { + internal func deleteTag(_ tag: ConnectionTag) { guard !tag.isPreset else { return } var tags = loadTags() tags.removeAll { $0.id == tag.id } - saveTags(tags) - SyncChangeTracker.shared.markDeleted(.tag, id: tag.id.uuidString) + guard saveTags(tags) else { return } + syncTracker.markDeleted(.tag, id: tag.id.uuidString) + notifyChanged() } /// Delete a custom tag and clear it from every connection that referenced it. /// Connections are persisted before the tag tombstone fires (sync delete-ordering invariant). - func deleteTag(_ tag: ConnectionTag, clearingFrom connectionStorage: ConnectionStorage) { + internal func deleteTag(_ tag: ConnectionTag, clearingFrom connectionStorage: ConnectionStorage) { guard !tag.isPreset else { return } connectionStorage.removeTagId(tag.id) deleteTag(tag) } /// Get tag by ID - func tag(for id: UUID) -> ConnectionTag? { + internal func tag(for id: UUID) -> ConnectionTag? { loadTags().first { $0.id == id } } /// Get tags for a list of IDs - func tags(for ids: [UUID]) -> [ConnectionTag] { + internal func tags(for ids: [UUID]) -> [ConnectionTag] { let allTags = loadTags() return ids.compactMap { id in allTags.first { $0.id == id } } } + + // MARK: - Private + + /// Announced from the mutators rather than from `saveTags`, because a sync pull applies one + /// record at a time and raises a single coalesced notification of its own for the batch. + private func notifyChanged() { + appEvents.connectionUpdated.send(nil) + } +} + +/// Decodes one tag and keeps going when it cannot, so a single unreadable entry costs that entry +/// rather than the whole library. +private struct SalvagedTag: Decodable { + let tag: ConnectionTag? + + init(from decoder: Decoder) throws { + tag = try? ConnectionTag(from: decoder) + } } diff --git a/TablePro/Core/Sync/SyncCoordinator.swift b/TablePro/Core/Sync/SyncCoordinator.swift index af6bc1e2a..6f61ade44 100644 --- a/TablePro/Core/Sync/SyncCoordinator.swift +++ b/TablePro/Core/Sync/SyncCoordinator.swift @@ -719,14 +719,7 @@ final class SyncCoordinator { guard let remoteTag = SyncRecordMapper.toTag(record) else { return false } if tombstoneIds.contains(remoteTag.id.uuidString) { return false } - var tags = services.tagStorage.loadTags() - if let index = tags.firstIndex(where: { $0.id == remoteTag.id }) { - tags[index] = remoteTag - } else { - tags.append(remoteTag) - } - services.tagStorage.saveTags(tags) - return true + return services.tagStorage.applyRemoteTag(remoteTag) } private func applyRemoteSSHProfile(_ record: CKRecord, tombstoneIds: Set) { diff --git a/TablePro/Resources/Localizable.xcstrings b/TablePro/Resources/Localizable.xcstrings index c53351a5f..7ec848570 100644 --- a/TablePro/Resources/Localizable.xcstrings +++ b/TablePro/Resources/Localizable.xcstrings @@ -240,6 +240,40 @@ } } }, + "A tag named “%@” already exists." : { + "localizations" : { + "ko" : { + "stringUnit" : { + "state" : "translated", + "value" : "“%@”라는 이름의 태그가 이미 있습니다." + } + }, + "tr" : { + "stringUnit" : { + "state" : "translated", + "value" : "“%@” adlı bir etiket zaten var." + } + }, + "vi" : { + "stringUnit" : { + "state" : "translated", + "value" : "Đã có thẻ tên “%@”." + } + }, + "zh-Hans" : { + "stringUnit" : { + "state" : "translated", + "value" : "已有名为“%@”的标签。" + } + }, + "zh-Hant" : { + "stringUnit" : { + "state" : "translated", + "value" : "已有名為「%@」的標籤。" + } + } + } + }, "Exclude the AUTO_INCREMENT counter" : { "extractionState" : "stale", "localizations" : { @@ -1602,6 +1636,40 @@ } } }, + "The saved tags could not be read. Nothing was changed." : { + "localizations" : { + "ko" : { + "stringUnit" : { + "state" : "translated", + "value" : "저장된 태그를 읽을 수 없습니다. 아무것도 변경되지 않았습니다." + } + }, + "tr" : { + "stringUnit" : { + "state" : "translated", + "value" : "Kayıtlı etiketler okunamadı. Hiçbir şey değiştirilmedi." + } + }, + "vi" : { + "stringUnit" : { + "state" : "translated", + "value" : "Không đọc được các thẻ đã lưu. Không có gì thay đổi." + } + }, + "zh-Hans" : { + "stringUnit" : { + "state" : "translated", + "value" : "无法读取已保存的标签。未做任何更改。" + } + }, + "zh-Hant" : { + "stringUnit" : { + "state" : "translated", + "value" : "無法讀取已儲存的標籤。未做任何變更。" + } + } + } + }, "(this Mac)" : { "extractionState" : "stale", "localizations" : { diff --git a/TablePro/Views/Connection/ConnectionTagEditor.swift b/TablePro/Views/Connection/ConnectionTagEditor.swift index 67c9891bc..c0771619c 100644 --- a/TablePro/Views/Connection/ConnectionTagEditor.swift +++ b/TablePro/Views/Connection/ConnectionTagEditor.swift @@ -25,11 +25,9 @@ struct ConnectionTagEditor: View { .sheet(isPresented: $showingCreateSheet) { CreateTagSheet { tagName, tagColor in let tag = ConnectionTag(name: tagName.lowercased(), isPreset: false, color: tagColor) - tagStorage.addTag(tag) + try tagStorage.addTag(tag) allTags = tagStorage.loadTags() - if let added = allTags.first(where: { $0.name == tag.name }) { - toggleOn(added.id) - } + toggleOn(tag.id) } } } @@ -168,7 +166,11 @@ private struct CreateTagSheet: View { @Environment(\.dismiss) private var dismiss @State private var tagName: String = "" @State private var tagColor: ConnectionColor = .gray - let onSave: (String, ConnectionColor) -> Void + @State private var errorMessage: String? + /// Throwing, because the library refuses a name it already holds. A sheet that dismissed on the + /// attempt applied the tag that was already there, with its colour rather than the one just + /// picked, and said nothing about the difference. + let onSave: (String, ConnectionColor) throws -> Void var body: some View { VStack(spacing: 16) { @@ -186,6 +188,14 @@ private struct CreateTagSheet: View { ColorPaletteView(selectedColor: $tagColor, includesNone: false, size: .compact) } + if let errorMessage { + Label(errorMessage, systemImage: "exclamationmark.triangle.fill") + .font(.caption) + .foregroundStyle(.red) + .fixedSize(horizontal: false, vertical: true) + .frame(maxWidth: .infinity, alignment: .leading) + } + HStack { Button("Cancel") { dismiss() @@ -193,8 +203,12 @@ private struct CreateTagSheet: View { .keyboardShortcut(.cancelAction) Button("Create") { - onSave(tagName, tagColor) - dismiss() + do { + try onSave(tagName, tagColor) + dismiss() + } catch { + errorMessage = error.localizedDescription + } } .buttonStyle(.borderedProminent) .keyboardShortcut(.defaultAction) @@ -203,6 +217,7 @@ private struct CreateTagSheet: View { } .padding(20) .frame(width: 300) + .onChange(of: tagName) { _, _ in errorMessage = nil } .onExitCommand { dismiss() } diff --git a/TableProTests/Core/Storage/TagStorageTests.swift b/TableProTests/Core/Storage/TagStorageTests.swift new file mode 100644 index 000000000..1963d12d6 --- /dev/null +++ b/TableProTests/Core/Storage/TagStorageTests.swift @@ -0,0 +1,160 @@ +// +// TagStorageTests.swift +// TableProTests +// + +import Combine +@testable import TablePro +import TableProSyncTransport +import XCTest + +@MainActor +final class TagStorageTests: XCTestCase { + private var suiteName: String! + private var defaults: UserDefaults! + private var syncSuiteName: String! + private var syncDefaults: UserDefaults! + private var metadata: SyncMetadataStorage! + private var tracker: SyncChangeTracker! + private var appEvents: AppEvents! + private var storage: TagStorage! + private var changeCount = 0 + private var changeSubscription: AnyCancellable? + + override func setUp() async throws { + try await super.setUp() + let unique = UUID().uuidString + suiteName = "com.TablePro.tests.TagStorage.\(unique)" + syncSuiteName = "com.TablePro.tests.TagStorage.sync.\(unique)" + defaults = try XCTUnwrap(UserDefaults(suiteName: suiteName)) + syncDefaults = try XCTUnwrap(UserDefaults(suiteName: syncSuiteName)) + metadata = SyncMetadataStorage(userDefaults: syncDefaults) + tracker = SyncChangeTracker(metadataStorage: metadata) + appEvents = AppEvents() + changeCount = 0 + changeSubscription = appEvents.connectionUpdated.sink { [weak self] _ in + self?.changeCount += 1 + } + storage = TagStorage(userDefaults: defaults, syncTracker: tracker, appEvents: appEvents) + changeCount = 0 + } + + override func tearDown() { + defaults.removePersistentDomain(forName: suiteName) + syncDefaults.removePersistentDomain(forName: syncSuiteName) + changeSubscription = nil + storage = nil + appEvents = nil + tracker = nil + metadata = nil + defaults = nil + syncDefaults = nil + suiteName = nil + syncSuiteName = nil + super.tearDown() + } + + private var customTag: ConnectionTag { + ConnectionTag(name: "staging", color: .orange) + } + + /// A store already on disk when the app starts. `init` seeds the presets through `loadTags`, + /// so an instance built first has a warm cache and never reads what a test writes after it. + private func makeStorage(seeding payload: Data) -> TagStorage { + defaults.set(payload, forKey: "com.TablePro.tags") + return TagStorage(userDefaults: defaults, syncTracker: tracker, appEvents: appEvents) + } + + // MARK: - Add + + func testAddTagReportsADuplicateName() throws { + try storage.addTag(customTag) + + XCTAssertThrowsError(try storage.addTag(ConnectionTag(name: "STAGING", color: .blue))) { error in + XCTAssertEqual(error as? TagStorageError, .duplicateName("STAGING")) + } + XCTAssertEqual(storage.loadTags().filter { $0.name.lowercased() == "staging" }.count, 1) + } + + func testAddingATagAnnouncesTheChange() throws { + try storage.addTag(customTag) + + XCTAssertEqual(changeCount, 1) + } + + func testARefusedAddAnnouncesNothing() throws { + try storage.addTag(customTag) + changeCount = 0 + + XCTAssertThrowsError(try storage.addTag(customTag)) + XCTAssertEqual(changeCount, 0) + } + + // MARK: - Delete + + func testDeletingATagAnnouncesTheChange() throws { + let tag = customTag + try storage.addTag(tag) + changeCount = 0 + + storage.deleteTag(tag) + + XCTAssertEqual(changeCount, 1) + XCTAssertTrue(metadata.tombstones(for: .tag).contains { $0.id == tag.id.uuidString }) + } + + func testAPresetIsNotDeleted() throws { + let preset = try XCTUnwrap(ConnectionTag.presets.first) + + storage.deleteTag(preset) + + XCTAssertTrue(storage.loadTags().contains { $0.id == preset.id }) + } + + // MARK: - Remote Apply + + /// saveTags marks every tag dirty and the push uploads every dirty tag, so writing a record + /// that changed nothing re-uploads the whole library to the device it came from. + func testApplyingAnUnchangedRemoteTagWritesNothing() throws { + let tag = customTag + try storage.addTag(tag) + tracker.clearAllDirty(.tag) + + XCTAssertFalse(storage.applyRemoteTag(tag)) + XCTAssertTrue(tracker.dirtyRecords(for: .tag).isEmpty) + } + + func testApplyingARemoteTagAnnouncesNothing() { + storage.applyRemoteTag(ConnectionTag(name: "from-another-mac")) + + XCTAssertEqual(changeCount, 0) + } + + // MARK: - Unreadable Store + + func testAnUnreadableStoreIsLeftUntouched() { + let junk = Data([0x00, 0x01, 0x02, 0x03]) + let storage = makeStorage(seeding: junk) + + XCTAssertEqual(storage.loadTags().map(\.name), ConnectionTag.presets.map(\.name)) + XCTAssertThrowsError(try storage.addTag(customTag)) { error in + XCTAssertEqual(error as? TagStorageError, .storeUnreadable) + } + XCTAssertEqual( + defaults.data(forKey: "com.TablePro.tags"), + junk, + "Every mutation rewrites the whole array, so writing over a store we could not read replaces the user's tags with the presets" + ) + } + + func testAnUnreadableEntryDoesNotTakeTheRestOfTheLibraryDown() throws { + let keep = ConnectionTag(name: "keep", color: .green) + let broken = ConnectionTag(name: "broken", color: .red) + let encoded = try JSONEncoder().encode([keep, broken]) + var elements = try XCTUnwrap(JSONSerialization.jsonObject(with: encoded) as? [[String: Any]]) + elements[1]["isPreset"] = "not a bool" + let storage = makeStorage(seeding: try JSONSerialization.data(withJSONObject: elements)) + + XCTAssertEqual(storage.loadTags().map(\.name), ["keep"]) + } +}