From 5ce63f54cd0dbc44e62d78c09eba570d199e6cee Mon Sep 17 00:00:00 2001 From: Brian Love Date: Tue, 25 Aug 2026 18:43:43 -0700 Subject: [PATCH 01/15] =?UTF-8?q?docs:=20spec=20the=20filter=20tree=20?= =?UTF-8?q?=E2=80=94=20tool=20panel=20SP2a?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 --- .../specs/2026-08-25-filter-tree-design.md | 136 ++++++++++++++++++ 1 file changed, 136 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-25-filter-tree-design.md diff --git a/docs/superpowers/specs/2026-08-25-filter-tree-design.md b/docs/superpowers/specs/2026-08-25-filter-tree-design.md new file mode 100644 index 000000000..956d56787 --- /dev/null +++ b/docs/superpowers/specs/2026-08-25-filter-tree-design.md @@ -0,0 +1,136 @@ +# Filter Tree (Tool Panel SP2a) — Design + +**Status:** approved direction; SP2a specced in full, SP2b (the builder UI) outlined. +**Parent:** `docs/superpowers/specs/2026-08-24-tool-panel-design.md` (SP2 of the tool panel). + +## What this is + +Boolean filter composition for the engine: filters become an **arbitrary-depth +tree** of AND/OR groups over the existing typed leaves, so a query can express +`price > 10 AND (status isAnyOf [a,b] OR owner contains "x")`. This is the +engine prerequisite for SP2b, the tool panel's filter builder section — the +same engine-first split SP1 used for column visibility, because the UI must +compose against a model that exists. + +## Decisions locked (and why) + +1. **Full OR groups, not multi-filter AND.** Chosen over the cheaper options + (one-per-column AND; flat multi-filter AND) — the builder's value is + expressing what the per-column menu cannot. +2. **Engine first (SP2a), builder second (SP2b).** A combined PR would span + row-model → core → react → ui → docs; SP1 was 30 commits _without_ an + engine rewrite. Each half ships and is testable alone. +3. **Arbitrary nesting.** The engine cost over a fixed two-level shape is + recursion versus a loop; the recursive type is cleaner than a special-cased + one; and SP2b may still _render_ a bounded depth while the model keeps the + full algebra. Deciding expressiveness once beats a pre-1.0 rework later. +4. **`filters` stays an array; groups are elements.** + `filters: readonly (Leaf | Group)[]`, top level an implicit AND — every + existing call site keeps compiling and the simple case stays one-liner + simple. Rejected: a single root group (ceremony on every simple consumer + forever) and a parallel `filterTree` field (two sources of truth — the + declared-but-read-by-nothing failure mode this repo keeps paying for). + +## The type + +```ts +/** @public */ +export interface PretableFilterGroupFor { + readonly op: "and" | "or"; + readonly children: readonly ( + PretableFilterFor | PretableFilterGroupFor + )[]; +} + +// PretableQueryFor.filters: +// readonly (PretableFilterFor | PretableFilterGroupFor)[] +``` + +- Leaves and groups discriminate structurally (`columnId`+`operator` vs + `op`+`children`). A public runtime guard `isPretableFilterGroup` ships so + consumers never hand-roll the discrimination; internal code uses the same + guard. +- **Type-level trap, named:** `PretableFilterFor` is a large distributive + conditional type. Extending the union around it must be probed with the + repo's `IsNever` discipline — a conditional that collapses to `never` + compiles every downstream guard while checking nothing, and this exact + failure shipped once before. + +## Evaluation semantics + +- Top-level array: AND over elements (unchanged behavior for all-leaf arrays). +- Group: `and` → `children.every`, `or` → `children.some`, short-circuiting, + recursion for nested groups. +- Leaf evaluation is untouched — the existing compiled `RuntimeFilter` path is + reused; the tree changes combination only, never leaf semantics. +- **Empty group ⇒ TRUE, regardless of `op`.** The naive algebra says empty-OR + is false, but that would let a half-built group in SP2b's UI blank the whole + grid mid-edit. Identity-true is the product-safe convention; it is stated in + the TSDoc and pinned by a test. An all-empty tree therefore filters nothing, + exactly like `filters: []` today. +- The query-equality comparison that gates recompiles (currently a flat + `every` over leaves) learns deep structural tree comparison. Getting this + wrong in the "always unequal" direction recompiles per publish; in the + "always equal" direction it never recompiles — both directions get tests. + +## Surface and controlled state + +- `state.filters` changes shape from `Record` (one per + column) to the query's own array-of-leaf-or-group — one vocabulary + everywhere, no projection layer. Pre-1.0, no compatibility aliases. +- **Header funnel semantics, scoped honestly:** + - The per-column FilterMenu **creates / edits / removes its column's + top-level leaf** — its exact job today, unchanged in capability. + - The funnel **lights when its column appears anywhere in the tree** + (recursive scan). + - A column filtered only inside groups shows a lit funnel and a menu with no + editable leaf; the explanatory "also filtered in advanced groups" line in + the menu belongs to SP2b, when a builder exists to point at. +- If the menu's write path today replaces the whole per-column record, it now + splices only the top-level leaf and must not disturb group elements — a + survives-test (assert the old behavior AND the group's integrity). + +## External authority and the wire + +- `filter: "external"` suppresses local evaluation exactly as today — + suppression happens at evaluation, and the tree does not change that seam. +- The tree flows through `onQueryChange` verbatim. **The server-side data docs' + filter contract updates in SP2a** (not SP2b), because the wire shape a + server can receive changes the moment this merges. + +## The audit (SP1's discipline) + +Every reader of `query.filters` / `state.filters` gets a recorded verdict — +recursive-aware, flat-assuming (fix), or display-only. Known candidates, to be +completed by grep during planning: the funnel projection, the filter-count / +post-filter row-count paths, CSV/export omission reporting, copy, +announcements, bench adapters, docs examples, the headless docs example. + +## Verification + +- row-model unit tests: evaluation (AND/OR/nested/empty-group), with fixtures + that can disprove — an OR fixture whose expected rows differ from the same + tree with AND, so a connective mix-up cannot pass. +- Deep-equality tests in both failure directions (never-equal / always-equal). +- Type-level `IsNever` probes on the extended union; api reports regenerate + (core + react move; build before `pnpm api`). +- FilterMenu write-path survives-tests; funnel-anywhere tests. +- Docs guard: any new table registered; the server-side filter page's wire + examples must keep passing the fence/import checks. +- Changesets: core minor (or row-model as versioning dictates), react minor. + +## Out of scope for SP2a + +The builder UI (SP2b), any rendering of groups anywhere, the FilterMenu's +"advanced groups" note, NOT/negation as a group op (leaves already carry +negated operators: `notContains`, `isNoneOf`, `notEquals` — a group-level NOT +is redundant today and can be added compatibly if SP2b finds a need), saved +views, and the section-strings i18n pass (SP2b, alongside its new strings). + +## SP2b outline (for continuity, not specced here) + +The tool panel's filter section renders the tree — likely with a rendered +depth cap over the full underlying algebra (its call); typed value editors +reused from the shipped cell-editor set; add/remove/re-op/regroup +interactions; the i18n pass over section strings; the shared menu-keyboard +extraction that the third `role="menu"` triggers. From f3382cdea9958d5dd9b8bfa8c08f5805fa35fb22 Mon Sep 17 00:00:00 2001 From: Brian Love Date: Tue, 25 Aug 2026 19:11:56 -0700 Subject: [PATCH 02/15] =?UTF-8?q?docs:=20plan=20the=20filter=20tree=20?= =?UTF-8?q?=E2=80=94=20SP2a?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 --- .../plans/2026-08-25-filter-tree-sp2a.md | 150 ++++++++++++++++++ 1 file changed, 150 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-25-filter-tree-sp2a.md diff --git a/docs/superpowers/plans/2026-08-25-filter-tree-sp2a.md b/docs/superpowers/plans/2026-08-25-filter-tree-sp2a.md new file mode 100644 index 000000000..b21714a1f --- /dev/null +++ b/docs/superpowers/plans/2026-08-25-filter-tree-sp2a.md @@ -0,0 +1,150 @@ +# Filter Tree (SP2a) Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Filters become an arbitrary-depth tree of AND/OR groups over the existing typed leaves — the engine prerequisite for the tool panel's filter builder (SP2b). + +**Architecture:** The row-model query is already the single source of truth for filters (grid-core holds no filter state; the surface's per-column record is a derived projection). A recursive group node joins the filter union; capture, evaluation, and equality in `compiled-query.ts` recurse; the surface's projection, funnel, and FilterMenu write path become tree-aware. No UI renders groups in SP2a. + +**Tech Stack:** TypeScript, vitest, API Extractor, changesets. No CSS, no new components. + +**Spec:** `docs/superpowers/specs/2026-08-25-filter-tree-design.md` — decisions there are settled: arbitrary nesting; `filters` stays an array with groups as elements (implicit top-level AND); **empty group evaluates TRUE regardless of `op`**; funnel lights on any occurrence; menu owns only its column's top-level leaf. + +--- + +## Ground truth (verified, 2026-08-25 — line numbers may drift, anchors won't) + +- `packages/row-model/src/column-types.ts:566` — `PretableQueryFor.filters: readonly PretableFilterFor[]`; `PretableFilterFor` is the big distributive conditional at `:470-530`. +- `packages/row-model/src/compiled-query.ts`: + - `:251` `RuntimeFilter { columnId, operator, value? }`; `:263` `RuntimeQuery`. + - `:572` `captureFilter(raw, index)` — validates and freezes each incoming leaf, `fail()`s with a `query.filters[i]` path. + - `:1491` evaluation: `this.#runtimeQuery.filters.every((filter) => …)`. + - `:906` `queryEqual` → `:914` `filtersEqual` — **order-insensitive multiset** match (used-set + findIndex). AND/OR are commutative, so order-insensitivity stays correct per level under recursion. + - `:150/:1170` `filterAuthority === "external"` gates evaluation — the tree does not touch this seam. +- `packages/react/src/pretable-surface.tsx`: + - `:2180-2190` builds the derived `Record` **from `rowModelSnapshot.query.filters`, with a cast that assumes every element is a leaf** (`entry.columnId`) — a group element would today land under key `undefined`. This is the projection that becomes tree-aware. + - `:403` the controlled `state.filters: Readonly>`. + - `:5848` funnel: `active={Boolean(snapshot.filters[column.id])}`; `:6616` menu reads `snapshot.filters[columnId]`. + - The menu's commit path goes through the surface's query write (find it from `:6616`'s handler; it rebuilds the filters array and calls the model's `setQuery` path). +- `ColumnFilter` (`packages/grid-core/src/types.ts:124`) is `{ operator, value? }` — **column-id-less**; the record's key carries the id. Grid-core has zero filter state or methods; nothing in grid-core changes in SP2a. +- Server-side docs live under `apps/website/content/docs/server-data/`. + +## File map + +| File | Responsibility | +|---|---| +| `packages/row-model/src/column-types.ts` | `PretableFilterGroupFor`, widen `PretableQueryFor.filters`, `isPretableFilterGroup` guard | +| `packages/row-model/src/compiled-query.ts` | recursive capture / evaluation / equality | +| `packages/row-model/src/__tests__/filter-tree.test.ts` | new — semantics suite | +| row-model's existing type-tests file (find: `ls packages/row-model/src/__tests__/*type*`) | `IsNever` probes | +| `packages/react/src/pretable-surface.tsx` | snapshot projection, funnel, menu read/write, `state.filters` shape | +| `packages/react/src/__tests__/` | extend the filter/controlled-state suites | +| `apps/website/content/docs/server-data/` filter page | wire contract | +| `packages/core` / `packages/react` `.api.md` + `.changeset/` | reports, changesets | + +## Standing rules + +- TDD; prettier before trusting a test; mutation-check every guard-like assertion; `pnpm build` before `pnpm api`; drawn-order/`getColumns()` rules don't apply here but the audit discipline does; subscribe to snapshots, never `getState`, in anything reactive; no stash/checkout with uncommitted work — restore mutations by targeted edit. +- **Pre-1.0, no compatibility aliases** — but "array of leaves keeps compiling" is a design requirement, not backcompat courtesy: verify it with an untouched existing test. + +--- + +### Task 1: row-model — the group type, guard, and capture + +**Files:** `packages/row-model/src/column-types.ts`, `packages/row-model/src/compiled-query.ts`, create `packages/row-model/src/__tests__/filter-tree.test.ts`, extend the row-model type-tests + +- [ ] **Step 1: Failing type probes first.** In the row-model type-test file, following its existing `IsNever`/assertion idiom: + - `PretableFilterGroupFor` is not `never` for a representative column set; + - `PretableQueryFor["filters"][number]` accepts both a leaf and a group (assignability probes both directions); + - a group with a misspelled `op: "nor"` is rejected; + - **the pre-existing leaf-only probes stay untouched and green** — that is the "array of leaves keeps compiling" requirement, checked by not editing them. +- [ ] **Step 2: Failing runtime tests** in `filter-tree.test.ts` (copy the harness of the nearest compiled-query test): + - `isPretableFilterGroup` true for `{op:"and",children:[]}`, false for every leaf shape (probe each operator family: text, number-between, date, enum-set, isEmpty); + - capture: a query whose filters include a nested group round-trips into the compiled query without throwing; `op: "xor"` fails with a `query.filters[1].op` path; a group whose `children` is not an array fails with its path; deep-frozen output (mutating a nested child throws in strict mode). +- [ ] **Step 3: Run both, confirm failures for the right reasons.** +- [ ] **Step 4: Implement.** + - `column-types.ts`: the interface exactly as the spec writes it; the guard narrow and total: + +```ts +/** @public */ +export function isPretableFilterGroup( + node: PretableFilterFor | PretableFilterGroupFor, +): node is PretableFilterGroupFor { + // Structural: groups carry `op` + `children`; every leaf carries `operator`. + // Checked positively on the group's fields so an unknown shape fails closed. + return ( + typeof node === "object" && + node !== null && + "children" in node && + ("op" in node + ? (node as { op: unknown }).op === "and" || + (node as { op: unknown }).op === "or" + : false) + ); +} +``` + + - `compiled-query.ts`: `RuntimeFilterNode = RuntimeFilter | RuntimeFilterGroup { op, children }`; `captureFilter` becomes the leaf half of a recursive `captureFilterNode(raw, path)` that validates `op` ∈ {and, or}, requires an array `children`, recurses with `path.children[i]`, and freezes each level. Keep `fail()`'s message voice. +- [ ] **Step 5:** Tests green; whole package: `pnpm --filter @pretable-internal/row-model test` (check the actual package name in its package.json first). Prettier. +- [ ] **Step 6: Commit** `feat(row-model): filter groups — the type, the guard, and capture`. + +### Task 2: recursion — evaluation and equality + +**Files:** `packages/row-model/src/compiled-query.ts`, extend `filter-tree.test.ts` + +- [ ] **Step 1: Failing tests.** Fixtures must be able to disprove (the repo rule — an OR fixture whose expected rows differ from the same tree under AND): + - rows `[{n:1},{n:5},{n:9}]`, tree `[{gt 4} , {op:"or", children:[{lt 2},{gt 8}]}]` → rows 9 only; same tree with the group's op flipped to `and` → no rows. Both asserted, so a connective mix-up cannot pass. + - nesting three deep evaluates correctly (compose the above inside another `or`). + - **empty group ⇒ TRUE for both ops** — `{op:"or",children:[]}` alongside a real leaf filters exactly as the leaf alone; same for `and`. (This is the spec's product-safety convention; the naive algebra would say empty-OR ⇒ false.) + - short-circuit is NOT observable behavior — do not test call counts; outcomes only. + - equality both directions: two trees equal up to sibling permutation at each level ARE equal (no recompile); trees differing only in a nested leaf's value are NOT equal. Assert through whatever the compiled query exposes for plan reuse (find how existing tests observe recompile-vs-reuse — follow that mechanism, not internals). +- [ ] **Step 2: Confirm failures.** +- [ ] **Step 3: Implement.** `:1491` → `evaluateFilterNode(node, row)`: leaf → existing single-filter evaluation unchanged; group → `op === "and" ? children.every : children.length === 0 ? true : children.some` (write the empty-OR case explicitly with the WHY comment — `some` on empty already returns false, which is precisely the wrong answer here). `filtersEqual` → `filterNodesEqual`: same used-set multiset shape, recursing when both sides are groups (`op` must match; children compared as an order-insensitive multiset per level). +- [ ] **Step 4:** Green; full package; prettier. **Mutation round, all restored by targeted edit:** flip `every`/`some` → the OR/AND twin tests fail; delete the empty-group special case → the empty-OR test fails; make `filterNodesEqual` ignore `op` → the equality test fails. +- [ ] **Step 5: Commit** `feat(row-model): filter trees evaluate and compare recursively`. + +### Task 3: API reports and the core changeset + +- [ ] **Step 1:** `pnpm build && pnpm api && pnpm api:check`. Expected surfacing: `PretableFilterGroupFor`, `isPretableFilterGroup`, the widened `filters` element type — in `core.api.md` and `react.api.md` (the query types flow through both). Anything else surfacing is a stop-and-report. +- [ ] **Step 2:** Changeset `@pretable/core` minor: the group node, the guard, implicit-AND array preserved, empty-group-TRUE semantics named (a CHANGELOG reader must learn that rule here). +- [ ] **Step 3: Commit** `chore: api reports and changeset for filter groups`. + +### Task 4: react — projection, funnel, menu, controlled state + +**Files:** `packages/react/src/pretable-surface.tsx`, extend the surface filter/controlled-state test files + +The audit is the heart of this task. `grep -n "query.filters\|state.filters\|snapshot.filters" packages/react/src apps/website apps/bench --include="*.ts*" -r` — every site gets a verdict (tree-aware / leaf-only-by-design / display-only), recorded as a code comment where non-obvious. Known sites: the projection (`:2180` — its leaf-assuming cast is the bug-in-waiting), funnel (`:5848`), menu read (`:6616`), menu commit, controlled `state.filters` (`:403`), CSV/export omissions, bench adapters, docs examples. + +- [ ] **Step 1: Failing tests:** + - **snapshot shape**: `snapshot.filters` becomes the query's array verbatim (leaves carry `columnId`; groups nest). Existing consumers of the old record shape inside the repo are part of this task's churn — the type change finds them. + - **funnel-anywhere**: a filter on column `a` buried two groups deep lights `a`'s funnel; no top-level leaf needed. (New recursive helper `columnHasFilter(filters, columnId)` — pure, exported from a surface-adjacent module or local, unit-tested directly.) + - **menu reads only its top-level leaf**: with `a` filtered both at top level and inside a group, the menu shows the top-level leaf's operator/value. + - **menu write splices, groups survive** (the survives-test): commit a new filter from the menu for `a` while a group mentioning `a` and `b` exists → the group element is byte-identical in the resulting query; clearing `a` from the menu removes only the top-level leaf. + - **controlled `state.filters`** takes the array shape; a controlled tree renders funnels and filters rows (jsdom: assert visible row count through the model, the way controlled-query tests do). +- [ ] **Step 2:** Failures confirmed. **Step 3:** Implement — projection passes the array through (delete the record-building loop), funnel uses the helper, menu read/write scoped to top-level leaves, `state.filters` type + application path updated, audit verdicts written. +- [ ] **Step 4:** Full `pnpm --filter @pretable/react test` and `pnpm --filter @pretable/app-website test` (docs examples may consume `snapshot.filters` — expected churn belongs here). Prettier. +- [ ] **Step 5:** `pnpm build && pnpm api && pnpm api:check` (react.api.md moves for the snapshot/state types); react changeset (minor) written now. +- [ ] **Step 6: Commit** `feat(react): the surface speaks filter trees — funnels, menu, controlled state`. + +### Task 5: the wire contract docs + +**Files:** the filter page under `apps/website/content/docs/server-data/` (find the one documenting `onQueryChange`'s filter payload) + +- [ ] **Step 1:** Read the page and the docs guard's current reach (`apps/website/lib/docs/__tests__/docs-api-surface.test.ts` — it checks fenced imports, prose `Pretable*` identifiers, and registered tables; it was hardened four rounds, assume it sees more than you expect). +- [ ] **Step 2:** Document: groups arrive verbatim in `query.filters`; leaves vs groups discriminate on `op`/`children` (name `isPretableFilterGroup` for consumers on the client edge); a server that only understands flat filters must decide explicitly (reject, flatten-if-all-AND, or implement) — present the three honestly; empty-group-TRUE is part of the contract. +- [ ] **Step 3:** `pnpm --filter @pretable/app-website test -- docs-api-surface` green (register any new table). Full website suite. Prettier (markdown tables). +- [ ] **Step 4: Commit** `docs(server-data): the filter wire contract grows groups`. + +### Task 6: final battery + +- [ ] All package suites (row-model, core, react, ui, website) — real counts; typecheck, lint, `pnpm format`. +- [ ] `pnpm build && pnpm api && pnpm api:check`; `git status` clean of stale reports. +- [ ] Website e2e, FULL suite, production build, root playwright binary from inside `apps/website`, `--workers=1` (the cockpit filter smoke exercises the menu path end-to-end). +- [ ] Re-verify both changesets against what actually shipped. +- [ ] Confirm the audit table is complete: re-run the grep, zero unverdicted sites. + +## Self-review + +**Spec coverage:** type+guard (T1), evaluation+equality+empty-group (T2), reports/changesets (T3), surface+funnel+menu+state (T4), wire docs (T5), audit (T4+T6), verification (T6). External authority needs no task — the seam is evaluation-side and T2's tests run under default authority; T4's audit confirms no authority-path reader assumes leaves. + +**Judgment calls made here, flagged:** (1) `snapshot.filters` becomes the query array *verbatim* rather than keeping a parallel per-column record for chrome — one vocabulary, and the funnel helper is cheaper than maintaining a projection that lies by omission; (2) the menu's write path is defined by splice-preserving-groups rather than rebuild-from-record — the survives-test enforces it. From bd4198f98e2e7eea37d7f7badc9c1f78e1fdab45 Mon Sep 17 00:00:00 2001 From: Brian Love Date: Tue, 25 Aug 2026 19:23:30 -0700 Subject: [PATCH 03/15] =?UTF-8?q?feat(row-model):=20filter=20groups=20?= =?UTF-8?q?=E2=80=94=20the=20type,=20the=20guard,=20and=20capture?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PretableQueryFor.filters` becomes an arbitrary-depth tree: each entry is either a typed leaf or a `PretableFilterGroupFor` joining its children with "and" or "or". `isPretableFilterGroup` narrows one node, checked positively on the group's own fields so an unknown shape fails closed. Capture is now recursive: `captureFilterNode` validates the join operator, requires a dense array of children, breadcrumbs failures as `query.filters[i].children[j]…`, and freezes every level. Validation recurses with the same breadcrumb, so a bogus column or operator inside a group is rejected at compile time rather than at evaluation. Evaluation still applies every leaf conjunctively (`filterLeavesOf`) and group equality is a conservative descriptor-key match — real tree semantics land in the next change. Flat queries are byte-for-byte unaffected. Co-Authored-By: Claude Opus 5 --- .../src/__tests__/filter-tree.test.ts | 257 ++++++++++++++++++ .../row-model/src/__tests__/types.test.ts | 88 ++++++ packages/row-model/src/column-types.ts | 36 ++- packages/row-model/src/compiled-query.ts | 183 ++++++++++--- 4 files changed, 530 insertions(+), 34 deletions(-) create mode 100644 packages/row-model/src/__tests__/filter-tree.test.ts diff --git a/packages/row-model/src/__tests__/filter-tree.test.ts b/packages/row-model/src/__tests__/filter-tree.test.ts new file mode 100644 index 000000000..f04e6b7f2 --- /dev/null +++ b/packages/row-model/src/__tests__/filter-tree.test.ts @@ -0,0 +1,257 @@ +import { describe, expect, test } from "vitest"; + +import { + CompiledQueryValidationError, + compileQuery, + createColumnHelper, + isPretableFilterGroup, + type PretableQueryFor, +} from "../index"; + +interface Holding { + id: number; + sector: string | null; + quantity: number | null; + asOf: Date; + status: "open" | "closed"; +} + +const column = createColumnHelper(); +const columns = [ + column.accessor("sector", { type: "text" }), + column.accessor("quantity", { type: "number" }), + column.accessor("asOf", { type: "date" }), + column.accessor("status", { type: "enum" }), +] as const; + +type Columns = typeof columns; +type Node = PretableQueryFor["filters"][number]; + +function queryFor(value: PretableQueryFor): PretableQueryFor { + return value; +} + +function compile(query: unknown) { + return compileQuery({ + derivations: columns, + query, + } as never); +} + +function caughtFrom(query: unknown): unknown { + try { + compile(query); + } catch (error) { + return error; + } + return undefined; +} + +describe("isPretableFilterGroup", () => { + test("accepts both join operators, including an empty group", () => { + expect(isPretableFilterGroup({ op: "and", children: [] })).toBe( + true, + ); + expect( + isPretableFilterGroup({ + op: "or", + children: [{ columnId: "sector", operator: "isEmpty" }], + }), + ).toBe(true); + }); + + test.each([ + ["text", { columnId: "sector", operator: "contains", value: "x" }], + [ + "number range", + { columnId: "quantity", operator: "between", value: [1, 2] }, + ], + ["date", { columnId: "asOf", operator: "on", value: "2026-01-01" }], + ["enum set", { columnId: "status", operator: "isAnyOf", value: ["open"] }], + ["valueless", { columnId: "sector", operator: "isEmpty" }], + ] as const)("rejects a %s leaf", (_family, leaf) => { + expect(isPretableFilterGroup(leaf as Node)).toBe(false); + }); + + test("fails closed on shapes that are neither", () => { + for (const shape of [ + null, + undefined, + "and", + 42, + {}, + { op: "and" }, + { children: [] }, + { op: "nor", children: [] }, + { op: "AND", children: [] }, + ]) { + expect(isPretableFilterGroup(shape as never)).toBe(false); + } + }); +}); + +describe("capture of a filter tree", () => { + test("round-trips a nested group into the compiled query", () => { + const plan = compile( + queryFor({ + filters: [ + { columnId: "quantity", operator: "gte", value: 4 }, + { + op: "or", + children: [ + { columnId: "sector", operator: "contains", value: "tech" }, + { + op: "and", + children: [ + { columnId: "status", operator: "isAnyOf", value: ["open"] }, + { columnId: "sector", operator: "isNotEmpty" }, + ], + }, + ], + }, + ], + sort: [], + rowGroups: [], + }), + ); + + expect(plan.query.filters).toHaveLength(2); + const group = plan.query.filters[1]; + expect(isPretableFilterGroup(group)).toBe(true); + if (!isPretableFilterGroup(group)) throw new Error("unreachable"); + expect(group.op).toBe("or"); + expect(group.children).toHaveLength(2); + expect(group.children[0]).toMatchObject({ + columnId: "sector", + operator: "contains", + value: "tech", + }); + const nested = group.children[1]; + if (!isPretableFilterGroup(nested)) throw new Error("unreachable"); + expect(nested.op).toBe("and"); + expect(nested.children[0]).toMatchObject({ + columnId: "status", + operator: "isAnyOf", + value: ["open"], + }); + }); + + test("rejects an unknown join operator at its own path", () => { + const caught = caughtFrom({ + filters: [ + { columnId: "quantity", operator: "gte", value: 4 }, + { op: "xor", children: [] }, + ], + sort: [], + rowGroups: [], + }); + + expect(caught).toBeInstanceOf(CompiledQueryValidationError); + expect(caught).toMatchObject({ + code: "invalid-query", + path: "query.filters[1].op", + }); + }); + + test("rejects a group whose children are not an array at its own path", () => { + const caught = caughtFrom({ + filters: [{ op: "and", children: { length: 0 } }], + sort: [], + rowGroups: [], + }); + + expect(caught).toBeInstanceOf(CompiledQueryValidationError); + expect(caught).toMatchObject({ + code: "invalid-query", + path: "query.filters[0].children", + }); + }); + + test("reports a nested failure with its full breadcrumb", () => { + const caught = caughtFrom({ + filters: [ + { + op: "and", + children: [ + { columnId: "sector", operator: "isEmpty" }, + { op: "and", children: [{ op: "nope", children: [] }] }, + ], + }, + ], + sort: [], + rowGroups: [], + }); + + expect(caught).toBeInstanceOf(CompiledQueryValidationError); + expect(caught).toMatchObject({ + path: "query.filters[0].children[1].children[0].op", + }); + }); + + test("deep-freezes every level of the captured tree", () => { + const plan = compile( + queryFor({ + filters: [ + { + op: "and", + children: [ + { + op: "or", + children: [{ columnId: "sector", operator: "isEmpty" }], + }, + ], + }, + ], + sort: [], + rowGroups: [], + }), + ); + + const outer = plan.query.filters[0]; + if (!isPretableFilterGroup(outer)) throw new Error("unreachable"); + const inner = outer.children[0]; + if (!isPretableFilterGroup(inner)) throw new Error("unreachable"); + const leaf = inner.children[0]; + + expect(Object.isFrozen(outer)).toBe(true); + expect(Object.isFrozen(outer.children)).toBe(true); + expect(Object.isFrozen(inner)).toBe(true); + expect(Object.isFrozen(inner.children)).toBe(true); + expect(Object.isFrozen(leaf)).toBe(true); + expect(() => { + (inner as { op: string }).op = "and"; + }).toThrow(TypeError); + expect(() => { + (leaf as { columnId: string }).columnId = "quantity"; + }).toThrow(TypeError); + }); + + test("copies the incoming tree rather than retaining it", () => { + const child = { columnId: "sector", operator: "isEmpty" }; + const group = { op: "and", children: [child] }; + const plan = compile({ filters: [group], sort: [], rowGroups: [] }); + + const captured = plan.query.filters[0]; + expect(captured).not.toBe(group); + if (!isPretableFilterGroup(captured)) + throw new Error("unreachable"); + expect(captured.children[0]).not.toBe(child); + }); + + test("still accepts an array of plain leaves", () => { + const plan = compile( + queryFor({ + filters: [ + { columnId: "quantity", operator: "gte", value: 4 }, + { columnId: "sector", operator: "contains", value: "tech" }, + ], + sort: [], + rowGroups: [], + }), + ); + expect(plan.query.filters).toHaveLength(2); + expect( + plan.query.filters.every((node) => !isPretableFilterGroup(node)), + ).toBe(true); + }); +}); diff --git a/packages/row-model/src/__tests__/types.test.ts b/packages/row-model/src/__tests__/types.test.ts index 59a86d883..7719a004a 100644 --- a/packages/row-model/src/__tests__/types.test.ts +++ b/packages/row-model/src/__tests__/types.test.ts @@ -11,6 +11,8 @@ import { type PretableDerivationTransition, type PretableDerivationsFor, type PretableExpansionDefault, + type PretableFilterFor, + type PretableFilterGroupFor, PretableDisposedModelError, type PretableGroupId, type PretableGroupKey, @@ -447,6 +449,92 @@ const badValueQuery: PretableQueryFor = { void badOperatorQuery; void badValueQuery; +// --- filter tree (SP2a) type probes --------------------------------------- +// +// `PretableFilterFor` is a distributive conditional type; widening the union +// it feeds can silently collapse it (or the new group type) to `never`, which +// would make every downstream narrowing probe vacuously green. These probes +// pin both halves as inhabited before anything else is asserted. +type IsNever = [T] extends [never] ? true : false; + +type _leafUnionIsInhabited = Expect< + Equal>, false> +>; +type _groupTypeIsInhabited = Expect< + Equal>, false> +>; +type _filterNodeIsInhabited = Expect< + Equal["filters"][number]>, false> +>; + +type FilterNode = PretableQueryFor["filters"][number]; + +// Assignability in both directions: the node slot accepts a leaf and a group, +// and neither half alone is the whole slot. +type _leafAssignableToNode = Expect< + [PretableFilterFor] extends [FilterNode] ? true : false +>; +type _groupAssignableToNode = Expect< + [PretableFilterGroupFor] extends [FilterNode] ? true : false +>; +type _nodeIsNotOnlyLeaf = Expect< + Equal< + [FilterNode] extends [PretableFilterFor] ? true : false, + false + > +>; +type _nodeIsNotOnlyGroup = Expect< + Equal< + [FilterNode] extends [PretableFilterGroupFor] + ? true + : false, + false + > +>; + +const treeQuery: PretableQueryFor = { + filters: [ + { columnId: "quantity", operator: "gte", value: 4 }, + { + op: "or", + children: [ + { columnId: "sector", operator: "contains", value: "tech" }, + { + op: "and", + children: [ + { columnId: "quantity", operator: "between", value: [1, 2] }, + { columnId: "sector", operator: "isEmpty" }, + ], + }, + ], + }, + ], + sort: [], + rowGroups: [], +}; +const badGroupOperatorQuery: PretableQueryFor = { + filters: [ + // @ts-expect-error a filter group joins with "and" or "or", never "nor" + { op: "nor", children: [] }, + ], + sort: [], + rowGroups: [], +}; +const badGroupChildQuery: PretableQueryFor = { + filters: [ + { + op: "and", + // @ts-expect-error a group child is still a typed leaf + children: [{ columnId: "quantity", operator: "contains", value: 4 }], + }, + ], + sort: [], + rowGroups: [], +}; +void treeQuery; +void badGroupOperatorQuery; +void badGroupChildQuery; + declare const model: PretableRowModel; type _row = Expect, Holding>>; type _rowId = Expect, number>>; diff --git a/packages/row-model/src/column-types.ts b/packages/row-model/src/column-types.ts index 5c0eec336..def61212a 100644 --- a/packages/row-model/src/column-types.ts +++ b/packages/row-model/src/column-types.ts @@ -531,6 +531,38 @@ export type PretableFilterFor = : never : never; +/** + * A branch of the filter tree: a join operator and the nodes it joins. A group + * may hold leaves, further groups, or nothing at all, to arbitrary depth. + * @public + */ +export interface PretableFilterGroupFor { + readonly op: "and" | "or"; + readonly children: readonly ( + PretableFilterFor | PretableFilterGroupFor + )[]; +} + +/** + * Narrows one filter-tree node to a group. Checked positively on the group's + * own fields, so an unknown shape fails closed rather than being treated as a + * branch with no children. + * @public + */ +export function isPretableFilterGroup( + node: PretableFilterFor | PretableFilterGroupFor, +): node is PretableFilterGroupFor { + return ( + typeof node === "object" && + node !== null && + "children" in node && + ("op" in node + ? (node as { op: unknown }).op === "and" || + (node as { op: unknown }).op === "or" + : false) + ); +} + /** @public */ export type PretableSortFor = Prettify< (TColumns extends readonly (infer TColumn)[] @@ -564,7 +596,9 @@ export type PretableRowGroupFor = Prettify< /** @public */ export interface PretableQueryFor { - readonly filters: readonly PretableFilterFor[]; + readonly filters: readonly ( + PretableFilterFor | PretableFilterGroupFor + )[]; readonly sort: readonly PretableSortFor[]; readonly rowGroups: readonly PretableRowGroupFor[]; } diff --git a/packages/row-model/src/compiled-query.ts b/packages/row-model/src/compiled-query.ts index c42e3559f..f7141251a 100644 --- a/packages/row-model/src/compiled-query.ts +++ b/packages/row-model/src/compiled-query.ts @@ -254,6 +254,41 @@ interface RuntimeFilter { readonly value?: unknown; } +interface RuntimeFilterGroup { + readonly op: "and" | "or"; + readonly children: readonly RuntimeFilterNode[]; +} + +type RuntimeFilterNode = RuntimeFilter | RuntimeFilterGroup; + +/** + * The internal twin of `isPretableFilterGroup`: same positive check on the + * group's own fields, over the already-captured runtime shape. + */ +function isRuntimeFilterGroup( + node: RuntimeFilterNode, +): node is RuntimeFilterGroup { + return "children" in node; +} + +/** + * The leaves of a filter tree, in depth-first order. Filter TREE evaluation is + * not wired up yet (SP2a task 2): the plan currently applies every leaf + * conjunctively regardless of the group it sits in, which is exactly the old + * behaviour for a flat list of leaves. + */ +function filterLeavesOf( + nodes: readonly RuntimeFilterNode[], +): readonly RuntimeFilter[] { + const leaves: RuntimeFilter[] = []; + const visit = (node: RuntimeFilterNode): void => { + if (isRuntimeFilterGroup(node)) node.children.forEach(visit); + else leaves.push(node); + }; + nodes.forEach(visit); + return leaves; +} + interface RuntimeOrdering { readonly columnId: string; readonly direction?: string; @@ -261,7 +296,7 @@ interface RuntimeOrdering { } interface RuntimeQuery { - readonly filters: readonly RuntimeFilter[]; + readonly filters: readonly RuntimeFilterNode[]; readonly sort: readonly RuntimeOrdering[]; readonly rowGroups: readonly RuntimeOrdering[]; } @@ -501,7 +536,7 @@ function captureQuery(source: object): RuntimeQuery { rawFilters, "query.filters", "filters must be an array", - captureFilter, + (entry, index) => captureFilterNode(entry, `query.filters[${index}]`), ), sort: captureDenseArray( rawSort, @@ -569,10 +604,33 @@ function captureDenseArray( return Object.freeze(captured); } -function captureFilter(raw: unknown, index: number): RuntimeFilter { - const path = `query.filters[${index}]`; +/** + * One node of the filter tree. A node carrying `children` is a group and is + * captured recursively, breadcrumbed as `.children[i]`; anything else is + * captured as a leaf. Every level is frozen on the way out, so the published + * tree is owned by the plan rather than aliasing the caller's objects. + */ +function captureFilterNode(raw: unknown, path: string): RuntimeFilterNode { if (raw === null || typeof raw !== "object") fail("filter entry is not an object", path); + const children = captureProperty(raw, "children", `${path}.children`); + if (children === undefined) return captureFilter(raw, path); + + const op = captureProperty(raw, "op", `${path}.op`); + if (op !== "and" && op !== "or") + fail("filter group must join with and or or", `${path}.op`); + return Object.freeze({ + op, + children: captureDenseArray( + children, + `${path}.children`, + "filter group children must be an array", + (entry, index) => captureFilterNode(entry, `${path}.children[${index}]`), + ), + }); +} + +function captureFilter(raw: object, path: string): RuntimeFilter { const columnId = captureProperty(raw, "columnId", `${path}.columnId`); const contextId = typeof columnId === "string" ? columnId : undefined; const operator = captureProperty( @@ -740,12 +798,27 @@ function validateOrdering( } } +function validateFilterNode( + node: RuntimeFilterNode, + columns: ReadonlyMap, + path: string, +): void { + if (!node || typeof node !== "object") + fail("filter entry is not an object", path); + if (isRuntimeFilterGroup(node)) { + node.children.forEach((child, index) => + validateFilterNode(child, columns, `${path}.children[${index}]`), + ); + return; + } + validateFilter(node, columns, path); +} + function validateFilter( filter: RuntimeFilter, columns: ReadonlyMap, - index: number, + path: string, ): void { - const path = `query.filters[${index}]`; if (!filter || typeof filter !== "object") fail("filter entry is not an object", path); const column = resolveColumn(columns, filter.columnId, `${path}.columnId`); @@ -855,7 +928,9 @@ function validateQuery( fail("filters, sort, and rowGroups must be arrays"); } const byId = new Map(columns.map((column) => [column.id, column])); - query.filters.forEach((filter, index) => validateFilter(filter, byId, index)); + query.filters.forEach((node, index) => + validateFilterNode(node, byId, `query.filters[${index}]`), + ); query.sort.forEach((entry, index) => validateOrdering(entry, byId, "sort", index), ); @@ -911,19 +986,40 @@ function queryEqual(left: RuntimeQuery, right: RuntimeQuery): boolean { ); } +/* + * Leaves are matched unordered and semantically. Groups are matched by their + * descriptor key, which is deliberately conservative — a reordered group + * reports "changed" rather than risking a plan reuse it did not earn. SP2a + * task 2 owns real tree equality and should replace the group arm. + */ +function filterNodesEqual( + left: RuntimeFilterNode, + right: RuntimeFilterNode, +): boolean { + if (isRuntimeFilterGroup(left) || isRuntimeFilterGroup(right)) { + return ( + isRuntimeFilterGroup(left) && + isRuntimeFilterGroup(right) && + filterDescriptorKey(left) === filterDescriptorKey(right) + ); + } + return ( + left.columnId === right.columnId && + left.operator === right.operator && + semanticValueEqual(left.value, right.value) + ); +} + function filtersEqual( - left: readonly RuntimeFilter[], - right: readonly RuntimeFilter[], + left: readonly RuntimeFilterNode[], + right: readonly RuntimeFilterNode[], ): boolean { if (left.length !== right.length) return false; const used = new Set(); return left.every((filter) => { const index = right.findIndex( (candidate, candidateIndex) => - !used.has(candidateIndex) && - filter.columnId === candidate.columnId && - filter.operator === candidate.operator && - semanticValueEqual(filter.value, candidate.value), + !used.has(candidateIndex) && filterNodesEqual(filter, candidate), ); if (index < 0) return false; used.add(index); @@ -939,7 +1035,9 @@ function derivationsEqualForPlan( if (left.length !== right.length) return false; const accessorIds = new Set(); const comparatorIds = new Set(); - query.filters.forEach((entry) => accessorIds.add(entry.columnId)); + filterLeavesOf(query.filters).forEach((entry) => + accessorIds.add(entry.columnId), + ); query.sort.forEach((entry) => { accessorIds.add(entry.columnId); comparatorIds.add(entry.columnId); @@ -1125,15 +1223,29 @@ function snapshotQuery( path: string, canonicalFilters = false, ): RuntimeQuery { - const filters = query.filters.map((filter, index) => - Object.freeze({ - ...filter, - value: cloneOwnedValue( - filter.value, - `${path}.filters[${index}].value`, - new WeakSet(), - ), - }), + const snapshotNode = ( + node: RuntimeFilterNode, + nodePath: string, + ): RuntimeFilterNode => + isRuntimeFilterGroup(node) + ? Object.freeze({ + op: node.op, + children: Object.freeze( + node.children.map((child, index) => + snapshotNode(child, `${nodePath}.children[${index}]`), + ), + ), + }) + : Object.freeze({ + ...node, + value: cloneOwnedValue( + node.value, + `${nodePath}.value`, + new WeakSet(), + ), + }); + const filters = query.filters.map((node, index) => + snapshotNode(node, `${path}.filters[${index}]`), ); if (canonicalFilters) filters.sort(compareFilterDescriptors); return Object.freeze({ @@ -1145,7 +1257,7 @@ function snapshotQuery( }); } -const EMPTY_FILTERS = Object.freeze([]) as readonly RuntimeFilter[]; +const EMPTY_FILTERS = Object.freeze([]) as readonly RuntimeFilterNode[]; const EMPTY_SORT = Object.freeze([]) as RuntimeQuery["sort"]; /** @@ -1176,14 +1288,19 @@ function canonicalRuntimeQuery( } function compareFilterDescriptors( - left: RuntimeFilter, - right: RuntimeFilter, + left: RuntimeFilterNode, + right: RuntimeFilterNode, ): number { return filterDescriptorKey(left).localeCompare(filterDescriptorKey(right)); } -function filterDescriptorKey(filter: RuntimeFilter): string { - return `${filter.columnId}\u0000${filter.operator}\u0000${filterValueKey(filter.value)}`; +function filterDescriptorKey(node: RuntimeFilterNode): string { + if (isRuntimeFilterGroup(node)) { + return `group\u0000${node.op}\u0000[${node.children + .map(filterDescriptorKey) + .join("\u0001")}]`; + } + return `${node.columnId}\u0000${node.operator}\u0000${filterValueKey(node.value)}`; } function filterValueKey(value: unknown): string { @@ -1458,6 +1575,7 @@ class CompiledQueryPlan // Parallel to `#runtimeQuery.filters`: one compiled predicate per filter, // built once at construction so no verdict ever re-normalizes operands or // re-resolves columns per row. + readonly #filterLeaves: readonly RuntimeFilter[]; readonly #compiledPredicates: readonly FilterPredicate[]; readonly #active: readonly RuntimeColumn[]; readonly #aggregateColumns: readonly RuntimeColumn[]; @@ -1529,13 +1647,12 @@ class CompiledQueryPlan this.#byId = new Map( this.#runtimeColumns.map((column) => [column.id, column]), ); - this.#compiledPredicates = this.#runtimeQuery.filters.map((filter) => + this.#filterLeaves = filterLeavesOf(this.#runtimeQuery.filters); + this.#compiledPredicates = this.#filterLeaves.map((filter) => compileFilterPredicate(filter, this.#byId.get(filter.columnId)!), ); const activeIds = new Set(); - this.#runtimeQuery.filters.forEach((entry) => - activeIds.add(entry.columnId), - ); + this.#filterLeaves.forEach((entry) => activeIds.add(entry.columnId)); this.#runtimeQuery.rowGroups.forEach((entry) => activeIds.add(entry.columnId), ); @@ -1708,7 +1825,7 @@ class CompiledQueryPlan * re-normalization per row. */ #filterVerdict(valueOf: (columnId: string) => unknown): boolean { - const filters = this.#runtimeQuery.filters; + const filters = this.#filterLeaves; return this.#compiledPredicates.every((predicate, index) => predicate(valueOf(filters[index].columnId)), ); From 852fe626544e685d65d4de67f1dbae35a4a6302a Mon Sep 17 00:00:00 2001 From: Brian Love Date: Tue, 25 Aug 2026 19:24:55 -0700 Subject: [PATCH 04/15] =?UTF-8?q?docs:=20correct=20the=20SP2a=20task=20ord?= =?UTF-8?q?er=20=E2=80=94=20react=20before=20api=20reports?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 --- docs/superpowers/plans/2026-08-25-filter-tree-sp2a.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/superpowers/plans/2026-08-25-filter-tree-sp2a.md b/docs/superpowers/plans/2026-08-25-filter-tree-sp2a.md index b21714a1f..c930a97b2 100644 --- a/docs/superpowers/plans/2026-08-25-filter-tree-sp2a.md +++ b/docs/superpowers/plans/2026-08-25-filter-tree-sp2a.md @@ -105,6 +105,13 @@ export function isPretableFilterGroup( ### Task 3: API reports and the core changeset +> **ORDERING CORRECTION (2026-08-25, after Task 1):** widening `PretableQueryFor.filters` +> breaks `@pretable/react`'s typecheck at its leaf-assuming sites — exactly the flush-out +> the plan predicted, but it means **`pnpm build` cannot succeed until Task 4 lands**, and +> this task depends on a successful build. **Execute Task 4 BEFORE Task 3**, and let Task 3 +> then cover both packages' reports in one pass instead of regenerating twice. + + - [ ] **Step 1:** `pnpm build && pnpm api && pnpm api:check`. Expected surfacing: `PretableFilterGroupFor`, `isPretableFilterGroup`, the widened `filters` element type — in `core.api.md` and `react.api.md` (the query types flow through both). Anything else surfacing is a stop-and-report. - [ ] **Step 2:** Changeset `@pretable/core` minor: the group node, the guard, implicit-AND array preserved, empty-group-TRUE semantics named (a CHANGELOG reader must learn that rule here). - [ ] **Step 3: Commit** `chore: api reports and changeset for filter groups`. From 3568a1b46d4b993156a9f78482aadbb458bf53ec Mon Sep 17 00:00:00 2001 From: Brian Love Date: Tue, 25 Aug 2026 19:34:21 -0700 Subject: [PATCH 05/15] fix(row-model): group-aware distinct-values keys, and a freeze test that tests capture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `filterSemanticKey` cast every filter to a leaf, so a group keyed as three `undefined`s: two queries differing only INSIDE a group collided on one `population: "filtered"` cache entry and the second was served the first one's answer. The key now recurses, sorting each group's child keys the way `canonicalRuntimeQuery` sorts the roots — both joins are commutative, so a reordered group is the same question. `filterDescriptorKey` sorts children for the same reason, which also lets a reordered group reuse its plan. The deep-freeze test read `plan.query`, which re-freezes everything it hands back: it proved `snapshotQuery` froze a copy and said nothing about capture, and passed with the capture-level freeze deleted. It now reads the plan's own captured tree through an internal test seam, with the snapshot half split off into its own test. Also corrects two comments that still called the compiled predicates parallel to `#runtimeQuery.filters` — under a tree they are parallel to `#filterLeaves` — and marks the temporary conjunctive flattening at both sites. Co-Authored-By: Claude Opus 5 --- .../src/__tests__/distinct-values.test.ts | 139 ++++++++++++++++++ .../src/__tests__/filter-tree.test.ts | 45 +++++- packages/row-model/src/compiled-query.ts | 44 +++++- packages/row-model/src/distinct-values.ts | 41 ++++-- 4 files changed, 247 insertions(+), 22 deletions(-) diff --git a/packages/row-model/src/__tests__/distinct-values.test.ts b/packages/row-model/src/__tests__/distinct-values.test.ts index bdba28bbe..50e2879cc 100644 --- a/packages/row-model/src/__tests__/distinct-values.test.ts +++ b/packages/row-model/src/__tests__/distinct-values.test.ts @@ -560,6 +560,145 @@ describe("bounded distinct-value dictionaries", () => { }); }); + test("distinguishes filter values that differ only INSIDE a group", async () => { + interface FilterRow { + id: number; + primary: string; + secondary: string; + } + const filterHelper = createColumnHelper(); + const filterColumns = [ + filterHelper.accessor("primary", { type: "enum" }), + filterHelper.accessor("secondary", { type: "enum" }), + ] as const; + const scheduler = new ManualScheduler(); + const model = createLocalRowModel({ + rows: [ + { id: 1, primary: "a", secondary: "x" }, + { id: 2, primary: "b", secondary: "x" }, + ], + columns: filterColumns, + query: { + filters: [ + { + op: "and", + children: [ + { columnId: "primary", operator: "isAnyOf", value: ["a"] }, + { columnId: "secondary", operator: "isAnyOf", value: ["x"] }, + ], + }, + ], + sort: [], + rowGroups: [], + }, + transitionScheduler: scheduler, + transitionClock: tickingClock(), + transitionBudgetMs: 1, + transitionMaxUnitsPerSlice: 1, + }); + const initial = model.distinctValues("primary", { + population: "filtered", + limit: 10, + }); + scheduler.flushAll(); + await expect(initial.finished).resolves.toMatchObject({ + values: [{ value: "a", count: 1 }], + }); + + // Identical outside the group; only a child operand changed. A key that + // does not recurse collapses both queries onto one cache entry and serves + // the first answer back. + const changed = model.setQuery({ + filters: [ + { + op: "and", + children: [ + { columnId: "primary", operator: "isAnyOf", value: ["b"] }, + { columnId: "secondary", operator: "isAnyOf", value: ["x"] }, + ], + }, + ], + sort: [], + rowGroups: [], + }); + scheduler.flushAll(); + await changed.finished; + const rebuilt = model.distinctValues("primary", { + population: "filtered", + limit: 10, + }); + expect(rebuilt.status).toBe("pending"); + scheduler.flushAll(); + await expect(rebuilt.finished).resolves.toMatchObject({ + values: [{ value: "b", count: 1 }], + }); + }); + + test("reuses the cache when a group's children are merely reordered", async () => { + interface FilterRow { + id: number; + primary: string; + secondary: string; + } + const filterHelper = createColumnHelper(); + const filterColumns = [ + filterHelper.accessor("primary", { type: "enum" }), + filterHelper.accessor("secondary", { type: "enum" }), + ] as const; + const scheduler = new ManualScheduler(); + const model = createLocalRowModel({ + rows: [ + { id: 1, primary: "a", secondary: "x" }, + { id: 2, primary: "b", secondary: "x" }, + ], + columns: filterColumns, + query: { + filters: [ + { + op: "and", + children: [ + { columnId: "primary", operator: "isAnyOf", value: ["a"] }, + { columnId: "secondary", operator: "isAnyOf", value: ["x"] }, + ], + }, + ], + sort: [], + rowGroups: [], + }, + transitionScheduler: scheduler, + transitionClock: tickingClock(), + transitionBudgetMs: 1, + transitionMaxUnitsPerSlice: 1, + }); + const initial = model.distinctValues("primary", { + population: "filtered", + limit: 10, + }); + scheduler.flushAll(); + await initial.finished; + + const reordered = model.setQuery({ + filters: [ + { + op: "and", + children: [ + { columnId: "secondary", operator: "isAnyOf", value: ["x"] }, + { columnId: "primary", operator: "isAnyOf", value: ["a"] }, + ], + }, + ], + sort: [], + rowGroups: [], + }); + await expect(reordered.finished).resolves.toBe(0); + expect( + model.distinctValues("primary", { + population: "filtered", + limit: 10, + }).status, + ).toBe("ready"); + }); + test("supports bounded search and ranges with explicit blank inclusion and ordering", async () => { const scheduler = new ManualScheduler(); const rows: readonly Row[] = [ diff --git a/packages/row-model/src/__tests__/filter-tree.test.ts b/packages/row-model/src/__tests__/filter-tree.test.ts index f04e6b7f2..42e1fd91e 100644 --- a/packages/row-model/src/__tests__/filter-tree.test.ts +++ b/packages/row-model/src/__tests__/filter-tree.test.ts @@ -4,6 +4,7 @@ import { CompiledQueryValidationError, compileQuery, createColumnHelper, + getCapturedFilterTreeForTesting, isPretableFilterGroup, type PretableQueryFor, } from "../index"; @@ -188,7 +189,7 @@ describe("capture of a filter tree", () => { }); }); - test("deep-freezes every level of the captured tree", () => { + test("deep-freezes every level of the tree it captured", () => { const plan = compile( queryFor({ filters: [ @@ -207,17 +208,25 @@ describe("capture of a filter tree", () => { }), ); - const outer = plan.query.filters[0]; + // Deliberately NOT `plan.query`: that getter re-freezes everything it + // hands back, so it proves `snapshotQuery` froze a copy and says nothing + // about capture. This reads the plan's own captured tree. + const captured = getCapturedFilterTreeForTesting(plan) as readonly Node[]; + const outer = captured[0]; if (!isPretableFilterGroup(outer)) throw new Error("unreachable"); const inner = outer.children[0]; if (!isPretableFilterGroup(inner)) throw new Error("unreachable"); const leaf = inner.children[0]; + expect(Object.isFrozen(captured)).toBe(true); expect(Object.isFrozen(outer)).toBe(true); expect(Object.isFrozen(outer.children)).toBe(true); expect(Object.isFrozen(inner)).toBe(true); expect(Object.isFrozen(inner.children)).toBe(true); expect(Object.isFrozen(leaf)).toBe(true); + expect(() => { + (outer as { op: string }).op = "or"; + }).toThrow(TypeError); expect(() => { (inner as { op: string }).op = "and"; }).toThrow(TypeError); @@ -226,6 +235,38 @@ describe("capture of a filter tree", () => { }).toThrow(TypeError); }); + test("re-freezes every level of the tree it publishes", () => { + const plan = compile( + queryFor({ + filters: [ + { + op: "and", + children: [ + { + op: "or", + children: [{ columnId: "sector", operator: "isEmpty" }], + }, + ], + }, + ], + sort: [], + rowGroups: [], + }), + ); + + const outer = plan.query.filters[0]; + if (!isPretableFilterGroup(outer)) throw new Error("unreachable"); + const inner = outer.children[0]; + if (!isPretableFilterGroup(inner)) throw new Error("unreachable"); + + expect(Object.isFrozen(outer)).toBe(true); + expect(Object.isFrozen(inner)).toBe(true); + expect(Object.isFrozen(inner.children[0])).toBe(true); + expect(() => { + (inner as { op: string }).op = "and"; + }).toThrow(TypeError); + }); + test("copies the incoming tree rather than retaining it", () => { const child = { columnId: "sector", operator: "isEmpty" }; const group = { op: "and", children: [child] }; diff --git a/packages/row-model/src/compiled-query.ts b/packages/row-model/src/compiled-query.ts index f7141251a..2dfebfdcb 100644 --- a/packages/row-model/src/compiled-query.ts +++ b/packages/row-model/src/compiled-query.ts @@ -1296,8 +1296,12 @@ function compareFilterDescriptors( function filterDescriptorKey(node: RuntimeFilterNode): string { if (isRuntimeFilterGroup(node)) { + // Children are keyed then sorted for the same reason the roots are + // sorted in `canonicalRuntimeQuery`: both joins are commutative, so a + // reordered group is the same question and must reuse the same plan. return `group\u0000${node.op}\u0000[${node.children .map(filterDescriptorKey) + .sort() .join("\u0001")}]`; } return `${node.columnId}\u0000${node.operator}\u0000${filterValueKey(node.value)}`; @@ -1572,10 +1576,16 @@ class CompiledQueryPlan readonly #runtimeColumns: readonly RuntimeColumn[]; readonly #runtimeQuery: RuntimeQuery; readonly #byId: ReadonlyMap; - // Parallel to `#runtimeQuery.filters`: one compiled predicate per filter, - // built once at construction so no verdict ever re-normalizes operands or - // re-resolves columns per row. + /* + * The LEAVES of `#runtimeQuery.filters`, depth-first — not the filter list + * itself, which under a tree holds groups and has a different length. + * SP2a task 2: every leaf is applied conjunctively regardless of the group + * it sits in, so an `or` group currently behaves as an `and`. + */ readonly #filterLeaves: readonly RuntimeFilter[]; + // Parallel to `#filterLeaves`: one compiled predicate per leaf, built once + // at construction so no verdict ever re-normalizes operands or re-resolves + // columns per row. readonly #compiledPredicates: readonly FilterPredicate[]; readonly #active: readonly RuntimeColumn[]; readonly #aggregateColumns: readonly RuntimeColumn[]; @@ -1821,8 +1831,11 @@ class CompiledQueryPlan * map, the verdict-only path supplies live accessor reads. Predicate * semantics live in `compileFilterPredicate`, applied here through the * construction-time `#compiledPredicates` array (parallel to - * `#runtimeQuery.filters`) — no `#byId` lookup and no operand - * re-normalization per row. + * `#filterLeaves`, NOT to `#runtimeQuery.filters`) — no `#byId` lookup and + * no operand re-normalization per row. + * + * SP2a task 2: this is a flat conjunction over every leaf in the tree. Join + * operators are not honoured yet, so an `or` group evaluates as an `and`. */ #filterVerdict(valueOf: (columnId: string) => unknown): boolean { const filters = this.#filterLeaves; @@ -1847,6 +1860,15 @@ class CompiledQueryPlan * memo belongs to the plan that wrote it, so this plan re-reads accessors * rather than repeating a verdict its own filters never produced. */ + static capturedFilterTreeForTesting( + plan: CompiledQuery, + ): readonly unknown[] { + if (!(plan instanceof CompiledQueryPlan)) { + throw new TypeError("Captured filters require a compiled query plan."); + } + return plan.#publicQuery.filters; + } + static filterVerdict( plan: unknown, input: CompiledRowInput, TRowId>, @@ -2296,6 +2318,18 @@ export function compareRecordRows( * resolve keys once per row instead of once per comparison; * `compareRecordRows` remains the general entry with identical semantics. */ +/** + * The plan's OWN captured filter tree — the objects `captureFilterNode` + * produced, not the re-frozen copy the `query` getter hands out. Exists so + * capture-level invariants (freezing, ownership) are assertable at all. + * @internal + */ +export function getCapturedFilterTreeForTesting( + plan: CompiledQuery, +): readonly unknown[] { + return CompiledQueryPlan.capturedFilterTreeForTesting(plan); +} + export function compareWithSortKeys( plan: CompiledQuery, left: CompiledRowInput, TRowId>, diff --git a/packages/row-model/src/distinct-values.ts b/packages/row-model/src/distinct-values.ts index 33c8ba9a5..0ac047dc2 100644 --- a/packages/row-model/src/distinct-values.ts +++ b/packages/row-model/src/distinct-values.ts @@ -1,4 +1,4 @@ -import type { PretableRowId } from "./column-types"; +import { isPretableFilterGroup, type PretableRowId } from "./column-types"; import { runCooperativeTransitionSlice, type CooperativeTransitionRuntime, @@ -421,23 +421,34 @@ function filterSemanticKey< const derivations = root.queryPlan .derivations as unknown as readonly RuntimeColumn[]; const byId = new Map(derivations.map((column) => [column.id, column])); - return [...root.queryPlan.query.filters] - .map((filter) => { - const runtime = filter as { - readonly columnId: string; - readonly operator: string; - readonly value?: unknown; + const nodeKey = (node: unknown): string => { + if (isPretableFilterGroup(node as never)) { + const group = node as { + readonly op: string; + readonly children: readonly unknown[]; }; + // Children are keyed then sorted for the same reason the roots are: + // and/or are commutative, so reordering a group is not a new question. return frame( - "f", - frame("c", runtime.columnId) + - frame("i", String(identityId(byId.get(runtime.columnId)?.accessor))) + - frame("p", runtime.operator) + - frame("v", semanticValueKey(runtime.value)), + "g", + frame("p", group.op) + + frame("k", group.children.map(nodeKey).sort().join("")), ); - }) - .sort() - .join(""); + } + const runtime = node as { + readonly columnId: string; + readonly operator: string; + readonly value?: unknown; + }; + return frame( + "f", + frame("c", runtime.columnId) + + frame("i", String(identityId(byId.get(runtime.columnId)?.accessor))) + + frame("p", runtime.operator) + + frame("v", semanticValueKey(runtime.value)), + ); + }; + return [...root.queryPlan.query.filters].map(nodeKey).sort().join(""); } function columnForDerivations( From 55100e13be601d9b6ced178c34787387d8db7657 Mon Sep 17 00:00:00 2001 From: Brian Love Date: Tue, 25 Aug 2026 19:43:18 -0700 Subject: [PATCH 06/15] fix(row-model): filter groups compare structurally, so a value cannot forge a sibling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The group arm of `filterNodesEqual` compared serialized descriptor keys, and that key is raw concatenation over unframed user operands. A filter VALUE could reproduce the separators and impersonate a sibling: an `and` group of `contains "a"` and `contains "b"` keyed identically to a one-child group whose operand was `asectorcontainsstring:b`. The keys matched, so `filtersEqual` and then `semanticallyMatches` did too, `compileQuery` handed back the PREVIOUS plan, and the incoming query was silently discarded with the old filters left applied. Groups now match structurally — join operator, then children as an unordered multiset through `filtersEqual`, recursing for nested groups. That is the comparison the tree needs anyway, brought forward rather than patched around. `filterDescriptorKey` keeps its original ordering job, where the ambiguity is harmless. Also documents why the child-key sort in `distinct-values`'s `nodeKey` is belt-and-braces: structural plan reuse absorbs a reordered group before this cache can see one, and the sort stops being redundant if that changes. Co-Authored-By: Claude Opus 5 --- .../src/__tests__/filter-tree.test.ts | 94 +++++++++++++++++++ packages/row-model/src/compiled-query.ts | 16 +++- packages/row-model/src/distinct-values.ts | 12 ++- 3 files changed, 115 insertions(+), 7 deletions(-) diff --git a/packages/row-model/src/__tests__/filter-tree.test.ts b/packages/row-model/src/__tests__/filter-tree.test.ts index 42e1fd91e..b988a94fe 100644 --- a/packages/row-model/src/__tests__/filter-tree.test.ts +++ b/packages/row-model/src/__tests__/filter-tree.test.ts @@ -39,6 +39,14 @@ function compile(query: unknown) { } as never); } +function recompile(query: unknown, previous: unknown) { + return compileQuery({ + derivations: columns, + query, + previous, + } as never); +} + function caughtFrom(query: unknown): unknown { try { compile(query); @@ -296,3 +304,89 @@ describe("capture of a filter tree", () => { ).toBe(true); }); }); + +describe("group identity", () => { + const groupOf = (children: readonly unknown[]) => ({ + op: "and", + children, + }); + const containing = (value: string) => ({ + columnId: "sector", + operator: "contains", + value, + }); + const queryOf = (children: readonly unknown[]) => ({ + filters: [groupOf(children)], + sort: [], + rowGroups: [], + }); + + test("rebuilds when a group child's value changes", () => { + const first = compile(queryOf([containing("a"), containing("b")])); + expect(recompile(queryOf([containing("a"), containing("b")]), first)).toBe( + first, + ); + expect( + recompile(queryOf([containing("a"), containing("c")]), first), + ).not.toBe(first); + }); + + test("reuses a plan when a group's children are merely reordered", () => { + const first = compile(queryOf([containing("a"), containing("b")])); + expect(recompile(queryOf([containing("b"), containing("a")]), first)).toBe( + first, + ); + }); + + test("a child value cannot forge a sibling and impersonate a larger group", () => { + // Built against the concatenated descriptor key: a leaf keys as + // `columnId\0operator\0string:` and a group joins its children + // with \u0001, none of it length-framed. This single operand reproduces + // the two-child key above byte for byte. + const forged = containing("a\u0001sector\u0000contains\u0000string:b"); + const first = compile(queryOf([containing("a"), containing("b")])); + + expect(recompile(queryOf([forged]), first)).not.toBe(first); + }); + + test("distinguishes groups that differ only in their join operator", () => { + const children = [containing("a"), containing("b")]; + const first = compile({ + filters: [{ op: "and", children }], + sort: [], + rowGroups: [], + }); + expect( + recompile( + { filters: [{ op: "or", children }], sort: [], rowGroups: [] }, + first, + ), + ).not.toBe(first); + }); + + test("recurses into nested groups rather than stopping at the top", () => { + const nested = (value: string) => ({ + filters: [ + { + op: "and", + children: [ + containing("a"), + { op: "or", children: [containing(value)] }, + ], + }, + ], + sort: [], + rowGroups: [], + }); + const first = compile(nested("deep")); + expect(recompile(nested("deep"), first)).toBe(first); + expect(recompile(nested("deeper"), first)).not.toBe(first); + }); + + test("never matches a group against a leaf", () => { + const first = compile(queryOf([containing("a")])); + expect( + recompile({ filters: [containing("a")], sort: [], rowGroups: [] }, first), + ).not.toBe(first); + }); +}); diff --git a/packages/row-model/src/compiled-query.ts b/packages/row-model/src/compiled-query.ts index 2dfebfdcb..29282ed5a 100644 --- a/packages/row-model/src/compiled-query.ts +++ b/packages/row-model/src/compiled-query.ts @@ -987,10 +987,15 @@ function queryEqual(left: RuntimeQuery, right: RuntimeQuery): boolean { } /* - * Leaves are matched unordered and semantically. Groups are matched by their - * descriptor key, which is deliberately conservative — a reordered group - * reports "changed" rather than risking a plan reuse it did not earn. SP2a - * task 2 owns real tree equality and should replace the group arm. + * Nodes are matched STRUCTURALLY, never by a serialized key: the descriptor + * key is raw concatenation over unframed user operands, so a filter value can + * forge the separators and impersonate a sibling — harmless for the ordering + * job the key exists for, a wrong-results bug as an identity test (the plan + * would be reused and the incoming query silently discarded). + * + * Groups match when their join operators match and their children match as an + * unordered multiset, recursively — the same used-set shape the leaf list uses + * one level up, for the same reason: both joins are commutative. */ function filterNodesEqual( left: RuntimeFilterNode, @@ -1000,7 +1005,8 @@ function filterNodesEqual( return ( isRuntimeFilterGroup(left) && isRuntimeFilterGroup(right) && - filterDescriptorKey(left) === filterDescriptorKey(right) + left.op === right.op && + filtersEqual(left.children, right.children) ); } return ( diff --git a/packages/row-model/src/distinct-values.ts b/packages/row-model/src/distinct-values.ts index 0ac047dc2..7202270db 100644 --- a/packages/row-model/src/distinct-values.ts +++ b/packages/row-model/src/distinct-values.ts @@ -427,8 +427,16 @@ function filterSemanticKey< readonly op: string; readonly children: readonly unknown[]; }; - // Children are keyed then sorted for the same reason the roots are: - // and/or are commutative, so reordering a group is not a new question. + /* + * Children are keyed then sorted for the same reason the roots are: + * and/or are commutative, so reordering a group is not a new question. + * + * Belt and braces today — deleting the sort breaks no test, because a + * reordered group is matched structurally by `filterNodesEqual` and so + * reuses its plan, and this cache never sees a root whose tree differs + * only in child order. It stops being redundant the moment plan reuse + * stops absorbing that case. + */ return frame( "g", frame("p", group.op) + From 919cb170e66595b3e7784a43ac3666ac9894a84c Mon Sep 17 00:00:00 2001 From: Brian Love Date: Tue, 25 Aug 2026 19:56:07 -0700 Subject: [PATCH 07/15] refactor(row-model): honest docs, a named node union, and an allocation-free recompile check Three comment defects this branch introduced. Two new functions had been inserted BETWEEN an existing doc block and the function it documented, so `filterVerdict`'s paragraph described the test seam and `compareWithSortKeys` was left undocumented under a stacked pair; both are moved clear. `isRuntimeFilterGroup` claimed the "same positive check" as the public guard when it is deliberately weaker, and now says why: capture has already rejected any node carrying `children` without a valid `op`, so over a captured tree `children` alone is decisive. `PretableFilterGroupFor`'s public TSDoc read as a finished feature, and now carries the one line saying `op: "or"` is accepted and validated but still evaluated as `and`. `PretableFilterNodeFor` names the leaf-or-group union that was spelled out at three sites and reinvented locally in the tests. The near-identical `filtersEqual`/`filterNodesEqual` pair becomes `filterNodeListEqual` and `filterNodeEqual`. `derivationsEqualForPlan` now takes the caller's `#filterLeaves` instead of re-deriving them: it runs on every recompile check, where the pre-tree code allocated nothing. `filterLeavesOf` is once again called exactly once per plan, in the constructor. Also drops a guard ternary equivalent to the equality checks it wrapped, drops an object check `validateFilter` already performs, corrects two wording drifts (captured vs published, leaf list vs node list), extracts a model factory in the distinct-values group tests, and turns a bare assertion loop into `test.each` so a failure names the shape. Co-Authored-By: Claude Opus 5 --- .../src/__tests__/distinct-values.test.ts | 186 +++++++----------- .../src/__tests__/filter-tree.test.ts | 29 ++- packages/row-model/src/column-types.ts | 27 +-- packages/row-model/src/compiled-query.ts | 76 ++++--- 4 files changed, 145 insertions(+), 173 deletions(-) diff --git a/packages/row-model/src/__tests__/distinct-values.test.ts b/packages/row-model/src/__tests__/distinct-values.test.ts index 50e2879cc..198b2169a 100644 --- a/packages/row-model/src/__tests__/distinct-values.test.ts +++ b/packages/row-model/src/__tests__/distinct-values.test.ts @@ -560,7 +560,7 @@ describe("bounded distinct-value dictionaries", () => { }); }); - test("distinguishes filter values that differ only INSIDE a group", async () => { + describe("filter groups", () => { interface FilterRow { id: number; primary: string; @@ -571,132 +571,86 @@ describe("bounded distinct-value dictionaries", () => { filterHelper.accessor("primary", { type: "enum" }), filterHelper.accessor("secondary", { type: "enum" }), ] as const; - const scheduler = new ManualScheduler(); - const model = createLocalRowModel({ - rows: [ - { id: 1, primary: "a", secondary: "x" }, - { id: 2, primary: "b", secondary: "x" }, - ], - columns: filterColumns, - query: { - filters: [ - { - op: "and", - children: [ - { columnId: "primary", operator: "isAnyOf", value: ["a"] }, - { columnId: "secondary", operator: "isAnyOf", value: ["x"] }, - ], - }, - ], - sort: [], - rowGroups: [], - }, - transitionScheduler: scheduler, - transitionClock: tickingClock(), - transitionBudgetMs: 1, - transitionMaxUnitsPerSlice: 1, - }); - const initial = model.distinctValues("primary", { - population: "filtered", - limit: 10, - }); - scheduler.flushAll(); - await expect(initial.finished).resolves.toMatchObject({ - values: [{ value: "a", count: 1 }], - }); - - // Identical outside the group; only a child operand changed. A key that - // does not recurse collapses both queries onto one cache entry and serves - // the first answer back. - const changed = model.setQuery({ - filters: [ + const andGroup = (primary: readonly string[]) => ({ + op: "and" as const, + children: [ + { + columnId: "primary" as const, + operator: "isAnyOf" as const, + value: primary, + }, { - op: "and", - children: [ - { columnId: "primary", operator: "isAnyOf", value: ["b"] }, - { columnId: "secondary", operator: "isAnyOf", value: ["x"] }, - ], + columnId: "secondary" as const, + operator: "isAnyOf" as const, + value: ["x"], }, ], - sort: [], - rowGroups: [], }); - scheduler.flushAll(); - await changed.finished; - const rebuilt = model.distinctValues("primary", { - population: "filtered", - limit: 10, - }); - expect(rebuilt.status).toBe("pending"); - scheduler.flushAll(); - await expect(rebuilt.finished).resolves.toMatchObject({ - values: [{ value: "b", count: 1 }], - }); - }); - test("reuses the cache when a group's children are merely reordered", async () => { - interface FilterRow { - id: number; - primary: string; - secondary: string; - } - const filterHelper = createColumnHelper(); - const filterColumns = [ - filterHelper.accessor("primary", { type: "enum" }), - filterHelper.accessor("secondary", { type: "enum" }), - ] as const; - const scheduler = new ManualScheduler(); - const model = createLocalRowModel({ - rows: [ - { id: 1, primary: "a", secondary: "x" }, - { id: 2, primary: "b", secondary: "x" }, - ], - columns: filterColumns, - query: { - filters: [ - { - op: "and", - children: [ - { columnId: "primary", operator: "isAnyOf", value: ["a"] }, - { columnId: "secondary", operator: "isAnyOf", value: ["x"] }, - ], - }, + function makeModel(filters: readonly unknown[]) { + const scheduler = new ManualScheduler(); + const model = createLocalRowModel({ + rows: [ + { id: 1, primary: "a", secondary: "x" }, + { id: 2, primary: "b", secondary: "x" }, ], + columns: filterColumns, + query: { filters, sort: [], rowGroups: [] } as never, + transitionScheduler: scheduler, + transitionClock: tickingClock(), + transitionBudgetMs: 1, + transitionMaxUnitsPerSlice: 1, + }); + const distinctPrimary = () => + model.distinctValues("primary", { population: "filtered", limit: 10 }); + return { model, scheduler, distinctPrimary }; + } + + test("distinguishes filter values that differ only INSIDE a group", async () => { + const { model, scheduler, distinctPrimary } = makeModel([ + andGroup(["a"]), + ]); + const initial = distinctPrimary(); + scheduler.flushAll(); + await expect(initial.finished).resolves.toMatchObject({ + values: [{ value: "a", count: 1 }], + }); + + // Identical outside the group; only a child operand changed. A cache key + // that does not recurse collapses both queries onto one entry and serves + // the first answer back. + const changed = model.setQuery({ + filters: [andGroup(["b"])], sort: [], rowGroups: [], - }, - transitionScheduler: scheduler, - transitionClock: tickingClock(), - transitionBudgetMs: 1, - transitionMaxUnitsPerSlice: 1, - }); - const initial = model.distinctValues("primary", { - population: "filtered", - limit: 10, + } as never); + scheduler.flushAll(); + await changed.finished; + const rebuilt = distinctPrimary(); + expect(rebuilt.status).toBe("pending"); + scheduler.flushAll(); + await expect(rebuilt.finished).resolves.toMatchObject({ + values: [{ value: "b", count: 1 }], + }); }); - scheduler.flushAll(); - await initial.finished; - const reordered = model.setQuery({ - filters: [ - { - op: "and", - children: [ - { columnId: "secondary", operator: "isAnyOf", value: ["x"] }, - { columnId: "primary", operator: "isAnyOf", value: ["a"] }, - ], - }, - ], - sort: [], - rowGroups: [], + test("reuses the cache when a group's children are merely reordered", async () => { + const { model, scheduler, distinctPrimary } = makeModel([ + andGroup(["a"]), + ]); + const initial = distinctPrimary(); + scheduler.flushAll(); + await initial.finished; + + const group = andGroup(["a"]); + const reordered = model.setQuery({ + filters: [{ op: "and", children: [...group.children].reverse() }], + sort: [], + rowGroups: [], + } as never); + await expect(reordered.finished).resolves.toBe(0); + expect(distinctPrimary().status).toBe("ready"); }); - await expect(reordered.finished).resolves.toBe(0); - expect( - model.distinctValues("primary", { - population: "filtered", - limit: 10, - }).status, - ).toBe("ready"); }); test("supports bounded search and ranges with explicit blank inclusion and ordering", async () => { diff --git a/packages/row-model/src/__tests__/filter-tree.test.ts b/packages/row-model/src/__tests__/filter-tree.test.ts index b988a94fe..a46346df0 100644 --- a/packages/row-model/src/__tests__/filter-tree.test.ts +++ b/packages/row-model/src/__tests__/filter-tree.test.ts @@ -6,6 +6,7 @@ import { createColumnHelper, getCapturedFilterTreeForTesting, isPretableFilterGroup, + type PretableFilterNodeFor, type PretableQueryFor, } from "../index"; @@ -26,7 +27,7 @@ const columns = [ ] as const; type Columns = typeof columns; -type Node = PretableQueryFor["filters"][number]; +type Node = PretableFilterNodeFor; function queryFor(value: PretableQueryFor): PretableQueryFor { return value; @@ -82,20 +83,18 @@ describe("isPretableFilterGroup", () => { expect(isPretableFilterGroup(leaf as Node)).toBe(false); }); - test("fails closed on shapes that are neither", () => { - for (const shape of [ - null, - undefined, - "and", - 42, - {}, - { op: "and" }, - { children: [] }, - { op: "nor", children: [] }, - { op: "AND", children: [] }, - ]) { - expect(isPretableFilterGroup(shape as never)).toBe(false); - } + test.each([ + ["null", null], + ["undefined", undefined], + ["a bare string", "and"], + ["a number", 42], + ["an empty object", {}], + ["a join with no children", { op: "and" }], + ["children with no join", { children: [] }], + ["an unknown join", { op: "nor", children: [] }], + ["a miscased join", { op: "AND", children: [] }], + ] as const)("fails closed on %s", (_shape, value) => { + expect(isPretableFilterGroup(value as never)).toBe(false); }); }); diff --git a/packages/row-model/src/column-types.ts b/packages/row-model/src/column-types.ts index def61212a..e7dc79faa 100644 --- a/packages/row-model/src/column-types.ts +++ b/packages/row-model/src/column-types.ts @@ -534,15 +534,24 @@ export type PretableFilterFor = /** * A branch of the filter tree: a join operator and the nodes it joins. A group * may hold leaves, further groups, or nothing at all, to arbitrary depth. + * + * NOT YET EVALUATED: the engine currently applies every leaf in the tree + * conjunctively, so an `op: "or"` group behaves as an `and`. Accepted and + * validated, but do not depend on the join until disjunction ships. * @public */ export interface PretableFilterGroupFor { readonly op: "and" | "or"; - readonly children: readonly ( - PretableFilterFor | PretableFilterGroupFor - )[]; + readonly children: readonly PretableFilterNodeFor[]; } +/** + * One node of the filter tree: either a typed leaf or a group of nodes. + * @public + */ +export type PretableFilterNodeFor = + PretableFilterFor | PretableFilterGroupFor; + /** * Narrows one filter-tree node to a group. Checked positively on the group's * own fields, so an unknown shape fails closed rather than being treated as a @@ -550,16 +559,14 @@ export interface PretableFilterGroupFor { * @public */ export function isPretableFilterGroup( - node: PretableFilterFor | PretableFilterGroupFor, + node: PretableFilterNodeFor, ): node is PretableFilterGroupFor { return ( typeof node === "object" && node !== null && "children" in node && - ("op" in node - ? (node as { op: unknown }).op === "and" || - (node as { op: unknown }).op === "or" - : false) + ((node as { op: unknown }).op === "and" || + (node as { op: unknown }).op === "or") ); } @@ -596,9 +603,7 @@ export type PretableRowGroupFor = Prettify< /** @public */ export interface PretableQueryFor { - readonly filters: readonly ( - PretableFilterFor | PretableFilterGroupFor - )[]; + readonly filters: readonly PretableFilterNodeFor[]; readonly sort: readonly PretableSortFor[]; readonly rowGroups: readonly PretableRowGroupFor[]; } diff --git a/packages/row-model/src/compiled-query.ts b/packages/row-model/src/compiled-query.ts index 29282ed5a..ad79947e3 100644 --- a/packages/row-model/src/compiled-query.ts +++ b/packages/row-model/src/compiled-query.ts @@ -262,8 +262,11 @@ interface RuntimeFilterGroup { type RuntimeFilterNode = RuntimeFilter | RuntimeFilterGroup; /** - * The internal twin of `isPretableFilterGroup`: same positive check on the - * group's own fields, over the already-captured runtime shape. + * The internal twin of `isPretableFilterGroup`, deliberately WEAKER: the + * public guard re-checks `op` because it runs on whatever a caller hands it, + * whereas capture has already rejected any node carrying `children` without a + * valid `op`. Over a captured tree the presence of `children` is therefore + * decisive on its own. Never call this on un-captured input. */ function isRuntimeFilterGroup( node: RuntimeFilterNode, @@ -607,7 +610,7 @@ function captureDenseArray( /** * One node of the filter tree. A node carrying `children` is a group and is * captured recursively, breadcrumbed as `.children[i]`; anything else is - * captured as a leaf. Every level is frozen on the way out, so the published + * captured as a leaf. Every level is frozen on the way out, so the captured * tree is owned by the plan rather than aliasing the caller's objects. */ function captureFilterNode(raw: unknown, path: string): RuntimeFilterNode { @@ -803,8 +806,6 @@ function validateFilterNode( columns: ReadonlyMap, path: string, ): void { - if (!node || typeof node !== "object") - fail("filter entry is not an object", path); if (isRuntimeFilterGroup(node)) { node.children.forEach((child, index) => validateFilterNode(child, columns, `${path}.children[${index}]`), @@ -980,7 +981,7 @@ function orderingEqual( function queryEqual(left: RuntimeQuery, right: RuntimeQuery): boolean { return ( - filtersEqual(left.filters, right.filters) && + filterNodeListEqual(left.filters, right.filters) && orderingEqual(left.sort, right.sort) && orderingEqual(left.rowGroups, right.rowGroups) ); @@ -994,10 +995,10 @@ function queryEqual(left: RuntimeQuery, right: RuntimeQuery): boolean { * would be reused and the incoming query silently discarded). * * Groups match when their join operators match and their children match as an - * unordered multiset, recursively — the same used-set shape the leaf list uses - * one level up, for the same reason: both joins are commutative. + * unordered multiset, recursively — the same used-set shape the node list one + * level up uses, for the same reason: both joins are commutative. */ -function filterNodesEqual( +function filterNodeEqual( left: RuntimeFilterNode, right: RuntimeFilterNode, ): boolean { @@ -1006,7 +1007,7 @@ function filterNodesEqual( isRuntimeFilterGroup(left) && isRuntimeFilterGroup(right) && left.op === right.op && - filtersEqual(left.children, right.children) + filterNodeListEqual(left.children, right.children) ); } return ( @@ -1016,7 +1017,7 @@ function filterNodesEqual( ); } -function filtersEqual( +function filterNodeListEqual( left: readonly RuntimeFilterNode[], right: readonly RuntimeFilterNode[], ): boolean { @@ -1025,7 +1026,7 @@ function filtersEqual( return left.every((filter) => { const index = right.findIndex( (candidate, candidateIndex) => - !used.has(candidateIndex) && filterNodesEqual(filter, candidate), + !used.has(candidateIndex) && filterNodeEqual(filter, candidate), ); if (index < 0) return false; used.add(index); @@ -1033,17 +1034,22 @@ function filtersEqual( }); } +/* + * `filterLeaves` is the caller's own `#filterLeaves`, passed rather than + * re-derived: this runs on every recompile check (`semanticallyMatches`, so + * every `setQuery`), and walking the tree here would allocate a fresh leaf + * array per call for a set the plan already holds. + */ function derivationsEqualForPlan( left: readonly RuntimeColumn[], right: readonly RuntimeColumn[], query: RuntimeQuery, + filterLeaves: readonly RuntimeFilter[], ): boolean { if (left.length !== right.length) return false; const accessorIds = new Set(); const comparatorIds = new Set(); - filterLeavesOf(query.filters).forEach((entry) => - accessorIds.add(entry.columnId), - ); + filterLeaves.forEach((entry) => accessorIds.add(entry.columnId)); query.sort.forEach((entry) => { accessorIds.add(entry.columnId); comparatorIds.add(entry.columnId); @@ -1624,6 +1630,7 @@ class CompiledQueryPlan this.#runtimeColumns, derivations, this.#runtimeQuery, + this.#filterLeaves, ) && queryEqual(this.#publicQuery, query), }; @@ -1850,6 +1857,20 @@ class CompiledQueryPlan ); } + /** + * The plan's OWN captured filter tree — the objects `captureFilterNode` + * produced, not the re-frozen copy the `query` getter hands out. + * @internal + */ + static capturedFilterTreeForTesting( + plan: CompiledQuery, + ): readonly unknown[] { + if (!(plan instanceof CompiledQueryPlan)) { + throw new TypeError("Captured filters require a compiled query plan."); + } + return plan.#publicQuery.filters; + } + /** * This plan's filter verdict for one row — accessor reads over the runtime * filter columns only, no metadata construction, no cache writes. Error @@ -1866,15 +1887,6 @@ class CompiledQueryPlan * memo belongs to the plan that wrote it, so this plan re-reads accessors * rather than repeating a verdict its own filters never produced. */ - static capturedFilterTreeForTesting( - plan: CompiledQuery, - ): readonly unknown[] { - if (!(plan instanceof CompiledQueryPlan)) { - throw new TypeError("Captured filters require a compiled query plan."); - } - return plan.#publicQuery.filters; - } - static filterVerdict( plan: unknown, input: CompiledRowInput, TRowId>, @@ -2214,14 +2226,16 @@ class CompiledQueryPlan previous.#runtimeColumns, next.#runtimeColumns, previous.#runtimeQuery, + previous.#filterLeaves, ) && derivationsEqualForPlan( previous.#runtimeColumns, next.#runtimeColumns, next.#runtimeQuery, + next.#filterLeaves, ) ); - const filtersChanged = !filtersEqual( + const filtersChanged = !filterNodeListEqual( previous.#runtimeQuery.filters, next.#runtimeQuery.filters, ); @@ -2318,12 +2332,6 @@ export function compareRecordRows( ); } -/** - * Orders two rows by keys the caller already resolved (via `sortKeysOf` or - * `fillSortKeysFromPrevious`) — no store lookups. Exists so O(n log n) sorts - * resolve keys once per row instead of once per comparison; - * `compareRecordRows` remains the general entry with identical semantics. - */ /** * The plan's OWN captured filter tree — the objects `captureFilterNode` * produced, not the re-frozen copy the `query` getter hands out. Exists so @@ -2336,6 +2344,12 @@ export function getCapturedFilterTreeForTesting( return CompiledQueryPlan.capturedFilterTreeForTesting(plan); } +/** + * Orders two rows by keys the caller already resolved (via `sortKeysOf` or + * `fillSortKeysFromPrevious`) — no store lookups. Exists so O(n log n) sorts + * resolve keys once per row instead of once per comparison; + * `compareRecordRows` remains the general entry with identical semantics. + */ export function compareWithSortKeys( plan: CompiledQuery, left: CompiledRowInput, TRowId>, From 4371b23986df974627565319f831e620f109c042 Mon Sep 17 00:00:00 2001 From: Brian Love Date: Tue, 25 Aug 2026 20:07:29 -0700 Subject: [PATCH 08/15] =?UTF-8?q?feat(row-model):=20filter=20trees=20evalu?= =?UTF-8?q?ate=20recursively=20=E2=80=94=20or=20finally=20means=20or?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task 1 accepted, validated and compared filter groups but flattened them for evaluation, so an `op: "or"` group behaved as an `and`. Evaluation now walks the join structure: the root list stays conjunctive, `and` groups hold when every child holds, `or` groups when any child does, recursively. An EMPTY group is TRUE under both joins, by an explicit branch rather than by accident: `some([])` is `false`, which would blank the grid the moment a builder UI adds a group and before the user fills it in. The tree is compiled ONCE per plan into closures — the same treatment the flat leaf predicates always got — and a query with no groups at all keeps the unchanged flat loop, so the per-row hot path is untouched where there is no join to honour. Capture now bounds nesting at 64 levels. Capture is the chokepoint every other recursion runs downstream of, and without the bound a ~1000-level tree was CAPTURED successfully and then overflowed the stack later, inside equality on a plan the engine had already accepted; past ~2000 levels `compileQuery` threw a raw RangeError instead of this module's validation error. Co-Authored-By: Claude Opus 5 --- .../src/__tests__/filter-tree.test.ts | 122 ++++++++++++++++ packages/row-model/src/column-types.ts | 8 +- packages/row-model/src/compiled-query.ts | 137 ++++++++++++++++-- 3 files changed, 251 insertions(+), 16 deletions(-) diff --git a/packages/row-model/src/__tests__/filter-tree.test.ts b/packages/row-model/src/__tests__/filter-tree.test.ts index a46346df0..03e21ea1e 100644 --- a/packages/row-model/src/__tests__/filter-tree.test.ts +++ b/packages/row-model/src/__tests__/filter-tree.test.ts @@ -4,6 +4,7 @@ import { CompiledQueryValidationError, compileQuery, createColumnHelper, + createLocalRowModel, getCapturedFilterTreeForTesting, isPretableFilterGroup, type PretableFilterNodeFor, @@ -389,3 +390,124 @@ describe("group identity", () => { ).not.toBe(first); }); }); + +describe("evaluation of a filter tree", () => { + interface Reading { + readonly id: string; + readonly n: number; + } + + const reading = createColumnHelper(); + const readingColumns = [ + reading.accessor("n", (row: Reading) => row.n, { type: "number" }), + ] as const; + + const ROWS: readonly Reading[] = Object.freeze([ + { id: "1", n: 1 }, + { id: "5", n: 5 }, + { id: "9", n: 9 }, + ]); + + const gt4 = { columnId: "n", operator: "gt", value: 4 } as const; + const lt2 = { columnId: "n", operator: "lt", value: 2 } as const; + const gt8 = { columnId: "n", operator: "gt", value: 8 } as const; + + /** The ids the model actually keeps visible under `filters`. */ + function visibleUnder(filters: readonly unknown[]): readonly string[] { + const model = createLocalRowModel({ + rows: [...ROWS], + columns: readingColumns, + getRowId: (row) => row.id, + query: { filters, sort: [], rowGroups: [] } as never, + }); + return model + .getState() + .snapshot.range(0, Number.MAX_SAFE_INTEGER) + .flatMap((row) => + (row as { kind: string }).kind === "data" + ? [String((row as { rowId: unknown }).rowId)] + : [], + ); + } + + test("an or group disjoins, and the same tree under and does not", () => { + // The pair is the point: the OR fixture's rows differ from the identical + // tree joined with `and`, so a connective mix-up cannot pass both. + expect(visibleUnder([gt4, { op: "or", children: [lt2, gt8] }])).toEqual([ + "9", + ]); + expect(visibleUnder([gt4, { op: "and", children: [lt2, gt8] }])).toEqual( + [], + ); + }); + + test("nesting three deep evaluates at every level", () => { + const nested = (innerOp: "and" | "or") => [ + gt4, + { op: "or", children: [lt2, { op: innerOp, children: [lt2, gt8] }] }, + ]; + expect(visibleUnder(nested("or"))).toEqual(["9"]); + expect(visibleUnder(nested("and"))).toEqual([]); + }); + + test.each(["and", "or"] as const)( + "an empty %s group is true, so a half-built group never blanks the grid", + (op) => { + expect(visibleUnder([gt4, { op, children: [] }])).toEqual( + visibleUnder([gt4]), + ); + expect(visibleUnder([gt4, { op, children: [] }])).toEqual(["5", "9"]); + }, + ); + + test("a group of groups still resolves when only one branch holds", () => { + expect( + visibleUnder([ + { + op: "or", + children: [ + { op: "and", children: [gt4, gt8] }, + { op: "and", children: [lt2, gt8] }, + ], + }, + ]), + ).toEqual(["9"]); + }); +}); + +describe("filter tree depth", () => { + // Mirrors the (unexported) `MAX_FILTER_TREE_DEPTH` in `compiled-query.ts`. + // Not imported: `index.ts` re-exports that module with `export *`, so an + // export here would move the package's public API surface. Drift is caught + // rather than hidden — both sides of the bound are asserted below, so + // raising or lowering the real limit fails one of them. + const MAX_FILTER_TREE_DEPTH = 64; + const leaf = { columnId: "sector", operator: "isEmpty" }; + const nest = (depth: number): unknown => + depth === 0 ? leaf : { op: "and", children: [nest(depth - 1)] }; + const queryAt = (depth: number) => ({ + filters: [nest(depth)], + sort: [], + rowGroups: [], + }); + + test("accepts a tree at the limit", () => { + expect(() => compile(queryAt(MAX_FILTER_TREE_DEPTH))).not.toThrow(); + }); + + test("rejects one level past the limit as a validation error, not a stack overflow", () => { + const caught = caughtFrom(queryAt(MAX_FILTER_TREE_DEPTH + 1)); + expect(caught).toBeInstanceOf(CompiledQueryValidationError); + expect(caught).toMatchObject({ code: "invalid-query" }); + expect((caught as CompiledQueryValidationError).path).toContain( + ".children[0]", + ); + }); + + test("a tree deep enough to overflow the stack never reaches equality", () => { + // The dangerous window: capture used to SUCCEED here and the RangeError + // surfaced later, from equality on a plan the engine had accepted. + const caught = caughtFrom(queryAt(5_000)); + expect(caught).toBeInstanceOf(CompiledQueryValidationError); + }); +}); diff --git a/packages/row-model/src/column-types.ts b/packages/row-model/src/column-types.ts index e7dc79faa..1538b8431 100644 --- a/packages/row-model/src/column-types.ts +++ b/packages/row-model/src/column-types.ts @@ -533,11 +533,11 @@ export type PretableFilterFor = /** * A branch of the filter tree: a join operator and the nodes it joins. A group - * may hold leaves, further groups, or nothing at all, to arbitrary depth. + * may hold leaves, further groups, or nothing at all, and may nest. * - * NOT YET EVALUATED: the engine currently applies every leaf in the tree - * conjunctively, so an `op: "or"` group behaves as an `and`. Accepted and - * validated, but do not depend on the join until disjunction ships. + * An `and` group holds when every child holds; an `or` group holds when any + * child does. An EMPTY group holds under both operators — it constrains + * nothing, so a group still being assembled never removes rows. * @public */ export interface PretableFilterGroupFor { diff --git a/packages/row-model/src/compiled-query.ts b/packages/row-model/src/compiled-query.ts index ad79947e3..91e15ad7c 100644 --- a/packages/row-model/src/compiled-query.ts +++ b/packages/row-model/src/compiled-query.ts @@ -275,10 +275,11 @@ function isRuntimeFilterGroup( } /** - * The leaves of a filter tree, in depth-first order. Filter TREE evaluation is - * not wired up yet (SP2a task 2): the plan currently applies every leaf - * conjunctively regardless of the group it sits in, which is exactly the old - * behaviour for a flat list of leaves. + * The leaves of a filter tree, in depth-first order — the tree's COLUMN + * DEPENDENCY set, flattened deliberately. Join operators are irrelevant here: + * a column is read if any leaf anywhere in the tree mentions it, whatever + * joins that leaf to its siblings. Evaluation does NOT go through this — see + * `compileFilterNode` / `evaluateCompiledFilterNode`. */ function filterLeavesOf( nodes: readonly RuntimeFilterNode[], @@ -292,6 +293,71 @@ function filterLeavesOf( return leaves; } +/* + * The filter tree compiled for evaluation: one closure per leaf with its + * operands already normalized, and the join structure preserved around them. + * Built ONCE per plan, exactly as the flat leaf predicates always were — + * evaluation never interprets `RuntimeFilterNode`s, so no per-row work + * re-resolves a column, re-normalizes an operand, or re-reads a join. + */ +type CompiledFilterNode = + | { + readonly kind: "leaf"; + readonly columnId: string; + readonly predicate: FilterPredicate; + } + | { + readonly kind: "group"; + readonly op: "and" | "or"; + readonly children: readonly CompiledFilterNode[]; + }; + +function compileFilterNode( + node: RuntimeFilterNode, + byId: ReadonlyMap, +): CompiledFilterNode { + if (isRuntimeFilterGroup(node)) { + return { + kind: "group", + op: node.op, + children: node.children.map((child) => compileFilterNode(child, byId)), + }; + } + return { + kind: "leaf", + columnId: node.columnId, + predicate: compileFilterPredicate(node, byId.get(node.columnId)!), + }; +} + +/* + * Whether the plan needs the tree evaluator at all. Only the ROOTS are + * examined, and that is sufficient: a group anywhere in the tree has a group + * at the root of its own branch, so all-leaf roots means a flat list. + */ +function hasFilterGroup(nodes: readonly RuntimeFilterNode[]): boolean { + return nodes.some(isRuntimeFilterGroup); +} + +function evaluateCompiledFilterNode( + node: CompiledFilterNode, + valueOf: (columnId: string) => unknown, +): boolean { + if (node.kind === "leaf") return node.predicate(valueOf(node.columnId)); + /* + * An EMPTY group is TRUE under BOTH joins — it constrains nothing, so it + * removes nothing. The branch is not decoration: `some([])` is `false`, + * which would make a half-built `or` group in a builder UI blank the grid + * the instant a user adds it and before they fill it in. Written once for + * both joins so the rule reads as one rule, even though `every([])` would + * already answer `true` for `and`. + */ + if (node.children.length === 0) return true; + return node.op === "and" + ? node.children.every((child) => evaluateCompiledFilterNode(child, valueOf)) + : node.children.some((child) => evaluateCompiledFilterNode(child, valueOf)); +} + interface RuntimeOrdering { readonly columnId: string; readonly direction?: string; @@ -607,13 +673,41 @@ function captureDenseArray( return Object.freeze(captured); } +/* + * The deepest a captured filter tree may nest, counting root nodes as depth 0. + * + * Capture is the chokepoint: validation, snapshotting, descriptor keys, leaf + * collection, structural equality and per-row evaluation all recurse over a + * tree only AFTER it has been captured, so bounding it here bounds every one + * of them at once. Without the bound the failure mode was not merely a deep + * tree — measured against this file's pre-bound revision, a 1000-level tree + * CAPTURED cleanly and then overflowed the stack in `filterNodeListEqual` on + * the next recompile, so the `RangeError` surfaced from a later `setQuery` on + * a plan the engine had already accepted; at 2000 `compileQuery` threw a raw + * `RangeError` instead of this module's validation error. + * + * 64 is chosen as far beyond any tree a human or a builder UI produces (real + * filter trees nest a handful of levels) while sitting an order of magnitude + * below the depth at which any of the downstream recursions is at risk. + */ +const MAX_FILTER_TREE_DEPTH = 64; + /** * One node of the filter tree. A node carrying `children` is a group and is * captured recursively, breadcrumbed as `.children[i]`; anything else is * captured as a leaf. Every level is frozen on the way out, so the captured * tree is owned by the plan rather than aliasing the caller's objects. + * + * `depth` is the node's own nesting level and is bounded — see + * `MAX_FILTER_TREE_DEPTH` for why the bound lives here and nowhere else. */ -function captureFilterNode(raw: unknown, path: string): RuntimeFilterNode { +function captureFilterNode( + raw: unknown, + path: string, + depth = 0, +): RuntimeFilterNode { + if (depth > MAX_FILTER_TREE_DEPTH) + fail(`filter group nesting exceeds ${MAX_FILTER_TREE_DEPTH} levels`, path); if (raw === null || typeof raw !== "object") fail("filter entry is not an object", path); const children = captureProperty(raw, "children", `${path}.children`); @@ -628,7 +722,8 @@ function captureFilterNode(raw: unknown, path: string): RuntimeFilterNode { children, `${path}.children`, "filter group children must be an array", - (entry, index) => captureFilterNode(entry, `${path}.children[${index}]`), + (entry, index) => + captureFilterNode(entry, `${path}.children[${index}]`, depth + 1), ), }); } @@ -1590,15 +1685,23 @@ class CompiledQueryPlan readonly #byId: ReadonlyMap; /* * The LEAVES of `#runtimeQuery.filters`, depth-first — not the filter list - * itself, which under a tree holds groups and has a different length. - * SP2a task 2: every leaf is applied conjunctively regardless of the group - * it sits in, so an `or` group currently behaves as an `and`. + * itself, which under a tree holds groups and has a different length. This + * is a DEPENDENCY set (which columns the filters read), never the evaluation + * order: joins are honoured by `#compiledFilterTree`. */ readonly #filterLeaves: readonly RuntimeFilter[]; // Parallel to `#filterLeaves`: one compiled predicate per leaf, built once // at construction so no verdict ever re-normalizes operands or re-resolves - // columns per row. + // columns per row. Drives the FLAT path only — see `#compiledFilterTree`. readonly #compiledPredicates: readonly FilterPredicate[]; + /* + * The compiled join structure, or `undefined` when the query is a flat list + * of leaves — which is the overwhelmingly common shape and stays on the + * byte-for-byte unchanged flat loop rather than paying a tree walk per row. + * Present only when a group actually exists, so nothing about the hot path + * changed for queries that have no groups to honour. + */ + readonly #compiledFilterTree: readonly CompiledFilterNode[] | undefined; readonly #active: readonly RuntimeColumn[]; readonly #aggregateColumns: readonly RuntimeColumn[]; readonly #operation: "set-query" | "set-derivations"; @@ -1674,6 +1777,13 @@ class CompiledQueryPlan this.#compiledPredicates = this.#filterLeaves.map((filter) => compileFilterPredicate(filter, this.#byId.get(filter.columnId)!), ); + this.#compiledFilterTree = hasFilterGroup(this.#runtimeQuery.filters) + ? Object.freeze( + this.#runtimeQuery.filters.map((node) => + compileFilterNode(node, this.#byId), + ), + ) + : undefined; const activeIds = new Set(); this.#filterLeaves.forEach((entry) => activeIds.add(entry.columnId)); this.#runtimeQuery.rowGroups.forEach((entry) => @@ -1847,10 +1957,13 @@ class CompiledQueryPlan * `#filterLeaves`, NOT to `#runtimeQuery.filters`) — no `#byId` lookup and * no operand re-normalization per row. * - * SP2a task 2: this is a flat conjunction over every leaf in the tree. Join - * operators are not honoured yet, so an `or` group evaluates as an `and`. + * The ROOT list joins conjunctively, as it always has — a query's top-level + * filters all have to hold. Below the roots, groups join by their own `op`. */ #filterVerdict(valueOf: (columnId: string) => unknown): boolean { + const tree = this.#compiledFilterTree; + if (tree !== undefined) + return tree.every((node) => evaluateCompiledFilterNode(node, valueOf)); const filters = this.#filterLeaves; return this.#compiledPredicates.every((predicate, index) => predicate(valueOf(filters[index].columnId)), From bae0f74333441d766905e117186dd133916d0730 Mon Sep 17 00:00:00 2001 From: Brian Love Date: Tue, 25 Aug 2026 20:25:10 -0700 Subject: [PATCH 09/15] refactor(row-model): one evaluation path for filters, grouped or not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Evaluation briefly kept two implementations: a compiled tree for queries holding groups, and the older flat predicate array for queries that did not. Two implementations of one semantics is a divergence waiting to happen, and the second one was already costing something concrete — a grouped query compiled every leaf predicate twice, once into the flat array it then never read. There is now one representation. Every node, leaf or group, compiles to a `CompiledFilterMatcher` closure; a sibling list compiles to one matcher for its join; the query's root list is that same call with `and`, which is what a top-level filter list has always meant. `#compiledPredicates` and the flat-vs-tree fork are gone. `#filterLeaves` stays — it is the column dependency set, not an evaluation order. The join loops are indexed rather than `every`/`some`, and that detail is the whole perf story. A callback join allocates a closure per group per row: on an isolated 200k-row four-leaf verdict loop the callback form measured 52ms against the old flat loop's 30ms, and the indexed form measures 30ms — level with the code it replaces, and level again on a 100k-row model build where the sign flips between rounds. Co-Authored-By: Claude Opus 5 --- packages/row-model/src/compiled-query.ts | 164 ++++++++++------------- 1 file changed, 74 insertions(+), 90 deletions(-) diff --git a/packages/row-model/src/compiled-query.ts b/packages/row-model/src/compiled-query.ts index 91e15ad7c..ba560b4df 100644 --- a/packages/row-model/src/compiled-query.ts +++ b/packages/row-model/src/compiled-query.ts @@ -294,68 +294,63 @@ function filterLeavesOf( } /* - * The filter tree compiled for evaluation: one closure per leaf with its - * operands already normalized, and the join structure preserved around them. - * Built ONCE per plan, exactly as the flat leaf predicates always were — - * evaluation never interprets `RuntimeFilterNode`s, so no per-row work - * re-resolves a column, re-normalizes an operand, or re-reads a join. + * A filter node compiled for evaluation: a closure answering that node's + * question about one row. The whole tree collapses into a nest of these at + * construction — column lookups resolved, operands normalized, joins baked + * in — so a verdict never inspects a `RuntimeFilterNode` and never branches + * on a node kind. A leaf and a group are the same callable to their parent, + * which is what lets ONE evaluation path serve a grouped query and a flat + * one without either paying for the other. */ -type CompiledFilterNode = - | { - readonly kind: "leaf"; - readonly columnId: string; - readonly predicate: FilterPredicate; - } - | { - readonly kind: "group"; - readonly op: "and" | "or"; - readonly children: readonly CompiledFilterNode[]; - }; - -function compileFilterNode( - node: RuntimeFilterNode, - byId: ReadonlyMap, -): CompiledFilterNode { - if (isRuntimeFilterGroup(node)) { - return { - kind: "group", - op: node.op, - children: node.children.map((child) => compileFilterNode(child, byId)), - }; - } - return { - kind: "leaf", - columnId: node.columnId, - predicate: compileFilterPredicate(node, byId.get(node.columnId)!), - }; -} +type CompiledFilterMatcher = ( + valueOf: (columnId: string) => unknown, +) => boolean; -/* - * Whether the plan needs the tree evaluator at all. Only the ROOTS are - * examined, and that is sufficient: a group anywhere in the tree has a group - * at the root of its own branch, so all-leaf roots means a flat list. +/** + * Compiles a sibling list joined by `op` into a single matcher. Used for + * groups and for the query's root list alike — the roots are an `and`, which + * is exactly what a top-level filter list has always meant. + * + * The join loops are indexed rather than `every`/`some`: a callback form + * allocates a closure per group PER ROW, measured at ~70% on the isolated + * verdict loop (200k rows, four leaves: 52ms against 30ms). Both operators + * are written once here, so there is one implementation of `and` and one of + * `or` whatever shape the tree has. */ -function hasFilterGroup(nodes: readonly RuntimeFilterNode[]): boolean { - return nodes.some(isRuntimeFilterGroup); -} - -function evaluateCompiledFilterNode( - node: CompiledFilterNode, - valueOf: (columnId: string) => unknown, -): boolean { - if (node.kind === "leaf") return node.predicate(valueOf(node.columnId)); +function compileFilterNodes( + nodes: readonly RuntimeFilterNode[], + op: "and" | "or", + byId: ReadonlyMap, +): CompiledFilterMatcher { /* - * An EMPTY group is TRUE under BOTH joins — it constrains nothing, so it - * removes nothing. The branch is not decoration: `some([])` is `false`, - * which would make a half-built `or` group in a builder UI blank the grid - * the instant a user adds it and before they fill it in. Written once for - * both joins so the rule reads as one rule, even though `every([])` would - * already answer `true` for `and`. + * An EMPTY list is TRUE under BOTH joins — it constrains nothing, so it + * removes nothing. The branch is not decoration: the `or` loop below falls + * through to `false` on no children, which would make a half-built `or` + * group in a builder UI blank the grid the instant a user adds it and + * before they fill it in. */ - if (node.children.length === 0) return true; - return node.op === "and" - ? node.children.every((child) => evaluateCompiledFilterNode(child, valueOf)) - : node.children.some((child) => evaluateCompiledFilterNode(child, valueOf)); + if (nodes.length === 0) return alwaysTrue; + + const matchers = nodes.map((node) => { + if (isRuntimeFilterGroup(node)) + return compileFilterNodes(node.children, node.op, byId); + const { columnId } = node; + const predicate = compileFilterPredicate(node, byId.get(columnId)!); + return (valueOf: (columnId: string) => unknown) => + predicate(valueOf(columnId)); + }); + + if (op === "and") + return (valueOf) => { + for (let index = 0; index < matchers.length; index += 1) + if (!matchers[index](valueOf)) return false; + return true; + }; + return (valueOf) => { + for (let index = 0; index < matchers.length; index += 1) + if (matchers[index](valueOf)) return true; + return false; + }; } interface RuntimeOrdering { @@ -1690,18 +1685,18 @@ class CompiledQueryPlan * order: joins are honoured by `#compiledFilterTree`. */ readonly #filterLeaves: readonly RuntimeFilter[]; - // Parallel to `#filterLeaves`: one compiled predicate per leaf, built once - // at construction so no verdict ever re-normalizes operands or re-resolves - // columns per row. Drives the FLAT path only — see `#compiledFilterTree`. - readonly #compiledPredicates: readonly FilterPredicate[]; /* - * The compiled join structure, or `undefined` when the query is a flat list - * of leaves — which is the overwhelmingly common shape and stays on the - * byte-for-byte unchanged flat loop rather than paying a tree walk per row. - * Present only when a group actually exists, so nothing about the hot path - * changed for queries that have no groups to honour. + * The whole filter tree as ONE closure, built at construction so no verdict + * re-normalizes an operand, re-resolves a column, or reads a join per row. + * + * Built the same way for EVERY query, grouped or flat. A flat query briefly + * had a second, separate predicate array and loop of its own; that bought a + * real cost (a grouped query compiled every leaf predicate twice) and the + * standing risk of two implementations of one semantics drifting apart, + * catchable only after the fact. It bought no speed: on the isolated + * verdict loop this single path runs level with the old flat one. */ - readonly #compiledFilterTree: readonly CompiledFilterNode[] | undefined; + readonly #compiledFilterTree: CompiledFilterMatcher; readonly #active: readonly RuntimeColumn[]; readonly #aggregateColumns: readonly RuntimeColumn[]; readonly #operation: "set-query" | "set-derivations"; @@ -1774,16 +1769,11 @@ class CompiledQueryPlan this.#runtimeColumns.map((column) => [column.id, column]), ); this.#filterLeaves = filterLeavesOf(this.#runtimeQuery.filters); - this.#compiledPredicates = this.#filterLeaves.map((filter) => - compileFilterPredicate(filter, this.#byId.get(filter.columnId)!), + this.#compiledFilterTree = compileFilterNodes( + this.#runtimeQuery.filters, + "and", + this.#byId, ); - this.#compiledFilterTree = hasFilterGroup(this.#runtimeQuery.filters) - ? Object.freeze( - this.#runtimeQuery.filters.map((node) => - compileFilterNode(node, this.#byId), - ), - ) - : undefined; const activeIds = new Set(); this.#filterLeaves.forEach((entry) => activeIds.add(entry.columnId)); this.#runtimeQuery.rowGroups.forEach((entry) => @@ -1949,25 +1939,19 @@ class CompiledQueryPlan } /* - * The one filter-predicate loop, parameterized over the value source the - * same way `#finalizeMetadata` is: `evaluate` supplies its collected value - * map, the verdict-only path supplies live accessor reads. Predicate - * semantics live in `compileFilterPredicate`, applied here through the - * construction-time `#compiledPredicates` array (parallel to - * `#filterLeaves`, NOT to `#runtimeQuery.filters`) — no `#byId` lookup and - * no operand re-normalization per row. + * The ONE filter verdict, parameterized over the value source the same way + * `#finalizeMetadata` is: `evaluate` supplies its collected value map, the + * verdict-only path supplies live accessor reads. Predicate semantics live + * in `compileFilterPredicate` and join semantics in + * `evaluateCompiledFilterNode`, both reached through the construction-time + * `#compiledFilterTree` — no `#byId` lookup and no operand re-normalization + * per row. * * The ROOT list joins conjunctively, as it always has — a query's top-level * filters all have to hold. Below the roots, groups join by their own `op`. */ #filterVerdict(valueOf: (columnId: string) => unknown): boolean { - const tree = this.#compiledFilterTree; - if (tree !== undefined) - return tree.every((node) => evaluateCompiledFilterNode(node, valueOf)); - const filters = this.#filterLeaves; - return this.#compiledPredicates.every((predicate, index) => - predicate(valueOf(filters[index].columnId)), - ); + return this.#compiledFilterTree(valueOf); } /** From 25de98b852c4f32f819ba6f9f3f40be229f49b0c Mon Sep 17 00:00:00 2001 From: Brian Love Date: Tue, 25 Aug 2026 20:31:34 -0700 Subject: [PATCH 10/15] docs(row-model): honest references and no rotting numbers in the filter-tree comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two comments pointed at `compileFilterNode` and `evaluateCompiledFilterNode`, neither of which exists — the collapse to one evaluation path merged both into `compileFilterNodes`, and one of the two stale pointers was written by that same commit rather than inherited from before it. The indexed-join comment carried hard figures for the closure-allocation cost it explains. Two harnesses disagreed about that pair by enough to matter, and the difference is invisible to a whole-model benchmark, so the figures come out and the instruction to measure on an isolated verdict loop goes in. The mechanism stands; only the numbers were unsafe to write down. The same block claimed the single path runs "level with" the flat loop it replaced. It does not: the flat loop was itself a callback join allocating a closure per row, so collapsing is a net win, not a wash. Said accurately now. Also gives the matcher tree its own `alwaysMatches` rather than borrowing the `FilterPredicate` twin from a thousand lines away — structurally identical, so tsc never minded, but a predicate answers about a value and a matcher about a row. Co-Authored-By: Claude Opus 5 --- packages/row-model/src/compiled-query.ts | 38 ++++++++++++++++-------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/packages/row-model/src/compiled-query.ts b/packages/row-model/src/compiled-query.ts index ba560b4df..5ed9d4b9d 100644 --- a/packages/row-model/src/compiled-query.ts +++ b/packages/row-model/src/compiled-query.ts @@ -279,7 +279,7 @@ function isRuntimeFilterGroup( * DEPENDENCY set, flattened deliberately. Join operators are irrelevant here: * a column is read if any leaf anywhere in the tree mentions it, whatever * joins that leaf to its siblings. Evaluation does NOT go through this — see - * `compileFilterNode` / `evaluateCompiledFilterNode`. + * `compileFilterNodes`. */ function filterLeavesOf( nodes: readonly RuntimeFilterNode[], @@ -306,16 +306,28 @@ type CompiledFilterMatcher = ( valueOf: (columnId: string) => unknown, ) => boolean; +/* + * Its own const rather than the `FilterPredicate` twin a thousand lines down: + * the two are structurally identical and tsc would accept either, but a + * predicate answers about a VALUE and a matcher about a ROW, and borrowing + * one for the other is a pun a reader has to unpick. + */ +const alwaysMatches: CompiledFilterMatcher = () => true; + /** * Compiles a sibling list joined by `op` into a single matcher. Used for * groups and for the query's root list alike — the roots are an `and`, which * is exactly what a top-level filter list has always meant. * - * The join loops are indexed rather than `every`/`some`: a callback form - * allocates a closure per group PER ROW, measured at ~70% on the isolated - * verdict loop (200k rows, four leaves: 52ms against 30ms). Both operators - * are written once here, so there is one implementation of `and` and one of - * `or` whatever shape the tree has. + * The join loops are indexed rather than `every`/`some`, which is not a style + * preference: a callback join allocates a closure per group PER ROW, on the + * hottest loop in the package. Deliberately unquantified — two harnesses + * measured the gap differently enough to disagree, and a figure pasted here + * would rot where no reader could re-derive it. Measure it yourself on an + * isolated verdict loop; a whole-model benchmark cannot resolve it. + * + * Both operators are written once here, so there is one implementation of + * `and` and one of `or` whatever shape the tree has. */ function compileFilterNodes( nodes: readonly RuntimeFilterNode[], @@ -329,7 +341,7 @@ function compileFilterNodes( * group in a builder UI blank the grid the instant a user adds it and * before they fill it in. */ - if (nodes.length === 0) return alwaysTrue; + if (nodes.length === 0) return alwaysMatches; const matchers = nodes.map((node) => { if (isRuntimeFilterGroup(node)) @@ -1693,8 +1705,9 @@ class CompiledQueryPlan * had a second, separate predicate array and loop of its own; that bought a * real cost (a grouped query compiled every leaf predicate twice) and the * standing risk of two implementations of one semantics drifting apart, - * catchable only after the fact. It bought no speed: on the isolated - * verdict loop this single path runs level with the old flat one. + * catchable only after the fact. It bought no speed either: the flat loop + * it saved allocated a closure per row of its own, so the single path is + * measurably the FASTER of the two on an isolated verdict loop. */ readonly #compiledFilterTree: CompiledFilterMatcher; readonly #active: readonly RuntimeColumn[]; @@ -1942,10 +1955,9 @@ class CompiledQueryPlan * The ONE filter verdict, parameterized over the value source the same way * `#finalizeMetadata` is: `evaluate` supplies its collected value map, the * verdict-only path supplies live accessor reads. Predicate semantics live - * in `compileFilterPredicate` and join semantics in - * `evaluateCompiledFilterNode`, both reached through the construction-time - * `#compiledFilterTree` — no `#byId` lookup and no operand re-normalization - * per row. + * in `compileFilterPredicate` and join semantics in `compileFilterNodes`, + * both reached through the construction-time `#compiledFilterTree` — no + * `#byId` lookup and no operand re-normalization per row. * * The ROOT list joins conjunctively, as it always has — a query's top-level * filters all have to hold. Below the roots, groups join by their own `op`. From 3c5bfe81c92c82f636c1a56982b8d4f9f6be9bd0 Mon Sep 17 00:00:00 2001 From: Brian Love Date: Tue, 25 Aug 2026 20:46:40 -0700 Subject: [PATCH 11/15] =?UTF-8?q?feat(react):=20the=20surface=20speaks=20f?= =?UTF-8?q?ilter=20trees=20=E2=80=94=20funnels,=20menu,=20controlled=20sta?= =?UTF-8?q?te?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `query.filters` became an AND/OR tree in the row model; the surface's chrome was still reading it as a flat list of leaves, through a cast that would have keyed every group element under `undefined`. The per-column record projection is deleted. `snapshot.filters` is now the query's array verbatim — leaves and groups, nested — and the two questions the chrome actually asks are answered by walks in the new `./filter-tree`: - `columnHasFilter` lights a funnel on any occurrence of a column at any depth, because a filter nested in a group still removes that column's rows; - `topLevelColumnFilter` / `withTopLevelColumnFilter` scope the column menu to its top-level leaf, read and write. A commit replaces that leaf in place and passes every other element through by reference, so a group the menu never authored survives byte-identical. `LabeledGridSurface`'s `is-filtered` decoration walks the tree by the same occurrence rule. `isPretableFilterGroup` and the node/group types are re-exported from `@pretable/core` and `@pretable/react` — the surface needs the guard, and so does any consumer reading `onQueryChange`'s filters. This is the first commit on the branch where `pnpm build` succeeds again. Co-Authored-By: Claude Opus 5 --- .changeset/filter-tree-surface.md | 34 +++ .../column-filters/ColumnFiltersGrid.tsx | 13 +- packages/core/core.api.md | 16 +- packages/core/src/public_api.ts | 3 + packages/core/src/types.ts | 2 + packages/react/react.api.md | 16 +- .../__tests__/filter-menu-surface.test.tsx | 208 +++++++++++++++++- .../react/src/__tests__/filter-tree.test.ts | 155 +++++++++++++ .../__tests__/labeled-grid-surface.test.tsx | 48 ++++ packages/react/src/filter-tree.ts | 119 ++++++++++ packages/react/src/labeled-grid-surface.tsx | 38 +++- packages/react/src/pretable-surface.tsx | 82 ++++--- packages/react/src/public_api.ts | 8 +- 13 files changed, 689 insertions(+), 53 deletions(-) create mode 100644 .changeset/filter-tree-surface.md create mode 100644 packages/react/src/__tests__/filter-tree.test.ts create mode 100644 packages/react/src/filter-tree.ts diff --git a/.changeset/filter-tree-surface.md b/.changeset/filter-tree-surface.md new file mode 100644 index 000000000..31f7d6b66 --- /dev/null +++ b/.changeset/filter-tree-surface.md @@ -0,0 +1,34 @@ +--- +"@pretable/react": minor +--- + +The surface speaks filter trees: funnels, the column menu, and controlled +state. + +`query.filters` is now an AND/OR tree — each element is either a typed leaf or +a `{ op, children }` group, and groups nest (see `@pretable/core` for the node +type, the `isPretableFilterGroup` guard, and the empty-group rule). The +surface's chrome follows: + +- The **funnel** lights on ANY occurrence of a column, at any depth. A filter + the user built inside a group still removes their rows, so it still shows as + a filter on that column. Previously the surface kept a per-column record + projected out of the query; a group carries no `columnId`, so that record + would have collapsed every group onto the single key `undefined` and left the + funnel dark. The record is gone — the surface holds the tree verbatim. +- The **column filter menu** owns exactly its column's TOP-LEVEL leaf. It + hydrates from that leaf (never from one nested in a group), and a commit + replaces it in place. Every group element passes through by reference: a menu + commit cannot edit, reorder, or drop a branch it did not author, and clearing + a column removes only its top-level leaf. +- **Controlled queries** take the tree shape. A controlled `query.filters` + containing groups renders funnels and filters rows exactly as the engine + evaluates it. +- `LabeledGridSurface`'s `is-filtered` header decoration walks the tree by the + same "occurrence anywhere" rule. + +`isPretableFilterGroup`, `PretableFilterGroupFor` and `PretableFilterNodeFor` +are re-exported from `@pretable/react` — a consumer reading `onQueryChange`'s +`filters` needs the guard to tell leaves from groups. + +No UI builds groups yet; nothing in this release deepens a tree on its own. diff --git a/apps/website/content/examples/column-filters/ColumnFiltersGrid.tsx b/apps/website/content/examples/column-filters/ColumnFiltersGrid.tsx index 20d5c397f..5bbbd642d 100644 --- a/apps/website/content/examples/column-filters/ColumnFiltersGrid.tsx +++ b/apps/website/content/examples/column-filters/ColumnFiltersGrid.tsx @@ -2,7 +2,7 @@ import { useState, type ComponentProps } from "react"; -import { PretableSurface } from "@pretable/react"; +import { isPretableFilterGroup, PretableSurface } from "@pretable/react"; import { columns } from "./columns"; import { type Order, orders } from "./data"; @@ -50,7 +50,16 @@ export function ColumnFiltersGrid() { {query.filters.length > 0 ? query.filters - .map((filter) => `${filter.columnId} ${filter.operator}`) + // `filters` is an AND/OR tree: an element is either a typed + // leaf or a group of nodes. The built-in column menu only ever + // writes top-level leaves, so no group appears here — but the + // type says one can, and `isPretableFilterGroup` is how a + // consumer tells them apart. + .map((filter) => + isPretableFilterGroup(filter) + ? `(${filter.op} group)` + : `${filter.columnId} ${filter.operator}`, + ) .join(" · ") : "(none)"} diff --git a/packages/core/core.api.md b/packages/core/core.api.md index 44b37e9bc..be69493b3 100644 --- a/packages/core/core.api.md +++ b/packages/core/core.api.md @@ -165,6 +165,9 @@ export type FilterValue = string | number | readonly [number, number] | readonly // @public export const GROUP_COLUMN_ID = "__pretable_group__"; +// @public +export function isPretableFilterGroup(node: PretableFilterNodeFor): node is PretableFilterGroupFor; + // @public export const numberFormats: { readonly money: (options: PretableCurrencyFormatOptions) => Intl.NumberFormatOptions; @@ -671,6 +674,17 @@ export type PretableFilterFor = TColumns extends readonly (infer TColu readonly value: PretableFilterOperandFor; }) : never : never; +// @public +export interface PretableFilterGroupFor { + // (undocumented) + readonly children: readonly PretableFilterNodeFor[]; + // (undocumented) + readonly op: "and" | "or"; +} + +// @public +export type PretableFilterNodeFor = PretableFilterFor | PretableFilterGroupFor; + // @public export type PretableFilterOperandFor = TType extends "text" ? string : TType extends "number" ? number : TType extends "date" ? string | number | Date : TType extends "boolean" ? boolean : [Extract, string>] extends [never] ? string : Extract, string>; @@ -1014,7 +1028,7 @@ export interface PretableProcessingOptions { // @public (undocumented) export interface PretableQueryFor { // (undocumented) - readonly filters: readonly PretableFilterFor[]; + readonly filters: readonly PretableFilterNodeFor[]; // (undocumented) readonly rowGroups: readonly PretableRowGroupFor[]; // (undocumented) diff --git a/packages/core/src/public_api.ts b/packages/core/src/public_api.ts index 68069be17..7bb3a52e9 100644 --- a/packages/core/src/public_api.ts +++ b/packages/core/src/public_api.ts @@ -11,6 +11,7 @@ export { createColumnHelper } from "./create-column-helper"; export { createLocalRowModel } from "./create-local-row-model"; export { numberFormats } from "./number-formats"; export type { PretableCurrencyFormatOptions } from "./number-formats"; +export { isPretableFilterGroup } from "@pretable-internal/row-model"; export { PretableDisposedModelError, PretableInvalidGroupKeyError, @@ -76,6 +77,8 @@ export type { PretableExpansionDefault, PretableExpansionState, PretableFilterFor, + PretableFilterGroupFor, + PretableFilterNodeFor, PretableFilterOperandFor, PretableFocusDirection, PretableFocusState, diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index ff293ff70..206b2a0a2 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -84,6 +84,8 @@ export type { PretableExpansionDefault, PretableExpansionState, PretableFilterFor, + PretableFilterGroupFor, + PretableFilterNodeFor, PretableFilterOperandFor, PretableFormatInput, PretableGroupId, diff --git a/packages/react/react.api.md b/packages/react/react.api.md index a242282cb..3e2cb76b7 100644 --- a/packages/react/react.api.md +++ b/packages/react/react.api.md @@ -195,6 +195,9 @@ export type FilterOperator = "contains" | "notContains" | "equals" | "notEquals" // @public (undocumented) export type FilterValue = string | number | readonly [number, number] | readonly [string, string] | readonly string[] | null; +// @public +export function isPretableFilterGroup(node: PretableFilterNodeFor): node is PretableFilterGroupFor; + // @beta export function LabeledGridSurface = TColumns extends readonly (infer TColu readonly value: PretableFilterOperandFor; }) : never : never; +// @public +export interface PretableFilterGroupFor { + // (undocumented) + readonly children: readonly PretableFilterNodeFor[]; + // (undocumented) + readonly op: "and" | "or"; +} + +// @public +export type PretableFilterNodeFor = PretableFilterFor | PretableFilterGroupFor; + // @public export type PretableFilterOperandFor = TType extends "text" ? string : TType extends "number" ? number : TType extends "date" ? string | number | Date : TType extends "boolean" ? boolean : [Extract, string>] extends [never] ? string : Extract, string>; @@ -1603,7 +1617,7 @@ export type PretableProps { // (undocumented) - readonly filters: readonly PretableFilterFor[]; + readonly filters: readonly PretableFilterNodeFor[]; // (undocumented) readonly rowGroups: readonly PretableRowGroupFor[]; // (undocumented) diff --git a/packages/react/src/__tests__/filter-menu-surface.test.tsx b/packages/react/src/__tests__/filter-menu-surface.test.tsx index 9e1b18430..d936b727f 100644 --- a/packages/react/src/__tests__/filter-menu-surface.test.tsx +++ b/packages/react/src/__tests__/filter-menu-surface.test.tsx @@ -10,7 +10,7 @@ import { import * as React from "react"; import { afterEach, describe, expect, it, vi } from "vitest"; -import type { ColumnFilter } from "@pretable/core"; +import { isPretableFilterGroup, type ColumnFilter } from "@pretable/core"; import { PretableSurface, type PretableSurfaceProps, @@ -98,15 +98,25 @@ function renderSurface(extra: TestOptions = {}) { onQueryChange={(next) => { if (controlledFilters === undefined) setQuery(next as typeof query); extra.onSortChange?.(next.sort); + // This harness is LEAF-ONLY BY DESIGN: the suites below it drive the + // per-column menu, which only ever writes top-level leaves. The + // tree-shaped cases live in the "filter trees" describe at the foot + // of this file and read `next.filters` unprojected. extra.onFiltersChange?.( Object.fromEntries( - next.filters.map((filter) => [ - filter.columnId, - { - operator: filter.operator, - ...("value" in filter ? { value: filter.value } : {}), - }, - ]), + next.filters.flatMap((filter) => + isPretableFilterGroup(filter) + ? [] + : [ + [ + filter.columnId, + { + operator: filter.operator, + ...("value" in filter ? { value: filter.value } : {}), + }, + ] as const, + ], + ), ), ); }} @@ -436,3 +446,185 @@ describe("PretableSurface — built-in filter funnel", () => { ); }); }); + +/** + * SP2a: `query.filters` is an AND/OR TREE — each top-level element is either a + * typed leaf or a group, and groups nest. These pin the three surface + * behaviors the tree changes: the funnel lights on ANY occurrence of a column, + * the menu owns only that column's TOP-LEVEL leaf, and a menu commit must + * leave every group element it did not author untouched. + */ +describe("PretableSurface — filter trees", () => { + type TreeNode = Record; + + function renderTreeSurface( + initialFilters: readonly TreeNode[], + extra: { + onQueryChange?: (query: { filters: readonly TreeNode[] }) => void; + onGridReady?: PretableSurfaceProps["onGridReady"]; + } = {}, + ) { + function Harness() { + const [query, setQuery] = React.useState(() => ({ + filters: initialFilters, + sort: [], + rowGroups: [], + })); + return ( + + ariaLabel="Bug grid" + columns={columns} + getRowId={getRowId} + onGridReady={extra.onGridReady} + overscan={0} + rows={rows} + query={query as never} + onQueryChange={(next) => { + setQuery(next as never); + extra.onQueryChange?.(next as never); + }} + viewportHeight={300} + /> + ); + } + return render(); + } + + const renderedRowIds = (view: ReturnType) => + view + .getAllByTestId("pretable-row") + .map((r) => r.getAttribute("data-pretable-row-id")); + + it("carries a controlled tree to the model verbatim and joins it with OR", async () => { + let grid: Parameters>[0] | null = + null; + // Chosen so OR and AND disagree: under `or` this is b1 (count 3 > 2) and + // b2 (title contains beta); under `and` it would be b2 alone. + const tree = [ + { + op: "or", + children: [ + { columnId: "title", operator: "contains", value: "beta" }, + { columnId: "count", operator: "gt", value: 2 }, + ], + }, + ]; + const view = renderTreeSurface(tree, { + onGridReady: (ready) => { + grid = ready; + }, + }); + + await expect + .poll(() => grid?.rowModel.getState().snapshot.query.filters) + .toEqual(tree); + await waitFor(() => expect(renderedRowIds(view)).toEqual(["b1", "b2"])); + }); + + it("lights a column's funnel for a leaf buried two groups deep", async () => { + const view = renderTreeSurface([ + { + op: "and", + children: [ + { + op: "or", + children: [ + { columnId: "title", operator: "contains", value: "alpha" }, + ], + }, + ], + }, + ]); + + // No top-level leaf mentions `title` at all — the old per-column record + // projection would have keyed the group under `undefined` and left this + // funnel dark. + await waitFor(() => + expect( + view.getByRole("button", { name: "Filter Title" }), + ).toHaveAttribute("data-pretable-filter-active", "true"), + ); + // Control: a column the tree never mentions stays dark. + expect(view.getByRole("button", { name: "Filter Count" })).toHaveAttribute( + "data-pretable-filter-active", + "false", + ); + }); + + it("hydrates the menu from the TOP-LEVEL leaf, not the nested one", () => { + const view = renderTreeSurface([ + { + op: "or", + children: [ + { columnId: "title", operator: "contains", value: "nested" }, + ], + }, + { columnId: "title", operator: "endsWith", value: "crash" }, + ]); + + fireEvent.click(view.getByRole("button", { name: "Filter Title" })); + const dialog = view.getByRole("dialog", { name: "Filter Title" }); + expect(within(dialog).getByLabelText("Filter operator")).toHaveValue( + "endsWith", + ); + expect(within(dialog).getByLabelText("Filter value")).toHaveValue("crash"); + }); + + it("a menu commit splices the top-level leaf and leaves the group untouched", async () => { + const group = { + op: "or", + children: [ + { columnId: "title", operator: "contains", value: "crash" }, + { columnId: "severity", operator: "isAnyOf", value: ["high"] }, + ], + }; + const onQueryChange = vi.fn(); + const view = renderTreeSurface( + [{ columnId: "title", operator: "contains", value: "alpha" }, group], + { onQueryChange }, + ); + + fireEvent.click(view.getByRole("button", { name: "Filter Title" })); + const dialog = view.getByRole("dialog", { name: "Filter Title" }); + act(() => { + fireEvent.change(within(dialog).getByLabelText("Filter value"), { + target: { value: "leak" }, + }); + }); + + await waitFor(() => expect(onQueryChange).toHaveBeenCalled()); + const next = onQueryChange.mock.lastCall?.[0] as { + filters: readonly TreeNode[]; + }; + // The leaf is REPLACED in place, and the group element the menu never + // authored comes through structurally identical, in its original slot. + expect(next.filters).toEqual([ + { columnId: "title", operator: "contains", value: "leak" }, + group, + ]); + }); + + it("clearing from the menu removes only the top-level leaf", async () => { + const group = { + op: "or", + children: [ + { columnId: "severity", operator: "isAnyOf", value: ["high"] }, + ], + }; + const onQueryChange = vi.fn(); + const view = renderTreeSurface( + [{ columnId: "title", operator: "contains", value: "alpha" }, group], + { onQueryChange }, + ); + + fireEvent.click(view.getByRole("button", { name: "Filter Title" })); + const dialog = view.getByRole("dialog", { name: "Filter Title" }); + fireEvent.click(within(dialog).getByText("Clear")); + + await waitFor(() => expect(onQueryChange).toHaveBeenCalled()); + const next = onQueryChange.mock.lastCall?.[0] as { + filters: readonly TreeNode[]; + }; + expect(next.filters).toEqual([group]); + }); +}); diff --git a/packages/react/src/__tests__/filter-tree.test.ts b/packages/react/src/__tests__/filter-tree.test.ts new file mode 100644 index 000000000..723a17ef3 --- /dev/null +++ b/packages/react/src/__tests__/filter-tree.test.ts @@ -0,0 +1,155 @@ +import { describe, expect, it } from "vitest"; + +import { + columnHasFilter, + topLevelColumnFilter, + withTopLevelColumnFilter, + type SurfaceFilterNode, +} from "../filter-tree"; + +describe("columnHasFilter", () => { + it("finds a top-level leaf", () => { + const filters: readonly SurfaceFilterNode[] = [ + { columnId: "a", operator: "contains", value: "x" }, + ]; + expect(columnHasFilter(filters, "a")).toBe(true); + expect(columnHasFilter(filters, "b")).toBe(false); + }); + + it("finds a leaf buried two groups deep, with no top-level leaf for it", () => { + const filters: readonly SurfaceFilterNode[] = [ + { columnId: "b", operator: "isNotEmpty" }, + { + op: "or", + children: [ + { columnId: "c", operator: "contains", value: "q" }, + { + op: "and", + children: [{ columnId: "a", operator: "equals", value: 3 }], + }, + ], + }, + ]; + expect(columnHasFilter(filters, "a")).toBe(true); + expect(columnHasFilter(filters, "c")).toBe(true); + expect(columnHasFilter(filters, "b")).toBe(true); + expect(columnHasFilter(filters, "d")).toBe(false); + }); + + it("an empty group mentions no column", () => { + expect(columnHasFilter([{ op: "and", children: [] }], "a")).toBe(false); + expect(columnHasFilter([], "a")).toBe(false); + }); + + it("does not mistake a group's `op` for a column match", () => { + // A group carries no `columnId`; a naive `entry.columnId === columnId` + // read would compare `undefined` and could match a column literally + // named "undefined". Neither is a hit here. + const filters: readonly SurfaceFilterNode[] = [{ op: "or", children: [] }]; + expect(columnHasFilter(filters, "undefined")).toBe(false); + }); + + it("walks a tree deeper than the engine bound without blowing up", () => { + // The engine rejects trees deeper than 64 at `compileQuery`; the surface + // helper must not hang before that rejection is reachable. + let node: SurfaceFilterNode = { + columnId: "a", + operator: "isEmpty", + }; + for (let i = 0; i < 200; i += 1) node = { op: "and", children: [node] }; + expect(columnHasFilter([node], "a")).toBe(true); + }); +}); + +describe("topLevelColumnFilter", () => { + it("reads the top-level leaf, not the one nested in a group", () => { + const filters: readonly SurfaceFilterNode[] = [ + { + op: "or", + children: [{ columnId: "a", operator: "contains", value: "nested" }], + }, + { columnId: "a", operator: "endsWith", value: "top" }, + ]; + expect(topLevelColumnFilter(filters, "a")).toEqual({ + operator: "endsWith", + value: "top", + }); + }); + + it("drops the `columnId` and omits an absent `value`", () => { + expect( + topLevelColumnFilter([{ columnId: "a", operator: "isEmpty" }], "a"), + ).toEqual({ operator: "isEmpty" }); + expect( + "value" in + (topLevelColumnFilter([{ columnId: "a", operator: "isEmpty" }], "a") ?? + {}), + ).toBe(false); + }); + + it("is null when only a nested leaf mentions the column", () => { + const filters: readonly SurfaceFilterNode[] = [ + { + op: "and", + children: [{ columnId: "a", operator: "contains", value: "nested" }], + }, + ]; + expect(topLevelColumnFilter(filters, "a")).toBeNull(); + }); +}); + +describe("withTopLevelColumnFilter", () => { + const group: SurfaceFilterNode = { + op: "or", + children: [ + { columnId: "a", operator: "contains", value: "nested" }, + { columnId: "b", operator: "isNotEmpty" }, + ], + }; + + it("replaces the top-level leaf in place and leaves the group identical", () => { + const filters: readonly SurfaceFilterNode[] = [ + { columnId: "a", operator: "contains", value: "old" }, + group, + ]; + const next = withTopLevelColumnFilter(filters, "a", { + operator: "endsWith", + value: "new", + }); + expect(next).toEqual([ + { columnId: "a", operator: "endsWith", value: "new" }, + group, + ]); + // Reference identity, not just structural equality: the group element the + // menu did not touch must be the very object the query already held. + expect(next[1]).toBe(group); + }); + + it("appends when there is no top-level leaf yet, after the group", () => { + const next = withTopLevelColumnFilter([group], "a", { + operator: "equals", + value: "x", + }); + expect(next).toEqual([ + group, + { columnId: "a", operator: "equals", value: "x" }, + ]); + expect(next[0]).toBe(group); + }); + + it("clearing removes only the top-level leaf; the group survives", () => { + const filters: readonly SurfaceFilterNode[] = [ + { columnId: "a", operator: "contains", value: "old" }, + group, + ]; + const next = withTopLevelColumnFilter(filters, "a", null); + expect(next).toEqual([group]); + expect(next[0]).toBe(group); + }); + + it("omits `value` when the committed filter has none", () => { + const next = withTopLevelColumnFilter([], "a", { operator: "isEmpty" }); + expect(next).toEqual([{ columnId: "a", operator: "isEmpty" }]); + expect("value" in next[0]!).toBe(false); + }); +}); diff --git a/packages/react/src/__tests__/labeled-grid-surface.test.tsx b/packages/react/src/__tests__/labeled-grid-surface.test.tsx index 4102372eb..51603129c 100644 --- a/packages/react/src/__tests__/labeled-grid-surface.test.tsx +++ b/packages/react/src/__tests__/labeled-grid-surface.test.tsx @@ -288,6 +288,54 @@ describe("LabeledGridSurface", () => { expect(timestampHeader).not.toHaveClass("is-filtered"); }); + it("applies the filter-active class for a leaf nested inside a group", () => { + // `filters` is an AND/OR tree. A column constrained only from inside a + // group is still constrained, so its header still reads as filtered — the + // same "occurrence anywhere" rule the surface's funnel uses. + const view = render( + row.id} + headerCellClassName="inspection-header-cell" + query={ + { + sort: [], + filters: [ + { + op: "or", + children: [ + { + op: "and", + children: [ + { + columnId: "severity", + operator: "contains", + value: "error", + }, + ], + }, + ], + }, + ], + rowGroups: [], + } as never + } + onQueryChange={() => {}} + overscan={0} + rows={rows} + viewportHeight={132} + />, + ); + + expect( + view.getByRole("columnheader", { name: "Sort Severity" }), + ).toHaveClass("is-filtered"); + expect( + view.getByRole("columnheader", { name: "Sort Timestamp" }), + ).not.toHaveClass("is-filtered"); + }); + it("passes query and onQueryChange through to the underlying surface", () => { const onQueryChange = vi.fn(); const view = render( diff --git a/packages/react/src/filter-tree.ts b/packages/react/src/filter-tree.ts new file mode 100644 index 000000000..2d8933187 --- /dev/null +++ b/packages/react/src/filter-tree.ts @@ -0,0 +1,119 @@ +import { isPretableFilterGroup, type ColumnFilter } from "@pretable/core"; + +/** + * The value-erased twin of `PretableFilterFor`: one typed leaf of + * the filter tree, seen from chrome that works from DRAWN column ids and + * runtime operand values rather than from a static column tuple. + */ +export interface SurfaceFilterLeaf { + readonly columnId: string; + readonly operator: ColumnFilter["operator"]; + readonly value?: ColumnFilter["value"]; +} + +/** The value-erased twin of `PretableFilterGroupFor`. */ +export interface SurfaceFilterGroup { + readonly op: "and" | "or"; + readonly children: readonly SurfaceFilterNode[]; +} + +/** + * One node of the surface's view of `query.filters`: a leaf or a group. + * + * The surface holds the filter tree VERBATIM — it keeps no per-column record + * beside it. A record cannot describe a group (a group has no `columnId`, so + * every group in a query would collapse onto the single key `undefined`), and + * a projection that lies by omission is worse than the two small walks below. + */ +export type SurfaceFilterNode = SurfaceFilterLeaf | SurfaceFilterGroup; + +/** + * `isPretableFilterGroup` is generic over a static column tuple; the surface's + * nodes are value-erased, and `as never` is what satisfies the parameter + * without weakening the guard — it is structural at runtime. Same collapse, + * and the same remedy, as the `distinctValues` call in `pretable-surface.tsx`. + */ +const isGroup = (node: SurfaceFilterNode): node is SurfaceFilterGroup => + isPretableFilterGroup(node as never); + +/** + * Does ANY leaf anywhere in the tree constrain `columnId`? + * + * This is what lights a column's funnel. Occurrence, not position: a filter + * the user built two groups deep in the filter builder still means "this + * column is filtered", and a funnel that only noticed top-level leaves would + * tell them their column was unconstrained while it removed their rows. + */ +export function columnHasFilter( + nodes: readonly SurfaceFilterNode[], + columnId: string, +): boolean { + return nodes.some((node) => + isGroup(node) + ? columnHasFilter(node.children, columnId) + : node.columnId === columnId, + ); +} + +/** + * The column's TOP-LEVEL leaf, as the per-column filter menu understands it — + * or `null` when only a group mentions the column. + * + * The menu edits one column with one operator and one operand; it cannot + * express a group, so it owns exactly the top-level leaf and reports nothing + * about the rest of the tree. Reaching into groups here would let a menu + * commit silently overwrite a branch the user built elsewhere. + */ +export function topLevelColumnFilter( + nodes: readonly SurfaceFilterNode[], + columnId: string, +): ColumnFilter | null { + for (const node of nodes) { + if (isGroup(node) || node.columnId !== columnId) continue; + return { + operator: node.operator, + ...(node.value === undefined ? {} : { value: node.value }), + }; + } + return null; +} + +/** + * The menu's write path: replace (or, cleared, remove) the column's top-level + * leaf and pass every other element through BY REFERENCE. + * + * Replacement is in place rather than remove-then-append so that committing a + * new operand does not reshuffle the array, and groups keep their slots. Every + * element this function did not author is the caller's own object, unchanged — + * that is the contract the surface's group elements survive on. + */ +export function withTopLevelColumnFilter( + nodes: readonly SurfaceFilterNode[], + columnId: string, + filter: ColumnFilter | null, +): readonly SurfaceFilterNode[] { + const replacement: SurfaceFilterLeaf | null = + filter === null + ? null + : { + columnId, + operator: filter.operator, + ...(filter.value === undefined ? {} : { value: filter.value }), + }; + let replaced = false; + const next: SurfaceFilterNode[] = []; + for (const node of nodes) { + if (isGroup(node) || node.columnId !== columnId) { + next.push(node); + continue; + } + // Only the FIRST top-level leaf is the menu's; any duplicate a consumer + // wrote is dropped, because the menu can only show one of them and + // leaving the others would make the commit look inert. + if (replaced || replacement === null) continue; + next.push(replacement); + replaced = true; + } + if (!replaced && replacement !== null) next.push(replacement); + return next; +} diff --git a/packages/react/src/labeled-grid-surface.tsx b/packages/react/src/labeled-grid-surface.tsx index 6bb6de41f..3c25f508a 100644 --- a/packages/react/src/labeled-grid-surface.tsx +++ b/packages/react/src/labeled-grid-surface.tsx @@ -1,9 +1,15 @@ +import { isPretableFilterGroup } from "@pretable/core"; import type { PretableRow, PretableRowId, PretableSortDirection, PretableQueryFor, } from "@pretable/core"; +import type { + SurfaceFilterGroup, + SurfaceFilterLeaf, + SurfaceFilterNode, +} from "./filter-tree"; import type { HTMLAttributes } from "react"; import type { PretableTelemetry } from "./surface-types"; import { SortAscIcon, SortDescIcon } from "./icons"; @@ -30,6 +36,23 @@ function isColumnFilterActive(filter: { return true; // number } +/** + * Every column an ACTIVE leaf constrains, at any depth. Groups carry no + * `columnId`; they are recursed into, never counted. + */ +function collectActiveFilterColumns( + nodes: readonly SurfaceFilterNode[], + into: Set, +): void { + for (const node of nodes) { + if (isPretableFilterGroup(node as never)) { + collectActiveFilterColumns((node as SurfaceFilterGroup).children, into); + } else if (isColumnFilterActive(node as SurfaceFilterLeaf)) { + into.add((node as SurfaceFilterLeaf).columnId); + } + } +} + /** * Input passed to a {@link LabeledGridSurface} format function. * @@ -182,10 +205,17 @@ export function LabeledGridSurface< // write back to the prop. const getPinnedClassName = (pinned: "left" | "right" | null) => pinned != null && pinnedClassName ? pinnedClassName : undefined; - const activeFilterColumns = new Set( - (query?.filters ?? []) - .filter((filter) => isColumnFilterActive(filter)) - .map((filter) => filter.columnId), + const activeFilterColumns = new Set(); + // TREE-AWARE: `query.filters` is an AND/OR tree, and this label decoration + // means "this column is constrained", which a leaf nested inside a group + // makes just as true as a top-level one. See `columnHasFilter` in + // `./filter-tree` for the same walk on the surface's own funnel. + collectActiveFilterColumns( + // Value-erased, like every other filter read outside the row model: + // `PretableFilterNodeFor` is discriminated over the static column + // tuple's operand types, and this walk cares only about `columnId`. + (query?.filters ?? []) as unknown as readonly SurfaceFilterNode[], + activeFilterColumns, ); const getFormattedValue = ({ column, diff --git a/packages/react/src/pretable-surface.tsx b/packages/react/src/pretable-surface.tsx index 4bd2b4528..e00bd9362 100644 --- a/packages/react/src/pretable-surface.tsx +++ b/packages/react/src/pretable-surface.tsx @@ -391,6 +391,12 @@ import { type PretableBodyStateKind, type PretableDataState, } from "./data-state"; +import { + columnHasFilter, + topLevelColumnFilter, + withTopLevelColumnFilter, + type SurfaceFilterNode, +} from "./filter-tree"; /** Local interaction facade used while the surface maps UI commands onto the * indexed grid and row model. Row data, queries, grouping, and expansion @@ -400,7 +406,12 @@ interface SurfaceFacade { getSnapshot(): { readonly viewport: PretableViewportState; readonly sort: readonly PretableSortEntry[]; - readonly filters: Readonly>; + // The query's filter TREE, verbatim — leaves and groups, nested. Not a + // per-column record: a group carries no `columnId`, so a record would + // collapse every group in the query onto the single key `undefined`. + // Chrome that wants a per-column answer asks `columnHasFilter` / + // `topLevelColumnFilter` (see `./filter-tree`). + readonly filters: readonly SurfaceFilterNode[]; readonly selection: PretableSelectionState; readonly focus: PretableFocusState & { readonly ref: PretableIndexedFocusRef | null; @@ -2177,17 +2188,13 @@ export function PretableSurface< [indexed.rowModel], ); const snapshot = useMemo(() => { - const filters: Record = {}; - for (const entry of rowModelSnapshot.query.filters as readonly { - readonly columnId: string; - readonly operator: ColumnFilter["operator"]; - readonly value?: ColumnFilter["value"]; - }[]) { - filters[entry.columnId] = { - operator: entry.operator, - ...(entry.value === undefined ? {} : { value: entry.value }), - }; - } + // TREE-AWARE, by passing through: the filter tree reaches the chrome + // exactly as the model holds it. The value erasure is the same one + // `queryWith` documents — `PretableFilterFor` collapses to + // `never` against runtime-supplied columns, so the surface reads the + // nodes through their value-erased twin. + const filters = rowModelSnapshot.query + .filters as unknown as readonly SurfaceFilterNode[]; const ranges = indexedSnapshot.selection.ranges.map(flattenIndexedRange); const ref = indexedSnapshot.focus.ref; return { @@ -2420,6 +2427,9 @@ export function PretableSurface< // `setQuery` on the engine or the surface reconstructing the // discriminated filter union from runtime data — both outside this file. const next = { + // TREE-AGNOSTIC PASS-THROUGH: an unnamed axis is re-submitted exactly + // as the model holds it, groups and nesting included. Only the caller + // that names `filters` decides what the tree becomes. filters: (parts.filters ?? current.filters) as never, sort: (parts.sort ?? current.sort) as never, rowGroups: (parts.rowGroups ?? current.rowGroups) as never, @@ -2467,20 +2477,12 @@ export function PretableSurface< const projectedQuery = currentQuery(); if (projectedQuery === surfaceContextRef.current.rowModelSnapshot.query) return current; - const projectedFilters: Record = {}; - for (const entry of projectedQuery.filters as readonly { - readonly columnId: string; - readonly operator: ColumnFilter["operator"]; - readonly value?: ColumnFilter["value"]; - }[]) { - projectedFilters[entry.columnId] = { - operator: entry.operator, - ...(entry.value === undefined ? {} : { value: entry.value }), - }; - } return { ...current, - filters: projectedFilters, + // Tree-aware for the same reason, and by the same pass-through, as + // the committed projection above. + filters: + projectedQuery.filters as unknown as readonly SurfaceFilterNode[], sort: projectedQuery.sort as readonly PretableSortEntry[], rowGroups: ( projectedQuery.rowGroups as readonly { @@ -2693,15 +2695,17 @@ export function PretableSurface< queryWith({ sort }); }, setColumnFilter(columnId: string, filter: ColumnFilter | null) { - const current = currentQuery(); - const filters = ( - current.filters as readonly { - readonly columnId: string; - }[] - ).filter((entry) => entry.columnId !== columnId); + // LEAF-ONLY BY DESIGN, and scoped to the TOP LEVEL: the per-column + // menu can express one operator over one operand, so it owns exactly + // this column's top-level leaf. Every group element passes through by + // reference — a menu commit must never edit or drop a branch the user + // assembled somewhere else. queryWith({ - filters: - filter === null ? filters : [...filters, { columnId, ...filter }], + filters: withTopLevelColumnFilter( + currentQuery().filters as unknown as readonly SurfaceFilterNode[], + columnId, + filter, + ), }); }, setRowGroups(columnIds: readonly string[]) { @@ -3218,6 +3222,8 @@ export function PretableSurface< : null; const current = rowModelSnapshot.query; indexedGrid.setQuery({ + // Tree-agnostic pass-through, as in `queryWith`: grouping changes + // resubmit the filter tree untouched. filters: current.filters, sort: current.sort, // `PretableRowGroupFor` — `never` for the same reason as the @@ -5845,7 +5851,10 @@ export function PretableSurface< togglePopover("filter", id, anchor) @@ -6612,9 +6621,10 @@ export function PretableSurface< type={col.type ?? "text"} allowedOperators={col.filterOperators} options={options} - initialFilter={ - snapshot.filters[filterOpenState.columnId] ?? null - } + initialFilter={topLevelColumnFilter( + snapshot.filters, + filterOpenState.columnId, + )} {...(col.type === "enum" && col.options === undefined ? { loadDistinctValues } : {})} diff --git a/packages/react/src/public_api.ts b/packages/react/src/public_api.ts index 4e87b58cf..8c7702a6d 100644 --- a/packages/react/src/public_api.ts +++ b/packages/react/src/public_api.ts @@ -153,7 +153,11 @@ export type { DensityHeights } from "@pretable/ui"; // Re-exports from @pretable/core (the engine types react users typically // touch — full headless surface lives in @pretable/core) -export { describeRowSelection, numberFormats } from "@pretable/core"; +export { + describeRowSelection, + isPretableFilterGroup, + numberFormats, +} from "@pretable/core"; export type { AutosizeOptions, ColumnAlign, @@ -198,6 +202,8 @@ export type { PretableFocusDirection, PretableFocusState, PretableFilterFor, + PretableFilterGroupFor, + PretableFilterNodeFor, PretableFilterOperandFor, PretableHeaderRowRef, PretableGridUiColumn, From 2e90222a92b88939dac34d621b9eaee9f4033db9 Mon Sep 17 00:00:00 2001 From: Brian Love Date: Tue, 25 Aug 2026 20:58:15 -0700 Subject: [PATCH 12/15] fix(react,core): core changeset, the deferred-gap marker, and reference teeth for the survives-test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings from the surface filter-tree commit. `@pretable/core` gains three public exports and a widened `PretableQueryFor.filters` in that commit but had no changeset. It has one now, and it names the two rules a CHANGELOG reader has to learn before upgrading: an EMPTY group holds for both `and` and `or` (naive algebra disagrees, and would blank the grid on a half-built group), and a tree nesting deeper than 64 levels is rejected by `compileQuery` with `code: "invalid-query"` — a new reason an otherwise well-formed query can be refused. The docs fixture server's `DocsQuery.filters` is leaf-only while the engine's is a tree, and nothing catches it: the type boundary is severed by `JSON.stringify` in each example's `fetch-rows.ts`, so typecheck is green over a real gap. It carries a verdict comment now, including the part that is better than it looks — a group reaches `columnTypeFor(undefined)`, which throws, so the route fails loudly rather than serving wrongly-filtered rows. The survives-test asserted only structural equality of the pass-through group, which a defensive clone would have satisfied. `setQuery` in `pretable-model.ts` hands `onQueryChange` the surface's own object before the row model re-captures it, so the group element IS assertable by reference — and now is. Cloning the group in `withTopLevelColumnFilter` previously failed three helper tests and left all eighteen surface tests green; it now fails the surface test too. Co-Authored-By: Claude Opus 5 --- .changeset/filter-tree-core.md | 42 +++++++++++++++++++ apps/website/app/api/docs/rows/dataset.ts | 23 ++++++++++ .../__tests__/filter-menu-surface.test.tsx | 28 ++++++++++++- 3 files changed, 91 insertions(+), 2 deletions(-) create mode 100644 .changeset/filter-tree-core.md diff --git a/.changeset/filter-tree-core.md b/.changeset/filter-tree-core.md new file mode 100644 index 000000000..e59895f92 --- /dev/null +++ b/.changeset/filter-tree-core.md @@ -0,0 +1,42 @@ +--- +"@pretable/core": minor +--- + +Filters are an AND/OR tree. + +`PretableQueryFor.filters` is still an array, and an array of plain leaves +still means exactly what it meant before — the top level is an implicit AND. +What is new is that an element may also be a **group**: + +```ts +interface PretableFilterGroupFor { + readonly op: "and" | "or"; + readonly children: readonly PretableFilterNodeFor[]; +} +``` + +Groups nest, so a query can express any AND/OR shape. `PretableFilterNodeFor` +is the union of a typed leaf and a group — the type most call sites reading +`filters` want — and `isPretableFilterGroup(node)` narrows one to a group. The +guard checks the group's own fields positively, so an unrecognized shape fails +closed rather than being treated as a branch with no children. + +Two rules a consumer has to know: + +- **An EMPTY group holds — for BOTH operators.** `{ op: "or", children: [] }` + keeps every row, exactly like `{ op: "and", children: [] }`. Naive algebra + says an empty OR is false; that answer is wrong for a product, because a + group the user is still assembling in a filter builder would blank the grid + the moment it appeared. An empty group constrains nothing. +- **Trees deeper than 64 levels are rejected.** `compileQuery` fails such a + query with `code: "invalid-query"` and a `query.filters[i].children[j]…` + path. This is a new reason for an existing rejection, and the only way an + otherwise well-formed query can now be refused. + +Evaluation, query equality (so plan reuse and recompile decisions), capture and +freezing, and `distinctValues` all recurse. Equality stays order-insensitive +per level, which AND and OR both license. + +Nothing in this release builds a group on its own — no UI renders or authors +one yet. `@pretable/react` ships the surface half alongside: funnels light on a +filter at any depth, and the per-column menu owns only its top-level leaf. diff --git a/apps/website/app/api/docs/rows/dataset.ts b/apps/website/app/api/docs/rows/dataset.ts index c57b8ef14..e6ea9541e 100644 --- a/apps/website/app/api/docs/rows/dataset.ts +++ b/apps/website/app/api/docs/rows/dataset.ts @@ -9,6 +9,29 @@ export interface DocsOrder { } export interface DocsQuery { + /** + * LEAF-ONLY, AND KNOWINGLY BEHIND THE ENGINE. `PretableQueryFor.filters` is + * an AND/OR TREE: an element is either a typed leaf or a + * `{ op, children }` GROUP, nestable. This shape admits leaves only. + * + * Nothing catches the mismatch at compile time, and that is not an + * oversight to be fixed by a cast: the type boundary is genuinely severed + * by `JSON.stringify` in each example's `fetch-rows.ts` — a query leaves the + * client as text and arrives here as `unknown`, so `pnpm typecheck` is green + * over a real gap. + * + * The failure is at least LOUD, not silent: a group has no `columnId`, so + * `matches()` calls `columnTypeFor(undefined)`, which throws + * `DocsQueryError` and the route answers with an error rather than with + * wrongly-filtered rows. + * + * Nothing in the docs builds a group yet — the built-in column menu writes + * top-level leaves only — so no example can reach this today. A server + * meeting a real tree has three honest choices (reject, flatten when every + * join is AND, or implement the recursion); which one this fixture makes, + * and how the wire contract states it, belongs to the server-data filter + * page, not here. + */ filters: readonly { columnId: string; operator: string; diff --git a/packages/react/src/__tests__/filter-menu-surface.test.tsx b/packages/react/src/__tests__/filter-menu-surface.test.tsx index d936b727f..3d492edde 100644 --- a/packages/react/src/__tests__/filter-menu-surface.test.tsx +++ b/packages/react/src/__tests__/filter-menu-surface.test.tsx @@ -579,9 +579,25 @@ describe("PretableSurface — filter trees", () => { ], }; const onQueryChange = vi.fn(); + let grid: Parameters>[0] | null = + null; const view = renderTreeSurface( [{ columnId: "title", operator: "contains", value: "alpha" }, group], - { onQueryChange }, + { + onQueryChange, + onGridReady: (ready) => { + grid = ready; + }, + }, + ); + + // The group as the MODEL holds it. `captureFilterNode` allocates and + // freezes a fresh node on every capture, so this is a different object + // from the `group` literal above — and it is the one the menu's write path + // reads, because `setColumnFilter` builds its next query from + // `currentQuery()`. + const capturedGroup = await vi.waitUntil( + () => grid?.rowModel.getState().snapshot.query.filters[1], ); fireEvent.click(view.getByRole("button", { name: "Filter Title" })); @@ -597,11 +613,19 @@ describe("PretableSurface — filter trees", () => { filters: readonly TreeNode[]; }; // The leaf is REPLACED in place, and the group element the menu never - // authored comes through structurally identical, in its original slot. + // authored comes through in its original slot. expect(next.filters).toEqual([ { columnId: "title", operator: "contains", value: "leak" }, group, ]); + // BYTE-IDENTICAL, with teeth: `onQueryChange` is handed the surface's own + // `next` object BEFORE `rowModel.setQuery` re-captures it (see `setQuery` + // in `pretable-model.ts`), so this element is literally the object + // `withTopLevelColumnFilter` passed through — not a copy that merely + // compares equal. A write path that rebuilt groups from a projection, or + // cloned them defensively, would satisfy the `toEqual` above and fail + // here. + expect(next.filters[1]).toBe(capturedGroup); }); it("clearing from the menu removes only the top-level leaf", async () => { From 8c6b0c61d1aca7f38bb7be44f302cb0b553a9b3f Mon Sep 17 00:00:00 2001 From: Brian Love Date: Tue, 25 Aug 2026 21:05:13 -0700 Subject: [PATCH 13/15] docs(server-data): the filter wire contract grows groups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `query.filters` is an AND/OR tree as of this branch, and the shape a server receives changed the moment it merges — so it is documented now, not alongside the builder UI that will produce trees from the UI. The section overview grows "What a filter looks like on the wire": a grouped JSON payload, and the four rules that make it mean what it says — the top-level array is an implicit AND (which is what a filter list has always meant, and what keeps a pre-group payload valid), leaves and groups discriminate on structure rather than a tag (`isPretableFilterGroup` on the client edge, `children` on the server, where no types survive `JSON.stringify`), an empty group is TRUE under either `op` (naive algebra says empty-OR is false, which would blank the grid under a half-built group), and nesting is bounded at 64 with a breadcrumbed `invalid-query` rejection. Then the part a server cannot leave implicit: reject, flatten only when every join is `and`, or implement the recursion — presented with what each one actually costs, because guessing produces the failure this section keeps warning about, a grid that looks filtered and is not. Says plainly that no UI builds a group yet: the funnel still writes its column's top-level leaf, and a group reaches an endpoint only from a query the consumer seeded themselves. The docs' own example server now REJECTS a group explicitly rather than incidentally. It already failed — a group has no `columnId`, so `columnTypeFor` threw — but with "Unknown column undefined", a message about the wrong thing. `matches()` tests for `children` and names the real reason, which is what lets the page cite it as the reject posture and what the page's own rule (a fixture that cannot answer a filter says which) already demanded. No group evaluation: it is a demo of the wire contract, not a filter engine. Co-Authored-By: Claude Opus 5 --- apps/website/app/api/docs/rows/dataset.ts | 37 ++++++++++--- .../content/docs/server-data/index.mdx | 54 ++++++++++++++++++- .../docs/server-data/query-ownership.mdx | 3 +- 3 files changed, 85 insertions(+), 9 deletions(-) diff --git a/apps/website/app/api/docs/rows/dataset.ts b/apps/website/app/api/docs/rows/dataset.ts index e6ea9541e..4a8a57492 100644 --- a/apps/website/app/api/docs/rows/dataset.ts +++ b/apps/website/app/api/docs/rows/dataset.ts @@ -20,17 +20,21 @@ export interface DocsQuery { * client as text and arrives here as `unknown`, so `pnpm typecheck` is green * over a real gap. * - * The failure is at least LOUD, not silent: a group has no `columnId`, so - * `matches()` calls `columnTypeFor(undefined)`, which throws - * `DocsQueryError` and the route answers with an error rather than with - * wrongly-filtered rows. + * So the rejection is a RUNTIME one, and it is deliberate rather than + * incidental: `matches()` tests for `children` and throws `DocsQueryError` + * naming the group, and the route answers with an error rather than with + * wrongly-filtered rows. (Left to itself the mismatch also failed — a group + * has no `columnId`, so `columnTypeFor` threw — but about a column, which + * is not what went wrong.) * * Nothing in the docs builds a group yet — the built-in column menu writes * top-level leaves only — so no example can reach this today. A server * meeting a real tree has three honest choices (reject, flatten when every - * join is AND, or implement the recursion); which one this fixture makes, - * and how the wire contract states it, belongs to the server-data filter - * page, not here. + * join is AND, or implement the recursion). This fixture REJECTS, by name, + * in `matches()`: it is a demo of the wire contract, not a filter engine, + * and implementing the recursion here would teach nothing the engine does + * not already do. The contract itself is stated on the section overview, + * `content/docs/server-data/index.mdx`. */ filters: readonly { columnId: string; @@ -456,6 +460,25 @@ function matches( row: DocsOrder, filter: DocsQuery["filters"][number], ): boolean { + /* + * The rejection this fixture owes the wire contract, said out loud. On the + * wire `query.filters` is an AND/OR tree (see `DocsQuery` above), and a + * group carries `children` where a leaf carries `columnId`. + * + * Without this branch a group was already rejected — `columnTypeFor` + * throws on the missing `columnId` — but with `Unknown column + * "undefined"`, a message about the wrong thing entirely. A fixture whose + * job is to teach that the server applied the filter has to name the + * reason it did not. + */ + if ("children" in filter) { + throw new DocsQueryError( + "This fixture answers leaf filters only, and this query carried a " + + "filter group. A server that does not implement AND/OR groups must " + + "say so rather than drop them: see /docs/server-data.", + ); + } + const type = columnTypeFor(filter.columnId); assertUsable(filter.columnId, type, filter.operator, filter.value); diff --git a/apps/website/content/docs/server-data/index.mdx b/apps/website/content/docs/server-data/index.mdx index 5ff0782f7..1e25670ed 100644 --- a/apps/website/content/docs/server-data/index.mdx +++ b/apps/website/content/docs/server-data/index.mdx @@ -73,10 +73,62 @@ The response is the three things it takes to describe a result — the rows, how } ``` -Two behaviors are deliberate. Every response waits 500 ms before it is sent, which is long enough that `loading` and `stale` are states you can watch rather than infer. And any filter whose value contains **fail** returns a 500, which is how the [lifecycle page](/docs/server-data/lifecycle) reaches the error phase on demand. A filter the fixture genuinely cannot answer — an unknown column, an operator its column's type cannot use, a missing operand — also returns a 500 with a message saying which, rather than quietly returning every row. +Two behaviors are deliberate. Every response waits 500 ms before it is sent, which is long enough that `loading` and `stale` are states you can watch rather than infer. And any filter whose value contains **fail** returns a 500, which is how the [lifecycle page](/docs/server-data/lifecycle) reaches the error phase on demand. A filter the fixture genuinely cannot answer — an unknown column, an operator its column's type cannot use, a missing operand, or an AND/OR group — also returns a 500 with a message saying which, rather than quietly returning every row. That last point is a rule to copy, not a fixture quirk: a backend that ignores a filter it does not understand produces a grid that looks filtered and is not. +## What a filter looks like on the wire + +`query.filters` is an array, and it always was. What changed is what an element of it may be: either a **leaf** — the `{ columnId, operator, value }` shape in the request above — or a **group**, `{ "op": "and" | "or", "children": [...] }`, whose children are themselves leaves or groups, to any depth. `filters` is a tree, and it reaches `onQueryChange` and then your endpoint exactly as the grid built it. Nothing flattens, rewrites, or simplifies it on the way out, and [external filter authority](/docs/server-data/query-ownership#what-external-authority-suppresses) does not either — suppression decides what the engine _applies_, never what it _reports_. + +```json +{ + "query": { + "filters": [ + { "columnId": "total", "operator": "gt", "value": 500 }, + { + "op": "or", + "children": [ + { "columnId": "region", "operator": "isAnyOf", "value": ["North"] }, + { "columnId": "customer", "operator": "contains", "value": "Labs" } + ] + } + ], + "sort": [], + "rowGroups": [] + } +} +``` + +That payload reads `total > 500 AND (region is North OR customer contains "Labs")`, and the four rules that make it mean that are the contract. + +**The top-level array is an implicit AND.** It is what a list of filters has always meant — each entry narrows the result further — so groups became _elements_ of that array rather than a new field beside it, and the ordinary one-leaf-per-column case stays the flat list it was. Two consequences follow from the same rule. A payload written before groups existed is still a correct payload. And `"filters": []` constrains nothing, because an AND over no conditions excludes no rows. + +**Leaves and groups discriminate on structure, not on a tag.** There is no `kind` field to switch on: a group is the node carrying `op` and `children`, a leaf is the node carrying `columnId` and `operator`. On the client edge, `isPretableFilterGroup` — exported from both `@pretable/core` and `@pretable/react` — makes that test and narrows `PretableFilterNodeFor` to `PretableFilterGroupFor`, so nothing has to hand-roll it. On the server there are no types left to narrow: the query arrived as JSON over HTTP, so write the test yourself, and test for `children`. That is the field a group cannot exist without, and the field a leaf never has. + +**An empty group matches every row, under either `op`.** Naive boolean algebra says an empty `or` is false, and that is exactly the wrong answer here: a group with nothing in it is a group someone is part-way through building, and a half-built condition that blanks the grid mid-edit is a bug the reader will read as data loss. So an empty group constrains nothing whichever way it joins — the same answer an empty top-level array gives, for the same reason. Copy that rule into your backend rather than deriving it, or the two sides will disagree about a query the grid considers unfiltered. + +**Nesting is bounded at 64 levels.** A tree deeper than that is rejected with the same typed `invalid-query` error an unknown column gets, and the message breadcrumbs the offending node — `query.filters[0].children[3].children[1]` — so you are told where, not just that. The bound exists because every consumer of a captured query recurses over it, and it sits far above any tree a person or a builder UI produces and far below the depth at which any of that recursion is at risk. In practice the grid rejects a too-deep tree before it can publish one, so your endpoint should never see it; bound your own recursion anyway, since a query can also arrive from a saved view, a URL, or a client that is not this grid. + +### A server that only understands flat filters has to decide + +It cannot be left implicit, because the failure mode of guessing is the one this page keeps warning about: a grid that looks filtered and is not. Three answers are defensible, and which one is right is a property of your backend, not of the grid. + +- **Reject.** Answer with an error the moment a group appears, naming it. Cheapest by far, correct at once, and it fails where a reader can see it — an error strip over the rows they had, rather than a result quietly computed from half of what they asked for. This is what the fixture endpoint above does: `matches()` in `app/api/docs/rows/dataset.ts` tests for `children` and returns a 500 that says so. It is the right posture for a demo, and the right first commit for a real backend too, because it buys you the freedom to implement groups later without having shipped a wrong answer in the meantime. +- **Flatten — but only when every join is `and`.** A tree whose groups all carry `"op": "and"` is genuinely equivalent to the flat list of its leaves, nesting and all, so collecting them loses nothing. The trap is that this is only true until the first `"op": "or"`, and an `or` is precisely what a user reaches for a group to express. So the flattening has to be _conditional_, and its else-branch has to be reject, never best-effort: a tree containing an `or` cannot be approximated by an AND of its leaves in either direction. Note that an empty group contributes no leaves, which is the correct reading of the rule above. +- **Implement the recursion.** It is smaller than it sounds — map a leaf to a predicate as you already do, join a group's children with `AND` or `OR`, parenthesize each group, and return the identity `TRUE` for an empty one. The work you actually owe is the parameter binding you owe leaves anyway, over a shape that now nests; a tree of user-supplied operators and operands assembled into SQL by string concatenation is an injection hole whatever its depth. + + + No UI builds a group yet. The header funnel writes, edits, and removes its own + column's top-level leaf, exactly as it always has; the tool panel's filter + builder is a later sub-project. Until it ships, a group can only reach your + endpoint from a query you put there yourself — seeded through the controlled + `query` prop, restored from a URL, or loaded from a saved view. The contract + is documented now rather than alongside that UI because a shape a server can + receive is a contract from the moment it is possible, not from the moment it + is common. + + ## Where to go next - [Query ownership](/docs/server-data/query-ownership) — the `processing` and `query`/`onQueryChange` contract, what external filtering suppresses, and what it deliberately does not. diff --git a/apps/website/content/docs/server-data/query-ownership.mdx b/apps/website/content/docs/server-data/query-ownership.mdx index d3501e4fb..02083a6ca 100644 --- a/apps/website/content/docs/server-data/query-ownership.mdx +++ b/apps/website/content/docs/server-data/query-ownership.mdx @@ -37,7 +37,7 @@ The grid says so when it can prove it: external `filter`, engine `sort`, and an `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. +Suppression changes what is applied, never what is reported. Shape included: `query.filters` is an AND/OR tree, so a group nested inside it is published through `onQueryChange` exactly as the grid holds it, under either authority — [what a filter looks like on the wire](/docs/server-data#what-a-filter-looks-like-on-the-wire) is what your endpoint receives, and what it owes an answer to. That distinction costs nothing while the rows and the query agree — the server already filtered by that query, so filtering its answer again removes nothing — and it is the whole point the moment they **disagree**, which is exactly what `stale` and `error` are: a query the reader has moved on to, and rows that answer a different one. [The lifecycle](/docs/server-data/lifecycle) deliberately keeps the previous result on screen while the next one loads, and re-applying the new filter to the old rows would empty a body that still holds a perfectly readable result. @@ -112,4 +112,5 @@ Two are surface-only, for different reasons. `query` is absent from `` - [Server-side data](/docs/server-data) — the section overview, its endpoint, and what the grid keeps owning. - [Loading, staleness, errors](/docs/server-data/lifecycle) — the six `dataState` phases, and why a failure never discards rows. +- [Server-side data](/docs/server-data#what-a-filter-looks-like-on-the-wire) — the filter payload's tree shape, and the three things a server that only understands flat filters can do about it. - [Filtering](/docs/grid/filtering) — operator semantics, the funnel menu, and the controlled-query idiom in local form. From d2d4624a8e486facbaa0fce3297aaed6d95f3248 Mon Sep 17 00:00:00 2001 From: Brian Love Date: Tue, 25 Aug 2026 21:12:13 -0700 Subject: [PATCH 14/15] refactor(react): first-wins stated and tested, one narrowing, one erasure helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three semantics moved silently when the per-column record projection was deleted, and none of them was written down. The record was LAST-wins: `filters[entry.columnId] = …` let a later top-level leaf overwrite an earlier one. `topLevelColumnFilter` takes the FIRST, and `withTopLevelColumnFilter` replaces that same one and drops the rest — the two halves agreed at runtime, but only the write side said so. The read side's doc said "the TOP-LEVEL leaf", singular and definite, which left the duplicate case undefined in prose. It says FIRST now, names the change of answer, and both rules have a test: nothing previously constructed two top-level leaves for one column, so neither first-wins nor the duplicate-drop branch was exercised outside the clearing path. The changeset carries the clause too — only a hand-authored `filters` can reach it, but a consumer who does deserves to read it there rather than discover it. Two pieces of duplication go with them. `LabeledGridSurface`'s walk — a second walk for a real reason, since it gates on `isColumnFilterActive` — narrowed through four raw casts because the guard was private; it is exported as `isSurfaceFilterGroup`, and the `as never` rationale now lives in one place. The value-erasure cast was spelled out at three call sites with three cross-referencing comments; `asSurfaceNodes` holds the single explanation and each site keeps only its own tree-semantics note, which is the part that differs. Also corrects the core changeset's depth bound, which was off by one: the check is `depth > 64` with the root at depth 0, so 65 node levels are accepted. Co-Authored-By: Claude Opus 5 --- .changeset/filter-tree-core.md | 10 +-- .changeset/filter-tree-surface.md | 13 ++-- .../react/src/__tests__/filter-tree.test.ts | 37 +++++++++++ packages/react/src/filter-tree.ts | 61 ++++++++++++++----- packages/react/src/labeled-grid-surface.tsx | 26 ++++---- packages/react/src/pretable-surface.tsx | 18 +++--- 6 files changed, 119 insertions(+), 46 deletions(-) diff --git a/.changeset/filter-tree-core.md b/.changeset/filter-tree-core.md index e59895f92..b3a301e89 100644 --- a/.changeset/filter-tree-core.md +++ b/.changeset/filter-tree-core.md @@ -28,10 +28,12 @@ Two rules a consumer has to know: says an empty OR is false; that answer is wrong for a product, because a group the user is still assembling in a filter builder would blank the grid the moment it appeared. An empty group constrains nothing. -- **Trees deeper than 64 levels are rejected.** `compileQuery` fails such a - query with `code: "invalid-query"` and a `query.filters[i].children[j]…` - path. This is a new reason for an existing rejection, and the only way an - otherwise well-formed query can now be refused. +- **Nesting is bounded at 64 levels below the root.** Top-level elements sit at + depth 0, so a node at depth 65 — a group nested more than 64 deep — makes + `compileQuery` fail the query with `code: "invalid-query"` and a + `query.filters[i].children[j]…` path. This is a new reason for an existing + rejection, and the only way an otherwise well-formed query can now be + refused. Evaluation, query equality (so plan reuse and recompile decisions), capture and freezing, and `distinctValues` all recurse. Equality stays order-insensitive diff --git a/.changeset/filter-tree-surface.md b/.changeset/filter-tree-surface.md index 31f7d6b66..b6bc16f14 100644 --- a/.changeset/filter-tree-surface.md +++ b/.changeset/filter-tree-surface.md @@ -16,11 +16,16 @@ surface's chrome follows: projected out of the query; a group carries no `columnId`, so that record would have collapsed every group onto the single key `undefined` and left the funnel dark. The record is gone — the surface holds the tree verbatim. -- The **column filter menu** owns exactly its column's TOP-LEVEL leaf. It +- The **column filter menu** owns exactly its column's FIRST top-level leaf. It hydrates from that leaf (never from one nested in a group), and a commit - replaces it in place. Every group element passes through by reference: a menu - commit cannot edit, reorder, or drop a branch it did not author, and clearing - a column removes only its top-level leaf. + replaces it in its existing slot rather than removing it and appending at the + end. Every group element passes through by reference: a menu commit cannot + edit, reorder, or drop a branch it did not author, and clearing a column + removes only its top-level leaf. Two ordering details change for a + hand-authored `filters` that carries duplicate top-level leaves for one + column — nothing the menu can produce: the menu now reads the FIRST of them + (the per-column record it replaced was last-wins), and a commit collapses + them to the single leaf it just wrote. - **Controlled queries** take the tree shape. A controlled `query.filters` containing groups renders funnels and filters rows exactly as the engine evaluates it. diff --git a/packages/react/src/__tests__/filter-tree.test.ts b/packages/react/src/__tests__/filter-tree.test.ts index 723a17ef3..71bfaae1d 100644 --- a/packages/react/src/__tests__/filter-tree.test.ts +++ b/packages/react/src/__tests__/filter-tree.test.ts @@ -98,6 +98,22 @@ describe("topLevelColumnFilter", () => { }); }); +it("reads the FIRST of two top-level leaves for the same column", () => { + // Only a hand-authored `filters` can hold duplicates — the menu never + // writes a second leaf for a column. When one does, first wins. The + // per-column record this replaced was LAST-wins (each entry overwrote the + // key), so this is a deliberate change of answer, not an accident. + const filters: readonly SurfaceFilterNode[] = [ + { columnId: "a", operator: "contains", value: "one" }, + { columnId: "a", operator: "equals", value: "two" }, + { op: "or", children: [] }, + ]; + expect(topLevelColumnFilter(filters, "a")).toEqual({ + operator: "contains", + value: "one", + }); +}); + describe("withTopLevelColumnFilter", () => { const group: SurfaceFilterNode = { op: "or", @@ -147,6 +163,27 @@ describe("withTopLevelColumnFilter", () => { expect(next[0]).toBe(group); }); + it("collapses two top-level leaves for the same column into one", () => { + // The read side takes the FIRST duplicate, so the write side must replace + // that same one and drop the rest: leaving a second `a` leaf behind would + // make the commit look inert, because the query would still carry the + // operand the user just replaced. + const filters: readonly SurfaceFilterNode[] = [ + { columnId: "a", operator: "contains", value: "one" }, + { columnId: "a", operator: "equals", value: "two" }, + group, + ]; + const next = withTopLevelColumnFilter(filters, "a", { + operator: "startsWith", + value: "three", + }); + expect(next).toEqual([ + { columnId: "a", operator: "startsWith", value: "three" }, + group, + ]); + expect(next[1]).toBe(group); + }); + it("omits `value` when the committed filter has none", () => { const next = withTopLevelColumnFilter([], "a", { operator: "isEmpty" }); expect(next).toEqual([{ columnId: "a", operator: "isEmpty" }]); diff --git a/packages/react/src/filter-tree.ts b/packages/react/src/filter-tree.ts index 2d8933187..df4b18952 100644 --- a/packages/react/src/filter-tree.ts +++ b/packages/react/src/filter-tree.ts @@ -28,13 +28,39 @@ export interface SurfaceFilterGroup { export type SurfaceFilterNode = SurfaceFilterLeaf | SurfaceFilterGroup; /** - * `isPretableFilterGroup` is generic over a static column tuple; the surface's - * nodes are value-erased, and `as never` is what satisfies the parameter - * without weakening the guard — it is structural at runtime. Same collapse, - * and the same remedy, as the `distinctValues` call in `pretable-surface.tsx`. + * `PretableQueryFor["filters"]` read as value-erased nodes. + * + * `PretableFilterNodeFor` discriminates its leaves over the column tuple's + * static `accessor` return types and literal `type`s. The surface's columns are + * runtime-supplied and value-erased, so that union collapses and no assignment + * between the two shapes is checkable — the same collapse `queryWith` and + * `distinctValues` document, and the reason this is a cast and not a + * conversion. It is the single place the erasure is spelled out; call sites + * carry only their own tree-semantics comment. */ -const isGroup = (node: SurfaceFilterNode): node is SurfaceFilterGroup => - isPretableFilterGroup(node as never); +export function asSurfaceNodes( + filters: readonly unknown[], +): readonly SurfaceFilterNode[] { + return filters as readonly SurfaceFilterNode[]; +} + +/** + * Narrows a value-erased node to a group — the surface's `isPretableFilterGroup`. + * + * The engine's guard is generic over a static column tuple, and the surface's + * nodes are value-erased, so `as never` is what satisfies the parameter. It + * does not weaken the check: the guard is structural at runtime and tests the + * group's own fields. Same collapse, and the same remedy, as the + * `distinctValues` call in `pretable-surface.tsx`. + * + * Exported so the SECOND walk over the tree — `LabeledGridSurface`'s + * `is-filtered` header decoration, which cannot share these functions because + * it gates on `isColumnFilterActive` — narrows through this one explanation + * instead of repeating the casts. + */ +export const isSurfaceFilterGroup = ( + node: SurfaceFilterNode, +): node is SurfaceFilterGroup => isPretableFilterGroup(node as never); /** * Does ANY leaf anywhere in the tree constrain `columnId`? @@ -49,27 +75,34 @@ export function columnHasFilter( columnId: string, ): boolean { return nodes.some((node) => - isGroup(node) + isSurfaceFilterGroup(node) ? columnHasFilter(node.children, columnId) : node.columnId === columnId, ); } /** - * The column's TOP-LEVEL leaf, as the per-column filter menu understands it — - * or `null` when only a group mentions the column. + * The column's FIRST top-level leaf, as the per-column filter menu understands + * it — or `null` when only a group mentions the column. * * The menu edits one column with one operator and one operand; it cannot - * express a group, so it owns exactly the top-level leaf and reports nothing - * about the rest of the tree. Reaching into groups here would let a menu - * commit silently overwrite a branch the user built elsewhere. + * express a group, so it owns exactly that leaf and reports nothing about the + * rest of the tree. Reaching into groups here would let a menu commit silently + * overwrite a branch the user built elsewhere. + * + * FIRST, definitely, and it matters: only a hand-authored `filters` can hold + * two top-level leaves for one column, but when it does, this reads the + * earlier one and `withTopLevelColumnFilter` replaces that same one — the two + * halves agree, which is the point. The per-column record this replaced was + * LAST-wins (each entry overwrote the key), so a consumer with duplicates sees + * the other leaf now. */ export function topLevelColumnFilter( nodes: readonly SurfaceFilterNode[], columnId: string, ): ColumnFilter | null { for (const node of nodes) { - if (isGroup(node) || node.columnId !== columnId) continue; + if (isSurfaceFilterGroup(node) || node.columnId !== columnId) continue; return { operator: node.operator, ...(node.value === undefined ? {} : { value: node.value }), @@ -103,7 +136,7 @@ export function withTopLevelColumnFilter( let replaced = false; const next: SurfaceFilterNode[] = []; for (const node of nodes) { - if (isGroup(node) || node.columnId !== columnId) { + if (isSurfaceFilterGroup(node) || node.columnId !== columnId) { next.push(node); continue; } diff --git a/packages/react/src/labeled-grid-surface.tsx b/packages/react/src/labeled-grid-surface.tsx index 3c25f508a..9087bd02f 100644 --- a/packages/react/src/labeled-grid-surface.tsx +++ b/packages/react/src/labeled-grid-surface.tsx @@ -1,14 +1,13 @@ -import { isPretableFilterGroup } from "@pretable/core"; import type { PretableRow, PretableRowId, PretableSortDirection, PretableQueryFor, } from "@pretable/core"; -import type { - SurfaceFilterGroup, - SurfaceFilterLeaf, - SurfaceFilterNode, +import { + asSurfaceNodes, + isSurfaceFilterGroup, + type SurfaceFilterNode, } from "./filter-tree"; import type { HTMLAttributes } from "react"; import type { PretableTelemetry } from "./surface-types"; @@ -39,16 +38,20 @@ function isColumnFilterActive(filter: { /** * Every column an ACTIVE leaf constrains, at any depth. Groups carry no * `columnId`; they are recursed into, never counted. + * + * A SECOND walk rather than `columnHasFilter`: this one gates on + * `isColumnFilterActive`, so a filter with no usable operand yet decorates + * nothing. The narrowing is shared even though the walk is not. */ function collectActiveFilterColumns( nodes: readonly SurfaceFilterNode[], into: Set, ): void { for (const node of nodes) { - if (isPretableFilterGroup(node as never)) { - collectActiveFilterColumns((node as SurfaceFilterGroup).children, into); - } else if (isColumnFilterActive(node as SurfaceFilterLeaf)) { - into.add((node as SurfaceFilterLeaf).columnId); + if (isSurfaceFilterGroup(node)) { + collectActiveFilterColumns(node.children, into); + } else if (isColumnFilterActive(node)) { + into.add(node.columnId); } } } @@ -211,10 +214,7 @@ export function LabeledGridSurface< // makes just as true as a top-level one. See `columnHasFilter` in // `./filter-tree` for the same walk on the surface's own funnel. collectActiveFilterColumns( - // Value-erased, like every other filter read outside the row model: - // `PretableFilterNodeFor` is discriminated over the static column - // tuple's operand types, and this walk cares only about `columnId`. - (query?.filters ?? []) as unknown as readonly SurfaceFilterNode[], + asSurfaceNodes(query?.filters ?? []), activeFilterColumns, ); const getFormattedValue = ({ diff --git a/packages/react/src/pretable-surface.tsx b/packages/react/src/pretable-surface.tsx index e00bd9362..6ec68fcb8 100644 --- a/packages/react/src/pretable-surface.tsx +++ b/packages/react/src/pretable-surface.tsx @@ -392,6 +392,7 @@ import { type PretableDataState, } from "./data-state"; import { + asSurfaceNodes, columnHasFilter, topLevelColumnFilter, withTopLevelColumnFilter, @@ -2189,12 +2190,8 @@ export function PretableSurface< ); const snapshot = useMemo(() => { // TREE-AWARE, by passing through: the filter tree reaches the chrome - // exactly as the model holds it. The value erasure is the same one - // `queryWith` documents — `PretableFilterFor` collapses to - // `never` against runtime-supplied columns, so the surface reads the - // nodes through their value-erased twin. - const filters = rowModelSnapshot.query - .filters as unknown as readonly SurfaceFilterNode[]; + // exactly as the model holds it — leaves, groups and nesting intact. + const filters = asSurfaceNodes(rowModelSnapshot.query.filters); const ranges = indexedSnapshot.selection.ranges.map(flattenIndexedRange); const ref = indexedSnapshot.focus.ref; return { @@ -2479,10 +2476,9 @@ export function PretableSurface< return current; return { ...current, - // Tree-aware for the same reason, and by the same pass-through, as - // the committed projection above. - filters: - projectedQuery.filters as unknown as readonly SurfaceFilterNode[], + // Tree-aware by the same pass-through as the committed projection + // above; this one just reads the not-yet-settled query. + filters: asSurfaceNodes(projectedQuery.filters), sort: projectedQuery.sort as readonly PretableSortEntry[], rowGroups: ( projectedQuery.rowGroups as readonly { @@ -2702,7 +2698,7 @@ export function PretableSurface< // assembled somewhere else. queryWith({ filters: withTopLevelColumnFilter( - currentQuery().filters as unknown as readonly SurfaceFilterNode[], + asSurfaceNodes(currentQuery().filters), columnId, filter, ), From 7ea17398362ef2d32c5da5413572963fd61fe398 Mon Sep 17 00:00:00 2001 From: Brian Love Date: Tue, 25 Aug 2026 21:23:05 -0700 Subject: [PATCH 15/15] fix(website): the fixture rejects a filter group up front, as the page promises MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The overview says the fixture endpoint errors "the moment a group appears". It did not. The rejection lived inside `matches()`, which `applyDocsQuery` calls per ROW inside `rows.filter(...)`, so it was reachable only when some row survived every earlier leaf. Two shapes therefore answered 200 with zero rows and no throw: filters: [{ region isAnyOf ["Nowhere"] }, { op: "or", children: [] }] any group at all over an empty `rows` Zero rows and an error are not the same answer. The first reads as "nothing matched" — a result quietly computed from less than the reader asked for, which is the failure this whole section argues against, and it does not stop being that because the result happens to be empty. So `applyDocsQuery` now scans `filters` for `children` before it reads a row. Whether a query is one this fixture can answer is a question about the QUERY, independent of the data, and it is asked once, where that is true. `matches()` keeps its branch as belt-and-braces for a direct caller, and both throw through one `rejectFilterGroup` so the wording cannot drift; the up-front path also breadcrumbs the offending index. Four tests pin it, including the two shapes that used to slip through and a positive control that leaf-only queries over the same shapes still answer. Removing the scan fails exactly the three group tests. Two staleness fixes alongside: - `server-data.types.tsx` said index.mdx has "both of its fences"; the grouped payload made it three. The page is still deliberately unbound — all three fences are JSON. - `query-ownership.mdx`'s See-also had two bullets titled "Server-side data" pointing at the same page. Mine is now "The filter wire contract". The Reject bullet on the page now describes the up-front scan and says why the placement, not just the check, is the part worth copying. Co-Authored-By: Claude Opus 5 --- .../api/docs/rows/__tests__/dataset.test.ts | 49 +++++++++++++++++ apps/website/app/api/docs/rows/dataset.ts | 55 +++++++++++++++---- .../app/docs/__tests__/server-data.types.tsx | 7 ++- .../content/docs/server-data/index.mdx | 2 +- .../docs/server-data/query-ownership.mdx | 2 +- 5 files changed, 98 insertions(+), 17 deletions(-) diff --git a/apps/website/app/api/docs/rows/__tests__/dataset.test.ts b/apps/website/app/api/docs/rows/__tests__/dataset.test.ts index 86b8e2a60..e2239af50 100644 --- a/apps/website/app/api/docs/rows/__tests__/dataset.test.ts +++ b/apps/website/app/api/docs/rows/__tests__/dataset.test.ts @@ -457,6 +457,55 @@ describe("queries this fixture cannot answer", () => { /array of selected values/, ); }); + + /* + * On the wire `query.filters` is an AND/OR tree, and this fixture answers + * leaves only. Rejecting is the posture the server-data overview documents, + * so it is pinned here — including in the shapes where the rejection was + * NOT reached before the check moved ahead of the row loop. Each of these + * three returned 200 with zero rows, which is a result computed from less + * than the query asked for and reads to a reader as "nothing matched". + */ + const GROUP = { + op: "or", + children: [], + } as unknown as DocsQuery["filters"][number]; + + test("a filter group is rejected by name, not by a message about a column", () => { + expect(() => + applyDocsQuery(DOCS_ORDERS, { ...EMPTY_DOCS_QUERY, filters: [GROUP] }), + ).toThrow(/carried a filter group at query\.filters\[0\]/); + }); + + test("a filter group behind a leaf that matches nothing is still rejected", () => { + expect(() => + applyDocsQuery(DOCS_ORDERS, { + ...EMPTY_DOCS_QUERY, + filters: [ + { columnId: "region", operator: "isAnyOf", value: ["Nowhere"] }, + GROUP, + ], + }), + ).toThrow(/carried a filter group at query\.filters\[1\]/); + }); + + test("a filter group over no rows at all is still rejected", () => { + expect(() => + applyDocsQuery([], { ...EMPTY_DOCS_QUERY, filters: [GROUP] }), + ).toThrow(DocsQueryError); + }); + + test("but a leaf-only query over the same shapes still answers", () => { + expect( + applyDocsQuery(DOCS_ORDERS, { + ...EMPTY_DOCS_QUERY, + filters: [ + { columnId: "region", operator: "isAnyOf", value: ["North"] }, + ], + }).length, + ).toBeGreaterThan(0); + expect(applyDocsQuery([], EMPTY_DOCS_QUERY)).toEqual([]); + }); }); describe("totalFor", () => { diff --git a/apps/website/app/api/docs/rows/dataset.ts b/apps/website/app/api/docs/rows/dataset.ts index 4a8a57492..bd0eb7ff5 100644 --- a/apps/website/app/api/docs/rows/dataset.ts +++ b/apps/website/app/api/docs/rows/dataset.ts @@ -21,11 +21,17 @@ export interface DocsQuery { * over a real gap. * * So the rejection is a RUNTIME one, and it is deliberate rather than - * incidental: `matches()` tests for `children` and throws `DocsQueryError` - * naming the group, and the route answers with an error rather than with - * wrongly-filtered rows. (Left to itself the mismatch also failed — a group - * has no `columnId`, so `columnTypeFor` threw — but about a column, which - * is not what went wrong.) + * incidental: `applyDocsQuery` scans `filters` for `children` BEFORE it + * reads a row and throws `DocsQueryError` naming the group, and the route + * answers with an error rather than with wrongly-filtered rows. + * + * Before the scan, per-row was the only check, and it was reachable only + * when a row survived the leaves ahead of it — so a leaf matching nothing, + * or an empty dataset, answered 200 with zero rows and no throw at all. See + * `applyDocsQuery` for why well-formedness is asked once, of the query. + * (Left to itself the mismatch also failed — a group has no `columnId`, so + * `columnTypeFor` threw — but about a column, which is not what went + * wrong.) * * Nothing in the docs builds a group yet — the built-in column menu writes * top-level leaves only — so no example can reach this today. A server @@ -456,6 +462,19 @@ function matchesText( } } +/** + * One wording for the one thing this fixture refuses, so the up-front scan in + * `applyDocsQuery` and the per-row branch in `matches()` cannot drift apart. + */ +function rejectFilterGroup(index?: number): never { + const where = index === undefined ? "" : ` at query.filters[${index}]`; + throw new DocsQueryError( + `This fixture answers leaf filters only, and this query carried a filter group${where}. ` + + "A server that does not implement AND/OR groups must say so rather " + + "than drop them: see /docs/server-data.", + ); +} + function matches( row: DocsOrder, filter: DocsQuery["filters"][number], @@ -471,13 +490,7 @@ function matches( * job is to teach that the server applied the filter has to name the * reason it did not. */ - if ("children" in filter) { - throw new DocsQueryError( - "This fixture answers leaf filters only, and this query carried a " + - "filter group. A server that does not implement AND/OR groups must " + - "say so rather than drop them: see /docs/server-data.", - ); - } + if ("children" in filter) rejectFilterGroup(); const type = columnTypeFor(filter.columnId); assertUsable(filter.columnId, type, filter.operator, filter.value); @@ -505,6 +518,24 @@ export function applyDocsQuery( rows: readonly DocsOrder[], query: DocsQuery, ): DocsOrder[] { + /* + * The group rejection has to happen HERE, before a single row is read. + * `matches()` carries the same test, but it runs per row inside the loop + * below, so it is reachable only if some row survives every earlier leaf: + * `[{ region isAnyOf ["Nowhere"] }, ]` — and any query at all over an + * empty `rows` — short-circuited to zero matches and answered 200 with no + * throw. A result quietly computed from less than the reader asked for is + * the one failure these pages exist to argue against, and it does not stop + * being that because the result happens to be empty. + * + * A query is well-formed or it is not, independently of the data; the check + * belongs where that question is asked once. `matches()` keeps its branch as + * belt-and-braces for any future caller that reaches it directly. + */ + for (const [index, filter] of query.filters.entries()) { + if ("children" in filter) rejectFilterGroup(index); + } + const filtered = rows.filter((row) => query.filters.every((filter) => matches(row, filter)), ); diff --git a/apps/website/app/docs/__tests__/server-data.types.tsx b/apps/website/app/docs/__tests__/server-data.types.tsx index 6b04389e2..1a86edcf8 100644 --- a/apps/website/app/docs/__tests__/server-data.types.tsx +++ b/apps/website/app/docs/__tests__/server-data.types.tsx @@ -7,9 +7,10 @@ * fence under that heading. The preamble above the first marker is prepended to * every region, which is what lets several snippets share one import. * - * `server-data/index.mdx` is deliberately NOT bound here: both of its fences - * are JSON request and response bodies for `POST /api/docs/rows`, and there is - * no TypeScript on the page to anchor a region to. Binding it would buy two + * `server-data/index.mdx` is deliberately NOT bound here: all three of its + * fences are JSON — the request and response bodies for `POST /api/docs/rows`, + * and a grouped `filters` payload — and there is no TypeScript on the page to + * anchor a region to. Binding it would buy two * `UNTRANSCRIBED_FENCES` excuses and nothing else. The route's own shapes are * held by the app's typecheck where they are declared. */ diff --git a/apps/website/content/docs/server-data/index.mdx b/apps/website/content/docs/server-data/index.mdx index 1e25670ed..f7aaa4ec3 100644 --- a/apps/website/content/docs/server-data/index.mdx +++ b/apps/website/content/docs/server-data/index.mdx @@ -114,7 +114,7 @@ That payload reads `total > 500 AND (region is North OR customer contains "Labs" It cannot be left implicit, because the failure mode of guessing is the one this page keeps warning about: a grid that looks filtered and is not. Three answers are defensible, and which one is right is a property of your backend, not of the grid. -- **Reject.** Answer with an error the moment a group appears, naming it. Cheapest by far, correct at once, and it fails where a reader can see it — an error strip over the rows they had, rather than a result quietly computed from half of what they asked for. This is what the fixture endpoint above does: `matches()` in `app/api/docs/rows/dataset.ts` tests for `children` and returns a 500 that says so. It is the right posture for a demo, and the right first commit for a real backend too, because it buys you the freedom to implement groups later without having shipped a wrong answer in the meantime. +- **Reject.** Answer with an error the moment a group appears, naming it. Cheapest by far, correct at once, and it fails where a reader can see it — an error strip over the rows they had, rather than a result quietly computed from half of what they asked for. This is what the fixture endpoint above does: `applyDocsQuery` in `app/api/docs/rows/dataset.ts` scans `filters` for `children` before it reads a single row, and returns a 500 that says so. Scanning up front rather than inside the row predicate is the part worth copying — a per-row check is reachable only if some row survives the leaves ahead of it, so a leaf matching nothing would have answered an empty result instead of an error. Whether a query is one you can answer is a question about the query, and it is asked once. It is the right posture for a demo, and the right first commit for a real backend too, because it buys you the freedom to implement groups later without having shipped a wrong answer in the meantime. - **Flatten — but only when every join is `and`.** A tree whose groups all carry `"op": "and"` is genuinely equivalent to the flat list of its leaves, nesting and all, so collecting them loses nothing. The trap is that this is only true until the first `"op": "or"`, and an `or` is precisely what a user reaches for a group to express. So the flattening has to be _conditional_, and its else-branch has to be reject, never best-effort: a tree containing an `or` cannot be approximated by an AND of its leaves in either direction. Note that an empty group contributes no leaves, which is the correct reading of the rule above. - **Implement the recursion.** It is smaller than it sounds — map a leaf to a predicate as you already do, join a group's children with `AND` or `OR`, parenthesize each group, and return the identity `TRUE` for an empty one. The work you actually owe is the parameter binding you owe leaves anyway, over a shape that now nests; a tree of user-supplied operators and operands assembled into SQL by string concatenation is an injection hole whatever its depth. diff --git a/apps/website/content/docs/server-data/query-ownership.mdx b/apps/website/content/docs/server-data/query-ownership.mdx index 02083a6ca..34a94a69d 100644 --- a/apps/website/content/docs/server-data/query-ownership.mdx +++ b/apps/website/content/docs/server-data/query-ownership.mdx @@ -112,5 +112,5 @@ Two are surface-only, for different reasons. `query` is absent from `` - [Server-side data](/docs/server-data) — the section overview, its endpoint, and what the grid keeps owning. - [Loading, staleness, errors](/docs/server-data/lifecycle) — the six `dataState` phases, and why a failure never discards rows. -- [Server-side data](/docs/server-data#what-a-filter-looks-like-on-the-wire) — the filter payload's tree shape, and the three things a server that only understands flat filters can do about it. +- [The filter wire contract](/docs/server-data#what-a-filter-looks-like-on-the-wire) — the filter payload's tree shape, and the three things a server that only understands flat filters can do about it. - [Filtering](/docs/grid/filtering) — operator semantics, the funnel menu, and the controlled-query idiom in local form.