Skip to content

fix(editor): forget or move a table's saved settings when SQL drops or renames it - #3196

Open
datlechin wants to merge 7 commits into
mainfrom
fix/editor-drop-rename-forgets-table-settings
Open

datlechin wants to merge 7 commits into
mainfrom
fix/editor-drop-rename-forgets-table-settings

Conversation

@datlechin

@datlechin datlechin commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

Root cause

A table's saved settings (filters, column layout, highlight rules, value display formats, FK label columns), its favorite, its Recent entry and any queued Truncate or Drop are adopted by CatalogEditAdoption.adoptDroppedTables and adoptTableRename. Only two events reach them: CatalogEvent.tablesDropped, posted by the sidebar Drop save path, and .tableRenamed, posted by the sidebar Rename. SQL run in a query tab, and SQL an MCP client runs, posts .statementsRan, which only schedules a catalog refresh. So DROP TABLE people; CREATE TABLE people (...) left the old table's filters, formats and highlight rules on the new table (a saved filter on a column the new table lacks opens the tab on a server error), kept its star and Recent entry, and left a queued sidebar Drop armed against the new table.

.statementsRan cannot carry this. It is posted for failed statements too, on purpose, because a refresh errs toward running. An adoption that forgets settings has to err the other way.

Fix

A second event, CatalogEvent.statementsSucceeded(SucceededStatements), carries only the statements the server accepted, the DatabaseScope they started in, and what the session said about committing them. CatalogChangeService turns it into drops and renames and feeds each one, in order, through the same recordDroppedTables and recordRenamedTable the sidebar uses, so tabs close or take the new name exactly as they do for a sidebar drop or rename.

  • TableEditStatementParser reads DROP TABLE|VIEW|MATERIALIZED VIEW|FOREIGN TABLE [IF EXISTS] a, b [CASCADE ...], ALTER TABLE|VIEW ... RENAME TO|AS, MySQL's bare RENAME b, RENAME TABLE a TO b, c TO d [ON CLUSTER] and SQL Server's EXEC sp_rename 'schema.old', 'new' [, 'OBJECT'], plus transaction control, USE, schema changes, temporary-table creation, and procedure calls and anonymous blocks. It works on SQLTokenCursor, which now exposes the spelling of the last token so an unquoted name keeps its case where the engine preserves it. A statement is read only when it is exactly one of these forms; a column rename, an unknown clause, or a second T-SQL command without a ; makes it .other. A conditional ALTER TABLE IF EXISTS ... RENAME is not read, because PostgreSQL answers it with a notice when the table is missing, and reading it as a rename would move stale settings over the target's own.
  • TableEditDialect places a name per engine: fold to lowercase (PostgreSQL, Redshift) or uppercase (Oracle), otherwise keep it; what a two- and three-part name means; whether USE selects a database. Covered: the MySQL family, the PostgreSQL family and Redshift, SQLite, SQL Server, Oracle, ClickHouse, DuckDB. CockroachDB, Snowflake and everything else answer nil and are left alone.
  • A bare name is placed only where every way the session could redirect it shows up in SQL text. On PostgreSQL, Redshift and DuckDB it does not: a search path moved by set_config, a function or an earlier run (the driver re-pins the schema only when its own record differs), or a temporary table made by SELECT ... INTO TEMP or a function, changes where it resolves, so there only a qualified name is placed. SQL Server resolves a bare name against the login's default schema, so the same holds. On MySQL, SQLite, ClickHouse and Oracle a bare name is placed unless the connection's TableNameHazards say otherwise. That record only grows: a temporary table of that name (CREATE TEMPORARY TABLE, SQLite's CREATE TABLE temp.x), a schema change such as ALTER SESSION SET CURRENT_SCHEMA, or code the server ran unseen (CALL, EXEC, DO, a PL/SQL block, MariaDB's BEGIN NOT ATOMIC). It is read from every statement that ran, failed ones included, because a procedure that failed can have created a temporary table first, and also from the connection's Startup Commands, which run on every connect before the session reports anything, and from the statements a SQL file run through Import Data sends, since the import runs on the editor's own session. A MySQL temporary table also hides the real one under its qualified name, so there the record covers qualified names too. Temporary names are compared case-insensitively, as SQLite finds them. Names inside pg_temp, pg_temp_N or DuckDB's temp catalog are never placed, an Oracle global temporary table is treated as the permanent table it is, and a drop or rename inside a MySQL /*!NNNNN ... */ comment is not read, because an older server skips the body and still answers success.
  • CommittedTableEdits decides what is committed. MySQL, MariaDB, Oracle and ClickHouse commit DDL as it runs, so success is enough. On a transactional engine a lone statement counts only when the session holds no transaction after it (read inside the same lease through heldSessionTransactionState(), and only for a statement that edits a table). A run counts only when the session held no transaction both before its first statement and after its last, both read inside the run's lease; the multi-statement run asks the second time only when it drops or renames a table, because on SQL Server asking is a round trip. A run the app wrapped keeps its edits pending until the app's COMMIT, since a bare ROLLBACK in the script does not stop the app wrapping it and is what ends the app's transaction. The text's own BEGIN, COMMIT, ROLLBACK, ROLLBACK TO, SAVEPOINT and RELEASE are followed with a nesting depth, so BEGIN; DROP TABLE public.people; ROLLBACK changes nothing and an edit still open when the run ends is dropped. A run the app wrapped and then rolled back counts for nothing. SET IMPLICIT_TRANSACTIONS, COMMIT AND CHAIN and PREPARE TRANSACTION stop the tracking for the rest of the run. The text alone cannot be trusted even between two idle answers: under SQL Server's IMPLICIT_TRANSACTIONS, set by an earlier run or by the server's user options, a DROP opens a transaction no statement shows, BEGIN TRAN takes @@TRANCOUNT to 2 and the script's COMMIT leaves it at 1. So a run whose text closes a transaction it never opened, or leaves one open that the session says is closed, adopts nothing. On PostgreSQL a stray COMMIT or ROLLBACK is only a warning, so there this holds back a drop that did commit.
  • SQL Server runs a batch whole, so a batch that succeeded is not a list of statements that did: IF, ELSE, WHILE, GOTO, RETURN, BREAK, CONTINUE, a label or a TRY...CATCH can skip one or swallow its error. A SQL Server run holding any of them adopts nothing. A batch run with GO n is replayed n times, so a rename chain run twice moves nothing.
  • Every execution path reports it: the editor's single statement, its parameterized single statement, the multi-statement run (which now keeps the session state it read before the first statement), the SQL Server batch run, and the MCP bridge's statement and script runs. An MCP script that fails part way still reports the batches that finished, through a ScriptBatchProgress box, with what the session held after the failing batch, because SQL Server committed them unless a transaction is still open.
  • CatalogEditAdoption takes its stores and its favorites storage by injection, matches queued operations by the object they name (TableScope) instead of by DatabaseTreeTableRef equality, and builds a SQL-named table's reference with the favorite spelling the sidebar saved, since the tree keys a favorite by the driver listing's schema, which Oracle, MySQL and SQLite leave empty.

Review

Four Codex review rounds and one independent review. Round one found a no-op ALTER TABLE IF EXISTS rename moving settings, temporary tables not tracked across runs, sp_rename missing, and the successful prefix of a failed MCP script going unreported; all four were fixed. Round two found new defects in the round-one temporary-table tracking (unobserved creation forms, session keying, rollback) and the cross-run search-path drift. Rather than patch those, bare names were cut back on PostgreSQL, Redshift and DuckDB, and the per-connection record became one that only grows, so nothing can take a hazard back wrongly. Two round-two findings are under-adoption, which leaves settings where they already are, and are listed below. Round three found three cheap, real defects, fixed without reopening the loop: a ROLLBACK inside a run the app wrapped, a temporary table matched case-sensitively on SQLite, and DDL inside a version-gated MySQL comment. Its fourth finding, SSH-backed SQLite answering .unknown for its transaction state so a lone statement there adopts nothing, is under-adoption and listed below.

An independent review then found two more. SQL Server implicit transactions set by an earlier run let an uncommitted DROP in a GO script or an MCP script be adopted, because the evidence carried only the session's state before the run: fixed by carrying the state after it too, and by treating a stray COMMIT or ROLLBACK, or a transaction the text leaves open, as the text and the session disagreeing. And the CHANGELOG entry claimed every SQL drop, while a SQL file run through Import Data still only refreshes: the entry now names query tabs and MCP clients, and the docs say Import Data and restores keep table settings. A fourth Codex round on that found three defects in the original design: hazards from Startup Commands and SQL import never recorded, T-SQL control flow skipping a statement the batch still listed, and GO n replayed once. All three are fixed. A fifth Codex round found two more gaps in the hazard record, both fixed: a temporary table renamed onto a real table's name (ALTER TABLE scratch RENAME TO people on SQLite) now keeps shadowing under the new name, and SQLite's CREATE VIRTUAL TABLE temp.people USING fts5(...) is read as a temporary table. It also raised three that are left, listed below with the reason.

Tests

Built and tested with verify.sh in the worktree.

  • New: TableEditStatementParserTests, TableEditDialectTests, CommittedTableEditsTests, SucceededStatementsProbeTests, ScriptBatchProgressTests, BatchStatementOutcomeSucceededCountTests, SQLTableEditAdoptionTests, and one case in SQLTokenCursorTests. With DatabaseAccessBridgeScriptTests, DatabaseAccessBridgeStatementTests, MCPScriptResultTests, ScriptResultEncoderTests, CatalogChangeServiceTests and CatalogChangeTests in the same run: 134 cases, all passed.
  • SQLTableEditAdoptionTests drives CatalogChangeService.record(...) into a real CatalogEditAdoption with a recording settings store and an isolated favorites storage: a drop, a rename, a rolled-back drop, an Oracle favorite saved without a schema, a queued sidebar Drop unstaged when SQL drops and recreates the table, a MySQL temporary table across runs, a failed procedure call reported only through .statementsRan, and a no-op conditional rename.
  • Owning suites of the changed types: BatchStatementRunTests, CatalogChangeClassifierTests, LoadedBrowseCatalogTests, RestoreStagedTableOperationsTests, SavedConnectionDatabaseAdoptionTests, SchemaServiceStaleSchemaTests, SQLSetAssignmentsTests, TableScopedSettingsRegistryTests, TabQueryTaskGuardTests, FavoriteTablesStorageTests, MainContentCoordinatorLazyLoadTests, MainContentCoordinatorSortTests, TabQueryIsolationTests, MySQLKillLatchTests, plus DatabaseAccessBridgeStatementTests: 202 cases, all passed on the commit before the round-three fixes, which touch only the parser, the walker and the evidence type covered by the run above.
  • After the review fixes: CommittedTableEditsTests, SucceededStatementsProbeTests, ScriptBatchProgressTests, ExecutedStatementTextsTests, ImportNameHazardTests, BatchStatementOutcomeSucceededCountTests, SQLTableEditAdoptionTests, TableEditStatementParserTests, TableEditDialectTests, DatabaseAccessBridgeScriptTests, DatabaseAccessBridgeStatementTests, MCPScriptResultTests, ScriptResultEncoderTests, ExplainAuthorizationTests, TabQueryIsolationTests, CatalogChangeServiceTests, BatchStatementRunTests, MultiStatementFailureTests, QueryBatchPlannerTests, SQLServerImportBatchTests, ImportDataSinkAdapterMappingTests, SQLTokenCursorTests: 268 cases, all passed. New cases cover a SQL Server run ending inside a transaction, a stray ROLLBACK and COMMIT, BEGIN TRAN ... COMMIT under implicit transactions, a transaction closed unseen, an MCP script whose session ends inside a transaction (through ScriptBatchRun and ScriptBatchProgress), GOTO, IF and TRY...CATCH batches, a GO 2 rename swap, a temporary table from Startup Commands, an import's hazard statements, a renamed temporary table and a temporary virtual table.
  • swiftlint lint --strict on every changed Swift file: 0 violations. Docs: check-writing-style.sh and check-docs-against-source.py pass.

Not verified, and what is left

  • Not run against live servers. The SQL Server behaviour under IMPLICIT_TRANSACTIONS (@@TRANCOUNT 0 before the first statement, BEGIN TRAN counting 2) is from Microsoft's documentation and the driver's own measured comment, not measured in this branch.
  • Under-adoption the review fixes add, all of which leave settings where they are: a run that ends with a transaction open adopts nothing, a drop it committed before opening one included; a stray COMMIT or ROLLBACK on PostgreSQL holds a run's drops back; a SQL Server run holding any control flow adopts nothing, including drops in its other batches; an MCP script stopped by a timeout adopts nothing, because the session's end answer never came.
  • Left from the fifth Codex round. A GO n batch whose first repetition committed and whose second failed counts as a failed batch and adopts nothing; carrying the repetition count through the batch outcome is its own change, and the gap only leaves settings in place. Savepoints are counted as nesting, so BEGIN; SAVEPOINT s; DROP ...; COMMIT (and SQL Server's SAVE TRAN, which does not raise @@TRANCOUNT) ends the walk one level deep and holds the drop back; a savepoint stack per engine would fix it, again under-adoption only. And a procedure that drops and recreates public.people followed by ALTER TABLE public.people RENAME TO persons moves the name's settings to persons: settings are keyed by name, the hidden drop already left them on the recreated table before this change, and refusing the visible rename would only strand them under a name that no longer exists. Transaction state after a statement comes from each driver's sessionTransactionState(); a driver that answers .unknown on a transactional engine (remote SQLite over SSH, PGlite, libSQL, Turso as far as I can tell) adopts nothing from a single statement.
  • No UI test: the flow needs a live database connection and a query run, which the UI test sandbox does not provide deterministically.
  • Bare names on PostgreSQL, Redshift and DuckDB. DROP TABLE people there still leaves the settings. Placing it safely needs the server's answer rather than the text's: the session's current_schemas(true) read inside the lease after the statement, or a check that the placed table is gone, per statement. That is its own change.
  • Case-insensitive engines (SQLite, SQL Server, MySQL on a case-insensitive file system) keep the settings of a table whose name the statement spells in a different case, because a name is matched exactly. Resolving to the catalog's spelling is a follow-up.
  • The per-connection hazard record is never cleared, so after a temporary table, a procedure call or a schema change, bare names on that connection stay unadopted until the app restarts.
  • Left alone on purpose: sp_rename with named arguments; Oracle's keyword-less RENAME a TO b, which resolves in the login's own schema; a rename that moves a table to another database or schema (ALTER TABLE ... SET SCHEMA, MySQL RENAME TABLE a TO other.a); CREATE OR REPLACE TABLE; DROP DATABASE and DROP SCHEMA run as SQL (the sidebar's container drop has its own adoption); dependents a CASCADE drop takes with it; SQL import runs; a function called from a query that creates a temporary table on MySQL or SQLite.
  • Running a dump that starts with DROP TABLE IF EXISTS t; CREATE TABLE t ... in a query tab now closes open tabs on t and forgets its saved settings where the name is placed, the same as a sidebar drop would.

Found while investigating #3172 (#3183)

CI fixes

The failed Package Tests job was SyncRecordMapperTests.unknownWireValueFailsClosed: the sync mapper's duplicate decoder turned an unknown Safe Mode value into Off. It now uses SafeModeLevel(wireValue:isReadOnly:), requiring confirmation while retaining legacy read-only restrictions. Added regressions for unknown read-only values and renames that preserve an unknown wire value.

Integrated main, preserving both sets of changelog entries. Fixed the confirmed SurrealDB compile error introduced by the integration: text called a length-taking JSON helper that had become a property. Restored that helper, retaining valid display truncation and complete export text.

Validation for these fixes:

  • Full swift test --package-path Packages/TableProCore: passed (1,197 Swift Testing tests plus XCTest).
  • SwiftLint on the changed Swift files: zero violations. The helper's documentation check still flags four pre-existing stale paths: TableProApp/rust-dameng, Contents/MacOS, Contents/Helpers, and /Applications/Xcode-beta.app.
  • Project regeneration: passed.
  • The three updated branches have identical SurrealDB sources and driver tests. An isolated SwiftPM build of all those plugin sources and the actual driver test file passed 50 tests in 8 suites on ci: retry test-product downloads and fix shared CI failures #3216. The identical corrected helper also passed an AllPlugins Xcode build earlier on fix(plugin-surrealdb): BETWEEN, REGEX and IS EMPTY filters become equality checks #3218.

GitHub Actions rerun after pushing these fixes.

@mintlify

mintlify Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated
TablePro 🟢 Ready View Preview Sep 30, 2026, 6:58 AM

💡 Tip: Enable Automations to automatically generate PRs for you.

Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
…e-forgets-table-settings

# Conflicts:
#	CHANGELOG.md
#	TablePro/Core/Utilities/SQL/SQLTokenCursor.swift

This branch was successfully deployed

1 active (outdated) deployment
staging - docs — 678bb413 Deployed Sep 30, 2026 by mintlify[bot]
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