fix(datagrid): key staged table operations by the object they target - #2556
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This was referenced Aug 27, 2026
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.
Found while investigating #2482 (rename a table and rename a database). Three verified defects in the staged table-operation path, all one root cause: a queued Truncate or Drop identified its target by a bare table name.
This is a prerequisite for the rename work, because renaming reconciles open tabs against the same identity and would otherwise inherit the same bug in a new place.
What was wrong
1. A staged drop or truncate ran against the database in front at Save time, not the one it was staged in.
ConnectionSession.pendingTruncates/pendingDeleteswereSet<String>of bare names, living on the connection and surviving a database switch.buildDataWritePlanthen resolved them againstselectedTabScope.Reproduction on MySQL, deterministic: two databases each holding
orders. Close every editor tab so the plan falls through to the browse scope. Browsestaging, right-clickorders, Delete, confirm. Switch toproduction. Press Save. Preview SQL and the authorization gate both showDROP TABLE \orders``, and it drops the production table.2. Drop and truncate emitted unqualified statements typed
TABLEon every hierarchical-schema engine.TableOperationSQLBuilderlooked the object's schema and kind up in the connection's flat table cache.SchemaService.runHierarchicalLoadwrites.loaded([])into that cache, so on Oracle, Dameng, Trino, Snowflake and BigQuery the lookup always missed.Reproduction on Oracle, connected as
SYSTEM, browsingHR: right-clickHR.EMP_VIEW, choose Drop View, Save. The statement isDROP TABLE "EMP_VIEW": no schema, wrong keyword. It raises ORA-00942, and for a table rather than a view it resolves againstSYSTEMinstead.3. Dropping a table closed the tab on a same-named table somewhere else.
closeTabsForDroppedTablesmatcheddeletedTables.contains(tab.tableContext.tableName)and compared nothing else.Reproduction on PostgreSQL: one database,
public.usersandanalytics.users, a tab open on each. Dropanalytics.usersand Save. Both tabs close and both row buffers are discarded, with nothing to undo it.The fix
The queue holds
DatabaseTreeTableRef, which carries database, schema, table and kind, instead of a name.TableOperationSQLBuilderreads the schema and the object's keyword off the queued reference, so it no longer consults a catalogue at all and [WIP] Improve system keyboard shortcut functionality #2 disappears with the lookup.StagedWriteScope(pure, unit-tested) decides where a save runs. A queue names its own database and the plan follows it, not the tab in front.TableTabIdentityis the triple a table tab is keyed on from the moment it opens, so the drop reconciliation matches the object rather than the name.WindowSidebarState.selectedTablescarries references rather thanTableInfo, which is where the database was being discarded on the way from the tree to the commands.Two refusals rather than guesses, both surfaced through the existing save-error path: a queue that reaches two databases, and a queue for a database the connection has since left. The second one exists because the statements are generated by the session's own driver before the plan is pinned, and a driver that qualifies from its live connection writes the database it is on into the statement.
SnowflakePluginDriver.qualifiedNamebuilds a three-part name fromcurrentDatabase,SurrealDBPluginDriveropens with its current namespace. Pinning afterwards cannot correct a name already written.Verified
verify.sh build: PASSverify.sh testover 17 suites: 154 executed, 154 passedverify.sh lint TablePro: 0 violations (the oneagent docswarning,CLAUDE.md:218AXCell, predates this branch)|inside a quoted identifier colliding two references in the same set). The fourth asked for the explanatory doc comments to be removed as "comments"; they follow the convention every file in this area already uses, which is to record why a shape exists and which defect produced it, so they stay.No UI automation: none of the three has a deterministic XCUITest flow, since each needs two databases or two schemas on a live server.