From c06258ab09cbcb5596b2a1780da26d442c12d95d Mon Sep 17 00:00:00 2001 From: Brian Love Date: Sun, 16 Aug 2026 21:13:46 -0700 Subject: [PATCH] fix(react,row-model): external sort authority actually suppresses local sorting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- .changeset/external-sort-authority.md | 19 ++ .../content/docs/grid/pretable-surface.mdx | 2 +- .../content/docs/server-data/index.mdx | 22 +-- .../content/docs/server-data/lifecycle.mdx | 4 +- .../docs/server-data/query-ownership.mdx | 15 +- .../content/docs/server-data/totals.mdx | 2 +- packages/core/core.api.md | 5 + packages/core/src/public_api.ts | 5 +- .../external-filter-authority.test.tsx | 8 +- .../external-sort-authority.test.tsx | 172 ++++++++++++++++++ packages/react/src/pretable-surface.tsx | 3 + packages/react/src/use-pretable.ts | 17 ++ packages/row-model/src/compiled-query.ts | 59 +++++- .../row-model/src/create-local-row-model.ts | 48 ++++- packages/row-model/src/index.ts | 1 + 15 files changed, 353 insertions(+), 29 deletions(-) create mode 100644 .changeset/external-sort-authority.md create mode 100644 packages/react/src/__tests__/external-sort-authority.test.tsx diff --git a/.changeset/external-sort-authority.md b/.changeset/external-sort-authority.md new file mode 100644 index 00000000..024dffd8 --- /dev/null +++ b/.changeset/external-sort-authority.md @@ -0,0 +1,19 @@ +--- +"@pretable/core": minor +"@pretable/react": minor +--- + +`processing: { sort: "external" }` now suppresses local sorting, the way +`filter: "external"` suppresses local filtering since #447. + +It previously suppressed nothing: the declaration was read in two advisory +places and the engine went on applying `query.sort`. That left the consumer who +declared it worse off than one who did not — declaring external sort authority +silences the partial-window warning and unlocks the full population as +`aria-rowcount`, while the local re-sort it silences the warning about kept +running. + +Suppression changes what is APPLIED, never what is REPORTED: `aria-sort`, +`onQueryChange` and the snapshot's `query` are untouched. A consumer holding a +complete window who legitimately sorts locally is unaffected — they declare +`"engine"`, which is the default. diff --git a/apps/website/content/docs/grid/pretable-surface.mdx b/apps/website/content/docs/grid/pretable-surface.mdx index 99a2b88c..0417cfe4 100644 --- a/apps/website/content/docs/grid/pretable-surface.mdx +++ b/apps/website/content/docs/grid/pretable-surface.mdx @@ -92,7 +92,7 @@ See [Row grouping and aggregation](/docs/grid/grouping) for typed grouping, aggr ## Server-applied filtering and sorting -`processing` declares whether filter and sort work was performed by the local engine or an external authority; `resultMeta` then describes the result those rows came from, and `dataState` says where its request stands. The two slices differ in what the declaration does. `filter: "external"` is acted on in rows mode: the surface stops the engine applying `query.filters` while continuing to publish them, so the funnel, `onQueryChange`, and `aria-sort` are unchanged. `sort: "external"` is a claim about the rows you supplied and suppresses nothing — the engine still orders what it holds. All of it — query ownership, totals, the lifecycle phases, and `renderBodyState` — is documented against a live endpoint in [Server-side data](/docs/server-data). +`processing` declares whether filter and sort work was performed by the local engine or an external authority; `resultMeta` then describes the result those rows came from, and `dataState` says where its request stands. Both are acted on in rows mode: the surface stops the engine applying `query.filters` and `query.sort` while continuing to publish them, so the funnel, `onQueryChange`, and `aria-sort` are unchanged. All of it — query ownership, totals, the lifecycle phases, and `renderBodyState` — is documented against a live endpoint in [Server-side data](/docs/server-data). ## Telemetry diff --git a/apps/website/content/docs/server-data/index.mdx b/apps/website/content/docs/server-data/index.mdx index c3dd917f..5ff0782f 100644 --- a/apps/website/content/docs/server-data/index.mdx +++ b/apps/website/content/docs/server-data/index.mdx @@ -14,20 +14,20 @@ Nothing about that is a different component. It is `` with four External processing moves less than people expect. The reader's intent, the interaction state, and the geometry stay in the grid; the data itself becomes yours. -| Concern | Owner | Notes | -| ------------------------- | -------- | ---------------------------------------------------------------------------------------------------------------------------------------------- | -| Query intent | grid | funnels, header clicks, and the group panel still produce filters, sort, and `rowGroups` — you receive them | -| Focus, selection, editing | grid | keyboard, marquee, and cell editors work identically against server-supplied rows | -| Viewport geometry | grid | row virtualization, column layout, pinning, and resizing never consult where the rows came from | -| Fetching | consumer | the grid issues no requests; nothing in it knows a network exists | -| Choosing the records | consumer | `processing.filter: "external"` declares that the server, not the engine, decided which records exist — and the engine stops re-selecting them | -| Choosing the order | consumer | `processing.sort: "external"` says the same about order but suppresses nothing; leaving it to the engine over a partial window sorts a sample | -| Totals | consumer | the row count is whatever `resultMeta.total` claims, and how sure you are of it is part of the claim | -| Lifecycle | consumer | `dataState` is never inferred and has no default — loading, staleness, and failure are things you declare | +| Concern | Owner | Notes | +| ------------------------- | -------- | ---------------------------------------------------------------------------------------------------------------------------------------------------- | +| Query intent | grid | funnels, header clicks, and the group panel still produce filters, sort, and `rowGroups` — you receive them | +| Focus, selection, editing | grid | keyboard, marquee, and cell editors work identically against server-supplied rows | +| Viewport geometry | grid | row virtualization, column layout, pinning, and resizing never consult where the rows came from | +| Fetching | consumer | the grid issues no requests; nothing in it knows a network exists | +| Choosing the records | consumer | `processing.filter: "external"` declares that the server, not the engine, decided which records exist — and the engine stops re-selecting them | +| Choosing the order | consumer | `processing.sort: "external"` says the same about order, and is acted on the same way; leaving it to the engine over a partial window sorts a sample | +| Totals | consumer | the row count is whatever `resultMeta.total` claims, and how sure you are of it is part of the claim | +| Lifecycle | consumer | `dataState` is never inferred and has no default — loading, staleness, and failure are things you declare | Two things about `processing` are worth stating plainly, because "external" reaches further than one slice and less far than the other. -`filter: "external"` **stops the engine selecting records, without changing what it reports**. The published filters stay published — the funnel still shows them, `onQueryChange` still hands them to you — and the engine stops re-applying them to the rows you brought back, because you already did. That matters exactly when the rows and the query disagree, which the lifecycle deliberately allows: while a new result loads, the previous one is still on screen answering the previous query. In the example above, a request that fails leaves the previous rows in place and leaves them readable — filter **Customer** for `fail` and the body keeps every row it already had, with an error strip above it, the same as `notContains` `fail`. Sort is not part of that bargain: `sort: "external"` is a claim about the order you supplied, and the engine still orders what it is given. +`filter: "external"` **stops the engine selecting records, without changing what it reports**. The published filters stay published — the funnel still shows them, `onQueryChange` still hands them to you — and the engine stops re-applying them to the rows you brought back, because you already did. That matters exactly when the rows and the query disagree, which the lifecycle deliberately allows: while a new result loads, the previous one is still on screen answering the previous query. In the example above, a request that fails leaves the previous rows in place and leaves them readable — filter **Customer** for `fail` and the body keeps every row it already had, with an error strip above it, the same as `notContains` `fail`. `sort: "external"` is the same bargain one axis over: the sort stays published and the engine stops re-applying it, so a window the server ranked keeps that ranking instead of being re-sorted as a sample. What the claim does buy is honesty about counts, and it cuts both ways. With both slices external and an exact total, `aria-rowcount` may publish the whole population instead of just the rows in the model, because loaded position and dataset position finally line up. In the other direction, declaring external filtering narrows what a select-all or a CSV export is allowed to call "all rows": unless the exact total says you already hold every matching record, the answer is the loaded ones. That is [Totals and honesty](/docs/server-data/totals). diff --git a/apps/website/content/docs/server-data/lifecycle.mdx b/apps/website/content/docs/server-data/lifecycle.mdx index 604d6407..d02a1fa4 100644 --- a/apps/website/content/docs/server-data/lifecycle.mdx +++ b/apps/website/content/docs/server-data/lifecycle.mdx @@ -25,7 +25,7 @@ The search below sends one POST to `/api/docs/rows` per submit and waits the end Most of those distinctions are ones only you can draw. `stale` and `refreshing` differ by whether the query changed; `loading` and `stale` differ by whether a result has ever committed. The grid witnessed neither event. One flag is usually enough for both — the example keeps a `hasCommitted` ref and reads it once per search. -One thing has to be true for `stale` to be worth declaring, and it is a claim you make on another page. The previous rows only stay readable if nothing re-selects them while they are there — and where the reader's filters live in the grid's own `query`, a funnel or a header, `stale` is precisely the moment the engine holds a filter the loaded rows have never answered. [`processing: { filter: "external" }`](/docs/server-data/query-ownership#what-external-filtering-suppresses) is what stops it applying that filter to them; short of it the body empties and refills on every search, which is the flicker this phase exists to prevent. The example above drives its search from a form outside the grid's query, so there is nothing there for the engine to re-apply — but [the overview's grid](/docs/server-data) filters through the funnel and depends on the declaration. +One thing has to be true for `stale` to be worth declaring, and it is a claim you make on another page. The previous rows only stay readable if nothing re-selects them while they are there — and where the reader's filters live in the grid's own `query`, a funnel or a header, `stale` is precisely the moment the engine holds a filter the loaded rows have never answered. [`processing: { filter: "external" }`](/docs/server-data/query-ownership#what-external-authority-suppresses) is what stops it applying that filter to them; short of it the body empties and refills on every search, which is the flicker this phase exists to prevent. The example above drives its search from a form outside the grid's query, so there is nothing there for the engine to re-apply — but [the overview's grid](/docs/server-data) filters through the funnel and depends on the declaration. What the surface actually draws depends on the phase **and** on whether the body currently has rows to show: @@ -52,7 +52,7 @@ Nothing here is inferable after the fact, either. An empty `rows` array is a res The choice keys on what the body is currently rendering rather than on how many records are loaded, which matters under [engine filter authority](/docs/server-data/query-ownership): a grid can hold plenty of records and still show nothing, and that case wants the block. Under external filter authority — the remote shape this rule was written for — the two counts are the same number. -The half of this you own is the same discipline on your side: when a request rejects, leave the last good rows in state. The example's failure handler sets the phase and nothing else — rows, total, and dataset key are all left exactly as the last successful search committed them, which is why its header is still sortable while the error strip is up. Clear the rows first and then declare `error` and you have thrown away a result the reader could still have used, and traded the strip for the full-viewport block. +The half of this you own is the same discipline on your side: when a request rejects, leave the last good rows in state. The example's failure handler sets the phase and nothing else — rows, total, and dataset key are all left exactly as the last successful search committed them, which is why its header stays interactive while the error strip is up — a click still publishes the new sort through `onQueryChange`, though under external sort authority the rows on screen keep the order the last successful response gave them until a request succeeds. Clear the rows first and then declare `error` and you have thrown away a result the reader could still have used, and traded the strip for the full-viewport block. The strip carries no live-region role of its own — the surface keeps exactly one live region, and the strip does not become a second one. diff --git a/apps/website/content/docs/server-data/query-ownership.mdx b/apps/website/content/docs/server-data/query-ownership.mdx index 4d72e27c..d3501e4f 100644 --- a/apps/website/content/docs/server-data/query-ownership.mdx +++ b/apps/website/content/docs/server-data/query-ownership.mdx @@ -1,6 +1,6 @@ --- title: Query ownership -description: "Who holds the reader's query, who hears about it, and what declaring external processing authority actually changes — which is the filtering the engine applies, three things the grid claims, and deliberately not the sort." +description: "Who holds the reader's query, who hears about it, and what declaring external processing authority actually changes — the filtering and the ordering the engine applies, and three things the grid claims." nav: Server-side data --- @@ -17,11 +17,11 @@ The grid below answers the first question with the shape nothing else in these d | Field | Type | Notes | | -------- | ------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `filter` | `"engine" \| "external"` | Who chose which records exist. Taken literally: the engine stops applying `query.filters`. The only slice `resolveDataScope` reads, and half of what `aria-rowcount` needs. | -| `sort` | `"engine" \| "external"` | Who chose their order. A claim only — the engine goes on ordering the rows it holds. Scope never consults it; the announced count does. | +| `sort` | `"engine" \| "external"` | Who chose their order. Taken literally: the engine stops applying `query.sort`. Scope never consults it; the announced count does. | Both are optional, and omitting one is the same as declaring it `"engine"`. -The two halves are not symmetric, and the asymmetry is the thing to carry away. `filter: "external"` is taken at its word: the engine stops selecting records, which is [the section below](/docs/server-data/query-ownership#what-external-filtering-suppresses). `sort: "external"` is a claim and only a claim; nothing about it stops the engine ordering the rows you handed it. +Both are taken at their word: `filter: "external"` stops the engine selecting records, and `sort: "external"` stops it ordering them. [The section below](/docs/server-data/query-ownership#what-external-authority-suppresses) is what that means in practice. Declaring one says nothing about the other — splitting them is legal, and the paragraph after next is about the one split worth avoiding. What **both** of them change is what the grid is willing to say, and there are exactly three of those: @@ -29,13 +29,13 @@ What **both** of them change is what the grid is willing to say, and there are e - **What it is allowed to call "all rows".** Scope reads `processing.filter` alone — sort does not enter into it. Under engine filter authority the loaded records _are_ the population, so scope is `"all"`. Under external, it is `"all"` only when an exact total says you already hold every matching record, and `"loaded"` otherwise. Every user-facing count label and the CSV export route through that answer, which is why a 200-row window onto 10,432 matches cannot be exported under a heading that says "all". - **What a filter menu offers.** An `enum` column with no declared `options` falls back to the distinct values of the records on hand. Under external filtering those records are one server-chosen window, so the funnel would be offering a fragment as if it were the complete universe for `isAnyOf` — and the engine says so, once per column, the first time such a menu opens. Declaring `column.options` is the fix, and the example above declares both of its enum columns. -Splitting the two slices is legal and sometimes right — a grid that loads the entire matching result can let the engine order it — but mixed authority over a **partial** window is the one combination to avoid. Sorting a server-selected window locally reorders a sample, not the population, and the header still reports an ordinary `aria-sort` over it. If the server chose the records, let it choose the order too. +Splitting the two slices is legal and sometimes right — a grid that loads the entire matching result can let the engine order it, and that consumer simply leaves `sort` at `"engine"` — but external `filter` with engine `sort` over a **partial** window is the one combination to avoid. Sorting a server-selected window locally reorders a sample, not the population, so the rows on screen are not the top N of anything, and the header still reports an ordinary `aria-sort` over them. If the server chose the records, let it choose the order too. The grid says so when it can prove it: external `filter`, engine `sort`, and an exact `resultMeta.total` counting more records than are loaded is the one case where "partial" is not a guess, and it warns once per page load. Silence is not a clearance — without an exact total there is nothing to measure the window against, and the combination is no safer for being unprovable. -### What external filtering suppresses +### What external authority suppresses -`filter: "external"` is not a hint. In rows mode the surface hands the authority to the row model it owns, and the compiled query plan keeps two versions of the query: the one it **reports** and the one it **applies**. Under external filtering the reported one keeps your filters and the applied one has none. `get query()` — and through it the snapshot, the funnel menu, and `onQueryChange` — reads the reported version; row evaluation reads the applied one. So the funnel still shows the filter you set, the callback still hands it to you, `aria-sort` is untouched, and the engine simply stops re-selecting the records you were given. +`filter: "external"` is not a hint. In rows mode the surface hands the authority to the row model it owns, and the compiled query plan keeps two versions of the query: the one it **reports** and the one it **applies**. Under external filtering the reported one keeps your filters and the applied one has none; under external sorting the same split holds for `query.sort`. `get query()` — and through it the snapshot, the funnel menu, and `onQueryChange` — reads the reported version; row evaluation reads the applied one. So the funnel still shows the filter you set, the callback still hands it to you, the header still reports `aria-sort` for the column you sorted, and the engine simply stops re-selecting and re-ordering the records you were given. Suppression changes what is applied, never what is reported. @@ -47,9 +47,8 @@ What suppression does not do is collapse the two counts everywhere. Under **engi ### What suppression does not cover -Three boundaries, because symmetry would be the reasonable assumption: +Two boundaries, because symmetry would be the reasonable assumption: -- **Sort.** `sort: "external"` suppresses nothing: the engine goes on applying `query.sort` to the rows it holds, so a header click reorders them locally under either value. Over a result the server ordered by that same sort it is a no-op, and it is what leaves a header usable while a request is failing. Deliberate, not an oversight — filtering is where the harm was, a consumer holding a complete window who sorts locally is doing something reasonable, and the one provably dishonest combination already warns, above. - **Explicit-model mode.** The authority reaches the row model the surface constructs, and no further. A model you built is yours: you already decide what goes into its query, so omit the filters the server applied rather than expecting `processing` to move anything. Passing `processing` alongside your own model changes what the grid announces and exports, not what it filters. - **Grouping.** `rowGroups` is not part of `processing` and never was. What does change under suppression is what a group folds: with no filters applied there is no post-filter subset, so group aggregates and child counts are computed over every loaded row. That is the right answer when the server chose the records, and it is a real behaviour change — a grid grouping a locally-filtered window will report different numbers once it declares external filtering. diff --git a/apps/website/content/docs/server-data/totals.mdx b/apps/website/content/docs/server-data/totals.mdx index abb64dd4..dffa3255 100644 --- a/apps/website/content/docs/server-data/totals.mdx +++ b/apps/website/content/docs/server-data/totals.mdx @@ -8,7 +8,7 @@ A grid that holds a server's answer knows nothing about the records it was not s So the type does not ask you for a number. It asks for a claim, and for how strong the claim is. `PretableMatchingTotal` has three shapes — `exact`, `estimate`, `unknown` — and the strength is the load-bearing half: one of the three unlocks things, and the other two exist so that a server which does not know the count has something honest to send. -The grid below sends the same request three ways. The rows never change: the endpoint returns all 480 orders whichever button you pick, because nothing asks it for a page. Only the claim about them changes, and with it what the grid is prepared to say. Its funnels and header sorts are switched off, unlike every other grid in this section — this one's query never leaves the client, and under the `filter: "external"` it declares, a funnel would set a filter that [nothing applies](/docs/server-data/query-ownership#what-external-filtering-suppresses). +The grid below sends the same request three ways. The rows never change: the endpoint returns all 480 orders whichever button you pick, because nothing asks it for a page. Only the claim about them changes, and with it what the grid is prepared to say. Its funnels and header sorts are switched off, unlike every other grid in this section — this one's query never leaves the client, and under the `filter: "external"` it declares, a funnel would set a filter that [nothing applies](/docs/server-data/query-ownership#what-external-authority-suppresses). diff --git a/packages/core/core.api.md b/packages/core/core.api.md index 477d70b1..c1a7d6db 100644 --- a/packages/core/core.api.md +++ b/packages/core/core.api.md @@ -1380,6 +1380,11 @@ export interface ɵPretableIndexedWindowing { // @internal export function ɵsetLocalRowModelFilterAuthority(model: object, authority: "engine" | "external"): void; +// Warning: (ae-internal-missing-underscore) The name "ɵsetLocalRowModelSortAuthority" should be prefixed with an underscore because the declaration is marked as @internal +// +// @internal +export function ɵsetLocalRowModelSortAuthority(model: object, authority: "engine" | "external"): void; + // (No @packageDocumentation comment for this package) ``` diff --git a/packages/core/src/public_api.ts b/packages/core/src/public_api.ts index 3027ceb7..0879848f 100644 --- a/packages/core/src/public_api.ts +++ b/packages/core/src/public_api.ts @@ -175,4 +175,7 @@ export type { PretableIndexedWindowing as ɵPretableIndexedWindowing } from "@pr // the authority a rows-mode model is built with can change while it is alive, // and react cannot reach the row model package directly without duplicating the // registry this reads. Not for consumer models — see the function's own doc. -export { ɵsetLocalRowModelFilterAuthority } from "@pretable-internal/row-model"; +export { + ɵsetLocalRowModelFilterAuthority, + ɵsetLocalRowModelSortAuthority, +} from "@pretable-internal/row-model"; diff --git a/packages/react/src/__tests__/external-filter-authority.test.tsx b/packages/react/src/__tests__/external-filter-authority.test.tsx index 0f9205fb..ad158651 100644 --- a/packages/react/src/__tests__/external-filter-authority.test.tsx +++ b/packages/react/src/__tests__/external-filter-authority.test.tsx @@ -203,10 +203,14 @@ describe("external filter authority suppresses local filtering", () => { act(() => grid!.setQuery(NARROWING_AND_SORTED)); await expect.poll(() => seen.at(-1)).toEqual(NARROWING_AND_SORTED); - // Uncontrolled, so the engine really applied it — and still drew the rows. + // Uncontrolled, so the engine really took the query — and still drew every + // row. `EXTERNAL` declares external sort authority too, so the order it + // draws them in is the one the server gave, not the one the new sort asks + // for: suppression applies to sort on the same terms as filter. The + // reporting assertions below are this test's actual subject. await expect .poll(() => renderedRowIds()) - .toEqual(["r2", "r3", "r5", "r1", "r6", "r4"]); + .toEqual(LOADED.map((row) => row.id)); expect(reportedHeaderState()).toEqual({ ariaSort: "ascending", filterActive: "true", diff --git a/packages/react/src/__tests__/external-sort-authority.test.tsx b/packages/react/src/__tests__/external-sort-authority.test.tsx new file mode 100644 index 00000000..fa18df08 --- /dev/null +++ b/packages/react/src/__tests__/external-sort-authority.test.tsx @@ -0,0 +1,172 @@ +import "@testing-library/jest-dom/vitest"; +import { cleanup, render, screen } from "@testing-library/react"; +import * as React from "react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { createColumnHelper } from "@pretable/core"; +import type { + PretableProcessingOptions, + PretableQueryFor, +} from "@pretable/core"; + +import { resetDevWarnings } from "../dev-warn"; +import { PretableSurface } from "../pretable-surface"; + +afterEach(cleanup); + +let warn: ReturnType; + +beforeEach(() => { + resetDevWarnings(); + warn = vi.spyOn(console, "warn").mockImplementation(() => {}); +}); + +afterEach(() => { + warn.mockRestore(); +}); + +type Row = { id: string; customer: string; amount: number }; + +const column = createColumnHelper(); +const columns = [ + column.accessor("customer", { type: "text", widthPx: 160 }), + column.accessor("amount", { type: "number", widthPx: 120 }), +] as const; + +/** + * A window the server returned, deliberately NOT in `customer` order. The + * server ranked these six out of a population of 480 — they are the answer to + * "top 6 by whatever the server was asked for", and their order carries that + * ranking. Re-sorting them locally does not reorder the population; it reorders + * a sample, and the result is six rows that are not the top six of anything. + */ +const LOADED: readonly Row[] = [ + { id: "r1", customer: "Northwind", amount: 60 }, + { id: "r2", customer: "Contoso", amount: 50 }, + { id: "r3", customer: "Fabrikam", amount: 40 }, + { id: "r4", customer: "Tailspin", amount: 30 }, + { id: "r5", customer: "Litware", amount: 20 }, + { id: "r6", customer: "Proseware", amount: 10 }, +]; + +const SERVER_ORDER = LOADED.map((row) => row.id); +/** What a local ascending sort on `customer` would produce instead. */ +const LOCAL_SORT_ORDER = ["r2", "r3", "r5", "r1", "r6", "r4"]; + +const EXTERNAL: PretableProcessingOptions = { + filter: "external", + sort: "external", +}; +const ENGINE: PretableProcessingOptions = { + filter: "external", + sort: "engine", +}; + +const SORTED_QUERY: PretableQueryFor = { + filters: [], + sort: [{ columnId: "customer", direction: "asc" }], + rowGroups: [], +}; + +/** Every data row currently drawn, by row id — never a count. */ +function renderedRowIds(): string[] { + return Array.from( + document.querySelectorAll("[data-pretable-row][data-pretable-row-id]"), + ).map((node) => node.getAttribute("data-pretable-row-id") ?? ""); +} + +/** 6 loaded of 480 matching: a partial window by construction. */ +function surface(processing: PretableProcessingOptions) { + return ( + + ariaLabel="Orders" + columns={columns} + rows={LOADED} + getRowId={(row) => row.id} + viewportHeight={2000} + processing={processing} + query={SORTED_QUERY} + resultMeta={{ total: { kind: "exact", count: 480 } }} + onQueryChange={() => undefined} + /> + ); +} + +function warnings(): string[] { + return warn.mock.calls.map((call: unknown[]) => String(call[0])); +} + +describe("the fixture can tell the two orders apart", () => { + it("server order and local-sort order are different sequences", () => { + // Without this, every assertion below could pass by coincidence. + expect(LOCAL_SORT_ORDER).not.toEqual(SERVER_ORDER); + expect([...LOCAL_SORT_ORDER].sort()).toEqual([...SERVER_ORDER].sort()); + }); + + it("engine sort authority really does reorder this fixture", () => { + render(surface(ENGINE)); + expect(renderedRowIds()).toEqual(LOCAL_SORT_ORDER); + }); +}); + +describe('sort: "external" suppresses local sorting', () => { + it("keeps the order the server returned", () => { + render(surface(EXTERNAL)); + expect(renderedRowIds()).toEqual(SERVER_ORDER); + }); + + it("follows a processing flip after mount, in both directions", async () => { + // `processing` is a render-time read, never a memo dependency, so a + // consumer really can flip it while one model lives. + const view = render(surface(ENGINE)); + expect(renderedRowIds()).toEqual(LOCAL_SORT_ORDER); + + view.rerender(surface(EXTERNAL)); + await expect.poll(() => renderedRowIds()).toEqual(SERVER_ORDER); + + view.rerender(surface(ENGINE)); + await expect.poll(() => renderedRowIds()).toEqual(LOCAL_SORT_ORDER); + }); + + it("still REPORTS the sort it stopped applying", () => { + render(surface(EXTERNAL)); + // Suppression changes what is APPLIED, never what is REPORTED. + expect( + screen.getByRole("columnheader", { name: /customer/i }), + ).toHaveAttribute("aria-sort", "ascending"); + }); +}); + +describe("the two claims the declaration unlocks are now earned", () => { + /** + * These two behaviours predate suppression, and before it they were the + * argument FOR it: declaring `sort: "external"` silenced the partial-window + * warning and published the population count, while the local sort it + * silenced the warning about went on running. Both are now truthful, and + * both are pinned here so that pairing cannot quietly come apart again. + * + * The warning still fires under engine sort authority, where it is still the + * right thing to say. + */ + it("the partial-window warning fires for engine sort, and not for external", () => { + render(surface(ENGINE)); + expect(warnings().join("\n")).toMatch(/wrong SAMPLE/); + + cleanup(); + resetDevWarnings(); + warn.mockClear(); + + render(surface(EXTERNAL)); + expect(warnings().join("\n")).not.toMatch(/wrong SAMPLE/); + }); + + /** + * `aria-rowcount` claims every loaded row sits at its true dataset position. + * That claim is only honest because the rows were not reordered locally — + * which is what the first block in this file proves. + */ + it("declaring external sort publishes the full population as aria-rowcount", () => { + render(surface(EXTERNAL)); + expect(screen.getByRole("grid")).toHaveAttribute("aria-rowcount", "481"); + }); +}); diff --git a/packages/react/src/pretable-surface.tsx b/packages/react/src/pretable-surface.tsx index 14f6dd92..5a968b9a 100644 --- a/packages/react/src/pretable-surface.tsx +++ b/packages/react/src/pretable-surface.tsx @@ -1915,6 +1915,9 @@ export function PretableSurface< // `query.filters` without re-applying them; the model branch below // deliberately omits it. ɵfilterAuthority: processing?.filter ?? "engine", + // Same rule, one axis over: `processing.sort === "external"` + // says the caller already ranked these records. + ɵsortAuthority: processing?.sort ?? "engine", ...(query === undefined ? {} : { query }), ...(initialExpansion === undefined ? {} : { initialExpansion }), viewportHeight: bodyViewportHeight, diff --git a/packages/react/src/use-pretable.ts b/packages/react/src/use-pretable.ts index 9b550346..5a9fdbaf 100644 --- a/packages/react/src/use-pretable.ts +++ b/packages/react/src/use-pretable.ts @@ -1,6 +1,7 @@ import { createLocalRowModel, ɵsetLocalRowModelFilterAuthority, + ɵsetLocalRowModelSortAuthority, type ColumnIdOf, type ColumnsOf, type PretableDerivationsFor, @@ -318,6 +319,7 @@ export function usePretable(rawOptions: unknown): unknown { * moves its authority. */ readonly ɵfilterAuthority?: "engine" | "external"; + readonly ɵsortAuthority?: "engine" | "external"; readonly ɵvisualColumns?: | readonly { readonly id: string }[] | ((query: PretableQueryFor) => readonly { @@ -358,6 +360,9 @@ export function usePretable(rawOptions: unknown): unknown { ...(rowsOptions.ɵfilterAuthority === undefined ? {} : { ɵfilterAuthority: rowsOptions.ɵfilterAuthority }), + ...(rowsOptions.ɵsortAuthority === undefined + ? {} + : { ɵsortAuthority: rowsOptions.ɵsortAuthority }), } as never) as PretableRowModel; }); const rowModel = @@ -375,6 +380,9 @@ export function usePretable(rawOptions: unknown): unknown { const lastFilterAuthority = useRef( mode === "rows" ? (rowsOptions.ɵfilterAuthority ?? "engine") : "engine", ); + const lastSortAuthority = useRef( + mode === "rows" ? (rowsOptions.ɵsortAuthority ?? "engine") : "engine", + ); /* * Guarded on `ownedModel`, not on `mode`: the model the caller supplied is @@ -391,6 +399,15 @@ export function usePretable(rawOptions: unknown): unknown { ɵsetLocalRowModelFilterAuthority(ownedModel, authority); }); + /** The sort twin, guarded on `ownedModel` for the same reason. */ + useLayoutEffect(() => { + if (ownedModel === null) return; + const authority = rowsOptions.ɵsortAuthority ?? "engine"; + if (lastSortAuthority.current === authority) return; + lastSortAuthority.current = authority; + ɵsetLocalRowModelSortAuthority(ownedModel, authority); + }); + useLayoutEffect(() => { if (mode !== "rows") return; const derivationsChanged = lastDerivations.current !== rowsOptions.columns; diff --git a/packages/row-model/src/compiled-query.ts b/packages/row-model/src/compiled-query.ts index 23abeeed..9db3c655 100644 --- a/packages/row-model/src/compiled-query.ts +++ b/packages/row-model/src/compiled-query.ts @@ -135,6 +135,22 @@ export interface CompiledQuery { */ export type CompiledFilterAuthority = "engine" | "external"; +/** + * Who ordered the records the plan is handed. `"external"` says something + * outside the engine already applied `query.sort`, so the plan publishes it and + * stops re-applying it. + * + * The case for this is narrower than filtering's and worth stating. A consumer + * holding the whole matching population who sorts locally is being perfectly + * reasonable — but that consumer declares `"engine"`, so suppression never + * touches them. It binds only the consumer who said the server owns ordering, + * and for that consumer re-sorting is wrong twice over: over a partial window a + * local sort reorders a server-selected SAMPLE, so the rows on screen are not + * the top N of anything; and while `dataState.phase === "stale"` it reorders + * rows that answer the previous query by the comparator of the new one. + */ +export type CompiledSortAuthority = "engine" | "external"; + export interface CompileQueryInput { readonly derivations: PretableDerivationsFor; readonly query: PretableQueryFor; @@ -147,6 +163,12 @@ export interface CompileQueryInput { * records the caller was handed are the records the engine draws. */ readonly filterAuthority?: CompiledFilterAuthority; + /** + * Defaults to `"engine"`. Under `"external"` the compiled plan reports + * `query.sort` unchanged and evaluates rows in the order they were handed in, + * so the ranking the caller was given is the ranking the engine draws. + */ + readonly sortAuthority?: CompiledSortAuthority; } export class CompiledQueryValidationError extends TypeError { @@ -244,6 +266,7 @@ interface InternalCompiledQuery { derivations: readonly RuntimeColumn[], query: RuntimeQuery, filterAuthority: CompiledFilterAuthority, + sortAuthority: CompiledSortAuthority, ): boolean; }; } @@ -296,6 +319,7 @@ interface CapturedCompileInput { readonly query: RuntimeQuery; readonly previous: object | undefined; readonly filterAuthority: CompiledFilterAuthority; + readonly sortAuthority: CompiledSortAuthority; } function captureCompileInput(input: object): CapturedCompileInput { @@ -307,6 +331,11 @@ function captureCompileInput(input: object): CapturedCompileInput { "filterAuthority", "input.filterAuthority", ); + const rawSortAuthority = captureProperty( + input, + "sortAuthority", + "input.sortAuthority", + ); if (rawQuery === null || typeof rawQuery !== "object") fail("query must be an object", "input.query"); if ( @@ -318,11 +347,18 @@ function captureCompileInput(input: object): CapturedCompileInput { 'filterAuthority must be "engine" or "external"', "input.filterAuthority", ); + if ( + rawSortAuthority !== undefined && + rawSortAuthority !== "engine" && + rawSortAuthority !== "external" + ) + fail('sortAuthority must be "engine" or "external"', "input.sortAuthority"); return { columns: captureColumns(rawColumns), query: captureQuery(rawQuery), previous: previous as object | undefined, filterAuthority: (rawAuthority ?? "engine") as CompiledFilterAuthority, + sortAuthority: (rawSortAuthority ?? "engine") as CompiledSortAuthority, }; } @@ -1059,23 +1095,31 @@ function snapshotQuery( } const EMPTY_FILTERS = Object.freeze([]) as readonly RuntimeFilter[]; +const EMPTY_SORT = Object.freeze([]) as RuntimeQuery["sort"]; /** * The query the plan APPLIES, as opposed to the one it reports. Filters are * sorted into a canonical order so plan identity survives a reordered filter * list — and dropped outright under external filter authority, which is the * single point where "the caller already selected these records" takes effect. + * + * Sort is the same idea one axis over: dropped under external sort authority, + * which is where "the caller already ranked these records" takes effect. Note + * that only `query.sort` goes — `rowGroups` keeps its own ordering, because + * grouping is not in `PretableProcessingOptions` and a consumer who declared + * external sort authority said nothing about it. */ function canonicalRuntimeQuery( query: RuntimeQuery, filterAuthority: CompiledFilterAuthority, + sortAuthority: CompiledSortAuthority, ): RuntimeQuery { return Object.freeze({ filters: filterAuthority === "external" ? EMPTY_FILTERS : Object.freeze([...query.filters].sort(compareFilterDescriptors)), - sort: query.sort, + sort: sortAuthority === "external" ? EMPTY_SORT : query.sort, rowGroups: query.rowGroups, }); } @@ -1295,6 +1339,7 @@ class CompiledQueryPlan readonly #aggregateColumns: readonly RuntimeColumn[]; readonly #operation: "set-query" | "set-derivations"; readonly #filterAuthority: CompiledFilterAuthority; + readonly #sortAuthority: CompiledSortAuthority; readonly #evaluationCache = new WeakMap(); /* @@ -1311,8 +1356,10 @@ class CompiledQueryPlan derivations: readonly RuntimeColumn[], query: RuntimeQuery, filterAuthority: CompiledFilterAuthority, + sortAuthority: CompiledSortAuthority, ) => this.#filterAuthority === filterAuthority && + this.#sortAuthority === sortAuthority && derivationsEqualForPlan( this.#runtimeColumns, derivations, @@ -1340,12 +1387,18 @@ class CompiledQueryPlan capturedQuery: RuntimeQuery, operation: "set-query" | "set-derivations", filterAuthority: CompiledFilterAuthority, + sortAuthority: CompiledSortAuthority, ) { this.#publicColumns = capturedColumns; this.#publicQuery = capturedQuery; this.#runtimeColumns = capturedColumns; this.#filterAuthority = filterAuthority; - this.#runtimeQuery = canonicalRuntimeQuery(capturedQuery, filterAuthority); + this.#sortAuthority = sortAuthority; + this.#runtimeQuery = canonicalRuntimeQuery( + capturedQuery, + filterAuthority, + sortAuthority, + ); this.#operation = operation; this.#byId = new Map( this.#runtimeColumns.map((column) => [column.id, column]), @@ -1529,6 +1582,7 @@ export function compileQuery( captured.columns, captured.query, captured.filterAuthority, + captured.sortAuthority, ) ) return previous; @@ -1538,5 +1592,6 @@ export function compileQuery( captured.query, input.operation ?? "set-query", captured.filterAuthority, + captured.sortAuthority, ); } diff --git a/packages/row-model/src/create-local-row-model.ts b/packages/row-model/src/create-local-row-model.ts index 67f6912a..69f40c9a 100644 --- a/packages/row-model/src/create-local-row-model.ts +++ b/packages/row-model/src/create-local-row-model.ts @@ -2,6 +2,7 @@ import { compileQuery, type CompiledFilterAuthority, type CompiledQuery, + type CompiledSortAuthority, } from "./compiled-query"; import { createCooperativeTransitionCandidate, @@ -133,6 +134,17 @@ interface CreateLocalRowModelBaseOptions< * @internal */ readonly ɵfilterAuthority?: CompiledFilterAuthority; + /** + * Who ordered the rows handed in. `"external"` keeps `query.sort` reported + * while the engine stops re-applying it. Set at construction for the same + * reason as {@link ɵfilterAuthority} — the initial store is built inside + * `createLocalRowModel`, so a model created under engine authority would + * paint one locally-sorted frame before any effect could correct it — and + * flipped afterwards through {@link ɵsetLocalRowModelSortAuthority}. + * + * @internal + */ + readonly ɵsortAuthority?: CompiledSortAuthority; /** Overrides the bounded consumer journal size for diagnostics and tests. */ readonly changeJournalCapacity?: number; /** Internal deterministic scheduler injection for cooperative rebuilds. */ @@ -164,6 +176,10 @@ const modelFilterAuthoritySetters = new WeakMap< object, (authority: CompiledFilterAuthority) => void >(); +const modelSortAuthoritySetters = new WeakMap< + object, + (authority: CompiledSortAuthority) => void +>(); /** * Re-declares who selected the loaded records, recompiling the plan when the @@ -189,6 +205,20 @@ export function ɵsetLocalRowModelFilterAuthority( modelFilterAuthoritySetters.get(model)?.(authority); } +/** + * Re-declares who ordered the loaded records. The sort twin of + * `ɵsetLocalRowModelFilterAuthority`, with the same registry design and + * the same ownership rule: withhold it from models the surface does not own. + * + * @internal + */ +export function ɵsetLocalRowModelSortAuthority( + model: object, + authority: "engine" | "external", +): void { + modelSortAuthoritySetters.get(model)?.(authority); +} + /** Direct diagnostics seam; intentionally absent from the package barrel. */ export function registerLocalRowModelInstrumentationForTesting( options: object, @@ -549,10 +579,12 @@ export function createLocalRowModel< const requestedQuery = options.query ?? emptyQuery(); let filterAuthority: CompiledFilterAuthority = options.ɵfilterAuthority ?? "engine"; + let sortAuthority: CompiledSortAuthority = options.ɵsortAuthority ?? "engine"; let queryPlan = compileQuery({ derivations: requestedDerivations, query: requestedQuery, filterAuthority, + sortAuthority, }); let derivations = queryPlan.derivations; let query = queryPlan.query; @@ -935,7 +967,12 @@ export function createLocalRowModel< }); const pendingDiagnostics = drafted.diagnostics; if (drafted.sameReferenceMutation) { - nextPlan = compileQuery({ derivations, query, filterAuthority }); + nextPlan = compileQuery({ + derivations, + query, + filterAuthority, + sortAuthority, + }); drafted = replaceFlatRowsDraft({ root: previousRoot, rows: nextRows, @@ -1070,6 +1107,7 @@ export function createLocalRowModel< previous: queryPlan, operation: "set-query", filterAuthority, + sortAuthority, }); if (nextPlan === queryPlan) { const superseded = cancelActiveTransition("superseded") !== undefined; @@ -1109,6 +1147,7 @@ export function createLocalRowModel< query, operation: "set-derivations", filterAuthority, + sortAuthority, }); const nextPlan = compileQuery({ derivations: capturedPlan.derivations, @@ -1116,6 +1155,7 @@ export function createLocalRowModel< previous: queryPlan, operation: "set-derivations", filterAuthority, + sortAuthority, }); if (nextPlan === queryPlan) { derivations = capturedPlan.derivations; @@ -1312,6 +1352,12 @@ export function createLocalRowModel< const transition = model.setQuery(query); void transition.finished.catch(() => undefined); }); + modelSortAuthoritySetters.set(model, (authority) => { + if (disposed || authority === sortAuthority) return; + sortAuthority = authority; + const transition = model.setQuery(query); + void transition.finished.catch(() => undefined); + }); distinctValues.attachModel(model); emitDiagnostics(initialStore.diagnostics, diagnosticSink); return model as unknown as PretableRowModel; diff --git a/packages/row-model/src/index.ts b/packages/row-model/src/index.ts index f4fef171..7aedff28 100644 --- a/packages/row-model/src/index.ts +++ b/packages/row-model/src/index.ts @@ -3,6 +3,7 @@ export * from "./compiled-query"; export { createLocalRowModel, ɵsetLocalRowModelFilterAuthority, + ɵsetLocalRowModelSortAuthority, type CreateLocalRowModelOptions, type CreateLocalRowModelWithDefaultIdOptions, } from "./create-local-row-model";