Skip to content

fix(example-showcase): make the react rollups use the adapter's query contract - #10481

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-10288-showcase-top-query-option
Aug 21, 2026
Merged

fix(example-showcase): make the react rollups use the adapter's query contract#10481
os-zhuang merged 1 commit into
mainfrom
claude/issue-10288-showcase-top-query-option

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#10288

What the card asked for, and what turned out to be true

The card is right that top is not 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 a bare top: reaches no branch and is dropped with no error.

Its consequence is the inverse of the filed one, and the correction is what shaped
this PR. The card says the queries "return the backend's default page size instead of up
to 500 rows", producing a silently capped KPI. There is no default page size. From this
repo's own sources:

So the dropped key did not truncate the read — it removed the cap. The page was fetching
every related row for the selected account, unbounded.

The KPI strip was wrong for a second, separate reason the card does not mention, and
it is the one a reader sees: the effect read its rows off pr.records. find() resolves
to a normalized QueryResultdata, total, page, pageSize, hasMore, never the
REST envelope's records — so pr.records was undefined on every call and the strip
read 0 / 0 / 0, not a capped number. That defect was already found and fixed once on
the sibling page: crm-workbench.page.ts carries the diagnosis in a comment ("Reading
.records here always missed, so the KPI cards silently stuck at 0 even though the
ListView beside them showed the same rows") and was still live here.

The harm, reproduced

The real page source, lifted out of the page file and executed against a
contract-faithful adapter double, over an account holding 640 projects and 640 invoices
(128 of them open):

── BEFORE (origin/main) ──
find(showcase_project) params = {"$filter":["account","=","acc_1"],"top":500}
-> $top seen by the adapter: UNDEFINED (no cap applied — the whole match set comes back)
find(showcase_invoice) params = {"$filter":["account","=","acc_1"],"top":500}
-> $top seen by the adapter: UNDEFINED (no cap applied — the whole match set comes back)
KPI strip: {"projects":0,"invoices":0,"openInvoices":0}
── AFTER (this branch) ──
find(showcase_project) params = {"$filter":["account","=","acc_1"],"$top":500}
-> $top seen by the adapter: 500
find(showcase_invoice) params = {"$filter":["account","=","acc_1"],"$top":500}
-> $top seen by the adapter: 500
KPI strip: {"projects":640,"invoices":640,"openInvoices":100,"capped":true}

Why this is not a two-character edit

$top: 500 alone would have created the defect the card describes. With a cap
applied, data.length is a page length, so the "Projects" and "Invoices" KPIs would have
silently reported 500 for every account above the cap — a capped count reading as a real
number, which is the card's own words for the harm. Both pages therefore count the
envelope's total, which with a limit present is the server's real count over the same
$filter (ObjectStackProtocolImplementation.findData runs engine.count on that
path).

"Open AR" is a per-row verdict and genuinely cannot be exact over a capped window — it is
the one number the cap really does bound. It renders as 100+ when the window was
truncated rather than passing for a total.

Changed

  • examples/app-showcase/src/ui/pages/renewals-pipeline.page.ts$top on both calls,
    rows off .data, counts off total, capped surfaced in the Open AR stat, and a
    numbered note on the effect stating both contracts (this page is the copy-paste
    surface, so the comment is load-bearing).
  • examples/app-showcase/src/ui/pages/crm-workbench.page.ts — same class, found by the
    sweep: { limit: 200 }{ $top: 200 }, headline count off total.
  • examples/app-showcase/test/react-page-adapter-query-contract.test.ts — new.

Sweep census

adapter.find / dataSource.find call sites, params-object keys extracted by brace
matching, classified by whether the file's adapter comes from useAdapter() (the
objectui DataAdapter) or from something else.

Positive control: the scanner was run first against origin/main and reported the
known-present renewals-pipeline.page.ts:67 and :68top: 500 before it was used to
claim anything absent.

In this repo, 3 unprefixed-key sites in 2 files, both under examples/**, both fixed
here:

filekey
examples/app-showcase/src/ui/pages/renewals-pipeline.page.ts:67top
examples/app-showcase/src/ui/pages/renewals-pipeline.page.ts:68top
examples/app-showcase/src/ui/pages/crm-workbench.page.ts:39limit

21 further key hits across 5 files are a differentadapter: plugin-auth's
better-auth adapter (findOne({ model, where }), 4 files) and a PostgresAdapter in
content/blog/protocol-first-development.mdx. Neither is the objectui DataAdapter;
model/where/field are correct there.

.records reads off a find() result, same sweep: 4 sites — the two repaired here, one
benign (crm-workbench already read .data first), and content/docs/ui/react-pages.mdx:147,
which is reported separately (below).

Sweeping objectui at c40f3b8 found 4 more unprefixed-key sites plus two
records-only reads. Outside this repo's file surface — reported, not touched.

The gate question: not built here, and the measurement

Ruling: no new os validate rule or check:* gate in this repo. A scoped regression
pin instead.

What a gate would have to cover, measured rather than guessed:

  • Exposure in this repo: 3 sites, 2 files, both in examples/**, 0 after this PR.
    There are exactly 3 kind:'react' pages in examples/** and one useAdapter sample
    in content/docs/**. That is the entire population an objectstack-side checker could
    ever read, because a react page's source is a string — ESLint cannot see into it, and
    tsc cannot either.
  • Recurrence is real, and it is not in this repo. The class has five measured
    instances by different authors: object-timeline (objectui#4009 / objectstack#7137),
    object-kanban (objectui#4025), crm-workbench's .records (repaired in place,
    comment still there), renewals-pipeline (this card), and the four live objectui sites
    the sweep just found. Four of the five live in objectui.
  • objectui already gates the sibling half.eslint-rules/no-query-params-under-options.js
    bans a $-prefixed key nested under options, and its header makes the case for
    mechanising this family: the mistake type-checks, it publishes, and "a review catches it
    once and then misses the next one." It does not look at a bare top-level key — that is
    the half with the live sites.
  • The decisive cost.QueryParams is objectui's contract. A checker in objectstack
    would have to hard-code another repo's key list and become a second source of truth for
    it — the exact second-de-facto-contract shape Prime Directive Add comprehensive test suite for Zod schema validation #12 warns about, on a
    contract this repo does not own and cannot keep in step. The rule belongs beside the
    type it enforces, next to the rule that already gates the other half. Reported as
    [objectui] four live adapter.find calls pass an unprefixed query option — and no-query-params-under-options gates only the sibling half of the class #10470 with the census and a suggested rule shape.

What is built instead, at ~50 lines and no new infrastructure:
test/react-page-adapter-query-contract.test.ts sweeps everykind:'react' page this
app ships for both contracts. It needs no key table — the assertion is the prefix rule
itself ("every key the page sends starts with $") plus "rows come off .data" — so it
duplicates no contract and closes the whole class for this repo's file surface. It carries
three controls: an extraction control (the lifted effect really contains both find
calls), a census control (the page list is non-empty and contains the renewals page), and
a positive control (both scanners fire on a known-bad synthetic source, and the
comment-skip does not silence a real read).

Reported separately, out of scope here

Ablations

The fix was committed first, then broken deliberately, each leg confirmed on disk by
counting the removed anchor and the injected text (a first attempt with perl -0pi
matched nothing and produced $toptop: — caught by that count, not by an exit code).

  • Leg A, $top: 500top: 500 (anchor 2 → 0, injection 0 → 2): 3 tests red
    expected false to be true on the all-keys-$-prefixed assertion, expected 128 to be 100 on Open AR (the unbounded read returning all 128 open invoices instead of the
    honest 100 inside the window), and the page sweep reporting {key: 'top', snippet: "{ $filter: […], top: 500 }"}. Note the direction: the ablation shows a missing cap,
    not a truncation.
  • Leg B, (res && res.data)(res && res.records) (anchor 1 → 0, injection 0 → 1):
    3 tests redexpected +0 to be 100, the in-window case losing capped: false,
    and the reads rows off QueryResult.data sweep firing.
  • Restored: git diff HEAD empty, tree byte-identical to the fix commit.

Verification

Gates re-derived from the real diff with node scripts/pm/dispatch-gates.mjs (no paths
passed — it takes its own change set from the merge base), run at fde5f1e:

gateverdict line
check:nul-bytesOK (scanned 6140 text file(s) … no raw ASCII control bytes)
check:changeset-gate-self-tests118 + 212 + 116 self-test assertions pass
check:objectui-changesetobjectui-range --self-test: all checks passed
check-adr-0087-registration.mjsthis PR adds no declared-breaking changeset
check-changeset-no-major.mjsThis diff introduces no 'major' bump
check-empty-changeset.mjsNo empty-frontmatter changeset introduced by this diff (1 declaring changeset(s) added)
check:cross-package-test-inputsOK: 12 package(s) read outside themselves, all declared
check:engine-double-contractOK — 340 pinned, 133 in the DEBT ledger, 2 exempt
check:where-matcher266 matcher(s) discovered … none new
check:query-options-erasureratchet holds: 67 unswept non-test site(s) … none new; test surface 240 site(s) in 47 file(s) — at the ceiling
check:type-check-coverageOK — 64/77 workspace packages type-checked
check:type-check-debt--re-measure: OK — 33 ledger entr(ies) re-measured in 352.5s, 1924 raw tsc error(s) total, none above its recorded number — full closure built first, exactly as lint.yml does

Package-level, at the same commit:

  • pnpm --filter @objectstack/example-showcase testTest Files 23 passed (23),
    Tests 361 passed (361).
  • pnpm --filter @objectstack/example-showcase typecheck — clean (this package's
    tsconfig.json includes test/**/*, so the new test file is in the program).
  • pnpm --filter @objectstack/example-showcase validate — exit 0, no new findings,
    confirming the card's point that os validate cannot see this class.

Changeset

examples/app-showcase is private: true, but this repo's .changeset/config.json sets
privatePackages: { version: true, tag: false }, so it is versioned and carries its own
CHANGELOG.md — and prior showcase-only PRs wrote real changesets for it. A changeset,
not the skip-changeset label.


Generated by Claude Code

… 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 `<ListView>`
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt
@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

PM review — ⭐ accepted. The premise correction is the whole value here, and it changed the fix.

Reviewed against the diff.

⭐ You inverted the card's consequence, and the inversion mattered

The card said a dropped top returns "the backend's default page size instead of up to 500 rows" — under-reporting from truncation. Measured: there is no default page size on the GET list route (pinned in client.test.ts via #6485; corroborated by objectql's "baseline — no params returns every row"). So the dropped key meant the read ran completely unbounded — the cap never happened at all. Opposite failure, opposite risk profile.

And you found a second defect the card never mentions: the effect read rows off pr.records, but find() resolves to a normalized QueryResult (data + total, never the REST envelope), so the KPI strip read 0/0/0 permanently. The card's headline symptom was real but not caused by what the card said caused it.

⭐⭐ The judgement that makes this PR right rather than merely correct

Fixing only $top would have CREATED the card's described harm, because data.length under a cap is exactly a silently capped count.

That is the finding. The obvious two-character fix — top$top — would have introduced the truncation the card thought it was reporting, and it would have looked like a clean close. Instead both pages count the envelope's total (the server's real count over the same $filter when a limit applies), and Open AR renders as 100+ when the window was truncated rather than passing a capped number off as a total.

⭐ And the ablation observed the direction rather than assuming it: flipping $top back to top made the number go up — 128 against 100 — which is a missing cap, not a truncation. That single observation is what proves the premise correction rather than merely asserting it.

What I verified

  • Not a one-key patch.crm-workbench's {limit: 200}{$top: 200}and its total handling; renewals-pipeline both call sites and.records.data + total.
  • The changeset is real and names the package, and ⭐ your reasoning got there against an intuitive wrong answer: examples/app-showcase is private: true, which reads like skip-changeset — but .changeset/config.json sets privatePackages {version: true, tag: false}, so it is versioned and carries its own CHANGELOG, and prior showcase-only PRs wrote real changesets. Judged at diff time against the config, not assumed from private: true. That is exactly the discipline the brief asked for, on the one case where the default would have been wrong.
  • Harm reproduced before the fix was believed — 640/640/128 fixture, before {0,0,0} with $top: undefined, after {640,640,100,capped:true}.
  • Census carried a positive control — the scanner was run against origin/main first and made to report the two known-present sites before any absence claim.
  • ✅ In-source comments now state the contract at the call sites, so the next copy-paste from this golden example carries the rule with it.

⭐ The gate ruling is the right call, for the right reason

"No new gate in objectstack" backed by four measurements, and the decisive one is architectural rather than economic: QueryParams is objectui's contract, so a checker here would hard-code another repo's key list and become a second source of truth for it — the Prime Directive #12 shape — on a contract this repo neither owns nor can keep in step with. Building a ~50-line test that needs no key table at all (its assertions are the prefix rule itself plus "rows come off .data") sidesteps that entirely. Noting honestly that 4 of 5 measured recurrences live in objectui, and that objectui already gates the sibling half, is what makes the ruling checkable.

⚠️ A pattern worth naming — two of my devs hit the same trap tonight

Your first perl -0pi attempt matched nothing and produced $toptop:, caught by your on-disk count and not by an exit code. #10466's dev hit the neighbouring form of it — perl -0pi interpolating $/, which under -0 is a NUL, silently turning a file binary while exiting 0. Different mechanism, same lesson, same night: an in-place editor's exit code is not evidence that it edited anything. Both of you caught it only because you count the anchor on disk before reading a verdict. That habit is doing real work.

⚠️ Also noted: the deriver named 12 gate families here. My briefs keep guessing narrower than reality; the hypothesis framing exists for exactly that reason.

Follow-ups, all correctly filed rather than absorbed

#10469 (the react-pages.mdx sample reads result.records and its Callout repeats the "default page size" claim that this PR just disproved) — and you named why you didn't repair it in place: content/docs/** pulls a different gate family, failing condition 4 of the bounded in-place exemption, plus the Callout needs a wording decision. #10470 (four live objectui sites, including {limit: 0} where dropping it inverts "no rows" into "every row" — the worst of the set) with a concrete proposal to extend objectui's existing rule.

Nothing for you to change. CI is finishing; I will flip ready and arm once it is green.


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review August 21, 2026 00:51
@os-zhuang
os-zhuang added this pull request to the merge queueAug 21, 2026
Merged via the queue into main with commit 6cca75cAug 21, 2026
27 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-10288-showcase-top-query-option branch August 21, 2026 01:39
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@os-zhuang@claude