From 64942799966271f325deed2f89a5a30be415749a Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 9 May 2026 22:23:16 +0700 Subject: [PATCH] fix(storage): propagate connection persistence failure instead of swallowing it --- CHANGELOG.md | 1 + TablePro/Core/Storage/ConnectionStorage.swift | 41 +++++++++++++++---- TablePro/Core/Storage/GroupStorage.swift | 4 +- TablePro/Core/Sync/SyncCoordinator.swift | 9 +++- TablePro/ViewModels/WelcomeViewModel.swift | 24 +++++++++-- .../ConnectionFormCoordinator.swift | 10 ++++- 6 files changed, 72 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d401afe3b..9c610ec9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- When saving the connections file fails (disk full, sandbox denied, encoding error), TablePro now aborts the dependent steps instead of continuing as if the save had succeeded. Previously a failed delete could remove the keychain password and queue a CloudKit tombstone for a record that was still on disk; on next sync the connection would be nuked from iCloud as well. Now: delete, add, update, duplicate, batch-delete, sync-pull, group-cleanup, and the plugin secure field migration all check the persistence result and skip the downstream side effects on failure. The connection form surfaces a localized error and keeps the form open instead of dismissing. - Result-grid cells on rows marked for deletion keep their dropdown / date / JSON / blob chevron visible at reduced opacity instead of hiding it, so the cell type is still legible while clearly inactive. Click on the dimmed chevron is a no-op; FK arrow navigation is unchanged. Matches the macOS HIG "disabled appearance" guideline. - Foreign key navigation from a table with unsaved edits opens the referenced table in a new window tab to preserve the edit buffer. Closing that new tab no longer wipes the original tab's data grid. Previously the new tab's teardown broadcast a connection-scoped event that other coordinators on the same connection received, causing them to release their cell data. - Tables sidebar refreshes automatically after a successful SQL import; the refresh notification now fires after the success sheet's dismissal animation, so the main window is key when the observer runs (#1114) diff --git a/TablePro/Core/Storage/ConnectionStorage.swift b/TablePro/Core/Storage/ConnectionStorage.swift index 6a34bdbea..0668185c4 100644 --- a/TablePro/Core/Storage/ConnectionStorage.swift +++ b/TablePro/Core/Storage/ConnectionStorage.swift @@ -103,16 +103,24 @@ final class ConnectionStorage { } } - /// Save all connections - func saveConnections(_ connections: [DatabaseConnection]) { + /// Save all connections. Returns `true` if persisted, `false` if encoding or + /// the atomic write failed. Callers that mutate dependent state (sync tracker, + /// keychain entries) MUST check the return value and abort on `false`. + /// Continuing on a failed save can nuke a user's password while leaving the + /// connection record on disk, then have the next sync delete the record from + /// iCloud too. + @discardableResult + func saveConnections(_ connections: [DatabaseConnection]) -> Bool { let storedConnections = connections.map { StoredConnection(from: $0) } do { let data = try encoder.encode(storedConnections) try data.write(to: fileURL, options: .atomic) cachedConnections = nil + return true } catch { Self.logger.error("Failed to save connections: \(error)") + return false } } @@ -125,7 +133,10 @@ final class ConnectionStorage { func addConnection(_ connection: DatabaseConnection, password: String? = nil) { var connections = loadConnections() connections.append(connection) - saveConnections(connections) + guard saveConnections(connections) else { + Self.logger.error("Aborted addConnection: persistence failed for \(connection.id, privacy: .public)") + return + } if !connection.localOnly && !connection.isSample { syncTracker.markDirty(.connection, id: connection.id.uuidString) } @@ -140,7 +151,10 @@ final class ConnectionStorage { var connections = loadConnections() if let index = connections.firstIndex(where: { $0.id == connection.id }) { connections[index] = connection - saveConnections(connections) + guard saveConnections(connections) else { + Self.logger.error("Aborted updateConnection: persistence failed for \(connection.id, privacy: .public)") + return + } if !connection.localOnly && !connection.isSample { syncTracker.markDirty(.connection, id: connection.id.uuidString) } @@ -159,7 +173,10 @@ final class ConnectionStorage { func deleteConnection(_ connection: DatabaseConnection) { var connections = loadConnections() connections.removeAll { $0.id == connection.id } - saveConnections(connections) + guard saveConnections(connections) else { + Self.logger.error("Aborted deleteConnection: persistence failed for \(connection.id, privacy: .public)") + return + } if !connection.localOnly && !connection.isSample { syncTracker.markDeleted(.connection, id: connection.id.uuidString) } @@ -181,7 +198,10 @@ final class ConnectionStorage { let idsToDelete = Set(connectionsToDelete.map(\.id)) var all = loadConnections() all.removeAll { idsToDelete.contains($0.id) } - saveConnections(all) + guard saveConnections(all) else { + Self.logger.error("Aborted deleteConnections: persistence failed for \(idsToDelete.count, privacy: .public) connection(s)") + return + } for conn in connectionsToDelete where !conn.localOnly && !conn.isSample { syncTracker.markDeleted(.connection, id: conn.id.uuidString) } @@ -233,7 +253,10 @@ final class ConnectionStorage { // Save the duplicate connection var connections = loadConnections() connections.append(duplicate) - saveConnections(connections) + guard saveConnections(connections) else { + Self.logger.error("Aborted duplicateConnection: persistence failed for \(duplicate.id, privacy: .public)") + return duplicate + } if !duplicate.localOnly { syncTracker.markDirty(.connection, id: duplicate.id.uuidString) } @@ -403,7 +426,9 @@ final class ConnectionStorage { } if changed { - saveConnections(connections) + if !saveConnections(connections) { + Self.logger.error("Failed to persist plugin secure field migration; will retry on next launch") + } } } } diff --git a/TablePro/Core/Storage/GroupStorage.swift b/TablePro/Core/Storage/GroupStorage.swift index 0870a85f9..a67d4b293 100644 --- a/TablePro/Core/Storage/GroupStorage.swift +++ b/TablePro/Core/Storage/GroupStorage.swift @@ -112,7 +112,9 @@ final class GroupStorage { } } if changed { - storage.saveConnections(connections) + if !storage.saveConnections(connections) { + Self.logger.error("Failed to clear groupId references after group deletion") + } } } diff --git a/TablePro/Core/Sync/SyncCoordinator.swift b/TablePro/Core/Sync/SyncCoordinator.swift index a645dd1f9..bd9ca4071 100644 --- a/TablePro/Core/Sync/SyncCoordinator.swift +++ b/TablePro/Core/Sync/SyncCoordinator.swift @@ -446,7 +446,9 @@ final class SyncCoordinator { if !connectionIdsToDelete.isEmpty { var connections = ConnectionStorage.shared.loadConnections() connections.removeAll { connectionIdsToDelete.contains($0.id) } - ConnectionStorage.shared.saveConnections(connections) + if !ConnectionStorage.shared.saveConnections(connections) { + Self.logger.error("Failed to apply remote connection deletions: persistence error") + } } if !groupIdsToDelete.isEmpty { var groups = GroupStorage.shared.loadGroups() @@ -504,7 +506,10 @@ final class SyncCoordinator { } else { connections.append(remoteConnection) } - ConnectionStorage.shared.saveConnections(connections) + guard ConnectionStorage.shared.saveConnections(connections) else { + Self.logger.error("Failed to apply remote connection update: persistence error for \(remoteConnection.id, privacy: .public)") + return false + } return true } diff --git a/TablePro/ViewModels/WelcomeViewModel.swift b/TablePro/ViewModels/WelcomeViewModel.swift index c0952b7e5..b2ba3723e 100644 --- a/TablePro/ViewModels/WelcomeViewModel.swift +++ b/TablePro/ViewModels/WelcomeViewModel.swift @@ -391,7 +391,11 @@ final class WelcomeViewModel { for i in connections.indices where ids.contains(connections[i].id) { connections[i].groupId = groupId } - storage.saveConnections(connections) + guard storage.saveConnections(connections) else { + connections = storage.loadConnections() + rebuildTree() + return + } rebuildTree() } @@ -400,7 +404,11 @@ final class WelcomeViewModel { for i in connections.indices where ids.contains(connections[i].id) { connections[i].groupId = nil } - storage.saveConnections(connections) + guard storage.saveConnections(connections) else { + connections = storage.loadConnections() + rebuildTree() + return + } rebuildTree() } @@ -526,7 +534,11 @@ final class WelcomeViewModel { } } - storage.saveConnections(connections) + guard storage.saveConnections(connections) else { + connections = storage.loadConnections() + rebuildTree() + return + } if !dirtyIds.isEmpty { services.syncTracker.markDirty(.connection, ids: dirtyIds) } @@ -561,7 +573,11 @@ final class WelcomeViewModel { order += 1 } - storage.saveConnections(connections) + guard storage.saveConnections(connections) else { + connections = storage.loadConnections() + rebuildTree() + return + } if !dirtyIds.isEmpty { services.syncTracker.markDirty(.connection, ids: dirtyIds) } diff --git a/TablePro/Views/ConnectionForm/ConnectionFormCoordinator.swift b/TablePro/Views/ConnectionForm/ConnectionFormCoordinator.swift index 28fbcd085..46849dd73 100644 --- a/TablePro/Views/ConnectionForm/ConnectionFormCoordinator.swift +++ b/TablePro/Views/ConnectionForm/ConnectionFormCoordinator.swift @@ -309,7 +309,10 @@ final class ConnectionFormCoordinator { var savedConnections = storage.loadConnections() if isNew { savedConnections.append(connectionToSave) - storage.saveConnections(savedConnections) + guard storage.saveConnections(savedConnections) else { + saveError = String(localized: "Could not save the connection. Check disk space and permissions, then try again.") + return + } if !connectionToSave.localOnly { services.syncTracker.markDirty(.connection, id: connectionToSave.id.uuidString) } @@ -324,7 +327,10 @@ final class ConnectionFormCoordinator { return } savedConnections[index] = connectionToSave - storage.saveConnections(savedConnections) + guard storage.saveConnections(savedConnections) else { + saveError = String(localized: "Could not save the connection. Check disk space and permissions, then try again.") + return + } if !connectionToSave.localOnly { services.syncTracker.markDirty(.connection, id: connectionToSave.id.uuidString) }