fix(react,row-model): external sort authority actually suppresses local sorting - #467
Merged
Conversation
…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>
Contributor
Vercel preview readyPreview: https://pretable-53we3ogw6-cacheplane.vercel.app Updated automatically by the |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
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.
processing: { sort: "external" }declares that something outside the grid chose the order of the records it was handed. The engine went on applyingquery.sortanyway.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 indata-scope.ts. The row model had afilterAuthorityand 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.tsxpins 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
CompiledQueryPlankeeps#publicQuery(read byget query()→ snapshot, header,onQueryChange) separate from#runtimeQuery(read byevaluate()). Under external sort authority the runtime query getssort: []while the public one keeps it.Only
query.sortis dropped.rowGroupskeeps its own ordering, because grouping is not inPretableProcessingOptionsand declaring external sort said nothing about it.Suppression changes what is APPLIED, never what is REPORTED —
aria-sortstill announces the column,onQueryChangestill 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
@internalconstruction argument (the initial store is built insidecreateLocalRowModel, so a model created under engine authority would paint one locally-sorted frame first), plus a WeakMap-registry setter for later flips, guarded onownedModelso a consumer's own model is never touched.react.api.mdis unchanged;core.api.mdgains one@internalɵ-prefixed line.Verification
expected [ 'r2', 'r3', 'r5', 'r1', 'r6', 'r4' ] to deeply equal [ 'r1' … 'r6' ].canonicalRuntimeQuery: 2 of 7 tests fail, and restoring it fixes them.EXTERNAL, which is exactly the behaviour this removes. Its reporting assertions — the test's actual subject — are untouched.pnpm testgreen (react 1234, website 556, bench 156, renderer-dom 127), plustypecheck,lint,format,buildandapi: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-ownershiploses its asymmetry section and its "suppresses nothing" bullet;indexandpretable-surfacelose the same claim;lifecycleno 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