refactor: clean up service layer, storage, and SwiftUI view patterns

Service layer:
- DB-1: offload DatabaseManager driver setup to nonisolated helper
- SVC-4: remove redundant NSLock from ExportService (@mainactor is sufficient)
- PLG-3: audit PluginMetadataRegistry thread-safety, document contract

Storage + sync:
- STG-2: move CKServerChangeToken from UserDefaults to Application Support file
- STG-3: delete stale iCloud Keychain copy on sync opt-out
- CKS-2: change CloudKit save policy to .ifServerRecordUnchanged with conflict retry
- APP-3.4: clean up transient connection Keychain entries after use

SwiftUI views:
- VIEW-1.3: replace NSAlert.runModal() with SwiftUI .alert in WelcomeWindowView
- VIEW-1.4: extract duplicated connection-ready logic into single method
- VIEW-2.2: replace NSNotificationCenter observer with async .task
- VIEW-4.1: use minWidth/minHeight instead of fixed frame on Settings
- VIEW-4.2: remove direct AppSettingsManager.shared from GeneralSettingsView
- VIEW-5.1: remove focusConnectionFormWindow() polling loop
- VIEW-7.1: deduplicate DoubleClickView into shared DoubleClickDetector

Author: datlechin

TablePro/AppDelegate+ConnectionHandler.swift

@@ -51,7 +51,11 @@ extension AppDelegate {
             connection = buildTransientConnection(from: parsed)
         }
 
+        let isTransient = matchedConnection == nil
+
         if !parsed.password.isEmpty {
+            // Save password to Keychain so the driver can resolve it at connect time.
+            // For transient connections, this is cleaned up after connect completes or fails.
             ConnectionStorage.shared.savePassword(parsed.password, for: connection.id)
         }
 
@@ -70,6 +74,14 @@ extension AppDelegate {
         openNewConnectionWindow(for: connection)
 
         Task { @MainActor in
+            defer {
+                // Clean up Keychain entry for transient connections after connect.
+                // The driver already holds the password in memory once connected,
+                // so the Keychain entry is no longer needed.
+                if isTransient {
+                    ConnectionStorage.shared.deletePassword(for: connection.id)
+                }
+            }
             do {
                 try await DatabaseManager.shared.connectToSession(connection)
                 for window in NSApp.windows where self.isWelcomeWindow(window) {

TablePro/Core/Database/DatabaseManager.swift

@@ -139,17 +139,11 @@ final class DatabaseManager {
         }
 
         do {
-            try await driver.connect()
-
-            // Apply query timeout from settings
-            let timeoutSeconds = AppSettingsManager.shared.general.queryTimeoutSeconds
-            if timeoutSeconds > 0 {
-                try await driver.applyQueryTimeout(timeoutSeconds)
-            }
-
-            // Run startup commands before schema init
-            await executeStartupCommands(
-                connection.startupCommands, on: driver, connectionName: connection.name
+            // Perform heavy driver setup off the main actor
+            try await performDriverSetup(
+                driver: driver,
+                startupCommands: connection.startupCommands,
+                connectionName: connection.name
             )
 
             // Initialize schema for drivers that support schema switching
@@ -605,16 +599,12 @@ final class DatabaseManager {
         // Use effective connection (tunneled) if available, otherwise original
         let connectionForDriver = session.effectiveConnection ?? session.connection
         let driver = try DatabaseDriverFactory.createDriver(for: connectionForDriver)
-        try await driver.connect()
-
-        // Apply timeout
-        let timeoutSeconds = AppSettingsManager.shared.general.queryTimeoutSeconds
-        if timeoutSeconds > 0 {
-            try await driver.applyQueryTimeout(timeoutSeconds)
-        }
 
-        await executeStartupCommands(
-            session.connection.startupCommands, on: driver, connectionName: session.connection.name
+        // Perform heavy driver setup off the main actor
+        try await performDriverSetup(
+            driver: driver,
+            startupCommands: session.connection.startupCommands,
+            connectionName: session.connection.name
         )
 
         if let savedSchema = session.currentSchema,
@@ -667,16 +657,12 @@ final class DatabaseManager {
 
             // Create new driver and connect
             let driver = try DatabaseDriverFactory.createDriver(for: effectiveConnection)
-            try await driver.connect()
-
-            // Apply timeout
-            let timeoutSeconds = AppSettingsManager.shared.general.queryTimeoutSeconds
-            if timeoutSeconds > 0 {
-                try await driver.applyQueryTimeout(timeoutSeconds)
-            }
 
-            await executeStartupCommands(
-                session.connection.startupCommands, on: driver, connectionName: session.connection.name
+            // Perform heavy driver setup off the main actor
+            try await performDriverSetup(
+                driver: driver,
+                startupCommands: session.connection.startupCommands,
+                connectionName: session.connection.name
             )
 
             if let savedSchema = activeSessions[sessionId]?.currentSchema,
@@ -768,6 +754,22 @@ final class DatabaseManager {
 
     nonisolated private static let startupLogger = Logger(subsystem: "com.TablePro", category: "DatabaseManager")
 
+    /// Connects the driver, applies query timeout, and runs startup commands off the main actor.
+    nonisolated private func performDriverSetup(
+        driver: DatabaseDriver,
+        startupCommands: String?,
+        connectionName: String
+    ) async throws {
+        try await driver.connect()
+
+        let timeoutSeconds = await AppSettingsManager.shared.general.queryTimeoutSeconds
+        if timeoutSeconds > 0 {
+            try await driver.applyQueryTimeout(timeoutSeconds)
+        }
+
+        await executeStartupCommands(startupCommands, on: driver, connectionName: connectionName)
+    }
+
     nonisolated private func executeStartupCommands(
         _ commands: String?, on driver: DatabaseDriver, connectionName: String
     ) async {

TablePro/Core/Plugins/PluginMetadataRegistry.swift

@@ -135,6 +135,13 @@ struct PluginMetadataSnapshot: Sendable {
     }
 }
 
+/// Thread-safety contract:
+/// - All mutable state (`snapshots`, `schemeIndex`, `reverseTypeIndex`) is protected by `lock`.
+/// - `registerBuiltInDefaults()` accesses state without the lock but is only called from `init()`,
+///   before the singleton is published to other threads.
+/// - `buildMetadataSnapshot(from:isDownloadable:)` is a pure function — no shared state access.
+/// - Not converted to a Swift `actor` because most call sites are synchronous; making every
+///   lookup `async` would be too invasive.
 final class PluginMetadataRegistry: @unchecked Sendable {
     static let shared = PluginMetadataRegistry()
 

TablePro/Core/Services/Export/ExportService.swift

@@ -72,20 +72,7 @@ final class ExportService {
 
     // MARK: - Cancellation
 
-    private let isCancelledLock = NSLock()
-    private var _isCancelled: Bool = false
-    var isCancelled: Bool {
-        get {
-            isCancelledLock.lock()
-            defer { isCancelledLock.unlock() }
-            return _isCancelled
-        }
-        set {
-            isCancelledLock.lock()
-            _isCancelled = newValue
-            isCancelledLock.unlock()
-        }
-    }
+    private(set) var isCancelled = false
 
     func cancelExport() {
         isCancelled = true

TablePro/Core/Storage/KeychainHelper.swift

@@ -263,10 +263,8 @@ final class KeychainHelper {
                 continue
             }
 
-            // When opting IN (synchronizable=true), delete the old local-only item safely.
-            // When opting OUT (synchronizable=false), keep the synchronizable item — deleting it
-            // would propagate via iCloud Keychain and remove it from other Macs still opted in.
             if synchronizable {
+                // When opting IN: delete the old local-only item safely.
                 let deleteQuery: [String: Any] = [
                     kSecClass as String: kSecClassGenericPassword,
                     kSecAttrService as String: service,
@@ -277,7 +275,24 @@ final class KeychainHelper {
                 let deleteStatus = SecItemDelete(deleteQuery as CFDictionary)
                 if deleteStatus != errSecSuccess, deleteStatus != errSecItemNotFound {
                     Self.logger.warning(
-                        "Migrated item '\(account, privacy: .public)' but failed to delete old entry: \(deleteStatus)"
+                        "Migrated item '\(account, privacy: .public)' but failed to delete old local entry: \(deleteStatus)"
+                    )
+                }
+            } else {
+                // When opting OUT: delete the stale synchronizable copy so it doesn't
+                // linger in iCloud Keychain. This will remove the credential from other
+                // devices that have iCloud Keychain enabled.
+                let deleteSyncQuery: [String: Any] = [
+                    kSecClass as String: kSecClassGenericPassword,
+                    kSecAttrService as String: service,
+                    kSecAttrAccount as String: account,
+                    kSecUseDataProtectionKeychain as String: true,
+                    kSecAttrSynchronizable as String: true
+                ]
+                let deleteStatus = SecItemDelete(deleteSyncQuery as CFDictionary)
+                if deleteStatus != errSecSuccess, deleteStatus != errSecItemNotFound {
+                    Self.logger.warning(
+                        "Migrated item '\(account, privacy: .public)' but failed to delete old synchronizable entry: \(deleteStatus)"
                     )
                 }
             }

TablePro/Core/Sync/CloudKitSyncEngine.swift

@@ -79,38 +79,98 @@ actor CloudKitSyncEngine {
     }
 
     private func pushBatch(records: [CKRecord], deletions: [CKRecord.ID]) async throws {
+        var recordsToSave = records
+
+        for attempt in 0..<Self.maxRetries {
+            let conflictedRecords = try await performPushOperation(
+                records: recordsToSave,
+                deletions: attempt == 0 ? deletions : []
+            )
+
+            if conflictedRecords.isEmpty { return }
+
+            Self.logger.info(
+                "Resolving \(conflictedRecords.count) conflict(s) (attempt \(attempt + 1)/\(Self.maxRetries))"
+            )
+
+            // Re-apply local changes onto the server's version of each conflicted record
+            recordsToSave = resolveConflicts(
+                localRecords: recordsToSave,
+                serverRecords: conflictedRecords
+            )
+        }
+
+        throw SyncError.unknown("Push failed after \(Self.maxRetries) conflict resolution attempts")
+    }
+
+    private func performPushOperation(
+        records: [CKRecord],
+        deletions: [CKRecord.ID]
+    ) async throws -> [CKRecord] {
         try await withRetry {
             let operation = CKModifyRecordsOperation(
                 recordsToSave: records,
                 recordIDsToDelete: deletions
             )
-            // Use .changedKeys so we don't need to track server change tags
-            // This overwrites only the fields we set, which is safe for our use case
-            operation.savePolicy = .changedKeys
+            // Use .ifServerRecordUnchanged to detect concurrent modifications.
+            // Conflicts are resolved by re-applying local changes onto the server record.
+            operation.savePolicy = .ifServerRecordUnchanged
             operation.isAtomic = false
 
             return try await withCheckedThrowingContinuation { continuation in
+                var conflicted: [CKRecord] = []
+
                 operation.perRecordSaveBlock = { recordID, result in
                     if case .failure(let error) = result {
-                        Self.logger.error(
-                            "Failed to save record \(recordID.recordName): \(error.localizedDescription)"
-                        )
+                        if let ckError = error as? CKError,
+                           ckError.code == .serverRecordChanged,
+                           let serverRecord = ckError.serverRecord {
+                            conflicted.append(serverRecord)
+                        } else {
+                            Self.logger.error(
+                                "Failed to save record \(recordID.recordName): \(error.localizedDescription)"
+                            )
+                        }
                     }
                 }
 
                 operation.modifyRecordsResultBlock = { result in
                     switch result {
                     case .success:
-                        continuation.resume()
+                        continuation.resume(returning: conflicted)
                     case .failure(let error):
-                        continuation.resume(throwing: error)
+                        // If the overall operation failed but we have conflicts, return them
+                        if !conflicted.isEmpty {
+                            continuation.resume(returning: conflicted)
+                        } else {
+                            continuation.resume(throwing: error)
+                        }
                     }
                 }
                 self.database.add(operation)
             }
         }
     }
 
+    /// Re-applies local field values onto the server's latest version of each conflicted record.
+    private func resolveConflicts(
+        localRecords: [CKRecord],
+        serverRecords: [CKRecord]
+    ) -> [CKRecord] {
+        let localByID = Dictionary(localRecords.map { ($0.recordID, $0) }, uniquingKeysWith: { _, new in new })
+
+        return serverRecords.compactMap { serverRecord in
+            guard let localRecord = localByID[serverRecord.recordID] else { return nil }
+
+            // Copy all locally-set fields onto the server record
+            for key in localRecord.allKeys() {
+                serverRecord[key] = localRecord[key]
+            }
+
+            return serverRecord
+        }
+    }
+
     // MARK: - Pull
 
     func pull(since token: CKServerChangeToken?) async throws -> PullResult {

TablePro/Core/Sync/SyncMetadataStorage.swift

@@ -16,15 +16,34 @@ final class SyncMetadataStorage {
 
     private let defaults = UserDefaults.standard
 
+    /// File URL for the sync token, stored in Application Support to avoid
+    /// accidental iCloud sync that can happen with UserDefaults.
+    private let syncTokenFileURL: URL = {
+        guard let appSupport = FileManager.default.urls(
+            for: .applicationSupportDirectory,
+            in: .userDomainMask
+        ).first else {
+            // Fallback to temp directory; should never happen on macOS
+            return FileManager.default.temporaryDirectory.appendingPathComponent("sync-token.data")
+        }
+        let directory = appSupport.appendingPathComponent("TablePro", isDirectory: true)
+        try? FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true)
+        return directory.appendingPathComponent("sync-token.data")
+    }()
+
     private enum Keys {
-        static let syncToken = "com.TablePro.sync.serverChangeToken"
         static let dirtyPrefix = "com.TablePro.sync.dirty."
         static let tombstonePrefix = "com.TablePro.sync.tombstones."
         static let lastSyncDate = "com.TablePro.sync.lastSyncDate"
         static let lastAccountId = "com.TablePro.sync.lastAccountId"
     }
 
-    private init() {}
+    /// Legacy UserDefaults key, used only for one-time migration.
+    private static let legacySyncTokenKey = "com.TablePro.sync.serverChangeToken"
+
+    private init() {
+        migrateSyncTokenFromUserDefaultsIfNeeded()
+    }
 
     // MARK: - Server Change Token
 
@@ -34,18 +53,18 @@ final class SyncMetadataStorage {
                 withRootObject: token,
                 requiringSecureCoding: true
             )
-            defaults.set(data, forKey: Keys.syncToken)
+            try data.write(to: syncTokenFileURL, options: .atomic)
         } catch {
-            Self.logger.error("Failed to archive sync token: \(error.localizedDescription)")
+            Self.logger.error("Failed to save sync token to file: \(error.localizedDescription)")
         }
     }
 
     func clearSyncToken() {
-        defaults.removeObject(forKey: Keys.syncToken)
+        try? FileManager.default.removeItem(at: syncTokenFileURL)
     }
 
     func loadSyncToken() -> CKServerChangeToken? {
-        guard let data = defaults.data(forKey: Keys.syncToken) else { return nil }
+        guard let data = try? Data(contentsOf: syncTokenFileURL) else { return nil }
 
         do {
             return try NSKeyedUnarchiver.unarchivedObject(
@@ -58,6 +77,21 @@ final class SyncMetadataStorage {
         }
     }
 
+    // MARK: - Migration
+
+    /// One-time migration of sync token from UserDefaults to Application Support file.
+    private func migrateSyncTokenFromUserDefaultsIfNeeded() {
+        guard let data = defaults.data(forKey: Self.legacySyncTokenKey) else { return }
+
+        do {
+            try data.write(to: syncTokenFileURL, options: .atomic)
+            defaults.removeObject(forKey: Self.legacySyncTokenKey)
+            Self.logger.info("Migrated sync token from UserDefaults to Application Support")
+        } catch {
+            Self.logger.error("Failed to migrate sync token to file: \(error.localizedDescription)")
+        }
+    }
+
     // MARK: - Dirty Entity Tracking
 
     func addDirty(type: SyncRecordType, id: String) {
@@ -167,7 +201,7 @@ final class SyncMetadataStorage {
     // MARK: - Clear All
 
     func clearAll() {
-        defaults.removeObject(forKey: Keys.syncToken)
+        clearSyncToken()
         defaults.removeObject(forKey: Keys.lastSyncDate)
         defaults.removeObject(forKey: Keys.lastAccountId)