Skip to content

Commit 719122b

Browse files
authored
fix: resolve 26 anti-patterns across window lifecycle, memory, connections, and SwiftUI (#441)
* fix: MongoDB Atlas auth failure due to wrong authSource default (#438) * 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 * 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] * chore: remove anti-patterns tracker doc * 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.
1 parent f1f5707 commit 719122b

26 files changed

Lines changed: 266 additions & 142 deletions

CHANGELOG.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Fixed
11+
12+
- MongoDB Atlas connections failing to authenticate (#438)
13+
- MongoDB TLS certificate verification skipped for SRV connections
14+
- Active tab data no longer refreshes when switching back to the app window
15+
- Undo history preserved when switching between database tables
16+
- Health monitor now detects stuck queries beyond the configured timeout
17+
- SSH tunnel closure errors now logged instead of silently discarded
18+
- Schema/database restore errors during reconnect now logged
19+
1020
## [0.23.1] - 2026-03-24
1121

1222
### Added

Plugins/MongoDBDriverPlugin/MongoDBConnection.swift

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,8 @@ final class MongoDBConnection: @unchecked Sendable {
190190
let effectiveAuthSource: String
191191
if let source = authSource, !source.isEmpty {
192192
effectiveAuthSource = source
193+
} else if useSrv {
194+
effectiveAuthSource = "admin"
193195
} else if !database.isEmpty {
194196
effectiveAuthSource = database
195197
} else {
@@ -206,8 +208,7 @@ final class MongoDBConnection: @unchecked Sendable {
206208
let sslEnabled = ["Preferred", "Required", "Verify CA", "Verify Identity"].contains(sslMode)
207209
if sslEnabled {
208210
params.append("tls=true")
209-
let verifiesCert = sslMode == "Verify CA" || sslMode == "Verify Identity"
210-
if !verifiesCert {
211+
if sslMode == "Preferred" {
211212
params.append("tlsAllowInvalidCertificates=true")
212213
}
213214
if !sslCACertPath.isEmpty {

TablePro/AppDelegate+WindowConfig.swift

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -100,18 +100,22 @@ extension AppDelegate {
100100

101101
// MARK: - Window Identification
102102

103+
private enum WindowId {
104+
static let main = "main"
105+
static let welcome = "welcome"
106+
static let connectionForm = "connection-form"
107+
}
108+
103109
func isMainWindow(_ window: NSWindow) -> Bool {
104-
guard let identifier = window.identifier?.rawValue else { return false }
105-
return identifier.contains("main")
110+
window.identifier?.rawValue == WindowId.main
106111
}
107112

108113
func isWelcomeWindow(_ window: NSWindow) -> Bool {
109-
window.identifier?.rawValue == "welcome" ||
110-
window.title.lowercased().contains("welcome")
114+
window.identifier?.rawValue == WindowId.welcome
111115
}
112116

113117
private func isConnectionFormWindow(_ window: NSWindow) -> Bool {
114-
window.identifier?.rawValue.contains("connection-form") == true
118+
window.identifier?.rawValue == WindowId.connectionForm
115119
}
116120

117121
// MARK: - Welcome Window
@@ -259,10 +263,7 @@ extension AppDelegate {
259263

260264
if remainingMainWindows == 0 {
261265
NotificationCenter.default.post(name: .mainWindowWillClose, object: nil)
262-
263-
DispatchQueue.main.async {
264-
self.openWelcomeWindow()
265-
}
266+
openWelcomeWindow()
266267
}
267268
}
268269
}
@@ -273,13 +274,9 @@ extension AppDelegate {
273274

274275
if isWelcomeWindow(window),
275276
window.occlusionState.contains(.visible),
276-
NSApp.windows.contains(where: { isMainWindow($0) && $0.isVisible }) {
277-
DispatchQueue.main.async { [weak self] in
278-
guard let self else { return }
279-
if self.isWelcomeWindow(window), window.isVisible {
280-
window.close()
281-
}
282-
}
277+
NSApp.windows.contains(where: { isMainWindow($0) && $0.isVisible }),
278+
window.isVisible {
279+
window.close()
283280
}
284281
}
285282

TablePro/Core/ChangeTracking/DataChangeManager.swift

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,8 @@ final class DataChangeManager {
125125

126126
// MARK: - Configuration
127127

128-
/// Clear all changes (called after successful save)
128+
/// Clear all tracked changes, preserving undo/redo history.
129+
/// Use when changes are invalidated but undo context may still be relevant.
129130
func clearChanges() {
130131
changes.removeAll()
131132
changeIndex.removeAll()
@@ -134,11 +135,18 @@ final class DataChangeManager {
134135
modifiedCells.removeAll()
135136
insertedRowData.removeAll()
136137
changedRowIndices.removeAll()
137-
undoManager.clearAll()
138138
hasChanges = false
139139
reloadVersion += 1
140140
}
141141

142+
/// Clear all tracked changes AND undo/redo history.
143+
/// Use after successful save, explicit discard, or new query execution
144+
/// where undo context is no longer meaningful.
145+
func clearChangesAndUndoHistory() {
146+
clearChanges()
147+
undoManager.clearAll()
148+
}
149+
142150
/// Atomically configure the manager for a new table
143151
func configureForTable(
144152
tableName: String,

TablePro/Core/Database/ConnectionHealthMonitor.swift

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@ actor ConnectionHealthMonitor {
3535
// MARK: - Configuration
3636

3737
private static let pingInterval: TimeInterval = 30.0
38-
private static let initialBackoffDelays: [TimeInterval] = [2.0, 4.0, 8.0]
3938
private static let maxBackoffDelay: TimeInterval = 120.0
4039

4140
// MARK: - Dependencies
@@ -227,15 +226,7 @@ actor ConnectionHealthMonitor {
227226
/// Uses the initial delay table for the first few attempts, then doubles
228227
/// the previous delay for subsequent attempts, capped at `maxBackoffDelay`.
229228
private func backoffDelay(for attempt: Int) -> TimeInterval {
230-
let delays = Self.initialBackoffDelays
231-
if attempt <= delays.count {
232-
return delays[attempt - 1]
233-
}
234-
// Exponential: last seed delay * 2^(attempt - seedCount)
235-
let lastSeed = delays[delays.count - 1]
236-
let exponent = attempt - delays.count
237-
let computed = lastSeed * pow(2.0, Double(exponent))
238-
return min(computed, Self.maxBackoffDelay)
229+
ExponentialBackoff.delay(for: attempt, maxDelay: Self.maxBackoffDelay)
239230
}
240231

241232
// MARK: - State Transitions

TablePro/Core/Database/DatabaseDriver.swift

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -315,12 +315,15 @@ extension DatabaseDriver {
315315
/// Factory for creating database drivers via plugin lookup
316316
@MainActor
317317
enum DatabaseDriverFactory {
318+
private static let logger = Logger(subsystem: "com.TablePro", category: "DatabaseDriverFactory")
319+
318320
static func createDriver(for connection: DatabaseConnection) throws -> DatabaseDriver {
319321
let pluginId = connection.type.pluginTypeId
320322
// If the plugin isn't registered yet and background loading hasn't finished,
321323
// fall back to synchronous loading for this critical code path.
322324
if PluginManager.shared.driverPlugins[pluginId] == nil,
323325
!PluginManager.shared.hasFinishedInitialLoad {
326+
logger.warning("Plugin '\(pluginId)' not loaded yet — performing synchronous load")
324327
PluginManager.shared.loadPendingPlugins()
325328
}
326329
guard let plugin = PluginManager.shared.driverPlugins[pluginId] else {

TablePro/Core/Database/DatabaseManager.swift

Lines changed: 73 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,8 @@ final class DatabaseManager {
4949
/// The health monitor skips pings while a query is running to avoid
5050
/// racing on non-thread-safe driver connections.
5151
@ObservationIgnored private var queriesInFlight: [UUID: Int] = [:]
52+
/// Tracks when the first query started for each session (used for staleness detection).
53+
@ObservationIgnored private var queryStartTimes: [UUID: Date] = [:]
5254

5355
/// Current session (computed from currentSessionId)
5456
var currentSession: ConnectionSession? {
@@ -130,7 +132,11 @@ final class DatabaseManager {
130132
// Close tunnel if SSH was established
131133
if connection.sshConfig.enabled {
132134
Task {
133-
try? await SSHTunnelManager.shared.closeTunnel(connectionId: connection.id)
135+
do {
136+
try await SSHTunnelManager.shared.closeTunnel(connectionId: connection.id)
137+
} catch {
138+
Self.logger.warning("SSH tunnel cleanup failed for \(connection.name): \(error.localizedDescription)")
139+
}
134140
}
135141
}
136142
removeSessionEntry(for: connection.id)
@@ -220,7 +226,11 @@ final class DatabaseManager {
220226
// Close tunnel if connection failed
221227
if connection.sshConfig.enabled {
222228
Task {
223-
try? await SSHTunnelManager.shared.closeTunnel(connectionId: connection.id)
229+
do {
230+
try await SSHTunnelManager.shared.closeTunnel(connectionId: connection.id)
231+
} catch {
232+
Self.logger.warning("SSH tunnel cleanup failed for \(connection.name): \(error.localizedDescription)")
233+
}
224234
}
225235
}
226236

@@ -256,7 +266,11 @@ final class DatabaseManager {
256266

257267
// Close SSH tunnel if exists
258268
if session.connection.sshConfig.enabled {
259-
try? await SSHTunnelManager.shared.closeTunnel(connectionId: session.connection.id)
269+
do {
270+
try await SSHTunnelManager.shared.closeTunnel(connectionId: session.connection.id)
271+
} catch {
272+
Self.logger.warning("SSH tunnel cleanup failed for \(session.connection.name): \(error.localizedDescription)")
273+
}
260274
}
261275

262276
// Stop health monitoring
@@ -343,11 +357,15 @@ final class DatabaseManager {
343357
operation: () async throws -> T
344358
) async throws -> T {
345359
queriesInFlight[sessionId, default: 0] += 1
360+
if queriesInFlight[sessionId] == 1 {
361+
queryStartTimes[sessionId] = Date()
362+
}
346363
defer {
347364
if let count = queriesInFlight[sessionId], count > 1 {
348365
queriesInFlight[sessionId] = count - 1
349366
} else {
350367
queriesInFlight.removeValue(forKey: sessionId)
368+
queryStartTimes.removeValue(forKey: sessionId)
351369
}
352370
}
353371
return try await operation()
@@ -402,13 +420,21 @@ final class DatabaseManager {
402420
result = try await driver.testConnection()
403421
} catch {
404422
if connection.sshConfig.enabled {
405-
try? await SSHTunnelManager.shared.closeTunnel(connectionId: connection.id)
423+
do {
424+
try await SSHTunnelManager.shared.closeTunnel(connectionId: connection.id)
425+
} catch {
426+
Self.logger.warning("SSH tunnel cleanup failed for \(connection.name): \(error.localizedDescription)")
427+
}
406428
}
407429
throw error
408430
}
409431

410432
if connection.sshConfig.enabled {
411-
try? await SSHTunnelManager.shared.closeTunnel(connectionId: connection.id)
433+
do {
434+
try await SSHTunnelManager.shared.closeTunnel(connectionId: connection.id)
435+
} catch {
436+
Self.logger.warning("SSH tunnel cleanup failed for \(connection.name): \(error.localizedDescription)")
437+
}
412438
}
413439

414440
return result
@@ -532,7 +558,16 @@ final class DatabaseManager {
532558
guard let self else { return false }
533559
// Skip ping while a user query is in-flight to avoid racing
534560
// on the same non-thread-safe driver connection.
535-
guard await self.queriesInFlight[connectionId] == nil else { return true }
561+
// Allow ping if the query appears stuck (exceeds timeout + grace period).
562+
if await self.queriesInFlight[connectionId] != nil {
563+
let queryTimeout = await TimeInterval(AppSettingsManager.shared.general.queryTimeoutSeconds)
564+
let maxStale = max(queryTimeout, 300) // At least 5 minutes
565+
if let startTime = await self.queryStartTimes[connectionId],
566+
Date().timeIntervalSince(startTime) < maxStale {
567+
return true // Query still within expected time
568+
}
569+
// Query appears stuck — fall through to ping
570+
}
536571
guard let mainDriver = await self.activeSessions[connectionId]?.driver else {
537572
return false
538573
}
@@ -547,12 +582,13 @@ final class DatabaseManager {
547582
guard let self else { return false }
548583
guard let session = await self.activeSessions[connectionId] else { return false }
549584
do {
550-
let driver = try await self.reconnectDriver(for: session)
585+
let driver = try await self.trackOperation(sessionId: connectionId) {
586+
try await self.reconnectDriver(for: session)
587+
}
551588
await self.updateSession(connectionId) { session in
552589
session.driver = driver
553590
session.status = .connected
554591
}
555-
556592
return true
557593
} catch {
558594
return false
@@ -619,13 +655,21 @@ final class DatabaseManager {
619655

620656
if let savedSchema = session.currentSchema,
621657
let schemaDriver = driver as? SchemaSwitchable {
622-
try? await schemaDriver.switchSchema(to: savedSchema)
658+
do {
659+
try await schemaDriver.switchSchema(to: savedSchema)
660+
} catch {
661+
Self.logger.warning("Failed to restore schema '\(savedSchema)' on reconnect: \(error.localizedDescription)")
662+
}
623663
}
624664

625665
// Restore database for MSSQL if session had a non-default database
626666
if let savedDatabase = session.currentDatabase,
627667
let adapter = driver as? PluginDriverAdapter {
628-
try? await adapter.switchDatabase(to: savedDatabase)
668+
do {
669+
try await adapter.switchDatabase(to: savedDatabase)
670+
} catch {
671+
Self.logger.warning("Failed to restore database '\(savedDatabase)' on reconnect: \(error.localizedDescription)")
672+
}
629673
}
630674

631675
return driver
@@ -659,8 +703,8 @@ final class DatabaseManager {
659703
await stopHealthMonitor(for: sessionId)
660704

661705
do {
662-
// Disconnect existing drivers
663-
session.driver?.disconnect()
706+
// Disconnect existing driver (re-fetch to avoid stale local reference)
707+
activeSessions[sessionId]?.driver?.disconnect()
664708

665709
// Recreate SSH tunnel if needed and build effective connection
666710
let effectiveConnection = try await buildEffectiveConnection(for: session.connection)
@@ -681,13 +725,21 @@ final class DatabaseManager {
681725

682726
if let savedSchema = activeSessions[sessionId]?.currentSchema,
683727
let schemaDriver = driver as? SchemaSwitchable {
684-
try? await schemaDriver.switchSchema(to: savedSchema)
728+
do {
729+
try await schemaDriver.switchSchema(to: savedSchema)
730+
} catch {
731+
Self.logger.warning("Failed to restore schema '\(savedSchema)' on reconnect: \(error.localizedDescription)")
732+
}
685733
}
686734

687735
// Restore database for MSSQL if session had a non-default database
688736
if let savedDatabase = activeSessions[sessionId]?.currentDatabase,
689737
let adapter = driver as? PluginDriverAdapter {
690-
try? await adapter.switchDatabase(to: savedDatabase)
738+
do {
739+
try await adapter.switchDatabase(to: savedDatabase)
740+
} catch {
741+
Self.logger.warning("Failed to restore database '\(savedDatabase)' on reconnect: \(error.localizedDescription)")
742+
}
691743
}
692744

693745
// Update session
@@ -741,8 +793,7 @@ final class DatabaseManager {
741793

742794
let maxRetries = 5
743795
for retryCount in 0..<maxRetries {
744-
// Exponential backoff: 2s, 4s, 8s, 16s, 32s (capped at 60s)
745-
let delay = min(60.0, 2.0 * pow(2.0, Double(retryCount)))
796+
let delay = ExponentialBackoff.delay(for: retryCount + 1, maxDelay: 60)
746797
Self.logger.info("SSH reconnect attempt \(retryCount + 1)/\(maxRetries) in \(delay)s for: \(session.connection.name)")
747798
try? await Task.sleep(nanoseconds: UInt64(delay * 1_000_000_000))
748799

@@ -768,18 +819,20 @@ final class DatabaseManager {
768819

769820
nonisolated private static let startupLogger = Logger(subsystem: "com.TablePro", category: "DatabaseManager")
770821

822+
@discardableResult
771823
nonisolated private func executeStartupCommands(
772824
_ commands: String?, on driver: DatabaseDriver, connectionName: String
773-
) async {
825+
) async -> [(statement: String, error: String)] {
774826
guard let commands, !commands.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else {
775-
return
827+
return []
776828
}
777829

778830
let statements = commands
779831
.components(separatedBy: CharacterSet(charactersIn: ";\n"))
780832
.map { $0.trimmingCharacters(in: .whitespacesAndNewlines) }
781833
.filter { !$0.isEmpty }
782834

835+
var failures: [(statement: String, error: String)] = []
783836
for statement in statements {
784837
do {
785838
_ = try await driver.execute(query: statement)
@@ -790,8 +843,10 @@ final class DatabaseManager {
790843
Self.startupLogger.warning(
791844
"Startup command failed for '\(connectionName)': \(statement)\(error.localizedDescription)"
792845
)
846+
failures.append((statement: statement, error: error.localizedDescription))
793847
}
794848
}
849+
return failures
795850
}
796851

797852
// MARK: - Schema Changes

0 commit comments

Comments
 (0)