fix(tabs): keep a restored table tab on its own database and load it after reconnect#1719
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5479130f6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Task { | ||
| await coordinator.switchDatabase( | ||
| to: selectedTab.tableContext.databaseName, clearTabs: false | ||
| ) | ||
| coordinator.lazyLoadCurrentTabIfNeeded() |
There was a problem hiding this comment.
When needsLazyLoad is set and the selected restored tab belongs to a different database, this schedules the switch asynchronously but handleConnectionStatusChange still falls through to the unconditional lazyLoadCurrentTabIfNeeded() later in the same method while the session is on the old database. That early load can register the tab's load task and execute the table query before the switch completes, so the post-switch load here is skipped or the tab can show data/errors from the previous database. Return after queuing the switch, or otherwise prevent the lower lazy-load path from running until the context switch is complete.
Useful? React with 👍 / 👎.
| } | ||
| } else { | ||
| executeQueryInternal(sql) | ||
| executeQueryInternal(sql, isAutoLoad: true) |
There was a problem hiding this comment.
This path is used by runQuery() for table tabs, not only by restored/lazy loads. Passing isAutoLoad: true makes explicit table executions such as Run/Refresh enter the new reconnect-error suppression path, so if the driver is disconnected the catch block suppresses the error, sets needsLazyLoad, and may replay the query later without user feedback. Reserve isAutoLoad for actual automatic loads and let explicit table executions report connection failures.
Useful? React with 👍 / 👎.
…e no-op database switch
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5d50298c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await switchDatabase(to: newTab.tableContext.databaseName, clearTabs: false) | ||
| lazyLoadCurrentTabIfNeeded() |
There was a problem hiding this comment.
When switching to a table tab in another PostgreSQL database, this path now reconnects the database and immediately lazy-loads the tab, but switchDatabase clears session.currentSchema for plugins that require reconnects (DatabaseManager+Sessions.swift lines 267-272; PostgreSQL sets that capability in PostgreSQLPlugin.swift line 124). For restored/open tabs in a non-default schema, the selected tab's tableContext.schemaName is never restored before lazyLoadCurrentTabIfNeeded(), so schema-dependent metadata/FK/row-edit fetches run against the default schema even though the tab belongs to another schema. Restore newTab.tableContext.schemaName after the database switch and before auto-loading.
Useful? React with 👍 / 👎.
1 participant
Problem
Reopening the app with a table tab open on a PostgreSQL connection (whose default database differs from the tab's database, e.g. an SSH-tunneled server) showed a "Query Execution Failed / Not connected to database" alert, and the restored tab opened and then closed without loading its data.
Root cause
Two coupled defects in the restore + reconnect path:
handleTabChangeto callswitchDatabase.switchDatabaseclears all tabs (correct for a deliberate user switch, since old tabs belong to the old database), so it wiped the very tab being restored. The query actually ran and returned rows, but the tab was already gone.DatabaseError.notConnected), so it surfaced as the modal. Separately,reconnectSessionnever re-syncedcurrentSchemafrom the driver, leaving a stale mismatch that routed restore through a fragile path.Fix
switchDatabase(to:clearTabs:)gains aclearTabsflag (defaulttrue, unchanged for user switches). Selecting a tab and the restore paths now passclearTabs: false, so switching the active database to match a tab keeps that tab and loads it.handleTabChange,restoreConnectionContext, and the deferredhandleConnectionStatusChangepath preserve the tab and calllazyLoadCurrentTabIfNeededafter the switch..connected, keyed on the driver's connection status rather than a specific error type. It loads on its own once the connection is ready.reconnectSessionsyncscurrentSchemafrom the reconnected driver, matching the initial connect.restoreSchemaAndRunQueryno longer runs a query when the driver is nil; it defers instead.User-initiated database switches (toolbar, sidebar "Use as Active Database", quick switcher) keep the tab-clearing behavior.
Tests
restoreSchemaAndRunQuerydefers vianeedsLazyLoadinstead of running a query when the driver is not ready.Verification
Verified by hand on an SSH PostgreSQL connection whose default database differs from the open table: quit with the table open, reopen, and the tab stays open and loads its rows with no error alert.