Skip to content

fix(datagrid): key staged table operations by the object they target - #2556

Merged
datlechin merged 1 commit into
mainfrom
fix/staged-table-operation-identity
Aug 27, 2026
Merged

fix(datagrid): key staged table operations by the object they target#2556
datlechin merged 1 commit into
mainfrom
fix/staged-table-operation-identity

Conversation

@datlechin

Copy link
Copy Markdown
Member

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 / pendingDeletes were Set<String> of bare names, living on the connection and surviving a database switch. buildDataWritePlan then resolved them against selectedTabScope.

Reproduction on MySQL, deterministic: two databases each holding orders. Close every editor tab so the plan falls through to the browse scope. Browse staging, right-click orders, Delete, confirm. Switch to production. Press Save. Preview SQL and the authorization gate both show DROP TABLE \orders``, and it drops the production table.

2. Drop and truncate emitted unqualified statements typed TABLE on every hierarchical-schema engine.

TableOperationSQLBuilder looked the object's schema and kind up in the connection's flat table cache. SchemaService.runHierarchicalLoad writes .loaded([]) into that cache, so on Oracle, Dameng, Trino, Snowflake and BigQuery the lookup always missed.

Reproduction on Oracle, connected as SYSTEM, browsing HR: right-click HR.EMP_VIEW, choose Drop View, Save. The statement is DROP TABLE "EMP_VIEW": no schema, wrong keyword. It raises ORA-00942, and for a table rather than a view it resolves against SYSTEM instead.

3. Dropping a table closed the tab on a same-named table somewhere else.

closeTabsForDroppedTables matched deletedTables.contains(tab.tableContext.tableName) and compared nothing else.

Reproduction on PostgreSQL: one database, public.users and analytics.users, a tab open on each. Drop analytics.users and 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.

  • TableOperationSQLBuilder reads 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.
  • TableTabIdentity is the triple a table tab is keyed on from the moment it opens, so the drop reconciliation matches the object rather than the name.
  • The tree's contextual menu narrows a cross-database selection to the clicked row's own database. One save runs against one database, so gathering rows from two of them into one queue has no correct answer; asking for them separately does.
  • WindowSidebarState.selectedTables carries references rather than TableInfo, 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.qualifiedName builds a three-part name from currentDatabase, SurrealDBPluginDriver opens with its current namespace. Pinning afterwards cannot correct a name already written.

Verified

  • verify.sh build: PASS
  • verify.sh test over 17 suites: 154 executed, 154 passed
  • verify.sh lint TablePro: 0 violations (the one agent docs warning, CLAUDE.md:218 AXCell, predates this branch)
  • Codex reviewed the working tree and found four issues; three were real and are fixed here (the stateful-driver generation hole above, legacy restored tabs whose persisted database is empty, and | 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.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@datlechin
datlechin merged commit 819dab5 into main Aug 27, 2026
8 checks passed
@datlechin
datlechin deleted the fix/staged-table-operation-identity branch August 27, 2026 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant