fix(datagrid): keep a column order whose columns have not changed - #2461
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 whosetabType != .table:TableStructureViewbuilds itsDataGridConfigurationwith notabType, so the Structure grid takes that branch. MeanwhilecaptureColumnLayout()recordscolumnOrderfor every grid andtableViewColumnDidMovepersists it, so the reorder is written and then thrown away on the way back in.DataGridColumnPool.computeTargetOrderreceivesniland returns the natural slot order, andattachAndOrderColumnsmoves 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 orderstoselect id, state, business_model from ordersput the newstatecolumn at the far right rather than between the other two, becausecomputeTargetOrderappends 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:
identitySchemais rebuilt from the current rows immediately before both production call sites, so it is the live column set.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..tabletabs 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"], butColumnIdentitySchema.slotByColumnNamekeeps the last occurrence of a duplicate name, socomputeTargetOrderresolves 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:
.tablegrids 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
testPASS, 86 cases across the four suites that own column layout and identity, and 161 across ten on the first commit. 0 failed.lint0 SwiftLint violations. (The wrapper'sagent docsstep reports one pre-existing stale symbol,CLAUDE.md:216 AXCell, in a file this branch does not touch.)queryTabDropsStaleColumnOrdernow rebuilds a three-column schema so it exercises a genuinely stale order rather than an empty one, which is what its name always claimed.queryTabDropsStaleColumnOrderstill 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.