fix(connections): make the group system report its refusals and survive a broken graph

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
 

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 {

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)")
                 }
             }
         }

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)
     }
 }

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

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