Skip to content

feat(datagrid): reorder table columns by dragging on every engine that can, and say why on the ones that cannot - #2586

Merged
datlechin merged 7 commits into
mainfrom
feat/column-reorder-across-engines
Aug 31, 2026
Merged

feat(datagrid): reorder table columns by dragging on every engine that can, and say why on the ones that cannot#2586
datlechin merged 7 commits into
mainfrom
feat/column-reorder-across-engines

Conversation

@datlechin

@datlechin datlechin commented Aug 31, 2026

Copy link
Copy Markdown
Member

Fixes #2479.

The bug

Dragging a column row in the Structure tab did nothing on 30 of the 32 engines, and said nothing about why.

The cause is that the grid offered the drag unconditionally. Registration of the com.TablePro.rowDrag type, validateDrop's .move and acceptDrop's true were all keyed on delegate != nil, which is every grid in the app. The engine gate (supportsColumnReorder, true for MySQL and MariaDB only) decided nothing but whether StructureGridDelegate.moveRowHandler was non-nil, and that was read after the drop had been accepted, as moveRowHandler?(source, destination). An optional call on nil evaluates to nothing, so the row lifted, the insertion gap opened, the drop was taken and nothing happened. AppKit's documented way to say "this row cannot move", returning [] from validateDrop and not registering the type, was never used. The results grid had the same defect.

Underneath that, reorder was single-shape: PluginDatabaseDriver.generateMoveColumnSQL returns one positional String?. That fits MySQL and MariaDB, would fit ClickHouse, and cannot express what Oracle, PostgreSQL or SQLite need.

The fix

Reorder becomes an engine-declared capability with a driver-generated statement plan.

  • PluginKit gains generateColumnReorderPlan(table:schema:columns:desiredOrder:), returning a PluginColumnReorderPlan of statements, a rollback, a cost, its caveats and whether TablePro may run it. generateMoveColumnSQL stays published and defaulted; removing a requirement breaks every plugin whose witness table references its default.
  • PluginMetadataSnapshot.supportsColumnReorder: Bool becomes columnReorder: ColumnReorderSupport (.alter / .rebuild / .unsupported).
  • ColumnReorderPolicy is the single pure answer to "may this column be dragged, and if not why not". The grid reads it to decide whether to offer the drag; the delegate reads it to decide whether there is a handler behind it, so the two cannot disagree.
  • DataGridView.rowReorder replaces the inferred hasMoveDelegate. Where reordering is impossible the row does not lift, and the row number carries a help tag with the reason. Dragging a row out to another app still works: the text and HTML pasteboard flavours are written either way.
  • The consent line is what the statements cost. A reorder that only rewrites the catalog runs on the drop, as MySQL already did. One that copies rows and drops the old table goes through the existing SQLReviewSheet, with its caveats and either a destructive Rebuild Table button or, where the engine's catalog cannot fully describe its own table, Open in Query Editor and no run button at all.

Engines

Engine Mechanism Runs on the drop
MySQL, MariaDB MODIFY COLUMN … FIRST | AFTER (already shipped, ported to the plan) yes
ClickHouse MODIFY COLUMN name type FIRST | AFTER yes
Oracle MODIFY (col INVISIBLE) then MODIFY (col VISIBLE) per column that must move yes
SQLite table rebuild from the stored CREATE TABLE text after review
PostgreSQL, libSQL, Turso, Cloudflare D1 table rebuild script no, the script is handed over

What was measured, not assumed

Every engine claim here was probed against a live server this session.

ClickHouse 26.8. MODIFY COLUMN c AFTER b without a type is a syntax error; the type is mandatory. With it, the statement is metadata only (zero rows in system.mutations) and preserves the default, comment, CODEC, TTL and the MATERIALIZED, ALIAS and EPHEMERAL kinds. So the plan emits type and position only. Reusing the plugin's full column renderer would have rewritten a MATERIALIZED column as a DEFAULT one and requoted every expression default. Log engine rejects MODIFY_COLUMN outright and reports it.

Oracle Free 23. The invisible/visible cycle moves a column to the end of the visible order, and composing it over the right suffix reaches any order. Verified on the primary key, an identity column and a virtual column; rows, defaults, NOT NULL, comments, the identity sequence, constraints, indexes and an inbound foreign key all survived, with no data movement. Needs 12.1.

SQLite 3.54. The rebuild follows SQLite's own documented ALTER TABLE procedure. The new table is written by moving the original column definitions as text inside the statement sqlite_master stored, so a CHECK, a COLLATE, a GENERATED ALWAYS AS, a DEFAULT 'hi, there' and a DECIMAL(10,2) all come through untouched. Re-rendering from PRAGMA table_info loses every one of them. Verified with an index, a trigger, an outbound foreign key and two dependent views; no PRAGMA legacy_alter_table is needed for the rename to pass the views.

PostgreSQL 17. Two real defects in my own script came out of running it, and both are fixed:

  1. CREATE INDEX failed with relation "ix_x_b" already exists, because ALTER TABLE … RENAME does not rename the table's indexes and the staging table still owned every name.
  2. Constraints came back as x_pkey1, x_a_b_key1 and x_c_check1. Declared inline in the CREATE TABLE while the staging table still held the originals, PostgreSQL silently picks another name.

So nothing that carries a name is created until the staging table is dropped, and the staging table cannot be dropped until every inbound foreign key has let go. The corrected script was re-run end to end: order changed, rows and the generated column intact, identity resequenced so the next insert does not collide, and x_pkey, x_a_b_key, x_c_check, x_pid_fkey and child_xa_fkey all keeping their original names.

Found by review, fixed here

Five more defects came out of the review pass, four of them destructive. Each was reproduced before being fixed.

  1. A generated MySQL column stopped being generated when it was moved. generateMoveColumnSQL restated the column with mysqlColumnAttributesSQL, which does not emit GENERATED ALWAYS AS. MODIFY replaces the whole definition, so the drag turned a generated column into a plain one holding stored defaults. It now uses the same builder ADD COLUMN uses. Pre-existing on MySQL and MariaDB.
  2. A dragged column was resolved against the wrong list when the column list was filtered or sorted. dataGridMoveRow is the one delegate method that did not map its display row through sourceRow(for:), so a drop's position named a different column than the user dragged. Mapping it back is not the fix, because a wanted order is a statement about every column and a filtered list is not showing every column, so the drag is now withheld with the reason. Pre-existing, and previously reachable only on MySQL and MariaDB.
  3. An FTS5 table would have been destroyed. sqlite_master stores one as CREATE VIRTUAL TABLE docs USING fts5(title, body), whose parentheses parse exactly like a column list. The rebuild would have recreated it as a plain table and taken the index and its shadow tables down with the DROP. The parser now accepts only an ordinary CREATE TABLE.
  4. A GENERATED ALWAYS AS IDENTITY column failed the PostgreSQL copy with "cannot insert a non-DEFAULT value into column". The copy now carries OVERRIDING SYSTEM VALUE, which is measured to be accepted and inert on a BY DEFAULT identity and on a table with no identity at all.
  5. A dependent view stopped the PostgreSQL rebuild at DROP TABLE, three quarters of the way through, because PostgreSQL binds a view to the table's OID and the view follows the rename onto the staging table. The plan now queries pg_depend and names those views in the caveats, so it is known before anything runs rather than discovered from an error.

Verification

Step Result
verify.sh build PASS
verify.sh test (7 suites, 49 cases, including an exhaustive pass over all 120 permutations of five columns) PASS
verify.sh lint (app, 7 plugins, PluginKit, tests) clean
verify.sh docs PASS
verify.sh abi main additive, zero symbol removals, no version bump
verify.sh plugins fails inside vendored oracle-nio's @TaskLocal macro, a known local-toolchain issue unrelated to this change. libSQL and Cloudflare D1 compiled; Oracle's compile is left to CI

The test suites include DataGridUpdateSnapshotTests, which owns a type this change renamed. One defect was caught by the tests themselves and fixed in the source: a downward drag emitted one ALTER per column it passed instead of one for the column dragged, because a greedy left-to-right walk is not minimal. The stationary set is now the longest subsequence common to both orders, and an exhaustive test walks every permutation of five columns through both mechanisms, checking the result and the move count against an independently computed minimum.

Not in this change

  • SQL Server. Its catalog does expose everything a rebuild needs, but reconstructing it needs a type/length/precision renderer, IDENTITY(seed,increment), computed columns and SET IDENTITY_INSERT, and I did not probe an end-to-end rebuild for it. Shipping a fourth dialect unmeasured beside three measured ones is not worth it; the machinery is now in place, so it is a contained follow-up.
  • Screenshots. The two visible surfaces are a standard NSView help tag on the row number and the existing SQLReviewSheet, which is unchanged. Capturing before-and-after would mean driving a sandboxed build to a Structure tab on a live connection per engine, which I did not do.
  • UI automation. The flow starts with a native drag inside the data grid, which XCUITest cannot reach: a grid row is reported as obscured by its own column elements, so a test clicks a point offset from the grid rather than a row. The pure decision behind the affordance is covered instead, by ColumnReorderPolicyTests.

@mintlify

mintlify Bot commented Aug 31, 2026

Copy link
Copy Markdown

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

Project Status Preview Updated (UTC)
TablePro 🟢 Ready View Preview Aug 31, 2026, 12:21 PM

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

@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 marked this pull request as draft August 31, 2026 13:13
@datlechin

Copy link
Copy Markdown
Member Author

Codex review: no-ship, 21 findings

Marked draft. A cold review by Codex (GPT-5, reviewing the diff at commits 1 and 2) returned:

The new reorder paths can operate on the wrong database, execute stale or partially authorized rebuild plans, and silently lose identity, generated-column, constraint, trigger, or collation semantics.

Five of its findings were already fixed in commits 3 to 5 (generated MySQL columns, filtered or sorted lists, FTS5 tables, OVERRIDING SYSTEM VALUE, dependent views). The rest are open and recorded here.

P1, open

# Finding Where
1 Plan and execute both resolve the ambient session driver, so a tab pinned to database A can plan and run against B after another tab moves the connection StructureColumnReorderHandler.swift
2 Open in Query Editor uses browseDatabaseName, not the tab's database, so an unqualified rebuild script can open against the wrong one MainContentCoordinator+SQLPreview.swift
3 A reviewed SQLite plan is captured before the sheet opens and executed after, so a schema change in between is silently dropped with the old table TableStructureView+ColumnReorder.swift
4 Each statement is authorized separately, so under Safe Mode a user can approve through COMMIT and cancel after it; rollback is then too late but the handler reports failure StructureColumnReorderHandler.swift
5 An Oracle cycle whose VISIBLE fails leaves the column invisible for good, and Oracle DDL cannot be rolled back OraclePlugin.swift
6 The rollback is unconditional, so a failure on a session that already had a user BEGIN open discards their unrelated uncommitted work StructureColumnReorderHandler.swift
7 A SQLite AUTOINCREMENT table loses its sqlite_sequence high-water mark with the old table, so a previously issued id can be reused SQLiteColumnReorderPlanner.swift
8 A PostgreSQL serial column's recreated default still depends on the staging table's sequence, so DROP TABLE refuses and the whole script rolls back PostgreSQLPluginDriver+ColumnReorder.swift
9 PostgreSQL 18 virtual generated columns (attgenerated = 'v') get no generated clause and are excluded from the copy, so the expression is lost PostgreSQLPluginDriver+ColumnReorder.swift
10 Exclusion constraints (contype = 'x') are in neither the constraint list nor the index list, so the guarantee is silently removed PostgreSQLPluginDriver+ColumnReorder.swift

P2, open

  • SQLite's foreign_keys pragma is forced ON afterwards rather than restored to what it was.
  • The Open in Editor handoff is not atomic: the query executor's own transaction makes the script's BEGIN nested, and remote libSQL and D1 cannot span statements at all.
  • Local-file libSQL supports transactions and should get the runnable path, not the editor one.
  • libSQL, Turso and D1 read columns through PRAGMA table_info, which omits generated columns, so desiredOrder never matches and every drag returns nil.
  • An already-installed pre-change Oracle, libSQL or D1 binary keeps the curated capability while the new protocol method resolves to its nil default, so the drag is offered and always fails.
  • A self-referencing foreign key appears in both the inbound and outbound lists; the inbound drop names a table that no longer holds it.
  • pg_get_triggerdef does not carry tgenabled, so a disabled or replica-only trigger comes back ordinarily enabled.
  • The pg_get_serial_sequence literal is built without escaping apostrophes in the qualified name.
  • mysqlColumnDefinitionSQL's generated branch omits charset and collation, so the fix in commit 3 is incomplete for a generated string column.
  • Structure opened on a view is offered the drag, and every planner emits table DDL.

Full report: Codex session 01a057b7-babd-7083-8778-38b0bf4b58de.

…cross-engines

# Conflicts:
#	CHANGELOG.md
#	TablePro/Core/Plugins/PluginMetadataRegistry+RegistryDefaults.swift
…it once, and stop the rebuild losing table state
@datlechin

Copy link
Copy Markdown
Member Author

All Codex findings addressed

Every open finding is fixed, each one reproduced against a live server first where behaviour was in question.

The three that changed the design

Scope. prepare and execute both run through DatabaseManager.withScopedDriver on the structure tab's own DatabaseScope, planning under .untracked and executing under a .protectedWrite lease, the same shape executeSchemaChanges uses. Open in Query Editor opens on the plan's scope too, carried on the request, so an unqualified script cannot land on a same-named table in whatever database the connection drifted to.

Authorization. Once, for the whole plan, before any statement runs, and outside the scoped block so a confirmation sheet does not hold the connection's driver. Per-statement was not merely slow: a user could approve through a rebuild's COMMIT and decline the statement after it, with nothing left to refuse.

Transactions belong to whoever runs the plan. Plans now carry DDL and nothing else, plus a prologue/epilogue for statements that cannot sit inside a transaction. Both runners already open one: TablePro's own path through beginTransaction, and the query editor's Run All, which wraps a multi-statement script exactly the same way. A plan that spelled BEGIN out in SQL would nest inside theirs. That also means only the transaction this plan opened is ever rolled back, so a failure no longer discards a transaction the user had open on the same session.

Reproduced, then fixed

Finding What the probe showed
PostgreSQL serial blocked the drop DROP TABLE refused: "default value for column a depends on sequence x_a_seq". ALTER SEQUENCE … OWNED BY before the drop fixes it, and the sequence keeps its original name
SQLite AUTOINCREMENT reused an id High-water mark fell 3 → 2 and the next insert reused 3, already issued to a deleted row. sqlite_sequence is now captured and restored
MySQL generated column The old statement errored outright: "'Changing the STORED status' is not supported for generated columns". Charset and collation now precede GENERATED ALWAYS AS, verified moving a utf8mb4_bin virtual column on MySQL 8.4
Exclusion constraints contype = 'x' was in neither list. Now recreated by name, and verified still enforcing after the rebuild
Self-referencing foreign key Appeared in both inbound and outbound; the inbound drop named a table that no longer held it. Excluded by conrelid <> confrelid
Trigger enable modes pg_get_triggerdef carries no tgenabled, so a disabled trigger came back firing. Modes are now re-emitted; verified t_off=D survives
PostgreSQL 18 virtual generated columns attgenerated = 'v' produced no clause and was excluded from the copy. Now emits VIRTUAL
SQLite foreign_keys Forced ON afterwards. Now read before and restored to what it was
libSQL / Turso / D1 generated columns PRAGMA table_info omits them, so the orders never matched and every drag returned nil. All three now use table_xinfo
Local-file libSQL Held a real transaction but was forced down the script-only path. isRunnable now follows the mode
Views Offered the drag, then failed on table DDL. Withheld with a reason
pg_get_serial_sequence literal Built without escaping apostrophes in the qualified name. Escaped

Stale plan after review. A rebuild is planned before its sheet opens and run after it closes, and it ends in a DROP. Drivers now answer columnReorderSchemaFingerprint, taken with the plan and again inside the execution lease; a mismatch refuses the run and says so rather than dropping a table that grew a column while the sheet was open.

Oracle compensation. A cycle whose VISIBLE half fails leaves a column hidden, and Oracle commits each DDL on its own. Every cycled column now carries a compensating VISIBLE that the executor runs on a mid-plan failure.

One I did not fix the suggested way

The finding on stale plugin binaries advertising reorder proposed gating the curated capability on a plugin implementation or version. PluginMetadataRegistry records, at the site the finding points at, that reading a new DriverPlugin static from a stale binary crashes with EXC_BAD_INSTRUCTION, which is what that gate would require. Rather than take that risk, the failure is made honest instead: a driver that returns no plan now reports that this engine's driver may predate column reorder and should be updated in Settings > Plugins, in place of a generic generation failure.

Verification

Build PASS, 54 cases across four suites PASS, lint clean, docs PASS, ABI still additive with zero symbol removals. verify.sh plugins fails only inside vendored oracle-nio's @TaskLocal macro, a known local-toolchain issue; libSQL and Cloudflare D1 both compiled.

The full PostgreSQL rebuild was re-run end to end against 17 on a table carrying a serial primary key, a stored generated column, a check, a unique, an exclusion constraint, an inbound foreign key, a self-reference, a partial index, a disabled trigger and a comment. Order changed; rows, generated value and sequence name intact; all six constraints back under their original names with the exclusion constraint still rejecting a conflict; t_off still disabled; staging table gone.

@datlechin
datlechin marked this pull request as ready for review August 31, 2026 17:18
@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 38802cb into main Aug 31, 2026
14 checks passed
@datlechin
datlechin deleted the feat/column-reorder-across-engines branch August 31, 2026 17:21
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.

Reorder table columns by dragging on engines other than MySQL and MariaDB

1 participant