From aa10832141e23bded3ede2095749249ce1816c12 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Thu, 3 Sep 2026 13:47:52 +0700 Subject: [PATCH] fix(connections): make the group system report its refusals and survive a broken graph Claude-Session: https://claude.ai/code/session_01L81TaoPWxkLd15CGw2riPq --- CHANGELOG.md | 7 + TablePro/Core/Events/AppEvents.swift | 4 +- .../Export/ConnectionExportService.swift | 16 +- TablePro/Core/Storage/GroupStorage.swift | 196 ++++++++++++--- TablePro/Core/Sync/SyncCoordinator.swift | 16 +- .../Models/Connection/ConnectionGroup.swift | 4 + .../Connection/ConnectionGroupTree.swift | 96 +++++++- TablePro/Resources/Localizable.xcstrings | 204 +++++++++++++++ TablePro/ViewModels/WelcomeViewModel.swift | 148 +++++------ .../Connection/ConnectionGroupPicker.swift | 28 ++- .../Views/Connection/GroupPopUpButton.swift | 3 +- .../Views/Connection/WelcomeWindowView.swift | 40 +-- .../Core/Storage/GroupStorageTests.swift | 233 ++++++++++++++++-- .../Models/ConnectionGroupTreeTests.swift | 124 ++++++++++ .../ViewModels/WelcomeViewModelTests.swift | 88 ++++++- 15 files changed, 1028 insertions(+), 179 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ff9378f59..848e98c7e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Silent fallback order when foreign keys between the exported tables form a cycle. (#2517) - Foreign keys declared twice in a SQL export on MySQL, SQL Server, DuckDB, Snowflake, CockroachDB and Redshift, and as an unsupported `ALTER TABLE` on SQLite, libSQL and Cloudflare D1. (#2517) - Foreign keys missing from Redshift's reconstructed `CREATE TABLE`. +- New group discarded without a word when the connection form's picker could not save it. (#1311) +- Group created in one window missing from another until relaunch. (#1311) +- 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) +- 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) ## [0.71.0] - 2026-09-02 diff --git a/TablePro/Core/Events/AppEvents.swift b/TablePro/Core/Events/AppEvents.swift index 8b54369579..1e5462b689 100644 --- a/TablePro/Core/Events/AppEvents.swift +++ b/TablePro/Core/Events/AppEvents.swift @@ -114,7 +114,9 @@ final class AppEvents { let pluginsRejected = PassthroughSubject<[RejectedPlugin], Never>() - private init() {} + /// Not private so a test can hand an isolated bus to the object under test. App code uses + /// `shared`, which is the only instance anything observes. + init() {} } struct ConnectionStatusChange: Sendable { diff --git a/TablePro/Core/Services/Export/ConnectionExportService.swift b/TablePro/Core/Services/Export/ConnectionExportService.swift index ce721f0f5d..ff3c8078be 100644 --- a/TablePro/Core/Services/Export/ConnectionExportService.swift +++ b/TablePro/Core/Services/Export/ConnectionExportService.swift @@ -369,15 +369,19 @@ enum ConnectionExportService { resolutions: [UUID: ImportResolution] ) -> ImportResult { if let envelopeGroups = preview.envelope.groups { - let existingGroups = GroupStorage.shared.loadGroups() for exportGroup in envelopeGroups { - let alreadyExists = existingGroups.contains { + /// Re-read per group rather than once for the envelope: two groups sharing a name + /// in one file both passed a snapshot taken before either was added. + let alreadyExists = GroupStorage.shared.loadGroups().contains { $0.name.lowercased() == exportGroup.name.lowercased() } - if !alreadyExists { - let color = exportGroup.color.flatMap { ConnectionColor(rawValue: $0) } ?? .none - let group = ConnectionGroup(name: exportGroup.name, color: color) - GroupStorage.shared.addGroup(group) + guard !alreadyExists else { continue } + let color = exportGroup.color.flatMap { ConnectionColor(rawValue: $0) } ?? .none + let group = ConnectionGroup(name: exportGroup.name, color: color) + do { + try GroupStorage.shared.addGroup(group) + } catch { + Self.logger.error("Skipped importing group: \(error.localizedDescription, privacy: .public)") } } } diff --git a/TablePro/Core/Storage/GroupStorage.swift b/TablePro/Core/Storage/GroupStorage.swift index 55a64e4bb0..49430a56e3 100644 --- a/TablePro/Core/Storage/GroupStorage.swift +++ b/TablePro/Core/Storage/GroupStorage.swift @@ -3,101 +3,213 @@ // TablePro // +import Combine import Foundation import os import TableProSyncTransport +internal enum GroupStorageError: LocalizedError, Equatable { + case duplicateName(String) + case depthExceeded + case wouldCreateCycle + case groupNotFound + case storeUnreadable + + internal var errorDescription: String? { + switch self { + case .duplicateName(let name): + return String( + format: String(localized: "A group named “%@” already exists here."), name + ) + case .depthExceeded: + return String( + format: String(localized: "Groups nest up to %lld levels."), + ConnectionGroup.maxNestingDepth + ) + case .wouldCreateCycle: + return String(localized: "A group cannot be moved inside itself.") + case .groupNotFound: + return String(localized: "That group no longer exists.") + case .storeUnreadable: + return String(localized: "The saved groups could not be read. Nothing was changed.") + } + } +} + /// Service for persisting connection groups @MainActor -final class GroupStorage { - static let shared = GroupStorage() +internal final class GroupStorage { + internal static let shared = GroupStorage() nonisolated private static let logger = Logger(subsystem: "com.TablePro", category: "GroupStorage") private let groupsKey = "com.TablePro.groups" private let defaults: UserDefaults private let syncTracker: SyncChangeTracker private let connectionStorageProvider: () -> ConnectionStorage + private let appEvents: AppEvents private let encoder = JSONEncoder() private let decoder = JSONDecoder() private var cachedGroups: [ConnectionGroup]? + /// 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 the user's groups with + /// whatever the caller happened to be holding. + private var storeIsUnreadable = false - init( + internal init( userDefaults: UserDefaults = .standard, syncTracker: SyncChangeTracker = .shared, - connectionStorage: @escaping @autoclosure () -> ConnectionStorage = .shared + connectionStorage: @escaping @autoclosure () -> ConnectionStorage = .shared, + appEvents: AppEvents = .shared ) { self.defaults = userDefaults self.syncTracker = syncTracker self.connectionStorageProvider = connectionStorage + self.appEvents = appEvents } // MARK: - Group CRUD /// Load all groups - func loadGroups() -> [ConnectionGroup] { + /// + /// A payload that decodes element by element keeps every group it can read: one entry written + /// by a future version, or truncated on disk, used to take the whole list down with it. + internal func loadGroups() -> [ConnectionGroup] { if let cached = cachedGroups { return cached } guard let data = defaults.data(forKey: groupsKey) else { + storeIsUnreadable = false cachedGroups = [] return [] } - do { - let groups = try decoder.decode([ConnectionGroup].self, from: data) + if let groups = try? decoder.decode([ConnectionGroup].self, from: data) { + storeIsUnreadable = false cachedGroups = groups return groups - } catch { - Self.logger.error("Failed to load groups: \(error)") - cachedGroups = [] + } + + guard let salvaged = try? decoder.decode([SalvagedGroup].self, from: data) else { + Self.logger.error("Group store could not be read; leaving it untouched") + storeIsUnreadable = true return [] } + + let groups = salvaged.compactMap(\.group) + Self.logger.error( + "Dropped \(salvaged.count - groups.count, privacy: .public) unreadable group entries" + ) + storeIsUnreadable = false + cachedGroups = groups + return groups } - /// Save all groups - func saveGroups(_ groups: [ConnectionGroup]) { + /// Save all groups. Callers that go on to write related state must check the result: a save + /// that failed leaves the store holding the previous set. + @discardableResult + internal func saveGroups(_ groups: [ConnectionGroup]) -> Bool { + guard !storeIsUnreadable else { + Self.logger.error("Refusing to overwrite an unreadable group store") + return false + } + do { let data = try encoder.encode(groups) defaults.set(data, forKey: groupsKey) cachedGroups = nil syncTracker.markDirty(.group, ids: groups.map { $0.id.uuidString }) + return true } catch { Self.logger.error("Failed to save groups: \(error)") + return false } } /// Add a new group (duplicate check scoped to siblings, enforces depth cap and cycle prevention) - func addGroup(_ group: ConnectionGroup) { + internal func addGroup(_ group: ConnectionGroup) throws { var groups = loadGroups() - guard !wouldCreateCircle(movingGroupId: group.id, toParentId: group.parentId, groups: groups) else { return } - guard validateDepth(parentId: group.parentId) else { return } + try validatePlacement(of: group, in: groups) + let siblings = groups.filter { $0.parentId == group.parentId } guard !siblings.contains(where: { $0.name.lowercased() == group.name.lowercased() }) else { - return + throw GroupStorageError.duplicateName(group.name) } + groups.append(group) - saveGroups(groups) + guard saveGroups(groups) else { throw GroupStorageError.storeUnreadable } + notifyChanged() } /// Update an existing group (enforces cycle prevention and depth cap on parentId changes) - func updateGroup(_ group: ConnectionGroup) { + internal func updateGroup(_ group: ConnectionGroup) throws { var groups = loadGroups() - guard let index = groups.firstIndex(where: { $0.id == group.id }) else { return } + guard let index = groups.firstIndex(where: { $0.id == group.id }) else { + throw GroupStorageError.groupNotFound + } if group.parentId != groups[index].parentId { - guard !wouldCreateCircle(movingGroupId: group.id, toParentId: group.parentId, groups: groups) else { return } - guard validateDepth(parentId: group.parentId) else { return } + try validatePlacement(of: group, in: groups) } + groups[index] = group - saveGroups(groups) + guard saveGroups(groups) else { throw GroupStorageError.storeUnreadable } + notifyChanged() } - /// Delete a group and all descendant groups, nil-out groupId on affected connections - func deleteGroup(_ group: ConnectionGroup) { + /// Apply a group that arrived from another device, reporting whether anything changed. + /// + /// Written exactly as it arrived. A record cannot be judged on its own, because a pull carries + /// no dependency order: a hierarchy the other device reversed legally, rooting B and then + /// moving A under B, arrives as two records, and whichever lands first describes a state that + /// looks like a cycle against the half of the change that has not arrived yet. Repairing it + /// here would root A permanently, and the next push would send that back as a revert of a move + /// the user made. `repairHierarchy` runs once the whole batch is in, when the graph is whole. + /// + /// The pull that calls this raises one change notification for the batch, so this raises none. + /// + /// A record identical to the one already stored is skipped, because `saveGroups` marks every + /// group dirty and the push uploads every dirty group. Writing an unchanged record therefore + /// re-uploads the whole list, which the other device receives and writes back, and two Macs + /// trade the same records forever. The iOS coordinator has always had this guard. + @discardableResult + internal func applyRemoteGroup(_ group: ConnectionGroup) -> Bool { var groups = loadGroups() + + if let index = groups.firstIndex(where: { $0.id == group.id }) { + guard groups[index] != group else { return false } + groups[index] = group + } else { + groups.append(group) + } + + return saveGroups(groups) + } + + /// Root every group left on a parent cycle, reporting whether anything moved. + /// + /// Called once a pull has applied every record it carried, which is the first moment the graph + /// can be judged. A cycle that survives to here was authored by a device that should not have + /// been able to author one, or predates the validation, and leaving it stored would make + /// `deleteGroup` compute a subtree the list does not show. + @discardableResult + internal func repairHierarchy() -> Bool { + let groups = loadGroups() + let repaired = groupsWithReachableParents(groups) + guard repaired != groups else { return false } + + Self.logger.error("Rooting \(cyclicGroupIds(groups).count, privacy: .public) groups left on a parent cycle") + return saveGroups(repaired) + } + + /// Delete a group and all descendant groups, nil-out groupId on affected connections + /// The delete set comes from the graph the list draws, not the raw stored one. A group left on + /// a cycle is drawn at the top level, and each member of that cycle is a descendant of the + /// other in the raw graph, so deleting one displayed root used to take the other with it. + internal func deleteGroup(_ group: ConnectionGroup) { + var groups = groupsWithReachableParents(loadGroups()) let descendantIds = collectAllDescendantGroupIds(groupId: group.id, groups: groups) let allIdsToDelete = descendantIds.union([group.id]) groups.removeAll { allIdsToDelete.contains($0.id) } - saveGroups(groups) + guard saveGroups(groups) else { return } for deletedId in allIdsToDelete { syncTracker.markDeleted(.group, id: deletedId.uuidString) @@ -117,18 +229,38 @@ final class GroupStorage { Self.logger.error("Failed to clear groupId references after group deletion") } } + notifyChanged() } /// Get group by ID - func group(for id: UUID) -> ConnectionGroup? { + internal func group(for id: UUID) -> ConnectionGroup? { loadGroups().first { $0.id == id } } - /// Validate that adding a child under parentId would not exceed max depth - func validateDepth(parentId: UUID?, maxDepth: Int = 3) -> Bool { - guard let pid = parentId else { return true } - let groups = loadGroups() - let parentDepth = depthOf(groupId: pid, groups: groups) - return parentDepth < maxDepth + // MARK: - Private + + /// Announced from the mutators rather than from `saveGroups`, 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) + } + + private func validatePlacement(of group: ConnectionGroup, in groups: [ConnectionGroup]) throws { + guard !wouldCreateCircle(movingGroupId: group.id, toParentId: group.parentId, groups: groups) else { + throw GroupStorageError.wouldCreateCycle + } + guard canPlaceGroup(group.id, under: group.parentId, groups: groups) else { + throw GroupStorageError.depthExceeded + } + } +} + +/// Decodes one group and keeps going when it cannot, so a single unreadable entry costs that entry +/// rather than the whole list. +private struct SalvagedGroup: Decodable { + let group: ConnectionGroup? + + init(from decoder: Decoder) throws { + group = try? ConnectionGroup(from: decoder) } } diff --git a/TablePro/Core/Sync/SyncCoordinator.swift b/TablePro/Core/Sync/SyncCoordinator.swift index 67e027f3e5..af6bc1e2a6 100644 --- a/TablePro/Core/Sync/SyncCoordinator.swift +++ b/TablePro/Core/Sync/SyncCoordinator.swift @@ -632,6 +632,13 @@ final class SyncCoordinator { } } + /// After the batch, never per record: a pull carries no dependency order, so a legal + /// hierarchy change spread over two records passes through a state that reads as a cycle + /// until both have landed. + if groupsOrTagsChanged { + services.groupStorage.repairHierarchy() + } + if actualConnectionChanges || groupsOrTagsChanged { services.appEvents.connectionUpdated.send(nil) } @@ -704,14 +711,7 @@ final class SyncCoordinator { guard let remoteGroup = SyncRecordMapper.toGroup(record) else { return false } if tombstoneIds.contains(remoteGroup.id.uuidString) { return false } - var groups = services.groupStorage.loadGroups() - if let index = groups.firstIndex(where: { $0.id == remoteGroup.id }) { - groups[index] = remoteGroup - } else { - groups.append(remoteGroup) - } - services.groupStorage.saveGroups(groups) - return true + return services.groupStorage.applyRemoteGroup(remoteGroup) } @discardableResult diff --git a/TablePro/Models/Connection/ConnectionGroup.swift b/TablePro/Models/Connection/ConnectionGroup.swift index 36265af210..a18ec1fac2 100644 --- a/TablePro/Models/Connection/ConnectionGroup.swift +++ b/TablePro/Models/Connection/ConnectionGroup.swift @@ -7,6 +7,10 @@ import Foundation /// A named group (folder) for organizing database connections struct ConnectionGroup: Identifiable, Hashable, Codable { + /// How deep groups may nest. Read by the storage that enforces it, the tree that renders it, + /// and the picker that dims a parent past it, so the three cannot drift apart. + static let maxNestingDepth = 3 + let id: UUID var name: String var color: ConnectionColor diff --git a/TablePro/Models/Connection/ConnectionGroupTree.swift b/TablePro/Models/Connection/ConnectionGroupTree.swift index 37196ff0a0..757ba67f8b 100644 --- a/TablePro/Models/Connection/ConnectionGroupTree.swift +++ b/TablePro/Models/Connection/ConnectionGroupTree.swift @@ -17,6 +17,59 @@ enum ConnectionGroupTreeNode: Identifiable { } } +// MARK: - Reachability + +/// Every group that lies on a parent cycle. A group that merely descends from one is left alone, +/// because rooting the cycle itself already makes the whole chain reachable again. +func cyclicGroupIds(_ groups: [ConnectionGroup]) -> Set { + let byId = Dictionary(groups.map { ($0.id, $0) }, uniquingKeysWith: { first, _ in first }) + var cyclic: Set = [] + var acyclic: Set = [] + + for group in groups { + var path: [UUID] = [] + var onPath: Set = [] + var current: ConnectionGroup? = group + + while let node = current { + if acyclic.contains(node.id) || cyclic.contains(node.id) { break } + if onPath.contains(node.id) { + if let start = path.firstIndex(of: node.id) { + cyclic.formUnion(path[start...]) + } + break + } + path.append(node.id) + onPath.insert(node.id) + current = node.parentId.flatMap { byId[$0] } + } + + acyclic.formUnion(path.filter { !cyclic.contains($0) }) + } + + return cyclic +} + +/// The groups as every reader must interpret them: one whose parent chain never reaches the root +/// is presented at the top level rather than under its own parent. +/// +/// Two Macs that reparent P under C and C under P inside one sync window each apply the other's +/// record, and the stored graph then has no root for either. Left under their own parent, neither +/// is reachable from the root, so both groups and every connection inside them leave the tree +/// with no row left to repair them from. Rooting them keeps a broken graph visible, and it is also +/// what stops a descendant walk following the cycle forever. +func groupsWithReachableParents(_ groups: [ConnectionGroup]) -> [ConnectionGroup] { + let cyclic = cyclicGroupIds(groups) + guard !cyclic.isEmpty else { return groups } + + return groups.map { group in + guard cyclic.contains(group.id) else { return group } + var rooted = group + rooted.parentId = nil + return rooted + } +} + // MARK: - Tree Building func buildGroupTree( @@ -26,6 +79,7 @@ func buildGroupTree( maxDepth: Int = 3, currentDepth: Int = 0 ) -> [ConnectionGroupTreeNode] { + let groups = groupsWithReachableParents(groups) var items: [ConnectionGroupTreeNode] = [] let validGroupIds = Set(groups.map(\.id)) @@ -158,20 +212,49 @@ func wouldCreateCircle(movingGroupId: UUID, toParentId: UUID?, groups: [Connecti return descendants.contains(targetId) } -func depthOf(groupId: UUID?, groups: [ConnectionGroup], visited: Set = []) -> Int { +func depthOf(groupId: UUID?, groups: [ConnectionGroup]) -> Int { + depthOfReachable(groupId: groupId, groups: groupsWithReachableParents(groups), visited: []) +} + +private func depthOfReachable(groupId: UUID?, groups: [ConnectionGroup], visited: Set) -> Int { guard let gid = groupId else { return 0 } guard !visited.contains(gid) else { return 0 } guard let group = groups.first(where: { $0.id == gid }) else { return 0 } - return 1 + depthOf(groupId: group.parentId, groups: groups, visited: visited.union([gid])) + return 1 + depthOfReachable(groupId: group.parentId, groups: groups, visited: visited.union([gid])) +} + +/// Whether a group may sit under a parent: it cannot enclose itself, and the subtree it carries +/// still has to fit inside the nesting cap. +/// +/// The storage enforces this and the menus dim against it, so both ask the same function. A group +/// that is not in `groups` yet carries no subtree, which is what makes this the rule for a new +/// group as well as for a move. +func canPlaceGroup(_ groupId: UUID, under parentId: UUID?, groups: [ConnectionGroup]) -> Bool { + guard !wouldCreateCircle(movingGroupId: groupId, toParentId: parentId, groups: groups) else { return false } + let parentDepth = depthOf(groupId: parentId, groups: groups) + let subtreeDepth = maxDescendantDepth(groupId: groupId, groups: groups) + return parentDepth + 1 + subtreeDepth <= ConnectionGroup.maxNestingDepth } func maxDescendantDepth(groupId: UUID, groups: [ConnectionGroup]) -> Int { - let children = groups.filter { $0.parentId == groupId } + maxDescendantDepthReachable(groupId: groupId, groups: groupsWithReachableParents(groups), visited: []) +} + +private func maxDescendantDepthReachable( + groupId: UUID, + groups: [ConnectionGroup], + visited: Set +) -> Int { + let children = groups.filter { $0.parentId == groupId && !visited.contains($0.id) } if children.isEmpty { return 0 } - return 1 + (children.map { maxDescendantDepth(groupId: $0.id, groups: groups) }.max() ?? 0) + let nextVisited = visited.union([groupId]) + return 1 + (children.map { + maxDescendantDepthReachable(groupId: $0.id, groups: groups, visited: nextVisited) + }.max() ?? 0) } func connectionCount(in groupId: UUID, connections: [DatabaseConnection], groups: [ConnectionGroup]) -> Int { + let groups = groupsWithReachableParents(groups) let directCount = connections.filter { $0.groupId == groupId }.count let descendants = collectAllDescendantGroupIds(groupId: groupId, groups: groups) let descendantCount = connections.filter { conn in @@ -203,7 +286,9 @@ private func sortGroups(_ groups: [ConnectionGroup]) -> [ConnectionGroup] { } } -private func sortConnections(_ connections: [DatabaseConnection]) -> [DatabaseConnection] { +/// The order the tree presents connections in. Anything that reorders them has to agree with it, +/// or a drag lands the row somewhere the list does not draw it. +func sortConnections(_ connections: [DatabaseConnection]) -> [DatabaseConnection] { connections.sorted { $0.sortOrder != $1.sortOrder ? $0.sortOrder < $1.sortOrder @@ -212,6 +297,7 @@ private func sortConnections(_ connections: [DatabaseConnection]) -> [DatabaseCo } private func buildGroupTreeIndex(groups: [ConnectionGroup], connections: [DatabaseConnection]) -> GroupTreeIndex { + let groups = groupsWithReachableParents(groups) let validGroupIds = Set(groups.map(\.id)) var childrenByParentId: [UUID?: [ConnectionGroup]] = [:] diff --git a/TablePro/Resources/Localizable.xcstrings b/TablePro/Resources/Localizable.xcstrings index 7396cb62e0..3d6df23004 100644 --- a/TablePro/Resources/Localizable.xcstrings +++ b/TablePro/Resources/Localizable.xcstrings @@ -172,6 +172,74 @@ } } }, + "A group cannot be moved inside itself." : { + "localizations" : { + "ko" : { + "stringUnit" : { + "state" : "translated", + "value" : "그룹을 자기 자신 안으로 이동할 수 없습니다." + } + }, + "tr" : { + "stringUnit" : { + "state" : "translated", + "value" : "Bir grup kendi içine taşınamaz." + } + }, + "vi" : { + "stringUnit" : { + "state" : "translated", + "value" : "Không thể di chuyển một nhóm vào chính nó." + } + }, + "zh-Hans" : { + "stringUnit" : { + "state" : "translated", + "value" : "分组不能移动到自身内部。" + } + }, + "zh-Hant" : { + "stringUnit" : { + "state" : "translated", + "value" : "群組無法移動到自身內部。" + } + } + } + }, + "A group named “%@” already exists here." : { + "localizations" : { + "ko" : { + "stringUnit" : { + "state" : "translated", + "value" : "이 위치에 “%@” 그룹이 이미 있습니다." + } + }, + "tr" : { + "stringUnit" : { + "state" : "translated", + "value" : "Burada “%@” adlı bir grup zaten var." + } + }, + "vi" : { + "stringUnit" : { + "state" : "translated", + "value" : "Đã có nhóm tên “%@” ở đây." + } + }, + "zh-Hans" : { + "stringUnit" : { + "state" : "translated", + "value" : "此处已有名为“%@”的分组。" + } + }, + "zh-Hant" : { + "stringUnit" : { + "state" : "translated", + "value" : "此處已有名為「%@」的群組。" + } + } + } + }, "Exclude the AUTO_INCREMENT counter" : { "extractionState" : "stale", "localizations" : { @@ -242,6 +310,74 @@ } } }, + "Group Not Updated" : { + "localizations" : { + "ko" : { + "stringUnit" : { + "state" : "translated", + "value" : "그룹이 업데이트되지 않음" + } + }, + "tr" : { + "stringUnit" : { + "state" : "translated", + "value" : "Grup Güncellenmedi" + } + }, + "vi" : { + "stringUnit" : { + "state" : "translated", + "value" : "Nhóm chưa được cập nhật" + } + }, + "zh-Hans" : { + "stringUnit" : { + "state" : "translated", + "value" : "分组未更新" + } + }, + "zh-Hant" : { + "stringUnit" : { + "state" : "translated", + "value" : "群組未更新" + } + } + } + }, + "Groups nest up to %lld levels." : { + "localizations" : { + "ko" : { + "stringUnit" : { + "state" : "translated", + "value" : "그룹은 최대 %lld단계까지 중첩됩니다." + } + }, + "tr" : { + "stringUnit" : { + "state" : "translated", + "value" : "Gruplar en fazla %lld düzeye kadar iç içe geçebilir." + } + }, + "vi" : { + "stringUnit" : { + "state" : "translated", + "value" : "Nhóm lồng nhau tối đa %lld cấp." + } + }, + "zh-Hans" : { + "stringUnit" : { + "state" : "translated", + "value" : "分组最多嵌套 %lld 层。" + } + }, + "zh-Hant" : { + "stringUnit" : { + "state" : "translated", + "value" : "群組最多巢狀 %lld 層。" + } + } + } + }, "MySQL and MariaDB. Drops the account a view was created under. The importing account becomes the definer, so the view runs with its privileges. An account the target server does not have makes the import fail." : { "extractionState" : "stale", "localizations" : { @@ -1398,6 +1534,74 @@ } } }, + "That group no longer exists." : { + "localizations" : { + "ko" : { + "stringUnit" : { + "state" : "translated", + "value" : "해당 그룹이 더 이상 존재하지 않습니다." + } + }, + "tr" : { + "stringUnit" : { + "state" : "translated", + "value" : "Bu grup artık mevcut değil." + } + }, + "vi" : { + "stringUnit" : { + "state" : "translated", + "value" : "Nhóm đó không còn tồn tại." + } + }, + "zh-Hans" : { + "stringUnit" : { + "state" : "translated", + "value" : "该分组已不存在。" + } + }, + "zh-Hant" : { + "stringUnit" : { + "state" : "translated", + "value" : "該群組已不存在。" + } + } + } + }, + "The saved groups could not be read. Nothing was changed." : { + "localizations" : { + "ko" : { + "stringUnit" : { + "state" : "translated", + "value" : "저장된 그룹을 읽을 수 없습니다. 아무것도 변경되지 않았습니다." + } + }, + "tr" : { + "stringUnit" : { + "state" : "translated", + "value" : "Kayıtlı gruplar okunamadı. Hiçbir şey değiştirilmedi." + } + }, + "vi" : { + "stringUnit" : { + "state" : "translated", + "value" : "Không đọc được các nhóm đã 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/ViewModels/WelcomeViewModel.swift b/TablePro/ViewModels/WelcomeViewModel.swift index 9925b17a1b..fcf97c747f 100644 --- a/TablePro/ViewModels/WelcomeViewModel.swift +++ b/TablePro/ViewModels/WelcomeViewModel.swift @@ -69,6 +69,11 @@ final class WelcomeViewModel { var renameGroupName = "" var showRenameGroupAlert = false + /// Why a group change was refused. Renaming, recolouring and moving are commands with no + /// surface of their own to report into, so the window presents this; creating a group has its + /// own sheet and reports there instead. + var groupErrorMessage: String? + var connectionError: String? var showConnectionError = false var pluginDiagnostic: PluginDiagnosticItem? @@ -460,21 +465,20 @@ final class WelcomeViewModel { let isDuplicate = siblings.contains { $0.id != target.id && $0.name.lowercased() == newName.lowercased() } - guard !isDuplicate else { return } + guard !isDuplicate else { + groupErrorMessage = GroupStorageError.duplicateName(newName).localizedDescription + return + } var updated = target updated.name = newName - groupStorage.updateGroup(updated) - groups = groupStorage.loadGroups() - rebuildTree() + guard applyGroupUpdate(updated) else { return } renameGroupTarget = nil } func updateGroupColor(_ group: ConnectionGroup, color: ConnectionColor) { var updated = group updated.color = color - groupStorage.updateGroup(updated) - groups = groupStorage.loadGroups() - rebuildTree() + applyGroupUpdate(updated) } func moveConnections(_ targets: [DatabaseConnection], toGroup groupId: UUID) { @@ -507,11 +511,10 @@ final class WelcomeViewModel { rebuildTree() } - func createGroup(name: String, color: ConnectionColor, parentId: UUID?) { + func createGroup(name: String, color: ConnectionColor, parentId: UUID?) throws { let group = ConnectionGroup(name: name, color: color, parentId: parentId) - groupStorage.addGroup(group) + try groupStorage.addGroup(group) groups = groupStorage.loadGroups() - guard groups.contains(where: { $0.id == group.id }) else { return } expandedGroupIds.insert(group.id) if let parentId { expandedGroupIds.insert(parentId) @@ -527,18 +530,26 @@ final class WelcomeViewModel { activeSheet = .newGroup(parentId: parentId) } + /// The placement rule lives in the storage that enforces it, so this no longer pre-checks what + /// it would only have to keep in step. The menu dims an impossible target through the same + /// `canPlaceGroup`, which leaves the throw for a graph that changed under the open menu. func moveGroup(_ group: ConnectionGroup, toParent newParentId: UUID?) { - guard !wouldCreateCircle(movingGroupId: group.id, toParentId: newParentId, groups: groups) else { return } - - let newParentDepth = depthOf(groupId: newParentId, groups: groups) - let subtreeDepth = maxDescendantDepth(groupId: group.id, groups: groups) - guard newParentDepth + 1 + subtreeDepth <= 3 else { return } - var updated = group updated.parentId = newParentId - groupStorage.updateGroup(updated) + applyGroupUpdate(updated) + } + + @discardableResult + private func applyGroupUpdate(_ group: ConnectionGroup) -> Bool { + do { + try groupStorage.updateGroup(group) + } catch { + groupErrorMessage = error.localizedDescription + return false + } groups = groupStorage.loadGroups() rebuildTree() + return true } // MARK: - Import / Export @@ -609,40 +620,41 @@ final class WelcomeViewModel { // MARK: - Reorder - func moveUngroupedConnections(from source: IndexSet, to destination: Int) { - let validGroupIds = Set(groups.map(\.id)) - let ungroupedIndices = connections.indices.filter { index in - guard let groupId = connections[index].groupId else { return true } - return !validGroupIds.contains(groupId) - } - - guard source.allSatisfy({ $0 < ungroupedIndices.count }), - destination <= ungroupedIndices.count else { return } - - let globalSource = IndexSet(source.map { ungroupedIndices[$0] }) - let globalDestination: Int - if destination < ungroupedIndices.count { - globalDestination = ungroupedIndices[destination] - } else if let last = ungroupedIndices.last { - globalDestination = last + 1 - } else { - globalDestination = 0 + /// Reorder the rows the list actually drew. + /// + /// `.onMove` reports positions in the rendered node list, and that list is not `connections`: + /// a top level hides every favorite, and a tag filter hides whatever it does not match. Mapping + /// those offsets into the unfiltered array moved a different connection than the one dragged, + /// so the ids come in from the view and the offsets are only ever applied to them. + /// + /// Connections the list did not draw keep the slots they held, so a drag between two visible + /// rows cannot reshuffle the rows around them. + func moveConnections(renderedIds: [UUID], from source: IndexSet, to destination: Int, inGroup groupId: UUID?) { + guard source.allSatisfy({ $0 < renderedIds.count }), destination <= renderedIds.count else { return } + + var reordered = renderedIds + reordered.move(fromOffsets: source, toOffset: destination) + + let renderedSet = Set(renderedIds) + let scope = sortConnections(connections.filter { isInScope($0, groupId: groupId) }) + guard scope.filter({ renderedSet.contains($0.id) }).count == renderedIds.count else { return } + + var cursor = 0 + var rankById: [UUID: Int] = [:] + for (rank, connection) in scope.enumerated() { + if renderedSet.contains(connection.id) { + rankById[reordered[cursor]] = rank + cursor += 1 + } else { + rankById[connection.id] = rank + } } - connections.move(fromOffsets: globalSource, toOffset: globalDestination) - - let updatedValidGroupIds = Set(groups.map(\.id)) - var order = 0 var updated: [DatabaseConnection] = [] - for i in connections.indices { - let isUngrouped = connections[i].groupId.map { !updatedValidGroupIds.contains($0) } ?? true - if isUngrouped { - if connections[i].sortOrder != order { - connections[i].sortOrder = order - updated.append(connections[i]) - } - order += 1 - } + for index in connections.indices { + guard let rank = rankById[connections[index].id], connections[index].sortOrder != rank else { continue } + connections[index].sortOrder = rank + updated.append(connections[index]) } guard storage.updateConnections(updated) else { @@ -653,40 +665,14 @@ final class WelcomeViewModel { rebuildTree() } - func moveGroupedConnections(in group: ConnectionGroup, from source: IndexSet, to destination: Int) { - let groupIndices = connections.indices.filter { connections[$0].groupId == group.id } - - guard source.allSatisfy({ $0 < groupIndices.count }), - destination <= groupIndices.count else { return } - - let globalSource = IndexSet(source.map { groupIndices[$0] }) - let globalDestination: Int - if destination < groupIndices.count { - globalDestination = groupIndices[destination] - } else if let last = groupIndices.last { - globalDestination = last + 1 - } else { - globalDestination = 0 + /// A connection with a `groupId` no group answers to is ungrouped, which is where the tree + /// draws it. + private func isInScope(_ connection: DatabaseConnection, groupId: UUID?) -> Bool { + guard let groupId else { + guard let assigned = connection.groupId else { return true } + return !groups.contains { $0.id == assigned } } - - connections.move(fromOffsets: globalSource, toOffset: globalDestination) - - var order = 0 - var updated: [DatabaseConnection] = [] - for i in connections.indices where connections[i].groupId == group.id { - if connections[i].sortOrder != order { - connections[i].sortOrder = order - updated.append(connections[i]) - } - order += 1 - } - - guard storage.updateConnections(updated) else { - connections = storage.loadConnections() - rebuildTree() - return - } - rebuildTree() + return connection.groupId == groupId } // MARK: - Private Helpers diff --git a/TablePro/Views/Connection/ConnectionGroupPicker.swift b/TablePro/Views/Connection/ConnectionGroupPicker.swift index b990459768..dc6f40545a 100644 --- a/TablePro/Views/Connection/ConnectionGroupPicker.swift +++ b/TablePro/Views/Connection/ConnectionGroupPicker.swift @@ -41,7 +41,7 @@ struct ConnectionGroupPicker: View { .sheet(isPresented: $showingCreateSheet) { CreateGroupSheet { groupName, groupColor, parentId in let group = ConnectionGroup(name: groupName, color: groupColor, parentId: parentId) - groupStorage.addGroup(group) + try groupStorage.addGroup(group) selectedGroupId = group.id allGroups = groupStorage.loadGroups() } @@ -57,11 +57,15 @@ struct CreateGroupSheet: View { @State private var groupColor: ConnectionColor = .none @State private var selectedParentId: UUID? @State private var allGroups: [ConnectionGroup] = [] + @State private var errorMessage: String? private let initialParentId: UUID? - let onSave: (String, ConnectionColor, UUID?) -> Void + /// Throwing, because the store refuses a duplicate sibling name, a cycle and a group nested + /// past the cap. A sheet that dismissed on the attempt left the caller holding the id of a + /// group that was never saved. + let onSave: (String, ConnectionColor, UUID?) throws -> Void - init(parentId: UUID? = nil, onSave: @escaping (String, ConnectionColor, UUID?) -> Void) { + init(parentId: UUID? = nil, onSave: @escaping (String, ConnectionColor, UUID?) throws -> Void) { self.initialParentId = parentId self.onSave = onSave } @@ -91,6 +95,14 @@ struct CreateGroupSheet: View { } } + 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() @@ -98,8 +110,12 @@ struct CreateGroupSheet: View { .keyboardShortcut(.cancelAction) Button("Create") { - onSave(groupName, groupColor, selectedParentId) - dismiss() + do { + try onSave(groupName, groupColor, selectedParentId) + dismiss() + } catch { + errorMessage = error.localizedDescription + } } .buttonStyle(.borderedProminent) .keyboardShortcut(.defaultAction) @@ -108,6 +124,8 @@ struct CreateGroupSheet: View { } .padding(20) .frame(width: 300) + .onChange(of: groupName) { _, _ in errorMessage = nil } + .onChange(of: selectedParentId) { _, _ in errorMessage = nil } .onAppear { allGroups = GroupStorage.shared.loadGroups() selectedParentId = initialParentId diff --git a/TablePro/Views/Connection/GroupPopUpButton.swift b/TablePro/Views/Connection/GroupPopUpButton.swift index f7d6d894da..0020c3df29 100644 --- a/TablePro/Views/Connection/GroupPopUpButton.swift +++ b/TablePro/Views/Connection/GroupPopUpButton.swift @@ -35,8 +35,7 @@ internal struct GroupMenuEntry: Equatable, Identifiable { } internal enum GroupMenuEntries { - /// The maximum nesting a group is allowed to be moved under, mirroring the tree's own limit. - internal static let maximumDepth = 3 + private static let maximumDepth = ConnectionGroup.maxNestingDepth internal static func forConnection(groups: [ConnectionGroup], noneTitle: String) -> [GroupMenuEntry] { var entries = [GroupMenuEntry(id: nil, title: noneTitle)] diff --git a/TablePro/Views/Connection/WelcomeWindowView.swift b/TablePro/Views/Connection/WelcomeWindowView.swift index 2ce3f96e0d..2982d6ee8f 100644 --- a/TablePro/Views/Connection/WelcomeWindowView.swift +++ b/TablePro/Views/Connection/WelcomeWindowView.swift @@ -90,7 +90,7 @@ struct WelcomeWindowView: View { switch sheet { case .newGroup(let parentId): CreateGroupSheet(parentId: parentId) { name, color, pid in - vm.createGroup(name: name, color: color, parentId: pid) + try vm.createGroup(name: name, color: color, parentId: pid) } case .activation: LicenseActivationSheet() @@ -143,6 +143,19 @@ struct WelcomeWindowView: View { } message: { Text("Enter a new name for the group.") } + .alert( + String(localized: "Group Not Updated"), + isPresented: Binding( + get: { vm.groupErrorMessage != nil }, + set: { if !$0 { vm.groupErrorMessage = nil } } + ) + ) { + Button(String(localized: "OK")) { vm.groupErrorMessage = nil } + } message: { + if let message = vm.groupErrorMessage { + Text(message) + } + } .alert( String(localized: "Connection Failed"), isPresented: $vm.showConnectionError @@ -554,11 +567,15 @@ private struct TreeRowsView: View { } .onMove(perform: allConnections ? { from, to in guard vm.searchText.isEmpty else { return } - if let parentGroupId, let group = vm.groups.first(where: { $0.id == parentGroupId }) { - vm.moveGroupedConnections(in: group, from: from, to: to) - } else { - vm.moveUngroupedConnections(from: from, to: to) - } + vm.moveConnections( + renderedIds: items.compactMap { item in + guard case .connection(let conn) = item else { return nil } + return conn.id + }, + from: from, + to: to, + inGroup: parentGroupId + ) } : nil) } @@ -653,14 +670,7 @@ private struct TreeRowsView: View { Divider() ForEach(vm.groups.filter({ $0.id != group.id })) { targetGroup in - let wouldCircle = wouldCreateCircle( - movingGroupId: group.id, - toParentId: targetGroup.id, - groups: vm.groups - ) - let targetDepth = vm.depthByGroup[targetGroup.id] ?? 0 - let subtreeDepth = vm.maxDescendantDepthByGroup[group.id] ?? 0 - let wouldExceedDepth = targetDepth + 1 + subtreeDepth > 3 + let canPlace = canPlaceGroup(group.id, under: targetGroup.id, groups: vm.groups) Button { vm.moveGroup(group, toParent: targetGroup.id) @@ -677,7 +687,7 @@ private struct TreeRowsView: View { } } } - .disabled(wouldCircle || wouldExceedDepth || group.parentId == targetGroup.id) + .disabled(!canPlace || group.parentId == targetGroup.id) } } } diff --git a/TableProTests/Core/Storage/GroupStorageTests.swift b/TableProTests/Core/Storage/GroupStorageTests.swift index d7170066eb..9441335bb3 100644 --- a/TableProTests/Core/Storage/GroupStorageTests.swift +++ b/TableProTests/Core/Storage/GroupStorageTests.swift @@ -3,6 +3,7 @@ // TableProTests // +import Combine import TableProPluginKit @testable import TablePro import XCTest @@ -18,6 +19,9 @@ final class GroupStorageTests: XCTestCase { private var tracker: SyncChangeTracker! private var connectionStorage: ConnectionStorage! private var connectionFileURL: URL! + private var appEvents: AppEvents! + private var changeCount = 0 + private var changeSubscription: AnyCancellable? override func setUp() async throws { try await super.setUp() @@ -40,10 +44,16 @@ final class GroupStorageTests: XCTestCase { userDefaults: defaults, syncTracker: tracker ) + appEvents = AppEvents() + changeCount = 0 + changeSubscription = appEvents.connectionUpdated.sink { [weak self] _ in + self?.changeCount += 1 + } storage = GroupStorage( userDefaults: defaults, syncTracker: tracker, - connectionStorage: self.connectionStorage + connectionStorage: self.connectionStorage, + appEvents: appEvents ) } @@ -55,6 +65,8 @@ final class GroupStorageTests: XCTestCase { suiteName = nil syncDefaults = nil syncSuiteName = nil + changeSubscription = nil + appEvents = nil storage = nil tracker = nil connectionStorage = nil @@ -87,9 +99,9 @@ final class GroupStorageTests: XCTestCase { // MARK: - Add - func testAddGroup() { + func testAddGroup() throws { let group = ConnectionGroup(name: "Staging", color: .orange) - storage.addGroup(group) + try storage.addGroup(group) let loaded = storage.loadGroups() XCTAssertEqual(loaded.count, 1) @@ -97,28 +109,45 @@ final class GroupStorageTests: XCTestCase { XCTAssertEqual(loaded[0].id, group.id) } - func testAddGroupPreventsDuplicateNames() { + func testAddGroupReportsADuplicateSiblingName() throws { let group1 = ConnectionGroup(name: "Production", color: .red) let group2 = ConnectionGroup(name: "production", color: .blue) - storage.addGroup(group1) - storage.addGroup(group2) + try storage.addGroup(group1) + XCTAssertThrowsError(try storage.addGroup(group2)) { error in + XCTAssertEqual(error as? GroupStorageError, .duplicateName("production")) + } let loaded = storage.loadGroups() XCTAssertEqual(loaded.count, 1) XCTAssertEqual(loaded[0].color, .red) } + func testAddGroupReportsANestingPastTheCap() throws { + var parentId: UUID? + for level in 0.. [UUID] { + nodes.compactMap { node in + guard case .connection(let conn) = node else { return nil } + return conn.id + } + } + + /// The favorite is drawn in its own section, so the tree hands `.onMove` three rows while the + /// stored array still holds four. Mapping those offsets into the array moved the connection + /// one slot over from the one the user dragged. + func testReorderMovesTheRowTheListDrewWhenAFavoriteIsHidden() { + var favorite = DatabaseConnection(name: "A", type: .mysql, sortOrder: 0) + favorite.isFavorite = true + let b = DatabaseConnection(name: "B", type: .mysql, sortOrder: 1) + let c = DatabaseConnection(name: "C", type: .mysql, sortOrder: 2) + let d = DatabaseConnection(name: "D", type: .mysql, sortOrder: 3) + connectionStorage.saveConnections([favorite, b, c, d]) + viewModel.loadConnections() + + let rendered = renderedConnectionIds(viewModel.treeItems) + XCTAssertEqual(rendered, [b.id, c.id, d.id]) + + viewModel.moveConnections(renderedIds: rendered, from: IndexSet(integer: 2), to: 0, inGroup: nil) + + XCTAssertEqual(renderedConnectionIds(viewModel.treeItems), [d.id, b.id, c.id]) + XCTAssertEqual( + viewModel.connections.first { $0.id == favorite.id }?.sortOrder, + 0, + "A row the list did not draw keeps the slot it held" + ) + } + + func testReorderInsideAGroupIgnoresRowsATagFilterHid() throws { + let group = ConnectionGroup(name: "Acme") + try groupStorage.addGroup(group) + let tagId = UUID() + + var hidden = DatabaseConnection(name: "Hidden", type: .mysql, sortOrder: 0) + hidden.groupId = group.id + var first = DatabaseConnection(name: "First", type: .mysql, sortOrder: 1) + first.groupId = group.id + first.tagIds = [tagId] + var second = DatabaseConnection(name: "Second", type: .mysql, sortOrder: 2) + second.groupId = group.id + second.tagIds = [tagId] + connectionStorage.saveConnections([hidden, first, second]) + + viewModel.loadConnections() + viewModel.tagFilter = TagFilter(selectedIds: [tagId]) + + guard case .group(_, let children)? = viewModel.treeItems.first else { + XCTFail("The group is missing from the tree") + return + } + let rendered = renderedConnectionIds(children) + XCTAssertEqual(rendered, [first.id, second.id]) + + viewModel.moveConnections(renderedIds: rendered, from: IndexSet(integer: 1), to: 0, inGroup: group.id) + + guard case .group(_, let reordered)? = viewModel.treeItems.first else { + XCTFail("The group is missing from the tree") + return + } + XCTAssertEqual(renderedConnectionIds(reordered), [second.id, first.id]) + XCTAssertEqual( + viewModel.connections.first { $0.id == hidden.id }?.sortOrder, + 0, + "The filtered-out connection keeps the slot it held" + ) + } + // MARK: - Welcome Router Requests private func waitForChooser(timeout: TimeInterval = 2) async -> DatabaseTypeChooserPayload? {