Skip to content

fix(datagrid): keep a column order whose columns have not changed - #2461

Merged
datlechin merged 3 commits into
mainfrom
fix/2449-column-order
Aug 26, 2026
Merged

fix(datagrid): keep a column order whose columns have not changed#2461
datlechin merged 3 commits into
mainfrom
fix/2449-column-order

Conversation

@datlechin

@datlechin datlechin commented Aug 26, 2026

Copy link
Copy Markdown
Member

Found while investigating #2449, and made visible by the fix that shipped there (#2455): reordering a column in the Structure tab now repaints correctly, and then silently snaps back.

The bug

Drag a column header in the Structure tab, or in a query result. The move lands and is written to the session's column layouts. On the next update, a keystroke in the Structure filter, a cell edit, a refresh, a re-run of the same query, the columns jump back to their original order with no explanation.

Root cause

savedColumnLayout(binding:) drops the order outright for every grid whose tabType != .table:

guard tabType == .table else {
    ...
    layout.columnOrder = nil
    return layout
}

TableStructureView builds its DataGridConfiguration with no tabType, so the Structure grid takes that branch. Meanwhile captureColumnLayout() records columnOrder for every grid and tableViewColumnDidMove persists it, so the reorder is written and then thrown away on the way back in. DataGridColumnPool.computeTargetOrder receives nil and returns the natural slot order, and attachAndOrderColumns moves the columns back.

That nil-ing is not arbitrary. It came from #1565 via e582a8b: in a SQL tab, going from select id, business_model from orders to select id, state, business_model from orders put the new state column at the far right rather than between the other two, because computeTargetOrder appends every schema slot the saved order does not name. Dropping the order fixed that, and also threw away orders the columns had never moved under.

The fix

The order is unsafe when the columns moved under it, not when the grid has no table behind it. So the sets are compared instead:

if let order = layout.columnOrder, Set(order) != Set(identitySchema.columnNames) {
    layout.columnOrder = nil
}

identitySchema is rebuilt from the current rows immediately before both production call sites, so it is the live column set.

  • Adding or removing a column in the SELECT list changes the set, the saved order is dropped, and the columns arrive in SELECT order. New columns appear at end #1565 stays fixed, and its regression test still passes unchanged in substance.
  • Re-running the same query, and every update of the Structure grid whose columns never change, keeps the set, so a reorder survives.

The emptiness guard moved below that check, because a reorder with no width change is the entire layout and used to fall through it and come back as nil.

.table tabs are untouched: they read from the persister, keyed per table, and keep today's behaviour.

Duplicate column names, found by review

Codex caught a hole in the first version of that rule. A set comparison accepts SELECT a.id, b.id, whose captured order is ["id", "id"], but ColumnIdentitySchema.slotByColumnName keeps the last occurrence of a duplicate name, so computeTargetOrder resolves both entries to slot 1, dedupes them, then appends slot 0: the two columns silently swap on the next update.

A saved order is a list of names, and a name identifies a column only while the names are unique, so the order is now dropped unless the current column names are unique as well as matching.

Scope

Deliberately not changed: .table grids still replay a saved order across a schema change, appending a newly added column at the end rather than discarding a layout the user built on purpose. That is the same trade-off #1565 names, decided the other way, and it is the right one where the order is persisted per table and deliberate.

Verification

  • test PASS, 86 cases across the four suites that own column layout and identity, and 161 across ten on the first commit. 0 failed.
  • lint 0 SwiftLint violations. (The wrapper's agent docs step reports one pre-existing stale symbol, CLAUDE.md:216 AXCell, in a file this branch does not touch.)
  • queryTabDropsStaleColumnOrder now rebuilds a three-column schema so it exercises a genuinely stale order rather than an empty one, which is what its name always claimed.
  • Three new cases: a query tab keeps an order saved for the same columns, keeps an order-only layout with no widths in it, and drops an order when two columns share a name.
  • Negative controls, one per rule: with the unconditional nil-ing restored, both preservation cases fail and queryTabDropsStaleColumnOrder still passes, so the tests separate the two behaviours rather than just pinning the new one. With the uniqueness half of the check removed, the duplicate-name case fails on its own.

No UI automation: the flow needs a live connection and a header drag, which does not run deterministically in TableProUITests.

@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 cc17e34 into main Aug 26, 2026
@datlechin
datlechin deleted the fix/2449-column-order branch August 26, 2026 14:53
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