fix: clean up session and UI when driver plugin is missing on connect#297
fix: clean up session and UI when driver plugin is missing on connect#297
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds defensive cleanup when database driver creation fails (SSH tunnel closed, session removed, currentSessionId cleared) and surfaces connection failures to the user by closing the main window, opening the welcome window, and presenting a "Connection Failed" error sheet. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
TablePro/Views/Connection/ConnectionFormView.swift (1)
1039-1046: Consider adding similar error handling here for consistency.The
connectAfterInstallmethod has the same connection flow but currently only logs errors. If connection fails after plugin installation, the main window could still get stuck in a loading state. Consider applying the same recovery pattern (close main, open welcome, show alert) for a consistent user experience.This is not blocking for this PR, as it's outside the scope of the current fix.
♻️ Suggested improvement
private func connectAfterInstall(_ connection: DatabaseConnection) { openWindow(id: "main", value: EditorTabPayload(connectionId: connection.id)) NSApplication.shared.closeWindows(withId: "welcome") Task { do { try await dbManager.connectToSession(connection) } catch { Self.logger.error( "Failed to connect after plugin install: \(error.localizedDescription, privacy: .public)") + NSApplication.shared.closeWindows(withId: "main") + openWindow(id: "welcome") + AlertHelper.showErrorSheet( + title: String(localized: "Connection Failed"), + message: error.localizedDescription, + window: nil + ) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Connection/ConnectionFormView.swift` around lines 1039 - 1046, The connectAfterInstall Task currently only logs errors; update its catch block to perform the same recovery flow used elsewhere: after catching the error from dbManager.connectToSession(connection) call inside connectAfterInstall, call the existing closeMainWindow() and openWelcomeWindow() helpers (or their equivalents) and then dispatch to the main actor to present the user-facing alert (e.g., showAlert or presentInstallationFailureAlert) with the error.localizedDescription; keep the Self.logger.error(...) call but append this recovery sequence so the UI doesn't remain stuck loading when connection fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@TablePro/Views/Connection/ConnectionFormView.swift`:
- Around line 1039-1046: The connectAfterInstall Task currently only logs
errors; update its catch block to perform the same recovery flow used elsewhere:
after catching the error from dbManager.connectToSession(connection) call inside
connectAfterInstall, call the existing closeMainWindow() and openWelcomeWindow()
helpers (or their equivalents) and then dispatch to the main actor to present
the user-facing alert (e.g., showAlert or presentInstallationFailureAlert) with
the error.localizedDescription; keep the Self.logger.error(...) call but append
this recovery sequence so the UI doesn't remain stuck loading when connection
fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd9360c6-ea62-4389-8a57-faadb4e7bdad
📒 Files selected for processing (2)
TablePro/Core/Database/DatabaseManager.swiftTablePro/Views/Connection/ConnectionFormView.swift
Summary
DatabaseDriverFactory.createDriver()in a do/catch that cleans up the session (activeSessions,currentSessionId, SSH tunnel) when the plugin is missing. Previously the session stayed in.connectingstatus forever, leaving the main window stuck in a loading state.Test plan
Summary by CodeRabbit