feat(datagrid): reorder table columns by dragging on every engine that can, and say why on the ones that cannot - #2586
Conversation
…an reorder, and add ClickHouse and Oracle
… SQLite, libSQL, Turso and Cloudflare D1
|
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. |
… copy an identity column into the rebuilt table
…use to rebuild a virtual table
Codex review: no-ship, 21 findingsMarked draft. A cold review by Codex (GPT-5, reviewing the diff at commits 1 and 2) returned:
Five of its findings were already fixed in commits 3 to 5 (generated MySQL columns, filtered or sorted lists, FTS5 tables, P1, open
P2, open
Full report: Codex session |
…cross-engines # Conflicts: # CHANGELOG.md # TablePro/Core/Plugins/PluginMetadataRegistry+RegistryDefaults.swift
…it once, and stop the rebuild losing table state
All Codex findings addressedEvery open finding is fixed, each one reproduced against a live server first where behaviour was in question. The three that changed the designScope. 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 Transactions belong to whoever runs the plan. Plans now carry DDL and nothing else, plus a Reproduced, then fixed
Stale plan after review. A rebuild is planned before its sheet opens and run after it closes, and it ends in a Oracle compensation. A cycle whose One I did not fix the suggested wayThe finding on stale plugin binaries advertising reorder proposed gating the curated capability on a plugin implementation or version. VerificationBuild PASS, 54 cases across four suites PASS, lint clean, docs PASS, ABI still additive with zero symbol removals. 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; |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.rowDragtype,validateDrop's.moveandacceptDrop'struewere all keyed ondelegate != nil, which is every grid in the app. The engine gate (supportsColumnReorder, true for MySQL and MariaDB only) decided nothing but whetherStructureGridDelegate.moveRowHandlerwas non-nil, and that was read after the drop had been accepted, asmoveRowHandler?(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[]fromvalidateDropand not registering the type, was never used. The results grid had the same defect.Underneath that, reorder was single-shape:
PluginDatabaseDriver.generateMoveColumnSQLreturns one positionalString?. 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.
generateColumnReorderPlan(table:schema:columns:desiredOrder:), returning aPluginColumnReorderPlanof statements, a rollback, a cost, its caveats and whether TablePro may run it.generateMoveColumnSQLstays published and defaulted; removing a requirement breaks every plugin whose witness table references its default.PluginMetadataSnapshot.supportsColumnReorder: BoolbecomescolumnReorder: ColumnReorderSupport(.alter/.rebuild/.unsupported).ColumnReorderPolicyis 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.rowReorderreplaces the inferredhasMoveDelegate. 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.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
MODIFY COLUMN … FIRST | AFTER(already shipped, ported to the plan)MODIFY COLUMN name type FIRST | AFTERMODIFY (col INVISIBLE)thenMODIFY (col VISIBLE)per column that must moveCREATE TABLEtextWhat was measured, not assumed
Every engine claim here was probed against a live server this session.
ClickHouse 26.8.
MODIFY COLUMN c AFTER bwithout a type is a syntax error; the type is mandatory. With it, the statement is metadata only (zero rows insystem.mutations) and preserves the default, comment,CODEC,TTLand theMATERIALIZED,ALIASandEPHEMERALkinds. So the plan emits type and position only. Reusing the plugin's full column renderer would have rewritten aMATERIALIZEDcolumn as aDEFAULTone and requoted every expression default.Logengine rejectsMODIFY_COLUMNoutright 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 TABLEprocedure. The new table is written by moving the original column definitions as text inside the statementsqlite_masterstored, so aCHECK, aCOLLATE, aGENERATED ALWAYS AS, aDEFAULT 'hi, there'and aDECIMAL(10,2)all come through untouched. Re-rendering fromPRAGMA table_infoloses every one of them. Verified with an index, a trigger, an outbound foreign key and two dependent views; noPRAGMA legacy_alter_tableis 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:
CREATE INDEXfailed withrelation "ix_x_b" already exists, becauseALTER TABLE … RENAMEdoes not rename the table's indexes and the staging table still owned every name.x_pkey1,x_a_b_key1andx_c_check1. Declared inline in theCREATE TABLEwhile 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_fkeyandchild_xa_fkeyall 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.
generateMoveColumnSQLrestated the column withmysqlColumnAttributesSQL, which does not emitGENERATED ALWAYS AS.MODIFYreplaces the whole definition, so the drag turned a generated column into a plain one holding stored defaults. It now uses the same builderADD COLUMNuses. Pre-existing on MySQL and MariaDB.dataGridMoveRowis the one delegate method that did not map its display row throughsourceRow(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.sqlite_masterstores one asCREATE 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 theDROP. The parser now accepts only an ordinaryCREATE TABLE.GENERATED ALWAYS AS IDENTITYcolumn failed the PostgreSQL copy with "cannot insert a non-DEFAULT value into column". The copy now carriesOVERRIDING SYSTEM VALUE, which is measured to be accepted and inert on aBY DEFAULTidentity and on a table with no identity at all.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 queriespg_dependand names those views in the caveats, so it is known before anything runs rather than discovered from an error.Verification
verify.sh buildverify.sh test(7 suites, 49 cases, including an exhaustive pass over all 120 permutations of five columns)verify.sh lint(app, 7 plugins, PluginKit, tests)verify.sh docsverify.sh abi mainverify.sh pluginsoracle-nio's@TaskLocalmacro, a known local-toolchain issue unrelated to this change. libSQL and Cloudflare D1 compiled; Oracle's compile is left to CIThe 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 oneALTERper 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
IDENTITY(seed,increment), computed columns andSET 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.NSViewhelp tag on the row number and the existingSQLReviewSheet, 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.ColumnReorderPolicyTests.