Skip to content

fix(datagrid): resolve default sort before dispatching the first table query#1605

Merged
datlechin merged 2 commits into
mainfrom
fix/default-sort-first-load
Jun 6, 2026
Merged

fix(datagrid): resolve default sort before dispatching the first table query#1605
datlechin merged 2 commits into
mainfrom
fix/default-sort-first-load

Conversation

@datlechin
Copy link
Copy Markdown
Member

Fixes #1603

Problem

With Default row sort set to Primary key, the first table opened after launch loaded unsorted. Closing and reopening it, or opening any other table, sorted correctly. The 0.49.1 fix (#1604) did not cover the first open.

Root cause

The first-load gate consumed its one-shot flag before the async schema fetch finished. On a cold cache, while fetchColumns was on the network, two other dispatch paths (the lazy-load view task and the new-window setup path) saw the flag already set, skipped the gate, and executed the unsorted query first. A warm cache resolved synchronously, which is why every open after the first worked.

Fix

Refactor of the first-load flow instead of another patch on the flag:

  • All three table-open entry points (first tab, tab reuse, new window) funnel into one per-tab gated path that awaits schema and primary key metadata, resolves the default sort, bakes ORDER BY into the query, then dispatches once. The flag, the fire-and-forget resolve task, and the duplicated PK resolution are deleted.
  • New SchemaColumnStore replaces the plain dictionary cache: in-flight task dedup so concurrent callers await the same fetch instead of double-querying.
  • Fast path: when nothing depends on metadata (sort is None, no plugin force, no hidden columns) the first query dispatches immediately, keeping today's first-paint latency for the default setting.
  • Because the sort state is set before the first dispatch, the column header shows the sort indicator from the first data frame.
  • Settings: the Default row sort explanation moved from a hover tooltip to a visible grouped-form footer.

Behavior: composite PK sorts by all key columns in key order; tables and views without a PK load unsorted with no indicator and no error; a user header sort always wins over the default.

Tests

  • DefaultSortInitialQueryTests rewritten: the old tests pre-populated the cache, so they passed while the bug shipped. New tests cover the cold-cache first query, composite PK, no-PK, schema fetch failure, the None fast path, user-sort preservation, scoped columns, and deselection during the await.
  • New SchemaColumnStoreTests cover fetch dedup (including the concurrent double-trigger that modeled the original race), failure retry, and invalidation.

Notes for review

  • The buffer-to-effective-columns fallback in FilterCoordinator.rebuildTableQuery stays: SortColumn stores a column index, so the ORDER BY must be built against the same column list the sort was resolved against.
  • DefaultSortHint stays @Frozen: removing @Frozen from a Library Evolution enum is an ABI-breaking change and this fix does not need it. Candidate for the next breaking PluginKit bump.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5907dfaf46

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


let task = Task {
if let entry = await fetch() {
self.entries[key] = entry
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Prevent stale schema loads from repopulating the cache

When refreshTables() or teardown calls schemaColumns.removeAll(), this store cancels in-flight loads and clears entries, but any metadata fetch that does not promptly observe cancellation can still resume and write its old result here. That means a schema refresh can be followed by the previous fetchColumns result reappearing under the same key, so later default-sort or hidden-column logic can use stale column/PK metadata until another invalidation; the generation check only controls loads cleanup and does not guard the cache write.

Useful? React with 👍 / 👎.

@datlechin datlechin merged commit 54cf105 into main Jun 6, 2026
3 checks passed
@datlechin datlechin deleted the fix/default-sort-first-load branch June 6, 2026 15:43
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7899dd23e5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

return
}

let sql = tab.content.query
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve default-sort resolution for manual table runs

This direct dispatch path now skips the default-sort preparation that was removed just above, so any first table execution that does not come from lazyLoadCurrentTabIfNeeded()—for example pressing the editor Run/Cmd-Enter action on a restored or deferred table tab—uses the original SELECT ... LIMIT ... without resolving the app/plugin default sort or rebuilding the table query. The new prepareTableTabFirstLoad only runs from the lazy-load task, but runQuery() still routes table tabs here directly, so the first query can be unsorted despite default sorting being enabled.

Useful? React with 👍 / 👎.

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 broken

1 participant