From fde5f1e57dbae04e5e8e0617cc09a7e24b8ca14d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 23:50:19 +0000 Subject: [PATCH] fix(example-showcase): make the react rollups use the adapter's query contract (#10288) `renewals-pipeline` passed `top: 500` and `crm-workbench` passed `limit: 200` to `adapter.find`. Neither is a query option. `QueryParams` (objectui packages/types/src/data.ts) declares only `$`-prefixed keys and `ObjectStackAdapter.convertQueryParams` (objectui packages/data-objectstack/src/index.ts) builds its outgoing options by copying exactly those, so the key reached no branch and was dropped with no warning. The consequence is the inverse of the filed one: the GET list route has NO default page size (packages/client/src/index.ts, pinned in packages/client/src/client.test.ts and measured by objectql's `baseline - no params returns every row`), so an absent `top` returns the ENTIRE match set. The cap the author wrote simply never happened. The same effect then read its rows off `.records`. `find()` resolves to a normalized `QueryResult` -- `data` plus `total`, never the REST envelope -- so `pr.records` was `undefined` on every call and the renewals KPI strip sat at 0/0/0 while the `` beside it showed the same rows correctly. `crm-workbench` carries a comment about exactly that defect, fixed there and still live here. Applying the cap is only half a fix: `data.length` under a `$top` IS the silently-capped count the card was filed about. Both pages now count the envelope's `total` -- the server's real count over the same `$filter` whenever a limit was applied. "Open AR" is a per-row verdict over the fetched window, the one number a cap genuinely bounds, so it renders as `100+` rather than passing for a total. `test/react-page-adapter-query-contract.test.ts` executes the page's real rollup effect against a contract-faithful adapter double and then sweeps every `kind:'react'` page in the app for both contracts, with an extraction control, a census control and a positive control on the scanners. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt --- ...wcase-react-page-adapter-query-contract.md | 31 ++ .../src/ui/pages/crm-workbench.page.ts | 14 +- .../src/ui/pages/renewals-pipeline.page.ts | 46 ++- .../react-page-adapter-query-contract.test.ts | 328 ++++++++++++++++++ 4 files changed, 407 insertions(+), 12 deletions(-) create mode 100644 .changeset/showcase-react-page-adapter-query-contract.md create mode 100644 examples/app-showcase/test/react-page-adapter-query-contract.test.ts diff --git a/.changeset/showcase-react-page-adapter-query-contract.md b/.changeset/showcase-react-page-adapter-query-contract.md new file mode 100644 index 0000000000..79288c37e5 --- /dev/null +++ b/.changeset/showcase-react-page-adapter-query-contract.md @@ -0,0 +1,31 @@ +--- +"@objectstack/example-showcase": patch +--- + +Fix the showcase react pages' `useAdapter()` query contract, and pin it (#10288) + +`renewals-pipeline` passed `top: 500` and `crm-workbench` passed `limit: 200` to +`adapter.find`. Neither is a query option: `QueryParams` declares only `$`-prefixed keys +and `ObjectStackAdapter.convertQueryParams` copies exactly those, so the key reached no +branch and was dropped with no error. The consequence is the opposite of a truncated +read — the GET list route has **no default page size**, so an absent `top` returns the +ENTIRE match set, and the cap the author wrote never happened. + +The same effect then read its rows off `.records`. `find()` resolves to a normalized +`QueryResult` (`data` + `total`), never the REST envelope, so `pr.records` was +`undefined` on every call and the renewals KPI strip sat at `0 / 0 / 0` while the +`` beside it showed the same rows correctly. Measured on a 640-row account with +the real page source driven against a contract-faithful adapter double: before, +`$top` arrives `undefined` and the strip reads `{projects: 0, invoices: 0, openInvoices: 0}`; +after, the cap is applied and it reads `{projects: 640, invoices: 640, openInvoices: 100, +capped: true}`. + +Applying the cap is only half a fix, because `data.length` under a `$top` is exactly the +silently-capped count the card was filed about — so both pages now count the envelope's +`total` (the server's real count over the same `$filter` whenever a limit was applied). +The one number a cap genuinely bounds, "Open AR", is a per-row verdict over the fetched +window; it renders as `100+` rather than passing for a total. + +`test/react-page-adapter-query-contract.test.ts` executes the page's real rollup effect +and then sweeps every `kind:'react'` page in the app for both contracts, with an +extraction control, a census control, and a positive control on the scanners. diff --git a/examples/app-showcase/src/ui/pages/crm-workbench.page.ts b/examples/app-showcase/src/ui/pages/crm-workbench.page.ts index 2024db3503..51e65b4be1 100644 --- a/examples/app-showcase/src/ui/pages/crm-workbench.page.ts +++ b/examples/app-showcase/src/ui/pages/crm-workbench.page.ts @@ -36,9 +36,19 @@ function Page() { // a "records" array. Reading .records here always missed, so the KPI cards // silently stuck at 0 even though the ListView beside them showed the same // rows. Read .data first, with .records/array fallbacks for robustness. - const all = await adapter.find('showcase_project', { limit: 200 }); + // + // The cap is $top, not 'limit': QueryParams declares only $-prefixed keys + // and the adapter copies only those, so a bare 'limit' is dropped without + // an error and the read runs unbounded. And once a cap IS applied, the + // headline count has to come from the envelope's 'total' (the server's + // real count over the same filter) — rows.length would report 200 for + // every workspace with more than 200 projects, and look right doing it. + // "Active" stays a per-row verdict over the 200 rows actually fetched; + // an exact one would need its own filtered count query. + const all = await adapter.find('showcase_project', { $top: 200 }); const rows = Array.isArray(all) ? all : (all && (all.data || all.records)) || []; - setStats({ total: rows.length, active: rows.filter((r) => r.status === 'active').length }); + const total = typeof (all && all.total) === 'number' ? all.total : rows.length; + setStats({ total, active: rows.filter((r) => r.status === 'active').length }); } catch (e) { console.warn('[CRM Workbench] failed to refresh stats', e); } }, [adapter]); React.useEffect(() => { refreshStats(); }, [refreshStats, reloadKey]); diff --git a/examples/app-showcase/src/ui/pages/renewals-pipeline.page.ts b/examples/app-showcase/src/ui/pages/renewals-pipeline.page.ts index 38a20f3d14..2814e166ba 100644 --- a/examples/app-showcase/src/ui/pages/renewals-pipeline.page.ts +++ b/examples/app-showcase/src/ui/pages/renewals-pipeline.page.ts @@ -13,7 +13,9 @@ import { definePage } from '@objectstack/spec/ui'; * * The 360 panel deliberately shows BOTH rollup styles side by side: * • hand-rolled — a `useAdapter()` effect counts related projects/invoices - * into a KPI strip (full control, you own loading/refresh), vs + * into a KPI strip (full control, you own loading/refresh, and you own the + * adapter's `$`-prefixed option contract and its `QueryResult` shape — see + * the numbered note on the effect), vs * • framework blocks — ``/`` do the same cross-object * reads declaratively (zero data code). * (This comparison absorbed the former Account Cockpit page.) @@ -56,19 +58,43 @@ function Page() { const [editing, setEditing] = React.useState(false); const [reload, setReload] = React.useState(0); const [stage, setStage] = React.useState('active'); - const [related, setRelated] = React.useState({ projects: 0, invoices: 0, openInvoices: 0 }); + const [related, setRelated] = React.useState({ projects: 0, invoices: 0, openInvoices: 0, capped: false }); // Hand-rolled rollup: the imperative counterpart of the framework blocks - // below. You own the queries, loading, and refresh (reload bumps re-run it). + // below. You own the queries, loading, and refresh (reload bumps re-run it) -- + // and, because you own them, you own the two adapter contracts the blocks hide: + // + // 1. QUERY OPTIONS ARE $-PREFIXED. QueryParams declares $select, $filter, + // $orderby, $skip, $top, $expand, $search, $count, and the adapter copies + // ONLY those. A bare 'top:' / 'limit:' reaches no branch and is dropped + // with no error, so the cap you wrote is never applied -- and the list + // route has no default page size, so the query comes back carrying EVERY + // matching row. + // 2. THE RESULT IS A QueryResult, NOT THE REST ENVELOPE. Rows arrive under + // 'data' (never 'records'), beside 'total' -- which, whenever $top was + // applied, is the server's real count over the same $filter rather than + // the page length. Counting data.length under a cap is how a KPI starts + // under-reporting in silence; count 'total' and the cap stays a fetch + // bound instead of a lie about the business. React.useEffect(() => { let alive = true; (async () => { - if (!adapter || !sel) { setRelated({ projects: 0, invoices: 0, openInvoices: 0 }); return; } - const pr = await adapter.find('showcase_project', { $filter: ['account', '=', sel], top: 500 }); - const iv = await adapter.find('showcase_invoice', { $filter: ['account', '=', sel], top: 500 }); - const projects = Array.isArray(pr) ? pr : (pr && pr.records) || []; - const invoices = Array.isArray(iv) ? iv : (iv && iv.records) || []; - if (alive) setRelated({ projects: projects.length, invoices: invoices.length, openInvoices: invoices.filter((r) => r.status !== 'paid' && r.status !== 'void').length }); + if (!adapter || !sel) { setRelated({ projects: 0, invoices: 0, openInvoices: 0, capped: false }); return; } + const pr = await adapter.find('showcase_project', { $filter: ['account', '=', sel], $top: 500 }); + const iv = await adapter.find('showcase_invoice', { $filter: ['account', '=', sel], $top: 500 }); + const rows = (res) => (Array.isArray(res) ? res : (res && res.data) || []); + const count = (res, list) => (typeof (res && res.total) === 'number' ? res.total : list.length); + const projects = rows(pr); + const invoices = rows(iv); + // Open AR is a per-row verdict, so it can only be read off the rows we + // actually fetched -- the one number here the cap genuinely bounds. The + // 'capped' flag says so on screen instead of letting it pass for a total. + if (alive) setRelated({ + projects: count(pr, projects), + invoices: count(iv, invoices), + openInvoices: invoices.filter((r) => r.status !== 'paid' && r.status !== 'void').length, + capped: invoices.length < count(iv, invoices), + }); })(); return () => { alive = false; }; }, [adapter, sel, reload]); @@ -132,7 +158,7 @@ function Page() {
- +
diff --git a/examples/app-showcase/test/react-page-adapter-query-contract.test.ts b/examples/app-showcase/test/react-page-adapter-query-contract.test.ts new file mode 100644 index 0000000000..9fcbe2a842 --- /dev/null +++ b/examples/app-showcase/test/react-page-adapter-query-contract.test.ts @@ -0,0 +1,328 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; + +import * as pages from '../src/ui/pages/index.js'; +import { RenewalsPipelinePage } from '../src/ui/pages/index.js'; + +/** + * The two `useAdapter()` contracts a hand-rolled rollup owns, pinned against the + * react pages this app ships. + * + * Both are DROP-SHAPED — nothing throws, nothing warns, and the page renders a + * plausible number either way, which is why neither `os validate` nor + * `tsc` nor a smoke test catches them: + * + * 1. `QueryParams` (objectui `packages/types/src/data.ts`) declares ONLY + * `$`-prefixed keys, and `ObjectStackAdapter.convertQueryParams` (objectui + * `packages/data-objectstack/src/index.ts`) builds its outgoing options by + * copying exactly those. A bare `top:` / `limit:` reaches no branch and is + * dropped. It does not fall back to a default page: the GET list route has + * no default page size (`packages/client/src/index.ts`, and pinned in + * `packages/client/src/client.test.ts`), so an absent `top` returns the + * ENTIRE match set — the cap the author wrote simply never happens. + * 2. `find()` resolves to a normalized `QueryResult` — rows under `data`, + * never the REST envelope's `records`, plus `total`. Reading `.records` + * yields `undefined` on every call, so a KPI over it sticks at 0 forever + * while the `` beside it shows the same rows correctly. + * + * `total` is the third half of the same contract: with a `$top` applied the + * server runs a real count over the same filter + * (`ObjectStackProtocolImplementation.findData`), so a KPI that counts + * `data.length` under a cap under-reports silently. Fixing (1) without reading + * `total` therefore CREATES the capped-count defect it was meant to remove. + */ + +// --------------------------------------------------------------------------- +// A contract-faithful `ObjectStackAdapter` double +// --------------------------------------------------------------------------- + +type Row = Record; + +/** + * Mirrors the three behaviours above, and nothing else. Deliberately literal: + * it reads ONLY `$`-prefixed keys (so an unprefixed one is dropped exactly as + * the real adapter drops it) and returns the `QueryResult` shape. + */ +function makeAdapterDouble(store: Record) { + const seen: Array<{ resource: string; params: Record }> = []; + + const matches = (row: Row, filter: unknown): boolean => { + if (!Array.isArray(filter)) return true; + const [field, op, value] = filter as [string, string, unknown]; + if (op !== '=') throw new Error(`double does not implement operator '${op}'`); + return row[field] === value; + }; + + return { + seen, + find(resource: string, params: Record = {}) { + seen.push({ resource, params }); + const all = (store[resource] ?? []).filter((r) => matches(r, params.$filter)); + // ONLY the `$` spelling is read — this is the drop under test. + const limit = typeof params.$top === 'number' ? params.$top : undefined; + const data = limit === undefined ? all : all.slice(0, limit); + // No limit → the whole match set came back, so its length IS the total. + // With a limit → the server counts over the same filter. + const total = limit === undefined ? data.length : all.length; + return Promise.resolve({ + data, + total, + page: 1, + pageSize: limit, + hasMore: data.length < total, + }); + }, + }; +} + +// --------------------------------------------------------------------------- +// Running the page's REAL rollup effect +// --------------------------------------------------------------------------- + +/** Slice the balanced `(...)` that starts at `from` (which must be `(`). */ +function balancedCall(src: string, from: number): string { + let depth = 0; + for (let i = from; i < src.length; i++) { + if (src[i] === '(') depth++; + else if (src[i] === ')') { + depth--; + if (depth === 0) return src.slice(from, i + 1); + } + } + throw new Error('unbalanced call expression'); +} + +/** Lift the `React.useEffect(...)` rollup out of the page source, verbatim. */ +function extractRollupEffect(source: string): string { + const anchor = 'React.useEffect('; + const at = source.indexOf(anchor); + if (at < 0) throw new Error('no React.useEffect in page source'); + return `React.useEffect${balancedCall(source, at + anchor.length - 1)};`; +} + +interface Rollup { + projects: number; + invoices: number; + openInvoices: number; + capped?: boolean; +} + +/** Execute the extracted effect with a stub React and the adapter double. */ +async function runRollup( + effectSource: string, + adapter: ReturnType, + sel: string | null, +): Promise { + let settle!: (v: Rollup) => void; + const done = new Promise((res) => { settle = res; }); + const React = { useEffect: (cb: () => unknown) => { cb(); } }; + // eslint-disable-next-line @typescript-eslint/no-implied-eval, no-new-func + const run = new Function('React', 'adapter', 'sel', 'reload', 'setRelated', effectSource) as ( + React: unknown, adapter: unknown, sel: unknown, reload: unknown, setRelated: (v: Rollup) => void, + ) => void; + run(React, adapter, sel, 0, settle); + return done; +} + +// --------------------------------------------------------------------------- + +describe('renewals-pipeline hand-rolled rollup — the adapter contract, executed', () => { + const effect = extractRollupEffect(RenewalsPipelinePage.source as string); + + it('lifted the real effect out of the page source (extraction control)', () => { + // Guards the harness itself: if the anchor ever stops matching, every + // assertion below would pass over an empty string. + expect(effect).toContain("adapter.find('showcase_project'"); + expect(effect).toContain("adapter.find('showcase_invoice'"); + expect(effect).toContain('setRelated'); + }); + + /** 640 projects and 640 invoices on one account — more than the page's cap. */ + function bigAccount() { + const projects: Row[] = []; + const invoices: Row[] = []; + for (let i = 0; i < 640; i++) { + projects.push({ id: `p${i}`, account: 'acc_1' }); + // 128 of the 640 invoices are open (every 5th). + invoices.push({ id: `i${i}`, account: 'acc_1', status: i % 5 === 0 ? 'open' : 'paid' }); + } + // A second account that must never be counted into the first one's KPIs. + projects.push({ id: 'p_other', account: 'acc_2' }); + invoices.push({ id: 'i_other', account: 'acc_2', status: 'open' }); + return { showcase_project: projects, showcase_invoice: invoices }; + } + + it('applies a real cap — the $top the author wrote reaches the adapter', async () => { + const adapter = makeAdapterDouble(bigAccount()); + await runRollup(effect, adapter, 'acc_1'); + + expect(adapter.seen).toHaveLength(2); + for (const call of adapter.seen) { + // The defect: an unprefixed key is silently ignored, so the read runs + // unbounded. Every key the page sends must be one the adapter reads. + expect(Object.keys(call.params).every((k) => k.startsWith('$'))).toBe(true); + expect(call.params.$top).toBe(500); + } + }); + + it('reports the account\'s true totals, not the page length under the cap', async () => { + const adapter = makeAdapterDouble(bigAccount()); + const rollup = await runRollup(effect, adapter, 'acc_1'); + + // 640, not 500 (the cap) and not 0 (the `.records` read) and not 641 + // (the other account's row leaking past `$filter`). + expect(rollup.projects).toBe(640); + expect(rollup.invoices).toBe(640); + // Open AR is a per-row verdict over the fetched window, so it IS bounded by + // the cap — 100 of the first 500, not the 128 that exist. The page must say + // so rather than presenting it as a total. + expect(rollup.openInvoices).toBe(100); + expect(rollup.capped).toBe(true); + }); + + it('is exact, and not capped, for an account inside the window', async () => { + const store = { + showcase_project: [ + { id: 'p1', account: 'acc_1' }, + { id: 'p2', account: 'acc_1' }, + { id: 'p3', account: 'acc_2' }, + ], + showcase_invoice: [ + { id: 'i1', account: 'acc_1', status: 'open' }, + { id: 'i2', account: 'acc_1', status: 'paid' }, + { id: 'i3', account: 'acc_1', status: 'void' }, + ], + }; + const rollup = await runRollup(effect, makeAdapterDouble(store), 'acc_1'); + expect(rollup).toMatchObject({ projects: 2, invoices: 3, openInvoices: 1, capped: false }); + }); + + it('zeroes the strip when no account is selected', async () => { + const rollup = await runRollup(effect, makeAdapterDouble({}), null); + expect(rollup).toMatchObject({ projects: 0, invoices: 0, openInvoices: 0 }); + }); +}); + +// --------------------------------------------------------------------------- +// The same two contracts, swept across every react page this app ships +// --------------------------------------------------------------------------- + +const DECLARED_QUERY_PARAM_PREFIX = '$'; + +/** Top-level keys of an object-literal source slice. */ +function topLevelKeys(objSrc: string): string[] { + const keys: string[] = []; + let depth = 0; + let i = 0; + let expectKey = true; + while (i < objSrc.length) { + const c = objSrc[i]; + if (c === '{' || c === '[' || c === '(') { depth++; i++; continue; } + if (c === '}' || c === ']' || c === ')') { depth--; i++; continue; } + if (depth === 1) { + if (c === ',') { expectKey = true; i++; continue; } + if (c === ':') { expectKey = false; i++; continue; } + if (expectKey) { + const m = /^(['"]?)([A-Za-z_$][\w$]*)\1\s*:/.exec(objSrc.slice(i)); + if (m) { keys.push(m[2]); i += m[0].length; expectKey = false; continue; } + } + } + i++; + } + return keys; +} + +interface QueryFinding { key: string; snippet: string } + +/** Every unprefixed key handed to an `adapter.find`/`findOne` in one source. */ +function unprefixedQueryKeys(source: string): QueryFinding[] { + const found: QueryFinding[] = []; + const call = /\b(?:adapter|dataSource)\s*\.\s*(?:find|findOne)\s*\(/g; + let m: RegExpExecArray | null; + while ((m = call.exec(source))) { + // Walk to the params object literal, staying inside this call's parens. + let i = m.index + m[0].length; + let depth = 1; + let objStart = -1; + while (i < source.length && depth > 0) { + const c = source[i]; + if (c === '(') depth++; + else if (c === ')') { depth--; if (depth === 0) break; } + else if (c === '{' && depth === 1) { objStart = i; break; } + i++; + } + if (objStart < 0) continue; + let braces = 0; + let objEnd = -1; + for (let j = objStart; j < source.length; j++) { + if (source[j] === '{') braces++; + else if (source[j] === '}') { braces--; if (braces === 0) { objEnd = j; break; } } + } + if (objEnd < 0) continue; + const obj = source.slice(objStart, objEnd + 1); + for (const k of topLevelKeys(obj)) { + if (!k.startsWith(DECLARED_QUERY_PARAM_PREFIX)) { + found.push({ key: k, snippet: obj.replace(/\s+/g, ' ').slice(0, 100) }); + } + } + } + return found; +} + +/** + * A `.records` read with no `.data` beside it, off a find() result. + * + * Comment lines are skipped: a page that explains the trap in prose (and + * `crm-workbench` does, right above the call it once got wrong) is documenting + * the contract, not violating it. The read itself is what this looks for. + */ +function recordsOnlyReads(source: string): string[] { + const out: string[] = []; + for (const line of source.split('\n')) { + const trimmed = line.trim(); + if (trimmed.startsWith('//') || trimmed.startsWith('*') || trimmed.startsWith('/*')) continue; + if (!trimmed.includes('.records')) continue; + if (trimmed.includes('.data')) continue; + out.push(trimmed); + } + return out; +} + +const REACT_PAGES = Object.values(pages as Record) + .filter((p): p is { name: string; kind?: string; source?: string } => + !!p && typeof p === 'object' && (p as { kind?: string }).kind === 'react') + .filter((p) => typeof p.source === 'string'); + +describe('every kind:"react" page in this app honours the useAdapter contracts', () => { + it('found the react pages to sweep (census control)', () => { + // A sweep over an empty list is vacuously green — this is what stops that. + expect(REACT_PAGES.length).toBeGreaterThanOrEqual(2); + expect(REACT_PAGES.map((p) => p.name)).toContain('showcase_renewals_pipeline'); + }); + + it('the scanners fire on a known-bad source (positive control)', () => { + const bad = ` + const a = await adapter.find('showcase_project', { $filter: ['account', '=', sel], top: 500 }); + const b = await adapter.find('showcase_invoice', { limit: 200 }); + // a comment mentioning .records must NOT count as a read + const rows = (a && a.records) || []; + `; + expect(unprefixedQueryKeys(bad).map((f) => f.key)).toEqual(['top', 'limit']); + expect(recordsOnlyReads(bad)).toEqual(['const rows = (a && a.records) || [];']); + }); + + it.each(REACT_PAGES.map((p) => [p.name, p.source as string] as const))( + '%s passes only $-prefixed query options', + (_name, source) => { + expect(unprefixedQueryKeys(source)).toEqual([]); + }, + ); + + it.each(REACT_PAGES.map((p) => [p.name, p.source as string] as const))( + '%s reads rows off QueryResult.data', + (_name, source) => { + expect(recordsOnlyReads(source)).toEqual([]); + }, + ); +});