Skip to content

chore: address low-priority cleanup items #403

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
datlechin wants to merge 4 commits into from
Closed

chore: address low-priority cleanup items #403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
74db7b3
fix: correct window management and event handling patterns
datlechin Mar 21, 2026
6ea678e
refactor: clean up service layer, storage, and SwiftUI view patterns
datlechin Mar 21, 2026
f0417dc
refactor: fix model, viewmodel, and theme architecture
datlechin Mar 21, 2026
f5eb348
chore: address low-priority cleanup items
datlechin Mar 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions TablePro/AppDelegate+ConnectionHandler.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ extension AppDelegate {
// MARK: - Database URL Handler

func handleDatabaseURL(_ url: URL) {
guard WindowOpener.shared.openWindow != nil else {
guard WindowOpener.shared.isReady else {
queuedURLEntries.append(.databaseURL(url))
scheduleQueuedURLProcessing()
return
Expand Down Expand Up @@ -51,7 +51,11 @@ extension AppDelegate {
connection = buildTransientConnection(from: parsed)
}

let isTransient = matchedConnection == nil

if !parsed.password.isEmpty {
// Save password to Keychain so the driver can resolve it at connect time.
// For transient connections, this is cleaned up after connect completes or fails.
ConnectionStorage.shared.savePassword(parsed.password, for: connection.id)
}

Expand All @@ -70,6 +74,14 @@ extension AppDelegate {
openNewConnectionWindow(for: connection)

Task { @MainActor in
defer {
// Clean up Keychain entry for transient connections after connect.
// The driver already holds the password in memory once connected,
// so the Keychain entry is no longer needed.
if isTransient {
ConnectionStorage.shared.deletePassword(for: connection.id)
}
}
do {
try await DatabaseManager.shared.connectToSession(connection)
for window in NSApp.windows where self.isWelcomeWindow(window) {
Expand All @@ -86,7 +98,7 @@ extension AppDelegate {
// MARK: - SQLite File Handler

func handleSQLiteFile(_ url: URL) {
guard WindowOpener.shared.openWindow != nil else {
guard WindowOpener.shared.isReady else {
queuedURLEntries.append(.sqliteFile(url))
scheduleQueuedURLProcessing()
return
Expand Down Expand Up @@ -131,7 +143,7 @@ extension AppDelegate {
// MARK: - DuckDB File Handler

func handleDuckDBFile(_ url: URL) {
guard WindowOpener.shared.openWindow != nil else {
guard WindowOpener.shared.isReady else {
queuedURLEntries.append(.duckdbFile(url))
scheduleQueuedURLProcessing()
return
Expand Down Expand Up @@ -176,7 +188,7 @@ extension AppDelegate {
// MARK: - Generic Database File Handler

func handleGenericDatabaseFile(_ url: URL, type dbType: DatabaseType) {
guard WindowOpener.shared.openWindow != nil else {
guard WindowOpener.shared.isReady else {
queuedURLEntries.append(.genericDatabaseFile(url, dbType))
scheduleQueuedURLProcessing()
return
Expand Down Expand Up @@ -229,7 +241,7 @@ extension AppDelegate {

var ready = false
for _ in 0..<25 {
if WindowOpener.shared.openWindow != nil { ready = true; break }
if WindowOpener.shared.isReady { ready = true; break }
try? await Task.sleep(for: .milliseconds(200))
}
guard let self else { return }
Expand Down Expand Up @@ -258,7 +270,7 @@ extension AppDelegate {

// MARK: - SQL File Queue (drained by .databaseDidConnect)

@objc func handleDatabaseDidConnect() {
@MainActor @objc func handleDatabaseDidConnect() {
guard !queuedFileURLs.isEmpty else { return }
let urls = queuedFileURLs
queuedFileURLs.removeAll()
Expand Down
4 changes: 1 addition & 3 deletions TablePro/AppDelegate+FileOpen.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,9 +193,7 @@ extension AppDelegate {
ConnectionStorage.shared.addConnection(connection)
NotificationCenter.default.post(name: .connectionUpdated, object: nil)

if let openWindow = WindowOpener.shared.openWindow {
openWindow(id: "connection-form", value: connection.id)
}
NotificationCenter.default.post(name: .openConnectionFormWindow, object: connection.id)
}

// MARK: - Plugin Install
Expand Down
10 changes: 5 additions & 5 deletions TablePro/AppDelegate+WindowConfig.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,11 @@ extension AppDelegate {
return menu
}

@objc func showWelcomeFromDock() {
@MainActor @objc func showWelcomeFromDock() {
openWelcomeWindow()
}

@objc func connectFromDock(_ sender: NSMenuItem) {
@MainActor @objc func connectFromDock(_ sender: NSMenuItem) {
guard let connectionId = sender.representedObject as? UUID else { return }
let connections = ConnectionStorage.shared.loadConnections()
guard let connection = connections.first(where: { $0.id == connectionId }) else { return }
Expand Down Expand Up @@ -201,7 +201,7 @@ extension AppDelegate {

// MARK: - Window Notifications

@objc func windowDidBecomeKey(_ notification: Notification) {
@MainActor @objc func windowDidBecomeKey(_ notification: Notification) {
guard let window = notification.object as? NSWindow else { return }
let windowId = ObjectIdentifier(window)

Expand Down Expand Up @@ -241,7 +241,7 @@ extension AppDelegate {
}
}

@objc func windowWillClose(_ notification: Notification) {
@MainActor @objc func windowWillClose(_ notification: Notification) {
guard let window = notification.object as? NSWindow else { return }

configuredWindows.remove(ObjectIdentifier(window))
Expand All @@ -261,7 +261,7 @@ extension AppDelegate {
}
}

@objc func windowDidChangeOcclusionState(_ notification: Notification) {
@MainActor @objc func windowDidChangeOcclusionState(_ notification: Notification) {
guard let window = notification.object as? NSWindow,
isHandlingFileOpen else { return }

Expand Down
4 changes: 0 additions & 4 deletions TablePro/AppDelegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,4 @@ class AppDelegate: NSObject, NSApplicationDelegate {
func applicationWillTerminate(_ notification: Notification) {
SSHTunnelManager.shared.terminateAllProcessesSync()
}

nonisolated deinit {
NotificationCenter.default.removeObserver(self)
}
}
2 changes: 0 additions & 2 deletions TablePro/ContentView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,6 @@ struct ContentView: View {
@State private var windowTitle: String
@Environment(\.openWindow)
private var openWindow
@Environment(AppState.self) private var appState

private let storage = ConnectionStorage.shared

init(payload: EditorTabPayload?) {
Expand Down
61 changes: 32 additions & 29 deletions TablePro/Core/Database/DatabaseManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,17 +139,11 @@ final class DatabaseManager {
}

do {
try await driver.connect()

// Apply query timeout from settings
let timeoutSeconds = AppSettingsManager.shared.general.queryTimeoutSeconds
if timeoutSeconds > 0 {
try await driver.applyQueryTimeout(timeoutSeconds)
}

// Run startup commands before schema init
await executeStartupCommands(
connection.startupCommands, on: driver, connectionName: connection.name
// Perform heavy driver setup off the main actor
try await performDriverSetup(
driver: driver,
startupCommands: connection.startupCommands,
connectionName: connection.name
)

// Initialize schema for drivers that support schema switching
Expand Down Expand Up @@ -330,6 +324,7 @@ final class DatabaseManager {

/// Test-only: remove an injected session
internal func removeSession(for connectionId: UUID) {
SharedSidebarState.removeConnection(connectionId)
removeSessionEntry(for: connectionId)
}
#endif
Expand Down Expand Up @@ -605,16 +600,12 @@ final class DatabaseManager {
// Use effective connection (tunneled) if available, otherwise original
let connectionForDriver = session.effectiveConnection ?? session.connection
let driver = try DatabaseDriverFactory.createDriver(for: connectionForDriver)
try await driver.connect()

// Apply timeout
let timeoutSeconds = AppSettingsManager.shared.general.queryTimeoutSeconds
if timeoutSeconds > 0 {
try await driver.applyQueryTimeout(timeoutSeconds)
}

await executeStartupCommands(
session.connection.startupCommands, on: driver, connectionName: session.connection.name
// Perform heavy driver setup off the main actor
try await performDriverSetup(
driver: driver,
startupCommands: session.connection.startupCommands,
connectionName: session.connection.name
)

if let savedSchema = session.currentSchema,
Expand Down Expand Up @@ -667,16 +658,12 @@ final class DatabaseManager {

// Create new driver and connect
let driver = try DatabaseDriverFactory.createDriver(for: effectiveConnection)
try await driver.connect()

// Apply timeout
let timeoutSeconds = AppSettingsManager.shared.general.queryTimeoutSeconds
if timeoutSeconds > 0 {
try await driver.applyQueryTimeout(timeoutSeconds)
}

await executeStartupCommands(
session.connection.startupCommands, on: driver, connectionName: session.connection.name
// Perform heavy driver setup off the main actor
try await performDriverSetup(
driver: driver,
startupCommands: session.connection.startupCommands,
connectionName: session.connection.name
)

if let savedSchema = activeSessions[sessionId]?.currentSchema,
Expand Down Expand Up @@ -768,6 +755,22 @@ final class DatabaseManager {

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

/// Connects the driver, applies query timeout, and runs startup commands off the main actor.
nonisolated private func performDriverSetup(
driver: DatabaseDriver,
startupCommands: String?,
connectionName: String
) async throws {
try await driver.connect()

let timeoutSeconds = await AppSettingsManager.shared.general.queryTimeoutSeconds
if timeoutSeconds > 0 {
try await driver.applyQueryTimeout(timeoutSeconds)
}

await executeStartupCommands(startupCommands, on: driver, connectionName: connectionName)
}

nonisolated private func executeStartupCommands(
_ commands: String?, on driver: DatabaseDriver, connectionName: String
) async {
Expand Down
7 changes: 7 additions & 0 deletions TablePro/Core/Plugins/PluginMetadataRegistry.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,13 @@ struct PluginMetadataSnapshot: Sendable {
}
}

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

Expand Down
2 changes: 1 addition & 1 deletion TablePro/Core/SchemaTracking/StructureChangeManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ final class StructureChangeManager {
self.currentForeignKeys = groupedFKs.keys.sorted().compactMap { name -> EditableForeignKeyDefinition? in
guard let fkInfos = groupedFKs[name], let first = fkInfos.first else { return nil }
return EditableForeignKeyDefinition(
id: first.id,
id: UUID(),
name: first.name,
columns: fkInfos.map { $0.column },
referencedTable: first.referencedTable,
Expand Down
15 changes: 1 addition & 14 deletions TablePro/Core/Services/Export/ExportService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,20 +72,7 @@ final class ExportService {

// MARK: - Cancellation

private let isCancelledLock = NSLock()
private var _isCancelled: Bool = false
var isCancelled: Bool {
get {
isCancelledLock.lock()
defer { isCancelledLock.unlock() }
return _isCancelled
}
set {
isCancelledLock.lock()
_isCancelled = newValue
isCancelledLock.unlock()
}
}
private(set) var isCancelled = false

func cancelExport() {
isCancelled = true
Expand Down
15 changes: 6 additions & 9 deletions TablePro/Core/Services/Infrastructure/WindowOpener.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,20 @@ internal final class WindowOpener {

internal static let shared = WindowOpener()

/// Set by ContentView when it appears. Safe to store — OpenWindowAction is app-scoped, not view-scoped.
internal var openWindow: OpenWindowAction?
/// True once any SwiftUI scene has appeared and stored `openWindow`.
/// Used as a readiness check by AppDelegate cold-start queue.
internal var isReady: Bool = false

/// The connectionId for the next window about to be opened.
/// Set by `openNativeTab` before calling `openWindow`, consumed by
/// `AppDelegate.windowDidBecomeKey` to set the correct `tabbingIdentifier`.
internal var pendingConnectionId: UUID?

/// Opens a new native window tab with the given payload.
/// Stores the connectionId so AppDelegate can set the correct tabbingIdentifier.
/// Opens a new native window tab by posting a notification.
/// The `OpenWindowHandler` in TableProApp receives it and calls `openWindow`.
internal func openNativeTab(_ payload: EditorTabPayload) {
pendingConnectionId = payload.connectionId
guard let openWindow else {
Self.logger.warning("openNativeTab called before openWindow was set — payload dropped")
return
}
openWindow(id: "main", value: payload)
NotificationCenter.default.post(name: .openMainWindow, object: payload)
}

/// Returns and clears the pending connectionId (consume-once pattern).
Expand Down
23 changes: 19 additions & 4 deletions TablePro/Core/Storage/KeychainHelper.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,10 +263,8 @@ final class KeychainHelper {
continue
}

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

@coderabbitai coderabbitai bot Mar 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if the password sync setting UI has appropriate warning text
rg -n -A5 -B5 'passwordSyncEnabled|keychainPasswordSyncEnabled' --type swift -g '!*.generated.swift' | head -80

Repository: datlechin/TablePro

Length of output: 4557

🏁 Script executed:

# First, let's find the SyncSettingsView file and check its full content
wc -l TablePro/Views/Settings/SyncSettingsView.swift

Repository: datlechin/TablePro

Length of output: 112

🏁 Script executed:

# Read the SyncSettingsView.swift file to see the UI implementation
cat -n TablePro/Views/Settings/SyncSettingsView.swift | sed -n '180,210p'

Repository: datlechin/TablePro

Length of output: 1583

🏁 Script executed:

# Search for any confirmation dialogs or alerts related to password sync or keychain
rg -n 'alert|confirm|confirmation|warning|dialog' --type swift TablePro/Views/Settings/ | grep -i 'password\|sync\|keychain' || echo "No alerts found with those keywords"

Repository: datlechin/TablePro

Length of output: 97

🏁 Script executed:

# More broadly search for presentationMode, sheet, or alert modifiers in SyncSettingsView
rg -n 'presentationMode|\.sheet|\.alert|\.confirmationDialog' --type swift TablePro/Views/Settings/SyncSettingsView.swift

Repository: datlechin/TablePro

Length of output: 44

🏁 Script executed:

# Read the entire SyncSettingsView.swift to see the UI implementation
cat -n TablePro/Views/Settings/SyncSettingsView.swift

Repository: datlechin/TablePro

Length of output: 9300

🏁 Script executed:

# Search for changelog or release notes files
fd -t f -i 'changelog|release|news|history' . | head -20

Repository: datlechin/TablePro

Length of output: 849

🏁 Script executed:

# Also search for any documentation about this breaking change
rg -n 'delete.*keychain|remove.*iCloud|opt.out.*password' --type md --type txt | head -20

Repository: datlechin/TablePro

Length of output: 44

🏁 Script executed:

# Check the CHANGELOG.md for any entries about password sync or keychain behavior changes
head -100 CHANGELOG.md

Repository: datlechin/TablePro

Length of output: 4189

Add user confirmation before silently deleting iCloud Keychain passwords on opt-out.

The code at lines 285-296 deletes synchronizable keychain entries when users toggle off password sync. This will remove credentials from all other devices with iCloud Keychain enabled, but users are not warned or asked to confirm.

Current state:

  • Settings UI (line 125) only states: "Syncs passwords via iCloud Keychain (end-to-end encrypted)"
  • No confirmation dialog before deletion
  • Behavior change not documented in CHANGELOG.md

Required:

  1. Add a confirmation dialog when toggling password sync off, explaining that this will delete credentials from all synced devices
  2. Document this breaking behavior in release notes
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@TablePro/Core/Storage/KeychainHelper.swift` around lines 281 - 298, Before
calling SecItemDelete(deleteSyncQuery...) when kSecAttrSynchronizable is being
turned off, present a user confirmation dialog that clearly states "Turning off
iCloud Keychain sync will delete these credentials from all devices using iCloud
Keychain" and requires explicit user consent; only proceed with constructing
deleteSyncQuery and invoking SecItemDelete if the user confirms, otherwise abort
the deletion and leave the local non‑synchronizable item as-is. Update the
Settings UI toggle handler that flips kSecAttrSynchronizable (the code path that
currently performs the migration and calls deleteSyncQuery/SecItemDelete) to
show the dialog and gate the deletion on the affirmative response, and add a
note to the RELEASE/CHANGELOG documenting this breaking behavior so users are
warned in the release notes.

Expand Down
Loading
Loading