Skip to content

fix(react,row-model): external sort authority actually suppresses local sorting - #467

Merged
blove merged 1 commit into
mainfrom
blove/external-sort-authority
Aug 17, 2026
Merged

fix(react,row-model): external sort authority actually suppresses local sorting#467
blove merged 1 commit into
mainfrom
blove/external-sort-authority

Conversation

@blove

@blove blove commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Supersedes #466, which GitHub closed automatically when its stacked base branch was deleted on merge. Same change, rebased onto main after #464 and #465.

processing: { sort: "external" } declares that something outside the grid chose the order of the records it was handed. The engine went on applying query.sort anyway.

The case, since #447 asked for it to be made

#447 deferred this on purpose, and the stated reason was that the case looked weaker than filtering's: a consumer with a complete window who sorts locally is doing something reasonable.

That objection dissolves once you ask who it protects. That consumer declares "engine" — the default. Suppression never binds them. It binds only the consumer who explicitly said the server owns ordering, and for that consumer the old behaviour was worse than doing nothing at all.

Measured before changing anything: sort: "external" was read in exactly two places, both advisory, both in data-scope.ts. The row model had a filterAuthority and no sort equivalent, so the declaration could not reach evaluation. What it did reach was:

  • warnOnEngineSortOverPartialWindow, which fires only when sort authority is not external. Declaring it silenced the one warning that says a local sort over a partial window "presents the wrong SAMPLE, not just the wrong order" — while the sort it was warning about kept running.
  • resolveAriaRowCount, which requires it to publish the population count. The same declaration therefore asserted that every loaded row sits at its true dataset position — about rows the engine had just reordered.

So the declaration bought a false positional claim and cost a true warning. external-sort-authority.test.tsx pins both halves as they now stand: the warning still fires under engine authority, where it is still the right thing to say, and the population count is now honest because the rows are no longer reordered.

The seam is the one #447 built

CompiledQueryPlan keeps #publicQuery (read by get query() → snapshot, header, onQueryChange) separate from #runtimeQuery (read by evaluate()). Under external sort authority the runtime query gets sort: [] while the public one keeps it.

Only query.sort is dropped. rowGroups keeps its own ordering, because grouping is not in PretableProcessingOptions and declaring external sort said nothing about it.

Suppression changes what is APPLIED, never what is REPORTEDaria-sort still announces the column, onQueryChange still publishes the sort, the snapshot still reports it. That guardrail has its own test.

It reaches the model the same two ways filter authority does, and for the same reasons: an @internal construction argument (the initial store is built inside createLocalRowModel, so a model created under engine authority would paint one locally-sorted frame first), plus a WeakMap-registry setter for later flips, guarded on ownedModel so a consumer's own model is never touched. react.api.md is unchanged; core.api.md gains one @internal ɵ-prefixed line.

Verification

  • The motivating test written first and watched fail: expected [ 'r2', 'r3', 'r5', 'r1', 'r6', 'r4' ] to deeply equal [ 'r1' … 'r6' ].
  • Independently re-verified by neutralising the suppression in canonicalRuntimeQuery: 2 of 7 tests fail, and restoring it fixes them.
  • The fixture carries its own controls — the two orders are proven distinct permutations of each other, and engine authority is proven to still reorder — so no assertion can pass by coincidence.
  • One existing test changed on purpose. fix(react,row-model): external filter authority actually suppresses local filtering #447's "publishes a query change through onQueryChange unchanged" asserted a locally-sorted body under EXTERNAL, which is exactly the behaviour this removes. Its reporting assertions — the test's actual subject — are untouched.
  • Repo-wide pnpm test green (react 1234, website 556, bench 156, renderer-dom 127), plus typecheck, lint, format, build and api:check.

Docs

The old behaviour was documented as deliberate in four places, and a reader following them would now be misled — the trap #449 was written to close.

query-ownership loses its asymmetry section and its "suppresses nothing" bullet; index and pretable-surface lose the same claim; lifecycle no longer says a header stays sortable through an error — it stays interactive and publishes the sort, but the rows keep the order the last successful response gave them. The renamed heading's anchor is updated at both inbound links.

🤖 Generated with Claude Code

…al sorting

`processing: { sort: "external" }` declares that something outside the grid
chose the order of the records it was handed. The engine went on applying
`query.sort` anyway.

#447 deferred this deliberately, because the case looked weaker than
filtering's: a consumer holding a complete window who sorts locally is being
reasonable. That objection dissolves on inspection — that consumer declares
`"engine"`, which is the default, so suppression never binds them. It binds
only the consumer who said the server owns ordering, and for that consumer
the previous behaviour was worse than doing nothing.

Measured before changing anything, `sort: "external"` was read in exactly two
places, both advisory, both in `data-scope.ts`. The row model had a
`filterAuthority` and no sort equivalent, so the declaration could not reach
evaluation at all. What it did reach was:

  - `warnOnEngineSortOverPartialWindow`, which fires only when sort authority
    is NOT external. Declaring it SILENCED the one warning that says a local
    sort over a partial window "presents the wrong SAMPLE, not just the wrong
    order" -- while the local sort it was warning about kept running.
  - `resolveAriaRowCount`, which requires it to publish the population count.
    So the same declaration asserted that every loaded row sits at its true
    dataset position, about rows the engine had just reordered.

So the declaration bought a false positional claim and cost a true warning.
`external-sort-authority.test.tsx` pins all of it, including both halves as
they now stand: the warning still fires under engine authority, and the
population count is now honest because the rows are no longer reordered.

The seam is the one #447 built. `CompiledQueryPlan` keeps `#publicQuery` (read
by `get query()` -> snapshot, header, `onQueryChange`) separate from
`#runtimeQuery` (read by `evaluate()`); under external sort authority the
runtime query gets `sort: []` while the public one keeps it. Only `query.sort`
is dropped -- `rowGroups` keeps its own ordering, because grouping is not in
`PretableProcessingOptions` and declaring external sort said nothing about it.

Suppression changes what is APPLIED, never what is REPORTED: `aria-sort` still
announces the column, `onQueryChange` still publishes the sort, the snapshot
still reports it. That guardrail has its own test.

Reached the model the same two ways filter authority is, and for the same
reasons: an `@internal` construction argument (the initial store is built
inside `createLocalRowModel`, so a model created under engine authority would
paint one locally-sorted frame first) plus a WeakMap-registry setter for later
flips, guarded on `ownedModel` so a consumer's own model is never touched.
`react.api.md` is unchanged; `core.api.md` gains one `@internal` ɵ-prefixed
line.

Verification:

  - The motivating test written first and watched fail:
    `expected [ 'r2', 'r3', 'r5', 'r1', 'r6', 'r4' ] to deeply equal
    [ 'r1' ... 'r6' ]`.
  - Independently re-verified by neutralising the suppression in
    `canonicalRuntimeQuery`: 2 of 7 tests fail, and restoring it fixes them.
  - The fixture carries its own controls -- the two orders are proven to be
    distinct permutations, and engine authority is proven to still reorder,
    so none of the assertions can pass by coincidence.
  - One existing test changed on purpose. #447's "publishes a query change
    through onQueryChange unchanged" asserted a locally-sorted body under
    `EXTERNAL`, which is the behaviour this commit removes; its reporting
    assertions -- the test's actual subject -- are untouched.
  - Repo-wide `pnpm test` green (react 1223, website 554, row-model 327,
    renderer-dom 127, grid-core 124, bench 155), plus typecheck, lint,
    format, build and `api:check`.

Docs: the previous behaviour was documented as deliberate in four places, and
a reader following them would now be misled. `query-ownership` loses its
asymmetry section, `index` and `pretable-surface` lose the claim that sort
"suppresses nothing", and `lifecycle` no longer says a header stays sortable
through an error -- it stays interactive and publishes the sort, but the rows
keep the last successful order. The renamed heading's anchor is updated at
both inbound links.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Vercel preview ready

Preview: https://pretable-53we3ogw6-cacheplane.vercel.app
Commit: c06258ab09cbcb5596b2a1780da26d442c12d95d

Updated automatically by the deploy-preview job.

@blove
blove merged commit 3124591 into main Aug 17, 2026
48 of 50 checks passed
@blove
blove deleted the blove/external-sort-authority branch August 17, 2026 15:26
@vercel

vercel Bot commented Aug 17, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
pretable Ignored Ignored Aug 17, 2026 3:33pm

Request Review

@blove blove mentioned this pull request Aug 17, 2026
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