Skip to content

fix(datagrid): track who chose the row order and let the sort direction be set - #2670

Open
datlechin wants to merge 1 commit into
mainfrom
fix/datagrid-row-sort
Open

fix(datagrid): track who chose the row order and let the sort direction be set#2670
datlechin wants to merge 1 commit into
mainfrom
fix/datagrid-row-sort

Conversation

@datlechin

Copy link
Copy Markdown
Member

Fixes #2665.

Three reported symptoms, one mechanism.

Root cause

SortState answered two different questions with one value: what order the rows are in, and what order the user asked for. The click cycle (HeaderSortCycle) and the header rendering (applySortIndicators) both read only its columns and ignored source entirely, whose single reader in the whole app was QueryTab.hasUserActiveSort. On top of that, direction was hard-coded ascending in two places: DefaultSortResolver for the app-applied default, and singleSortTransition for the first click on any column.

Measured, not assumed

Driven against the Chinook sample in a throwaway sandbox, with the SQL read out of the app's own query_history.db:

Setting Action SQL issued What the user sees
Primary key open Album ORDER BY \AlbumId` ASC` header already shows an active ascending marker
Primary key click AlbumId #1 ORDER BY \AlbumId` DESC` jumps past ascending
Primary key click AlbumId #2 (none) cleared, so ascending needs a third click
Engine order open Customer (none) unsorted
Engine order click CustomerId #1 ORDER BY \CustomerId` ASC` rows identical
Engine order click CustomerId #2 ORDER BY \CustomerId` DESC` finally visible

So the third symptom is not a swallowed click. The query really re-runs; ascending on a rowid table is the order already on screen.

Against stock AppKit (swiftc harness, real NSTableView, macOS 27.0 build 26A5406e):

  • sortDescriptorPrototype.ascending sets the first-click direction. DataGridColumnPool already set a prototype on every data column, hard-coded ascending: true, so AppKit has been running a second sort model underneath this one.
  • tableView.sortDescriptors reaches no accessibility client: every header cell's accessibilitySortDirection stayed .unknown through every change to it. setAccessibilitySortDirection is the real channel, and nothing called it, so the sorted column was silent to VoiceOver.
  • During an aborted header drag a mouseDragged override is called zero times, and column order and widths do not change, so all three of the old drag guards passed and the grid sorted. AppKit itself correctly declined to.

The fix

  • SortSource is three-valued: .unset, .defaultSort, .user. An empty sort the user chose is now a different value from a tab that has not decided, which is what makes Don't Sort stick. It persists on PersistedTab and on TabNavigationEntry; a file written before the key existed decodes to the behaviour it was written under.
  • New Sort direction setting, shipping Ascending. It feeds the three places that must agree: DefaultSortResolver, HeaderSortCycle, and the column's sortDescriptorPrototype, so AppKit's model stops diverging.
  • The default order draws a muted marker with no priority number, so a table nobody sorted does not look sorted.
  • The header is drawn from the model, never painted ahead of it.
  • Accessibility publishes the sorted column through setAccessibilitySortDirection, cleared on the rest, and the descriptor mirror writes the whole list rather than only its leading entry.

Collateral fixed in the same change

Found while investigating, each verified against the code before it was fixed:

  • Don't Sort undone by the default row sort on the next load (this is what forced the model change).
  • An aborted column-header drag firing a sort.
  • A shift-clicked second sort column lost when the tab was reused for another table.
  • A saved sort destroyed on relaunch when its column was hidden.
  • The sort marker moving onto another column after a query returned different columns.
  • A sort chevron left behind after Cancel on Discard Unsaved Changes.
  • Move Column Up and Down dimmed on the Structure tab after Don't Sort, from a column: -1 descriptor.
  • MCP browse_table losing its ORDER BY when the sort column was outside columns. The missing sort column is appended, so every requested column keeps the position it was asked for; docs/external-api/mcp-tools.mdx records that.

Verification

  • build: PASS.
  • swiftlint --strict: clean on the app target and on both test targets.
  • docs: check-writing-style.sh and check-docs-against-source.py both pass.
  • Unit suites: not re-run after the review fixes. testmanagerd on this machine has been wedged since 27 Aug ("The test runner hung before establishing connection"), every run returns 0 cases executed, and SIP refuses launchctl kickstart, so it needs a logout to clear. The same 25 suites ran 224/225 before the review fixes, the one failure being a bad fixture in a test added here, since rebuilt from a real encoder round-trip. CI runs them on this PR.
  • HeaderSortUITests is new and has not been executed locally for the same reason.

CLAUDE.md's header invariant claimed sortDescriptors was published "for accessibility". That half was wrong and is corrected.

https://claude.ai/code/session_01CCwzDmzDy9SW2DT5peRhAm

@mintlify

mintlify Bot commented Sep 7, 2026

Copy link
Copy Markdown

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

Project Status Preview Updated
TablePro 🟢 Ready View Preview Sep 7, 2026, 8:47 PM

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

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.

Default row sort to DESC

1 participant