fix(tabs): bind each tab to its own database instead of the sidebar's selection (#2026) - #2027
Merged
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…wsed database (#2026) Claude-Session: https://claude.ai/code/session_018VQbD2VKEBFifVpa9ojAYQ
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2026. Also covers the part of #2015 that survived its fix.
The bug
Three reports, one cause:
ton database A, switch the sidebar to B: the tab stays open (correct, per Query tabs are lost when switching schemas #1669/feat: close all the tabs #1972) but errorsTable 'B.t' doesn't exist.Root cause
An operation was addressed by
connectionIdalone. A connection id does not say which database, so the database came fromConnectionSession.currentDatabase, one variable doing three unrelated jobs:USE db),Tabs carry their own
(database, schema). The two disagreed the moment the sidebar moved, and the app reconciled them backwards: it moved the global to satisfy the tab.withMetadataDriver(connectionId:)hardcodedsession.activeDatabase, so all 35 call sites read ambient state.TableStructureViewwas handed the tab's database and never used it for the fetch. That is symptom 1.pinDatabaseBeforeSchemaChange, added by the Modifying table structure while multiple connections are open can cause connection corruption #2015 fix, never restored. That is symptom 2.trackOperationis a counter rather than a lock, so even the pinned paths raced across windows. That is symptom 3.The fix
A tab's target becomes a value, resolved once at creation and never re-derived.
DatabaseScope { connectionId, database, schema }.resolvedScopefreezes a tab's identity the wayresolvedSchemaNamealready did one tier down for schema (#1774). An empty database is a legal value meaning "the server", not "unbound", because a connection with no database selected is a supported state.Then a split by operation kind:
MetadataConnectionPoolwas already keyed this way, which is why the Tree sidebar layout never had symptom 1. TheconnectionId-only overload is deleted, so every call site now states tab-scoped or browse-scoped: 17 each, plus one dialog-scoped.SessionDriverGate, a per-connection FIFO mutex. The switch is issued every time rather than cached, because nothing tracks where the driver is: a reconnect, a Redis SELECT, another window or a typedUSEall move it. The pin runs inside the gate and re-validates there, so a route decided before the caller queued cannot let a statement land on the wrong database, and a failed pin throws before the body runs.Cross-database tabs on PostgreSQL, Redshift and CockroachDB run on a pooled connection for their own database, so they work rather than silently returning another database's rows. They do not share temp tables, session variables or an open transaction with the query editor, which is documented. PGlite cannot open a second connection, so it names the database in an error instead.
Why a refactor and not a patch
Threading
database:into the seven structure fetches makes the reported error disappear in about 20 lines. It leaves the data tab's schema fetch, three row-count paths, the column-scope cache key, fetch-all, the ER diagram, row-edit DML, multi-statement DML, column reorder, trigger DDL and create-table all ambient, and it fixes neither of the other two symptoms.This is also the model every mature client uses. DataGrip's Database Explorer never retargets an open console, DBeaver makes "Link with editor" an explicit keystroke, TablePlus opens a new workspace, and Postico seeds only new tabs from the current database. Beekeeper Studio is the one window-scoped client and it produces this exact error string in an issue open since 2022.
Design invariant
A macOS window tab is a full
NSWindowthe user can tear out at any time viamoveTabToNewWindow(_:), anduserTabbingPreferencedecides whether an open even becomes a tab. So "Move Tab to New Window" must be semantically a no-op.scope(for:)reads no window, toolbar or coordinator state, andTabScopeIsWindowIndependentTestsasserts it.Also fixed along the way
MetadataConnectionPool.runSeriallyawaited an unstructured task, so cancelling the caller neither cancelled nor propagated. That is the shape behind Every 2nd Refresh cancels the Query #2021, and it matters now that user queries run there.Testing
New:
DatabaseScopeTests,SessionDriverGateTests,TableStructureLoaderScopeTests,ScopedDriverRoutingTests,DataRefreshScopeTests.TableStructureLoaderScopeTests.everyReadUsesTheTabsDatabaseis the test that would have caught this: browse cursor on B, loader on A, every recorded scope must be A andbrowseScopemust never be consulted.DatabaseManagerSchemaChangeRoutingTestsasserted the bug as correct behaviour (currentDatabase == "orders"after a save on aninventory-cursored session). That assertion is inverted, and the fixture now uses three distinct values so "the saved default was not rewritten" is a real assertion rather than a tautology.Verification
swiftlint lint --strictpasses across all 1244 files, so everything parses. The project was not built, so type checking is unverified. Please build before reviewing in depth and I will fix whatever the compiler reports.A read-only audit was run over the whole diff before opening this. It confirmed 24 findings (3 refuted), all fixed here. The ones worth knowing about:
Rundid nothing on a connection with no database selected, a legal MySQL state. An empty database is now a server scope rather than an unbound one.Not in this change
SchemaServiceis still keyed by connection alone, so two windows on one connection cannot list different databases. Connecting to a new schema will retain the tabs from the previous connection #2026 is fixed without it.DatabaseManager.switchDatabaseshould becomeselectBrowseDatabaseto match the renamed state it writes.ExportModecarries no tab identity.schema:argument and reads its own active database, so export reads on that family follow the pinned driver rather than a per-table override. That is a plugin-side fix.schemaKey, because an ER tab never stores a schema on its tab context.https://claude.ai/code/session_018VQbD2VKEBFifVpa9ojAYQ