From 707a6c3a6358401ec0d354082af4bda397d013cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ng=C3=B4=20Qu=E1=BB=91c=20=C4=90=E1=BA=A1t?= Date: Tue, 24 Mar 2026 08:36:47 +0700 Subject: [PATCH 1/5] fix: MongoDB Atlas auth failure due to wrong authSource default (#438) --- CHANGELOG.md | 5 +++++ Plugins/MongoDBDriverPlugin/MongoDBConnection.swift | 5 +++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c5b8a757..4fbd7202 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- MongoDB Atlas connections failing to authenticate (#438) +- MongoDB TLS certificate verification skipped for SRV connections + ## [0.23.1] - 2026-03-24 ### Added diff --git a/Plugins/MongoDBDriverPlugin/MongoDBConnection.swift b/Plugins/MongoDBDriverPlugin/MongoDBConnection.swift index 69b7a00f..3aecdb39 100644 --- a/Plugins/MongoDBDriverPlugin/MongoDBConnection.swift +++ b/Plugins/MongoDBDriverPlugin/MongoDBConnection.swift @@ -190,6 +190,8 @@ final class MongoDBConnection: @unchecked Sendable { let effectiveAuthSource: String if let source = authSource, !source.isEmpty { effectiveAuthSource = source + } else if useSrv { + effectiveAuthSource = "admin" } else if !database.isEmpty { effectiveAuthSource = database } else { @@ -206,8 +208,7 @@ final class MongoDBConnection: @unchecked Sendable { let sslEnabled = ["Preferred", "Required", "Verify CA", "Verify Identity"].contains(sslMode) if sslEnabled { params.append("tls=true") - let verifiesCert = sslMode == "Verify CA" || sslMode == "Verify Identity" - if !verifiesCert { + if sslMode == "Preferred" { params.append("tlsAllowInvalidCertificates=true") } if !sslCACertPath.isEmpty { From f7e276e2c9ad88457de96efba00c1ba586c3f931 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ng=C3=B4=20Qu=E1=BB=91c=20=C4=90=E1=BA=A1t?= Date: Tue, 24 Mar 2026 09:06:21 +0700 Subject: [PATCH 2/5] fix: resolve 26 anti-patterns across window lifecycle, memory, connections, and SwiftUI P0 (Critical): - Skip selected tab during memory eviction to prevent visible refresh on window re-focus - Wrap reconnectDriver in trackOperation to prevent health monitor race condition P1 (High): - Cancel redisDatabaseSwitchTask in coordinator teardown - Add deinit to AIChatViewModel to cancel streaming task - Replace 5x fire-and-forget SSH tunnel closes with error logging - Log schema/database switch errors instead of silently discarding - Re-fetch session for driver disconnect to avoid stale reference - Add WindowAccessor NSViewRepresentable for reliable window capture - Use viewWindow instead of NSApp.keyWindow in command actions P2 (Medium): - Fix weak self in LicenseManager/AnalyticsService periodic task loops - Remove imprecise global changeManager check from eviction scheduling - Add query staleness detection so stuck queries don't mask dead connections - Log warning for synchronous plugin loading fallback - Add deinit to AppSettingsManager for observer cleanup - Split clearChanges/clearChangesAndUndoHistory for undo preservation - Make TabDiskActor.save() throw so callers can handle disk errors - Remove fragile subtitle-based window matching fallback - Use viewDidMoveToWindow for synchronous window capture P3 (Low): - Remove stale isKeyWindow snapshot from configureWindow - Centralize window identifier constants with exact matching - Remove unnecessary DispatchQueue.main.async in AppDelegate handlers - Add warning log for WindowLifecycleMonitor re-registration - Unify exponential backoff via shared ExponentialBackoff utility - Return startup command failures from executeStartupCommands - Replace recursive asyncAfter with cancellable Task loop --- TablePro/AppDelegate+WindowConfig.swift | 29 +- .../ChangeTracking/DataChangeManager.swift | 12 +- .../Database/ConnectionHealthMonitor.swift | 11 +- TablePro/Core/Database/DatabaseDriver.swift | 3 + TablePro/Core/Database/DatabaseManager.swift | 91 +++++-- .../Infrastructure/AnalyticsService.swift | 9 +- .../TabPersistenceCoordinator.swift | 12 +- .../WindowLifecycleMonitor.swift | 5 + .../Services/Licensing/LicenseManager.swift | 6 +- .../Core/Storage/AppSettingsManager.swift | 6 + TablePro/Core/Storage/TabDiskActor.swift | 18 +- .../Connection/ExponentialBackoff.swift | 18 ++ TablePro/ViewModels/AIChatViewModel.swift | 6 +- .../Views/Components/WindowAccessor.swift | 28 ++ .../Views/Connection/WelcomeWindowView.swift | 21 +- .../MainContentCoordinator+ChangeGuard.swift | 2 +- .../MainContentCoordinator+Discard.swift | 2 +- ...ainContentCoordinator+MultiStatement.swift | 2 +- .../MainContentCoordinator+QueryHelpers.swift | 2 +- .../MainContentCoordinator+Refresh.swift | 2 +- .../MainContentCoordinator+SaveChanges.swift | 2 +- .../Main/MainContentCommandActions.swift | 2 +- .../Views/Main/MainContentCoordinator.swift | 12 +- TablePro/Views/Main/MainContentView.swift | 81 +++--- docs/development/anti-patterns-tracker.md | 255 ++++++++++++++++++ 25 files changed, 499 insertions(+), 138 deletions(-) create mode 100644 TablePro/Core/Utilities/Connection/ExponentialBackoff.swift create mode 100644 TablePro/Views/Components/WindowAccessor.swift create mode 100644 docs/development/anti-patterns-tracker.md diff --git a/TablePro/AppDelegate+WindowConfig.swift b/TablePro/AppDelegate+WindowConfig.swift index 50f3cb74..e575a6b9 100644 --- a/TablePro/AppDelegate+WindowConfig.swift +++ b/TablePro/AppDelegate+WindowConfig.swift @@ -100,18 +100,22 @@ extension AppDelegate { // MARK: - Window Identification + private enum WindowId { + static let main = "main" + static let welcome = "welcome" + static let connectionForm = "connection-form" + } + func isMainWindow(_ window: NSWindow) -> Bool { - guard let identifier = window.identifier?.rawValue else { return false } - return identifier.contains("main") + window.identifier?.rawValue == WindowId.main } func isWelcomeWindow(_ window: NSWindow) -> Bool { - window.identifier?.rawValue == "welcome" || - window.title.lowercased().contains("welcome") + window.identifier?.rawValue == WindowId.welcome } private func isConnectionFormWindow(_ window: NSWindow) -> Bool { - window.identifier?.rawValue.contains("connection-form") == true + window.identifier?.rawValue == WindowId.connectionForm } // MARK: - Welcome Window @@ -259,10 +263,7 @@ extension AppDelegate { if remainingMainWindows == 0 { NotificationCenter.default.post(name: .mainWindowWillClose, object: nil) - - DispatchQueue.main.async { - self.openWelcomeWindow() - } + openWelcomeWindow() } } } @@ -273,13 +274,9 @@ extension AppDelegate { if isWelcomeWindow(window), window.occlusionState.contains(.visible), - NSApp.windows.contains(where: { isMainWindow($0) && $0.isVisible }) { - DispatchQueue.main.async { [weak self] in - guard let self else { return } - if self.isWelcomeWindow(window), window.isVisible { - window.close() - } - } + NSApp.windows.contains(where: { isMainWindow($0) && $0.isVisible }), + window.isVisible { + window.close() } } diff --git a/TablePro/Core/ChangeTracking/DataChangeManager.swift b/TablePro/Core/ChangeTracking/DataChangeManager.swift index 5a3ec4ac..adb5195b 100644 --- a/TablePro/Core/ChangeTracking/DataChangeManager.swift +++ b/TablePro/Core/ChangeTracking/DataChangeManager.swift @@ -125,7 +125,8 @@ final class DataChangeManager { // MARK: - Configuration - /// Clear all changes (called after successful save) + /// Clear all tracked changes, preserving undo/redo history. + /// Use when changes are invalidated but undo context may still be relevant. func clearChanges() { changes.removeAll() changeIndex.removeAll() @@ -134,11 +135,18 @@ final class DataChangeManager { modifiedCells.removeAll() insertedRowData.removeAll() changedRowIndices.removeAll() - undoManager.clearAll() hasChanges = false reloadVersion += 1 } + /// Clear all tracked changes AND undo/redo history. + /// Use after successful save, explicit discard, or new query execution + /// where undo context is no longer meaningful. + func clearChangesAndUndoHistory() { + clearChanges() + undoManager.clearAll() + } + /// Atomically configure the manager for a new table func configureForTable( tableName: String, diff --git a/TablePro/Core/Database/ConnectionHealthMonitor.swift b/TablePro/Core/Database/ConnectionHealthMonitor.swift index e33d81a3..105eb721 100644 --- a/TablePro/Core/Database/ConnectionHealthMonitor.swift +++ b/TablePro/Core/Database/ConnectionHealthMonitor.swift @@ -35,7 +35,6 @@ actor ConnectionHealthMonitor { // MARK: - Configuration private static let pingInterval: TimeInterval = 30.0 - private static let initialBackoffDelays: [TimeInterval] = [2.0, 4.0, 8.0] private static let maxBackoffDelay: TimeInterval = 120.0 // MARK: - Dependencies @@ -227,15 +226,7 @@ actor ConnectionHealthMonitor { /// Uses the initial delay table for the first few attempts, then doubles /// the previous delay for subsequent attempts, capped at `maxBackoffDelay`. private func backoffDelay(for attempt: Int) -> TimeInterval { - let delays = Self.initialBackoffDelays - if attempt <= delays.count { - return delays[attempt - 1] - } - // Exponential: last seed delay * 2^(attempt - seedCount) - let lastSeed = delays[delays.count - 1] - let exponent = attempt - delays.count - let computed = lastSeed * pow(2.0, Double(exponent)) - return min(computed, Self.maxBackoffDelay) + ExponentialBackoff.delay(for: attempt, maxDelay: Self.maxBackoffDelay) } // MARK: - State Transitions diff --git a/TablePro/Core/Database/DatabaseDriver.swift b/TablePro/Core/Database/DatabaseDriver.swift index 980781ec..dddcb78d 100644 --- a/TablePro/Core/Database/DatabaseDriver.swift +++ b/TablePro/Core/Database/DatabaseDriver.swift @@ -315,12 +315,15 @@ extension DatabaseDriver { /// Factory for creating database drivers via plugin lookup @MainActor enum DatabaseDriverFactory { + private static let logger = Logger(subsystem: "com.TablePro", category: "DatabaseDriverFactory") + static func createDriver(for connection: DatabaseConnection) throws -> DatabaseDriver { let pluginId = connection.type.pluginTypeId // If the plugin isn't registered yet and background loading hasn't finished, // fall back to synchronous loading for this critical code path. if PluginManager.shared.driverPlugins[pluginId] == nil, !PluginManager.shared.hasFinishedInitialLoad { + logger.warning("Plugin '\(pluginId)' not loaded yet — performing synchronous load") PluginManager.shared.loadPendingPlugins() } guard let plugin = PluginManager.shared.driverPlugins[pluginId] else { diff --git a/TablePro/Core/Database/DatabaseManager.swift b/TablePro/Core/Database/DatabaseManager.swift index 1c9c0b90..27ad5c9f 100644 --- a/TablePro/Core/Database/DatabaseManager.swift +++ b/TablePro/Core/Database/DatabaseManager.swift @@ -49,6 +49,8 @@ final class DatabaseManager { /// The health monitor skips pings while a query is running to avoid /// racing on non-thread-safe driver connections. @ObservationIgnored private var queriesInFlight: [UUID: Int] = [:] + /// Tracks when the first query started for each session (used for staleness detection). + @ObservationIgnored private var queryStartTimes: [UUID: Date] = [:] /// Current session (computed from currentSessionId) var currentSession: ConnectionSession? { @@ -130,7 +132,11 @@ final class DatabaseManager { // Close tunnel if SSH was established if connection.sshConfig.enabled { Task { - try? await SSHTunnelManager.shared.closeTunnel(connectionId: connection.id) + do { + try await SSHTunnelManager.shared.closeTunnel(connectionId: connection.id) + } catch { + Self.logger.warning("SSH tunnel cleanup failed for \(connection.name): \(error.localizedDescription)") + } } } removeSessionEntry(for: connection.id) @@ -220,7 +226,11 @@ final class DatabaseManager { // Close tunnel if connection failed if connection.sshConfig.enabled { Task { - try? await SSHTunnelManager.shared.closeTunnel(connectionId: connection.id) + do { + try await SSHTunnelManager.shared.closeTunnel(connectionId: connection.id) + } catch { + Self.logger.warning("SSH tunnel cleanup failed for \(connection.name): \(error.localizedDescription)") + } } } @@ -256,7 +266,11 @@ final class DatabaseManager { // Close SSH tunnel if exists if session.connection.sshConfig.enabled { - try? await SSHTunnelManager.shared.closeTunnel(connectionId: session.connection.id) + do { + try await SSHTunnelManager.shared.closeTunnel(connectionId: session.connection.id) + } catch { + Self.logger.warning("SSH tunnel cleanup failed for \(session.connection.name): \(error.localizedDescription)") + } } // Stop health monitoring @@ -343,11 +357,15 @@ final class DatabaseManager { operation: () async throws -> T ) async throws -> T { queriesInFlight[sessionId, default: 0] += 1 + if queriesInFlight[sessionId] == 1 { + queryStartTimes[sessionId] = Date() + } defer { if let count = queriesInFlight[sessionId], count > 1 { queriesInFlight[sessionId] = count - 1 } else { queriesInFlight.removeValue(forKey: sessionId) + queryStartTimes.removeValue(forKey: sessionId) } } return try await operation() @@ -402,13 +420,21 @@ final class DatabaseManager { result = try await driver.testConnection() } catch { if connection.sshConfig.enabled { - try? await SSHTunnelManager.shared.closeTunnel(connectionId: connection.id) + do { + try await SSHTunnelManager.shared.closeTunnel(connectionId: connection.id) + } catch { + Self.logger.warning("SSH tunnel cleanup failed for \(connection.name): \(error.localizedDescription)") + } } throw error } if connection.sshConfig.enabled { - try? await SSHTunnelManager.shared.closeTunnel(connectionId: connection.id) + do { + try await SSHTunnelManager.shared.closeTunnel(connectionId: connection.id) + } catch { + Self.logger.warning("SSH tunnel cleanup failed for \(connection.name): \(error.localizedDescription)") + } } return result @@ -532,7 +558,16 @@ final class DatabaseManager { guard let self else { return false } // Skip ping while a user query is in-flight to avoid racing // on the same non-thread-safe driver connection. - guard await self.queriesInFlight[connectionId] == nil else { return true } + // Allow ping if the query appears stuck (exceeds timeout + grace period). + if await self.queriesInFlight[connectionId] != nil { + let queryTimeout = await TimeInterval(AppSettingsManager.shared.general.queryTimeoutSeconds) + let maxStale = max(queryTimeout, 300) // At least 5 minutes + if let startTime = await self.queryStartTimes[connectionId], + Date().timeIntervalSince(startTime) < maxStale { + return true // Query still within expected time + } + // Query appears stuck — fall through to ping + } guard let mainDriver = await self.activeSessions[connectionId]?.driver else { return false } @@ -547,12 +582,13 @@ final class DatabaseManager { guard let self else { return false } guard let session = await self.activeSessions[connectionId] else { return false } do { - let driver = try await self.reconnectDriver(for: session) + let driver = try await self.trackOperation(sessionId: connectionId) { + try await self.reconnectDriver(for: session) + } await self.updateSession(connectionId) { session in session.driver = driver session.status = .connected } - return true } catch { return false @@ -619,13 +655,21 @@ final class DatabaseManager { if let savedSchema = session.currentSchema, let schemaDriver = driver as? SchemaSwitchable { - try? await schemaDriver.switchSchema(to: savedSchema) + do { + try await schemaDriver.switchSchema(to: savedSchema) + } catch { + Self.logger.warning("Failed to restore schema '\(savedSchema)' on reconnect: \(error.localizedDescription)") + } } // Restore database for MSSQL if session had a non-default database if let savedDatabase = session.currentDatabase, let adapter = driver as? PluginDriverAdapter { - try? await adapter.switchDatabase(to: savedDatabase) + do { + try await adapter.switchDatabase(to: savedDatabase) + } catch { + Self.logger.warning("Failed to restore database '\(savedDatabase)' on reconnect: \(error.localizedDescription)") + } } return driver @@ -659,8 +703,8 @@ final class DatabaseManager { await stopHealthMonitor(for: sessionId) do { - // Disconnect existing drivers - session.driver?.disconnect() + // Disconnect existing driver (re-fetch to avoid stale local reference) + activeSessions[sessionId]?.driver?.disconnect() // Recreate SSH tunnel if needed and build effective connection let effectiveConnection = try await buildEffectiveConnection(for: session.connection) @@ -681,13 +725,21 @@ final class DatabaseManager { if let savedSchema = activeSessions[sessionId]?.currentSchema, let schemaDriver = driver as? SchemaSwitchable { - try? await schemaDriver.switchSchema(to: savedSchema) + do { + try await schemaDriver.switchSchema(to: savedSchema) + } catch { + Self.logger.warning("Failed to restore schema '\(savedSchema)' on reconnect: \(error.localizedDescription)") + } } // Restore database for MSSQL if session had a non-default database if let savedDatabase = activeSessions[sessionId]?.currentDatabase, let adapter = driver as? PluginDriverAdapter { - try? await adapter.switchDatabase(to: savedDatabase) + do { + try await adapter.switchDatabase(to: savedDatabase) + } catch { + Self.logger.warning("Failed to restore database '\(savedDatabase)' on reconnect: \(error.localizedDescription)") + } } // Update session @@ -741,8 +793,7 @@ final class DatabaseManager { let maxRetries = 5 for retryCount in 0.. [(statement: String, error: String)] { guard let commands, !commands.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else { - return + return [] } let statements = commands @@ -780,6 +832,7 @@ final class DatabaseManager { .map { $0.trimmingCharacters(in: .whitespacesAndNewlines) } .filter { !$0.isEmpty } + var failures: [(statement: String, error: String)] = [] for statement in statements { do { _ = try await driver.execute(query: statement) @@ -790,8 +843,10 @@ final class DatabaseManager { Self.startupLogger.warning( "Startup command failed for '\(connectionName)': \(statement) — \(error.localizedDescription)" ) + failures.append((statement: statement, error: error.localizedDescription)) } } + return failures } // MARK: - Schema Changes diff --git a/TablePro/Core/Services/Infrastructure/AnalyticsService.swift b/TablePro/Core/Services/Infrastructure/AnalyticsService.swift index 811e3262..ce4b6e8e 100644 --- a/TablePro/Core/Services/Infrastructure/AnalyticsService.swift +++ b/TablePro/Core/Services/Infrastructure/AnalyticsService.swift @@ -64,12 +64,13 @@ final class AnalyticsService { heartbeatTask?.cancel() heartbeatTask = Task { [weak self] in // Initial delay before first heartbeat (let connections establish) - try? await Task.sleep(for: .seconds(self?.initialDelay ?? 10)) + guard let delay = self?.initialDelay else { return } + try? await Task.sleep(for: .seconds(delay)) while !Task.isCancelled { - await self?.sendHeartbeat() - try? await Task.sleep(for: .seconds(self?.heartbeatInterval ?? 86_400)) - guard self != nil else { return } + guard let target = self else { return } + await target.sendHeartbeat() + try? await Task.sleep(for: .seconds(target.heartbeatInterval)) } } } diff --git a/TablePro/Core/Services/Infrastructure/TabPersistenceCoordinator.swift b/TablePro/Core/Services/Infrastructure/TabPersistenceCoordinator.swift index ec09b953..824a474d 100644 --- a/TablePro/Core/Services/Infrastructure/TabPersistenceCoordinator.swift +++ b/TablePro/Core/Services/Infrastructure/TabPersistenceCoordinator.swift @@ -48,7 +48,11 @@ internal final class TabPersistenceCoordinator { ? selectedTabId : nonPreviewTabs.first?.id Task { - await TabDiskActor.shared.save(connectionId: connId, tabs: persisted, selectedTabId: normalizedSelectedId) + do { + try await TabDiskActor.shared.save(connectionId: connId, tabs: persisted, selectedTabId: normalizedSelectedId) + } catch { + TabDiskActor.logSaveError(connectionId: connId, error: error) + } } } @@ -59,7 +63,11 @@ internal final class TabPersistenceCoordinator { let selectedId = selectedTabId Task { - await TabDiskActor.shared.save(connectionId: connId, tabs: persistedTabs, selectedTabId: selectedId) + do { + try await TabDiskActor.shared.save(connectionId: connId, tabs: persistedTabs, selectedTabId: selectedId) + } catch { + TabDiskActor.logSaveError(connectionId: connId, error: error) + } } } diff --git a/TablePro/Core/Services/Infrastructure/WindowLifecycleMonitor.swift b/TablePro/Core/Services/Infrastructure/WindowLifecycleMonitor.swift index a29f6ce9..cbd6e6c5 100644 --- a/TablePro/Core/Services/Infrastructure/WindowLifecycleMonitor.swift +++ b/TablePro/Core/Services/Infrastructure/WindowLifecycleMonitor.swift @@ -9,9 +9,11 @@ import AppKit import Foundation +import OSLog @MainActor internal final class WindowLifecycleMonitor { + private static let logger = Logger(subsystem: "com.TablePro", category: "WindowLifecycleMonitor") internal static let shared = WindowLifecycleMonitor() private struct Entry { @@ -41,6 +43,9 @@ internal final class WindowLifecycleMonitor { internal func register(window: NSWindow, connectionId: UUID, windowId: UUID, isPreview: Bool = false) { // Remove any existing entry for this windowId to avoid duplicate observers if let existing = entries[windowId] { + if existing.window !== window { + Self.logger.warning("Re-registering windowId \(windowId) with a different NSWindow") + } if let observer = existing.observer { NotificationCenter.default.removeObserver(observer) } diff --git a/TablePro/Core/Services/Licensing/LicenseManager.swift b/TablePro/Core/Services/Licensing/LicenseManager.swift index 85019d66..23824957 100644 --- a/TablePro/Core/Services/Licensing/LicenseManager.swift +++ b/TablePro/Core/Services/Licensing/LicenseManager.swift @@ -93,9 +93,9 @@ final class LicenseManager { } while !Task.isCancelled { - try? await Task.sleep(for: .seconds(self?.revalidationInterval ?? 604_800)) - guard self != nil else { return } - await self?.revalidate() + guard let self else { return } + try? await Task.sleep(for: .seconds(self.revalidationInterval)) + await self.revalidate() } } } diff --git a/TablePro/Core/Storage/AppSettingsManager.swift b/TablePro/Core/Storage/AppSettingsManager.swift index 647fad94..577e2555 100644 --- a/TablePro/Core/Storage/AppSettingsManager.swift +++ b/TablePro/Core/Storage/AppSettingsManager.swift @@ -17,6 +17,12 @@ import os final class AppSettingsManager { static let shared = AppSettingsManager() + deinit { + if let observer = accessibilityTextSizeObserver { + NSWorkspace.shared.notificationCenter.removeObserver(observer) + } + } + // MARK: - Published Settings var general: GeneralSettings { diff --git a/TablePro/Core/Storage/TabDiskActor.swift b/TablePro/Core/Storage/TabDiskActor.swift index bf0b7610..85bdd589 100644 --- a/TablePro/Core/Storage/TabDiskActor.swift +++ b/TablePro/Core/Storage/TabDiskActor.swift @@ -68,17 +68,17 @@ internal actor TabDiskActor { // MARK: - Public API - /// Save tab state for a connection. Errors are logged internally. - internal func save(connectionId: UUID, tabs: [PersistedTab], selectedTabId: UUID?) { + /// Save tab state for a connection. Throws on encoding or disk write failure. + internal func save(connectionId: UUID, tabs: [PersistedTab], selectedTabId: UUID?) throws { let state = TabDiskState(tabs: tabs, selectedTabId: selectedTabId) + let data = try encoder.encode(state) + let fileURL = tabStateFileURL(for: connectionId) + try data.write(to: fileURL, options: .atomic) + } - do { - let data = try encoder.encode(state) - let fileURL = tabStateFileURL(for: connectionId) - try data.write(to: fileURL, options: .atomic) - } catch { - Self.logger.error("Failed to save tab state for \(connectionId): \(error.localizedDescription)") - } + /// Log a save error from callers that handle errors externally. + nonisolated static func logSaveError(connectionId: UUID, error: Error) { + logger.error("Failed to save tab state for \(connectionId): \(error.localizedDescription)") } /// Load tab state for a connection. Returns nil if the file is missing or corrupt. diff --git a/TablePro/Core/Utilities/Connection/ExponentialBackoff.swift b/TablePro/Core/Utilities/Connection/ExponentialBackoff.swift new file mode 100644 index 00000000..1cfa28cb --- /dev/null +++ b/TablePro/Core/Utilities/Connection/ExponentialBackoff.swift @@ -0,0 +1,18 @@ +import Foundation + +/// Shared exponential backoff calculator for reconnection delays. +enum ExponentialBackoff { + private static let seeds: [TimeInterval] = [2, 4, 8] + + /// Calculate delay for a given attempt (1-based). + /// Sequence: 2s, 4s, 8s, then doubles from last seed, capped at maxDelay. + static func delay(for attempt: Int, maxDelay: TimeInterval = 120) -> TimeInterval { + guard attempt > 0 else { return seeds[0] } + if attempt <= seeds.count { + return seeds[attempt - 1] + } + let lastSeed = seeds[seeds.count - 1] + let exponent = attempt - seeds.count + return min(lastSeed * pow(2.0, Double(exponent)), maxDelay) + } +} diff --git a/TablePro/ViewModels/AIChatViewModel.swift b/TablePro/ViewModels/AIChatViewModel.swift index feabecee..26d580a7 100644 --- a/TablePro/ViewModels/AIChatViewModel.swift +++ b/TablePro/ViewModels/AIChatViewModel.swift @@ -88,7 +88,7 @@ final class AIChatViewModel { // MARK: - Private - private var streamingTask: Task? + @ObservationIgnored nonisolated(unsafe) private var streamingTask: Task? private var streamingAssistantID: UUID? private var lastUsedFeature: AIFeature = .chat private let chatStorage = AIChatStorage.shared @@ -101,6 +101,10 @@ final class AIChatViewModel { loadConversations() } + deinit { + streamingTask?.cancel() + } + // MARK: - Actions /// Send the current input text as a user message diff --git a/TablePro/Views/Components/WindowAccessor.swift b/TablePro/Views/Components/WindowAccessor.swift new file mode 100644 index 00000000..6cd8c8e8 --- /dev/null +++ b/TablePro/Views/Components/WindowAccessor.swift @@ -0,0 +1,28 @@ +import SwiftUI + +/// Captures the hosting NSWindow from within a SwiftUI view hierarchy. +/// Use as a `.background { WindowAccessor { window in ... } }` modifier. +/// Uses `viewDidMoveToWindow` for synchronous capture — no async deferral, +/// so the window is available before any notifications fire. +struct WindowAccessor: NSViewRepresentable { + var onWindow: (NSWindow) -> Void + + func makeNSView(context: Context) -> WindowAccessorView { + let view = WindowAccessorView() + view.onWindow = onWindow + return view + } + + func updateNSView(_ nsView: WindowAccessorView, context: Context) {} +} + +final class WindowAccessorView: NSView { + var onWindow: ((NSWindow) -> Void)? + + override func viewDidMoveToWindow() { + super.viewDidMoveToWindow() + if let window { + onWindow?(window) + } + } +} diff --git a/TablePro/Views/Connection/WelcomeWindowView.swift b/TablePro/Views/Connection/WelcomeWindowView.swift index 6e14af26..2c58e52d 100644 --- a/TablePro/Views/Connection/WelcomeWindowView.swift +++ b/TablePro/Views/Connection/WelcomeWindowView.swift @@ -755,26 +755,15 @@ struct WelcomeWindowView: View { /// Focus the connection form window as soon as it's available private func focusConnectionFormWindow() { - // Poll rapidly until window is found (much faster than fixed delay) - func attemptFocus(remainingAttempts: Int = 10) { - for window in NSApp.windows { - if window.identifier?.rawValue.contains("connection-form") == true - || window.title == "Connection" - { + Task { @MainActor in + for _ in 0..<10 { + for window in NSApp.windows where + window.identifier?.rawValue == "connection-form" { window.makeKeyAndOrderFront(nil) return } + try? await Task.sleep(for: .milliseconds(20)) } - // Window not found yet, try again in 20ms - if remainingAttempts > 0 { - DispatchQueue.main.asyncAfter(deadline: .now() + 0.02) { - attemptFocus(remainingAttempts: remainingAttempts - 1) - } - } - } - // Start immediately on next run loop - DispatchQueue.main.async { - attemptFocus() } } } diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+ChangeGuard.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+ChangeGuard.swift index eb3e5025..c1394aa0 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+ChangeGuard.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+ChangeGuard.swift @@ -30,7 +30,7 @@ extension MainContentCoordinator { let window = NSApp.keyWindow let confirmed = await confirmDiscardChanges(action: action, window: window) if confirmed { - changeManager.clearChanges() + changeManager.clearChangesAndUndoHistory() } completion(confirmed) } diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+Discard.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+Discard.swift index b2dc2c64..f0a22778 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+Discard.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+Discard.swift @@ -74,7 +74,7 @@ extension MainContentCoordinator { pendingTruncates.removeAll() pendingDeletes.removeAll() - changeManager.clearChanges() + changeManager.clearChangesAndUndoHistory() if let index = tabManager.selectedTabIndex { tabManager.tabs[index].pendingChanges = TabPendingChanges() diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+MultiStatement.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+MultiStatement.swift index 7ea549ef..fc77a090 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+MultiStatement.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+MultiStatement.swift @@ -134,7 +134,7 @@ extension MainContentCoordinator { updatedTab.errorMessage = nil tabManager.tabs[idx] = updatedTab - changeManager.clearChanges() + changeManager.clearChangesAndUndoHistory() changeManager.reloadVersion += 1 } } catch { diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+QueryHelpers.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+QueryHelpers.swift index b2d4962b..b31c7787 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+QueryHelpers.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+QueryHelpers.swift @@ -193,7 +193,7 @@ extension MainContentCoordinator { // doesn't linger while Phase 2 metadata loads in background. // Only clear if there are no pending edits from the user. if isEditable && !changeManager.hasChanges { - changeManager.clearChanges() + changeManager.clearChangesAndUndoHistory() } } diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+Refresh.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+Refresh.swift index 461208a1..7f4ba906 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+Refresh.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+Refresh.swift @@ -29,7 +29,7 @@ extension MainContentCoordinator { let confirmed = await confirmDiscardChanges(action: .refresh, window: window) if confirmed { onDiscard() - changeManager.clearChanges() + changeManager.clearChangesAndUndoHistory() // Only execute query if we're in a table tab // Query tabs should not auto-execute on refresh (use Cmd+Enter to execute) if let tabIndex = tabManager.selectedTabIndex, diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+SaveChanges.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+SaveChanges.swift index d194aa90..a610badb 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+SaveChanges.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+SaveChanges.swift @@ -228,7 +228,7 @@ extension MainContentCoordinator { throw error } - changeManager.clearChanges() + changeManager.clearChangesAndUndoHistory() if let index = tabManager.selectedTabIndex { tabManager.tabs[index].pendingChanges = TabPendingChanges() tabManager.tabs[index].errorMessage = nil diff --git a/TablePro/Views/Main/MainContentCommandActions.swift b/TablePro/Views/Main/MainContentCommandActions.swift index 9c2c269b..5a1533e0 100644 --- a/TablePro/Views/Main/MainContentCommandActions.swift +++ b/TablePro/Views/Main/MainContentCommandActions.swift @@ -375,7 +375,7 @@ final class MainContentCommandActions { } private func discardAndClose() { - coordinator?.changeManager.clearChanges() + coordinator?.changeManager.clearChangesAndUndoHistory() pendingTruncates.wrappedValue.removeAll() pendingDeletes.wrappedValue.removeAll() rightPanelState.editState.clearEdits() diff --git a/TablePro/Views/Main/MainContentCoordinator.swift b/TablePro/Views/Main/MainContentCoordinator.swift index bb42b5b4..3698cd4d 100644 --- a/TablePro/Views/Main/MainContentCoordinator.swift +++ b/TablePro/Views/Main/MainContentCoordinator.swift @@ -204,13 +204,17 @@ final class MainContentCoordinator { } }() - /// Evict row data for all tabs in this coordinator to free memory. + /// Evict row data for background tabs in this coordinator to free memory. /// Called when the coordinator's native window-tab becomes inactive. - /// Data is re-fetched automatically when the tab becomes active again. + /// The currently selected tab is kept in memory so the user sees no + /// refresh flicker when switching back — matching native macOS behavior. + /// Background tabs are re-fetched automatically when selected. func evictInactiveRowData() { + let selectedId = tabManager.selectedTabId for tab in tabManager.tabs where !tab.rowBuffer.isEvicted && !tab.resultRows.isEmpty && !tab.pendingChanges.hasChanges + && tab.id != selectedId { tab.rowBuffer.evict() } @@ -351,6 +355,8 @@ final class MainContentCoordinator { currentQueryTask = nil changeManagerUpdateTask?.cancel() changeManagerUpdateTask = nil + redisDatabaseSwitchTask?.cancel() + redisDatabaseSwitchTask = nil for task in activeSortTasks.values { task.cancel() } activeSortTasks.removeAll() @@ -933,7 +939,7 @@ final class MainContentCoordinator { guard let self else { return } guard capturedGeneration == queryGeneration else { return } guard !Task.isCancelled else { return } - changeManager.clearChanges() + changeManager.clearChangesAndUndoHistory() } } } catch { diff --git a/TablePro/Views/Main/MainContentView.swift b/TablePro/Views/Main/MainContentView.swift index 730a18b0..b9742f11 100644 --- a/TablePro/Views/Main/MainContentView.swift +++ b/TablePro/Views/Main/MainContentView.swift @@ -192,6 +192,11 @@ struct MainContentView: View { /// Split into two halves to help the Swift type checker with the long modifier chain. private var bodyContent: some View { bodyContentCore + .background { + WindowAccessor { window in + configureWindow(window) + } + } .task(id: currentTab?.tableName) { // Only load metadata after the tab has executed at least once — // avoids a redundant DB query racing with the initial data query @@ -215,33 +220,7 @@ struct MainContentView: View { coordinator.aiViewModel = rightPanelState.aiViewModel coordinator.rightPanelState = rightPanelState - // Register NSWindow reference and set per-connection tab grouping - DispatchQueue.main.async { - // Find our window by title rather than keyWindow to avoid races - // when multiple windows open simultaneously - let targetTitle = windowTitle - let window = NSApp.keyWindow - ?? NSApp.windows.first { $0.isVisible && $0.title == targetTitle } - guard let window else { return } - let isPreview = tabManager.selectedTab?.isPreview ?? payload?.isPreview ?? false - if isPreview { - window.subtitle = "\(connection.name) — Preview" - } else { - window.subtitle = connection.name - } - window.tabbingIdentifier = "com.TablePro.main.\(connection.id.uuidString)" - window.tabbingMode = .preferred - coordinator.windowId = windowId - - WindowLifecycleMonitor.shared.register( - window: window, - connectionId: connection.id, - windowId: windowId, - isPreview: isPreview - ) - viewWindow = window - isKeyWindow = window.isKeyWindow - } + // Window registration is handled by WindowAccessor in .background } .onDisappear { // Mark teardown intent synchronously so deinit doesn't warn @@ -273,14 +252,7 @@ struct MainContentView: View { // Tab state is NOT cleared here — it's preserved for next reconnect. // Only handleTabsChange(count=0) clears state (user explicitly closed all tabs). guard !WindowLifecycleMonitor.shared.hasWindows(for: connectionId) else { return } - - let hasVisibleWindow = NSApp.windows.contains { window in - window.isVisible && (window.subtitle == connectionName - || window.subtitle == "\(connectionName) — Preview") - } - if !hasVisibleWindow { - await DatabaseManager.shared.disconnectSession(connectionId) - } + await DatabaseManager.shared.disconnectSession(connectionId) } } .onChange(of: pendingChangeTrigger) { @@ -353,13 +325,12 @@ struct MainContentView: View { // Schedule row data eviction for inactive native window-tabs. // 5s delay avoids thrashing when quickly switching between tabs. - // Skip eviction entirely if the active tab has unsaved in-memory changes, - // since evictInactiveRowData only checks tab-level pendingChanges. + // Per-tab pendingChanges checks inside evictInactiveRowData() protect + // tabs with unsaved changes from eviction. evictionTask?.cancel() evictionTask = Task { @MainActor in try? await Task.sleep(for: .seconds(5)) guard !Task.isCancelled else { return } - guard !changeManager.hasChanges else { return } coordinator.evictInactiveRowData() } } @@ -605,6 +576,30 @@ struct MainContentView: View { || AppState.shared.hasStructureChanges } + /// Configure the hosting NSWindow — called by WindowAccessor when the window is available. + private func configureWindow(_ window: NSWindow) { + let isPreview = tabManager.selectedTab?.isPreview ?? payload?.isPreview ?? false + if isPreview { + window.subtitle = "\(connection.name) — Preview" + } else { + window.subtitle = connection.name + } + window.tabbingIdentifier = "com.TablePro.main.\(connection.id.uuidString)" + window.tabbingMode = .preferred + coordinator.windowId = windowId + + WindowLifecycleMonitor.shared.register( + window: window, + connectionId: connection.id, + windowId: windowId, + isPreview: isPreview + ) + viewWindow = window + + // Update command actions window reference now that it's available + commandActions?.window = window + } + private func setupCommandActions() { let actions = MainContentCommandActions( coordinator: coordinator, @@ -621,16 +616,8 @@ struct MainContentView: View { rightPanelState: rightPanelState, editingCell: $editingCell ) - actions.window = NSApp.keyWindow + actions.window = viewWindow commandActions = actions - - // Safety fallback: if window wasn't key yet at onAppear time, - // retry on next run loop when the window is guaranteed to be visible - if actions.window == nil { - DispatchQueue.main.async { [weak actions] in - actions?.window = NSApp.keyWindow - } - } } // MARK: - Database Switcher diff --git a/docs/development/anti-patterns-tracker.md b/docs/development/anti-patterns-tracker.md new file mode 100644 index 00000000..f635f189 --- /dev/null +++ b/docs/development/anti-patterns-tracker.md @@ -0,0 +1,255 @@ +# Anti-Patterns & Incorrect Behavior Tracker + +Status legend: `[ ]` = TODO, `[x]` = Fixed, `[-]` = Won't fix + +--- + +## 1. Window Lifecycle & Focus Management + +### 1.1 [x] Active tab evicted on window focus loss (CRITICAL) +- **File**: `TablePro/Views/Main/MainContentView.swift:349-365` +- **Problem**: When the window loses focus, `evictInactiveRowData()` evicts ALL tabs including the currently visible one. When the user switches back, the visible grid re-fetches data, causing a visible refresh flicker. +- **Correct behavior**: Native macOS apps (Xcode, Safari, TablePlus) keep the active tab's content in memory. Only background/non-visible tabs should be eviction candidates. +- **Fix**: `evictInactiveRowData()` must skip the currently selected tab. + +### 1.2 [x] Unreliable window discovery via NSApp.keyWindow (HIGH) +- **File**: `TablePro/Views/Main/MainContentView.swift:219-244` +- **Problem**: Window registration uses `DispatchQueue.main.async` then queries `NSApp.keyWindow`, which can return the wrong window in multi-window scenarios. Fallback uses fragile title-based matching. +- **Fix**: Capture the window through SwiftUI's view hierarchy (NSViewRepresentable) instead of querying global state. + +### 1.3 [x] Duplicate window discovery pattern in command actions (HIGH) +- **File**: `TablePro/Views/Main/MainContentView.swift:624-633` +- **Problem**: `actions.window = NSApp.keyWindow` with async retry if nil. Same unreliable pattern as 1.2. +- **Fix**: Share the window reference captured in 1.2's fix. + +### 1.4 [x] Fragile subtitle-based window matching (MEDIUM) +- **File**: `TablePro/Views/Main/MainContentView.swift:276-283` +- **Problem**: `window.subtitle == connectionName` is fragile -- subtitles can be empty, duplicated, or changed. Inconsistent with `WindowLifecycleMonitor.hasWindows()` used elsewhere. +- **Fix**: Always use `WindowLifecycleMonitor.hasWindows(for: connectionId)` as single source of truth. + +### 1.5 [x] Notification observer fires before window registration (MEDIUM) +- **File**: `TablePro/Views/Main/MainContentView.swift:324-348` +- **Problem**: `viewWindow` is set asynchronously in `onAppear`, so `didBecomeKeyNotification` observers that compare `notificationWindow === viewWindow` silently miss early notifications when `viewWindow` is still nil. +- **Fix**: Ensure window registration completes before subscribing to notifications, or queue missed notifications. + +### 1.6 [x] Stale isKeyWindow initialization (MEDIUM) +- **File**: `TablePro/Views/Main/MainContentView.swift:243` +- **Problem**: `isKeyWindow = window.isKeyWindow` is a single-point-in-time snapshot that's immediately stale. The notification handlers already track this separately, creating dual sources of truth. +- **Fix**: Remove the synchronous initialization; let `didBecomeKeyNotification`/`didResignKeyNotification` be the sole source. + +### 1.7 [x] Inconsistent window identifier matching (LOW) +- **File**: `TablePro/AppDelegate+WindowConfig.swift:103-115` +- **Problem**: `isMainWindow` uses `.contains("main")`, `isWelcomeWindow` uses `== "welcome"` OR title fallback, `isConnectionFormWindow` uses `.contains("connection-form")`. Substring matching can match unintended windows. +- **Fix**: Use exact matching with a centralized `WindowIdentifier` enum/constants. + +### 1.8 [-] ObjectIdentifier-based window tracking (LOW) +- **File**: `TablePro/AppDelegate+WindowConfig.swift:210-248` +- **Problem**: `configuredWindows` uses `ObjectIdentifier(window)` which is memory-address-based. If an NSWindow is deallocated and a new one allocated at the same address, they get the same identifier. +- **Fix**: Use UUID-based window identifiers. + +### 1.9 [x] Unnecessary DispatchQueue.main.async in notification handlers (LOW) +- **File**: `TablePro/AppDelegate+WindowConfig.swift:263-282` +- **Problem**: Window operations dispatched async from notification handlers that already run on main thread. Introduces race windows where window state changes between check and action. +- **Fix**: Keep critical window operations synchronous when already on main thread. + +### 1.10 [-] Duplicate observers for didBecomeKeyNotification (LOW) +- **Files**: `TablePro/AppDelegate+WindowConfig.swift:210+`, `TablePro/Views/Main/MainContentView.swift:324+` +- **Problem**: Both AppDelegate and MainContentView observe `NSWindow.didBecomeKeyNotification` independently. Both fire for every window focus change. +- **Fix**: Centralize in `WindowLifecycleMonitor` and have views subscribe to a custom signal. + +--- + +## 2. Memory Management & Task Lifecycle + +### 2.1 [x] Missing redisDatabaseSwitchTask cancellation in teardown (HIGH) +- **File**: `TablePro/Views/Main/MainContentCoordinator.swift:106-108, 350-355` +- **Problem**: `currentQueryTask` and `changeManagerUpdateTask` are cancelled in `teardown()`, but `redisDatabaseSwitchTask` is not. This leaves a dangling Task if coordinator is deallocated during a Redis database switch. +- **Fix**: Add `redisDatabaseSwitchTask?.cancel(); redisDatabaseSwitchTask = nil` to `teardown()`. + +### 2.2 [x] Missing deinit in AIChatViewModel (HIGH) +- **File**: `TablePro/ViewModels/AIChatViewModel.swift:91-92` +- **Problem**: `streamingTask` is never cancelled if the view model is deallocated while streaming. The Task continues executing in the background. +- **Fix**: Add `deinit { streamingTask?.cancel() }`. + +### 2.3 [x] Weak self with optional chaining in periodic Task loops (MEDIUM) +- **Files**: + - `TablePro/Core/Services/Licensing/LicenseManager.swift:86-100` + - `TablePro/Core/Services/Infrastructure/AnalyticsService.swift:63-75` +- **Problem**: Inside `while !Task.isCancelled` loops, `self?.revalidationInterval ?? 604_800` silently uses the fallback value if self is deallocated, then continues the loop with a default interval forever. +- **Fix**: Use `guard let self else { return }` at the top of the task, or re-check inside the loop with early exit. + +### 2.4 [x] Eviction pending-changes check is incomplete (MEDIUM) +- **File**: `TablePro/Views/Main/MainContentView.swift:349-365` +- **Problem**: The pre-eviction check uses `changeManager.hasChanges` (in-memory changes), but `evictInactiveRowData()` checks `tab.pendingChanges.hasChanges` (tab-level). These are different — a tab may have tab-level changes even if `changeManager` is empty. +- **Fix**: Check both `changeManager.hasChanges` and `tab.pendingChanges.hasChanges` consistently, or defer the check to just before eviction runs. + +### 2.5 [x] WindowLifecycleMonitor observer re-registration without full cleanup (LOW) +- **File**: `TablePro/Core/Services/Infrastructure/WindowLifecycleMonitor.swift:41-66` +- **Problem**: If `register()` is called twice with the same `windowId` but a different `NSWindow`, the old observer is removed but the old NSWindow is now orphaned without observation. +- **Fix**: Guard against re-registration with a different window or explicitly unregister the old window. + +### 2.6 [-] Fire-and-forget disconnect Task in WindowLifecycleMonitor (LOW) +- **File**: `TablePro/Core/Services/Infrastructure/WindowLifecycleMonitor.swift:190-211` +- **Problem**: `Task { await DatabaseManager.shared.disconnectSession(...) }` has no error handling. If disconnect fails, the connection persists as a zombie. +- **Fix**: Log errors from the disconnect call. + +--- + +## 3. Connection & Query Execution + +### 3.1 [x] Missing trackOperation in reconnectDriver (CRITICAL) +- **File**: `TablePro/Core/Database/DatabaseManager.swift:599-632` +- **Problem**: `reconnectDriver()` does NOT call `trackOperation()`, so `queriesInFlight` is never incremented. The health monitor's ping (`SELECT 1`) can race with the reconnect on the same driver, causing concurrent driver access. +- **Fix**: Wrap reconnection in `trackOperation()` or stop the health monitor before reconnecting. + +### 3.2 [x] Stale session reference in reconnectSession (HIGH) +- **File**: `TablePro/Core/Database/DatabaseManager.swift:601-632` +- **Problem**: Line 649 captures `session` into a local variable. Lines 682-690 re-fetch from `activeSessions[sessionId]`, creating inconsistency. The local `session` could have stale schema/database values. +- **Fix**: Always re-fetch from `activeSessions[sessionId]` before accessing mutable state, or pass all needed state as parameters. + +### 3.3 [x] Fire-and-forget SSH tunnel close (HIGH) +- **Files**: `TablePro/Core/Database/DatabaseManager.swift:131-135, 220-224, 258-260` +- **Problem**: SSH tunnels are closed in `Task { try? await ... }` blocks. If the task is cancelled or fails, the tunnel remains open. No error logging. Appears in 3+ locations. +- **Fix**: `await` the tunnel close synchronously or at minimum log errors. + +### 3.4 [x] Discarded schema/database switch errors during reconnect (HIGH) +- **File**: `TablePro/Core/Database/DatabaseManager.swift:620-629, 682-691` +- **Problem**: `try? await schemaDriver.switchSchema(to: savedSchema)` silently discards errors. If schema switch fails, the connection operates in the wrong schema without any warning. +- **Fix**: Log the error and optionally update session status to reflect the failure. + +### 3.5 [-] No cancel-await-run synchronization for queries (MEDIUM → demoted to P3) +- **File**: `TablePro/Views/Main/MainContentCoordinator.swift:37-39` +- **Problem**: `currentQueryTask?.cancel()` followed immediately by `runQuery()`. Cancel doesn't wait for the running task to finish, so two queries can be in-flight simultaneously writing to the same `QueryTab`. +- **Fix**: `await` the cancelled task's completion before starting the new query, or use a serial queue. + +### 3.6 [x] Health monitor ping skips when queriesInFlight exists (MEDIUM) +- **File**: `TablePro/Core/Database/DatabaseManager.swift:535` +- **Problem**: `guard await self.queriesInFlight[connectionId] == nil else { return true }` returns "healthy" if any query is in-flight. But a stuck query would keep the monitor returning true forever, masking a dead connection. +- **Fix**: Track query start time and consider connections unhealthy if a query has been in-flight beyond a threshold. + +### 3.7 [x] Inconsistent exponential backoff between DatabaseManager and HealthMonitor (LOW) +- **Files**: `DatabaseManager.swift:745`, `ConnectionHealthMonitor.swift:38, 230-238` +- **Problem**: DatabaseManager uses `2.0 * pow(2.0, retryCount)` formula. HealthMonitor uses a predefined `[2.0, 4.0, 8.0]` array with different progression. Same app, different reconnect timing. +- **Fix**: Unify backoff strategy into a shared utility. + +### 3.8 [x] Synchronous plugin loading fallback on main thread (MEDIUM) +- **File**: `TablePro/Core/Database/DatabaseDriver.swift:318-333` +- **Problem**: `PluginManager.shared.loadPendingPlugins()` is called synchronously on main thread when a plugin isn't loaded yet. Can freeze the UI if loading takes >100ms. +- **Fix**: Pre-load all plugins at app startup, or make this async with a loading indicator. + +--- + +## 4. Architecture & State Management + +### 4.1 [-] AppSettingsManager dual notification channels (HIGH → demoted to P3) +- **File**: `TablePro/Core/Storage/AppSettingsManager.swift:18-193` +- **Problem**: Settings changes fire BOTH `@Observable` property updates AND `NotificationCenter.post()`. Views that observe both channels get redundant updates. Unclear which to use. +- **Fix**: Use `@Observable` for local subscriptions, remove `NotificationCenter` posts for setting changes. Keep NotificationCenter only for system-wide events (accessibility). + +### 4.2 [x] AppSettingsManager missing observer cleanup (MEDIUM) +- **File**: `TablePro/Core/Storage/AppSettingsManager.swift:179` +- **Problem**: `observeAccessibilityTextSizeChanges()` registers a NotificationCenter observer stored in `@ObservationIgnored`. No `deinit` removes it. +- **Fix**: Add `deinit { if let observer = accessibilityTextSizeObserver { NotificationCenter.default.removeObserver(observer) } }`. + +### 4.3 [-] SharedSidebarState registry race condition (MEDIUM → demoted to P3) +- **File**: `TablePro/Models/UI/SharedSidebarState.swift:51-62` +- **Problem**: `static var registry: [UUID: SharedSidebarState]` with no synchronization. Concurrent calls from different threads can create duplicate instances for the same connection. +- **Fix**: Mark SharedSidebarState and registry as `@MainActor`, or use `OSAllocatedUnfairLock`. + +### 4.4 [-] Static termination observer never removed (MEDIUM → demoted to P3) +- **File**: `TablePro/Views/Main/MainContentCoordinator.swift:197-203` +- **Problem**: `registerTerminationObserver` is static and called once. The observer is never removed and fires for every coordinator instance at app termination. +- **Fix**: Make instance-level, remove in teardown. Or use a dedicated `AppTerminationManager`. + +### 4.5 [x] Undo stack destroyed on tab switch (MEDIUM) +- **File**: `TablePro/Core/ChangeTracking/DataChangeManager.swift:129-137` +- **Problem**: `clearChanges()` calls `undoManager.clearAll()`. When switching tabs, if `clearChanges()` is called, the undo history is permanently lost even though changes are persisted via `saveState()`/`restoreState()`. +- **Fix**: Separate `clearChanges()` (preserves undo) from `clearChangesAndUndoHistory()` (full reset on commit). + +### 4.6 [x] Tab state save silently fails on disk errors (MEDIUM) +- **File**: `TablePro/Core/Storage/TabDiskActor.swift:79-81` +- **Problem**: File I/O errors during tab state save are logged but never returned. If write fails (disk full), tabs are lost silently. +- **Fix**: Return `Result` so callers can retry or alert the user. + +### 4.7 [x] Silent startup command failures (LOW) +- **File**: `TablePro/Core/Database/DatabaseManager.swift:783-793` +- **Problem**: User-defined startup SQL commands that fail are logged but the connection continues. User expects commands like `SET ROLE` to succeed but has no indication of failure. +- **Fix**: Collect errors and show a warning: "Startup commands failed. Continue anyway?" + +--- + +## 5. SwiftUI Patterns + +### 5.1 [-] Unnecessary DispatchQueue.main.async in SwiftUI callbacks (MEDIUM → demoted to P3) +- **Files** (8+ locations): + - `TablePro/Views/Results/DataGridView.swift:400-403, 433-441, 537-545` + - `TablePro/Views/AIChat/AIChatPanelView.swift:205-207, 225-227` + - `TablePro/Views/Main/Child/MainEditorContentView.swift:348, 358-365` + - `TablePro/Views/Toolbar/ConnectionSwitcherPopover.swift:351-352` + - `TablePro/Views/Components/SQLReviewPopover.swift:90-91` + - `TablePro/Views/Connection/WelcomeWindowView.swift:770-778` + - `TablePro/Views/Results/Extensions/DataGridView+Editing.swift:164-166, 196-199, 218-221` +- **Problem**: `onAppear`, `onChange`, and body callbacks already execute on main thread. Wrapping in `DispatchQueue.main.async` adds latency and can cause visual glitches or double-renders. +- **Fix**: Remove unnecessary `DispatchQueue.main.async` wrappers. Use `Task { @MainActor in }` only when deferral is genuinely needed. + +### 5.2 [-] Multiple onChange handlers on related state (LOW) +- **File**: `TablePro/Views/AIChat/AIChatPanelView.swift:210-228` +- **Problem**: Three separate `onChange` handlers for `messages.last?.content`, `messages.count`, and `activeConversationID` all trigger scrolling. If a new message arrives on a conversation switch, scrolling happens multiple times. +- **Fix**: Consolidate into a single scroll trigger. + +### 5.3 [x] Recursive asyncAfter retry for window focus (LOW) +- **File**: `TablePro/Views/Connection/WelcomeWindowView.swift:770-778` +- **Problem**: Recursive `DispatchQueue.main.asyncAfter(deadline: .now() + 0.02)` with retry count. No cancellation if view disappears. Hardcoded timing. +- **Fix**: Use a cancellable `Task` with `Task.sleep`. + +### 5.4 [-] DispatchQueue.main.async in updateNSView path (LOW) +- **File**: `TablePro/Views/Results/DataGridView.swift:400-403, 433-441` +- **Problem**: Inside `updateNSView` -> `updateColumns`, a binding update is dispatched to the next cycle. This causes the view to re-evaluate before the flag is cleared, potentially causing flicker. +- **Fix**: Use `Task.yield()` or accept the binding update synchronously. + +--- + +## Priority Order for Fixes + +### P0 - Critical (fix immediately) +1. **1.1** Active tab evicted on window focus loss +2. **3.1** Missing trackOperation in reconnectDriver + +### P1 - High (fix soon) +3. **2.1** Missing redisDatabaseSwitchTask cancellation +4. **2.2** Missing deinit in AIChatViewModel +5. **3.3** Fire-and-forget SSH tunnel close +6. **3.4** Discarded schema/database switch errors +7. **3.2** Stale session reference in reconnectSession +8. **1.2** Unreliable window discovery via NSApp.keyWindow +9. **1.3** Duplicate window discovery pattern +10. **4.1** AppSettingsManager dual notification channels + +### P2 - Medium (fix in next cycle) +11. **2.3** Weak self in periodic Task loops +12. **2.4** Eviction pending-changes check incomplete +13. **3.5** No cancel-await-run synchronization +14. **3.6** Health monitor ping masking stuck queries +15. **3.8** Synchronous plugin loading on main thread +16. **4.2** AppSettingsManager missing observer cleanup +17. **4.3** SharedSidebarState registry race condition +18. **4.4** Static termination observer never removed +19. **4.5** Undo stack destroyed on tab switch +20. **4.6** Tab state save silently fails +21. **1.4** Fragile subtitle-based window matching +22. **1.5** Notification observer fires before window registration +23. **5.1** Unnecessary DispatchQueue.main.async in SwiftUI + +### P3 - Low (backlog) +24. **1.6** Stale isKeyWindow initialization +25. **1.7** Inconsistent window identifier matching +26. **1.8** ObjectIdentifier-based window tracking +27. **1.9** Unnecessary async in notification handlers +28. **1.10** Duplicate observers for didBecomeKeyNotification +29. **2.5** WindowLifecycleMonitor observer re-registration +30. **2.6** Fire-and-forget disconnect Task +31. **3.7** Inconsistent exponential backoff +32. **4.7** Silent startup command failures +33. **5.2** Multiple onChange handlers on related state +34. **5.3** Recursive asyncAfter retry +35. **5.4** DispatchQueue.main.async in updateNSView From 9493b95df9f251f882eec523e54af236bd7613fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ng=C3=B4=20Qu=E1=BB=91c=20=C4=90=E1=BA=A1t?= Date: Tue, 24 Mar 2026 09:23:28 +0700 Subject: [PATCH 3/5] fix: address review feedback on anti-patterns PR - WindowAccessor: deduplicate viewDidMoveToWindow calls, update closure in updateNSView - AIChatViewModel: add comment explaining nonisolated(unsafe) requirement - CHANGELOG: add user-visible behavioral changes under [Unreleased] --- CHANGELOG.md | 5 +++++ TablePro/ViewModels/AIChatViewModel.swift | 2 ++ TablePro/Views/Components/WindowAccessor.swift | 11 +++++++---- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4fbd7202..6e6a0a34 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - MongoDB Atlas connections failing to authenticate (#438) - MongoDB TLS certificate verification skipped for SRV connections +- Active tab data no longer refreshes when switching back to the app window +- Undo history preserved when switching between database tables +- Health monitor now detects stuck queries beyond the configured timeout +- SSH tunnel closure errors now logged instead of silently discarded +- Schema/database restore errors during reconnect now logged ## [0.23.1] - 2026-03-24 diff --git a/TablePro/ViewModels/AIChatViewModel.swift b/TablePro/ViewModels/AIChatViewModel.swift index 26d580a7..b975de1c 100644 --- a/TablePro/ViewModels/AIChatViewModel.swift +++ b/TablePro/ViewModels/AIChatViewModel.swift @@ -88,6 +88,8 @@ final class AIChatViewModel { // MARK: - Private + /// nonisolated(unsafe) is required because deinit is not @MainActor-isolated, + /// so accessing a @MainActor property from deinit requires opting out of isolation. @ObservationIgnored nonisolated(unsafe) private var streamingTask: Task? private var streamingAssistantID: UUID? private var lastUsedFeature: AIFeature = .chat diff --git a/TablePro/Views/Components/WindowAccessor.swift b/TablePro/Views/Components/WindowAccessor.swift index 6cd8c8e8..6f45c8d4 100644 --- a/TablePro/Views/Components/WindowAccessor.swift +++ b/TablePro/Views/Components/WindowAccessor.swift @@ -13,16 +13,19 @@ struct WindowAccessor: NSViewRepresentable { return view } - func updateNSView(_ nsView: WindowAccessorView, context: Context) {} + func updateNSView(_ nsView: WindowAccessorView, context: Context) { + nsView.onWindow = onWindow + } } final class WindowAccessorView: NSView { var onWindow: ((NSWindow) -> Void)? + private weak var capturedWindow: NSWindow? override func viewDidMoveToWindow() { super.viewDidMoveToWindow() - if let window { - onWindow?(window) - } + guard let window, window !== capturedWindow else { return } + capturedWindow = window + onWindow?(window) } } From 260f53245a5128be0be2e86a48f6d4c710a6ce4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ng=C3=B4=20Qu=E1=BB=91c=20=C4=90=E1=BA=A1t?= Date: Tue, 24 Mar 2026 09:24:57 +0700 Subject: [PATCH 4/5] chore: remove anti-patterns tracker doc --- docs/development/anti-patterns-tracker.md | 255 ---------------------- 1 file changed, 255 deletions(-) delete mode 100644 docs/development/anti-patterns-tracker.md diff --git a/docs/development/anti-patterns-tracker.md b/docs/development/anti-patterns-tracker.md deleted file mode 100644 index f635f189..00000000 --- a/docs/development/anti-patterns-tracker.md +++ /dev/null @@ -1,255 +0,0 @@ -# Anti-Patterns & Incorrect Behavior Tracker - -Status legend: `[ ]` = TODO, `[x]` = Fixed, `[-]` = Won't fix - ---- - -## 1. Window Lifecycle & Focus Management - -### 1.1 [x] Active tab evicted on window focus loss (CRITICAL) -- **File**: `TablePro/Views/Main/MainContentView.swift:349-365` -- **Problem**: When the window loses focus, `evictInactiveRowData()` evicts ALL tabs including the currently visible one. When the user switches back, the visible grid re-fetches data, causing a visible refresh flicker. -- **Correct behavior**: Native macOS apps (Xcode, Safari, TablePlus) keep the active tab's content in memory. Only background/non-visible tabs should be eviction candidates. -- **Fix**: `evictInactiveRowData()` must skip the currently selected tab. - -### 1.2 [x] Unreliable window discovery via NSApp.keyWindow (HIGH) -- **File**: `TablePro/Views/Main/MainContentView.swift:219-244` -- **Problem**: Window registration uses `DispatchQueue.main.async` then queries `NSApp.keyWindow`, which can return the wrong window in multi-window scenarios. Fallback uses fragile title-based matching. -- **Fix**: Capture the window through SwiftUI's view hierarchy (NSViewRepresentable) instead of querying global state. - -### 1.3 [x] Duplicate window discovery pattern in command actions (HIGH) -- **File**: `TablePro/Views/Main/MainContentView.swift:624-633` -- **Problem**: `actions.window = NSApp.keyWindow` with async retry if nil. Same unreliable pattern as 1.2. -- **Fix**: Share the window reference captured in 1.2's fix. - -### 1.4 [x] Fragile subtitle-based window matching (MEDIUM) -- **File**: `TablePro/Views/Main/MainContentView.swift:276-283` -- **Problem**: `window.subtitle == connectionName` is fragile -- subtitles can be empty, duplicated, or changed. Inconsistent with `WindowLifecycleMonitor.hasWindows()` used elsewhere. -- **Fix**: Always use `WindowLifecycleMonitor.hasWindows(for: connectionId)` as single source of truth. - -### 1.5 [x] Notification observer fires before window registration (MEDIUM) -- **File**: `TablePro/Views/Main/MainContentView.swift:324-348` -- **Problem**: `viewWindow` is set asynchronously in `onAppear`, so `didBecomeKeyNotification` observers that compare `notificationWindow === viewWindow` silently miss early notifications when `viewWindow` is still nil. -- **Fix**: Ensure window registration completes before subscribing to notifications, or queue missed notifications. - -### 1.6 [x] Stale isKeyWindow initialization (MEDIUM) -- **File**: `TablePro/Views/Main/MainContentView.swift:243` -- **Problem**: `isKeyWindow = window.isKeyWindow` is a single-point-in-time snapshot that's immediately stale. The notification handlers already track this separately, creating dual sources of truth. -- **Fix**: Remove the synchronous initialization; let `didBecomeKeyNotification`/`didResignKeyNotification` be the sole source. - -### 1.7 [x] Inconsistent window identifier matching (LOW) -- **File**: `TablePro/AppDelegate+WindowConfig.swift:103-115` -- **Problem**: `isMainWindow` uses `.contains("main")`, `isWelcomeWindow` uses `== "welcome"` OR title fallback, `isConnectionFormWindow` uses `.contains("connection-form")`. Substring matching can match unintended windows. -- **Fix**: Use exact matching with a centralized `WindowIdentifier` enum/constants. - -### 1.8 [-] ObjectIdentifier-based window tracking (LOW) -- **File**: `TablePro/AppDelegate+WindowConfig.swift:210-248` -- **Problem**: `configuredWindows` uses `ObjectIdentifier(window)` which is memory-address-based. If an NSWindow is deallocated and a new one allocated at the same address, they get the same identifier. -- **Fix**: Use UUID-based window identifiers. - -### 1.9 [x] Unnecessary DispatchQueue.main.async in notification handlers (LOW) -- **File**: `TablePro/AppDelegate+WindowConfig.swift:263-282` -- **Problem**: Window operations dispatched async from notification handlers that already run on main thread. Introduces race windows where window state changes between check and action. -- **Fix**: Keep critical window operations synchronous when already on main thread. - -### 1.10 [-] Duplicate observers for didBecomeKeyNotification (LOW) -- **Files**: `TablePro/AppDelegate+WindowConfig.swift:210+`, `TablePro/Views/Main/MainContentView.swift:324+` -- **Problem**: Both AppDelegate and MainContentView observe `NSWindow.didBecomeKeyNotification` independently. Both fire for every window focus change. -- **Fix**: Centralize in `WindowLifecycleMonitor` and have views subscribe to a custom signal. - ---- - -## 2. Memory Management & Task Lifecycle - -### 2.1 [x] Missing redisDatabaseSwitchTask cancellation in teardown (HIGH) -- **File**: `TablePro/Views/Main/MainContentCoordinator.swift:106-108, 350-355` -- **Problem**: `currentQueryTask` and `changeManagerUpdateTask` are cancelled in `teardown()`, but `redisDatabaseSwitchTask` is not. This leaves a dangling Task if coordinator is deallocated during a Redis database switch. -- **Fix**: Add `redisDatabaseSwitchTask?.cancel(); redisDatabaseSwitchTask = nil` to `teardown()`. - -### 2.2 [x] Missing deinit in AIChatViewModel (HIGH) -- **File**: `TablePro/ViewModels/AIChatViewModel.swift:91-92` -- **Problem**: `streamingTask` is never cancelled if the view model is deallocated while streaming. The Task continues executing in the background. -- **Fix**: Add `deinit { streamingTask?.cancel() }`. - -### 2.3 [x] Weak self with optional chaining in periodic Task loops (MEDIUM) -- **Files**: - - `TablePro/Core/Services/Licensing/LicenseManager.swift:86-100` - - `TablePro/Core/Services/Infrastructure/AnalyticsService.swift:63-75` -- **Problem**: Inside `while !Task.isCancelled` loops, `self?.revalidationInterval ?? 604_800` silently uses the fallback value if self is deallocated, then continues the loop with a default interval forever. -- **Fix**: Use `guard let self else { return }` at the top of the task, or re-check inside the loop with early exit. - -### 2.4 [x] Eviction pending-changes check is incomplete (MEDIUM) -- **File**: `TablePro/Views/Main/MainContentView.swift:349-365` -- **Problem**: The pre-eviction check uses `changeManager.hasChanges` (in-memory changes), but `evictInactiveRowData()` checks `tab.pendingChanges.hasChanges` (tab-level). These are different — a tab may have tab-level changes even if `changeManager` is empty. -- **Fix**: Check both `changeManager.hasChanges` and `tab.pendingChanges.hasChanges` consistently, or defer the check to just before eviction runs. - -### 2.5 [x] WindowLifecycleMonitor observer re-registration without full cleanup (LOW) -- **File**: `TablePro/Core/Services/Infrastructure/WindowLifecycleMonitor.swift:41-66` -- **Problem**: If `register()` is called twice with the same `windowId` but a different `NSWindow`, the old observer is removed but the old NSWindow is now orphaned without observation. -- **Fix**: Guard against re-registration with a different window or explicitly unregister the old window. - -### 2.6 [-] Fire-and-forget disconnect Task in WindowLifecycleMonitor (LOW) -- **File**: `TablePro/Core/Services/Infrastructure/WindowLifecycleMonitor.swift:190-211` -- **Problem**: `Task { await DatabaseManager.shared.disconnectSession(...) }` has no error handling. If disconnect fails, the connection persists as a zombie. -- **Fix**: Log errors from the disconnect call. - ---- - -## 3. Connection & Query Execution - -### 3.1 [x] Missing trackOperation in reconnectDriver (CRITICAL) -- **File**: `TablePro/Core/Database/DatabaseManager.swift:599-632` -- **Problem**: `reconnectDriver()` does NOT call `trackOperation()`, so `queriesInFlight` is never incremented. The health monitor's ping (`SELECT 1`) can race with the reconnect on the same driver, causing concurrent driver access. -- **Fix**: Wrap reconnection in `trackOperation()` or stop the health monitor before reconnecting. - -### 3.2 [x] Stale session reference in reconnectSession (HIGH) -- **File**: `TablePro/Core/Database/DatabaseManager.swift:601-632` -- **Problem**: Line 649 captures `session` into a local variable. Lines 682-690 re-fetch from `activeSessions[sessionId]`, creating inconsistency. The local `session` could have stale schema/database values. -- **Fix**: Always re-fetch from `activeSessions[sessionId]` before accessing mutable state, or pass all needed state as parameters. - -### 3.3 [x] Fire-and-forget SSH tunnel close (HIGH) -- **Files**: `TablePro/Core/Database/DatabaseManager.swift:131-135, 220-224, 258-260` -- **Problem**: SSH tunnels are closed in `Task { try? await ... }` blocks. If the task is cancelled or fails, the tunnel remains open. No error logging. Appears in 3+ locations. -- **Fix**: `await` the tunnel close synchronously or at minimum log errors. - -### 3.4 [x] Discarded schema/database switch errors during reconnect (HIGH) -- **File**: `TablePro/Core/Database/DatabaseManager.swift:620-629, 682-691` -- **Problem**: `try? await schemaDriver.switchSchema(to: savedSchema)` silently discards errors. If schema switch fails, the connection operates in the wrong schema without any warning. -- **Fix**: Log the error and optionally update session status to reflect the failure. - -### 3.5 [-] No cancel-await-run synchronization for queries (MEDIUM → demoted to P3) -- **File**: `TablePro/Views/Main/MainContentCoordinator.swift:37-39` -- **Problem**: `currentQueryTask?.cancel()` followed immediately by `runQuery()`. Cancel doesn't wait for the running task to finish, so two queries can be in-flight simultaneously writing to the same `QueryTab`. -- **Fix**: `await` the cancelled task's completion before starting the new query, or use a serial queue. - -### 3.6 [x] Health monitor ping skips when queriesInFlight exists (MEDIUM) -- **File**: `TablePro/Core/Database/DatabaseManager.swift:535` -- **Problem**: `guard await self.queriesInFlight[connectionId] == nil else { return true }` returns "healthy" if any query is in-flight. But a stuck query would keep the monitor returning true forever, masking a dead connection. -- **Fix**: Track query start time and consider connections unhealthy if a query has been in-flight beyond a threshold. - -### 3.7 [x] Inconsistent exponential backoff between DatabaseManager and HealthMonitor (LOW) -- **Files**: `DatabaseManager.swift:745`, `ConnectionHealthMonitor.swift:38, 230-238` -- **Problem**: DatabaseManager uses `2.0 * pow(2.0, retryCount)` formula. HealthMonitor uses a predefined `[2.0, 4.0, 8.0]` array with different progression. Same app, different reconnect timing. -- **Fix**: Unify backoff strategy into a shared utility. - -### 3.8 [x] Synchronous plugin loading fallback on main thread (MEDIUM) -- **File**: `TablePro/Core/Database/DatabaseDriver.swift:318-333` -- **Problem**: `PluginManager.shared.loadPendingPlugins()` is called synchronously on main thread when a plugin isn't loaded yet. Can freeze the UI if loading takes >100ms. -- **Fix**: Pre-load all plugins at app startup, or make this async with a loading indicator. - ---- - -## 4. Architecture & State Management - -### 4.1 [-] AppSettingsManager dual notification channels (HIGH → demoted to P3) -- **File**: `TablePro/Core/Storage/AppSettingsManager.swift:18-193` -- **Problem**: Settings changes fire BOTH `@Observable` property updates AND `NotificationCenter.post()`. Views that observe both channels get redundant updates. Unclear which to use. -- **Fix**: Use `@Observable` for local subscriptions, remove `NotificationCenter` posts for setting changes. Keep NotificationCenter only for system-wide events (accessibility). - -### 4.2 [x] AppSettingsManager missing observer cleanup (MEDIUM) -- **File**: `TablePro/Core/Storage/AppSettingsManager.swift:179` -- **Problem**: `observeAccessibilityTextSizeChanges()` registers a NotificationCenter observer stored in `@ObservationIgnored`. No `deinit` removes it. -- **Fix**: Add `deinit { if let observer = accessibilityTextSizeObserver { NotificationCenter.default.removeObserver(observer) } }`. - -### 4.3 [-] SharedSidebarState registry race condition (MEDIUM → demoted to P3) -- **File**: `TablePro/Models/UI/SharedSidebarState.swift:51-62` -- **Problem**: `static var registry: [UUID: SharedSidebarState]` with no synchronization. Concurrent calls from different threads can create duplicate instances for the same connection. -- **Fix**: Mark SharedSidebarState and registry as `@MainActor`, or use `OSAllocatedUnfairLock`. - -### 4.4 [-] Static termination observer never removed (MEDIUM → demoted to P3) -- **File**: `TablePro/Views/Main/MainContentCoordinator.swift:197-203` -- **Problem**: `registerTerminationObserver` is static and called once. The observer is never removed and fires for every coordinator instance at app termination. -- **Fix**: Make instance-level, remove in teardown. Or use a dedicated `AppTerminationManager`. - -### 4.5 [x] Undo stack destroyed on tab switch (MEDIUM) -- **File**: `TablePro/Core/ChangeTracking/DataChangeManager.swift:129-137` -- **Problem**: `clearChanges()` calls `undoManager.clearAll()`. When switching tabs, if `clearChanges()` is called, the undo history is permanently lost even though changes are persisted via `saveState()`/`restoreState()`. -- **Fix**: Separate `clearChanges()` (preserves undo) from `clearChangesAndUndoHistory()` (full reset on commit). - -### 4.6 [x] Tab state save silently fails on disk errors (MEDIUM) -- **File**: `TablePro/Core/Storage/TabDiskActor.swift:79-81` -- **Problem**: File I/O errors during tab state save are logged but never returned. If write fails (disk full), tabs are lost silently. -- **Fix**: Return `Result` so callers can retry or alert the user. - -### 4.7 [x] Silent startup command failures (LOW) -- **File**: `TablePro/Core/Database/DatabaseManager.swift:783-793` -- **Problem**: User-defined startup SQL commands that fail are logged but the connection continues. User expects commands like `SET ROLE` to succeed but has no indication of failure. -- **Fix**: Collect errors and show a warning: "Startup commands failed. Continue anyway?" - ---- - -## 5. SwiftUI Patterns - -### 5.1 [-] Unnecessary DispatchQueue.main.async in SwiftUI callbacks (MEDIUM → demoted to P3) -- **Files** (8+ locations): - - `TablePro/Views/Results/DataGridView.swift:400-403, 433-441, 537-545` - - `TablePro/Views/AIChat/AIChatPanelView.swift:205-207, 225-227` - - `TablePro/Views/Main/Child/MainEditorContentView.swift:348, 358-365` - - `TablePro/Views/Toolbar/ConnectionSwitcherPopover.swift:351-352` - - `TablePro/Views/Components/SQLReviewPopover.swift:90-91` - - `TablePro/Views/Connection/WelcomeWindowView.swift:770-778` - - `TablePro/Views/Results/Extensions/DataGridView+Editing.swift:164-166, 196-199, 218-221` -- **Problem**: `onAppear`, `onChange`, and body callbacks already execute on main thread. Wrapping in `DispatchQueue.main.async` adds latency and can cause visual glitches or double-renders. -- **Fix**: Remove unnecessary `DispatchQueue.main.async` wrappers. Use `Task { @MainActor in }` only when deferral is genuinely needed. - -### 5.2 [-] Multiple onChange handlers on related state (LOW) -- **File**: `TablePro/Views/AIChat/AIChatPanelView.swift:210-228` -- **Problem**: Three separate `onChange` handlers for `messages.last?.content`, `messages.count`, and `activeConversationID` all trigger scrolling. If a new message arrives on a conversation switch, scrolling happens multiple times. -- **Fix**: Consolidate into a single scroll trigger. - -### 5.3 [x] Recursive asyncAfter retry for window focus (LOW) -- **File**: `TablePro/Views/Connection/WelcomeWindowView.swift:770-778` -- **Problem**: Recursive `DispatchQueue.main.asyncAfter(deadline: .now() + 0.02)` with retry count. No cancellation if view disappears. Hardcoded timing. -- **Fix**: Use a cancellable `Task` with `Task.sleep`. - -### 5.4 [-] DispatchQueue.main.async in updateNSView path (LOW) -- **File**: `TablePro/Views/Results/DataGridView.swift:400-403, 433-441` -- **Problem**: Inside `updateNSView` -> `updateColumns`, a binding update is dispatched to the next cycle. This causes the view to re-evaluate before the flag is cleared, potentially causing flicker. -- **Fix**: Use `Task.yield()` or accept the binding update synchronously. - ---- - -## Priority Order for Fixes - -### P0 - Critical (fix immediately) -1. **1.1** Active tab evicted on window focus loss -2. **3.1** Missing trackOperation in reconnectDriver - -### P1 - High (fix soon) -3. **2.1** Missing redisDatabaseSwitchTask cancellation -4. **2.2** Missing deinit in AIChatViewModel -5. **3.3** Fire-and-forget SSH tunnel close -6. **3.4** Discarded schema/database switch errors -7. **3.2** Stale session reference in reconnectSession -8. **1.2** Unreliable window discovery via NSApp.keyWindow -9. **1.3** Duplicate window discovery pattern -10. **4.1** AppSettingsManager dual notification channels - -### P2 - Medium (fix in next cycle) -11. **2.3** Weak self in periodic Task loops -12. **2.4** Eviction pending-changes check incomplete -13. **3.5** No cancel-await-run synchronization -14. **3.6** Health monitor ping masking stuck queries -15. **3.8** Synchronous plugin loading on main thread -16. **4.2** AppSettingsManager missing observer cleanup -17. **4.3** SharedSidebarState registry race condition -18. **4.4** Static termination observer never removed -19. **4.5** Undo stack destroyed on tab switch -20. **4.6** Tab state save silently fails -21. **1.4** Fragile subtitle-based window matching -22. **1.5** Notification observer fires before window registration -23. **5.1** Unnecessary DispatchQueue.main.async in SwiftUI - -### P3 - Low (backlog) -24. **1.6** Stale isKeyWindow initialization -25. **1.7** Inconsistent window identifier matching -26. **1.8** ObjectIdentifier-based window tracking -27. **1.9** Unnecessary async in notification handlers -28. **1.10** Duplicate observers for didBecomeKeyNotification -29. **2.5** WindowLifecycleMonitor observer re-registration -30. **2.6** Fire-and-forget disconnect Task -31. **3.7** Inconsistent exponential backoff -32. **4.7** Silent startup command failures -33. **5.2** Multiple onChange handlers on related state -34. **5.3** Recursive asyncAfter retry -35. **5.4** DispatchQueue.main.async in updateNSView From 07d1dbcf4930cdd4f1e0e95f3a134f2f57d480a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ng=C3=B4=20Qu=E1=BB=91c=20=C4=90=E1=BA=A1t?= Date: Tue, 24 Mar 2026 09:27:10 +0700 Subject: [PATCH 5/5] fix: guard clearChangesAndUndoHistory with tab ID check Prevent clearing another tab's undo state when a background query or multi-statement execution completes after the user has switched tabs. --- .../Extensions/MainContentCoordinator+MultiStatement.swift | 6 ++++-- .../Extensions/MainContentCoordinator+QueryHelpers.swift | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+MultiStatement.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+MultiStatement.swift index fc77a090..d8d64019 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+MultiStatement.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+MultiStatement.swift @@ -134,8 +134,10 @@ extension MainContentCoordinator { updatedTab.errorMessage = nil tabManager.tabs[idx] = updatedTab - changeManager.clearChangesAndUndoHistory() - changeManager.reloadVersion += 1 + if tabManager.selectedTabId == tabId { + changeManager.clearChangesAndUndoHistory() + changeManager.reloadVersion += 1 + } } } catch { // Rollback on failure diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+QueryHelpers.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+QueryHelpers.swift index b31c7787..4e6b8359 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+QueryHelpers.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+QueryHelpers.swift @@ -192,7 +192,7 @@ extension MainContentCoordinator { // Clear stale edit state immediately so the save banner // doesn't linger while Phase 2 metadata loads in background. // Only clear if there are no pending edits from the user. - if isEditable && !changeManager.hasChanges { + if tabManager.selectedTabId == tabId, isEditable, !changeManager.hasChanges { changeManager.clearChangesAndUndoHistory() } }